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