A docker to choose from a gradient between two colors.
GitHub page: https://github.com/tusooa/krita-docker-color-slider
Demo video: https://raw.githubusercontent.com/tusooa/krita-docker-color-slider/master/krita-docker-color-slider-demo.mp4
Details
Details
- Build Krita with the patch.
- Start Krita. In Settings -> Configure Krita -> Python Plugin Manager, enable Mixer Slider Docker. Restart Krita.
- Right-click on menu bar, and enable Mixer Slider Docker.
Expected: The docker appears in the window. You may need to adjust its size and/or position. A button with S should appear on the left. On the right there is a slider from blue to black between blue and black colored squares. (Test default color is correctly displayed) - Create a new document.
- Click the S button on the left of the docker.
Expected: A dialog with a label and a line edit appears. - In the line edit, replace the number with 0. Click OK button.
Expected: The docker has only the S button in the center. (Test the docker can correctly remove lines) - Click the S button. In the dialog, replace 0 with 4. Click OK.
Expected: 4 lines of color gradient from blue to black appear. (Test the docker can correctly add lines) - Click some point on one slider.
Expected: The foreground color should be set to the color at that point. A white triangle appears at the cursor point. - Choose some foreground color from another color selector (pop-up palette, advanced color selector, etc.).
- Click the square on the left of one slider.
Expected: The color of the square becomes the current foreground color. The white triangle appears at the left-most of the slider. - Chose some other foreground color from another color selector. Click the square on the right of that slider.
Expected: Similar to the above, but the white triangle appears at the right-most. The slider has a gradient between the two colors. - Press the mouse inside that slider, and move the mouse to the right-most.
Expected: If you have advanced color selector, the color shown on it should not change. (The right-most color should be the one on the right button.) This step is to check that there is no error converting between ManagedColor and QColor. - Set the left color to a color with a hue (blue, green, yellow, etc.) and the right one to a graytone (with Hue=-1).
Expected: The gradient should not have a change in hue. - Set the left color to red and the right color to blue-ish cyan. (So that the two hues on the advanced-color-selector HSV ring goes through the arc at the bottom (magenta and blue), which is a minor arc (<180 degrees).)
Expected: Magenta and blue appear on the slider. No yellow or green. - Swap the left and right colors. (set the left color to blue-ish cyan and the right to red, don't need to be exactly the same, just ensure the minor arc between them goes through magenta and blue)
Expected: Same (direction should flip, though). - Set the left color to red and the right color to green-ish cyan.
Expected: Yellow and green appear on the slider. No magenta or blue. - Swap the left and right colors.
Expected: Same. - Restart Krita.
Expected: The docker appears the same as the one before restarting, except there is no triangle cursors. (Test that all settings are saved.)
Diff Detail
Diff Detail
- Repository
- R37 Krita
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Hm, I have yet got to test it, but maybe it should be renamed to 'mixer slider'? Color slider docker was the name of the HSV color sliders which have been removed but should make a comeback at some point.
Also adding rebecca as she's got the most python experience.
Comment Actions
Rename the docker as suggested by Wolthera.
Fixed a bug where the docker could not display the left and right colors correctly if there is no config available.
Comment Actions
I just tested this and it works. I can see the use of this. What do other people think of this feature? I like how little space the docker takes. Would be neat if the "S" button had a tooltip.
Code-wise it looks good to me, except for a couple of nitpicky remarks. Btw, I'm currently working on getting all existing Python modules PEP8 compliant. I'm not expecting new submits to follow this as long as it's still a work in progress (some python plugins are already done, some aren't), but if you are interested, here's how to set up code checks: https://phabricator.kde.org/D18404
| plugins/python/mixer_slider_docker/color_slider.py | ||
|---|---|---|
| 75 | Debug prints should be removed, I guess. Otherwise the command line gets very wordy. | |
| plugins/python/mixer_slider_docker/mixer_slider_docker.py | ||
| 112 | I think this were easier to read if you build a list of all the strings first and then use join once, instead of this concatenation of two joins. Took me a bit to understand where all the commas end up. | |
Comment Actions
Simplified logic in color_to_settings()
pep8 naming
Suppressed debug output
Reduced unnecessary imports
Added tooltip for settings button
Comment Actions
Thank you for reviewing this.
I asked on IRC. One commented it is useful; another remarked that it is just another Digital Colors Mixer. However, this docker is more convenient and intuitive than Digital Colors Mixer. In addition, this feature is inspired by a similar one in PaintToolSai, so I guess former SAI users might be happy to see it. When I was using SAI, I did find it useful.
On PEP8: Lowercase with underscore naming cannot be applied to some functions, as they are overriding functions in C++, which use camel case naming. PyQt5 document does not seem to provide a way to make both lint and app happy. @pyqtSlot does make a different signature from the python one, but it seems it should be only used with slots. The from xxx import * in __init__.py seems to be a convention in Python plugins here, so I am currently sticking to that. Application and i18n are things that do not need an import so I do not think I can do anything with the warning that they are undefined.
Also, according to the HACKING file, 79 characters per line can be ignored to improve code readability. Except these, the code conforms to PEP8.
BTW: Krita does not seem to have i18nc function to provide a comment for translators, so I just used a comment on the previous line, which KDE community wiki says is okay. But I still want to make sure whether it really works.
Comment Actions
The patch works as expected :)
I have found only two minor issues:
- The settings dialog should use QSpinBox instead of QLineEdit
- [DISPUTABLE] The arrow looks as if it is offset to the top. Was it expected?
Please fix at least the first point and push the patch into master without any further review :)
| plugins/python/mixer_slider_docker/ui_mixer_slider_docker.py | ||
|---|---|---|
| 48 | Please use QSpinBox instead of the a raw textbox. Spinbox also supports mouse wheel events processing :) | |
Comment Actions
The patch works as expected :)
I have found only two minor issues:
- The settings dialog should use QSpinBox instead of QLineEdit
- [DISPUTABLE] The arrow looks as if it is offset to the top. Was it expected?
Please fix at least the first point and push the patch into master without any further review :)
Thank you for the review. But I think it would be prudent not to push this commit for now. The plugin only works with RGBA color space, not others. To fix this, we will have to add to the Python API, namely ManagedColor.fromQColor() function.
And yes, (2) is expected.
Comment Actions
Okay. Actually, I don't think it should actually "support" non-rgb color spaces. It just show colors correctly when image color space is non-rgb. For that it must use KisDisplayColorConverter class, converts image colors into a correct QColor.
And, yes, there is a bug when working with RGBA F32 color spaces. R and B channels are swapped :(
Comment Actions
That's because floating point RGB colorspaces are RGB, while integer are BGR. As for KisDisplayConverter, we don't expose that to Python yet.
Comment Actions
I mean it might not be necessary to expose KisDisplayConverter, just getting the inverse of colorForCanvas() is okay.