Improve background color code for URL Navigator and adjacent fullscreen toolbars
ClosedPublic

Authored by huoni on Feb 27 2018, 2:10 AM.

Details

Summary

Similar to D10781: Replace manual palette adjustment with background role (fullscreen info label), modifying background role to get a dark background instead of
manually adjusting palette is a preferred solution. This way the palette is still
inherited so does not need to be continually adjusted when the overall palette
changes, like when switching to/from fullscreen.

This also makes the entire fullscreen toolbar (including URL nav) have a consistent background color

Fullscreen left edge before:

Fullscreen left edge after:

Fullscreen right edge before:

Fullscreen right edge after:

Test Plan

In Browse, the URL Navigator should have a darker background color than the rest of the UI.

In Fullscreen Browse, the entire toolbar should have a consistent, dark, background color.

Diff Detail

Repository
R260 Gwenview
Branch
url-nav-palette (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
huoni requested review of this revision.Feb 27 2018, 2:10 AM
huoni created this revision.
huoni added a comment.EditedFeb 27 2018, 2:14 AM

@rkflx

Any other places currently using dark()or light()?

This is the only other place that is easily replaced with the background role. There are more complicated uses in previewitemdelegate.cpp and contextbarbutton.cpp which may not be easily changed.
I will address the other two points separately. edit: also address in this patch

huoni updated this revision to Diff 28165.Feb 27 2018, 2:31 AM
  • Bring fullscreen toolbars' background in line with URL navigator

I decided to combine this change here as they kind of go together

huoni retitled this revision from Replace manual palette adjustment with background role (URL Nav) to Improve background color code for URL Navigator and adjacent fullscreen toolbars.Feb 27 2018, 2:33 AM
huoni edited the summary of this revision. (Show Details)
rkflx added a subscriber: ngraham.Feb 28 2018, 7:44 PM

The quest for perfection continues ;) (Although at some point we should probably also focus on "actual" problems Gwenview's users struggle with…)

Change LGTM, just needs the rebase as mentioned in D10881.

I decided to combine this change here as they kind of go together

+1


@ngraham This makes the colour in windowed mode slightly more saturated. Just wanted to ask if you are okay with that:

#flatpakicons


@rkflx

Any other places currently using dark()or light()?

This is the only other place that is easily replaced with the background role. There are more complicated uses in previewitemdelegate.cpp and contextbarbutton.cpp which may not be easily changed.

ContextBarButton is not used anymore, see D7988 where we fought similar colour issues. At this time we were not sure whether we were allowed to remove the class completely, meanwhile I think we are (see D8785#176817). Another option would be to bring it back and fix the issues it had in another way. Depends on whether we want standard overlay buttons or custom ones (nicer look, but has maintenance issues in the long run).

Let's not change PreviewItemDelegate for now, we'll have to see what to do with it wrt. HiDPI fixes anyway.

ngraham accepted this revision as: ngraham.Feb 28 2018, 7:48 PM

Seems fine, no objections.

This revision is now accepted and ready to land.Feb 28 2018, 7:48 PM
huoni updated this revision to Diff 28299.Feb 28 2018, 10:56 PM
  • Rebase
huoni updated this revision to Diff 28301.EditedFeb 28 2018, 10:59 PM
  • rebase properly

Upstream keeps ending up set to master instead of origin/master, so I only rebased on local master before.

rkflx accepted this revision.Feb 28 2018, 11:02 PM

properly

Indeed ;)

This revision was automatically updated to reflect the committed changes.
huoni added a comment.Mar 1 2018, 1:59 AM

The quest for perfection continues ;) (Although at some point we should probably also focus on "actual" problems Gwenview's users struggle with…)

I do think ironing out all the little things does a lot for perception of quality. At least for me as a user, 'polish' makes a big difference.
You're probably right though. I'll start looking into more serious issues :)

The quest for perfection continues ;) (Although at some point we should probably also focus on "actual" problems Gwenview's users struggle with…)

I do think ironing out all the little things does a lot for perception of quality. At least for me as a user, 'polish' makes a big difference.

I strongly agree.

In terms of bigger features, one of the things that I think Gwenview needs is an inline image tweaker/editor. One is actually under construction as a re-usable KPart for inclusion in Spectacle: https://github.com/DamirPorobic/KImageAnnotator (see also T6321: [WISH] Create a quick image editor for Spectacle)

It might be worth it to join that effort and add some photo-centric featured (e.g. light levels, sepia, saturation, sharpness, fun effects, etc) and then include it in Gwenview, too. This would help Gwenview users stay within the app for simple image adjustments and not have to edit their photos in GIMP, which anyway has somewhat user-unfriendly features for this kind of thing.

huoni added a comment.Mar 1 2018, 11:27 PM

In terms of bigger features, one of the things that I think Gwenview needs is an inline image tweaker/editor. One is actually under construction as a re-usable KPart for inclusion in Spectacle: https://github.com/DamirPorobic/KImageAnnotator (see also T6321: [WISH] Create a quick image editor for Spectacle)

It might be worth it to join that effort and add some photo-centric featured (e.g. light levels, sepia, saturation, sharpness, fun effects, etc) and then include it in Gwenview, too. This would help Gwenview users stay within the app for simple image adjustments and not have to edit their photos in GIMP, which anyway has somewhat user-unfriendly features for this kind of thing.

Well I can get behind that effort! I've really wanted a simply annotator in Spectacle for a while.
Bit of a step up from my simple fixes so far, but I'll definitely have a look!