Differential D10763

Fix background color for image view toolbar (crop/red-eye)
ClosedPublic
Actions

Authored by huoni on Feb 23 2018, 7:42 AM.

Details

Reviewers
rkflx
ngraham
Group Reviewers
Gwenview
Commits
R260:14e8e10f0a5a: Fix background color for image view toolbar (crop/red-eye)
Summary

The slide-in toolbar in image view, used by the crop and red-eye tools, sets its content dynamically.
This content wasn't getting the app's palette, resulting in the background colour being fixed.
This was only a serious problem in light color schemes in fullscreen mode, where the background color of the tool stayed light but the text also went light (due to fullscreen being dark themed).

This patch simply manually sets the toolbar's content's palette when it changes.

Normal, before:

Normal, after:

Fullscreen, before

Fullscreen, after

Test Plan

First set your color scheme to something light (e.g. Honeycomb, Zion).
Go to View, press F4, click the Operations tab, click Crop or Red Eye Reduction.
Go to fullscreen. The toolbar should be dark.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
huoni requested review of this revision.Feb 23 2018, 7:42 AM
huoni created this revision.
huoni edited the summary of this revision. (Show Details)Feb 23 2018, 7:46 AM
huoni added reviewers: Gwenview, rkflx.
ngraham added a subscriber: ngraham.Feb 23 2018, 5:51 PM

Hmm, in the After Normal screenshot, I don't like how there's now no line or other visual separation between the footer and the new toolbar. The two elements seem to blend into one another. I kind of liked it better before, IMHO.

Full Screen view a definite improvement, though.

huoni updated this revision to Diff 27883.Feb 23 2018, 10:14 PM
  • Set toolbar to darker background

In order to set a darker background, we need to watch for palette change events and
re-modify the palette.

huoni edited the summary of this revision. (Show Details)Feb 23 2018, 10:15 PM
In D10763#212253, @ngraham wrote:

Hmm, in the After Normal screenshot, I don't like how there's now no line or other visual separation between the footer and the new toolbar. The two elements seem to blend into one another. I kind of liked it better before, IMHO.

Full Screen view a definite improvement, though.

Yeah having the distinction is probably good. How does it look now?

ngraham accepted this revision as: ngraham.Feb 23 2018, 10:20 PM
ngraham edited the summary of this revision. (Show Details)

Much better! Hope you don't mind that I re-arranged your screenshots; it's easier to see the differences between befores and afters by switching between them rapidly using the left and right arrow keys if they're adjacent.

I'll let @rkflx have a pass at this before we land it.

This revision is now accepted and ready to land.Feb 23 2018, 10:21 PM
huoni added a comment.Feb 23 2018, 10:23 PM

Hope you don't mind that I re-arranged your screenshots

No worries, I'll remember that for next time :)

rkflx requested changes to this revision.Feb 25 2018, 11:57 PM

Thanks, Nate already made sure the only visual complaint I had got fixed ;)

In D10649#211998, @huoni wrote:

It only took me 7 hours to figure out, but it's fixed. With one line of course :).

We all can relate… And as usual, the reviewer spoils the party (but only slightly).

lib/slidecontainer.cpp
70 ↗(On Diff #27883)

SlideContainer is a generic container, and e.g. SaveBar is derived from it, thus the darkening affects more than just the Crop and Red Eye tools. You don't see it, because SaveBar does its own colouring again. Nevertheless I'd be in favour of keeping the code base "tidy".

Grepping for dark( and fiddling with the values it seems to me that ToolContainerContent will * - Provide a darker background and is probably what we want. Could you place some/most/all of your code over there?

This revision now requires changes to proceed.Feb 25 2018, 11:57 PM
huoni updated this revision to Diff 28079.Feb 26 2018, 2:48 AM
  • Move code to proper location
  • Change background role instead of monitoring for palette changes
huoni updated this revision to Diff 28080.Feb 26 2018, 2:49 AM
  • Remove eventwatcher include
huoni added a comment.EditedFeb 26 2018, 2:53 AM
In D10763#214010, @rkflx wrote:

We all can relate… And as usual, the reviewer spoils the party (but only slightly).

One day I will submit a patch that is correct the first time. One day... :)

I think I've found a much better solution. Instead of dis-inheriting the palette, and manually setting a background color every time the app palette changes, we just set the background role to a darker shade. This way the palette is still inherited and therefore we don't need to watch for palette change events.
We're back to one line! Actually, negative two lines :P

This might also be a better solution over in D10781: Replace manual palette adjustment with background role (fullscreen info label).

huoni added a comment.Feb 26 2018, 2:54 AM

Unrelated, but I've found another bug - the auto-hiding toolbar in fullscreen does not autohide when the crop tool is active.

rkflx accepted this revision.Feb 26 2018, 10:28 PM

Yay, patch looks even better now.

This revision is now accepted and ready to land.Feb 26 2018, 10:28 PM
Closed by commit R260:14e8e10f0a5a: Fix background color for image view toolbar (crop/red-eye) (authored by huoni, committed by rkflx). · Explain WhyFeb 26 2018, 10:30 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28145

View Options

lib/documentview/documentviewcontroller.cpp

Loading...