Review Request: Fix icon generation and installation on OS X

[Laszlo Papp]

> On Dec. 20, 2012, 6:58 p.m., Laszlo Papp wrote:
> > 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.
> 
> Yue Liu wrote:
>     that doesn't save many characters but increased complexity since you have to emulate a 2d array.
> 
> Laszlo Papp wrote:
>     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 Papp wrote:
>     Also, the raw data is not separated from the code this way as much as it could be.
> 
> Yue Liu wrote:
>     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.
> 
> Laszlo Papp wrote:
>     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?
> 
> Yue Liu wrote:
>     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.

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


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107752/#review23767
-----------------------------------------------------------


On Dec. 20, 2012, 6:10 p.m., Yue Liu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107752/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2012, 6:10 p.m.)
> 
> 
> 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: http://git.reviewboard.kde.org/r/107752/diff/
> 
> 
> Testing
> -------
> 
> Works well on 4.9 branch.
> Not sure if some changes breaks other platforms.
> 
> 
> Thanks,
> 
> Yue Liu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20121221/ec68166b/attachment.htm>