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 <[email protected]> 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="[email protected]" 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?
>
