KMessageWidget : revert to using highlight colour for Information style
Needs RevisionPublic

Authored by rjvbb on Jun 28 2018, 11:03 AM.

Details

Reviewers
ngraham
Group Reviewers
Frameworks
VDG
Summary

A recent commit (00bce130d35e9dc398709e690a05f8dde70f52b3) modified the KMessageWidget look and feel. While I fail to see the point to make them look like their Kirigami equivalent this doesn't really bother me either.

The change did introduce 1 regression IMHO: the Information type background colour is now hardcoded instead of being based on the highlight colour as before. This is also the type I encounter most (in Kate, KDevelop etc).

The patch below reverts the colour hardcoding by obtaining the base colour from the current palette, as before.

Additionally, I propose to use the current widget palette instead of the global application palette, as we cannot know whether or not the code calling us installed a custom palette for the particular widget we're working with.

I've marked this WIP because I plan to use the occasion to investigate whether or not it's possible to drop some of the other hardcoded colours.

I am also open to discussion whether it would be more appropriate to use the user's tooltip colour (QPalette::ToolTipBase) for Information messages, or to introduce a type-specific darkening factor for the calculation of the final background colour.

Test Plan

Reverts the annoying blueish background of the new-style Information messages to a colour I picked myself.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 525
Build 536: arc lint + arc unit
rjvbb created this revision.Jun 28 2018, 11:03 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 28 2018, 11:03 AM
rjvbb requested review of this revision.Jun 28 2018, 11:03 AM
cfeck added a subscriber: cfeck.Jun 28 2018, 11:45 AM

here is WIP feedback: the comments explain the change, but they are not needed to explain the code. Remove them.

ngraham requested changes to this revision.Jun 28 2018, 1:00 PM

KWidgetsAddons is a Tier 1 framework, so it can't depend on any other KDE Frameworks, including KConfig, which is where the current theme's full palette is stored. Because of that, we're restricted to hardcoding the colors for the Positive, Warning, and Error styles rather than reading them from the current theme. My thinking was that since we're already forced to do this, it made more sense to just hardcode everything and preserve a consistent visual style until and unless we can somehow make it use the theme's colors.

I'm not necessarily against using the palette's highlight color for the Information widget, but I'd strongly prefer a solution that allows the use of all colors from the active theme, not just one.

We can't use the local widget's own palette, for reasons explained below in an inline comment.

src/kmessagewidget.cpp
263

Using the global application palette was deliberate. Try your change and then suspend a terminal window that has a dark background when you're using Breeze light. Local widgets may set a palette that's incompatible with some of the colors we're forced to hardcode due to KWidgetsAddons being a Tier 1 framework, or may set a palette that's incomplete and results in unreadable text. It's much safer to just use the app's own palette.

275

"as before" not needed. Really, @cfeck is right and the entire comment isn't needed.

282

Same

This revision now requires changes to proceed.Jun 28 2018, 1:00 PM
rjvbb updated this revision to Diff 36841.Jun 28 2018, 2:02 PM

This revision incorporates the feedback and implements the approach already evoked:

  • use the user's highlight colour for positive messages, with a 0.4 alpha value
  • use the user's tooltip background colour for informational messages. The alpha value is based on the psychometric (perceived) intensity of the selected colour and its difference with the widget background intensity: half the average of those values, limited by half the intensity itself.
  • warning and error messages remain orange and red respectively as those are commonly accepted uses

The final message background colour is determined as an average of the type-specific colour and the widget background colour, and should thus probably use the receiving widget's actual colour for best effects. Text colour is still taken from the application palette.

This scheme works quite nicely for me with my own palette, but also (IMHO) with the 4 Breeze palettes, the Oxygen palette and Krita Neutral. Breeze Dark apparently uses the same background colour for regular windows and tooltips. I rather like the fact that as a result, informational messages don't stand out any more than tooltips or like they do with the current implementation - I think that must have been the intent of the theme developer(s).

(A pity it's probably not possible to add a palette (theme) picker to the kmessagewidget test app?)

rjvbb set the repository for this revision to R236 KWidgetsAddons.Jun 28 2018, 2:02 PM
rjvbb marked 3 inline comments as done.
rjvbb added a comment.Jun 28 2018, 2:06 PM

5.47.0 (stock) vs. 5.42.0 (stock), QtCurve (top) vs. Macintosh native (below):

with revision 2 of my patch:

-1

The upper two in second revision look quite bad.

Another idea would have been to have the widget style alter the appearance like it already does for things like KTitleWidget

ngraham requested changes to this revision.Jun 28 2018, 2:25 PM

I'm not sure that's the right approach. This new version has bugs with Breeze light:

  • Suspend output in a Konsole window that has a dark background color: the text is unreadable.
  • All Information-style widgets have a gray background.
  • Positive-style widgets are now blue.

...And conceptual issues:

  • One of the goals was to match the colors of the Kirigami equivalent. Consistency is important.
  • There's no kind way to say this, but I don't think the new colors are attractive.
This revision now requires changes to proceed.Jun 28 2018, 2:25 PM
rjvbb added a comment.Jun 28 2018, 3:14 PM

Another idea would have been to have the widget style alter the appearance like it already does for things like KTitleWidget

Care to develop?

  • Suspend output in a Konsole window that has a dark background color: the text is unreadable.

By hitting ^S? I get a yellow background regardless of the palette but it is possible that you're right that I should just use the application palette. Or (or and) maybe text should take the background colour intensity or better yet, colour contrast into consideration. Because what would happen if someone uses a dark scheme with a light (green or blue) colour for regular text, wouldn't that also lead to unreadable text?

  • All Information-style widgets have a gray background.

That depends on your palette. They are grey only when you also use grey (or black or white) tooltips. If that's the background you chose for delivering one kind of informational feedback, why not also use it for other kind(s)?

  • Positive-style widgets are now blue.

And why is that a problem? If you don't like blue, use a different palette? I agree blue is a problem (for physiological reasons) but in general, not just for positive messages.

It would help here if there were documentation about what those messages are supposed to do. But conceptually speaking I think it's perfectly appropriate that they use the highlighting colour.

*This* is where consistency comes in, IMHO.

  • One of the goals was to match the colors of the Kirigami equivalent. Consistency is important.

I've said this before, the consistency argument doesn't fly here IMHO, or rather, it works against the current state of things. Kirigami uses QtQuick which is a completely different beast than QtWidget-based things, and depending on the version you use it will not even use your selected widget style. Kirigami is also the newcomer, and IMHO it's not up to existing software to adapt to newcomers but up to those to adapt to the existing environment.

  • There's no kind way to say this, but I don't think the new colors are attractive.

There's also no kind way to say that you don't have to think so because those are *my* colours. And I'm sure I find the Breeze colour scheme much more disgusting O:-)
No kind way either of saying that I don't use any kirithingy apps personally, in part because I don't care for the mobile-app look they have: form over function), and that I want to keep the subdued "professional" look I've achieved in the (KTextEditor based) applications I do use extensively.
I'm reminded of a quote from a KDE ML I read this morning, with which I sadly have to agree:

"the wonderful thing about the apparent current mindset of the KDE developers, that the KDE desktop is no longer something for end-users, but just a toy for the edification of its developers. Continuity? Usability?  Bah, who needs that?  Here's some eye-candy instead."

Let's avoid discussing tastes beyond the fact that one shouldn't impose a particular palette that isn't neutral (= grey scale).

I notice that Qt's example "Settings Editor" application opens and parses ~/.config/kdeglobals just fine:

That would mean that QSettings can indeed be used to get the colours to be used from that file. Which ones would we use?

broulik removed a subscriber: broulik.Jun 28 2018, 3:15 PM

This patch isn't the right place for discussing changing the conceptual color scheme for the widgets.

This patch also isn't the place for insulting the people who you're hoping will accept your changes. :)

If you want to implement a private method to get theme colors that doesn't rely on KConfig, you can use the ones that were in the original comments:

  • Window: ForegroundPositive for Positive
  • Window: ForegroundActive for Information
  • Window: ForegroundNeutral for Warning
  • Window: ForegroundNegative for Negative
rjvbb updated this revision to Diff 36845.Jun 28 2018, 5:01 PM

Version using the suggested colour values from ~/.config/kdeglobals , if the file exists and has a Colors:Window group.

In my case this gives informational and error messages almost the same background colour because I have configured a dark red colour for active foreground text (a darker colour than the negative foreground colour, in fact).

The advantage of an approach like in the previous version is that at least some message colours adapt when the application uses a custom palette (e.g. KDevelop or digiKam).

rjvbb set the repository for this revision to R236 KWidgetsAddons.Jun 28 2018, 5:01 PM
rjvbb updated this revision to Diff 36846.Jun 28 2018, 5:11 PM

cleanup

Thanks, I think this approach is more feasible! I have some comments below:

src/kmessagewidget.cpp
267

This commented-out line and its comments should be removed.

267

Never mind, you were too fast for me. :)

286

I don't think all of this complicated code is necessary. The theme itself is supposed to ensure readability with the colors that it uses. Also, this results in a slight regression for the Breeze Dark theme: the Information widget doesn't get enough alpha and is too bright, worsening readability compared to the status quo.

I just realized that we have another case in KWidgetsAddons that could benefit from your read-theme-colors-without-using-kconfig idea: D12756: [KDateTable] Use more appropriate and readable text colors for weekends and holidays

Would you mind putting it into a function and submitting it in another patch? I think it could be nice to have available more generally.

src/kmessagewidget.cpp
286

Also, commits should be atomic; even if we want to do this, it should be in another patch since it represents a separate conceptual change compared to the status quo, as opposed to simply a bugfix or missing feature.

rjvbb marked 2 inline comments as done.Jun 28 2018, 5:24 PM
rjvbb added inline comments.
src/kmessagewidget.cpp
286

The issue I have with using these colours is that they are foreground colours, to be used against one of 2 or 3 background colours. *That* is what the theme can be expected to ensure.

There is however no obstacle whatsoever to introducing a few new colour categories, specifically for this use. Only then can you blame the theme if things don't work out.

I can modify (and maybe simplify) the alpha calculation of course.

Here's what my theme gives with the tweaked alpha. Without that, information messages would be a darker red than error messages. That would be wrong, but I don't see how there's anything wrong with using a dark red for active text and a brighter red for negative. Red on grey works very nicely for text that should draw attention.

ngraham added inline comments.Jun 28 2018, 5:30 PM
src/kmessagewidget.cpp
286

That's your theme's problem. You used almost the same color for Negative and Active. Isn't the whole point of this to respect the user's theme? If the user chooses theme colors that are problematic, we can't help that.

Either way, we certainly can't include a workaround that's tailored to a problem introduced by your specific choice of theme colors.

rjvbb added inline comments.Jun 28 2018, 5:30 PM
src/kmessagewidget.cpp
286

What, the alpha tweak? Do you really want to split this up in a 1st commit that makes things worse for some followed by one that make things better than the status quo?

We'll see about that later, any splitting up is going to me the WIP and review more complicated (if not I could also submit 5 different reviews, 1 introducing the basic QSettings code, and one for using it for each of the message types...)

cfeck added inline comments.Jun 28 2018, 7:13 PM
src/kmessagewidget.cpp
261

missing if (rgb.size() >= 3)

271

missing space before ?

rjvbb updated this revision to Diff 36858.Jun 28 2018, 8:39 PM

The need for an extra check in qColorFromSettings() meant I changed the API to put all tests in there and added a default return value argument. IOW, basically the same API as KConfig has.

I've simplified the information alpha calculation, now looking only at the message background colour intensity. I think that should address the issue with dark themes.

rjvbb set the repository for this revision to R236 KWidgetsAddons.Jun 28 2018, 8:39 PM
cfeck added a comment.Jun 28 2018, 9:23 PM

I like the refactoring, much easier to read.

I have to agree with Nathan that the special casing for the alpha is unneeded. For users having a black selection color, it wouldn't tint at all.

We deliberately used a small blending value (0.2) to make sure the resulting color is not too different from the standard window background color.

src/kmessagewidget.cpp
264

>= 4

rjvbb updated this revision to Diff 36886.Jun 29 2018, 2:09 PM

A new revision:

  • makes a start with the requested export of the QSettings functionality
  • drops the alpha tweak and introduces a new experiment:

Since background colours are not obtained from theme colours designed for that purpose AND they're mixed with the widget background, I tried to use the hardcoded default colours as a reference. Instead of changing the selected theme colours in a qualitative fashion they are brightness-corrected to give them the same perceived intensity as the default colours as much as that is possible in RGB space. Basically this is greyscale normalisation done individually for each colour role, and the idea is to achieve a more consistent contrast pattern across message roles and themes.

The correction is activated by an env. variable (KMESSAGEWIDGET_BACKGROUND_BRIGHTNESS_MATCHED). I introduced to make testing comparisons easier (= avoid recompilation). The effect is very subtle with the Breeze themes (improves readability a bit for the dark ones, I think), stronger with my own theme. It may however have unexpected effects so maybe the switch should be preserved IF this experiment is maintained.

rjvbb set the repository for this revision to R236 KWidgetsAddons.Jun 29 2018, 2:09 PM
cfeck added a comment.Jun 29 2018, 5:35 PM

I don't understand. The brightness should be as close as possible to the brightness of the window background, that's why we only used a small blending factor. If your window text color is black, and your hightlight background is also black (with white highlight text), you see nothing?

rjvbb updated this revision to Diff 36939.Jun 30 2018, 10:10 AM

Using the theme's normal foreground colour for message text for more reliable readability with darker themes.

Brightness matching to the fallback colour should never occur here, at least not when the palette text colour is black (as it tends to be in my testing). Brightness matching would make the resulting colour just about as dark (matching to a darker colour almost always succeeds).

rjvbb set the repository for this revision to R236 KWidgetsAddons.Jun 30 2018, 10:11 AM

A few examples showing the subtle effect of brightness matching, comparing to the Breeze theme (the brightness reference):

Oxygen:

Obsidian Coast

My own theme:

rjvbb updated this revision to Diff 36943.Jun 30 2018, 12:44 PM

Turns out it's trivial to check if and what custom theme was activated through the KColorSchemeManager.

rjvbb set the repository for this revision to R236 KWidgetsAddons.Jun 30 2018, 12:44 PM
rjvbb marked 8 inline comments as done.

Thanks for your continued work on this. I continue to believe that in order to move forward, it needs to be broken up into separate atomic commits:

  • One to add a color parsing function to KWidgetsAddons so other local clients can use it
  • Another to adopt that functionality in KMessageWidget
  • A third one to add the background brightness introspection/calculation functionality
  • Fourth and fifth ones to adopt the brightness processing in KMessageWidget as well as in the Kirigami inlineMessage widget

Why all this work? Three good reasons:

  • Principle: commits should be atomic.
  • Practicality: the first two commits are non-controversial and will probably be committed quickly, and then we don't have to gate them on the result of a discussion about the more controversial brightness matching part.
  • Consistency: if we change the visual style of KMessageWidget, we need to make the same change in Kirigami's inlineMessage or else we're missing the point of why the original change was made in the first place--to keep consistency between the two implementations. A reasonable amount of consistency between Kirigami and QWidgets-based Desktop apps is important to avoid generating the perception that Kirigami is a "mobile first" framework that just produces "big dumb phone apps on the desktop." We want people to enthusiastically adopt Kirigami for desktop apps, and that won't happen if the same control looks and feels needlessly different from the QWidget version.
rjvbb updated this revision to Diff 37000.Jul 1 2018, 11:12 AM

KThemeSettings moved to its own header+implementation files, and extended so different groups can be read. I don't think it needs much else in terms of functionality or complexity, if anything. There's no point in exporting it anyway (so it has a very specific target audience).

Let's please do most of the preparatory work here so I don't have to juggle more than 1 review/diff that concerns multiple files in the same directory for longer than strictly necessary.

rjvbb set the repository for this revision to R236 KWidgetsAddons.Jul 1 2018, 11:12 AM
aacid added a subscriber: aacid.EditedJul 2 2018, 10:48 PM
This comment has been deleted.
src/kmessagewidget.cpp
272

I can't find the definition of ForegroundPositive in https://cgit.kde.org/kcmutils.git/tree/src/kdeglobals.kcfg am i missing something?

rjvbb added a comment.EditedJul 3 2018, 10:27 AM

Since there's been mention about aligning Kirigami and re: the (IMHO) controversial approach of using foreground (text) colours for background purposes:

Have you guys considered using the 4 colours in question only for the message text and outer frame, keeping the background intact (or possibly just a bit lighter or darker depending on its initial brightness and/or the theme/user's contrast setting)? The result should be more subtle (edit: less gaudy) and more reliably as readable as allowed by the theme.

I've done a very quick assessment, using the 0.2 alpha to darken dark window backgrounds and brighten light window backgrounds (using QColor::darker and QColor::lighter) which gives

That's Breeze vs. Breeze Dark vs. my custom theme.

two observations:

  • lighter background colours are brightened less progressively to avoid hitting pure white too quickly
  • a hair-thin black outline for the text could increase readability for some combinations but appears to be impossible with stylesheets. (This is true in general, not just for this experiment.)
ngraham added a reviewer: VDG.Jul 3 2018, 1:06 PM
ngraham requested changes to this revision.Jul 3 2018, 1:09 PM

Have you guys considered using the 4 colours in question only for the message text and outer frame, keeping the background intact (or possibly just a bit lighter or darker depending on its initial brightness and/or the theme/user's contrast setting)? The result should be more subtle (edit: less gaudy) and more reliably as readable as allowed by the theme.
[...]

That's Breeze vs. Breeze Dark vs. my custom theme.

I'm against this purely on visual grounds. That just doesn't look good, sorry. It also makes the link text very hard to read with Breeze dark, regressing something we were explicitly trying to fix with the current design.

I don't see how this patch can move forward as long as it's a mix of uncontroversial changes (use colors from the theme) and huge conceptual changes (totally change how colors are used and the color and alpha of the background). Allow me to repeat my request to split this up into multiple patches so that we can move forward with something. If you need some help juggling multiple patches using arc, I'd be happy to lend a hand.

This revision now requires changes to proceed.Jul 3 2018, 1:09 PM
rjvbb added a comment.Jul 3 2018, 3:37 PM

Seems my reply per email went AWOL:

This is a work-in-progress ticket, but I can change the title because it is indeed not just about reverting a regression.

That just doesn't look good, sorry.

Again, that's an argument one should avoid. It's too subjective. I myself find it looks much better (I'm biased against everything Breeze, sorry) and easier on the eyes because I prefer almost monochromatic palettes.

Yes, I said that the current implementation looks too gaudy and that's subjective too. What I think I *can* claim is that using a background colour that comes from the theme's own background colours makes message widgets fit much better within the theme's design. And of course it avoids violating a basic, common-sensical and straightforward instruction from KColorScheme: don't use foreground colours for background purposes. See also my remark below about impaired colour vision.

I have no readability problems with Breeze Dark (not any more than with dark themes in general). It should be possible to lighten or darken the text colour depending on contrast, to achieve a stronger contrast. Not very easy though due to (perceived) colour contrast effects. For instance:

This does remind me of the fact that I have normal colour vision but a significant part of the population has impaired colour vision. It's tricky to start mixing colours like the original code does, and guarantee that the resulting colour pair can be distinguished sufficiently well by everyone. Simple brightening/darkening should be safer but not 100% so either, probably. In short, themes designed for specific visual shortcomings should continue to work better if you only use colours from those themes, and for their intended purposes.

Re: splitting:
If the KThemeSettings class looks more or less like what you had in mind and something that can go in quickly I'll split that off and then we can take things from there.

ngraham added a comment.EditedJul 3 2018, 4:52 PM

That just doesn't look good, sorry.

Again, that's an argument one should avoid. It's too subjective. I myself find it looks much better (I'm biased against everything Breeze, sorry) and easier on the eyes because I prefer almost monochromatic palettes.

If that were true, VDG couldn't exist. One of the major reasons why it does is so that people with recognized expertise in visual design can make design decisions and provide visual advice so that others who are better at back-end programming don't have to burden themselves with those tasks themselves. It's incumbent upon each of us to be humble and know our own limitations: just as more front-endy people shouldn't try to dictate implementation details, more back-endy people shouldn't question and fight against visual design decisions. And that's important.

I would argue that this is doubly true if you can acknowledge that you have a bias against the default appearance. If this is true, it's highly likely that any design changes you propose will (even unconsciously) represent a departure from the style that you don't like. If we accept those kinds of changes, the result is a jumbled mess of divergent styles rather than a cohesive whole--which is a worse outcome than a visual style that you don't like but is cohesive and consistent. As a Mac user, you're probably familiar with this principle because it's what Apple does very well. Not everybody loves the way it looks and feels, but it's at least consistent.

I therefore once again recommend that you split this up into multiple patches so that we can separate the non-controversial elements (use colors from the theme) from the controversial ones (alter the concept behind the widget background color). The alternative is that the discussion in this patch goes in circles for days until everyone is exhausted and nothing is merged. Let's try to move forward with what we can agree on instead of arguing with one another and getting entrenched in our positions.

rjvbb retitled this revision from KMessageWidget : revert to using highlight colour for Information style (WIP) to KMessageWidget : revert to using highlight colour for Information style.Nov 13 2018, 10:37 AM