Re: [PATCH] Do not skip wifi slave connections

[Thomas Haller]

On Thu, 2017-05-18 at 12:33 -0400, Nikolay Martynov wrote:
> > Anyway, the patch LGTM.
> 
> Thanks!

Hi,

patch merged:
https://git.gnome.org/browse/network-manager-applet/commit/?id=15187f557234ee63d2d4730a16ba1c78bc43a516

Thanks,
Thomas

signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH] Do not skip wifi slave connections

[Nikolay Martynov]

Hi.

2017-05-18 12:22 GMT-04:00 Beniamino Galvani :
> On Thu, May 18, 2017 at 11:42:47AM -0400, Nikolay Martynov wrote:
>> Just to clarify: this patch only affects wifi bonded connections.
>> 'Classic' ethernet bond slaves are still skipped - so this change
>> should not affect existing users.
>> I think the intention of original patch was to hide ethernet ones.
>> The problem with wifi slaves is that they are not hidden since they
>> actually come from scan results - they still pop up in the list of
>> available APs. And this is the good thing - this means I can connect
>> and disconnect wifi bond slave at will from the applet.
>> So, with this in mind - could you please clarify why you think this is
>> not the right thing to do so I could try to address that? :)
>>
>
> Since wifi slave connection are displayed only if the matching SSID is
> found, and they don't waste space in the menu because they are grouped
> in the AP submenu, I think it's ok to display them.
>
> On the other hand, you still wouldn't be able to control the bond and
> the ethernet slave from the applet, so I wonder if this is really
> useful.
  As I've sort of tried to explain in comment to the patch:
  * Before this patch clicking on AP that is bond slave makes NM
create new connection for that AP name (i.e. it asks for secrets and
everything) - this is not what user would normally want because he
already has this SSID configured as bond slave
  * With this patch clicking same AP actually establishes bond slave
connection that was configured before and doesn't create a new one.
  From this perspective this is actually useful.

>
> Anyway, the patch LGTM.

Thanks!


-- 
Martynov Nikolay.
Email: mar.ko...@gmail.com
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH] Do not skip wifi slave connections

[Beniamino Galvani]

On Thu, May 18, 2017 at 11:42:47AM -0400, Nikolay Martynov wrote:
> Just to clarify: this patch only affects wifi bonded connections.
> 'Classic' ethernet bond slaves are still skipped - so this change
> should not affect existing users.
> I think the intention of original patch was to hide ethernet ones.
> The problem with wifi slaves is that they are not hidden since they
> actually come from scan results - they still pop up in the list of
> available APs. And this is the good thing - this means I can connect
> and disconnect wifi bond slave at will from the applet.
> So, with this in mind - could you please clarify why you think this is
> not the right thing to do so I could try to address that? :)
> 

Since wifi slave connection are displayed only if the matching SSID is
found, and they don't waste space in the menu because they are grouped
in the AP submenu, I think it's ok to display them.

On the other hand, you still wouldn't be able to control the bond and
the ethernet slave from the applet, so I wonder if this is really
useful.

Anyway, the patch LGTM.

Beniamino


signature.asc
Description: PGP signature
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH] Do not skip wifi slave connections

[Nikolay Martynov]

Hi.

Thanks for your response!

2017-05-18 11:32 GMT-04:00 Thomas Haller :
> On Tue, 2017-05-16 at 22:13 -0400, Nikolay Martynov wrote:
>> If slave connection is a wifi connection we should still display it
>> Otherwise this AP gets scanned regardless, but when user clicks it
>> new
>> network is created in NM's configuation - which is unlikely what user
>> desired
>
> Hi Nikolay,
>
> I see what the patch does, but I am not convinced that it's the right
> thing to do.
>
> In a way, it changes
> https://git.gnome.org/browse/network-manager-applet/commit/?h=0abe8a8f46bcdce5907882ba355ec69156b33164
>
> Other opinions?

Just to clarify: this patch only affects wifi bonded connections.
'Classic' ethernet bond slaves are still skipped - so this change
should not affect existing users.
I think the intention of original patch was to hide ethernet ones.
The problem with wifi slaves is that they are not hidden since they
actually come from scan results - they still pop up in the list of
available APs. And this is the good thing - this means I can connect
and disconnect wifi bond slave at will from the applet.
So, with this in mind - could you please clarify why you think this is
not the right thing to do so I could try to address that? :)

Thanks!

-- 
Martynov Nikolay.
Email: mar.ko...@gmail.com
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

Re: [PATCH] Do not skip wifi slave connections

[Thomas Haller]

On Tue, 2017-05-16 at 22:13 -0400, Nikolay Martynov wrote:
> If slave connection is a wifi connection we should still display it
> Otherwise this AP gets scanned regardless, but when user clicks it
> new
> network is created in NM's configuation - which is unlikely what user
> desired

Hi Nikolay,

I see what the patch does, but I am not convinced that it's the right
thing to do.

In a way, it changes
https://git.gnome.org/browse/network-manager-applet/commit/?h=0abe8a8f46bcdce5907882ba355ec69156b33164

Other opinions?


Thomas

signature.asc
Description: This is a digitally signed message part
___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list

[PATCH] Do not skip wifi slave connections

[Nikolay Martynov]

If slave connection is a wifi connection we should still display it
Otherwise this AP gets scanned regardless, but when user clicks it new
network is created in NM's configuation - which is unlikely what user desired

Signed-off-by: Nikolay Martynov 
---
 src/applet.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/applet.c b/src/applet.c
index 0ee9eb8e..2929863d 100644
--- a/src/applet.c
+++ b/src/applet.c
@@ -286,12 +286,13 @@ applet_get_all_connections (NMApplet *applet)
all_connections = nm_client_get_connections (applet->nm_client);
connections = g_ptr_array_new_full (all_connections->len, 
g_object_unref);
 
-   /* Ignore slave connections */
+   /* Ignore slave connections unless they are wifi connections */
for (i = 0; i < all_connections->len; i++) {
connection = all_connections->pdata[i];
 
s_con = nm_connection_get_setting_connection (connection);
-   if (s_con && !nm_setting_connection_get_master (s_con))
+   if (s_con && (!nm_setting_connection_get_master (s_con)
+   || nm_connection_get_setting_wireless 
(connection)))
g_ptr_array_add (connections, g_object_ref 
(connection));
}
 
-- 
2.11.0

___
networkmanager-list mailing list
networkmanager-list@gnome.org
https://mail.gnome.org/mailman/listinfo/networkmanager-list