GNOME Bugzilla – Bug 786736
WiFi enhancements for AP list
Last modified: 2018-01-21 15:04:32 UTC
Patches: wifi: update the list when signal strength changes The best AP can be chosen even if no AP is added/removed. wifi: add a scan button Manual scanning is useful when you want to discover new APs while connected to another AP.
Created attachment 358302 [details] [review] 0001-wifi-update-the-list-when-signal-strength-changes.patch
Created attachment 358303 [details] [review] 0002-wifi-add-a-scan-button.patch
Review of attachment 358302 [details] [review]: ::: panels/network/net-device-wifi.c @@ +274,3 @@ + for (i = 0; i < aps->len; i++) { + ap = NM_ACCESS_POINT (g_ptr_array_index (aps, i)); + g_signal_connect_object (ap, "notify", this will notify us for things other strength. should be notify::strength @@ +292,2 @@ populate_ap_list (device_wifi); + nm_device_wifi_connect_access_points (nm_device_wifi, device_wifi); this means we'll potentially add multiple signal connections to the same AP instance, overloading us with redundant notify signals emissions. we should only connect the AP instance that appeared here. note that, right now, this function is used both to handle APs being added as well as removed
Review of attachment 358303 [details] [review]: this needs UI design. perhaps a better approach, and one that doesn't require UI changes, is to have a background timer that periodically asks for scans while the wifi panel is open. also, doesn't NM scan all the time anyway? doesn't it have an API to request scans to be more frequent for a while?
Created attachment 358358 [details] [review] 0001-wifi-update-the-list-when-AP-properties-change.patch
Created attachment 358359 [details] [review] 0002-network-wifi.ui-fix-indentation.patch
Created attachment 358360 [details] [review] 0003-wifi-add-a-scan-button.patch
(In reply to Rui Matos from comment #3) > Review of attachment 358302 [details] [review] [review]: > > ::: panels/network/net-device-wifi.c > @@ +274,3 @@ > + for (i = 0; i < aps->len; i++) { > + ap = NM_ACCESS_POINT (g_ptr_array_index (aps, i)); > + g_signal_connect_object (ap, "notify", > > this will notify us for things other strength. should be notify::strength I actually meant all the properties including security and others that could possibly appear in the future. > @@ +292,2 @@ > populate_ap_list (device_wifi); > + nm_device_wifi_connect_access_points (nm_device_wifi, device_wifi); > > this means we'll potentially add multiple signal connections to the same AP > instance, overloading us with redundant notify signals emissions. we should > only connect the AP instance that appeared here. note that, right now, this > function is used both to handle APs being added as well as removed Fixed and separated add/remove. (In reply to Rui Matos from comment #4) > this needs UI design. perhaps a better approach, and one that doesn't > require UI changes, is to have a background timer that periodically asks for > scans while the wifi panel is open. > > also, doesn't NM scan all the time anyway? doesn't it have an API to request > scans to be more frequent for a while? Scanning doesn't have to be too frequent. The user can decide when they really need to re-scan (and that's what most other implementations do). Instead of wating for an unknown amount of time, I'd personally prefer to start scanning right away.
(In reply to Rui Matos from comment #4) > Review of attachment 358303 [details] [review] [review]: > > this needs UI design. And by this did you mean the UI in my patch needs improving? If so, what's the preferred UI layout? Thanks for your comments.
Comment on attachment 358360 [details] [review] 0003-wifi-add-a-scan-button.patch Separating this patch into #789869 as the refresh button is not related with the rest of the patches.
Review of attachment 358358 [details] [review]: Looks good to me (except a nit in the commit message that i'll fix)
Review of attachment 358359 [details] [review]: Sure.
Thanks for the patches. I updated the commit message, and pushed.