Make KMessageWidget match Kirigami inlineMessage's style
ClosedPublic

Authored by ngraham on Apr 25 2018, 2:54 AM.

Details

Summary

This patch makes the venerable and wonderful KMessageWidget match the slick style of Kirigami's new inlineMessage control. As a by-product, it fixes a few usability bugs.

I didn't implement the drop shadow here because I couldn't get it to look right.

BUG: 385299
BUG: 381255

Test Plan

Spectacle, Breeze light, information:

Spectacle, Breeze dark, information:

Spectacle, Breeze light, positive:

Spectacle, Breeze dark, positive:

Dolphin, Breeze light, negative:

Dolphin, Breeze dark, negative:

Dolphin, Breeze light, warning:

Konsole, Breeze light with dark profile theme, warning:

Konsole, Breeze dark with light profile theme, information:

Konsole, Breeze light, information:

Kate, Breeze light, positive:

Kate, Breeze dark, information:

Diff Detail

Repository
R236 KWidgetsAddons
Branch
arcpatch-D12508
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.Apr 25 2018, 2:54 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 25 2018, 2:54 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
ngraham requested review of this revision.Apr 25 2018, 2:54 AM
ngraham edited the summary of this revision. (Show Details)Apr 25 2018, 2:57 AM
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)Apr 25 2018, 3:33 AM
ngraham added a subscriber: jnoack.Apr 25 2018, 3:39 AM
elvisangelaccio requested changes to this revision.Apr 25 2018, 8:20 AM
elvisangelaccio added a subscriber: elvisangelaccio.
elvisangelaccio added inline comments.
CMakeLists.txt
20 ↗(On Diff #33028)

Unfortunately we can't do this, kwidgetsaddons is tier1 and it cannot depend on another framework.

We'll need to duplicate the relevant code from kconfigwidgets here.

This revision now requires changes to proceed.Apr 25 2018, 8:20 AM
ngraham updated this revision to Diff 33063.Apr 25 2018, 1:00 PM

Remove KConfigWdgets dependency and revert to hardcoded background colors

What a shame; this means that it won't inherit background colors from the active theme. I guess that's why it was implemented this way before. I've reverted that part and added a comment into the code so future folks aren't tripped up as I was.

Still more red than green in this diff though!

ngraham marked an inline comment as done.Apr 25 2018, 1:02 PM
ngraham edited the summary of this revision. (Show Details)Apr 25 2018, 1:07 PM
mart added a subscriber: mart.Apr 25 2018, 3:43 PM
mart added inline comments.
CMakeLists.txt
20 ↗(On Diff #33028)

if one doesn't fear horrible and beoken code... the color scheme could be read by QSettings... tough not sure if the hack is worth it

cfeck requested changes to this revision.EditedApr 25 2018, 3:44 PM
cfeck added a subscriber: cfeck.

The gradient was a remnant from the Oxygen days. Glad that it's gone.

Could you please restore the comments that state where the colors were taken from?

EDIT: Ah, you added a separate comment above...

src/kmessagewidget.cpp
294

If you use setAlphaF(0.2) you don't need to explain the value.

This revision now requires changes to proceed.Apr 25 2018, 3:44 PM
abetts added a subscriber: abetts.Apr 25 2018, 3:52 PM

+1, looking good. Keep up the work!

ngraham updated this revision to Diff 33089.Apr 25 2018, 5:12 PM

Address @cfeck's review comments

The gradient was a remnant from the Oxygen days. Glad that it's gone.

Could you please restore the comments that state where the colors were taken from?

EDIT: Ah, you added a separate comment above...

I did, but I decided to take your advice and add more anyway for maximum clarity.

ngraham marked an inline comment as done.Apr 25 2018, 5:14 PM
ngraham added a subscriber: anemeth.

Can we maybe do something in Breeze widget style to override the colors with theme colors? Breeze does some special behavior for other widgets as well like KTitleWidget. Might be worth looking into?

Can we maybe do something in Breeze widget style to override the colors with theme colors? Breeze does some special behavior for other widgets as well like KTitleWidget. Might be worth looking into?

An interesting idea. Worth mentioning that nothing's being lost here, though; the colors were already hardcoded. Making them respect the theme would be lovely, but could be done as a separate patch after (if?) this goes in.

Sure! It has to work without Breeze and on other platforms anyway but for a unified look we could add some special Breeze code on top maybe :)

ngraham edited the summary of this revision. (Show Details)Apr 25 2018, 5:45 PM
cfeck accepted this revision.Apr 25 2018, 7:06 PM
cfeck added a comment.EditedApr 25 2018, 7:12 PM

Regarding the Konsole isse, it may not be the only application expecting a window background. Maybe some autoFillBackground helps, see http://doc.qt.io/qt-5/qwidget.html#autoFillBackground-prop

Imo KMessageWidget should not rely on being placed where there's enough contrast. The transparency just doesn't work in the case of Konsole.
Either Konsole needs to move the widget outside the terminal painting area which looks complicated to achieve or the message widget should provide some API
which apps can use in cases where the background has not enough contrast.

Regarding the Konsole isse, it may not be the only application expecting a window background. Maybe some autoFillBackground helps, see http://doc.qt.io/qt-5/qwidget.html#autoFillBackground-prop

I just tried setting autoFillBackground: The background is not animated, so it breaks the animation of the message widget. Also it is painted black. I guess those things could be fixed within the KMessageWidget.

The question is what makes more sense, fixing each application where the message widget looks broken or fixing the message widget to look not broken. 🤔

Imo KMessageWidget should not rely on being placed where there's enough contrast. The transparency just doesn't work in the case of Konsole.

Hmm, I see your point. Let me see what I can do with it...

I'm working on setting the background color within the KMessageWidget itself, which should make these implementation issues in individual apps non-blockers.

ngraham updated this revision to Diff 33254.Apr 28 2018, 3:13 PM

set autoFillBackground, which fixes most client use cases except for when the parent widget's palette's background color matches the theme's text color (as with a dark Konsole theme on Breeze light)

ngraham updated this revision to Diff 33265.Apr 28 2018, 8:23 PM

Further improvements; always draw the theme background color under the KMessageWidget, which handles the case of the theme and active window defining different colors.

One remaining issue: the KMessageWidget now has a 1px border around it that may be the wrong color.

ngraham planned changes to this revision.Apr 28 2018, 8:24 PM
ngraham edited the summary of this revision. (Show Details)
elvisangelaccio resigned from this revision.Apr 29 2018, 3:15 PM

I'll just leave my +1 to the visual change.

ngraham updated this revision to Diff 33332.Apr 30 2018, 1:34 PM

Match inlineMessage with border thickness and corner roundness

This revision is now accepted and ready to land.Apr 30 2018, 1:34 PM
ngraham planned changes to this revision.Apr 30 2018, 1:45 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 33786.May 7 2018, 6:37 PM

Pre-generate the final background color to resolve extra border issue

This revision is now accepted and ready to land.May 7 2018, 6:37 PM
ngraham edited the test plan for this revision. (Show Details)May 7 2018, 6:57 PM
cfeck accepted this revision.May 7 2018, 7:56 PM

My "ship it" still stands, especially after you resolved the transparency issue :)

Thanks! I'm waiting for at least @hein before landing it.

abetts added a comment.May 7 2018, 8:35 PM

+1 on this!

hein accepted this revision.May 8 2018, 5:33 AM

Looks good to me, thanks for working on this.

ngraham closed this revision.May 8 2018, 1:14 PM

Can we revert the setIcon() part? It changes application behavior, an in the case of Kate/KWrite, this is note wanted, see my comments.

Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptMay 10 2018, 6:59 PM

Somehow my other comments were lost, here we go:

  • could you also update the screenshot in the doxygen documentation?
  • setIcon() is behavior incompatible, and in fact, the referenced bugs did not complain about icons. So why the change? In my opinion this is not good enough in case of Kate/KWrite.
  • setIcon(): Now the API documentation in the header file is wrong, since it says by default no icon is set.

@ngraham Could we have another revision?

Sure, I'll be happy to fix those issues.

The additional setIcon() call is to match the behavior of the Kirigami version, which always has an icon by default. I can revert that if we're okay with having it be a little bit inconsistent with the Kirigami version.

I think the setIcon() is also buggy, since if you call setIcon() and then setMessageType(), the icon is lost. So yes, I would be in favor of a better solution.

If you revert the setIcon() part, do the screenshot examples all loose their icon? Or is it still set explicitly in the respective application code?