Make it possible to show the QR of a network connection
ClosedPublic

Authored by apol on Jul 14 2019, 7:31 PM.

Details

Summary

It's useful to be able to share a network connection by showing its QR
on the device so others can take a picture of it.
It shows a maximised window that will disappear when we click on it.

Test Plan

Looked at my wifi's QR

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.
apol created this revision.Jul 14 2019, 7:31 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 14 2019, 7:31 PM
Restricted Application added 1 blocking reviewer(s): jgrulich. · View Herald Transcript
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.Jul 14 2019, 7:31 PM

The more things we put into the context menu, the more important I think it becomes to port the delegates to use a Krigami basiclistitem and make the items in the context menu show up on hover, with some of the more esoteric ones being in an overflow menu. We should probably also first extend it to support actions that show their text, so the connext/disconnect action has the most visual prominence.

Anyway, enough rambling, that would be material for another patch anyway, I just wanted to mention it.

applet/contents/ui/ConnectionItem.qml
170

"Show network's QR code"

applet/contents/ui/ShowQR.qml
46

I don't think you need to use a ColumnLayout if it'll have only one item in it. You can just add margins to the Prison item itself.

kded/networkmanagement.notifyrc
817 ↗(On Diff #61752)

"Show network's QR code"

apol marked 3 inline comments as done.Jul 14 2019, 11:21 PM
apol added inline comments.
applet/contents/ui/ShowQR.qml
46

Yes, good catch. I had something there and ended up removing it.

apol updated this revision to Diff 61767.Jul 14 2019, 11:22 PM
apol marked an inline comment as done.

Address Nate's comments

apol added a comment.Jul 14 2019, 11:23 PM

The more things we put into the context menu, the more important I think it becomes to port the delegates to use a Krigami basiclistitem and make the items in the context menu show up on hover, with some of the more esoteric ones being in an overflow menu. We should probably also first extend it to support actions that show their text, so the connext/disconnect action has the most visual prominence.

I agree, this view could be improved.

ngraham added inline comments.Jul 14 2019, 11:53 PM
applet/contents/ui/ShowQR.qml
54

These won't do anything because it's not in a Layout anymore

apol marked an inline comment as done.Jul 15 2019, 12:33 AM
apol updated this revision to Diff 61773.Jul 15 2019, 1:22 AM

Removed unnecessary things

One thing I noticed when trying this out is that when the full-screen window showing the QR code appears, plasma-nm's system tray popup doesn't close, and the QR code window is drawn underneath it. On my 16x9 screen, the system tray popup just barely doesn't overlap the QR code, but I bet it would on a 4:3 screen, or a screen in portrait orientation.

Also maybe the menu item could have the icon view-barcode. It's not idea but maybe it's better than nothing?

apol added a comment.Jul 15 2019, 1:49 AM

One thing I noticed when trying this out is that when the full-screen window showing the QR code appears, plasma-nm's system tray popup doesn't close, and the QR code window is drawn underneath it. On my 16x9 screen, the system tray popup just barely doesn't overlap the QR code, but I bet it would on a 4:3 screen, or a screen in portrait orientation.

Weird, for me the window stays on top. Maybe it's a X11 vs Wayland thing?

Also maybe the menu item could have the icon view-barcode. It's not idea but maybe it's better than nothing?

Added the icon.

Yeah maybe. Here's what happens on X11:

jgrulich added inline comments.Jul 15 2019, 4:48 AM
applet/contents/ui/ConnectionItem.qml
171

I think this should be limited to wireless connections only (Type == PlasmaNM.Enums.Wireless).

libs/handler.cpp
188

Each item in the model already exposes SecurityType so you don't need to search for the AP and find security type it uses.

212

Networks using WPA2-EAP or DynamicWEP don't have secrets stored as 802-11-wireless-security, these are under 802-1-x. Anyway, I think we should maybe filter these out, you will most likely won't be sharing them anyway. You can do that in the applet already, check predictableWirelessPassword property.

broulik added inline comments.
applet/contents/ui/ShowQR.qml
26

Add this as RUNTIME Cmake dependency

28

Can you instad do a StackView and show the barcode inside the applet like Klipper does it? This would make it more consistent.

libs/handler.cpp
230

What does this do?

libs/handler.h
73

Docs, coding style.

apol updated this revision to Diff 61835.Jul 16 2019, 1:58 AM
apol marked 5 inline comments as done.

addressed comments

apol updated this revision to Diff 61836.Jul 16 2019, 2:08 AM

Fix visible logic

apol added inline comments.Jul 16 2019, 2:09 AM
applet/contents/ui/ShowQR.qml
28

I thought about that too, my use-case was though to give access to wifi on somebody by my side, making them aim at my screen would be weird.

libs/handler.cpp
230

Yeah forgot to remove it.

jgrulich added inline comments.Jul 16 2019, 6:02 AM
libs/handler.cpp
170

I actually thought that instead of all this logic, where you search for wireless device, AP and then to get the security type, you will pass those information from the applet, instead of the specificObject. Even ssid is exposed from the model so you can just pass ssid as an argument and security type as another one and almost all this code below can go away.

apol updated this revision to Diff 61885.Jul 17 2019, 1:16 AM

Pass different data as function arguments as suggested

jgrulich accepted this revision.Jul 17 2019, 4:15 AM

Much simple! Thanks.

This revision is now accepted and ready to land.Jul 17 2019, 4:15 AM
apol updated this revision to Diff 61944.Jul 18 2019, 2:17 AM

Hopefully address Nate's complaint on how it ends up behind the expanded plasmoid

ngraham accepted this revision.Jul 18 2019, 3:04 AM

Yep, that did it!

This revision was automatically updated to reflect the committed changes.