[KMessageWidget] Draw it with QPainter instead of using stylesheet
ClosedPublic

Authored by davidre on Jan 30 2020, 2:39 PM.

Details

Summary

While being a prepatory step to KMessageWidget respecting the current color
scheme via KStyle this also simplifies this widget by a huge amount. This
includes removing the nested content Frame and the indirection via a QPixmap
for the animations.

Test Plan

kmessagewidgettest works as before
Before:


After:

Diff Detail

Repository
R236 KWidgetsAddons
Branch
no-css (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21855
Build 21873: arc lint + arc unit
davidre created this revision.Jan 30 2020, 2:39 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 30 2020, 2:39 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
davidre requested review of this revision.Jan 30 2020, 2:39 PM
davidre edited the test plan for this revision. (Show Details)Jan 30 2020, 2:39 PM
ngraham accepted this revision.Jan 30 2020, 2:48 PM
ngraham added a subscriber: ngraham.

Very nice. Much simpler and more comprehensible now.

This revision is now accepted and ready to land.Jan 30 2020, 2:48 PM

The border radius is slightly different because I copied what Kirigami does for the inner rect but I can also go back to radius 4

davidre updated this revision to Diff 74697.Jan 30 2020, 2:54 PM

Tiny mistakes

davidre edited the test plan for this revision. (Show Details)Jan 30 2020, 2:54 PM

The border radius is slightly different because I copied what Kirigami does for the inner rect but I can also go back to radius 4

Nevermind that was because of above mistake

Does that also work in Kate for floating messages like when the search wraps? What I want to know is whether the corners behind the green frame are transparent in this case, or whether the corners are painted solid. If I remember correctly, these kind of bugs were the reason to use Qt StyleSheets. And it must work with all styles.

Please do NOT yet commit, especially since we'd only have two days of testing period: Saturday, 1st of February is the next frameworks tag.

Can you provide screenshots of more styles? :-)

davidre added a comment.EditedJan 30 2020, 4:46 PM

Does that also work in Kate for floating messages like when the search wraps? What I want to know is whether the corners behind the green frame are transparent in this case, or whether the corners are painted solid. If I remember correctly, these kind of bugs were the reason to use Qt StyleSheets. And it must work with all styles.

Please do NOT yet commit, especially since we'd only have two days of testing period: Saturday, 1st of February is the next frameworks tag.

Should or shouldn't it be transparent? Also I don't understand which part should be transparent and which shouldn't? The border was never transparent and the background also not. Screenshots:

Can you provide screenshots of more styles? :-)

Of course:
Adwaita


Adwaita Dark

Fusion

MS Windows 9x

Oxygen

QtCurve

Does that also work in Kate for floating messages like when the search wraps? What I want to know is whether the corners behind the green frame are transparent in this case, or whether the corners are painted solid. If I remember correctly, these kind of bugs were the reason to use Qt StyleSheets. And it must work with all styles.

Please do NOT yet commit, especially since we'd only have two days of testing period: Saturday, 1st of February is the next frameworks tag.

Sorry forgot the screenshots, but you're right the text shines through now in KTextEditor and it doesn't seem to positioned correctly. For the first one I could go back to mixing the color manually instead of just drawing it with an alpha of 0.2. I will look into the second issue

davidre updated this revision to Diff 74714.Jan 30 2020, 5:11 PM

Draw opaque and mix the colors manually again.

davidre added a comment.EditedJan 30 2020, 5:18 PM

Kate with this patch and Oxygen style:


The misplacement I spotted earlier is specific to QtCurve and also happens with the current KMessageWidget.

Better :) with corners I mean the 1-3 pixels left due to the rounding corners. These pixels were once also drawn as background although they are outside of the frame. It may be a minor detail, but imho such details are important. But indeed, the screenshots look good.

Next test: open a file in kate. Now either change the file externally or delete it. Kate should show an animated KMessageWidget. Does it also correctly work with a small kate? I.e. small width of kate window?

src/kmessagewidget.cpp
317

Can't you use parentWidget()?
See: https://doc.qt.io/qt-5/qwidget.html#parentWidget

cfeck added a subscriber: cfeck.Jan 30 2020, 11:07 PM
cfeck added inline comments.
src/kmessagewidget.cpp
39

Does it need to be static to avoid an external symbol?

davidre added inline comments.Jan 30 2020, 11:20 PM
src/kmessagewidget.cpp
39

I don't think so:

internal linkage
[...]
variables, variable templates (since C++14), functions, or function templates declared static;
non-volatile non-template (since C++14) non-inline (since C++17) non-exported (since C++20) const-qualified variables (including constexpr) that aren't declared extern and aren't previously declared to have external linkage;

davidre added inline comments.Jan 31 2020, 12:33 PM
src/kmessagewidget.cpp
317

I knew there was a function like that but couldn't remember its name. Thanks!

davidre updated this revision to Diff 74766.Jan 31 2020, 12:33 PM
davidre marked an inline comment as done.

Use parentWidget()

Better :) with corners I mean the 1-3 pixels left due to the rounding corners. These pixels were once also drawn as background although they are outside of the frame. It may be a minor detail, but imho such details are important. But indeed, the screenshots look good.

Next test: open a file in kate. Now either change the file externally or delete it. Kate should show an animated KMessageWidget. Does it also correctly work with a small kate? I.e. small width of kate window?

It seems to work better than before, on the left the current one, on the right this patch:

ngraham requested changes to this revision.Jan 31 2020, 1:45 PM

Uh-oh, now it looks like this in Konsole:

Compare to the current appearance:

I recall wrestling with this issue in D12508. Might be worth looking over that.

This revision now requires changes to proceed.Jan 31 2020, 1:45 PM

Uh-oh, now it looks like this in Konsole:

Compare to the current appearance:

I recall wrestling with this issue in D12508. Might be worth looking over that.

That's because it wants to look transparent and mixes the background color with its color. One fix could be to also use the parent's text color (in this case white) or stop using QPalette::Window for data storage and mix with the window color of the widget's palette and use something else (maybe QPalette::Base). Opinions?

Something like this:

Ping, how should I proceed?

Something like this:

That would be fine, yes.

In the current code I took the approach of always using the system color scheme even if the local view overrode it, as Konsole does. Using the view's local color scheme isn't incorrect, just different. Arguably it's more correct. I say go for it if you think it makes sense.

davidre updated this revision to Diff 75716.Feb 14 2020, 4:50 PM

Explicitly set TextColor and palettes of the labels and react to parent changes . For example necessary for konsole
that uses stylesheet which break palette propagation and reparents the widget after creation.

davidre updated this revision to Diff 75717.Feb 14 2020, 4:53 PM

remove debug remains

ngraham accepted this revision.Feb 14 2020, 6:37 PM
This revision is now accepted and ready to land.Feb 14 2020, 6:37 PM
cfeck added inline comments.Feb 14 2020, 7:43 PM
src/kmessagewidget.cpp
156–157

too

194

typo: it is

314

missing spaces around /

382

missing space before {

davidre updated this revision to Diff 76018.Feb 19 2020, 6:45 PM
davidre marked 4 inline comments as done.

Code style and typos

cfeck accepted this revision.Feb 19 2020, 8:26 PM

Merci :)

apol added a subscriber: apol.Feb 20 2020, 2:32 PM

+1 code looks much better!

So I think this is ready to be merged and I would do so this weekend

This revision was automatically updated to reflect the committed changes.