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.
ngraham | |
mart |
Plasma | |
VDG |
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.
No Linters Available |
No Unit Test Coverage |
Buildable 17826 | |
Build 17844: arc lint + arc unit |
applets/systemtray/package/contents/ui/ExpandedRepresentation.qml | ||
---|---|---|
91 | How do I use tooltips with PC3 here again? |
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.
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 | ||
---|---|---|
91 | Plasmacomponents.ToolTip { text: i18n("Keep Open") } |
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
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.
In fact it is working, but there is no background change. Old version had blue background when pinned.
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 | ||
---|---|---|
55 | maybe this could be somehow more precise |
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.
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.
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?
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:
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?
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
applets/systemtray/package/contents/ui/ExpandedRepresentation.qml | ||
---|---|---|
21 | You effectively bumped the runtime dependency of plasma-framework to Qt 5.13. |
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. |
(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. |
applets/systemtray/package/contents/ui/ExpandedRepresentation.qml | ||
---|---|---|
21 | Should have checked that, thanks David and Meven :) |