> On Dec. 12, 2014, 7:08 a.m., 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.
Ralf any updates on this patch? - Jeremy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review71863 ----------------------------------------------------------- On Dec. 15, 2014, 1:01 a.m., Ralf Habacker wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/121448/ > ----------------------------------------------------------- > > (Updated Dec. 15, 2014, 1:01 a.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 > 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
