D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham updated this revision to Diff 34884.
ngraham added a comment.


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

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12756?vs=34883=34884

BRANCH
  arcpatch-D12756

REVISION DETAIL
  https://phabricator.kde.org/D12756

AFFECTED FILES
  src/kdatetable.cpp

To: ngraham, #frameworks
Cc: cfeck, kde-frameworks-devel, mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks
Cc: cfeck, kde-frameworks-devel, mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham updated this revision to Diff 34883.
ngraham added a comment.


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

REPOSITORY
  R236 KWidgetsAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12756?vs=33824=34883

REVISION DETAIL
  https://phabricator.kde.org/D12756

AFFECTED FILES
  src/kdatetable.cpp

To: ngraham, #frameworks
Cc: cfeck, kde-frameworks-devel, mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham added a subscriber: cfeck.
ngraham added a comment.


  In D12756#268311 , @cfeck wrote:
  
  > 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).

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks
Cc: cfeck, kde-frameworks-devel, mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Christoph Feck]

cfeck resigned from this revision.
cfeck added a comment.


  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.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks
Cc: kde-frameworks-devel, mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham added a comment.


  Friendly ping!

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: kde-frameworks-devel, mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  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.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: kde-frameworks-devel, mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham added a comment.


  In D12756#259691 , @mwolff wrote:
  
  > 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`.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Milian Wolff]

mwolff added a comment.


  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...

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Milian Wolff]

mwolff added a comment.


  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.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: mwolff, apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Aleix Pol Gonzalez]

apol added a comment.


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

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: apol, michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham edited the summary of this revision.
ngraham edited the test plan for this revision.

REPOSITORY
  R236 KWidgetsAddons

REVISION DETAIL
  https://phabricator.kde.org/D12756

To: ngraham, #frameworks, cfeck
Cc: michaelh, ngraham, bruns

D12756: [KDateTable] Use a more visible red color

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: Frameworks, cfeck.
Restricted Application added a project: Frameworks.
ngraham requested review of this revision.

REVISION SUMMARY
  When I started work on this, I first wrote a conditional lightness check that 
switched between `Qt::red` and `Qt::darkRed` depending on the theme's window 
background color. That worked, but then noticed that `Qt::darkRed` isn't all 
that visible for Breeze light in the first place. `Qt::red` looks better and is 
more visible for both of them, so let's just use that instead!
  
  BUG: 389075
  FIXED-IN 5.47

TEST PLAN
  Breeze Light:
  
  Breeze Dark:

REPOSITORY
  R236 KWidgetsAddons

BRANCH
  kdatetable-more-visible-red-color (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D12756

AFFECTED FILES
  src/kdatetable.cpp

To: ngraham, #frameworks, cfeck
Cc: michaelh, ngraham, bruns