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
