DevHeads.net

Review Request: Fix icon generation and installation on OS X

Review request for kdelibs.

Description
There are two issues when using kde4_add_app_icon on mac. a) apps using kdeinit won't install icon files to thier app bundles, b) mac app icon generating method is outdated and does not support retina resolution.

The patch changed kde4_add_kdeinit_executable and kde4_add_app_icon to solve these issues.

Diffs
cmake/modules/KDE4Macros.cmake 0753879

Diff: <a href="http://git.reviewboard.kde.org/r/107752/diff/" title="http://git.reviewboard.kde.org/r/107752/diff/">http://git.reviewboard.kde.org/r/107752/diff/</a>

Testing
Works well on 4.9 branch.
Not sure if some changes breaks other platforms.

Thanks,

Yue Liu

Comments

Re: Review Request: Fix icon generation and installation on OS X

By Yue Liu at 12/20/2012 - 14:10

(Updated Dec. 20, 2012, 6:10 p.m.)

Review request for kdelibs.

Changes
changed repeated codes with a macro

Description
There are two issues when using kde4_add_app_icon on mac. a) apps using kdeinit won't install icon files to thier app bundles, b) mac app icon generating method is outdated and does not support retina resolution.

The patch changed kde4_add_kdeinit_executable and kde4_add_app_icon to solve these issues.

Diffs (updated)
cmake/modules/KDE4Macros.cmake 0753879

Diff: <a href="http://git.reviewboard.kde.org/r/107752/diff/" title="http://git.reviewboard.kde.org/r/107752/diff/">http://git.reviewboard.kde.org/r/107752/diff/</a>

Testing
Works well on 4.9 branch.
Not sure if some changes breaks other platforms.

Thanks,

Yue Liu

Re: Review Request: Fix icon generation and installation on OS X

By Commit Hook at 12/21/2012 - 11:55

This review has been submitted with commit 0695c3fc8eb9e288ede3b792213e8a40f7278e94 by Yue Liu to branch master.

- Commit Hook

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Commit Hook at 12/21/2012 - 11:54

This review has been submitted with commit 52ee764212613012f1f6bc5924dd7d7b4c5a531a by Yue Liu to branch KDE/4.9.

- Commit Hook

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Laszlo Papp at 12/21/2012 - 06:15

Ship it!

If it does not break on other systems, like Linux, Windows and so forth, and if you do not wish to improve it, you have my +1.

- Laszlo Papp

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Laszlo Papp at 12/20/2012 - 14:58

Great, thank you for your care! One suggestion to this, and I think then it is fine from that point of view: you could write a foreach on top of the "copy_icons" macro, and avoid the same function name in each line.

- Laszlo Papp

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Yue Liu at 12/20/2012 - 21:48

In this case I think a non-programmer won't be able to change anything, a non-programmer even don't know what this macro is used for. If add foreach loop it just increase programmers' effort to understand the code. And next time these patterns change will be the time apple invents a new icon format, at that time even the whole processing logic should be changed, separated patterns doesn't help. Also I see this kind of coding style is already being used in other places, such as win32 icon selecting process just several lines above.

- Yue

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Yue Liu at 12/20/2012 - 17:27

sorry i don't get it what's the difference between call that ten times directly and use a foreach loop to call that ten times. And those patterns has to be in the code some where, what's the difference between put them in an array and put them in several adjacent lines.

- Yue

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Yue Liu at 12/20/2012 - 15:28

that doesn't save many characters but increased complexity since you have to emulate a 2d array.

- Yue

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Laszlo Papp at 12/21/2012 - 05:10

To be honest, this discussion takes longer than adding a simple foreach, and hence programming this nicely. :)))

If you think calling the same stuff manually potentially 10+ times is a good habit, I have nothing more to add to it. I will try to make this nicer then on my own later when I find the time.

By the way, referring to an existing bad habit is not an excuse in my opinion, especially in this case.

- Laszlo

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Laszlo Papp at 12/20/2012 - 20:26

You wanna separate data and code usually as much as possible while programming so that non-programmers can change raw data, and the code works just out of the box. This is a fairly essential principle, and surely, if you have a raw data storage variable, it is not mixed right in the middle.

Don't you feel that 10 or potentially more later function calls are just plain wrong?

- Laszlo

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Laszlo Papp at 12/20/2012 - 16:55

Also, the raw data is not separated from the code this way as much as it could be.

- Laszlo

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Laszlo Papp at 12/20/2012 - 16:52

You follow the 2D logic either way, but now it is done manually by copy/paste as many times as you need it which is currently ten times the same call.

- Laszlo

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Kurt Hindenburg at 12/19/2012 - 11:15

Thanks for this - I was just trying to figure out why the konsole .icns wasn't copied to the Resources folder. The patch does work for Konsole in generating the .icns and installing it. I do get warnings for some .png that it couldn't create. If there is concern about the icon/png creation, perhaps split the patches into 2. I'd like to see the fix for the .icns Resources to go in asap and backported. I tested this on my mac mini ML w/ macports - konsole from master trunk.

- Kurt Hindenburg

On Dec. 16, 2012, 7:26 a.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Laszlo Papp at 12/16/2012 - 09:47

cmake/modules/KDE4Macros.cmake
<http://git.reviewboard.kde.org/r/107752/#comment18079>

Wouldn't it be nicer to write a loop for this with the icon resolutions?

- Laszlo Papp

On Dec. 16, 2012, 7:26 a.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Yue Liu at 12/20/2012 - 14:36

fixed in r2

- Yue

On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Yue Liu at 12/16/2012 - 11:37

and for some resolutions one icon is needed, for some two icons are needed (like 16x16@2x and 32x32, both are 32x32)

- Yue

On Dec. 16, 2012, 7:26 a.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Yue Liu at 12/16/2012 - 11:32

osx icon only uses some specific resolution, 22x22 and 48x48 are not used.

- Yue

On Dec. 16, 2012, 7:26 a.m., Yue Liu wrote:

Re: Review Request: Fix icon generation and installation on OS X

By Laszlo Papp at 12/16/2012 - 12:20

Yes, so?

You have 2-3 factors, but most of the code is the same for all. In other words, that is what functions exist for with parameters.

- Laszlo

On Dec. 16, 2012, 7:26 a.m., Yue Liu wrote: