D24720: [applets/systemtray] Rewrite popups with layouts

filipf added inline comments. INLINE COMMENTS > davidedmundson wrote in ExpandedRepresentation.qml:21 > It is a requirement to still work on Qt5.12 for the next release. Well > spotted Meven. > > I'll adjust this. Should have checked that, thanks David and Meven :) REPOSITORY R120 Plasma

D24720: [applets/systemtray] Rewrite popups with layouts

davidedmundson added a comment. (nice patch btw, I love layouts) INLINE COMMENTS > filipf wrote in 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. It is a requirement to still work on

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added inline comments. INLINE COMMENTS > meven wrote in 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. I just used the imports provided in

D24720: [applets/systemtray] Rewrite popups with layouts

meven added inline comments. INLINE COMMENTS > ExpandedRepresentation.qml:21 > +import QtQuick 2.13 > +import QtQuick.Layouts 1.13 > + 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.

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

This revision was automatically updated to reflect the committed changes. Closed by commit R120:5b3f5bd9c32e: [applets/systemtray] Rewrite popups with layouts (authored by filipf). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24720?vs=68485=69217

D24720: [applets/systemtray] Rewrite popups with layouts

safaalfulaij added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added a comment. In D24720#557915 , @ngraham wrote: > It it just awaiting review from a #plasma person? Yes. REPOSITORY R120 Plasma Workspace BRANCH popup-rewrite (branched from

D24720: [applets/systemtray] Rewrite popups with layouts

ngraham added a subscriber: manueljlin. ngraham added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

filipf marked 5 inline comments as done. filipf added a comment. Everything should work fine now hopefully. REPOSITORY R120 Plasma Workspace BRANCH popup-rewrite (branched from master) REVISION DETAIL https://phabricator.kde.org/D24720 To: filipf, #plasma, #vdg, ngraham Cc: ognarb,

D24720: [applets/systemtray] Rewrite popups with layouts

filipf updated this revision to Diff 68485. filipf added a comment. - pin button now appears in the "Status and Notifications" popup again - simpler code for checking layout mirroring REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

D24720: [applets/systemtray] Rewrite popups with layouts

ognarb added inline comments. INLINE COMMENTS > ExpandedRepresentation.qml:48 > +//Menu mode > +if (!activeApplet && hiddenItemsView.visible && > LayoutMirroring.enabled === false) { > +return units.smallSpacing; !LayoutMirroring.enabled

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. Oh I see what you mean. This is fine then! REPOSITORY R120 Plasma Workspace BRANCH popup-rewrite (branched from master) REVISION DETAIL https://phabricator.kde.org/D24720 To:

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

ngraham added a comment. This works for me: P482 Patch for https://phabricator.kde.org/D24720 F7642232: Screenshot_20191021_113604.png F7642255: Screenshot_20191021_114030.png

D24720: [applets/systemtray] Rewrite popups with layouts

filipf updated this revision to Diff 68403. filipf added a comment. restore original margin between sidebar and container REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24720?vs=68201=68403 BRANCH popup-rewrite (branched from master) REVISION

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added a comment. 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?

D24720: [applets/systemtray] Rewrite popups with layouts

ngraham added a comment. 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? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24720

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

filipf updated this revision to Diff 68201. filipf added a comment. make all this work right with RTL layouts REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24720?vs=68182=68201 BRANCH popup-rewrite (branched from master) REVISION DETAIL

D24720: [applets/systemtray] Rewrite popups with layouts

ngraham added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added a comment. 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. INLINE COMMENTS > ExpandedRepresentation.qml:55 > } else if (activeApplet &&

D24720: [applets/systemtray] Rewrite popups with layouts

filipf updated this revision to Diff 68182. filipf added a comment. don't alter padding REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24720?vs=68103=68182 BRANCH popup-rewrite (branched from master) REVISION DETAIL

D24720: [applets/systemtray] Rewrite popups with layouts

ngraham added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

kmaterka added a comment. 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. REPOSITORY

D24720: [applets/systemtray] Rewrite popups with layouts

ngraham added a comment. +1, let;s not change the paddings here. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24720 To: filipf, #plasma, #vdg Cc: ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen,

D24720: [applets/systemtray] Rewrite popups with layouts

kmaterka added a comment. 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,

D24720: [applets/systemtray] Rewrite popups with layouts

mart added a comment. In D24720#548745 , @GB_2 wrote: > > F7610748: Screenshot_20191017_013250.png > > Can we align the header with the search field/content here? ish: since they arrive from

D24720: [applets/systemtray] Rewrite popups with layouts

mart added a comment. 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 INLINE COMMENTS > filipf wrote in ExpandedRepresentation.qml:93 > How do I use

D24720: [applets/systemtray] Rewrite popups with layouts

GB_2 added a comment. > F761074 Can we align the header with the search field/content here? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24720 To: filipf, #plasma, #vdg Cc: GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas,

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added a comment. 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

D24720: [applets/systemtray] Rewrite popups with layouts

filipf updated this revision to Diff 68103. filipf added a comment. remove useless leftover stuff from WIP versions REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24720?vs=68102=68103 BRANCH popup-rewrite (branched from master) REVISION DETAIL

D24720: [applets/systemtray] Rewrite popups with layouts

filipf added inline comments. INLINE COMMENTS > ExpandedRepresentation.qml:93 > +icon.name: "window-pin" > +//tooltip: i18n("Keep Open") > +} How do I use tooltips with PC3 here again? REPOSITORY R120 Plasma Workspace REVISION DETAIL

D24720: [applets/systemtray] Rewrite popups with layouts

filipf created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. filipf requested review of this revision. REVISION SUMMARY Currently the code that draws plasmoid popups utilizes an anchor based approach. I think layouts are a more elegant solution so