[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-panel-devel
Subject:    Re: Review Request: adding default colors format for kolourpicker and
From:       "Pino Toscano" <pino () kde ! org>
Date:       2009-09-30 16:49:27
Message-ID: 20090930164927.7972.20226 () localhost
[Download RAW message or body]


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


[prev in list] [next in list] [prev in thread] [next in thread]