[Applet] Port to ExpandableListItem
ClosedPublic

Authored by ngraham on Mar 13 2020, 5:26 PM.

Details

Summary

Depends on D28033

Test Plan

All functionality still works. There are minimal visual changes, mostly little things
to make the appearance consistent with other applets using the new
ExpandableListItem component.

Diff Detail

Repository
R116 Plasma Network Management Applet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Mar 13 2020, 5:26 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 13 2020, 5:26 PM
Restricted Application added 1 blocking reviewer(s): jgrulich. · View Herald Transcript
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.Mar 13 2020, 5:26 PM
gvgeo added a subscriber: gvgeo.EditedMar 13 2020, 9:31 PM

Labels need eliding. Full width has problems?
Button need to expand also. Click always collapse?
Listitem.qml can be removed now. Not exactly.

Btw, I didn't see itemExpanded use anywhere.
In expandedView (main patch) there is height in Layout.

Button right margin is constant, looks slightly small for known networks.
Now is not possible to cancel a connection attempt. Is there a use case?

ngraham edited the summary of this revision. (Show Details)Mar 16 2020, 12:15 AM
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 77696.Mar 16 2020, 3:17 AM

Adjust to API changes in D28033; now subtitle eliding works when the button is visible

ngraham updated this revision to Diff 77697.Mar 16 2020, 3:21 AM

Use consistent highlight animation durations

ngraham updated this revision to Diff 77703.Mar 16 2020, 4:01 AM

Make the password field align in the same way that the default expanded view content does

ngraham retitled this revision from [WIP] [Applet] Port to ExpandableListItem to [Applet] Port to ExpandableListItem.Mar 16 2020, 4:05 AM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham updated this revision to Diff 77704.Mar 16 2020, 4:06 AM

Remove FIXME; it was fixed by an upstream change in D28033

ngraham updated this revision to Diff 77936.Mar 18 2020, 4:19 PM

ExpandableListItem is now in PlasmaExtras

Seems to work with all functionality working as before, well done!! Just few details:

  1. I noticed that expanding an active connection makes it to switch the expanded "tab view" to the second tab with details information, it first expands showing the traffic monitor and then moves to the details view instead. I see that everytime I restart plasmashell and open it.
  2. When connecting to an unknown wireless connection, the connect button is enabled, it's not greyed out, which is identical to the original behavior, however making it to pupup the password dialog, the button is then greyed out, which is correct, but closing it doesn't make the button again active, it's still greyed out so it doesn't restore the original state.
ngraham planned changes to this revision.Apr 6 2020, 2:38 PM

Thanks for the review. Will fix.

ngraham updated this revision to Diff 79522.Apr 6 2020, 6:59 PM
  • Don't switch to the details tab when showing the expanded view
  • Re-enable the Connect button when hiding the password field
jgrulich added a comment.EditedApr 6 2020, 7:16 PM

Both issues fixed, however I found two news:

  1. Getting file:///usr/lib64/qt5/qml/org/kde/plasma/extras/ExpandableListItem.qml:438:13: QML BusyIndicator: Binding loop detected for property "implicitHeight" spammed in terminal from plasmashell process, but this is not related to this review.
  2. You are not able to disconnect a connection which is being activated, for example when you accidentally click on one to connect and want to stop it, I don't know if it's super important, but it was there and I'm using it from time to time (wanted to use it right now).
  3. I'm thinking of not using the busy indicator, it makes the UI to lag and I have been experiencing it recently and it's super annoying, I don't know it's the rendering process or what, but everytime I activate a connection, the UI is super slow until it connects. Might be combination of the busy indicator and something in the background, but not using the busy indicator made it significantly less laggy. Do you experince it as well?

Yeah, the PC3 busy indicator component itself seems to be to blame for items 1 and 3. I'll look into it.

I hadn't really considered #2 as a valid use case while working on the component, but I can look into it.

Yeah, the PC3 busy indicator component itself seems to be to blame for items 1 and 3. I'll look into it.

I hadn't really considered #2 as a valid use case while working on the component, but I can look into it.

It was there before, some connections may take a while to connect and if you activated them by mistake, you might avoid the waiting by just disconnecting it. For example in case of wireless connections, other connections might disappear when connecting so you have no way how to connect to the one you want, not until it connects or fails to connect.

Fix binding loops: D28787

ngraham updated this revision to Diff 79981.Apr 13 2020, 4:37 AM

Use new showDefaultActionButtonWhenBusy option to allow disconnecting while a connection attempt is in progress

jgrulich accepted this revision.Apr 14 2020, 11:31 AM

Those were probably all issues I could find. I will keep using it and if I find something later, I will let you know. Thank you.

This revision is now accepted and ready to land.Apr 14 2020, 11:31 AM
This revision was automatically updated to reflect the committed changes.

This breaks entering a WiFi password for me. Pressing enter just closes the password field with no other reaction. Pressing the Connect button does nothing

Crap. I could have sworn I tested that, but I see you're right. Will fix ASAP.