> On Dez. 12, 2014, 3:08 nachm., Alex Merry wrote: > > modules/ECMAddAppIcon.cmake, line 15 > > <https://git.reviewboard.kde.org/r/121448/diff/1/?file=332663#file332663line15> > > > > 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. > > Ralf Habacker wrote: > The doc in the origin cmake macro file seems to be out of sync with the > official doc located in FindKDE4Internals.cmake, which is: > > --------------------------------------------------- > # adds an application icon to target source list. > # Make sure you have a 128x128 icon, or the icon won't display on Mac OS > X. > # Mac OSX notes : the application icon is added to a Mac OS X bundle so > that Finder and friends show the right thing. > # Win32 notes: the application icon(s) are compiled into the application > # There is some workaround in kde4_add_kdeinit_executable to make it > possible for those applications as well. > # Parameters: > # SRCS_VAR - specifies the list of source files > # pattern - regular expression for searching application icons > # Example: KDE4_ADD_APP_ICON( myapp_SOURCES "pics/cr*-myapp.png") > # Example: KDE4_ADD_APP_ICON( myapp_KDEINIT_SRCS > "icons/oxygen/*/apps/myapp.png") > --------------------------------------------------- > > normal file mode is not documentated and does not work > > Alex Merry wrote: > I would still much prefer an API to match ecm_add_icons, as I noted at > the start of this review. Would you be willing to do that? If not, I can > probably find some time over Christmas to look at it. > > Ralf Habacker wrote: > Just one question: As it turns out that the macro used in KDE4 only > supports a "simple pattern" style and no single files, what is different or > has been extended with KDE5 to require this now ? > > Alex Merry wrote: > Best CMake practices include listing all source files in the > CMakeLists.txt file, rather than globbing them. That way, CMake knows when a > file has been added (or removed), because the CMakeLists.txt file will > change, and so it will automatically reconfigure when you run `make`. The > icon installation function (`ecm_add_icons`) was changed to support and > encourage that approach, and I think this function should also change to > match it. > > Additionally, I would like consistency between interfaces. Having > ecm_add_app_icon behave completely differently to ecm_add_icons just makes > things that much harder for users of e-c-m. They should use the same file > naming scheme (or compatible schemes, at least), and similar arguments, in so > far as they need similar information. > > I'll admit it makes porting a little harder, but it should be possible to > create a porting script without too much difficulty. > > Jeremy Whiting wrote: > Ralf any updates on this patch? > > Ralf Habacker wrote: > @Alex, you wrote > > The icon installation function (ecm_add_icons) > > you are refering to ecm_install_icons ? > > >was changed to support and encourage that approach, and I think this > function should also change to match it. > > What could be matched ? As far as I can see only the ICONS parameter, all > other parameter do not match by design. :-( > > Merging the ICONS parameter with recent ecm_add_app_icon parameter list > will have the following syntax: > > 1. ecm_add_app_icon(<source_var> ICONS <icon> [<icon> [...]]) > > the additional mentioned syntax forms are not supported by > ecm_install_icons so we are free to choose what we like. > > According to http://www.cmake.org/cmake/help/v3.0/command/install.html > the mentioned term 'PATTERN' only supports '*' at the end of files and > therefore does not fit into the recent implementation. > > So the term 'GLOB' which is defined in the cmake file command > http://www.cmake.org/cmake/help/v3.0/command/file.html remains and fit into > what is implemented in the patch > > 2. ecm_add_app_icons(<source_var> GLOB <icon-glob-expression>) [1] > > and > > 3. ecm_add_app_icon(<source_var< <icon-glob-expression>) [1] > > because ecm_install_icons also have a compatibility mode to support old > style parameter list: > > " From > https://projects.kde.org/projects/kdesupport/extra-cmake-modules/repository/revisions/master/entry/modules/ECMInstallIcons.cmake > An old form of arguments will also be accepted:: > # ecm_install_icons(<icon_install_dir> [<l10n_code>]) > " > > syntax 3 is what is already implemented by the recent patch. > > Can we agree ? > > @Jeremey: > 1. Syntax 3 is already implemented and need only a few spelling fixed > before commit. > 2. Currently I do not have any access to a Mac/OSX development machine, > so I can only cover the windows part. Is there anyone having access to a Mac > OSX development machine ? > 3. If not I would remove the Mac OSX support for now.
Autonumbering of review board replaces the number 3. into 1. on publish in the syntax form ecm_add_app_icon(<source_var< <icon-glob-expression>) [1] which is refered in the sentence syntax 3 is what is already implemented by the recent patch. - Ralf ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review71863 ----------------------------------------------------------- On Dez. 15, 2014, 9:01 vorm., Ralf Habacker wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121448/ > ----------------------------------------------------------- > > (Updated Dez. 15, 2014, 9:01 vorm.) > > > 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 > ECMAddAppIcon.cmake > > https://git.reviewboard.kde.org/media/uploaded/files/2014/12/15/8b3e226f-a70b-4998-983a-813730a436bf__ECMAddAppIcon.cmake > ECMAddAppIcon.cmake > > https://git.reviewboard.kde.org/media/uploaded/files/2014/12/15/8433995f-b88f-426d-af54-46aba635ae1e__ECMAddAppIcon.cmake > > > Thanks, > > Ralf Habacker > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
