cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Axel Nennker <ignisvul...@gmail.com>
Subject Re: Started Implementing CB-2606 Add support for <icon in config.xml
Date Fri, 20 Dec 2013 16:27:19 GMT
inline.

Do you know why the tests fail? They look ok for me but those are the first
jasmine tests I ever wrote.
BTW: the note in cordova-cli's README.md on test seems a little short "npm
test". I suggest to be a little more verbose here:
- clone mobile-spec e.g.: git clone
https://github.com/apache/cordova-mobile-spec
- clone coho e.g.: git clone https://github.com/apache/cordova-coho
- some more stuff I already forgot like "npm install" in some dir

Who could work on the other platforms? src/metadata/<platform>_parser.js
I think CB-2606 should only go live if all current platforms are supported.

2013/12/20 Andrew Grieve <agrieve@chromium.org>

> 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.
>
AN: no

>   - 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.
>
AN: no

>
> - 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?
>
AN: one icon element should be enough for all platforms. On Android this
would be copied to res/drawable the other res/drawable-Xdpi stay empty

>   - 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
>
AN: OK

>     - 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.
>
AN: The question is whether cordova wants to use the W3C widget definition
http://www.w3.org/TR/widgets/#the-icon-element-and-its-attributes
I do not know how IOS handles this or whether we can come up with a scheme
that handles all currently supported platforms.
As a developer I usually know which platforms I support and would let my
designer the icons for each platform and device category I support. I would
then specify in the icons element exactly which icon goes where depending
on platform.
So the need for a common scheme is not really "real world".

>
> - 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/)
>
AN: Currently icons are in <project_root>/res/icon/<platform>/
https://github.com/AxelNennker/cordova-cli/blob/master/src/metadata/android_parser.js#L75

>
> - 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?
>
AN: I would copy them over although I do not have a use case. I would
remove icons for other platforms.

>
> - 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.
>
AN: cdv is currently the prefix used in HelloWorld's config.xml
I would prefer to extend the W3C widget definition as in my example
config.xml using the cdv prefix.
When/If cordova moves to www/config.json and drops the W3C widget then it
is early enough.
Phonegap uses the gap prefix for their namespace and I think cordova should
have a namespace too until this major change to json.

>
> - 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.
>
AN: I would not ditch the W3C widget syntax now.

>
>
> Code-wise nits:
> - Try and use camelCase for vars & properties, CamelCase for class names
> (icon->Icon).
>
AN: I just copied the definition for preference and modified it. I would
like to stay consistent inside one file.


> - 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.
>
AN: e.g.: Google Chrome barfs on e.g. "if(window.nonExistingProperty) ..."
with TypeError of ReferenceException or something.
"'property' in Obj" always works.
I can remove that if-statement anyway because config_parser and
android_parser are commited simultaniously anyway and the config_parser
defines "config.icon" anyway.


> - Some comments would be helpful. Especially in places where they address
> the "why", or where the variable names don't say much.
>
AN: right... but: config_parser.js' class definition for icon is mostly a
copy of preference. I admit that the use case for get(id) is missing.
To use case for "get(src)" does not make much sense because the src is not
necessarily unique (unlike <preference name).
Will add some comments to android_parser.js' icon handling code.




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