[plasma-nm/applet] Add right-click context menu to directly customize a connection
AcceptedPublic

Authored by vpilo on Jan 24 2019, 5:34 PM.

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.
vpilo created this revision.Jan 24 2019, 5:34 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 24 2019, 5:34 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
vpilo requested review of this revision.Jan 24 2019, 5:34 PM
vpilo added a reviewer: VDG.Jan 24 2019, 5:34 PM
GB_2 added a subscriber: GB_2.Jan 24 2019, 5:44 PM

Next time please upload images directly to Phabricator.

vpilo edited the summary of this revision. (Show Details)Jan 24 2019, 5:44 PM
GB_2 added a comment.Jan 24 2019, 5:45 PM

Thanks!
Looks good!

vpilo added a comment.Jan 24 2019, 5:46 PM
In D18504#399351, @GB_2 wrote:

Next time please upload images directly to Phabricator.

👍

abetts added a subscriber: abetts.Jan 24 2019, 5:47 PM

Do you prefer a right click over a settings or 3-dot button?

vpilo added a comment.EditedJan 24 2019, 6:00 PM

Do you prefer a right click over a settings or 3-dot button?

There's already very little space - I personally think it would become cluttered.
An alternative might be a link when the connection is clicked (and gets expanded).

Or a button that appears next to the connect/disconnect button only on hover, but I don't know if this is an UI that could cater to other kinds of flows, like touch.

I think a context menu is fine here, since it's not replacing anything else. We can examine the non-right-click presentation later. It might be nice to have a Configure button/icon show up on hover just like the disconnect one does. That's what Kirigami lists do.

jgrulich added a subscriber: jgrulich.EditedJan 25 2019, 6:55 AM

I'm not convinced this is needed at all. How often do you need to modify a connection? I also don't think many people would discover this feature, because many of them also didn't discover context menu in KCM.

Edit:
Now when you want to configure a connection, you go to the settings button in the top right corner and you can easily configure a connection. With your change you need one click with the right mouse button and additional click on the configure button. It's true that in the KCM it might be harder to find connection you want to edit, on the other hand there is a search bar.

I'm not convinced this is needed at all. How often do you need to modify a connection?

Often enough that it annoys me that I have to open the connection editor first and then search the connection *again* in the list of connections. +1 for the change.

applet/contents/ui/ConnectionItem.qml
144

What't this for?

146

Context menus open on press, not click

151

I would prefer PlasmaComponent.Menu here which is a "proper" popping up menu rather than an inline item like QQC2 unfortunately always does.

153

I don't think this header is needed, we hardly do that elsewhere.

158

I guess you can just use the stateChangeButton text?

165

Sneaky, I didn't intend KCMShell.open to accept arbitrary argument :D

I'm not convinced this is needed at all. How often do you need to modify a connection?

Often enough that it annoys me that I have to open the connection editor first and then search the connection *again* in the list of connections. +1 for the change.

Okay, I will not be against it since it basically doesn't change anything, just adds additional functionality.

Does it work for you on wayland? When the menu opens, it opens in the left top corner of the applet and cannot be closed unless you select any option, but that's probably wayland + qt fault or perhaps it will change when using PlasmaComponents.Menu instead.

Does it work for you on wayland?

Haven't tried, I suspect it fails to set a proper transient parent for the menu

vpilo updated this revision to Diff 50248.Jan 25 2019, 1:30 PM
vpilo marked 4 inline comments as done.
  • Review comments
vpilo added a comment.Jan 25 2019, 1:30 PM

Does it work for you on wayland? When the menu opens, it opens in the left top corner of the applet and cannot be closed unless you select any option, but that's probably wayland + qt fault or perhaps it will change when using PlasmaComponents.Menu instead.

Works for me with PlasmaComponents.Menu.

applet/contents/ui/ConnectionItem.qml
144

I didn't want to interfere with other buttons/mouse actions/gestures. I tested without this and it looks like there's no issue. I took it out.

153

I thought it made it easier when the wifi list changes to recognize if you clicked on the right item. Should I still take it away?

165

it's not a bug, it's a feature!

Jokes aside, it can be pretty useful to give arguments to KCModules.
e.g. an app might say "change the file association by going to this configuration screen" with a link opening the right KCM with the right tab

Does it work for you on wayland? When the menu opens, it opens in the left top corner of the applet and cannot be closed unless you select any option, but that's probably wayland + qt fault

This might be https://bugreports.qt.io/browse/QTBUG-51640, though I see that got closed...

vpilo added a comment.Jan 25 2019, 3:21 PM

Does it work for you on wayland? When the menu opens, it opens in the left top corner of the applet and cannot be closed unless you select any option, but that's probably wayland + qt fault

This might be https://bugreports.qt.io/browse/QTBUG-51640, though I see that got closed...

I used the new PlasmaComponents.Menu in the second diff and it is now working in Wayland too. It's unstable in my VM so I don't usually try too hard with it :P

jgrulich accepted this revision.Jan 28 2019, 6:56 AM

Looks good to me, I woud personally keep the header in the popup menu, at least I know I clicked on correct connection.

This revision is now accepted and ready to land.Jan 28 2019, 6:56 AM
This revision was automatically updated to reflect the committed changes.
Codezela added a subscriber: Codezela.EditedFeb 3 2019, 2:53 PM

small tweak we can make the connection name in the context menu bold header msybe
because it look like disabled now
it looks strange to me
what do u think

broulik reopened this revision.Feb 4 2019, 9:54 AM

I can no longer right click the details entries (e.g. IP address) to copy them to clipboard, the menu is blocked by this new menu. Perhaps the menu should only be on the text sections of the list item rather than span the entire delgate including details and plotter.

applet/contents/ui/ConnectionItem.qml
147

Please pass the cursor position with it, the menu currently always opens in the top left corner rather than wherre the mouse is

This revision is now accepted and ready to land.Feb 4 2019, 9:54 AM
vpilo added a comment.Feb 5 2019, 8:15 AM

small tweak we can make the connection name in the context menu bold header msybe
because it look like disabled now
it looks strange to me
what do u think

I tried. Somehow, even if I should be able to, I can't access the font property. The applet fails to load.

small tweak we can make the connection name in the context menu bold header msybe
because it look like disabled now
it looks strange to me
what do u think

I tried. Somehow, even if I should be able to, I can't access the font property. The applet fails to load.

The problem is that you're using a disabled menu item for the title, which makes the font light automatically. Try setting supportsMouseEvents: false for it, and then you can style it however you want (+1 for bold, do font.weight: Font.Bold to get that)

vpilo added a comment.Feb 5 2019, 4:27 PM

small tweak we can make the connection name in the context menu bold header msybe
because it look like disabled now
it looks strange to me
what do u think

I tried. Somehow, even if I should be able to, I can't access the font property. The applet fails to load.

The problem is that you're using a disabled menu item for the title, which makes the font light automatically. Try setting supportsMouseEvents: false for it, and then you can style it however you want (+1 for bold, do font.weight: Font.Bold to get that)

Looks like I can't, none of the items in the object tree seem to support that property, and it's not in the Qt dox? It's only in a few kirigami items. Can you be more specific? Are you suggesting I use another type of Menu?

ngraham added a subscriber: mart.Feb 5 2019, 5:05 PM

The problem is that you're using a disabled menu item for the title, which makes the font light automatically. Try setting supportsMouseEvents: false for it, and then you can style it however you want (+1 for bold, do font.weight: Font.Bold to get that)

Looks like I can't, none of the items in the object tree seem to support that property, and it's not in the Qt dox? It's only in a few kirigami items. Can you be more specific? Are you suggesting I use another type of Menu?

Darn, can't help then. Maybe @mart can?

@vpilo to move this forward, maybe just remove the header/title in the menu entirely. It's not really necessary after all.