Refactor: Unified Color Picker Code & Removed Some Duplication
ClosedPublic

Authored by emmetoneill on May 17 2018, 7:16 AM.

Details

Summary

This patch is part of task T8708, focused on further improving Krita's color picker.

As of right now, Krita effectively has two near-duplicate paths when it comes to color picking code: the code for the dedicated Color Selector Tool (P) exists mainly in "kis_tool_colorpicker", while the code for the ctrl-key activated color picker that you can pull up while other tools are active [as well as the scratchpad's picker] exists mainly in "kis_tool_utils".

This patch addresses this issue by, as much as possible without some much bigger changes, unifying all of the core color picking functionality within the KisToolUtils namespace's pickColor function.

Essentially all of the duplicate code was removed from KisToolColorPicker::pickColor, which now calls into the same KisToolUtils::pickColor function that's used elsewhere creating a single place in the code where things like sampling radius and color picker blending are implemented. As a result, any change to the code in KisToolUtils::pickColor will now affect all of Krita's color pickers (the dedicated tool [P], the ctrl-activated picker, and the scratchpad picker) in a uniform and predictable way.

While this patch does solve the most of the code-duplication issue that existed in Krita's color pickers, there are still a few ways in which the dedicated tool and the ctrl-picker behave inconsistently. In my view, more extensive refactoring can be done in the future to create more uniform and predictable behavior across the various pickers, resulting in more color picking code being moved into KisToolUtils::pickColor from elsewhere. For now though, I wanted to focus on code-reuse improvements that don't have any user-facing design implications.

Also, some minor code-style cleanup was done here and there.

Test Plan

Test the various color pickers (dedicated tool, ctrl-picker, scratch pad) for *mostly predictable and equivalent behavior.

*Certain aspects of the dedicated/ctrl picker can't be unified without some bigger changes, such as 'sample from merged/active only'.

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
emmetoneill requested review of this revision.May 17 2018, 7:16 AM
emmetoneill created this revision.
emmetoneill edited the summary of this revision. (Show Details)May 17 2018, 7:19 AM
dkazakov accepted this revision.May 17 2018, 1:28 PM

Hi, @emmetoneill!

Thank you for a patch! It looks and works perfectly good! :)
I'll push it now :)

This revision is now accepted and ready to land.May 17 2018, 1:28 PM
This revision was automatically updated to reflect the committed changes.

Great. Thanks again, Dmitry.