networkmanager: Add Wi-Fi support#22884
Conversation
martinpitt
added
the
no-test
This looks cool, but I wonder if we require the level of details, for example what is a "Mode" of a network? Looking closer I suppose this is to make it clear that it is network which requires credentials? In GNOME's WiFi settings it simply shows a lock next to the WiFi icon: 
|
|
@jelly great question! This currently shows "Infra" vs. "AdHoc" vs. "AP" vs. "Mesh". It's indeed not super interesting. I'm happy to drop the mode, and put the lock icon somewhere else. Thanks for showing the GNOME screenshot! This leaves out a lot of interesting things though -- The signal strength is very relevant, the Rate is still quite relevant when picking from multiple options, and it's not quite clear how this is sorted. In cities/apartments there are now regularly a dozen visible WiFis. |
Sorting seems to be on strength: 
The third Wifi endpoint stands out because it is "configured previously" so is ranked higher. |
martinpitt
force-pushed
the
wifi-poc
branch
2 times, most recently
from
3ea5e1d to
892bca4
Compare
|
Nice, code looks good to me. I think the general UI approach (list all access points directly on the details page instead of in a dropdown or dialog) makes sense for Cockpit. I can't say whether it makes sense for Anaconda. It might be too "experty". The Cockpit Storage page clearly is an expert mode thing that most people are not supposed to enter. But connecting your machine to Wifi in order to complete the installation is probably a step that many people will have to do. Is the full Cockpit Networking UI appropriate for that? Most of the stuff on the overview is irrelevant and you have to look closely to find your wifi device. Or would there be a short-cut that opens the details page of a wifi device directly from Anaconda, without having to go via the Cockpit Networking Overview? Smaller things: "Forgetting" could be in a kebab. Does it make sense to list hidden networks if they have no connect button? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@mvollmer thanks for reviewing! The "anaconda integration" and "design review" is outside of this initial PR, it's tracked by https://issues.redhat.com/browse/COCKPIT-1764 and https://issues.redhat.com/browse/COCKPIT-1765. This was originally meant to just be a demo, but I think by now we sort of agreed that we can land this -- it doesn't hurt, and already is useful to e.g. connect you RasPi etc.
Good idea, done in a new commit for easier re-review.
I'm not sure. If your home net is hidden, and there is just one, it's still a good indication of how strong it is, or if it is present in the first place. I'm also fine with hiding it for the first iteration, I don't have a strong opinion. This could be part of the design review. I also implemented your cancelling during connection request in a separate commit, again for easier review. I also fixed the flake. Screenshots updated. |
martinpitt
force-pushed
the
wifi-poc
branch
2 times, most recently
from
f972557 to
24e99cf
Compare
|
Next commit hides the Details column on the Overview if there aren't any. With that there is on effective visual change on machines without WiFi. So this pixel failure should stop happening now. |
|
One thing I noticed when trying to try out searching for network is that the whole page is scrollable, that feels a bit janky. 
The current design shows hidden networks, I wonder how useful that is. I have 5 hidden networks which I can't connect too either. Maybe we should filter those instead? I have an existing "profile" which is shown as with a The bigger problem I had is that my current saved connection is invalid, clicking So we are missing some error handling. This Apartment has two access points GNOME settings shows them as one but the current UI shows both: 
In universities and offices is also the case so that would then many instances of the same "network". When I forget the network and re-connect it shows a different icon and not the pin anymore. That feels very confusing. Also after keeping the page open for a while it only shows me the connected network, the previously scanned networks seem to be gone. Is that expected? |
martinpitt
changed the title
Indeed it looks quite extensive in Brno office: One thing I noticed is that if I disconnect from RH Wi-Fi it disconnects as expected, but when I click |
martinpitt
removed
the
no-test
Not sure what you mean or what to do about that.. It feels quite okay to me, but I'm also rather unsensitive to that kind of problem. Can you please elaborate? Or we keep that for the "UX design review"?
Moved to the bottom as "N hidden networks" without actions, as discussed.
Yeah, suggestions appreciated! (→ UX design review)
Ah, all my WPA connections have their password directly in the .nmconnection file. This is using some per-user credentials I figure, nm-applet doesn't seem to do that. I'll look at that!
Fixed, as discussed. It now only shows the strongest AP for each SSID.
You mean it shows the ((o)) icon for "connected"? Again, suggestions appreciated. Would it be clearer to show that and the pin?
Yeah, that just seems how NM does it.. I don't want to cache that information in the UI to be honest. @tomasmatus I believe your feedback is also covered by the above? Thanks! |
cockpituous
left a comment
cockpituous
left a comment
There was a problem hiding this comment.
There are more than 10 code coverage comments, see the full report here.
Update the model to collect Wi-Fi access points and provide a scan method. On a Wi-Fi device page, show a table of available networks with mode, signal strength, rate, and security status. The currently active connection (if any) is always at the top, followed by known networks (with a saved connection), then unknown networks sorted by descending signal strength (by default, the table can also be sorted by name), and finally the number of hidden networks. Allow the user to connect, disconnect, and forget networks, and re-scan. In the Networking overview, if there are any Wi-Fi devices, add a "Details" column which shows the number of available and the currently connected networks. We can test this with `mac80211_hwsim` and `hostapd`, to create the kinds of networks that we want to cover: one open (with two APs), two WPA (so that we can have one active and one inactive one), and one hidden network. https://issues.redhat.com/browse/COCKPIT-1751
We don't want the dialog to start out with invalid helper texts. As we disable the connect button anyway while password (or SSID for hidden) is empty, there is then never a case when we actually show them, so just drop them. cockpit-project#22884 (comment)
Worked before anyway, presumably the activeConnection ref somehow changes (D-Bus proxy recreates it?) cockpit-project#22884 (comment)
Most of these only appear in debug statements. connectToAP() actually uses the value for configuring NM, but we only call it on known networks, so assert that Ssid is valid. cockpit-project#22884 (comment)
It's too expensive and not useful enough on the overview. OTOH, trigger a scan on each opening of a WiFi device details page, so that the information there is current and useful. This means that we the number of APs in the overview is less reliable, but that's fine -- after a while NM forgets them anyway. Curiously the tests (which check "3 networks" on the overview) are still fine -- apparently NM does a passive scan automatically on startup, or for a newly appearing device? cockpit-project#22884 (comment)
Less code duplication and traverse the AP list only once. Also simplify and explain the MAC duplication dance. Debug-log available APs. cockpit-project#22884 (comment)
Set/use a `data-known` flag instead. cockpit-project#22884 (comment)
O(n²) → O(n log n). Still not great, but difficult to improve. cockpit-project#22884 (comment)
When connecting to a Wi-Fi network with a saved connection but no stored password (no `psk=` in `.nmconnection` file), the Networking page did not show any error or other visible status change. Sadly, the NetworkManager API does not make this easy: It transitions through device states (via D-Bus signals) very quickly (PREPARE → CONFIG → NEED_AUTH → FAILED → DISCONNECTED), and React batches all these property updates together. By the time the `useEffect` runs, the device has already returned to DISCONNECTED state and the interesting failure reason (NM_DEVICE_STATE_REASON_NO_SECRETS) already was reset and the state is just NM_DEVICE_STATE_DISCONNECTED. Fix this by capturing the failure reason in the D-Bus signal handler and storing it on the device object. The UI can then retrieve and consume this cached reason when checking for failures. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
cockpituous
left a comment
cockpituous
left a comment
There was a problem hiding this comment.
There are more than 10 code coverage comments, see the full report here.
| function access_point_mode_to_text(mode) { | ||
| switch (mode) { | ||
| // NM_802_11_MODE_ADHOC | ||
| case 1: return _("Adhoc"); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| // NM_802_11_MODE_INFRA | ||
| case 2: return _("Infra"); | ||
| // NM_802_11_MODE_AP | ||
| case 3: return _("AP"); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| // NM_802_11_MODE_AP | ||
| case 3: return _("AP"); | ||
| // NM_802_11_MODE_MESH | ||
| case 4: return _("Mesh"); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| // NM_802_11_MODE_MESH | ||
| case 4: return _("Mesh"); | ||
| // subsumes NM_802_11_MODE_UNKNOWN | ||
| default: return _("Unknown"); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| exporters: [ | ||
| function (obj) { | ||
| // Find connection for this SSID (undefined if none exists) | ||
| obj.Connection = (self.get_settings()?.Connections || []).find(con => { |
There was a problem hiding this comment.
This added line is not executed by any test. Details
|
|
||
| prototype: { | ||
| activate: function(connection, specific_object) { | ||
| priv(this).lastFailureReason = undefined; // Clear stale failure reason from previous attempts |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| request_scan: function() { | ||
| utils.debug("request_scan: requesting scan for", this.Interface); | ||
| call_object_method(this, 'org.freedesktop.NetworkManager.Device.Wireless', 'RequestScan', {}) | ||
| .catch(error => { |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| call_object_method(this, 'org.freedesktop.NetworkManager.Device.Wireless', 'RequestScan', {}) | ||
| .catch(error => { | ||
| // RequestScan can fail if a scan was recently done, that's OK | ||
| console.warn("request_scan: scan failed for", this.Interface + ":", error.toString()); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| console.warn("wait_connection: connection failed for", this.Interface, "without captured reason"); | ||
| reject(new Error("Connection failed")); |
There was a problem hiding this comment.
These 2 added lines are not executed by any test. Details
| } | ||
| break; | ||
|
|
||
| case 20: // NM_DEVICE_STATE_UNAVAILABLE |
There was a problem hiding this comment.
This added line is not executed by any test. Details
|
|
||
| let accessPoints = dev.AccessPoints || []; | ||
| const accessPoints = dev.AccessPoints || []; | ||
| if (accessPoints.length === 0) |
There was a problem hiding this comment.
accessPoints is not used any more after this, it might be cleaner to also use dev.visibleSsids and dev.hiddenApCount here to determine whether to show the "Available networks" card. But on the other hand, I think it's better to always show the card but have a EmptyState saying "No available networks". This is UI design -> so no changes requested, but I wanted to write it down.
|
|
||
| const onCancel = () => { | ||
| if (connecting) { | ||
| if (connecting && createdConnection) { |
There was a problem hiding this comment.
Doesn't this extra check for createConnection mean that connecting to "known" APs can no longer be cancelled?
There was a problem hiding this comment.
Ah, but there is no dialog anyway when connecting to "known" APs, right?
There was a problem hiding this comment.
Correct, that just happens in the background. If the password is wrong (or another error occurs), there is this error popup that tells you the error message, and in the specific case of "no password available", suggests you to forget and re-connect.
| useInit(() => { | ||
| if (dev?.DeviceType === '802-11-wireless') { | ||
| utils.debug("Requesting initial WiFi scan for", dev_name); | ||
| dev.request_scan(); |
There was a problem hiding this comment.
It would be nice if the "Refresh" button would react to this (disabled with spinner), then the user knows what's happening.
| // Wait for a connection to complete | ||
| // For WiFi, pass expected_ssid to verify we connected to the right network | ||
| // Returns a Promise that resolves on success or cancel, rejects with {reason} on failure | ||
| wait_connection: function(expected_ssid) { |
There was a problem hiding this comment.
Nice, thanks! I really prefer calling functions like this from a event handler to sticking useEffects into render code. :-)
There was a problem hiding this comment.
Me too! It just took some time to explore and build the state machine, first in my head and then in code.
We don't want the dialog to start out with invalid helper texts. As we disable the connect button anyway while password (or SSID for hidden) is empty, there is then never a case when we actually show them, so just drop them. #22884 (comment)
Worked before anyway, presumably the activeConnection ref somehow changes (D-Bus proxy recreates it?) #22884 (comment)
Most of these only appear in debug statements. connectToAP() actually uses the value for configuring NM, but we only call it on known networks, so assert that Ssid is valid. #22884 (comment)
It's too expensive and not useful enough on the overview. OTOH, trigger a scan on each opening of a WiFi device details page, so that the information there is current and useful. This means that we the number of APs in the overview is less reliable, but that's fine -- after a while NM forgets them anyway. Curiously the tests (which check "3 networks" on the overview) are still fine -- apparently NM does a passive scan automatically on startup, or for a newly appearing device? #22884 (comment)
Less code duplication and traverse the AP list only once. Also simplify and explain the MAC duplication dance. Debug-log available APs. #22884 (comment)
Set/use a `data-known` flag instead. #22884 (comment)
O(n²) → O(n log n). Still not great, but difficult to improve. #22884 (comment)
6 participants
Update the model to collect Wi-Fi access points and provide a scan method.
On a Wi-Fi device page, show a table of available networks with mode, signal
strength, rate, and security status. The currently active connection (if any)
is always at the top, followed by known networks (with a saved connection) and
then unknown networks sorted by descending signal strenght (by default, the
table can also be sorted by name). Allow the user to connect, disconnect, and
forget networks.
In the Networking overview, if there are any Wi-Fi devices, add a "Details"
column which shows the number of available and the currently connected
networks.
We can test this with
mac80211_hwsimandhostapd, to create the four kindsof networks that we want to cover: one open, two WPA (so that we can have one
active and one inactive one), and one hidden network.
https://issues.redhat.com/browse/COCKPIT-1751
https://issues.redhat.com/browse/COCKPIT-1776
FYI @KKoukiou @rvykydal @garrett @mvollmer
Overview page now shows available and connected network for WiFi adapters:
Details page shows all networks. Currently connected one is at the top, then the known ones, then the unknown ones. Each group is sorted by descending signal strength by default, but the table can also be sorted by name. You can also typeahead-search to filter, this input appears if there are more than 3 networks:
Connection dialog for password protected network:
Connection dialog for "Connect to hidden network":
TODOs:
For historical purposes, martinpitt@c860450 was the branch with the incremental commits. It does not yet contain the above flake fix, which was just showing the filter already for
>= 3networks instead of> 3, to match our tests.Networking: Add Wi-Fi support
If there are any Wi-Fi devices, the Networking page overview shows the number of available networks and the currently connected network.
The details page show a table of available networks with mode/security, signal strength, and rate. The currently active connection (if any) is always at the top. It is followed by known networks, and then unknown networks sorted by descending signal strenght. You can connect, disconnect, and forget networks.