GNOME Bugzilla – Bug 723570
More network indicator updates
Last modified: 2021-07-05 14:46:40 UTC
Following the SystemStatus wiki page.
Created attachment 268018 [details] [review] NetworkMenu: use the carrier state to the decide if wired should be shown The design says "when a network cable is connected", not "When a network cable is connected and the link is up and we have an IP etc. etc." (which is what ACTIVATED would imply).
Created attachment 268019 [details] [review] NetworkMenu: recognize Android tethered phones and mark them as wwan Android phones tethered via USB are exposed as ethernet devices with the rndis_host driver by NetworkManager (because that's how they are implemented in the lower layers) but the UI should point to them as phones, with a different icon and position in the menu.
Created attachment 268020 [details] [review] NMDeviceWired: don't unconditionally access device.active_connection The active_connection might be null, but the NMApplet code might still belive the device wrapper is the primary connection (because dbus signals are emitted one after the other). (Also, remove an old and wrong comment about bluetooth in wwan code) Fixes: (gnome-shell:22511): Gjs-WARNING **: JS ERROR: Exception in callback for signal: icon-changed: TypeError: this._device.active_connection is null NMDeviceWired<.getIndicatorIcon@resource:///org/gnome/shell/ui/status/network.js:475 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 NMApplet<._updateIcon@resource:///org/gnome/shell/ui/status/network.js:1790 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 _emit@resource:///org/gnome/gjs/modules/signals.js:124 NMConnectionSection<._addConnection/<@resource:///org/gnome/shell/ui/status/network.js:265 _emit@resource:///org/gnome/gjs/modules/signals.js:124 NMConnectionItem<._sync@resource:///org/gnome/shell/ui/status/network.js:137 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 NMConnectionItem<.setActiveConnection@resource:///org/gnome/shell/ui/status/network.js:169 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 NMConnectionDevice<._activeConnectionChanged@resource:///org/gnome/shell/ui/status/network.js:327 wrapper@resource:///org/gnome/gjs/modules/lang.js:169
Created attachment 268021 [details] [review] NetworkMenu: update for latest designs Distinguish reaching the full internet from being in a non-routed LAN, and show a different icon when being an hotspot master.
Review of attachment 268018 [details] [review]: OK.
Thanks for this Giovanni!
Review of attachment 268020 [details] [review]: Ugh. I really dislike this, because it means we would show offline for a split second before the other D-Bus signal is fired. Maybe we should defer this updating to a BEFORE_REDRAW?
Review of attachment 268021 [details] [review]: Looks OK otherwise. ::: js/ui/status/network.js @@ +310,3 @@ + _canReachInternet: function() { + if (this._client.primary_connection != this._device.active_connection) + return true; I don't quite understand this. The primary connection should always be the active connection if there is an active connection.
(In reply to comment #8) > Review of attachment 268021 [details] [review]: > > Looks OK otherwise. > > ::: js/ui/status/network.js > @@ +310,3 @@ > + _canReachInternet: function() { > + if (this._client.primary_connection != this._device.active_connection) > + return true; > > I don't quite understand this. The primary connection should always be the > active connection if there is an active connection. Not if you have multiple active interfaces
Review of attachment 268019 [details] [review]: ::: js/ui/status/network.js @@ +433,3 @@ this.parent(client, device, settings); + // FIXME: find out what iPhones use and add it here iPhones use the ipheth driver. @@ +435,3 @@ + // FIXME: find out what iPhones use and add it here + if (device.driver == 'rndis_host') { + // Android tethering Not sure about this one rndis_host is a generic driver that is used by pretty much all usb to Ethernet adapters. Android might be the most common user of the driver currently but given that many ultrabooks don't have any Ethernet port I am not so sure how well this works out in practice.
<drago01> danw: re https://bugzilla.gnome.org/show_bug.cgi?id=723570#c10 is there a better way to detect "android" ? <Services> Bug 723570: normal, Normal, ---, gnome-shell-maint, NEW, More network indicator updates <danw> i don't know... probably not given that "android" really only defines software, not hardware <danw> in the general case, it's not possible to distinguish a tethered phone from a usb ethernet adapter <drago01> ok that's what I though (well expect the iphone because nothing else uses that driver) <danw> (Although, eg, I don't think Samsung makes any USB ethernet adapters, and there are probably other heuristics or hardcoded checks you could do to cover at least the most popular android phones)
(In reply to comment #9) > Not if you have multiple active interfaces Oh, right.
(Note that the NM team wanted to add per-device connectivity fields along with a per-device activating_connection as well, we just haven't gotten there yet)
(In reply to comment #7) > Review of attachment 268020 [details] [review]: > > Ugh. I really dislike this, because it means we would show offline for a split > second before the other D-Bus signal is fired. Maybe we should defer this > updating to a BEFORE_REDRAW? It makes no difference: if we get the update before the next frame is drawn, we draw with both updates, if we don't, we draw with the intermediate state. There's no way around that, short of a timeout. (In reply to comment #13) > (Note that the NM team wanted to add per-device connectivity fields along with > a per-device activating_connection as well, we just haven't gotten there yet) Heh... connectivity is a property of the default route. Secondary interfaces might be limited for a good reason, we should not show the question mark for them. (In reply to comment #11) > <drago01> danw: re https://bugzilla.gnome.org/show_bug.cgi?id=723570#c10 is > there a better way to detect "android" ? > <Services> Bug 723570: normal, Normal, ---, gnome-shell-maint, NEW, More > network indicator updates > <danw> i don't know... probably not given that "android" really only defines > software, not hardware > <danw> in the general case, it's not possible to distinguish a tethered phone > from a usb ethernet adapter > <drago01> ok that's what I though (well expect the iphone because nothing else > uses that driver) > <danw> (Although, eg, I don't think Samsung makes any USB ethernet adapters, > and there are probably other heuristics or hardcoded checks you could do to > cover at least the most popular android phones) So I understand our only hope is a udev rule that identifies them by Vendor/Product ID? Ugh...
Created attachment 268098 [details] [review] NetworkMenu: recognize tethered iPhones and mark them as wwan iPhones tethered via USB are exposed as ethernet devices with the ipheth driver by NetworkManager (because that's how they are implemented in the lower layers) but the UI should point to them as phones, with a different icon and position in the menu. This patch was originally written for Android, except that it uses a more general ethernet driver, so we need a special marker for Android phones, probably through an udev rule. Rebasing this away from the stack would have been a huge pain, and it's such a low hanging fruit... Not properly tested.
Comment on attachment 268018 [details] [review] NetworkMenu: use the carrier state to the decide if wired should be shown Attachment 268018 [details] pushed as c4aeaf7 - NetworkMenu: use the carrier state to the decide if wired should be shown
(In reply to comment #14) > (In reply to comment #7) > > Review of attachment 268020 [details] [review] [details]: > > > > Ugh. I really dislike this, because it means we would show offline for a split > > second before the other D-Bus signal is fired. Maybe we should defer this > > updating to a BEFORE_REDRAW? > > It makes no difference: if we get the update before the next frame is drawn, we > draw with both updates, if we don't, we draw with the intermediate state. > There's no way around that, short of a timeout. > > (In reply to comment #13) > > (Note that the NM team wanted to add per-device connectivity fields along with > > a per-device activating_connection as well, we just haven't gotten there yet) > > Heh... connectivity is a property of the default route. Secondary interfaces > might be limited for a good reason, we should not show the question mark for > them. > > (In reply to comment #11) > > <drago01> danw: re https://bugzilla.gnome.org/show_bug.cgi?id=723570#c10 is > > there a better way to detect "android" ? > > <Services> Bug 723570: normal, Normal, ---, gnome-shell-maint, NEW, More > > network indicator updates > > <danw> i don't know... probably not given that "android" really only defines > > software, not hardware > > <danw> in the general case, it's not possible to distinguish a tethered phone > > from a usb ethernet adapter > > <drago01> ok that's what I though (well expect the iphone because nothing else > > uses that driver) > > <danw> (Although, eg, I don't think Samsung makes any USB ethernet adapters, > > and there are probably other heuristics or hardcoded checks you could do to > > cover at least the most popular android phones) > > So I understand our only hope is a udev rule that identifies them by > Vendor/Product ID? > Ugh... Yeah ... Vendor might be good enough though.
(In reply to comment #17) > > > <danw> (Although, eg, I don't think Samsung makes any USB ethernet adapters, > > > and there are probably other heuristics or hardcoded checks you could do to > > > cover at least the most popular android phones) > > > > So I understand our only hope is a udev rule that identifies them by > > Vendor/Product ID? > > Ugh... > > Yeah ... Vendor might be good enough though. <hadess> danw, pretty sure you can get samsung branded usb ethernet adapters for their ultrabooks <danw> ah... so you'd need to go down to the product id level <hadess> i'm pretty sure we'd see other interfaces than plain usbnet on an android phone
Created attachment 268492 [details] [review] NetworkMenu: fix connection name update If the connection name is changed, the UUID doesn't necessarily, so checkConnection would take the early return path. Make sure we update the existing menu item too.
Created attachment 268493 [details] [review] NetworkMenu: sort connections by name only We don't watch for timestamp changes, and sorting by name keeps the order consistent and predictable. In practice, there should be at most 3 or 4 elements, so the user will always read all them at once and sorting is irrelevant.
Created attachment 268494 [details] [review] NetworkMenu: use radio items instead of switches for multiple profiles Multiple connections for the same device are mutually exclusive, so a switch is not the appropriate UI metaphor. Use a radio item instead, and provide a separate "Turn off" item to disconnect. Behavior when there is only one connection is not changed, there is a single Connect/Turn off item.
Created attachment 268498 [details] [review] NetworkMenu: recognize tethered phones and mark them as wwan Phones tethered via USB are exposed as ethernet devices by NetworkManager (because that's how they are implemented in the lower layers) but the UI should point to them as phones, with a different icon and position in the menu. To do so, we use an helper method in libnm-gtk, which in turn uses a udev rule that tags the network device specially. See bug 723906 for the new libnm-gtk API.
Review of attachment 268493 [details] [review]: OK.
Review of attachment 268498 [details] [review]: Does this require a nmgtk patch to land first?
Review of attachment 268494 [details] [review]: I'm not sure if NetworkManager supports MultiVPN yet, but the big ripple with VPN is that there can be more than one VPN connection, so we need a switch UI there. We initially didn't show connections anywhere else, so that's why we showed a switch. But if MultiVPN isn't supported for now, let's just do this and switch (heh) it back when it is.
Review of attachment 268492 [details] [review]: This needs a comment to be more clear.
Attachment 268020 [details] pushed as bde1451 - NMDeviceWired: don't unconditionally access device.active_connection Attachment 268021 [details] pushed as f43ff45 - NetworkMenu: update for latest designs Attachment 268492 [details] pushed as 488a426 - NetworkMenu: fix connection name update Attachment 268493 [details] pushed as 2bb3aed - NetworkMenu: sort connections by name only Attachment 268494 [details] pushed as fe7ece1 - NetworkMenu: use radio items instead of switches for multiple profiles Leaving the tether patch until the new API lands in nm-gtk or nm-glib.
*** Bug 684953 has been marked as a duplicate of this bug. ***
(In reply to Jasper St. Pierre (not reading bugmail) from comment #24) > Review of attachment 268498 [details] [review] [review]: > > Does this require a nmgtk patch to land first? It requires the nm-applet helper from the blocker bug.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.