Differential D24720

[applets/systemtray] Rewrite popups with layouts
ClosedPublic
Actions

Authored by filipf on Oct 16 2019, 11:52 PM.

Details

Reviewers
ngraham
mart
Group Reviewers
Plasma
VDG
Commits
R120:5b3f5bd9c32e: [applets/systemtray] Rewrite popups with layouts
Summary

Currently the code that draws plasmoid popups utilizes an anchor based approach.

I think layouts are a more elegant solution so this patch makes use of them.

Test Plan

Diff Detail

Repository
R120 Plasma Workspace
Branch
popup-rewrite (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18000
Build 18018: arc lint + arc unit
filipf created this revision.Oct 16 2019, 11:52 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 16 2019, 11:52 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
filipf requested review of this revision.Oct 16 2019, 11:52 PM
filipf edited the test plan for this revision. (Show Details)Oct 16 2019, 11:53 PM
filipf added reviewers: Plasma, VDG.
filipf added inline comments.
applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
108

How do I use tooltips with PC3 here again?

filipf updated this revision to Diff 68103.Oct 16 2019, 11:57 PM

remove useless leftover stuff from WIP versions

filipf added a comment.Oct 17 2019, 12:05 AM

The paddings issue is actually the same as with notifications (D21813). We can tweak it here and then alter it for all desktop themes, some of which already add extra padding on their own. This means we're messing with their looks.

Or we could add padding to our SVGs, which seems neater. I don't have the SVG skills to do it, but if we opt for that I'll just revert the padding changes here.

GB_2 added a subscriber: GB_2.EditedOct 17 2019, 8:22 AM

Can we align the header with the search field/content here?

mart added a subscriber: mart.Oct 17 2019, 8:49 AM

code-wise i like it.
It changes the look quite a bit, by adding a lot of padding. Is that ok for VDG?
personally i liked that the blue line touched the separator line, same as kickoff

applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
108

Plasmacomponents.ToolTip {

text: i18n("Keep Open")

}

mart added a comment.Oct 17 2019, 8:53 AM
In D24720#548745, @GB_2 wrote:

Can we align the header with the search field/content here?

ish: since they arrive from different places, header drawn by the systray and the search field drawn by the klipper plasmoid.
so the positioning should be adjusted that it works "most of the case", unless the applet shown there does something weird or uses non standard paddings.
but yeas, should be adjusted that it looks aligned with default systray plasmoids

kmaterka added a subscriber: kmaterka.Oct 17 2019, 2:48 PM

Pin button is no longer working,which is surprising :)

About padding: I would vote for not changing padding nor margins, at least not in this change. It will be easier to review and test it you split this into two changes. This popup size is pretty small, if you add paddings or margins space left for applet is even smaller.

ngraham added a subscriber: ngraham.Oct 17 2019, 3:02 PM

+1, let;s not change the paddings here.

kmaterka added a comment.Oct 17 2019, 3:23 PM
In D24720#549097, @kmaterka wrote:

Pin button is no longer working,which is surprising :)

In fact it is working, but there is no background change. Old version had blue background when pinned.

ngraham added a comment.Oct 17 2019, 3:32 PM
In D24720#549139, @kmaterka wrote:
In D24720#549097, @kmaterka wrote:

Pin button is no longer working,which is surprising :)

In fact it is working, but there is no background change. Old version had blue background when pinned.

Make sure that you have plasma-framework from git master. That should fix it.

filipf updated this revision to Diff 68182.Oct 17 2019, 5:53 PM

don't alter padding

filipf edited the test plan for this revision. (Show Details)Oct 17 2019, 5:56 PM
filipf added a comment.Oct 17 2019, 5:59 PM

Agreed, let's not change the padding.

One issue that remains is that headings don't work well with RTL layouts but I can't quite figure out a solution yet.

applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
67–68

maybe this could be somehow more precise

ngraham added a comment.Oct 17 2019, 6:14 PM

Nice, this feels indistinguishible from the current one, which is a good sign. I see what you mean about the Headings in RTL. Does the Kirigami version work properly? If so, I wonder if it might be worth it to just use that instead given that the future of PlasmaComponents is on shaky ground (T11558: kill plasma components in favour of qqc2-desktop-style)

filipf updated this revision to Diff 68201.Oct 17 2019, 8:03 PM

make all this work right with RTL layouts

filipf added a comment.Oct 17 2019, 8:05 PM
In D24720#549292, @ngraham wrote:

Nice, this feels indistinguishible from the current one, which is a good sign. I see what you mean about the Headings in RTL. Does the Kirigami version work properly? If so, I wonder if it might be worth it to just use that instead given that the future of PlasmaComponents is on shaky ground (T11558: kill plasma components in favour of qqc2-desktop-style)

It was unfortunately all the same as with Plasma imports.

By adding checks for orientation and using a spacer in the headings row layout I was able to get RTL working right though.

ngraham added a comment.Oct 19 2019, 2:05 PM

Seems like the RTL problem may be that your RowLayouts that are within the parent ColumnLayout don't have Layout.fillWidth: true set. Does the problem go away if you set that?

filipf added a comment.Oct 20 2019, 10:03 AM
In D24720#550200, @ngraham wrote:

Seems like the RTL problem may be that your RowLayouts that are within the parent ColumnLayout don't have Layout.fillWidth: true set. Does the problem go away if you set that?

The heading RowLayout had that set in the previous version of the diff but it didn't help.

filipf updated this revision to Diff 68403.Oct 21 2019, 7:49 AM

restore original margin between sidebar and container

filipf edited the test plan for this revision. (Show Details)Oct 21 2019, 7:50 AM
filipf edited the summary of this revision. (Show Details)
ngraham added a comment.Oct 21 2019, 5:41 PM

This works for me:

P482 Patch for https://phabricator.kde.org/D24720
1diff --git a/applets/systemtray/package/contents/ui/ExpandedRepresentation.qml b/applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
2index 0d723d974..9976e7ca6 100644
3--- a/applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
4+++ b/applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
5@@ -39,38 +39,11 @@ ColumnLayout {
6 property alias hiddenLayout: hiddenItemsView.layout
7
8 RowLayout {
9+ Layout.fillWidth: true
10
11 PlasmaExtras.Heading {
12 id: heading
13 level: 1
14- Layout.leftMargin: {
15- //Menu mode
16- if (!activeApplet && hiddenItemsView.visible && LayoutMirroring.enabled === false) {
17- return units.smallSpacing;
18-
19- //applet open, sidebar
20- } else if (activeApplet && hiddenItemsView.visible && LayoutMirroring.enabled === false) {
21- return hiddenItemsView.width + units.largeSpacing;
22-
23- //applet open, no sidebar
24- } else {
25- return 0;
26- }
27- }
28- Layout.rightMargin: {
29- //Menu mode
30- if (!activeApplet && hiddenItemsView.visible && LayoutMirroring.enabled === true) {
31- return units.smallSpacing;
32-
33- //applet open, sidebar
34- } else if (activeApplet && hiddenItemsView.visible && LayoutMirroring.enabled === true) {
35- return hiddenItemsView.width + units.largeSpacing;
36-
37- //applet open, no sidebar
38- } else {
39- return 0;
40- }
41- }
42
43 visible: activeApplet
44 text: activeApplet ? activeApplet.title : ""
45@@ -110,6 +83,7 @@ ColumnLayout {
46 }
47
48 RowLayout {
49+ Layout.fillWidth: true
50 spacing: 0 // must be 0 so that the separator is as close to the indicator as possible
51
52 HiddenItemsView {

filipf added a comment.Oct 21 2019, 7:15 PM

The latest version already works with RTL without the fillWidth code being needed. So as far as I can tell the diff you posted only effectively removes the margins from headings. But I think we should keep them, i.e., align those headings with the container (@GB_2 already mentioned this : D24720#548745)

But in the case of RTL perhaps the headings should be on the other side actually, same as before?

ngraham accepted this revision.Oct 21 2019, 7:18 PM

Oh I see what you mean. This is fine then!

This revision is now accepted and ready to land.Oct 21 2019, 7:18 PM
filipf added a comment.Oct 21 2019, 7:30 PM

Based on a search of some examples of RTL UI settings I'm going to say headings also go on the right side, so it seems they were actually not correctly placed before.

But I actually still have two other dilemmas:

  • the pin icon doesn't show up anymore in the "Status and Notifications" popup and I have no idea why
  • the pin icon is now noticeably bigger, should I make it smaller?
ognarb added a subscriber: ognarb.Oct 21 2019, 7:41 PM
ognarb added inline comments.
applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
49

!LayoutMirroring.enabled instead of === false

137

Same

138

Same

filipf updated this revision to Diff 68485.Oct 21 2019, 8:35 PM
  • pin button now appears in the "Status and Notifications" popup again
  • simpler code for checking layout mirroring
filipf marked 5 inline comments as done.Oct 21 2019, 8:36 PM

Everything should work fine now hopefully.

ngraham added a subscriber: manueljlin.Nov 2 2019, 7:00 PM

This seems like it's a prerequisite to starting work implementing @manueljlin's excellent mockups in T10470.

Is there anything blocking this? It it just awaiting review from a Plasma person?

filipf added a comment.Nov 2 2019, 7:06 PM
In D24720#557915, @ngraham wrote:

It it just awaiting review from a Plasma person?

Yes.

mart accepted this revision.Nov 3 2019, 9:15 AM
safaalfulaij added a subscriber: safaalfulaij.Nov 3 2019, 3:50 PM

Hi :)
Please tag me in case you need any RTL help.
Yes, headers are aligned based on direction before, which was bad. It's almost better to align them to the other side even if they're not translated. You never know.
And good that you spotted the issue with QML's Layouts regarding RTL:)

As a user, I'm happy to see more work being done to refactor the code and make it easier! <3

Closed by commit R120:5b3f5bd9c32e: [applets/systemtray] Rewrite popups with layouts (authored by filipf). · Explain WhyNov 3 2019, 4:12 PM
This revision was automatically updated to reflect the committed changes.
filipf added a comment.Nov 3 2019, 4:14 PM
In D24720#558099, @safaalfulaij wrote:

Hi :)
Please tag me in case you need any RTL help.
Yes, headers are aligned based on direction before, which was bad. It's almost better to align them to the other side even if they're not translated. You never know.
And good that you spotted the issue with QML's Layouts regarding RTL:)

As a user, I'm happy to see more work being done to refactor the code and make it easier! <3

Will do, thanks :)

meven added a subscriber: meven.Nov 8 2019, 7:43 AM
meven added inline comments.
applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
21

You effectively bumped the runtime dependency of plasma-framework to Qt 5.13.
Did we need to ? Would 5.12 suffice ?
Or was it just time to start using Qt 5.13.

filipf added inline comments.Nov 8 2019, 10:29 AM
applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
21

I just used the imports provided in the Qt documentation. Feel free to bump it down to 5.12 or even the original 5.1.

davidedmundson added a subscriber: davidedmundson.Nov 8 2019, 10:49 AM

(nice patch btw, I love layouts)

applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
21

It is a requirement to still work on Qt5.12 for the next release. Well spotted Meven.

I'll adjust this.

filipf added inline comments.Nov 8 2019, 12:16 PM
applets/systemtray/package/contents/ui/ExpandedRepresentation.qml
21

Should have checked that, thanks David and Meven :)

Revision Contents
Changeset List

Diff 68485

View Options

applets/systemtray/package/contents/ui/ExpandedRepresentation.qml

Loading...