Bug 368551 - KisScreenColorPicker doesn't update secondary color from KoDualColorButton
Summary: KisScreenColorPicker doesn't update secondary color from KoDualColorButton
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: Color Selectors (show other bugs)
Version: git master (please specify the git hash!)
Platform: Microsoft Windows Microsoft Windows
: NOR normal
Target Milestone: ---
Assignee: wolthera
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-10 13:51 UTC by Lee Zhen Yong
Modified: 2016-09-17 15:39 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
screenshot with pointers (72.25 KB, image/png)
2016-09-10 13:55 UTC, Lee Zhen Yong
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lee Zhen Yong 2016-09-10 13:51:13 UTC
In rKRITA93ef7c8 there's a brand new screen color picker that pops up when you click the buttons in the KoDualColorButton widget. But only the primary color button works. When the secondary color button is click, the same color picker dialog appears, but it doesn't update the secondary color when a new color is picked, and selecting color from the screen won't work (it'll stop you from clicking outside the modal dialog).

Reproducible: Always

Steps to Reproduce:
1. Click secondary color button in KoDualColorButton widget.
2. Try to pick a color from color wheel or pick a sample a pixel color from screen (teardrop icon)


Expected Results:  
The color picker should update the secondary color in KoDualColorButton widget.
Comment 1 Lee Zhen Yong 2016-09-10 13:55:17 UTC
Created attachment 101014 [details]
screenshot with pointers
Comment 2 Lee Zhen Yong 2016-09-10 14:01:25 UTC
the same dialog appears when selecting "Image Background Color" when creating new file. Oddly, the color picker doesn't update the color, but the color sampling from screen works.
Comment 3 Halla Rempt 2016-09-15 13:31:10 UTC
Hi Lee,

Thanks for your report. I can confirm the issue.
Comment 4 wolthera 2016-09-17 12:23:38 UTC
Uhm, I got the fix for the second, but I need a bit more information about the first. Is it that you expect the background color-picker to behave exactly like the foreground one?
Comment 5 wolthera 2016-09-17 12:25:27 UTC
Git commit c34c1d5e5343dab0d25af53f08bfb83543a3f53f by Wolthera van Hovell tot Westerflier.
Committed on 17/09/2016 at 12:19.
Pushed by woltherav into branch 'master'.

The visual color selector should probably sent out color updates, regardless of modality.
Ref T2438

M  +1    -0    libs/ui/widgets/kis_visual_color_selector.cpp

http://commits.kde.org/krita/c34c1d5e5343dab0d25af53f08bfb83543a3f53f
Comment 6 Lee Zhen Yong 2016-09-17 15:11:21 UTC
Cool. That fixes it!

"you expect the background color-picker to behave exactly like the foreground one?"

Shouldn't it? I'm not so sure. I'm actually pretty new to Krita in addition to the source code.

Another stuffs I noticed are minor:
- the color picker still updates the color when I choose "cancel". Not sure if that's suppose to happen.
- when I select a new color the color picker should show a comparison between the new and old color. The old color will always change to white or black depending whether I am changing the foreground or background.
- KoDualColorButton's foreground color updates as the color picker is selecting a new color, but the same doesn't apply for KoDualColorButton's background color. 

Sorry I'm putting it all in a single comment. Lazy to open another bug report.
Comment 7 wolthera 2016-09-17 15:39:13 UTC
Well, right now it isn't, because I made one a modal dialog and the other a non-modal one. I can make both the same type, of course.

But could you first update master, I made a lot of fixes today, including point 2.

Point 1 and 3 is the same as the modal/non-modal stuff. The foreground picker updates when you choose the color, not on cancel/ok, as those are only there for escaping the dialog when using gnome which thinks itself too cool for close buttons on dialogs windows.

You can also use T2438 on pabricator to comment.