> 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 > >
