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 <agri...@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 <ignisvul...@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? >
