D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham, mart
Cc: davidedmundson, meven, safaalfulaij, manueljlin, ognarb, ngraham, kmaterka, 
mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[David Edmundson]

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 Qt5.12 for the next release. Well spotted 
Meven.

I'll adjust this.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham, mart
Cc: davidedmundson, meven, safaalfulaij, manueljlin, ognarb, ngraham, kmaterka, 
mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 the Qt documentation. Feel free to bump it 
down to 5.12 or even the original 5.1.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham, mart
Cc: meven, safaalfulaij, manueljlin, ognarb, ngraham, kmaterka, mart, GB_2, 
plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Méven Car]

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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham, mart
Cc: meven, safaalfulaij, manueljlin, ognarb, ngraham, kmaterka, mart, GB_2, 
plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 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 :)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham, mart
Cc: safaalfulaij, manueljlin, ognarb, ngraham, kmaterka, mart, GB_2, 
plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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

REVISION DETAIL
  https://phabricator.kde.org/D24720

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

To: filipf, #plasma, #vdg, ngraham, mart
Cc: safaalfulaij, manueljlin, ognarb, ngraham, kmaterka, mart, GB_2, 
plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Safa Alfulaij]

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 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

REPOSITORY
  R120 Plasma Workspace

BRANCH
  popup-rewrite (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham, mart
Cc: safaalfulaij, manueljlin, ognarb, ngraham, kmaterka, mart, GB_2, 
plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 master)

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham
Cc: manueljlin, ognarb, ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Nathaniel Graham]

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 
 person?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  popup-rewrite (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham
Cc: manueljlin, ognarb, ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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, ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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
  https://phabricator.kde.org/D24720?vs=68403=68485

BRANCH
  popup-rewrite (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24720

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

To: filipf, #plasma, #vdg, ngraham
Cc: ognarb, ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Carl Schwan]

ognarb added inline comments.

INLINE COMMENTS

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

!LayoutMirroring.enabled instead of === false

> ExpandedRepresentation.qml:136
> +Layout.fillHeight: true
> +Layout.leftMargin: hiddenItemsView.visible && activeApplet && 
> LayoutMirroring.enabled === false ? units.largeSpacing : 0
> +Layout.rightMargin: hiddenItemsView.visible && activeApplet && 
> LayoutMirroring.enabled === true ? units.largeSpacing : 0

Same

> ExpandedRepresentation.qml:137
> +Layout.leftMargin: hiddenItemsView.visible && activeApplet && 
> LayoutMirroring.enabled === false ? units.largeSpacing : 0
> +Layout.rightMargin: hiddenItemsView.visible && activeApplet && 
> LayoutMirroring.enabled === true ? units.largeSpacing : 0
>  }

Same

REPOSITORY
  R120 Plasma Workspace

BRANCH
  popup-rewrite (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham
Cc: ognarb, ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 "Status and Notifications" 
popup and I have no idea why
  - the pin icon is now noticeably bigger, should I make it smaller?

REPOSITORY
  R120 Plasma Workspace

BRANCH
  popup-rewrite (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg, ngraham
Cc: ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Nathaniel Graham]

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: filipf, #plasma, #vdg, ngraham
Cc: ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 already mentioned this : D24720#548745 
)
  
  F7642540: image.png 
  
  But in the case of RTL perhaps the headings should be on the other side 
actually, same as before?
  
  F7642559: image.png 

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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Nathaniel Graham]

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 


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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 DETAIL
  https://phabricator.kde.org/D24720

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

To: filipf, #plasma, #vdg
Cc: ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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?
  
  
  The heading RowLayout had that set in the previous version of the diff but it 
didn't help.

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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Nathaniel Graham]

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

To: filipf, #plasma, #vdg
Cc: ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 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 QtQuickControls2 + kirigami 
)
  
  
  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.

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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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
  https://phabricator.kde.org/D24720

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

To: filipf, #plasma, #vdg
Cc: ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Nathaniel Graham]

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 PlasmaComponents is on shaky ground (T11558: kill 
plasma components in favour of QtQuickControls2 + kirigami 
)

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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 && hiddenItemsView.visible) {
> -return hiddenItemsView.iconColumnWidth + 
> units.largeSpacing;
> +return hiddenItemsView.width + separator.width + 
> units.smallSpacing / 2;
>  

maybe this could be somehow more precise

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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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
  https://phabricator.kde.org/D24720

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

To: filipf, #plasma, #vdg
Cc: ngraham, kmaterka, mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Nathaniel Graham]

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 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.

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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Konrad Materka]

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
  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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Nathaniel Graham]

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, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Konrad Materka]

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, if you add paddings or 
margins space left for applet is even smaller.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg
Cc: kmaterka, mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Marco Martin]

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 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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg
Cc: mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Marco Martin]

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 tooltips with PC3 here again?

Plasmacomponents.ToolTip {

  text: i18n("Keep Open")

}

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg
Cc: mart, GB_2, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra

D24720: [applets/systemtray] Rewrite popups with layouts

[Björn Feber]

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, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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
  https://phabricator.kde.org/D24720

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

To: filipf, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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
  https://phabricator.kde.org/D24720

To: filipf, #plasma, #vdg
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24720: [applets/systemtray] Rewrite popups with layouts

[Filip Fila]

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 this patch makes use of them.
  
  I also tried to tweak the paddings some.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  popup-rewrite (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D24720

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

To: filipf
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart