Custom background color
ClosedPublic

Authored by albertfreeman on Sep 29 2017, 3:20 AM.

Details

Summary

BUG: 182994

Adds an option to the config dialog that enables background color (the color around the displayed page) to be changed (while by default preserving the Qt toolkit selection as not to affect existing users).

Reasons for this change:
Accessibility, eye strain, aesthetic reasons, color displayed on monitor can affect power consumption (how: depends on display technology).
Many people want this change occording to Bugzilla and other sources.

Maintenance: Nearly no additional maintenance:
This is no new subsystem but a trivial feature with no complex code dependencies, and we are already showing a colour selection dialog and setting colours in other places in Okular.

Other less important information:
https://git.reviewboard.kde.org/r/130219/
https://mail.kde.org/pipermail/okular-devel/2017-September/025520.html

Test Plan

Tested everything, it all works:
Toggled the custom background color, changed custom background color, removed okular settings file (with: "rm ~/.config/okular*") to verify it uses the usual qt theme colour by default (where the settings file remembered the custom color).

Diff Detail

Repository
R223 Okular
Lint
Lint Skipped
Unit
Unit Tests Skipped
albertfreeman created this revision.Sep 29 2017, 3:20 AM
Restricted Application added a project: Okular. · View Herald TranscriptSep 29 2017, 3:20 AM
albertfreeman edited the summary of this revision. (Show Details)Sep 29 2017, 3:27 AM
ngraham edited the summary of this revision. (Show Details)Sep 29 2017, 4:33 AM
ngraham added a subscriber: ngraham.

Nice!

Can you add some details of your testing to the Test Plan section? A screenshot of the new functionality is always very much appreciated.

albertfreeman edited the summary of this revision. (Show Details)Sep 29 2017, 4:58 AM
albertfreeman edited the test plan for this revision. (Show Details)

Can you please fix the summary so that it's not necessary to read the reviewboard entry?

albertfreeman retitled this revision from Enable custom background color to be enabled and changed from settings to Custom background color to be enabled and changed from settings.Sep 29 2017, 7:01 AM
albertfreeman retitled this revision from Custom background color to be enabled and changed from settings to Custom background color.
albertfreeman edited the summary of this revision. (Show Details)Sep 29 2017, 7:11 AM
albertfreeman added a subscriber: aacid.
albertfreeman edited the summary of this revision. (Show Details)Sep 29 2017, 7:14 AM
albertfreeman edited the summary of this revision. (Show Details)

Reverted changes of accelerators that my Qt Designer automatically made.

rkflx added a comment.Sep 29 2017, 5:58 PM

Thanks for bringing this to Phabricator and implementing my suggestion on such short notice. I tested your patch and cannot find anything wrong with it in terms of behaviour. For the code if have a trivial nitpick (see below) and I have a comment regarding the commit message. Please fix both, then I would be in favour of committing this. However, we should probably wait for @aacid's agreement here.

Can you please fix the summary so that it's not necessary to read the reviewboard entry?

I think you might have followed this advice a little bit too literal. The summary and test plan of this Diff will become the commit message. Therefore just a link (like it was before) is too little and a verbatim copy of the complete discussion (as it is now) is too much. Please change it to just summarize in a couple of sentences:

  • what is meant by the commit title (i.e. it adds an option to the config dialog which can be used to customize the background color around the displayed page)
  • why the change is useful (i.e. QPalette::Dark sometimes results in colours which are unpleasant, or users just want a different colour with changing it desktop-wide not being an option)
  • why the feature is relevant (i.e. many users voting on bugzilla, other apps also allow it)
  • what's the maintenance cost of the feature (i.e. nearly none)
  • who will be affected (i.e. only those users who opt to change the default)

This would help anyone using git blame to reason about the code in the future tremendously, as well providing context for your reviewers. Your test plan is already quite good, no need to change it. Keep the BUG: 182994 added by @ngraham, this will automatically close the bug once committed. Extra points for the screenshot :)

conf/okular.kcfg
298

Do not add this empty line.

Removed blank line from conf/okular.kcfg.

albertfreeman edited the summary of this revision. (Show Details)Sep 30 2017, 12:06 PM
ngraham accepted this revision.Sep 30 2017, 3:25 PM

The patch applies cleanly and the feature works well. Nice job!

@aacid and @rkflx, if there are no other objections, should we get this into 17.08, or just master?

This revision is now accepted and ready to land.Sep 30 2017, 3:25 PM
aacid added a comment.Sep 30 2017, 6:16 PM

The patch applies cleanly and the feature works well. Nice job!

@aacid and @rkflx, if there are no other objections, should we get this into 17.08, or just master?

This is not a bugfix, so if you decide it should be commited it should never end up in 17.08

Note i already disagreed on having this but since i stepped down from maintainership i won't complain too much if you decide to ignore me, but at please please rename the useCustomBackgroundColor slot to something different, it having the exact same name as Okular::Settings::useCustomBackgroundColor() is confusing. calling it something like setBackgroundColourButtonEnabled would make more sense in my opinion

ngraham requested changes to this revision.Sep 30 2017, 6:19 PM

@aacid thanks for the info. Master it is, then.

@albertfreeman, can you rename useCustomBackgroundColor() to something different? I agree with Albert that the "set" prefix would be better than "use" here.

This revision now requires changes to proceed.Sep 30 2017, 6:19 PM

Rename slot to setCustomBackgroundColorButton.

ngraham accepted this revision.Oct 1 2017, 1:10 AM
This revision is now accepted and ready to land.Oct 1 2017, 1:10 AM
This revision was automatically updated to reflect the committed changes.

this was committed with the wrong authorship. please revert and commit again with the right author.

Ok, reverted and repushed.

@albertfreeman I've got your email from the old review on phabricator. Unfortunately this review did not contain the details. How did you send this review here? Did you use arcanist?

I didn't use Arcanist. I recreated it via the phabricator.kde.org interface.

rkflx added a comment.Oct 1 2017, 9:43 AM

this was committed with the wrong authorship. please revert and commit again with the right author.

I already raised this in R223:6b5a7c9a1a00#2448402, because I thought in Phab I was supposed to raise issues with commits at said commit and not at the review? Please tell me what's the correct procedure, so I'll be doing it right in the future.

Both are

In D8051#151095, @rkflx wrote:

this was committed with the wrong authorship. please revert and commit again with the right author.

I already raised this in R223:6b5a7c9a1a00#2448402, because I thought in Phab I was supposed to raise issues with commits at said commit and not at the review? Please tell me what's the correct procedure, so I'll be doing it right in the future.

IIRC what you write on the commit is notified to the committer; if you write it on the review it reaches all the people in the review.

Sorry about that, guys. I just did arc land. Was that not the right approach to land this?

It is, but when you did arc patch <ID>, the patch committed locally ended up with you as author because the patch had no authorship information.
So yes, arc patch, then check the author and if incorrect fix it (see the message in R223:6b5a7c9a1a00dec994cfb03b0abf369539168758#2448469 ) and then you can do arc land. You can also do arc land --hold before a final recheck.