[Kde-pim] Review Request 125128: Don't hardcode colors, use system theme instead (for some at least)

Thu Sep 10 11:04:56 BST 2015

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

Looks like a good improvement to me, the individually configurable colors go back to the time where this wasn't available system-wide yet.

I'll leave the +2 to Laurent.

- Volker Krause

On Sept. 10, 2015, 9:15 a.m., Daniel Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125128/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2015, 9:15 a.m.)
> 
> 
> Review request for KDEPIM and Laurent Montel.
> 
> 
> Repository: kdepim
> 
> 
> Description
> -------
> 
> This patch does three things: firstly it makes sure that default message list colors always use KColorScheme instead of being some random hardcoded shades of blue, green and red. As a result the KMail default colors work on both light and dark widget color themes - currently when you use dark color theme you need to change at least the "Unread mail" color in the settings otherwise it's too dark and illegible.
> 
> Secondly it generates the 3 quotation color levels from KColorScheme (using QColor::darker()/QColor::lighter()), which again makes this usable by default with both light and dark widget themes without having to tweak the custom colors.
> 
> Finally it removes the color customization options for PGP signature/encryption and makes full use of KColorScheme colors again. The reason is that on dark theme (which has very light (white) text in most cases) users end up with white text on top of a light green/yellow background which again is very hard to lead due to low contrast. To fix this we would need to add options to specify not just background color but also foreground color for each of the signature types. However I don't think that there's anyone who ever changed the default KMail color schemes for the PGP boxes, for that reason I think that removing the customization option and always using background and foreground from the global color themes is better as it always ensures good contrast between text and background and simplifies the code. The only exception is the encryption box - there is no good color type in KColorScheme that would match the case so here I simply hardcoded the blue background + white text (which makes the text actually well legible on both dark and light color schemes, currently the light color scheme is hard to read due to low contrast between darkish blue background and black text). The other reason is that the encryption box is purely informative, so the color does not have any meaning on any color theme, while the PGP signature colors actually have an important meaning attached to them, so they should map to the KDE-wide color theme.
> 
> Implementation wise I tried to concentrate the default values into common classes removing some of the duplicated hardocded values.
> 
> 
> Diffs
> -----
> 
>   messagelist/messagelistutil.cpp b48123a 
>   messagecore/settings/messagecore.kcfg b1e3d52 
>   messagecore/settings/globalsettings_messagecore.kcfgc 945c4dc 
>   messagecore/messagecoreutil.cpp PRE-CREATION 
>   messagecore/messagecoreutil.h PRE-CREATION 
>   kmail/configuredialog/configureappearancepage.cpp 361b4e9 
>   kmail/editor/kmcomposereditorng.cpp afa18f7 
>   kmail/editor/widgets/cryptostateindicatorwidget.cpp 0164afa 
>   kmail/util.h f6a1c24 
>   kmail/util.cpp 31e0b03 
>   messagecore/CMakeLists.txt 281e683 
>   messageviewer/autotests/data/encapsulated-with-attachment.mbox.html 39464dc 
>   messageviewer/autotests/data/forward-openpgp-signed-encrypted.mbox.html 3b0b883 
>   messageviewer/autotests/data/html.mbox.html 1d11778 
>   messageviewer/autotests/data/htmlonly.mbox.html 1045018 
>   messageviewer/autotests/data/inlinepgpencrypted-appendix.mbox.html 3abc158 
>   messageviewer/autotests/data/inlinepgpencrypted.mbox.html 92f73f6 
>   messageviewer/autotests/data/no-content-type.mbox.html f0036df 
>   messageviewer/autotests/data/openpgp-encrypted-enigmail1.6.mbox.html 01ede4f 
>   messageviewer/autotests/data/openpgp-encrypted-partially-signed-attachments.mbox.html 39dcf30 
>   messageviewer/autotests/data/openpgp-encrypted-two-attachments.mbox.html 63a9be1 
>   messageviewer/autotests/data/openpgp-encrypted.mbox.html 94849b4 
>   messageviewer/autotests/data/openpgp-inline-charset-encrypted.mbox.html 8daf746 
>   messageviewer/autotests/data/openpgp-inline-signed.mbox.html 1743b65 
>   messageviewer/autotests/data/openpgp-signed-base64-mailman-footer.mbox.html bb92d8a 
>   messageviewer/autotests/data/openpgp-signed-encrypted-two-attachments.mbox.html 479c422 
>   messageviewer/autotests/data/openpgp-signed-encrypted.mbox.html e0858bb 
>   messageviewer/autotests/data/openpgp-signed-mailinglist.mbox.html 34512fc 
>   messageviewer/autotests/data/openpgp-signed-two-attachments.mbox.html 2c1eb6d 
>   messageviewer/autotests/data/signed-forward-openpgp-signed-encrypted.mbox.html 4dffcd8 
>   messageviewer/autotests/data/smime-encrypted-octet-stream.mbox.html 20b2dbf 
>   messageviewer/autotests/data/smime-encrypted.mbox.html 20b2dbf 
>   messageviewer/autotests/data/smime-signed-encrypted.mbox.html cf0e6fa 
>   messageviewer/autotests/data/text+html-maillinglist.mbox.html b35368e 
>   messageviewer/autotests/data/tnef-one-file.mbox.html 28acaf5 
>   messageviewer/autotests/data/tnef-two-files.mbox.html 8fcf6b3 
>   messageviewer/viewer/csshelperbase.h 5c15f24 
>   messageviewer/viewer/csshelperbase.cpp 70a9851 
> 
> Diff: https://git.reviewboard.kde.org/r/125128/diff/
> 
> 
> Testing
> -------
> 
> Tested on both light and dark Breeze themes, in both cases the default colors work reasonably well.
> 
> 
> File Attachments
> ----------------
> 
> Encrypted + signed
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/09/09/3b00783d-b8cc-4f40-a61f-3bcd0a5d93ae__encrypted-light.png
> Signed with untrusted/unknown key
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/09/09/1138e1c5-91f9-412c-b2f4-e80821f91ffb__signed-untrusted-light.png
> Signed with bad key
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/09/09/4bd51482-c7d6-4ad3-9cb2-6f0d078312d6__signed-bad-light.png
> 
> 
> Thanks,
> 
> Daniel Vrátil
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/