refactor(#495): method to discover hidden networks (opt-in)#496
refactor(#495): method to discover hidden networks (opt-in)#496justinjest wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
I appreciate your contributions- could you just read the contribution guidelines and make sure you follow things like the commit message hygiene? You should also reference the issue you are closing in your PR description (i.e. "Closes #495" so that it properly links the issue as being closed by this PR once merged).
| pub wifi_groups: Vec<WifiNetworkGroup>, | ||
| /// All Wi-Fi groups keyed by interface and SSID. | ||
| pub all_wifi_groups: Vec<WifiNetworkGroup>, | ||
| /// Saved Wi-Fi profiles keyed by SSID. |
There was a problem hiding this comment.
I don't yet know how I feel about breaking the current shape of AppletNetworkSummary...I am at work but will think on this more soon. Surely, there is a better way to approach this.
There was a problem hiding this comment.
I've been looking into this more and I've found a way to preserve the api by creating a similar structure for applet_summary as I used for wifi_groups. This would enable users to determine which they want and keep the same api bindings for both, but would require different function calls to receive hidden ap's in both cases. If there are any other ideas I'm happy to explore them as well, but if this sounds like a possible solution I can send it up for your review.
There was a problem hiding this comment.
Hmm yeah I was thinking something similar.
Happy to review these changes whenever you'd like to add them.
cachebag
added
api-surface
cachebag
changed the title
|
I'll make sure to include the issue number in the commits in the future and be clearer about the connections to the issue's in my PR's. I apologize for the inconvenience this has caused and will be more mindful in the future. |
cachebag
left a comment
cachebag
left a comment
There was a problem hiding this comment.
This is looking like exactly what I was thinking! Thanks so much for your work on this.
Looking more closely, I think hidden APs should not be represented as WifiNetworkGroups at all: without an SSID, multiple unrelated APs on the same interface would be grouped together.
I’d prefer keeping wifi_groups() and applet_summary() visible-only, adding an AccessPoint::is_hidden() helper based on ssid_bytes.is_empty(), and letting callers access hidden APs individually through snapshot.access_points.
This avoids us changing the summary shape or adding parallel *_detailed() methods while still making hidden AP discovery straightforward. The tests should also model production, where an empty raw SSID is displayed as "<Hidden Network>".
Also, re: the commit messages, please take a look at the commit history in the repo for an idea of what a commit typically looks like. I won't be a stickler about it and make you rebase or reword any existing commits but try to follow the format we've used. Here is an example
refactor(#495): a simple summary of what was changed
optionally, a body.
Thank you!!! :]
| .entry((ap.interface.clone(), ap.ssid.clone())) | ||
| .or_default() | ||
| .push(ap.clone()); | ||
| if matches!(filter, FilterMode::VisibleOnly) && ap.ssid.is_empty() { |
There was a problem hiding this comment.
Blocking: production hidden APs do not have an empty ssid. list_access_points() converts empty SSID bytes to "<Hidden Network>", so this condition never filters them and wifi_groups() behaves the same as wifi_groups_detailed().
Please detect this using ap.ssid_bytes.is_empty(), ideally behind an AccessPoint::is_hidden() helper.
| continue; | ||
| } | ||
| grouped | ||
| .entry((ap.interface.clone(), ap.ssid.clone())) |
There was a problem hiding this comment.
Blocking: hidden APs cannot safely be grouped by (interface, ssid). Every hidden BSSID on an interface has the same missing/display SSID, so unrelated networks get collapsed into one WifiNetworkGroup.
I think wifi_groups() and applet_summary() should remain visible-only, while hidden networks remain individual AccessPoints through snapshot.access_points or a small hidden_access_points() helper.
| fn hidden_networks_available() { | ||
| let snapshot = snapshot( | ||
| vec
A minor refactor to address issue #495. I believe this will make hidden ap's available, and should now hide them by default. If we wanted to also handle the parsing of the hidden ap's names I can work on that next, however I believe that is out of the scope of this issue.
I addressed some of the examples to show that all_wifi_groups are available; however I did not add them to many examples that did not use them directly.