From kde-buildsystem Sat Jan 24 10:53:00 2015 From: "Ralf Habacker" Date: Sat, 24 Jan 2015 10:53:00 +0000 To: kde-buildsystem Subject: Re: Review Request 122135: Add ecm_add_app_icon function. Message-Id: <20150124105300.32443.33240 () probe ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-buildsystem&m=142209680617119 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============4777325970564750661==" --===============4777325970564750661== Content-Type: multipart/alternative; boundary="===============6360936761880139290==" --===============6360936761880139290== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122135/#review74653 ----------------------------------------------------------- Ship it! modules/ECMAddAppIcon.cmake pixel size 22 is not used in the code and should be removed here modules/ECMAddAppIcon.cmake trailing whitespace modules/ECMAddAppIcon.cmake trailing whitespace modules/ECMAddAppIcon.cmake This should also be ${CMAKE_CURRENT_SOURCE_DIR} windows support otherwise looks good. - Ralf Habacker On Jan. 24, 2015, 11:11 vorm., Alex Merry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/122135/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2015, 11:11 vorm.) > > > Review request for Extra Cmake Modules, Ralf Habacker and Jeremy Whiting. > > > Repository: extra-cmake-modules > > > Description > ------- > > This adds an application icon to an executable from PNG files for > Windows and Mac OS X. Unlike the similar kde4_add_app_icon macro from > kdelibs, this requires icons to be explicitly listed as arguments > (meaning CMake can tell when ones are added or deleted and reconfigure > as appropriate), and it works with Matthias Benkmann's png2ico tool, as > well as the KDE-Win tool of the same name. > > Currently missing unit tests. Also completely untested (except that > `make test` runs on Linux, so there are no obvious syntax errors). > > With thanks to Ralf Habacker for the initial work on porting > kde4_add_app_icon. > > CHANGELOG: Add ECMAddAppIcon module to add icons to executable targets > on Windows and Mac OS X. > > > Diffs > ----- > > docs/find-module/FindPng2Ico.rst PRE-CREATION > docs/module/ECMAddAppIcon.rst PRE-CREATION > find-modules/FindPng2Ico.cmake PRE-CREATION > modules/ECMAddAppIcon.cmake PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/122135/diff/ > > > Testing > ------- > > `make test` passes, which just provides a very basic check for syntax errors. > > > Thanks, > > Alex Merry > > --===============6360936761880139290== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit

This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122135/

Ship it!

modules/ECMAddAppIcon.cmake (Diff revision 5)
		19	# ``<size>`` is a numeric pixel size (typically 16, 22, 32, 48, 64, 128 or

pixel size 22 is not used in the code and should be removed here

modules/ECMAddAppIcon.cmake (Diff revision 5)
		140	${windows_icons}

trailing whitespace

modules/ECMAddAppIcon.cmake (Diff revision 5)
		148	ARGS "${_outfilename}.ico" ${windows_icons}

trailing whitespace

modules/ECMAddAppIcon.cmake (Diff revision 5)
		150	WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"

This should also be ${CMAKE_CURRENT_SOURCE_DIR}

windows support otherwise looks good.

- Ralf Habacker

On Januar 24th, 2015, 11:11 vorm. CET, Alex Merry wrote:

Review request for Extra Cmake Modules, Ralf Habacker and Jeremy Whiting.

By Alex Merry.

Updated Jan. 24, 2015, 11:11 vorm.

Repository: extra-cmake-modules

Description

This adds an application icon to an executable from PNG files for Windows and Mac OS X. Unlike the similar kde4_add_app_icon macro from kdelibs, this requires icons to be explicitly listed as arguments (meaning CMake can tell when ones are added or deleted and reconfigure as appropriate), and it works with Matthias Benkmann's png2ico tool, as well as the KDE-Win tool of the same name.

Currently missing unit tests. Also completely untested (except that `make test` runs on Linux, so there are no obvious syntax errors).

With thanks to Ralf Habacker for the initial work on porting kde4_add_app_icon.

CHANGELOG: Add ECMAddAppIcon module to add icons to executable targets on Windows and Mac OS X.

Testing

make test passes, which just provides a very basic check for syntax errors.

Diffs

docs/find-module/FindPng2Ico.rst (PRE-CREATION)
docs/module/ECMAddAppIcon.rst (PRE-CREATION)
find-modules/FindPng2Ico.cmake (PRE-CREATION)
modules/ECMAddAppIcon.cmake (PRE-CREATION)

--===============6360936761880139290==-- --===============4777325970564750661== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Kde-buildsystem mailing list Kde-buildsystem@kde.org https://mail.kde.org/mailman/listinfo/kde-buildsystem --===============4777325970564750661==--