[WIP] Further Color Picker Upgrades, Fixes and Refactors.
Needs RevisionPublic

Authored by emmetoneill on Nov 12 2018, 6:56 AM.

Details

Reviewers
dkazakov
Group Reviewers
Krita
Summary

They say you can't teach and old dog new tricks, and so I'm back to hacking Krita's color pickers to fix some things that bother me, to improve others, and to make things a bit more consistent across the various pickers.

- Feature: Holding Shift (technically Ctrl+Shift in the ctrl-picker) while picking colors enters "pure" mode, which temporarily bypasses things like radius and mixing. (Also addresses BUG:397889)

  • I've slowed down and tuned the "resample" compression timing a bit while dragging the picker around the screen. This prevents sampling from happening too frequently and makes it easier to control color picker mixing. Tapping once still goes into effect instantly.
  • Both main pickers (the dedicated "P" tool, as well as the ctrl-picker within kis_tool_paint) now share more code and work more consistently with each other. There is still probably more that can be done on this front.
  • Colors can now be picked from Reference Images with both the dedicated Color Picker Tool as well as the Ctrl-Picker. As of now, there are probably a few minor bugs.
  • I've revamped the Color Picker's Tool Options widget. It looks a bit nicer and is a bit better organized.
  • Some related behind-the-scenes code refactoring and reorganization. The growing number of color picker utility functions have been moved out of KisToolUtils into their own KisPickerUtils namespace in the same file.
  • I've changed the Color Picker terminology from "blend" to "mix". "Mixing" is probably a better description of what's happening, and I think "blending" is too easy to confuse with other things, such as the painting technique as well as things like blend modes. This was a poor naming choice on my part from the beginning, and while I know this has the potential to cause confusion, I think it'll be better in the long run. (Sorry!) I'll also make sure to change this in the documentation before whenever this gets pushed.
  • The dedicated Color Picker Tool's add to palette functionality works again.
  • By default the Paint Tool's ctrl-picker does not share configuration with the dedicated picker. If users want to use dedicated picker settings like mixing and radius from within the ctrl-picker, they must now opt in by enabling a new checkbox (Settings > Configure > General > Tools > "Use Color Picker's tool options for ctrl-picker"). I've added a tool tip to this option that explains in more detail what it does.

Thanks as always,
Emmet

Test Plan

Test out the various functionality of the various color pickers (Paint Tool + Ctrl, dedicated Color Picker Tool, etc.)

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
emmetoneill created this revision.Nov 12 2018, 6:56 AM
Restricted Application added a reviewer: Krita. · View Herald TranscriptNov 12 2018, 6:56 AM
Restricted Application added a project: Krita. · View Herald Transcript
emmetoneill requested review of this revision.Nov 12 2018, 6:56 AM
emmetoneill edited the summary of this revision. (Show Details)Nov 12 2018, 7:12 AM

Part of T8708

Hi, @emmetoneill!

I have tested your patch very briefly. It looks fine :) Here are the notes:

  1. [Regression?] Normal Ctrl+click in a paint too has stopped working for some reason. Do you have the same?
  2. Shift+Ctrl+click seem to work fine and does what expected
  3. Blending while dragging seem to work.
  4. The Color Picker GUI looks good :)

Hi, @emmetoneill!

I have tested your patch very briefly. It looks fine :) Here are the notes:

  1. [Regression?] Normal Ctrl+click in a paint too has stopped working for some reason. Do you have the same?
  2. Shift+Ctrl+click seem to work fine and does what expected
  3. Blending while dragging seem to work.
  4. The Color Picker GUI looks good :)

@dkazakov Hey, Dmitry.

1.) It's still working for me! Could you double-check your dedicated Color Picker Tool Options? It's possible that Update is not checked or Mix is set at 0% (which is actually a separate bug, as the minimum should probably be 1%)?

2/3/4.) Thanks!

It looks like I didn't set Update to be checked by default in the .ui file, so that may be what caused the issue.

dkazakov requested changes to this revision.Nov 21 2018, 6:30 AM

Hi, @emmetoneill!

1.) It's still working for me! Could you double-check your dedicated Color Picker Tool Options?

Yes, this problem was something local in the configs. I've reset the configs and the ctrl+click started to work as before.

[REGRESSION]
After that I looked at the code and found out a really serious regression: now the "ctrl+right click" shortcut doesn't pick the color to background color (yes, we have a separate shortcut for that), because the destination of the color-pick is read from m_pickerSettings instead of the action type like it was before.

Proposed solution:

  1. Don't pass ColorPickerConfig to the stroke strategy at all, pass only 'mix' and 'radius' as separate parameters
  2. Recover sigColorUpdated() signal to return color only. The strategy shouldn't even know what target color it is going to write to
  3. Make a decision about where to write the updated color in the tool itself
This revision now requires changes to proceed.Nov 21 2018, 6:30 AM
emmetoneill updated this revision to Diff 48766.EditedJan 5 2019, 10:55 PM
emmetoneill edited the summary of this revision. (Show Details)
emmetoneill edited the test plan for this revision. (Show Details)

Hey again, and happy new year.

I went ahead and made the necessary fixes.
There are still some minor differences between the Color Picker Tool and the Paint Tool's ctrl-picker,
but without bigger changes to the tool system this is probably as unified as I can make them.

rempt added a subscriber: rempt.Jan 7 2019, 10:26 AM

Maybe we should also have a global action or option to enable/disable picking the alpha value. People are getting confused again...

Maybe we should also have a global action or option to enable/disable picking the alpha value. People are getting confused again...

Sure. What's confusing people?

dkazakov requested changes to this revision.Jan 8 2019, 11:26 AM
dkazakov added a subscriber: Deevad.

Hi, @emmetoneill!

I have tested your patch and found the following issues:

  1. The controls are really bigger than the original ones for some reason.

New:


Old:

  1. In color picker tool, when I press Shift, I see no indication. I guess we should show the "pure" mode with some additional shortcut.
  1. [not regression] In color picker tool, when changing the mode of the tool from "Sample All Visible Layers" to "Current Layer", the cursor doesn't change. I guess originally, we planned to show different cursors, but for some reason it doesn't work.
  1. I agree with @rempt that adding blending into the normal "Ctrl+click" gesture will be highly confusing to the painters. Speaking truly, I would just prefer to avoid adding this feature to the brush tool. If the user wants to blend the colors, he can always activate the picker tool (last work for @Deevad and other painters). Or this option should be available directly in the Brush Tool's options. Otherwise we'll get tons of reports telling that "my color picker doesn't work"
  1. Please don't add Ctrl+Shift+click gesture for "pure" mode. Partially, because of point 4) (it should be an option instead), partially, because this shortcut is used in other places and you'll introduce a conflict. More that that, the shortcut itself is confusing now: it shows an icon of "pick from current layer", but picks from projection; and it doesn't have Ctrl+Alt+Shift+click counterpart. I think it is better just to remove that shortcut at all.
This revision now requires changes to proceed.Jan 8 2019, 11:26 AM
  1. The controls are really bigger than the original ones for some reason. New:
    Old:

Alright, I can make them smaller.

  1. In color picker tool, when I press Shift, I see no indication. I guess we should show the "pure" mode with some additional shortcut.
  2. [not regression] In color picker tool, when changing the mode of the tool from "Sample All Visible Layers" to "Current Layer", the cursor doesn't change. I guess originally, we planned to show different cursors, but for some reason it doesn't work.

Makes sense to me.

  1. I agree with @rempt that adding blending into the normal "Ctrl+click" gesture will be highly confusing to the painters.

Oh, is that what was confusing people? The blending/mixing? I thought we were talking about picking alpha value from semitransparent colors...

Speaking truly, I would just prefer to avoid adding this feature to the brush tool. If the user wants to blend the colors, he can always activate the picker tool (last work for @Deevad and other painters). Or this option should be available directly in the Brush Tool's options. Otherwise we'll get tons of reports telling that "my color picker doesn't work"

I appreciate the honesty. Frankly, I'm of the opinion that when the user changes their Color Picker's tool options, those settings should apply uniformly. Does the user really understand that the dedicated Color Picker tool is a separate entity from the Brush Tool's Ctrl-Picker? Do they need to understand these kinds of inner-workings? I don't really think that they do.

We could add a global option in the settings menu that enables and disables mixing for the ctrl-picker, if that's what people want. If so, this same logic should apply to radius and other dedicated picker settings in the future. So maybe the option would be something like Apply Color Picker Tool options to the Brush Tool's ctrl-picker.? [Disabled by default].

We could cherry-pick certain picker settings to add to the Brush Tool's Tool Option panel, but this is kind of sloppy and would be just as confusing, if not more. Will the users understand why something that appears to be the same setting exists in two different places? Maybe, but maybe not.

Ideally, for me at least, the Brush's ctrl-picker would simply be a temporary invocation of the real color picker tool which would also temporarily append the picker's options under the brush's within the Tool Options panel. Releasing ctrl would exit this temporary mode, popping the picker's options off the Tool Options docker like a stack. But that's probably not going to happen.

In lieu of that, however, I do feel that there should at least be consistency across these pickers, both in how they function and how they are controlled. Creating coherence and homogenizing these two separate things is really the central theme of this patch, after all.

  1. Please don't add Ctrl+Shift+click gesture for "pure" mode. Partially, because of point 4) (it should be an option instead), partially, because this shortcut is used in other places and you'll introduce a conflict. More that that, the shortcut itself is confusing now: it shows an icon of "pick from current layer", but picks from projection; and it doesn't have Ctrl+Alt+Shift+click counterpart. I think it is better just to remove that shortcut at all.

A big part of the reason the pure mode is being added is to make it easier for people to quickly switch between picking with and without mix/radius, without having to fiddle with their tool options, meaning it's actually more valuable from within the Brush Tool (as the picker's Tool Options are no longer on the screen). This type of things has also been requested (BUG:397889).


Anyway, I appreciate the honest and detailed review, Dmitry.
If what I want to do diverges too far from the overarching goals of Krita, I hope that we can be frank about that.
I try to be tough, so above feelings and all else I value clear criticism! =]

Thanks,
Emmet

emmetoneill updated this revision to Diff 50029.EditedJan 21 2019, 10:02 PM
emmetoneill edited the summary of this revision. (Show Details)

Alright, I've made the requested changes:

  • Made the Color Picker's Tool Options widgets smaller.
  • Got rid of the shift modifier's pure mode.
  • By default the Paint Tool's ctrl-picker does not share configuration with the dedicated picker. If users want to use dedicated picker settings like mixing and radius from within the ctrl-picker, they must now opt in by enabling a new checkbox (Settings > Configure > General > Tools > "Use Color Picker's tool options for ctrl-picker"). I've added a tool tip to this option that explains in more detail what it does.

It's not really my ideal, but I can live with it and I hope that's an OK compromise for what you guys want.

As always, thanks for the review and let me know if there is anything else that I should change. If not, I'm ready to push the changes.

edit: I almost forgot. I added a small bit of code for changing the cursor when switching sampling mode (visible vs layer), but I can't seem to get it to work for some reason. It seems like it should be as simple as calling useCursor() when the mode changes, but maybe there's something I'm missing there. See inline comment below:

emmetoneill added inline comments.Tue, Jan 22, 9:04 PM
plugins/tools/basictools/kis_tool_colorpicker.cc
99

Any idea why this might not be working?

I've tried scattering calls to this function across various places (constructor, activation, gui change signal, etc.).
Printing debug lines shows me that the code is being run, but useCursor seems to be having no effect.
It's probably something simple, but I figured I'd just ask about it.

I appreciate the honesty. Frankly, I'm of the opinion that when the user changes their Color Picker's tool options, those settings should apply uniformly. Does the user really understand that the dedicated Color Picker tool is a separate entity from the Brush Tool's Ctrl-Picker? Do they need to understand these kinds of inner-workings? I don't really think that they do.

Yes, exactly. People do not link the Ctrl+click picking to the color picker tool. Ctrl+click does exactly what is announced in its description in the shortcuts manager: "Pick Foreground Color from Merged Image". No extra functionality should be available unless there are controls for it right in from of user's eyes.

We could add a global option in the settings menu that enables and disables mixing for the ctrl-picker, if that's what people want. If so, this same logic should apply to radius and other dedicated picker settings in the future. So maybe the option would be something like Apply Color Picker Tool options to the Brush Tool's ctrl-picker.? [Disabled by default].

I don't think anyone will use a global switcher. Switching global settings is pain for a painter. He/she will better just paint instead. The settings should either be available in from of the user's eyes, or just not used.

As an alternative, you could add an additional "Canvas Input Action" shortcut, which would not be called "Pick Foreground Color from Merged Image", but "Invoke Color Picker Tool". But in such a case, you don't have to refactor any code in the brush tool. You can just temporarily activate the Color Picker tool itself, like we do it for "Draw Line" shortcut.

We could cherry-pick certain picker settings to add to the Brush Tool's Tool Option panel, but this is kind of sloppy and would be just as confusing, if not more. Will the users understand why something that appears to be the same setting exists in two different places? Maybe, but maybe not.

"Invoke Color Picker Tool" shortcut sounds as a sane alternative to it. GUI will show the user that the tool is switched when he presses the shortcut, so he will understand where to search for options.

Ideally, for me at least, the Brush's ctrl-picker would simply be a temporary invocation of the real color picker tool which would also temporarily append the picker's options under the brush's within the Tool Options panel. Relsing ctrl would exit this temporary mode, popping the picker's options off the Tool Options docker like a stack. But that's probably not going to happen.

Please check how 'V' shortcut is implemented in KisToolInvocationAction. It can be done much simpler than your current patch :)

dkazakov requested changes to this revision.Fri, Feb 1, 2:00 PM

Hi, @emmetoneill!

Here is a daft patch for activating Color Picker tool with a shortcut. You can integrate it and get rid of that global option. Then I'll finally test the feature for regressions and we will commit that :)

1diff --git a/libs/ui/input/kis_tool_invocation_action.cpp b/libs/ui/input/kis_tool_invocation_action.cpp
2index a7b501f..792c5d4 100644
3--- a/libs/ui/input/kis_tool_invocation_action.cpp
4+++ b/libs/ui/input/kis_tool_invocation_action.cpp
5@@ -37,12 +37,14 @@ class KisToolInvocationAction::Private
6 public:
7 Private()
8 : active(false),
9- lineToolActivated(false)
10+ lineToolActivated(false),
11+ colorPickerToolActivated(false)
12 {
13 }
14
15 bool active;
16 bool lineToolActivated;
17+ bool colorPickerToolActivated;
18 };
19
20 KisToolInvocationAction::KisToolInvocationAction()
21@@ -57,6 +59,7 @@ KisToolInvocationAction::KisToolInvocationAction()
22 indexes.insert(i18n("Confirm"), ConfirmShortcut);
23 indexes.insert(i18n("Cancel"), CancelShortcut);
24 indexes.insert(i18n("Activate Line Tool"), LineToolShortcut);
25+ indexes.insert(i18n("Activate Color Picker Tool"), ColorPickerToolShortcut);
26 setShortcutIndexes(indexes);
27 }
28
29@@ -73,6 +76,9 @@ void KisToolInvocationAction::activate(int shortcut)
30 if (shortcut == LineToolShortcut) {
31 KoToolManager::instance()->switchToolTemporaryRequested("KritaShape/KisToolLine");
32 d->lineToolActivated = true;
33+ } else if (shortcut == ColorPickerToolShortcut) {
34+ KoToolManager::instance()->switchToolTemporaryRequested("KritaSelected/KisToolColorPicker");
35+ d->colorPickerToolActivated = true;
36 }
37
38 inputManager()->toolProxy()->activateToolAction(KisTool::Primary);
39@@ -88,6 +94,9 @@ void KisToolInvocationAction::deactivate(int shortcut)
40 if (shortcut == LineToolShortcut && d->lineToolActivated) {
41 d->lineToolActivated = false;
42 KoToolManager::instance()->switchBackRequested();
43+ } else if (shortcut == ColorPickerToolShortcut && d->colorPickerToolActivated) {
44+ d->colorPickerToolActivated = false;
45+ KoToolManager::instance()->switchBackRequested();
46 }
47 }
48
49@@ -103,7 +112,10 @@ bool KisToolInvocationAction::canIgnoreModifiers() const
50
51 void KisToolInvocationAction::begin(int shortcut, QEvent *event)
52 {
53- if (shortcut == ActivateShortcut || shortcut == LineToolShortcut) {
54+ if (shortcut == ActivateShortcut ||
55+ shortcut == LineToolShortcut ||
56+ shortcut == ColorPickerToolShortcut) {
57+
58 d->active =
59 inputManager()->toolProxy()->forwardEvent(
60 KisToolProxy::BEGIN, KisTool::Primary, event, event);
61diff --git a/libs/ui/input/kis_tool_invocation_action.h b/libs/ui/input/kis_tool_invocation_action.h
62index d23c9bc..f6f5a08 100644
63--- a/libs/ui/input/kis_tool_invocation_action.h
64+++ b/libs/ui/input/kis_tool_invocation_action.h
65@@ -34,7 +34,8 @@ public:
66 ActivateShortcut,
67 ConfirmShortcut,
68 CancelShortcut,
69- LineToolShortcut
70+ LineToolShortcut,
71+ ColorPickerToolShortcut
72 };
73 explicit KisToolInvocationAction();
74 ~KisToolInvocationAction() override;

This revision now requires changes to proceed.Fri, Feb 1, 2:00 PM