Converted to Page with a PlasmoidHeading in the heading
ClosedPublic

Authored by niccolove on Mar 31 2020, 3:01 PM.

Details

Summary

Now the main view is a Page, the toolbar is moved to the heading and it's contained in a PlasmoidHeading

Depends on D28466
Depends on D28575

Test Plan

Diff Detail

Repository
R97 Bluedevil
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
niccolove created this revision.Mar 31 2020, 3:01 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 31 2020, 3:01 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
niccolove requested review of this revision.Mar 31 2020, 3:01 PM
niccolove planned changes to this revision.Mar 31 2020, 3:01 PM
niccolove added a reviewer: Plasma.
niccolove updated this revision to Diff 78996.Mar 31 2020, 3:03 PM

Remove unrelated changes

niccolove planned changes to this revision.Mar 31 2020, 3:03 PM
niccolove updated this revision to Diff 78997.Mar 31 2020, 3:04 PM

remove unrelated changes pt2

niccolove updated this revision to Diff 78998.Mar 31 2020, 3:05 PM

unrelated pt3

niccolove edited the summary of this revision. (Show Details)Mar 31 2020, 3:06 PM
niccolove updated this revision to Diff 78999.Mar 31 2020, 3:07 PM

I said components 3

niccolove planned changes to this revision.Mar 31 2020, 3:07 PM
niccolove updated this revision to Diff 79000.Mar 31 2020, 3:11 PM

Import components3 separately

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?

ngraham retitled this revision from Converted to Page with a PlasmodHeading in the heading to Converted to Page with a PlasmoidHeading in the heading.Mar 31 2020, 4:12 PM
ngraham added a subscriber: ngraham.

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

Screenshots in the Test Plan section are always appreciated :)

niccolove edited the test plan for this revision. (Show Details)Mar 31 2020, 5:24 PM

Screenshots in the Test Plan section are always appreciated :)

True, sorry. Added

ngraham requested changes to this revision.Mar 31 2020, 5:41 PM

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

src/applet/package/contents/ui/FullRepresentation.qml
31–32

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

This revision now requires changes to proceed.Mar 31 2020, 5:41 PM
niccolove planned changes to this revision.Apr 1 2020, 5:17 PM
niccolove added inline comments.
src/applet/package/contents/ui/FullRepresentation.qml
31–32

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

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)

niccolove updated this revision to Diff 79155.Apr 2 2020, 4:18 PM

Fixed layout

niccolove marked 2 inline comments as done.Apr 2 2020, 4:19 PM
ngraham requested changes to this revision.Apr 3 2020, 4:22 PM

So here's what I see in the system tray popup:

The toolbar doesn't touch the edges of its surroundings, so there's a differently-colored margin around it.

This revision now requires changes to proceed.Apr 3 2020, 4:22 PM

So here's what I see in the system tray popup:

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

niccolove requested review of this revision.Apr 9 2020, 9:29 PM
ngraham requested changes to this revision.Apr 9 2020, 9:56 PM

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:
  1. When Bluetooth is disabled, there's an extra header row with nothing on it:
This revision now requires changes to proceed.Apr 9 2020, 9:56 PM
niccolove updated this revision to Diff 79889.Apr 11 2020, 9:56 PM

Removed extra line

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?

Does activeApplet.fullRepresentationItem actually have a header property?

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.

ngraham requested changes to this revision.Apr 14 2020, 3:38 AM

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

src/applet/package/contents/ui/FullRepresentation.qml
31–32

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

This revision now requires changes to proceed.Apr 14 2020, 3:38 AM
niccolove added inline comments.Apr 14 2020, 9:58 AM
src/applet/package/contents/ui/FullRepresentation.qml
31–32

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.

ngraham added inline comments.Apr 14 2020, 1:41 PM
src/applet/package/contents/ui/FullRepresentation.qml
31–32

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 States though. I find that it leads to messy, imperative code.

niccolove requested review of this revision.Apr 16 2020, 9:18 PM

Should finally work with latest D28575

Hmm, seems broke:

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

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 🥴

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
ngraham accepted this revision.Apr 16 2020, 9:44 PM

LGTM before the rebase though!

This revision is now accepted and ready to land.Apr 16 2020, 9:44 PM
niccolove updated this revision to Diff 80321.Apr 16 2020, 9:49 PM

rebase x2

niccolove updated this revision to Diff 80324.Apr 16 2020, 9:53 PM

rebase x3

ngraham accepted this revision.Apr 16 2020, 9:54 PM

Shipit! Shipit real good!

This revision was automatically updated to reflect the committed changes.