Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/
---

(Updated Oct. 1, 2017, 6:34 a.m.)


Status
--

This change has been discarded.


Review request for Okular.


Repository: okular


Description
---

Enable background color to be changed from settings


Diffs
-

  CMakeLists.txt 0c5b7ac01 
  conf/dlggeneral.h f363d260a 
  conf/dlggeneral.cpp 964a655bc 
  conf/dlggeneralbase.ui cf4ebca09 
  conf/okular.kcfg 69ea8cf62 
  ui/pageview.cpp 9766422b9 


Diff: https://git.reviewboard.kde.org/r/130219/diff/4/


Testing
---


Thanks,

Albert Freeman

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/
---

(Updated Oct. 1, 2017, 6:33 a.m.)


Status
--

This change has been marked as submitted.


Review request for Okular.


Repository: okular


Description
---

Enable background color to be changed from settings


Diffs
-

  CMakeLists.txt 0c5b7ac01 
  conf/dlggeneral.h f363d260a 
  conf/dlggeneral.cpp 964a655bc 
  conf/dlggeneralbase.ui cf4ebca09 
  conf/okular.kcfg 69ea8cf62 
  ui/pageview.cpp 9766422b9 


Diff: https://git.reviewboard.kde.org/r/130219/diff/4/


Testing
---


Thanks,

Albert Freeman

Re: Review Request 130219: Enable background color to be changed from settings

[Henrik Fehlauer]

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

Re: Review Request 130219: Enable background color to be changed from settings

[Henrik Fehlauer]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/#review103747
---



- Henrik Fehlauer


On Sept. 17, 2017, 1:05 p.m., Albert Freeman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130219/
> ---
> 
> (Updated Sept. 17, 2017, 1:05 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Enable background color to be changed from settings
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 0c5b7ac01 
>   conf/dlggeneral.h f363d260a 
>   conf/dlggeneral.cpp 964a655bc 
>   conf/dlggeneralbase.ui cf4ebca09 
>   conf/okular.kcfg 69ea8cf62 
>   ui/pageview.cpp 9766422b9 
> 
> 
> Diff: https://git.reviewboard.kde.org/r/130219/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Freeman
> 
>

Re: Review Request 130219: Enable background color to be changed from settings

[Henrik Fehlauer]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/#review103745
---



- Henrik Fehlauer


On Sept. 17, 2017, 1:05 p.m., Albert Freeman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130219/
> ---
> 
> (Updated Sept. 17, 2017, 1:05 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Enable background color to be changed from settings
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 0c5b7ac01 
>   conf/dlggeneral.h f363d260a 
>   conf/dlggeneral.cpp 964a655bc 
>   conf/dlggeneralbase.ui cf4ebca09 
>   conf/okular.kcfg 69ea8cf62 
>   ui/pageview.cpp 9766422b9 
> 
> 
> Diff: https://git.reviewboard.kde.org/r/130219/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Freeman
> 
>

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/
---

(Updated Sept. 17, 2017, 1:05 p.m.)


Review request for Okular.


Changes
---

Cleaned up code


Repository: okular


Description
---

Enable background color to be changed from settings


Diffs (updated)
-

  CMakeLists.txt 0c5b7ac01 
  conf/dlggeneral.h f363d260a 
  conf/dlggeneral.cpp 964a655bc 
  conf/dlggeneralbase.ui cf4ebca09 
  conf/okular.kcfg 69ea8cf62 
  ui/pageview.cpp 9766422b9 


Diff: https://git.reviewboard.kde.org/r/130219/diff/4/

Changes: https://git.reviewboard.kde.org/r/130219/diff/3-4/


Testing
---


Thanks,

Albert Freeman

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

> On Aug. 9, 2017, 6:24 p.m., Albert Astals Cid wrote:
> > Thanks for contributing to Okular :)
> > 
> > Why would you need a different background color?
> 
> Albert Freeman wrote:
> I personally much prefer black as a background color.
> 
> Albert Astals Cid wrote:
> Maybe you could edit your color scheme in system settings instead?
> 
> Albert Freeman wrote:
> Through system settings I can change the okular background color but it 
> also affects the window color of the rest of okular and all other 
> applications. Additionally it seems to mix whatever color is selected with 
> grey for the final background color, so if I set it as RGB 0, 0, 0 black it 
> will appear a darker grey and if I set it RGB 255, 255, 255 white it will 
> appear a lighter grey. The rest of the okular window does not mix with grey 
> howevver. Should I try to make the system settings colors more granular in 
> whichever kde codebase is responsible AND require an okular code change to 
> adapt to that or just allow a choice of custom background color or system 
> color scheme in okular in a better way than my attached code does (since my 
> attached code is flawed)? Or should I just patch this manually to okular 
> source code just on my machine?
> 
> Albert Astals Cid wrote:
> I do appreciate that this is very important for you, but honestly i think 
> that following the color scheme is the right thing to do [on a side note, I 
> don't see how we change the color, all we do is directly call 
> QPainter:::fillRect( ..., backColor ); ]
> 
> You are right, adding another "different background" setting in system 
> settings color schemes doesn't make sense, how would you even name it?
> 
> At this point we have two options, either accept the code or not.
> 
> I have two problems with accepting the code:
> 
> * In my mind, It is a very niche feature
> * Once we accept the feature we have to maintain it for all of the 
> eternity (okular codebase is more than 10 years old and has gone though Qt3 
> to Qt4 porting and Qt4 to Qt5 porting) so even if it's "not much code", it 
> all adds up
> 
> I'm open to convincing that it's not a niche feature or that it's really 
> trivial to maintain for the next 10 years :)

Youtube video with ~100 upvotes and ~60'000 views (although it changes the 
pages color instead of the background 
color)(https://www.youtube.com/watch?v=_N7PgpkH8lM)

So the code is useful for accessibility, eye strain and aesthetic reasons.

Additionally, power consumption is minorly affected by color and depends on 
display technology, sometimes black uses more power, othertimes white does 
(https://www.quora.com/Does-a-white-background-use-more-energy-on-an-LCD-than-if-it-was-set-to-black)


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/#review103582
---


On Aug. 20, 2017, 2:59 p.m., Albert Freeman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130219/
> ---
> 
> (Updated Aug. 20, 2017, 2:59 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Enable background color to be changed from settings
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ab7ad239 
>   conf/dlggeneral.h f363d260 
>   conf/dlggeneral.cpp 964a655b 
>   conf/dlggeneralbase.ui cf4ebca0 
>   conf/okular.kcfg 69ea8cf6 
>   mobile/components/CMakeLists.txt f1af2602 
>   ui/pageview.cpp 3d935a2e 
>   ui/pageviewutils.cpp a57712ca 
> 
> 
> Diff: https://git.reviewboard.kde.org/r/130219/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Freeman
> 
>

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/
---

(Updated Aug. 20, 2017, 2:59 p.m.)


Review request for Okular.


Repository: okular


Description
---

Enable background color to be changed from settings


Diffs (updated)
-

  CMakeLists.txt ab7ad239 
  conf/dlggeneral.h f363d260 
  conf/dlggeneral.cpp 964a655b 
  conf/dlggeneralbase.ui cf4ebca0 
  conf/okular.kcfg 69ea8cf6 
  mobile/components/CMakeLists.txt f1af2602 
  ui/pageview.cpp 3d935a2e 
  ui/pageviewutils.cpp a57712ca 

Diff: https://git.reviewboard.kde.org/r/130219/diff/


Testing
---


Thanks,

Albert Freeman

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

> On Aug. 9, 2017, 6:24 p.m., Albert Astals Cid wrote:
> > Thanks for contributing to Okular :)
> > 
> > Why would you need a different background color?
> 
> Albert Freeman wrote:
> I personally much prefer black as a background color.
> 
> Albert Astals Cid wrote:
> Maybe you could edit your color scheme in system settings instead?

Through system settings I can change the okular background color but it also 
affects the window color of the rest of okular and all other applications. 
Additionally it seems to mix whatever color is selected with grey for the final 
background color, so if I set it as RGB 0, 0, 0 black it will appear a darker 
grey and if I set it RGB 255, 255, 255 white it will appear a lighter grey. The 
rest of the okular window does not mix with grey howevver. Should I try to make 
the system settings colors more granular in whichever kde codebase is 
responsible AND require an okular code change to adapt to that or just allow a 
choice of custom background color or system color scheme in okular in a better 
way than my attached code does (since my attached code is flawed)? Or should I 
just patch this manually to okular source code just on my machine?


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/#review103582
---


On Aug. 11, 2017, 2:50 a.m., Albert Freeman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130219/
> ---
> 
> (Updated Aug. 11, 2017, 2:50 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Enable background color to be changed from settings
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ab7ad239 
>   conf/dlggeneral.h f363d260 
>   conf/dlggeneral.cpp 964a655b 
>   conf/dlggeneralbase.ui cf4ebca0 
>   conf/okular.kcfg 69ea8cf6 
>   mobile/components/CMakeLists.txt f1af2602 
>   ui/pageview.cpp 3d935a2e 
>   ui/pageviewutils.cpp a57712ca 
> 
> Diff: https://git.reviewboard.kde.org/r/130219/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Freeman
> 
>

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Astals Cid]

> On Aug. 9, 2017, 6:24 p.m., Albert Astals Cid wrote:
> > Thanks for contributing to Okular :)
> > 
> > Why would you need a different background color?
> 
> Albert Freeman wrote:
> I personally much prefer black as a background color.

Maybe you could edit your color scheme in system settings instead?


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/#review103582
---


On Aug. 11, 2017, 2:50 a.m., Albert Freeman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130219/
> ---
> 
> (Updated Aug. 11, 2017, 2:50 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Enable background color to be changed from settings
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ab7ad239 
>   conf/dlggeneral.h f363d260 
>   conf/dlggeneral.cpp 964a655b 
>   conf/dlggeneralbase.ui cf4ebca0 
>   conf/okular.kcfg 69ea8cf6 
>   mobile/components/CMakeLists.txt f1af2602 
>   ui/pageview.cpp 3d935a2e 
>   ui/pageviewutils.cpp a57712ca 
> 
> Diff: https://git.reviewboard.kde.org/r/130219/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Freeman
> 
>

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

> On Aug. 9, 2017, 6:24 p.m., Albert Astals Cid wrote:
> > Thanks for contributing to Okular :)
> > 
> > Why would you need a different background color?

I personally much prefer black as a background color.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/#review103582
---


On Aug. 11, 2017, 2:50 a.m., Albert Freeman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130219/
> ---
> 
> (Updated Aug. 11, 2017, 2:50 a.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Enable background color to be changed from settings
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ab7ad239 
>   conf/dlggeneral.h f363d260 
>   conf/dlggeneral.cpp 964a655b 
>   conf/dlggeneralbase.ui cf4ebca0 
>   conf/okular.kcfg 69ea8cf6 
>   mobile/components/CMakeLists.txt f1af2602 
>   ui/pageview.cpp 3d935a2e 
>   ui/pageviewutils.cpp a57712ca 
> 
> Diff: https://git.reviewboard.kde.org/r/130219/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Freeman
> 
>

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/
---

(Updated Aug. 11, 2017, 2:50 a.m.)


Review request for Okular.


Repository: okular


Description
---

Enable background color to be changed from settings


Diffs (updated)
-

  CMakeLists.txt ab7ad239 
  conf/dlggeneral.h f363d260 
  conf/dlggeneral.cpp 964a655b 
  conf/dlggeneralbase.ui cf4ebca0 
  conf/okular.kcfg 69ea8cf6 
  mobile/components/CMakeLists.txt f1af2602 
  ui/pageview.cpp 3d935a2e 
  ui/pageviewutils.cpp a57712ca 

Diff: https://git.reviewboard.kde.org/r/130219/diff/


Testing
---


Thanks,

Albert Freeman

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/#review103582
---



Thanks for contributing to Okular :)

Why would you need a different background color?


conf/dlggeneralbase.ui (line 371)


Can you please not change the accelerators?



conf/okular.kcfg (line 294)


That's unacceptable, what about people that use a color theme different 
than yours?


- Albert Astals Cid


On Aug. 9, 2017, 12:59 p.m., Albert Freeman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130219/
> ---
> 
> (Updated Aug. 9, 2017, 12:59 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Enable background color to be changed from settings
> 
> 
> Diffs
> -
> 
>   conf/dlggeneralbase.ui cf4ebca0 
>   conf/okular.kcfg 69ea8cf6 
>   ui/pageview.cpp 3d935a2e 
> 
> Diff: https://git.reviewboard.kde.org/r/130219/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Albert Freeman
> 
>

Re: Review Request 130219: Enable background color to be changed from settings

[Albert Freeman]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130219/
---

(Updated Aug. 9, 2017, 12:59 p.m.)


Review request for Okular.


Repository: okular


Description
---

Enable background color to be changed from settings


Diffs
-

  conf/dlggeneralbase.ui cf4ebca0 
  conf/okular.kcfg 69ea8cf6 
  ui/pageview.cpp 3d935a2e 

Diff: https://git.reviewboard.kde.org/r/130219/diff/


Testing
---


Thanks,

Albert Freeman