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