Add option to easily configure and start a hotspot
ClosedPublic

Authored by jgrulich on Jan 3 2020, 1:04 PM.

Details

Summary

This adds a simple checkbox, which will create and stop hotspot. Hotspot can be also
configured from the KCM, where users can choose a name and password.

BUG: 413323

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.
jgrulich created this revision.Jan 3 2020, 1:04 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 3 2020, 1:04 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
jgrulich requested review of this revision.Jan 3 2020, 1:04 PM
jgrulich edited the summary of this revision. (Show Details)EditedJan 3 2020, 1:05 PM
jgrulich added reviewers: Plasma, ngraham.
jgrulich edited the summary of this revision. (Show Details)

There are two things which need work or I'm not sure about:

  1. If this should be placed as a checkbox in the applet
  2. We need a different icon for the hotspot, currently there is no such icon in the Plasma theme and freeze for Plasma 5.18 is soon. This is meant to be a kind request for someone who does icons for Plasma.
jgrulich edited the summary of this revision. (Show Details)Jan 3 2020, 1:11 PM
apol added a subscriber: apol.Jan 3 2020, 3:05 PM

Cool feature!

applet/contents/ui/Toolbar.qml
134

is it always prossible to create the hotspot? I'd expect it not to be possible at all sometimes (visible should be false then) and if the wifi is in use it might not be able to do it? (enabled false then).

ngraham added a reviewer: VDG.Jan 3 2020, 4:46 PM

Nice feature.

UI-wise, this would probably be better off as a button than a checkbox. Creating a hotspot is an action, and action == button. Once the hotspot has been created, the button text can change from "Create Hotspot..." to "Disable Hotspot". This is a bit nonstandard HIG-wise, but I think it's appropriate rnough here.

If you need an icon, please file a bug in Breeze | Icons and we'll get on it ASAP. Even if the icon doesn't get in in time for Plasma 5.18, having an iconless button isn't the end of the world.

mthw added a subscriber: mthw.Jan 3 2020, 6:43 PM

@apol You are right, it is not possible to create a hotspot if one is already connected to a WiFi network. Currently enabling hotspot disables your previous connection (WiFi) and hides available WiFi networks.

alexde added a subscriber: alexde.EditedJan 3 2020, 7:53 PM

UI-wise, this would probably be better off as a button than a checkbox.

Personally, I'm more inclined to a checkbox. Why is "turn wifi on/off" not an action? Right now, I don't see the big difference.

Is the new "hotspot" equivalent to the "shared wifi", you can already set up?
If so, without really reading the patch details yet, just some remarks or ideas:

  1. If you have configured several hotspots in the KCM, you probably need to choose a default one or a specific one when activating the hotspot.
  2. What do you think about adding a small configure button for the default hotspot directly on the plasma-nm frontend next to the hotspot checkbox/button?
    • That would make it much easier for newcomers to use and configure their hotspot without first digging in the KCM.
  3. I also think that a hotspot (shared wifi) connection should be better separated from the other wifi connections in the KCM. If I create a new hotspot in the KCM now, it hides in the long list of known connections.
  4. That's somehow also true for the plasma-nm list of available networks: It's not really clear that a hotspot in the list is a indeed hotspot but not another access point.
In D26392#587159, @mthw wrote:

@apol You are right, it is not possible to create a hotspot if one is already connected to a WiFi network. Currently enabling hotspot disables your previous connection (WiFi) and hides available WiFi networks.

Is this also true if one has two wifi network cards? IIRC I could run both with an additional usb netwok card simultaneously.


@jgrulich do you intend to create another patch to display all connected clients, similiar to the list of active connections? :)

In D26392#587159, @mthw wrote:

@apol You are right, it is not possible to create a hotspot if one is already connected to a WiFi network. Currently enabling hotspot disables your previous connection (WiFi) and hides available WiFi networks.

That's a question. Do we want to allow to create a hotspot if users are already connected to w WiFi network? You are allowed to do this for example on Android. Users might not realize why they are not allowed to create a hotspot and disconnecting from WiFi doesn't seem to me be a good first step.

UI-wise, this would probably be better off as a button than a checkbox.

Personally, I'm more inclined to a checkbox. Why is "turn wifi on/off" not an action? Right now, I don't see the big difference.

Is the new "hotspot" equivalent to the "shared wifi", you can already set up?
If so, without really reading the patch details yet, just some remarks or ideas:

  1. If you have configured several hotspots in the KCM, you probably need to choose a default one or a specific one when activating the hotspot.

This is meant to be as simple as possible, allowing users to create a hotspot without needing them to know the details. On Android you also don't get to choose.

  1. What do you think about adding a small configure button for the default hotspot directly on the plasma-nm frontend next to the hotspot checkbox/button?
    • That would make it much easier for newcomers to use and configure their hotspot without first digging in the KCM.
  2. I also think that a hotspot (shared wifi) connection should be better separated from the other wifi connections in the KCM. If I create a new hotspot in the KCM now, it hides in the long list of known connections.

I will probably use a new icon for this. I will need to request it.

  1. That's somehow also true for the plasma-nm list of available networks: It's not really clear that a hotspot in the list is a indeed hotspot but not another access point.

Same here. A different icon should be enough.

In D26392#587159, @mthw wrote:

@apol You are right, it is not possible to create a hotspot if one is already connected to a WiFi network. Currently enabling hotspot disables your previous connection (WiFi) and hides available WiFi networks.

Is this also true if one has two wifi network cards? IIRC I could run both with an additional usb netwok card simultaneously.

In case of multiple wifi cards we will choose the one which is not in use. Still any wifi card makes this option available, even if it's in use.


@jgrulich do you intend to create another patch to display all connected clients, similiar to the list of active connections? :)

I don't think NetworkManager provides this information.

jgrulich marked an inline comment as done.Jan 6 2020, 5:25 AM
jgrulich added inline comments.
applet/contents/ui/Toolbar.qml
134

See Component.onCompleted: { }. We make this visible based on handler.hotspotSupported, which checks NM version and whether there is available wifi card. It's done this way because it's a non-notifyable property.

jgrulich updated this revision to Diff 72847.Jan 6 2020, 8:14 AM
jgrulich marked an inline comment as done.

Use button instead of checkbox

I used ToolButton instead of a regular button, reason is that I don't think there is enough space. If there is modem device available, there will be three checkboxes on the left and with a regular button in various languages this might not be enough space.

This no longer compiles for me:

/home/nate/kde/src/plasma-nm/libs/handler.cpp: In member function ‘void Handler::createHotspot()’:
/home/nate/kde/src/plasma-nm/libs/handler.cpp:622:94: error: ‘addAndActivateConnection2’ is not a member of ‘NetworkManager’; did you mean ‘addAndActivateConnection’?
  622 |     QDBusPendingReply<QDBusObjectPath, QDBusObjectPath, QVariantMap> reply = NetworkManager::addAndActivateConnection2(connectionSettings->toMap(), wifiDev->uni(), QString(), options);
      |                                                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                                              addAndActivateConnection

It requires networkmanager-qt from master (upcoming 5.66 version).

This no longer compiles for me:

/home/nate/kde/src/plasma-nm/libs/handler.cpp: In member function ‘void Handler::createHotspot()’:
/home/nate/kde/src/plasma-nm/libs/handler.cpp:622:94: error: ‘addAndActivateConnection2’ is not a member of ‘NetworkManager’; did you mean ‘addAndActivateConnection’?
  622 |     QDBusPendingReply<QDBusObjectPath, QDBusObjectPath, QVariantMap> reply = NetworkManager::addAndActivateConnection2(connectionSettings->toMap(), wifiDev->uni(), QString(), options);
      |                                                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~
      |                                                                                              addAndActivateConnection

Did you manage to test it?

No, even with networkmanager-qt from git master, I still get the same build failure here. :(

No, even with networkmanager-qt from git master, I still get the same build failure here. :(

That's weird, because you can see here [1] that it's definitely there.

[1] - https://cgit.kde.org/networkmanager-qt.git/commit/?id=72a30f13c5ceac3e1b76d23f15d551d3689d3e15

Can someone please look into this review? Either try it or check the code? I would like to have this as part of Plasma 5.18 and deadline for that is in few days. @ngraham how is it going with the icon?

apol added a comment.Jan 11 2020, 7:48 PM
In D26392#587159, @mthw wrote:

@apol You are right, it is not possible to create a hotspot if one is already connected to a WiFi network. Currently enabling hotspot disables your previous connection (WiFi) and hides available WiFi networks.

That's a question. Do we want to allow to create a hotspot if users are already connected to w WiFi network? You are allowed to do this for example on Android. Users might not realize why they are not allowed to create a hotspot and disconnecting from WiFi doesn't seem to me be a good first step.

Leaving the user without a connection would be quite dishearting and possibly confusing.

I can imagine the use-case of: I am in this building with weird authentication (thinking university) and I want to share that connection with my phone, so I create a hotspot and we don't have to do weird configuration on the phone. Would apply to e.g. vpns too.

apol added a comment.Jan 11 2020, 7:51 PM

Otherwise lgtm

libs/models/networkmodel.cpp
517

unrelated change.

Can someone please look into this review? Either try it or check the code? I would like to have this as part of Plasma 5.18 and deadline for that is in few days. @ngraham how is it going with the icon?

@cblack is working on it IIRC. It's too late to show the icon in Plasma 5.18 anyway (the icon will be in the breeze-icons repo which is a framework, and Frameworks 5.66 which Plasma 5.18 relies on has already been tagged. So there's no huge rush there IMO.

I'm still unable to test this because of the odd build failure that I cannot understand, explain, or overcome. :/

In D26392#592136, @apol wrote:
In D26392#587159, @mthw wrote:

@apol You are right, it is not possible to create a hotspot if one is already connected to a WiFi network. Currently enabling hotspot disables your previous connection (WiFi) and hides available WiFi networks.

That's a question. Do we want to allow to create a hotspot if users are already connected to w WiFi network? You are allowed to do this for example on Android. Users might not realize why they are not allowed to create a hotspot and disconnecting from WiFi doesn't seem to me be a good first step.

Leaving the user without a connection would be quite dishearting and possibly confusing.

I can imagine the use-case of: I am in this building with weird authentication (thinking university) and I want to share that connection with my phone, so I create a hotspot and we don't have to do weird configuration on the phone. Would apply to e.g. vpns too.

What about allowing to create a hotspot if the WiFi is in use, but it's not used as primary connection (e.g. Ethernet or Mobile connections are used primarily). In that case the user wouldn't be without connection if we disconnect him.

Can someone please look into this review? Either try it or check the code? I would like to have this as part of Plasma 5.18 and deadline for that is in few days. @ngraham how is it going with the icon?

@cblack is working on it IIRC. It's too late to show the icon in Plasma 5.18 anyway (the icon will be in the breeze-icons repo which is a framework, and Frameworks 5.66 which Plasma 5.18 relies on has already been tagged. So there's no huge rush there IMO.

I'm still unable to test this because of the odd build failure that I cannot understand, explain, or overcome. :/

Then I'm not sure what to use instead. If I use a regular button with text, it won't definitely fit there with some translations. In Czech for example it would be "Vytvořit přístupový bod" which takes quite a lot of space, thus I would prefer just a button with an icon. If the icon is created, we can ship it together with plasma-nm for this release.

mthw added a comment.Jan 13 2020, 9:03 AM
This comment was removed by mthw.
mthw added a comment.Jan 13 2020, 9:18 AM

Is is possible to check if a hotspot icon exists, and if it does, show it, otherwise show a text?

meven added a subscriber: meven.Jan 13 2020, 9:24 AM
This comment was removed by meven.
mthw added a comment.Jan 13 2020, 9:32 AM

@meven But we already know it's too late for the icon, at least for distros that won't update to KF5 5.67 before Plasma 5.18. Are you saying this can't go forward without an icon?

If we use an icons-only button/checkbox, then this can't go in without the icon, which means it's deferred to Plasma 5.19. There's still (barely) time to get it into 5.18 if we make sure that we use a UI that has both icons and text.

apol added a comment.Jan 13 2020, 4:07 PM

What about allowing to create a hotspot if the WiFi is in use, but it's not used as primary connection (e.g. Ethernet or Mobile connections are used primarily). In that case the user wouldn't be without connection if we disconnect him.

Surely. And if the laptop is capable of sharing a wifi connection through a hotspot (or through ethernet), that should be possible as well.

jgrulich updated this revision to Diff 73437.Jan 13 2020, 4:47 PM
jgrulich marked an inline comment as done.

Address review comments:

  1. Add text to the toolbutton and make it checkable
  2. Allow to create hotspot only if WiFi is available or it's not used as primary connection
ngraham accepted this revision as: VDG.Jan 13 2020, 4:51 PM
apol added a comment.Jan 15 2020, 11:29 AM

Patch looks good to me overall. +1

libs/handler.cpp
615

I'd prefer the initializer list syntax
const QVariantMap options = { {QLatin1String("persist"), QLatin1String("volatile")} };

623

Use QOverload instead of static_cast, for readability.

jgrulich updated this revision to Diff 73614.Jan 15 2020, 11:49 AM
jgrulich marked an inline comment as done.
  • Code improvements
jgrulich marked an inline comment as done.Jan 15 2020, 11:50 AM
apol accepted this revision.Jan 15 2020, 11:52 AM

LGTM

This revision is now accepted and ready to land.Jan 15 2020, 11:52 AM
This revision was automatically updated to reflect the committed changes.