Review Request 119183: A HSV color slider docker.

[Dmitry Kazakov]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119183/#review66476
-----------------------------------------------------------


There are a few minor issues:

1) Coding style, which can be fixed using 'astyle' (see a comment below)
2) Saving of booleans into config file is usually done via integers.

When it is fixed, the patch is ok to go to master.


krita/plugins/extensions/dockers/colorslider/kis_color_slider.h
<https://git.reviewboard.kde.org/r/119183/#comment46345>

    Better change the docs :)



krita/plugins/extensions/dockers/colorslider/kis_color_slider_dock.cpp
<https://git.reviewboard.kde.org/r/119183/#comment46347>

    This break KDE coding style policies:
    https://techbase.kde.org/Policies/Library_Code_Policy
    
    Actually you can simplify these blocks of code quite easily, since C++ operator == returns bool:
    
    m_SlidersConfigArray[0] = hsvH == "true";
    
    It is also common to save boolean values into KConfigGroup as integer, instead of strings, because C++ automatically converts 0,1 <-> false,true.
    
    In such a case your code will look extremely simple:
    
    m_SlidersConfigArray[0] = cfg.readEntry("hsvH", 0);
    
    Until no user has these options as strings, it is safe to change it to integers :)



krita/plugins/extensions/dockers/colorslider/kis_color_slider_widget.cpp
<https://git.reviewboard.kde.org/r/119183/#comment46349>

    This is also not KDE-style :(
    
    Probably, you could run astyle on your files to get them right.
    
    https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Artistic_Style_.28astyle.29_automatic_code_formatting



krita/plugins/extensions/dockers/colorslider/kis_color_slider_widget.cpp
<https://git.reviewboard.kde.org/r/119183/#comment46348>

    The same as previous comment


- Dmitry Kazakov


On Сен. 13, 2014, 9:04 п.п., Wolthera van Hövell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119183/
> -----------------------------------------------------------
> 
> (Updated Сен. 13, 2014, 9:04 п.п.)
> 
> 
> Review request for Calligra, Dmitry Kazakov and Boudewijn Rempt.
> 
> 
> Bugs: https://bugs.kde.org/show_bug.cgi?id=313787
>     http://bugs.kde.org/show_bug.cgi?id=https://bugs.kde.org/show_bug.cgi?id=313787
> 
> 
> Repository: calligra
> 
> 
> Description
> -------
> 
> I have been working on this https://bugs.kde.org/show_bug.cgi?id=313787.
> 
> This patch adds a color sliders docker to Krita.
> The intention is to eventually get to a docker that can can be modified through the preferences, so one could have only saturation sliders, or two saturations sliders and a value slider.
> This docker is only intended for derived models, where the specific colour selector is for actual colour spaces.
> 
> 
> Diffs
> -----
> 
>   krita/plugins/extensions/dockers/CMakeLists.txt f94eb44 
>   krita/plugins/extensions/dockers/advancedcolorselector/kis_color_selector_settings.cpp f171c2e 
>   krita/plugins/extensions/dockers/advancedcolorselector/wdg_color_selector_settings.ui dbe9070 
>   krita/plugins/extensions/dockers/colorslider/CMakeLists.txt PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_color_slider.h PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_color_slider.cpp PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_color_slider_dock.h PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_color_slider_dock.cpp PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_color_slider_input.h PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_color_slider_input.cpp PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_color_slider_widget.h PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_color_slider_widget.cpp PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_hsv_slider.h PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/kis_hsv_slider.cpp PRE-CREATION 
>   krita/plugins/extensions/dockers/colorslider/krita_colorslider.desktop PRE-CREATION 
>   libs/pigment/KoColorConversions.cpp fc7954b 
> 
> Diff: https://git.reviewboard.kde.org/r/119183/diff/
> 
> 
> Testing
> -------
> 
> A lot, no user testing, there's still a handful of open bugs.
> We have 12 sliders, for each of the HSX colour models.
> It compiles, and it doesn't crash.
> * ~~Kocolorsliders wasn't intended for HSX, so we'll need to write a new widget for HSX.~~[done]
> * ~~For now, all 12 sliders show up using kocolor sliders. I have managed to prevent it from zealously updating everything else, so it doesn't break other things.~~ [Thanks to the fix by dmitry on the specific colour selector, this has been fixed]
> * ~~The number-input is broken. Half of the time~~ [fixed, I made it so that it will only change after loosing focus]
> * ~~So is the slider input itself, I suspect this is rounding errors again, and will have to adjust the self-update, like I did for the colour palette.~~ We need to increase the precision of all the algoritms.
> * ~~Config still needs to be put in.[done, however, after the config is changed, it doesn't re-update the docker again, on restart the update takes place.]~~[done]
> * ~~There's still licenses missing~~[done]
> * ~~The code is a bloody mess.~
> * ~~There's a bug in the hsy and hsi code I wrote, but there's also bugs in the qt hsv and hsl code(gray in particular is strange) I'll chase these down with increasing the precision.~~ Fixed.
> * There's some awkwardness between the different colour models updating eachother, leading to slider-dancing. Hopefull this'll be fixed with precision increase.
> * The sliders don't update properly on LUT changes. This also happens in the specific colour selector.
> I want to do these last two in a seperate patch.
> * Made the H and S sliders work when the colour is black, as requested by Sven.
> * Aligned the labels to the right..
> * Couldn't set the labels to align perfectly.
> 
> 
> Thanks,
> 
> Wolthera van Hövell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20140914/cf94906a/attachment.htm>