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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request 125128: Don't hardcode colors, use system theme instead (for some at le
From:       "Volker Krause" <vkrause () kde ! org>
Date:       2015-09-10 10:04:56
Message-ID: 20150910100456.24667.64085 () mimi ! kde ! org
[Download RAW message or body]

-----------------------------------------------------------
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@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread] 