cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: Started Implementing CB-2606 Add support for <icon in config.xml
Date Fri, 20 Dec 2013 15:18:20 GMT
Had a quick look.

Thanks for taking this on! It's truly an overdue feature.

I've got a bunch of comments below, most of which I'm hoping others will
chime in on as well because they are design opinions more than technical
questions.

- Should CLI be in the business of resizing icons or converting image
formats?
  - I think the answer here should be no, but it's good to be explicit
about it.
  - If users want to have auto-generation of .pngs for .svgs, then they
should use their own build tool to do it, and CLI will work with the result.

- Should you have to supply an icon for each platform separately, or should
we support one <icon> tag that will be used as the default for all
platforms?
  - This is also meant to address your point about needing to specify all
densities for android to clobber the template ones.
  - What I think:
    - If an <icon> exists, delete all existing icons from within the
platforms
    - Specifying the platform should not be the norm. It should be enough
to have a bunch if <icon src=foo density=bar>, and platforms will pick up
the sizes of icons that they require.

- How should we encode icon densities?
  - We should require that all <icon> tags have the image dimensions
included in them
  - I think "size" makes more sense than "density"
  - We could support both 999x999 as well as *dpi (which will just map to a
pixel size).

- Paths should be relative to project root (not www/)

- Should we copy the <icon> tags into the platform-level config.xml?
  - I don't think platforms could make use of them, so why copy them?

- I don't think we're using "cdv:" prefix on any other xml attributes.
  - I realize this is "proper" XML, since they aren't a part of the widget
XML namespace, but in practice I don't think it matters, and I think it's a
bit ugly. Just my opinion.

- For plugin.xml, we have:
<platform name="android">
   ...
</platform>

Should we use the same syntax here instead of a platform attribute on the
tags?
  - I think using <platform> would help in our goal to have plugin.xml and
config.xml converge.


Code-wise nits:
- Try and use camelCase for vars & properties, CamelCase for class names
(icon->Icon).
- Instead of "icon" in config, use if (config.icon), since it's not
uncommon to assume missing is the same as being set to null / undefined.
- Some comments would be helpful. Especially in places where they address
the "why", or where the variable names don't say much.






On Fri, Dec 20, 2013 at 8:56 AM, Axel Nennker <ignisvulpis@gmail.com> wrote:

> Hi,
>
> I started to implement CB-2606
> https://github.com/AxelNennker/cordova-cli
>
> The code is here:
> https://github.com/AxelNennker/cordova-cli
>
> This
> https://github.com/AxelNennker/cordova-cli/blob/master/src/config_parser.js
> changes the config_parser to support the icon element.
>
> On Android using this config_parser might be used like this:
>
> https://github.com/AxelNennker/cordova-cli/blob/master/src/metadata/android_parser.js
> Currently the src attribute from the icon element in config.xml is relative
> to www.
>  <?xml version='1.0' encoding='utf-8'?>
> <widget id="com.example.hello" version="0.0.1" xmlns="
> http://www.w3.org/ns/widgets" xmlns:cdv="http://cordova.apache.org/ns/1.0
> ">
>     <name>HelloWorld</name>
>     <description>
>         A sample Apache Cordova application that responds to the
> deviceready event.
>     </description>
>     <author email="dev@cordova.apache.org" href="http://cordova.io">
>         Apache Cordova Team
>     </author>
>     <content src="index.html" />
>     <access origin="*" />
>     <icon src="icon.png" cdv:platform="android"/>
>     <icon src="icon.png" cdv:platform="android" cdv:density="ldpi"/>
>     <icon src="icon.png" cdv:platform="android" cdv:density="mdpi"/>
>     <icon src="icon.png" cdv:platform="android" cdv:density="hdpi"/>
>     <icon src="icon.png" cdv:platform="android" cdv:density="xhdpi"/>
>     <preference name="test" value="android"/>
> </widget>
>
> I started to write some tests but some fail although I don't know why. It
> works with Cordovas HelloWorld and the new config.xml anyway. I need help
> with the tests.
>
> cheers
> Axel
>
> ps: It is a little strange that I have to specify all densities to
> overwrite the default icon. Maybe the default icon should not be there in
> the first place?
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message