D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R116:5e55e45d8794: Add option to easily configure and start a 
hotspot (authored by jgrulich).

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26392?vs=73614=73615

REVISION DETAIL
  https://phabricator.kde.org/D26392

AFFECTED FILES
  applet/contents/ui/Toolbar.qml
  kcm/kcm.cpp
  kcm/qml/ConfigurationDialog.qml
  kcm/qml/main.qml
  kded/networkmanagement.notifyrc
  libs/configuration.cpp
  libs/configuration.h
  libs/declarative/enabledconnections.h
  libs/handler.cpp
  libs/handler.h
  libs/models/networkmodel.cpp

To: jgrulich, #plasma, ngraham, #vdg, apol
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Aleix Pol Gonzalez]

apol accepted this revision.
apol added a comment.
This revision is now accepted and ready to land.


  LGTM

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  hotspot

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg, apol
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich updated this revision to Diff 73614.
jgrulich marked an inline comment as done.
jgrulich added a comment.


  - Code improvements

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26392?vs=73437=73614

BRANCH
  hotspot

REVISION DETAIL
  https://phabricator.kde.org/D26392

AFFECTED FILES
  applet/contents/ui/Toolbar.qml
  kcm/kcm.cpp
  kcm/qml/ConfigurationDialog.qml
  kcm/qml/main.qml
  kded/networkmanagement.notifyrc
  libs/configuration.cpp
  libs/configuration.h
  libs/declarative/enabledconnections.h
  libs/handler.cpp
  libs/handler.h
  libs/models/networkmodel.cpp

To: jgrulich, #plasma, ngraham, #vdg
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Aleix Pol Gonzalez]

apol added a comment.


  Patch looks good to me overall. +1

INLINE COMMENTS

> handler.cpp:615
> +
> +QVariantMap options;
> +options.insert(QLatin1String("persist"), QLatin1String("volatile"));

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

> handler.cpp:623
> +connect(watcher, ::finished, this, 
> ::replyFinished);
> +connect(watcher, ::finished, this, 
> static_cast *)>(::hotspotCreated));
> +}

Use QOverload instead of static_cast, for readability.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich updated this revision to Diff 73437.
jgrulich marked an inline comment as done.
jgrulich added a comment.


  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

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26392?vs=72847=73437

BRANCH
  hotspot

REVISION DETAIL
  https://phabricator.kde.org/D26392

AFFECTED FILES
  applet/contents/ui/Toolbar.qml
  kcm/kcm.cpp
  kcm/qml/ConfigurationDialog.qml
  kcm/qml/main.qml
  kded/networkmanagement.notifyrc
  libs/configuration.cpp
  libs/configuration.h
  libs/declarative/enabledconnections.h
  libs/handler.cpp
  libs/handler.h
  libs/models/networkmodel.cpp

To: jgrulich, #plasma, ngraham, #vdg
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Aleix Pol Gonzalez]

apol added a comment.


  > 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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Matej Mrenica]

mthw added a comment.


  @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?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Méven Car]

meven added a comment.


  In D26392#592892 , @mthw wrote:
  
  > Is is possible to check if a hotspot icon exists, and if it does, show it, 
otherwise show a text?
  
  
  It is being taken care of : D26595 

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: meven, cblack, alexde, mthw, apol, plasma-devel, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Matej Mrenica]

mthw added a comment.


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

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: cblack, alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, 
mart

D26392: Add option to easily configure and start a hotspot

[Matej Mrenica]

mthw added a comment.


  A checkbox with a tooltip "Create hotspot" and an icon next to it should be 
enought, right? Just like Wifi and Airplane mode...

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: cblack, alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, 
mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  In D26392#592288 , @ngraham wrote:
  
  > In D26392#592133 , @jgrulich 
wrote:
  >
  > > 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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: cblack, alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, 
mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  In D26392#592136 , @apol wrote:
  
  > In D26392#588345 , @jgrulich 
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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: cblack, alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, 
mart

D26392: Add option to easily configure and start a hotspot

[Nathaniel Graham]

ngraham added a subscriber: cblack.
ngraham added a comment.


  In D26392#592133 , @jgrulich wrote:
  
  > 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. :/

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: cblack, alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, 
mart

D26392: Add option to easily configure and start a hotspot

[Aleix Pol Gonzalez]

apol added a comment.


  Otherwise lgtm

INLINE COMMENTS

> networkmodel.cpp:517
>  NetworkModelItem *originalItem = nullptr;
> -
>  for (NetworkModelItem *item : 
> m_list.returnItems(NetworkItemsList::Connection, connection)) {

unrelated change.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Aleix Pol Gonzalez]

apol added a comment.


  In D26392#588345 , @jgrulich 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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  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?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  In D26392#590884 , @ngraham wrote:
  
  > 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

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  In D26392#588994 , @ngraham wrote:
  
  > 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 reply = 
NetworkManager::addAndActivateConnection2(connectionSettings->toMap(), 
wifiDev->uni(), QString(), options);
  > |   
   ^
  > |   
   addAndActivateConnection
  >
  
  
  Did you manage to test it?

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


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

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Nathaniel Graham]

ngraham added a comment.


  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 reply = 
NetworkManager::addAndActivateConnection2(connectionSettings->toMap(), 
wifiDev->uni(), QString(), options);
  | 
 ^
  | 
 addAndActivateConnection

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich updated this revision to Diff 72847.
jgrulich marked an inline comment as done.
jgrulich added a comment.


  Use button instead of checkbox

REPOSITORY
  R116 Plasma Network Management Applet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D26392?vs=72682=72847

BRANCH
  hotspot

REVISION DETAIL
  https://phabricator.kde.org/D26392

AFFECTED FILES
  applet/contents/ui/Toolbar.qml
  kcm/kcm.cpp
  kcm/qml/ConfigurationDialog.qml
  kcm/qml/main.qml
  kded/networkmanagement.notifyrc
  libs/configuration.cpp
  libs/configuration.h
  libs/declarative/enabledconnections.h
  libs/handler.cpp
  libs/handler.h
  libs/models/networkmodel.cpp

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich marked an inline comment as done.
jgrulich added inline comments.

INLINE COMMENTS

> apol wrote in Toolbar.qml:133
> 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).

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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  In D26392#587200 , @alexde wrote:
  
  > > 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.
  
  > 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.
  
  I will probably use a new icon for this. I will need to request it.
  
  > 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.
  
  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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Alex Debus]

alexde added a comment.


  > 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.
  
  (I assume "hotspot" and "shared wifi" are equivalent.)
  
  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? :)

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: alexde, mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Matej Mrenica]

mthw added a comment.


  @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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: mthw, apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Nathaniel Graham]

ngraham added a reviewer: VDG.
ngraham added a comment.


  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.

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham, #vdg
Cc: apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Aleix Pol Gonzalez]

apol added a comment.


  Cool feature!

INLINE COMMENTS

> Toolbar.qml:133
> +
> +onClicked: {
> +if (checked) {

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).

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham
Cc: apol, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich added a comment.


  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

REPOSITORY
  R116 Plasma Network Management Applet

REVISION DETAIL
  https://phabricator.kde.org/D26392

To: jgrulich, #plasma, ngraham
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D26392: Add option to easily configure and start a hotspot

[Jan Grulich]

jgrulich created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
jgrulich requested review of this revision.

REVISION 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.

REPOSITORY
  R116 Plasma Network Management Applet

BRANCH
  hotspot

REVISION DETAIL
  https://phabricator.kde.org/D26392

AFFECTED FILES
  applet/contents/ui/Toolbar.qml
  kcm/kcm.cpp
  kcm/qml/ConfigurationDialog.qml
  kcm/qml/main.qml
  kded/networkmanagement.notifyrc
  libs/configuration.cpp
  libs/configuration.h
  libs/declarative/enabledconnections.h
  libs/handler.cpp
  libs/handler.h
  libs/models/networkmodel.cpp

To: jgrulich
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart