[KDateTable] Use more appropriate and readable text colors for weekends and holidays
Needs ReviewPublic

Authored by ngraham on May 8 2018, 2:26 PM.

Details

Reviewers
cfeck
Group Reviewers
Frameworks
VDG
Summary

Use a conditional lightness check to determine the color for the non-working day numbers, and switch between two grays that each connote "not a work day" and look readable against dark and light backgrounds.

BUG: 389075
FIXED-IN 5.47

Test Plan

Breeze Light:

Breeze Dark:

Diff Detail

Repository
R236 KWidgetsAddons
Branch
arcpatch-D12756
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.May 8 2018, 2:26 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 8 2018, 2:26 PM
ngraham requested review of this revision.May 8 2018, 2:26 PM
ngraham edited the summary of this revision. (Show Details)May 8 2018, 2:28 PM
ngraham edited the test plan for this revision. (Show Details)
apol added a subscriber: apol.May 8 2018, 3:06 PM

Shouldn't we be using a color from the palette/color scheme?

We can't because KWidgetsAddons is a tier 1 framework and can't rely on any other frameworks, and the positive/negative colors come from KColorScheme rather than anywhere in Qt.

mwolff added a subscriber: mwolff.May 8 2018, 6:34 PM

Maybe instead use the HighlightText QPalette color? Hardcoding red may work for the two styles you present, but I could just set the view background to red and the text becomes unusable.

That would change the color to something different (e.g. with the Breeze themes, it's blue). If we're okay with that I can do it, but I was trying to produce as minimal a patch as possible.

mwolff added a comment.May 8 2018, 7:14 PM

Let's try to fix the bug for real, instead of implementing half-baked workarounds that only work for the default configurations.

Alternative ideas: add setters for the two possible cell text colors, then somehow set the KColorScheme color from the outside.

We really should upstream more of KColorScheme to Qt/QPalette...

Let's try to fix the bug for real, instead of implementing half-baked workarounds that only work for the default configurations.

Alternative ideas: add setters for the two possible cell text colors, then somehow set the KColorScheme color from the outside.

We really should upstream more of KColorScheme to Qt/QPalette...

That would require changes to all clients, which doesn't seem desirable. "Fixing the bug for real" probably looks like making KWidgetsAddons not a tier 1 frameworks anymore so it can use KColorScheme.

FWIW, we ran into the same issue with the recent visual update to KMessageWidget: D12508. The only practical near-term options here are to hardcode colors, or start depending on KColorScheme.

So what's our path forward? I'd prefer to land this as-is since it does indeed fix the bug for now, and then we can have a larger discussion regarding what to do about the fact that KWidgetsAddons can't access all the colors from the active scheme, which is an issue for multiple things, not just this thing here.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 15 2018, 10:14 PM

Friendly ping!

cfeck resigned from this revision.May 25 2018, 7:15 PM

I will not block this change, but cannot approve it either.

Reasons:

  • The pure red is looks too saturated, as if something dangerous is about to happen. Maybe add the VDG as a reviewer.
  • We should not use hardcoded colors on varying backgrounds. While the chance that someone uses Qt::red as a background color is zero, the code _should_ handle the dark vs. bright case as stated in the bug report.
ngraham added a subscriber: cfeck.May 25 2018, 7:32 PM

I will not block this change, but cannot approve it either.

Reasons:

  • The pure red is looks too saturated, as if something dangerous is about to happen. Maybe add the VDG as a reviewer.

I understand, and agree. I'm also not sure that red is the right color here in the first place.

  • We should not use hardcoded colors on varying backgrounds. While the chance that someone uses Qt::red as a background color is zero, the code _should_ handle the dark vs. bright case as stated in the bug report.

OK, I'll implement that part at least (even if for now we keep the colors identical, whatever we choose).

ngraham updated this revision to Diff 34883.May 25 2018, 9:05 PM

Use a conditional lightness check but the same colors, pending a better color option (TBD)

ngraham edited the summary of this revision. (Show Details)May 25 2018, 9:08 PM
ngraham updated this revision to Diff 34884.May 25 2018, 9:19 PM

Use conditional grays instead of reds (red was never an appropriate color anyway)

ngraham retitled this revision from [KDateTable] Use a more visible red color to [KDateTable] Use more appropriate and readable text colors for weekends and holidays.May 25 2018, 9:20 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham added a reviewer: VDG.
ngraham edited the summary of this revision. (Show Details)May 26 2018, 3:12 AM
rkflx added a subscriber: rkflx.EditedMay 26 2018, 6:20 AM

Use conditional grays instead of reds (red was never an appropriate color anyway)

Hm, now Sundays look like they belong to the previous/next month. (Do we even need to change colour everywhere, or would it be enough to only highlight the weekend in the header? Edit: Hm, I think that's a feature, so should probably be kept…)

You could also look at the way KDE3 styled the header (see https://edu.kde.org/kstars/feature/SetTime.png).

I don't know. Perhaps it's time to involve more VDG people. VDG, how should we style weekend days and day names? Currently we draw them in dark red, which is problematic for a few reasons:

(This screenshot also exposes another bug: in my locale, Saturdays are inexplicably marked as work days, even though the "sat" header gets the correct color)

alex-l added a subscriber: alex-l.May 26 2018, 5:59 PM

Gray seems OK to me. And I confirm the bug with Saturday column.

cfeck removed a reviewer: VDG.May 31 2018, 6:54 PM
QColor c;
c = QColor(Qt::gray);
qDebug() << c.lightnessF();
c = QColor(Qt::darkGray);
qDebug() << c.lightnessF();

I get:
0.635294
0.501961

Both colors are very similar and neither has enough contrast for background colors with lightness at 0.55.

Additionally, I would prefer computing the brightness of a color using qGray() or something similar to account for the different luminances of the primary colors.

cfeck added a reviewer: VDG.May 31 2018, 6:55 PM

I love how phabricator thinks I wanted to remove anyone.