Hi all, (Sorry for the noise, seems like Reviewboard does not let me comment anymore?)
> https://git.reviewboard.kde.org/r/130219/ @albertfreeman: Thanks for providing a patch instead of just complaining on Bugzilla. As Reviewboard will become readonly in the near future, I would encourage you to bring this review over to Phabricator, so we can discuss further improvements (if you are still interested, which I hope, as the latest version of your patch is much improved). For example, you could improve the wording and layout: [ ] Use custom background color: [color chooser] (← Note inversion of checkbox logic, and disable colour chooser if checkbox is unchecked. Possibly decrease the colour chooser's width.) Advantages: Does away with the unmeaningful "Use UI theme", does not duplicate the words "background color" and is more compact (only one line) and less cluttered. @aacid: FWIW, I'm running a patched Okular with a different (hard coded) background colour since years, because basing the non-page background colour on the window background colour gives a very ugly result in my case (I don't think the code is to blame here, it's just impossible to find an algorithm which works for every possible colour). But I'm not alone, the following bugs have a total of 36 votes as of today: https://bugs.kde.org/show_bug.cgi?id=182994 https://bugs.kde.org/show_bug.cgi?id=307116 https://bugs.kde.org/show_bug.cgi?id=319736 https://bugs.kde.org/show_bug.cgi?id=372055 https://bugs.kde.org/show_bug.cgi?id=369627#c15 Looking at other apps like Gwenview or Libreoffice, those often allow to change the non-page background colour, too. As for the maintenance argument, I don't think it holds: 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. Cheers, Henrik
