Fix sidebar labels being unreadable when selected or hovered over
ClosedPublic

Authored by ngraham on Sep 13 2017, 4:41 AM.

Details

Summary

BUG: 382139

Previously these labels were white-on-light-blue with light themes; now they use the correct theme color property, and become black appropriately.

Test Plan

Tested on KDE Neon (dev unstable). Verified that with the default Breeze color scheme, tab text is more readable when hovered over and then active but not in focus. See before and after:

Also tested these changes with all standard color schemes as well as the Oxygen theme to make sure that there were no visual regressions.

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
ngraham created this revision.Sep 13 2017, 4:41 AM
ngraham edited the test plan for this revision. (Show Details)
aacid edited edge metadata.Sep 14 2017, 9:57 PM

Could you give it a try with the oxygen style?

I did test with Oxygen, as that's a built-in theme. It looks fine:

aacid added a comment.Sep 16 2017, 4:41 PM

I did test with Oxygen, as that's a built-in theme. It looks fine:

I don't think that's the oxygen style, that may be the oxygen color theme.

okular -style oxygen

is what i mean

Oh I see. Here's how it looks with the Oxygen style:

aacid accepted this revision.Sep 19 2017, 7:22 AM
This revision is now accepted and ready to land.Sep 19 2017, 7:22 AM
rkflx added a subscriber: rkflx.Sep 19 2017, 7:48 AM

Great, one item less on my list of things to fix in Okular :)

@ngraham Just a suggestion for future Diffs: Have a look at git's 50/72 rule (…of thumb) and the reasoning behind it, as the title of your commit message is quite long, really ;-)

Also small coding style nit: superfluous parens could be removed, but not really important

ngraham updated this revision to Diff 19720.Sep 21 2017, 4:23 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)

Removed unnecessary parentheses and cleaned up the summary/commit message

ngraham updated this revision to Diff 19721.Sep 21 2017, 4:25 AM

Missed one more tiny formatting issue

rkflx accepted this revision.Sep 21 2017, 7:02 AM

Thanks. Title is still unchanged, but this was more of a hint for future contributions, anyway.

Given you are now listed as "KDE developer", I assume you can commit by yourself? I guess this should go into the bugfix branch (Applications/17.08), then merge to master.

ngraham retitled this revision from Improve text visibility for Okular sidebar tabs that are hovered-over or selected-but-out-of-focus when using Breeze color scheme to Fix sidebar labels being unreadable when selected or hovered over.Sep 21 2017, 12:55 PM
ngraham edited the summary of this revision. (Show Details)

Thanks Henrik! Yes, I have a developer account and should be able to commit now. I haven't had the time to actually set that up yet though. I'll look into it late tonight or tomorrow.

I managed to get arc set up. Am I basically going to do this?

  1. arc land --onto Applications/17.08
  2. git checkout master; git merge Applications/17.08

One potential issue: arc land --onto Applications/17.08 --preview shows a large number of extraneous commits that would be landed onto that branch, not just my own.

rkflx added a comment.EditedSep 22 2017, 8:55 AM

arc will squash and push all commits from the current branch which are not yet in the target branch, in your case all commits exclusive to master in addition to your patch. As you noticed correctly, that's not what you'll want. I'd probably prefer to move such a patch to Applications/17.08 locally via git first, to check everything worked and still compiles:

  • git checkout Applications/17.08, git pull
  • git cherry-pick master (or perhaps arcpatch-D7793, depending on your current state)
  • gitk, make, arc land --preview, …
  • arc land

Now for the merge (see also here):

  • git checkout master
  • git reset --hard origin/master to get rid of your original patch
  • git pull
  • git merge -Xours origin/Applications/17.08
  • gitk --all, git push --dry-run
  • git push
ngraham closed this revision.Sep 22 2017, 3:39 PM