----------------------------------------------------------- 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 > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
