----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review71863 -----------------------------------------------------------
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](http://api.kde.org/ecm/module/ECMInstallIcons.html) 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 be ecm_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. modules/ECMAddAppIcon.cmake <https://git.reviewboard.kde.org/r/121448/#comment50083> 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. modules/ECMAddAppIcon.cmake <https://git.reviewboard.kde.org/r/121448/#comment50082> You say regular expression, but it's actually a very restrained glob pattern - you have to put a * where the icon size would go. modules/ECMAddAppIcon.cmake <https://git.reviewboard.kde.org/r/121448/#comment50085> 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. - Alex Merry On Dec. 12, 2014, 1:34 p.m., Ralf Habacker wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121448/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2014, 1:34 p.m.) > > > Review request for Extra Cmake Modules, KDE Frameworks and Laurent Navet. > > > Repository: extra-cmake-modules > > > Description > ------- > > 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 > https://projects.kde.org/projects/kdesupport/kdewin. > > > Diffs > ----- > > modules/ECMAddAppIcon.cmake PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/121448/diff/ > > > Testing > ------- > > > File Attachments > ---------------- > > ECMAddAppIcon.cmake > > https://git.reviewboard.kde.org/media/uploaded/files/2014/12/12/a05ee2b5-64e3-4e44-ae34-4e1b7110e5f1__ECMAddAppIcon.cmake > > > Thanks, > > Ralf Habacker > >
_______________________________________________ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem
