add: Cellular Network support (rebase of #2765) - #3096
Conversation
Bump the network API version, drop the deleted term freeze file, regenerate the settings search index, and reuse existing translation terms where they already cover the new cellular UI.
Assigning SettingsData.networkPreference through the readonly alias threw on click, so the Cellular segment in Control Center did nothing.
The Control Center type switcher was bound only to preference, so Cellular snapped back to Ethernet. The settings modem row highlighted on hover but had no click handler. Show the modem as a connect target when no APN profiles exist.
Clicking Mobile broadband only activated an existing profile, so a modem with no APN did nothing useful. Fall back to AddAndActivate like ethernet connect.
| } | ||
| } | ||
|
|
||
| b.cellularDevices[iface] = &cellularDeviceInfo{ |
There was a problem hiding this comment.
This map access needs to be protected. Either uses the syncmap from dankgo or protect with RW mutex
There was a problem hiding this comment.
Done in e922cc9. Added cellularDevicesSnapshot() / cellularDeviceByIface() / setCellularDeviceInfo() / removeCellularDeviceByPath() guarded by the existing devMutex, mirroring the ethernet and wifi accessors already in this file — cellularDevices is now covered by the same pattern rather than a second mechanism. Every raw access is converted: Initialize, both signal-pump loops, handleDeviceAdded, handleDeviceRemoved, updateCellularState, updateAllCellularDevices and DisconnectCellularDevice. Also updated the devMutex doc comment to mention the map.
Happy to switch to syncmap instead if you would rather the new map not extend devMutex.
| // CancelCredentials provides a mock function for the type MockBackend | ||
| func (_mock *MockBackend) CancelCredentials(token string) error { | ||
| ret := _mock.Called(token) | ||
| // ActivateCellularConnection provides a mock function with given fields: uuid |
There was a problem hiding this comment.
This file shouldnt be hand-edited its generated. Re-generate mocks with mockery v3 just by running mockery e.g. go run github.com/vektra/mockery/v3@latest
This info about mockery was missing from CONTRIBUTING.md but I just added it on master
There was a problem hiding this comment.
Done in e922cc9 — regenerated with go run github.com/vektra/mockery/v3@latest (v3.7.3) instead of hand-editing. The file is now purely additive against master (395 insertions, 0 deletions); the hand-edited version had drifted from the generator output.
One note: mockery also regenerates internal/mocks/internal/plugins/mock_GitClient.go, which is stale on master and unrelated to cellular. I reverted that one to keep this PR focused — worth a separate regen commit on master.
| StyledText { | ||
| width: parent.width | ||
| visible: (NetworkService.cellularDevices?.length ?? 0) === 0 && cellularConnectionsModel.values.length === 0 | ||
| text: I18n.tr("No devices found") |
There was a problem hiding this comment.
I feel like this text could render twice when theres no modem and no saved config (line 632 also)
There was a problem hiding this comment.
Good catch, confirmed. The label at the top of cellularColumn was gated only on cellularDevices.length === 0, while this one also checks the connections model — so with no modem and no saved config both were visible. Dropped the top one in e922cc9; this one's condition was already the correct combined check.
| } | ||
|
|
||
| func (b *NetworkManagerBackend) refreshCellularState() { | ||
| b.updateAllCellularDevices() |
There was a problem hiding this comment.
Re-use this method in more places, some places are repeating this logic
There was a problem hiding this comment.
Done in e922cc9. refreshCellularState() is now used in SetCellularEnabled, DisconnectCellular, DisconnectCellularDevice, ActivateCellularConnection and the device-removal path in backend_networkmanager_signals.go.
I left the remaining sites in signals.go alone on purpose — those interleave ethernet/wifi/hotspot updates and fire a single onStateChange at the end, so folding the helper in would emit an extra mid-sequence broadcast.
| if _, err := nm.AddAndActivateConnection(settings, dev); err != nil { | ||
| delete(settings["gsm"], "auto-config") | ||
| if _, retryErr := nm.AddAndActivateConnection(settings, dev); retryErr != nil { | ||
| return fmt.Errorf("failed to create and activate cellular connection: %w", err) |
There was a problem hiding this comment.
This should wrap retryErr, seems like it'd be more indicative of the real error
There was a problem hiding this comment.
Agreed — fixed in e922cc9, it now wraps retryErr. Also added a short comment noting the retry exists because older NetworkManager/ModemManager stacks reject gsm.auto-config.
| width: enableCellularLabel.implicitWidth + Theme.spacingL * 2 | ||
| height: enableCellularLabel.implicitHeight + Theme.spacingM * 2 | ||
| radius: height / 2 | ||
| color: enableCellularButton.containsMouse ? Qt.rgba(Theme.primary.r, Theme.primary.g, Theme.primary.b, 0.12) : Qt.rgba(Theme.primary.r, Theme.primary.g, Theme.primary.b, 0.08) |
There was a problem hiding this comment.
Instead of hand rolling Qt.rgba re-use the appropriate tokens like wifiOffContent does Theme.surfaceTextSecondary/primaryHover/primaryHoverLight
There was a problem hiding this comment.
Done in e922cc9 — the icon now uses Theme.surfaceTextSecondary and the button uses Theme.primaryHover / Theme.primaryHoverLight, matching wifiOffContent.
- Guard cellularDevices map with devMutex via snapshot/by-iface/set/remove helpers, mirroring the existing ethernet and wifi accessors - Regenerate mocks with mockery v3 instead of hand-editing mock_Backend.go - Reuse refreshCellularState() in SetCellularEnabled, DisconnectCellular, DisconnectCellularDevice, ActivateCellularConnection and device removal - Wrap retryErr rather than the first error when the auto-config retry fails - Use Theme.surfaceTextSecondary/primaryHover/primaryHoverLight in the cellular-off state instead of hand-rolled Qt.rgba values - Drop the duplicate "No devices found" label so it can no longer render twice with no modem and no saved config Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Still testing |
Hardware testing doneRe-tested on the Fibocom L831-EAU-00 (Vodacom SA, Build / unit
Cellular against the real modemDriven over the backend socket (API v31, NetworkManager backend), starting from a deleted GSM profile so the create-from-scratch path was exercised:
All four UIControl Center cellular panel renders correctly in both states:
Two things I could not prove on this hardware
|
| } | ||
| } | ||
|
|
||
| function enableCellularRadio() { |
There was a problem hiding this comment.
Not used so I'd just delete it, in addition to the function in DMSNetworkService
There was a problem hiding this comment.
Done in 29bc88d — removed enableCellularRadio() from both NetworkService.qml and DMSNetworkService.qml. The backend network.cellular.enable IPC handler is still there.
| types.push("cellular"); | ||
| return types.length > 0 ? types : ["wifi"]; | ||
| } | ||
| property int selectedTypeIndex: -1 |
There was a problem hiding this comment.
Think it would be more robust to store the selected value instead of index
property string selectedType: ""
readonly property string currentConnectionType: {
if (selectedType && connectionTypes.includes(selectedType))
return selectedType;
return connectionTypes[Math.max(0, currentPreferenceIndex)] || "wifi";
}There was a problem hiding this comment.
Done in 29bc88d — selectedType is now a string, and currentConnectionType uses it when it is still in connectionTypes, otherwise it falls back to preference. If a NIC disappears while the panel is open the pane no longer remaps to a different medium via a stale index.
| if (!selected) | ||
| return; | ||
| NetworkService.setNetworkPreference(index === 0 ? "ethernet" : "wifi"); | ||
| selectedTypeIndex = index; |
There was a problem hiding this comment.
And here instead of index:
currentIndex: Math.max(0, connectionTypes.indexOf(currentConnectionType))
onSelectionChanged: (index, selected) => {
if (!selected)
return;
selectedType = connectionTypes[index] || "wifi";
NetworkService.setNetworkPreference(selectedType);
}There was a problem hiding this comment.
Done in 29bc88d — currentIndex is connectionTypes.indexOf(currentConnectionType), and onSelectionChanged writes selectedType from the chosen value.
- Toast "Failed to toggle cellular" instead of always "enable" - Remove unused enableCellularRadio() wrappers - Store Control Center network type as a value, not a list index
|
/claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| connRef := connName | ||
| if connUUID != "" { | ||
| connRef = "uuid:" + connUUID | ||
| } | ||
|
|
||
| if err := exec.Command("nmcli", "con", "mod", connRef, | ||
| "connection.autoconnect-priority", fmt.Sprintf("%d", autoconnectPriority), | ||
| "ipv4.route-metric", fmt.Sprintf("%d", routeMetric), | ||
| "ipv6.route-metric", fmt.Sprintf("%d", routeMetric)).Run(); err != nil { |
There was a problem hiding this comment.
uuid:<uuid> is not valid nmcli syntax. nmcli takes the selector as a separate argument (nmcli con mod uuid <UUID> ...); a bare string is matched against id/uuid/path, but uuid:abcd-... matches nothing, so every invocation fails with Error: unknown connection.
Since practically every NM profile has a UUID, connRef is almost always the broken form — this silently disables priority/route-metric updates for ethernet and WiFi too, not just cellular. (Compare backend_networkmanager_vpn.go:1308, which passes the bare UUID.)
| connRef := connName | |
| if connUUID != "" { | |
| connRef = "uuid:" + connUUID | |
| } | |
| if err := exec.Command("nmcli", "con", "mod", connRef, | |
| "connection.autoconnect-priority", fmt.Sprintf("%d", autoconnectPriority), | |
| "ipv4.route-metric", fmt.Sprintf("%d", routeMetric), | |
| "ipv6.route-metric", fmt.Sprintf("%d", routeMetric)).Run(); err != nil { | |
| args := []string{"con", "mod"} | |
| if connUUID != "" { | |
| args = append(args, "uuid", connUUID) | |
| } else { | |
| args = append(args, connName) | |
| } | |
| args = append(args, | |
| "connection.autoconnect-priority", fmt.Sprintf("%d", autoconnectPriority), | |
| "ipv4.route-metric", fmt.Sprintf("%d", routeMetric), | |
| "ipv6.route-metric", fmt.Sprintf("%d", routeMetric)) | |
| if err := exec.Command("nmcli", args...).Run(); err != nil { |
There was a problem hiding this comment.
Good catch, and it's worse than a cellular bug — confirmed and fixed in 3efc98c.
Verified empirically on my test box using con show (same selector parsing as con mod, but read-only):
$ nmcli con show uuid:9f010098-7fb0-341d-8859-034f060aeb39
Error: uuid:9f010098-7fb0-341d-8859-034f060aeb39 - no such connection profile.
$ nmcli con show uuid 9f010098-7fb0-341d-8859-034f060aeb39
OK
$ nmcli con show "Wired connection 1"
OK
And you're right that this was a regression I introduced, not just a cellular gap: master passed connName here, and since practically every profile has a UUID, connRef was almost always the broken form — so autoconnect-priority and route-metric updates were silently no-ops for ethernet and WiFi too. The failure only reached log.Warnf and the caller ignores the error, which is why nothing surfaced.
Applied your suggestion as-is (arg-list construction, uuid <UUID> when present, bare name otherwise). gofmt clean, go build ./... and go test ./internal/server/... pass.
| if (NetworkService.cellularToggling) | ||
| return I18n.tr("Connecting..."); |
There was a problem hiding this comment.
cellularToggling is only set by toggleCellularRadio(), so this state is "enabling/disabling the radio", not connecting — the label is wrong. NetworkDetail.qml already uses the right wording for the same flag.
| if (NetworkService.cellularToggling) | |
| return I18n.tr("Connecting..."); | |
| if (NetworkService.cellularToggling) | |
| return NetworkService.cellularEnabled ? I18n.tr("Disabling cellular...") : I18n.tr("Enabling cellular..."); |
There was a problem hiding this comment.
Fixed in 3efc98c — applied your suggestion.
While doing it I found the terms you pointed at weren't actually in the catalog: en.json/template.json were never regenerated for this PR, so 7 of the 23 new I18n.tr() terms were missing, including both Enabling cellular... and Disabling cellular... (already used at NetworkDetail.qml:542). Added them.
I did not just run extract_translations.py — it rewrites the catalog wholesale and came out at −2599/+970, dropping 107 existing terms while adding 8. Given i18nsync.py warns that --prune deletes every POEditor term missing from the local en.json along with its translations, that looked destructive and well out of scope here. So I spliced in only the 7 missing entries, generating them through the script's own create_poeditor_json/create_template_json so the context/reference/comment/tags fields match its output exactly. Diff is additive only: en.json +66, template.json +49, both still sorted.
Worth flagging separately: the committed catalog and a fresh extraction disagree by ~100 terms on master too, so something is stale there independent of this PR.
Claude reviewTwo findings; the nmcli connection reference is a regression that affects existing WiFi/Ethernet preference handling, not just cellular.
Checked: Go backend/manager/handlers/priority, NM signal + device add/remove paths and devMutex accessors, backend stubs and regenerated mocks, QML services and Control Center/Settings cellular UI, tab index 46 collisions, search index, and that en.json/template.json were untouched. Model: claude-opus-5. |
`nmcli con mod uuid:<UUID>` is not valid syntax -- nmcli expects the selector keyword as its own argument, so every priority update failed with "no such connection profile". Because practically every NM profile has a UUID, this silently disabled autoconnect-priority and route-metric updates for ethernet and WiFi as well as cellular. Also fix the Cellular settings status text, which said "Connecting..." for a flag that only tracks radio enable/disable, and add the seven cellular terms that were missing from the translation catalogs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@bbedward anything else needed for this? |
3 participants
Description
Rebased and review-fixed version of #2765 (cellular network support).
Original work by @yechielw. Rebased onto current
masterand tested on a machine with a Fibocom L831-EAU-00 (Vodacom LTE).Review from @bbedward on #2765:
master(term_freeze.jsonis gone)APIVersion30 → 31settings_search_index.jsonviaextract_settings_index.pywpa_supplicantcellular stubs so theBackendinterface still compilesFixes found while testing:
userPreferencealias (Cellular chip was a no-op)AddAndActivateConnectionlike Ethernet connectType of change
Related issues
Continues / supersedes #2765
Checklist
I18n.tr(), reusing existing terms where possiblego test ./internal/server/...passed