Re: Review Request 121448: Introduce ECMAddAppIcon.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/ --- (Updated Jan. 24, 2015, 1:27 nachm.) Status -- This change has been discarded. 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
Re: Review Request 121448: Introduce ECMAddAppIcon.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review74272 --- 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. - Alex Merry On Dec. 15, 2014, 8: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, 8: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
Re: Review Request 121448: Introduce ECMAddAppIcon.
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? @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. - Ralf --- This is an automatically generated e-mail. To reply, visit:
Re: Review Request 121448: Introduce ECMAddAppIcon.
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
Re: Review Request 121448: Introduce ECMAddAppIcon.
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. 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
Re: Review Request 121448: Introduce ECMAddAppIcon.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review74164 --- 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. - Jeremy Whiting 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
Re: Review Request 121448: Introduce ECMAddAppIcon.
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
Re: Review Request 121448: Introduce ECMAddAppIcon.
On Dec. 12, 2014, 2:08 p.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 ? 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. - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review71863 --- On Dec. 15, 2014, 8: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, 8: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
Re: Review Request 121448: Introduce ECMAddAppIcon.
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. 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 ? - 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
Re: Review Request 121448: Introduce ECMAddAppIcon.
--- 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. Changes --- - fixed spelling error in example function name 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 (updated) 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
Re: Review Request 121448: Introduce ECMAddAppIcon.
On Dec. 12, 2014, 2:08 p.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 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. - Alex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review71863 --- On Dec. 15, 2014, 8: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, 8: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
Re: Review Request 121448: Introduce ECMAddAppIcon.
On Dez. 11, 2014, 5:02 nachm., Aleix Pol Gonzalez wrote: modules/ECMAddAppIcon.cmake, line 36 https://git.reviewboard.kde.org/r/121448/diff/1/?file=332663#file332663line36 I wouldn't take WINCE into account, its support hasn't been ported into kf5. Also missing indentation. WINCE sure, will update the patch Also missing indentation. To which area of the file are you refering ? Looking on other files in the module subdirectory I see 2 or 4 spaces indention for cmake code. The script uses 4 spaces. On Dez. 11, 2014, 5:02 nachm., Ralf Habacker wrote: 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! Regarding patterns pattern has be introduced in kde4 times because applications provides many different combinations of resolutions, which has otherwise to be maintained by hand. You cannot run simply ls hi*-app-app-name.png and paste the result into this macro because the windows icon format supports only a subset of resolutions. Umbrello for example returns hi128-app-umbrello.png hi16-app-umbrello.png hi22-app-umbrello.png hi32-app-umbrello.png hi48-app-umbrello.png hi64-app-umbrello.png from which you need to exclude 'hi22-app-umbrello.png' by hand. The script maintains this for you by using the following form: ecm_app_app_icon(myapp_SRCS pics/cr*-myapp.png) single files are also possible ecm_app_app_icon(myapp_SRCS pics/cr16-myapp.png;pics/cr32-myapp.png) Would it be possible to include a test? You mean to create a simple test app and CMakeLists.txt including this script ? sure, on windows this would require to have the png2ico tool installed for example from the kdewin-tool binary package or build from kdewin repo You'll see some examples in the repository. which repo are your refering ? extra-cmake-modules do not contain any example - Ralf --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review71815 --- On Dez. 11, 2014, 4:40 nachm., Ralf Habacker wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/ --- (Updated Dez. 11, 2014, 4:40 nachm.) 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 --- Thanks, Ralf Habacker ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121448: Introduce ECMAddAppIcon.
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121448/#review71859
---
modules/ECMAddAppIcon.cmake
https://git.reviewboard.kde.org/r/121448/#comment50079
Just recgonized that using list() do not propagate the added file to the
parent scope. I had to change this line to
set(${appsources} ${${appsources}};${_outfilename}.rc PARENT_SCOPE)
modules/ECMAddAppIcon.cmake
https://git.reviewboard.kde.org/r/121448/#comment50080
dito
set(${appsources} ${${appsources}};${_outfilename}.icns PARENT_SCOPE)
- Ralf Habacker
On Dez. 11, 2014, 4:40 nachm., Ralf Habacker wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121448/
---
(Updated Dez. 11, 2014, 4:40 nachm.)
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
---
Thanks,
Ralf Habacker
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121448: Introduce ECMAddAppIcon.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/ --- (Updated Dez. 12, 2014, 2:34 nachm.) Review request for Extra Cmake Modules, KDE Frameworks and Laurent Navet. Changes --- - remove WINCE - add additional example - add png2ico requirement in notes - fix parent scope issue 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 (updated) ECMAddAppIcon.cmake https://git.reviewboard.kde.org/media/uploaded/files/2014/12/12/a05ee2b5-64e3-4e44-ae34-4e1b7110e5f1__ECMAddAppIcon.cmake Thanks, Ralf Habacker ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121448: Introduce ECMAddAppIcon.
--- 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-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121448: Introduce ECMAddAppIcon.
On Dic. 11, 2014, 1:02 p.m., Ralf Habacker wrote: 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! Ralf Habacker wrote: Regarding patterns pattern has be introduced in kde4 times because applications provides many different combinations of resolutions, which has otherwise to be maintained by hand. You cannot run simply ls hi*-app-app-name.png and paste the result into this macro because the windows icon format supports only a subset of resolutions. Umbrello for example returns hi128-app-umbrello.png hi16-app-umbrello.png hi22-app-umbrello.png hi32-app-umbrello.png hi48-app-umbrello.png hi64-app-umbrello.png from which you need to exclude 'hi22-app-umbrello.png' by hand. The script maintains this for you by using the following form: ecm_app_app_icon(myapp_SRCS pics/cr*-myapp.png) single files are also possible ecm_app_app_icon(myapp_SRCS pics/cr16-myapp.png;pics/cr32-myapp.png) Would it be possible to include a test? You mean to create a simple test app and CMakeLists.txt including this script ? sure, on windows this would require to have the png2ico tool installed for example from the kdewin-tool binary package or build from kdewin repo You'll see some examples in the repository. which repo are your refering ? extra-cmake-modules do not contain any example As far as I know, the Windows .ico format supports arbitrary resolutions. The limitations come from png2ico. - Nicolás --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review71815 --- On Dic. 12, 2014, 10:34 a.m., Ralf Habacker wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/ --- (Updated Dic. 12, 2014, 10:34 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 Thanks, Ralf Habacker ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121448: Introduce ECMAddAppIcon.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/ --- (Updated Dez. 11, 2014, 4:40 nachm.) 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 --- Thanks, Ralf Habacker ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 121448: Introduce ECMAddAppIcon.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/#review71815 --- modules/ECMAddAppIcon.cmake https://git.reviewboard.kde.org/r/121448/#comment50051 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! - Aleix Pol Gonzalez On Dec. 11, 2014, 3:40 p.m., Ralf Habacker wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121448/ --- (Updated Dec. 11, 2014, 3:40 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 --- Thanks, Ralf Habacker ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
