D8051: Custom background color

[Henrik Fehlauer]

rkflx added a comment.


  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 :)

INLINE COMMENTS

> okular.kcfg:298
> +  </entry>
> +
>   </group>

Do not add this empty line.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D8051

To: albertfreeman, #okular, aacid, elvisangelaccio, rkflx
Cc: aacid, ltoscano, ngraham
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20170929/63353358/attachment.html>