Details

Reviewers

vonreth
vpinon
apol
alexmerry
krop

Commits

R240:2c2bb8f4766f: Improve ECMAddAppIconMacro.

Summary

Add support for SIDEBAR_ICONS on macOS
Allow specifying a basename for the icon file via OUTFILE_BASENAME
Add support for HiRes icons on Windows via icotool

I'm sorry this went all into one big change. I see that it's not optimal,
but it's really hard to rip it apart...

Also to me the whole code with support for two flavors of png2ico and icotool seems very spaghetti-ish. IMHO there's no good reason to keep supporting all three, icotool is the only maintained project and the only one supporting more than 128px wide icons. That's why I would suggest to simplify the whole code by only supporting icotool in one of the next releases.

Test Plan

We use this version of ECMAddAppIconMacro in ownCloud client and it works...
I tested icotool natively and while cross-compiling on linux.

SIDEBAR_ICONS are also working...

If you want to test this with the ownCloud client, it's best to use
https://github.com/dschmidt/owncloud-client/tree/fix-app-icon-macro
because that contains a small fix I just PR'ed and which is not
in master yet. (We maintain a fork of the module there, so compiling it for Windows or macOS will automatically use the version of the module that I'm submitting)

Diff Detail

Repository

R240 Extra CMake Modules

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

dschmidt created this revision.Jun 23 2018, 1:34 PM

Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptJun 23 2018, 1:34 PM

Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript

dschmidt requested review of this revision.Jun 23 2018, 1:34 PM

Harbormaster completed remote builds in B251: Diff 36562.Jun 23 2018, 1:34 PM

dschmidt edited the summary of this revision. (Show Details)Jun 23 2018, 1:35 PM

dschmidt edited the summary of this revision. (Show Details)Jun 23 2018, 1:41 PM

dschmidt added reviewers: vpinon, apol, alexmerry.

Updated documentation

Harbormaster completed remote builds in B252: Diff 36563.Jun 23 2018, 2:06 PM

Update documentation again

Harbormaster completed remote builds in B253: Diff 36564.Jun 23 2018, 2:08 PM

dschmidt edited the test plan for this revision. (Show Details)Jun 23 2018, 2:10 PM

krop added a subscriber: krop.Jun 23 2018, 2:29 PM

krop added inline comments.

find-modules/FindIcoTool.cmake
2	Missing doc Missing license
modules/ECMAddAppIcon.cmake
70	extra line that shouldn't be there
105–107	ECMAddAppIcon has an unit test in tests/ECMAddAppIconTest/ Please check if you can test the new parameters.

dschmidt marked an inline comment as done.Jun 24 2018, 12:14 PM

dschmidt added inline comments.

find-modules/FindIcoTool.cmake
2	@vpinon Can you let me know what you want here?
modules/ECMAddAppIcon.cmake
105–107	Added a test for OUTFILE_BASENAME and WIP for SIDEBAR_ICONS. Need to get my hands on a macOS system to test further...

vpinon added inline comments.Jun 24 2018, 7:29 PM

find-modules/FindIcoTool.cmake
2	Hello, FindIcoTool.cmake is largely copied from FindPng2Ico.cmake, I'm even not sure I can really claim the copyright. In any case, I would choose the same license as all other ECM files (among which FindPng2Ico.cmake), which doesn't give its name but I believe is LGPLv2?

krop added inline comments.Jun 26 2018, 9:44 AM

find-modules/FindIcoTool.cmake
2	FindPng2Ico uses the BSD license. FindIcoTool shall use the same one.

Tests stub and parameter rename

Harbormaster completed remote builds in B340: Diff 36707.Jun 26 2018, 6:18 PM

Add copyright, license and documentation to FindIcoTool

Harbormaster completed remote builds in B343: Diff 36710.Jun 26 2018, 6:24 PM

Added license, documentation and Alex Merry as copyright holder as the find module for png2ico was written by him.

Fix macOS tests

Harbormaster completed remote builds in B344: Diff 36712.Jun 26 2018, 7:50 PM

dschmidt updated this revision to Diff 36713.Jun 26 2018, 7:56 PM

This comment was removed by dschmidt.

Harbormaster completed remote builds in B345: Diff 36713.Jun 26 2018, 7:56 PM

Squashed everything into one commit, sigh.

Harbormaster completed remote builds in B346: Diff 36715.Jun 26 2018, 8:15 PM

dschmidt edited the summary of this revision. (Show Details)Jun 26 2018, 8:22 PM

Commit fixes to the actual ECMAddAppIconMacro

Harbormaster completed remote builds in B348: Diff 36718.Jun 26 2018, 8:36 PM

Remove debug spam ...

Harbormaster completed remote builds in B349: Diff 36719.Jun 26 2018, 8:40 PM

dschmidt marked 2 inline comments as done.Jun 26 2018, 8:47 PM

krop added inline comments.Jun 29 2018, 12:55 PM

find-modules/FindIcoTool.cmake
26	The next release will be 5.48
modules/ECMAddAppIcon.cmake
11–12	5.48
30	5.48
35	5.48
142–145	24 is mentioned in the doc but unused here. Is it expected?
161	36? It's not mentioned in the doc.
230	not 24 ?
tests/ECMAddAppIconTest/CMakeLists.txt
40	The test shouldn't "replace" the simple form. Add a new one instead to have both forms tested.

Fix "Since version" and wrong resolutions in a few places

Harbormaster completed remote builds in B449: Diff 36889.Jun 29 2018, 3:15 PM

Fix comment

Harbormaster completed remote builds in B450: Diff 36890.Jun 29 2018, 3:20 PM

Fix since version in FindIcoTool too

Harbormaster completed remote builds in B451: Diff 36891.Jun 29 2018, 3:24 PM

dschmidt marked 8 inline comments as done.Jun 29 2018, 3:28 PM

dschmidt added inline comments.

modules/ECMAddAppIcon.cmake
161	Good catch, I forgot to update this part after I fixed the resolutions in a different place.. The resolutions I copied from the original ownCloud macro were wrong in the first place, maybe because Apple changed the rules/resolutions at some point.

from a buildsystem pov, the change looks fine. Maybe someone has comments to add.

@apol ?

+1
Looks good to me, even tested. :)

Thanks !

This revision is now accepted and ready to land.Jul 3 2018, 4:22 PM

The 5.48 release is close. Don't forget to push these changes.

Closed by commit R240:2c2bb8f4766f: Improve ECMAddAppIconMacro. (authored by dschmidt). · Explain WhyJul 8 2018, 6:00 PM

This revision was automatically updated to reflect the committed changes.

dschmidt marked an inline comment as done.

Seems this was pushed only after the tagging of 5.48 was done on Saturday night, so too late for that release.
If this should be still part of 5.48, get in contact with dfaure so he could consider moving the tag/release pointer to include this commit, or please update the since tags to 5.49 in prepartion of the next release.

			Path
A	M		docs/find-module/FindIcoTool.rst (1 line)
A	M		find-modules/FindIcoTool.cmake (80 lines)
M			modules/ECMAddAppIcon.cmake (271 lines)
A	M	(bin)	tests/ECMAddAppIconTest/128-category-name-sidebar.png
A	M	(bin)	tests/ECMAddAppIconTest/16-category-name-sidebar.png
M			tests/ECMAddAppIconTest/CMakeLists.txt (51 lines)
M			tests/ECMAddAppIconTest/check_files.cmake.in (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	36562	06fa81a		Jun 23 2018, 1:34 PM	★	★
Diff 2	36563	06fa81a	Updated documentation	Jun 23 2018, 2:06 PM	★	★
Diff 3	36564	06fa81a	Update documentation again	Jun 23 2018, 2:08 PM	★	★
Diff 4	36707	c98759e	Tests stub and parameter rename	Jun 26 2018, 6:18 PM	★	★
Diff 5	36710	c98759e	- Add copyright, license and documentation to FindIcoTool	Jun 26 2018, 6:24 PM	★	★
Diff 6	36712	acf8a53	Fix macOS tests	Jun 26 2018, 7:50 PM	★	★
Diff 7	36713	08e1d06	Removed crap commit	Jun 26 2018, 7:56 PM	★	★
Diff 8	36715	06fa81a	Squashed everything into one commit, sigh.	Jun 26 2018, 8:15 PM	★	★
Diff 9	36718	06fa81a	Commit fixes to the actual ECMAddAppIconMacro	Jun 26 2018, 8:36 PM	★	★
Diff 10	36719	06fa81a	Remove debug spam ...	Jun 26 2018, 8:40 PM	★	★
Diff 11	36889	06fa81a	Fix "Since version" and wrong resolutions in a few places	Jun 29 2018, 3:15 PM	★	★
Diff 12	36890	06fa81a	Fix comment	Jun 29 2018, 3:20 PM	★	★
Diff 13	36891	06fa81a	Fix since version in FindIcoTool too	Jun 29 2018, 3:24 PM	★	★
Diff 14	37360	d6cc583	R240:2c2bb8f4766fec5e391839dbc093b638acee3a22	Jul 8 2018, 6:00 PM	★	★

Improve ECMAddAppIconMacro.
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 37360

docs/find-module/FindIcoTool.rst

find-modules/FindIcoTool.cmake

modules/ECMAddAppIcon.cmake

tests/ECMAddAppIconTest/128-category-name-sidebar.png

tests/ECMAddAppIconTest/16-category-name-sidebar.png

tests/ECMAddAppIconTest/CMakeLists.txt

tests/ECMAddAppIconTest/check_files.cmake.in

Improve ECMAddAppIconMacro.ClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 37360

docs/find-module/FindIcoTool.rst

find-modules/FindIcoTool.cmake

modules/ECMAddAppIcon.cmake

tests/ECMAddAppIconTest/128-category-name-sidebar.png

tests/ECMAddAppIconTest/16-category-name-sidebar.png

tests/ECMAddAppIconTest/CMakeLists.txt

tests/ECMAddAppIconTest/check_files.cmake.in

Improve ECMAddAppIconMacro.
ClosedPublic
Actions

Revision Contents
Changeset List