D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

This revision was automatically updated to reflect the committed changes.
Closed by commit R97:4a8adbaeac4e: Converted to Page with a PlasmoidHeading in 
the heading (authored by niccolove).

REPOSITORY
  R97 Bluedevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28467?vs=80324=80327

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

AFFECTED FILES
  src/applet/package/contents/ui/FullRepresentation.qml
  src/applet/package/contents/ui/Toolbar.qml

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Shipit! Shipit real good!

REPOSITORY
  R97 Bluedevil

BRANCH
  arcpatch-D28467

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove updated this revision to Diff 80324.
niccolove added a comment.


  rebase x3

REPOSITORY
  R97 Bluedevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28467?vs=80321=80324

BRANCH
  arcpatch-D28467

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

AFFECTED FILES
  src/applet/package/contents/ui/FullRepresentation.qml
  src/applet/package/contents/ui/Toolbar.qml

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove updated this revision to Diff 80321.
niccolove added a comment.


  rebase x2

REPOSITORY
  R97 Bluedevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28467?vs=80320=80321

BRANCH
  arcpatch-D28467

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

AFFECTED FILES
  src/applet/package/contents/ui/FullRepresentation.qml
  src/applet/package/contents/ui/Toolbar.qml

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove updated this revision to Diff 80320.
niccolove added a comment.


  rebase

REPOSITORY
  R97 Bluedevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28467?vs=79889=80320

BRANCH
  arcpatch-D28467

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

AFFECTED FILES
  src/applet/package/contents/ui/FullRepresentation.qml
  src/applet/package/contents/ui/Toolbar.qml

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  LGTM before the rebase though!

REPOSITORY
  R97 Bluedevil

BRANCH
  master

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove added a comment.


  In D28467#649913 , @ngraham wrote:
  
  > Hmm, seems broke:
  >
  >   
file:///home/nate/kde/usr/share/plasma/plasmoids/org.kde.plasma.bluetooth/contents/ui/FullRepresentation.qml:145:1:
 Expected token `}'
  >
  
  
  Forgot to rebase 若

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham added a comment.


  Sorry, I actually broke it in my attempt to rebase it so that it lands. You 
need to do the rebase on top of master:
  
git checkout master
git pull
git checkout [your branch]
git pull --rebase origin master
arc diff

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham added a comment.


  Hmm, seems broke:
  

file:///home/nate/kde/usr/share/plasma/plasmoids/org.kde.plasma.bluetooth/contents/ui/FullRepresentation.qml:145:1:
 Expected token `}'

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove requested review of this revision.
niccolove added a comment.


  Should finally work with latest D28575 

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

> niccolove wrote in FullRepresentation.qml:31
> I though that was done by
> 
>   toolbar.visible = (state == "DevicesState" || state == "NoDevicesState");
> 
> At the end of the file. If you try to use the widget standalone, you will 
> notice that it does disappear. Problem is, system tray doesn't notice. I 
> can't understand why it doesn't read the ".visible" property.

It's because the binding is broken.

In QML, there are two ways to change a property: with a binding, or 
imperatively.

A binding uses a colon, and looks like this: `visible: (state == "DevicesState" 
|| state == "NoDevicesState")` You want to use binding as much as possible 
because the property being bound (`visible`) will auto-update when the 
condition changes.

Imparative code uses an equals sign: `toolbar.visible = (state == 
"DevicesState" || state == "NoDevicesState");`

This breaks any existing bindings, which means you become responsible for 
manually updating the property in question `toolbar.visible` whenever you want 
it to change. Theoretically that's taken care of by `onStateChanged:` but for 
some reason it doesn't seem to be working. Perhaps there's a state not 
accounted for there? In general I don't like using explicit `State`s though. I 
find that it leads to messy, imperative code.

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove added inline comments.

INLINE COMMENTS

> ngraham wrote in FullRepresentation.qml:31
> the visibility should be dependent on it having any items in it; or else it's 
> still visible when Bluetooth is disabled.

I though that was done by

  toolbar.visible = (state == "DevicesState" || state == "NoDevicesState");

At the end of the file. If you try to use the widget standalone, you will 
notice that it does disappear. Problem is, system tray doesn't notice. I can't 
understand why it doesn't read the ".visible" property.

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  This'll need a rebase, as I landed the ExpandableListItem port a few days ago.

INLINE COMMENTS

> FullRepresentation.qml:31
>  
> -PlasmaBt.DevicesProxyModel {
> -id: devicesModel
> -sourceModel: BluezQt.DevicesModel { }
> +header: Toolbar {
> +id: toolbar

the visibility should be dependent on it having any items in it; or else it's 
still visible when Bluetooth is disabled.

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove added a comment.


  In D28467#646626 , @ngraham wrote:
  
  > Does `activeApplet.fullRepresentationItem` actually have a `header` 
property?
  
  
  Yes. I tried to console log it (...header) and it returns Toolbar qml 
element, which is correct, and the element supposed to have the visible 
property.

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham added a comment.


  Does `activeApplet.fullRepresentationItem` actually have a `header` property?

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove added a comment.


  I need a small help for the second one. When there's no toolbar, the 
toolbar.visible is set to false; otherwise, it's true. But when I'm in the 
systemtray and I try to check 
activeApplet.fullRepresentationItem.header.visible, it always returns false. 
What could that be?

REPOSITORY
  R97 Bluedevil

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

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove updated this revision to Diff 79889.
niccolove added a comment.


  Removed extra line

REPOSITORY
  R97 Bluedevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28467?vs=79155=79889

BRANCH
  master

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

AFFECTED FILES
  src/applet/package/contents/ui/FullRepresentation.qml
  src/applet/package/contents/ui/Toolbar.qml

To: niccolove, #plasma, ngraham
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  Looking good. I see two remaining visual issues:
  
  1. There's an extra one-pixel line above the section header text that touches 
the bottom line of the applet's header: F8227723: 
Screenshot_20200409_155326.png 
  
  2. When Bluetooth is disabled, there's an extra header row with nothing on 
it: F8227726: Screenshot_20200409_155444.png 


REPOSITORY
  R97 Bluedevil

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

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove added a comment.


  In D28467#640854 , @ngraham wrote:
  
  > So here's what I see in the system tray popup: F8213414: 
Screenshot_20200403_102113.png 
  >
  > The toolbar doesn't touch the edges of its surroundings, so there's a 
differently-colored margin around it.
  
  
  It's correct, because the systray has clipping disabled. But fear not, as 
D28575  is here :P

REPOSITORY
  R97 Bluedevil

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

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  So here's what I see in the system tray popup: F8213414: 
Screenshot_20200403_102113.png 
  
  The toolbar doesn't touch the edges of its surroundings, so there's a 
differently-colored margin around it.

REPOSITORY
  R97 Bluedevil

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

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove updated this revision to Diff 79155.
niccolove added a comment.


  Fixed layout

REPOSITORY
  R97 Bluedevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28467?vs=79000=79155

BRANCH
  master

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

AFFECTED FILES
  src/applet/package/contents/ui/FullRepresentation.qml
  src/applet/package/contents/ui/Toolbar.qml

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove added a comment.


  In D28467#639033 , @ngraham wrote:
  
  > Make sure you test how this looks in the System Tray too, and not just as a 
standalone applet. ;)
  
  
  Two lines will appear, but that will soon be corrected by another patch that 
I'm working on (1/2 days and should be ready)

REPOSITORY
  R97 Bluedevil

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

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove added inline comments.

INLINE COMMENTS

> ngraham wrote in FullRepresentation.qml:31
> `PlasmaComponents3.Page` doesn't seem to have a `heading` property. Perhaps 
> you meant `header`?

Apparently I managed to test the qml, and then commit an older version of the 
file, which is broken, and I don't even have the correct version anymore. Yay

REPOSITORY
  R97 Bluedevil

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

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  Make sure you test how this looks in the System Tray too, and not just as a 
standalone applet. ;)

INLINE COMMENTS

> FullRepresentation.qml:31
>  
> -PlasmaBt.DevicesProxyModel {
> -id: devicesModel
> -sourceModel: BluezQt.DevicesModel { }
> +heading: Toolbar {
> +id: toolbar

`PlasmaComponents3.Page` doesn't seem to have a `heading` property. Perhaps you 
meant `header`?

REPOSITORY
  R97 Bluedevil

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

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Niccolò Venerandi]

niccolove added a comment.


  In D28467#639027 , @ngraham wrote:
  
  > Screenshots in the Test Plan section are always appreciated :)
  
  
  True, sorry. Added

REPOSITORY
  R97 Bluedevil

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

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham added a comment.


  Screenshots in the Test Plan section are always appreciated :)

REPOSITORY
  R97 Bluedevil

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

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

D28467: Converted to Page with a PlasmoidHeading in the heading

[Nathaniel Graham]

ngraham added a comment.


  In D28467#638980 , @niccolove 
wrote:
  
  > I'm a bit confused by
  >
  >   import org.kde.plasma.components 2.0 as PlasmaComponents
  >   import org.kde.plasma.components 3.0 as PlasmaComponents3
  >
  >
  > is it correct?
  
  
  Yes

REPOSITORY
  R97 Bluedevil

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

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