----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/1669/#review2508 -----------------------------------------------------------
As said also on IRC (after being asked), I'm NOT ok with the all patch as it is: - it mixes a new good feature (the latex formatting) with a really questionable one (the removal of the color popup) - it goes A LOT of code messup (unwanted spaces around, coding style changes in existing code, inconsistent spacing, etc) also on IRC I asked you to provide first a clean (code-wise) patch (== new review) for ONLY adding the latex formatting, then (after discussion) the other change. I see nothing of that, hence the revert of the commit of this "patch". Also, would be nice to know what is the rationale for the removal of the popup menu, given it is basically part of the workflow of the applet. Now, some few comments on the rest. /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.cpp <http://reviewboard.kde.org/r/1669/#comment1835> Missing i18n. /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.cpp <http://reviewboard.kde.org/r/1669/#comment1836> redF(), greenF() and blueF() /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.cpp <http://reviewboard.kde.org/r/1669/#comment1837> There's a KLocale function to remove the & mark. - Pino On 2009-09-21 13:11:09, Tomaz Canabrava wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/1669/ > ----------------------------------------------------------- > > (Updated 2009-09-21 13:11:09) > > > Review request for Plasma. > > > Summary > ------- > > remvoes the default menu that appears whenever we want to pick a color, not > it uses a default colorstring format to do that, the color string format > should be choosen before picking colors ( click on the round color button, go > to Default Color Format and choose your favorite one), then just pick colors > without the annoying menu popping everytime. > > Also, added support for latex colors. > > > Diffs > ----- > > /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.h 1026319 > /trunk/KDE/kdeplasma-addons/applets/kolourpicker/kolourpicker.cpp 1026319 > > Diff: http://reviewboard.kde.org/r/1669/diff > > > Testing > ------- > > everything working, no regressions found, no new bugs introduced ( from my > tests ) > > > Thanks, > > Tomaz > > _______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
