Review Request #121448 - Created Dec. 11, 2014 and discarded
This module, which has been migrated from the related KDE4 macto kde4_app_app_icon,
supports platform specific application icon for Windows and Mac OSX.
On Windows this function depends on the external tool png2ico, which is
provided by the kdewin-tools binary package. Sources are available at
I wouldn't take WINCE into account, its support hasn't been ported into kf5.
Also missing indentation.
Regarding patterns, why not just using a list of the icons we need? patterns+cmake have weird effects...
Would it be possible to include a test? It's reasonably easy to create ecm tests nowadays and very useful afterwards. You'll see some examples in the repository.
By the way, thanks for looking into this, really needed!
- remove WINCE
- add additional example
- add png2ico requirement in notes
- fix parent scope issue
The assumptions this funciton makes about the form of the arguments it is passed and the form of the file names need to be made explicit in the documentation, otherwise I can't judge whether the code is correct.
I've made some suggestions for improvements, but I would actually recommend ignoring those and redoing the calling style completely to match how ecm_install_icons works - ie: you expect the files to be explicitly listed, but with the filenames in a certain form (the same form as for ecm_install_icons, but maybe with less constraints - just that the size is at the start followed by a hyphen, say), so you can extract the icon size easily. The syntax would then beecm_add_app_icons(<sources_var> ICONS <icon> [<icon> [...]])
This has the dual advantage of behaving similarly to ecm_install_icons (predictability) and allowing the benefits of explicitly listing the icons in the CMakeLists.txt file without the drawbacks of having to manually exclude certain sizes on Windows.
I would rather have the syntax be something like
ecm_add_app_icon(<sources_var> GLOBS pat [pat [...]])
or maybe even
ecm_add_app_icon(<sources_var> [GLOBS <pat> [<pat> [...]]] [FILES <file> [<file> [...]]])
where FILES arguments would not be globbed, but GLOBS would be. You could use PATTERNS if you don't like GLOBS - I was going for similarity with the file(GLOB) command, since that's what's ultimately used.
Having keyword arguments makes calls clearer, and gives greater scope for future changes to the argument list.
You say regular expression, but it's actually a very restrained glob pattern - you have to put a * where the icon size would go.
Would this actually work? The code looks to me like pattern_rx would just be the filename again, so fn would be empty, and the icon would never be appended to _list.
- removed doc about unsopported normal file mode
- fixed 'pattern' wording
Ralf, Can you update the diff rather than uploading the file? It's much easier to apply a diff than to figure out where to put this file and if any other changes are needed to use it, etc.
So, I took your latest upload as a basis and wrote: https://git.reviewboard.kde.org/r/122135/
This makes the usage what I was trying to get at with my earlier reviews, and also makes it work with Matthias Benkmann's png2ico tool (which is what people will find if they just do a web search for "png2ico"); this is split into a separate find module. I've improved the documentation as well, and refactored the code so that more is run on all platforms (which makes debugging easier for users of the module and for developers of e-c-m). It is, however, completely untested for now.