Repository navigation
Conversation
Share saved connection identities between workspace tabs and Locations; fence superseded startup attempts; confirm and sequence single-session replacements; retain precise dialog recovery context. Persist browser-style tab ordering and use named canon colors for every menu selection state. Add bridge, rendered-menu, lifecycle and startup regression coverage plus user documentation and screenshots.
Preserve the selected-radio hierarchy from aethersdr#6885 while retaining active-path disabling and confirmed single-session switching.
ten9876
left a comment
There was a problem hiding this comment.
Review of #6889 (stacked on #6885)
What I ran: this PR's own delta (136dba0b1..d878d7e3c), built on Linux with Qt 6.12.0 both on its head and merged with current main (155850f50).
- I drove it through the automation bridge against the built-in demo radio (
DEMO-0001): offscreen, isolated settings store, TX disabled. - I added a second, unreachable saved connection (TEST-NET
192.0.2.10) so there was a second tab to work with. - Findings that belong to #6885 are in my review there.
Issue fit. There is no issue of its own. The body's claims are radio-tab Connect, Switch, Disconnect, Rename, Settings and Remove; switch confirmation; persisted drag order; explicit choices superseding auto-connect; and failed IP connects returning to their Location. Most of these held up when driven (see the end). Two did not survive: the old radio being adopted as the new tab, and Connect on "Discovered nearby".
CI. Green, but the runs were created at 06:28Z, before main moved (tip 14:09Z), and the PR is now CONFLICTING. The only conflict is the generated touchpoint manifest, which regenerates cleanly.
Scope
| File / group | Claimed in body? | Verdict |
|---|---|---|
RadioTabBar.{h,cpp}, tab menu, drag order (RadioSwitcher.tabOrder) |
Yes | In scope |
MainWindow_Session.cpp switch controller and auto-connect supersession |
Yes | In scope |
ConnectionPanel target and handoff API |
Yes | In scope |
RadioModel::prepareWanConnection → rebuildBackendForFamily("flex") (RadioModel.cpp:3895) |
No (only in window-chrome.md) |
Family-string branch (#5554). Maintainer accepts it as SmartLink-is-Flex-only, provided the body says so |
| The title-bar "+" now opens Locations instead of the discovery popover; tabs take right-click for their own menu | No | Behavior change. Maintainer accepts it, provided the body says so |
New bridge verb titlebar showRadioMenu; selectRadio also matches by serial |
No | New public surface. Maintainer accepts it, provided the body says so |
menu-reference.md, window-chrome.md, screenshots |
Partly | Fine, but menu-reference.md silently drops "…or remove the tab" (see nit 3) |
Blockers
1. Connect on a "Discovered nearby" radio silently does nothing when that radio is saved at another Location. Reproduced on the demo.
- Steps: the demo is saved under Home. Select the empty POTA Location; the demo is listed as "AetherSDR Demo · Discovered nearby", and its card has an enabled Connect button. Click it.
get radioreportedconnected: falseat 2, 4, 6, 8 and 10 s. No status message appeared; the status line still showed the unrelated earlier "No radio responded at 192.0.2.10."- Cause:
connectWorkspaceSelection()(src/gui/ConnectionPanel.cpp:4756-4760) returns on an emptyconnectionTarget().connectionTargets()(:3799) skips radios saved at another Location, butrefreshWorkspacestill lists them asnearby:<family>:<serial>entries. - #6885's version of this function found the radio in discovery and connected, so this is a regression this PR introduces.
- Fix: resolve
nearby:entries to a local target, or show and disable the entry with a "Saved at Home" hint. Don't return silently. - Note that the first nearby row selected by default in that list was a real FLEX on my LAN, so this view is the main way users reach discovered radios.
2. The old radio's automatic reconnect can be recorded as the newly chosen tab. Reasoned from the code. The demo can't reach this path: SimBackend leaves m_lastInfo.address null, so RadioModel's reconnect timer never arms. I confirmed that with sim disconnect; the demo stayed "Link lost" with no retry.
- Radio A drops unexpectedly, and
RadioModel's 5 s reconnect timer (RadioModel.cpp:2707) is armed withm_connectAttemptActive == false. - The operator picks tab B.
beginExplicitRadioChoice()(MainWindow_Session.cpp:323-345) only callsm_radioModel.disconnectFromRadio()whenisConnectAttemptInFlight()is true, so the timer stays armed. - If B parks before
connectToRadio()runs, for example in the SmartLink "attention" phase waiting for sign-in, the timer reconnects A. connectionStateChanged(true)(:748-751) then adoptsm_pendingRadioTarget(B) asm_activeRadioTargetwithout comparing serials. Tab B shows Connected while radio A is live, andrememberSuccessfulTargetsaves B's path aslastConnection.- An operator who keys up thinking they are on B is transmitting on A, which puts it in Principle VI territory.
- Fix:
- stop the pending reconnect in
beginExplicitRadioChoice()whenever the radio isn't connected, not only when an attempt is in flight; - and in the
connectedbranch, adopt the pending target only when its serial matchesm_radioModel.serial(). A routed path with no serial can be matched by its probed serial. Otherwise fall through to theelsebranch.
- stop the pending reconnect in
Nits (non-blocking)
m_radioDisconnectPendingis cleared only on aconnectionStateChanged(false)edge (MainWindow_Session.cpp:340,:545,:770).- An aborted attempt that never reached Connected may never emit that edge.
SimBackend::disconnectRadioreturns early when!m_connected, and a Flex socket still inConnectingStateis suspect too. - If it sticks, every later connect waits in
m_afterRadioDisconnectforever. - I could not make it stick on the demo: choosing the demo during the TEST-NET probe connected cleanly within 0.5 s. A timeout, or clearing it in
failRadioSwitch, would make it robust.
- An aborted attempt that never reached Connected may never emit that edge.
- Cancelling the client-slot picker leaves the switch pending.
connectSelectedRadio(~:555) returns "Connection canceled" withoutfailRadioSwitch(). The tab stays "Connecting…" and auto-connect stays suppressed. - Removed behavior in "Remove from tabs." The old remove path cleared
m_lastSessionTab, stoppedm_heartbeatMissTimerand cleared the link alarm (from #6198,5f09371e2).handleRadioTabAction's remove does none of these.m_lastSessionTabis now never assigned (MainWindow.h:2033), so it is dead state. Please carry the cleanup across, or say why it's no longer needed. - Tab reordering is mouse-only.
docs/a11y.mdsays "every interactive widget reachable by keyboard, with a non-mouse activation path … new code shouldn't add to the list." Move left / Move right items in the tab context menu would close it. LastRoutedRadioIpis now written on every connect, including SmartLink (MainWindow_Session.cpp:763-765). It readslastRadioInfo(), which a WAN connect never sets. So after a routed session followed by a SmartLink one, the WAN serial is paired with the old routed IP. Write these keys only for non-WAN sessions, as before.- One failure produces two state transitions.
setManualMessagenow emitsconnectionNeedsAttentionon errors during a target attempt (ConnectionPanel.cpp:~1824), and the startup-probe failure then also emitsconnectionAttemptFailed.- In my run, a failed TEST-NET switch left the connection panel and a separate "Connection failed" window (
radioSwitchReconnectPrevious) both open. - That window is also untracked: no
QPointer; seedocs/style/dialog-patterns.md.
- In my run, a failed TEST-NET switch left the connection panel and a separate "Connection failed" window (
- Editing the Icom user name or IP clears the typed password on every keystroke (
ConnectionPanel.cpp:4319-4328). Fixing a typo in the user name wipes the password. Clear it only when the identity actually changes, for example oneditingFinishedwith a changed value. - Browsing the radio list writes the whole workspace on every row change and rebuilds every list (
ConnectionPanel.cpp:4330). It is correct (40 rapid row changes ran without a crash), but it is a settings write per arrow key. KeepbrowsedRadiosin memory and persist it later. refreshRadioTabscost. It callsvalidateConnectionTargetfor every target. Each call enumeratesQMediaDevicesand rebuildsconnectionTargets(), which is O(n²). It runs on every discovery tick and everyTransmitModel::stateChanged, so on each MOX edge. Enumerate once per refresh.RadioSwitcher.tabOrderkeeps ids of deleted connections forever (MainWindow_Session.cpp:918-930), and thesettings.save()results there are unchecked.- A cached SmartLink token is never dropped. A failed refresh never removes the account's
m_sessionCredentialsentry (SmartLinkClient.cpp:179vs:300), so auto-login keeps re-posting a revoked token until restart. Manual sign-in does recover. Remove the entry in the refresh-failure branch. - Misleading tooltip. On the demo tab, Rename is disabled with "Save this radio to a Location before renaming it…", but the demo was already saved in Home. Only the simulator-label half of the message applies.
- Test coverage. The tab render, drag and Escape tests and the stale-probe test are real: they fail without the change. But nothing covers the MainWindow switch controller (prompt, disconnect, continue, fail), the TX refusal,
m_startupConnectSuperseded, or thetabOrderwrite and reload sort. "Persists across restarts" is backed only by a widget signal; I verified it on the demo instead (below).
What I tried to break that held
- Persisted tab order. I dragged "Testnet rig" ahead of the Simulator (
drag … -200 0).RadioSwitcherstored{"tabOrder":["…e690720d…","…67794e0a…"],"version":1}, and after killing and relaunching the process the tabs came back in the dragged order. - Switch confirmation. With the demo connected, "Switch to this radio…" on the TEST-NET tab opened
radioSwitchConfirmation: "Switch from Home · Simulator (not on the air) to Home · Testnet rig?" Cancel keptconnected: true, serial: DEMO-0001. Switch disconnected the demo. - Failed IP connect returns to its Location. The panel reopened on "Radios at Home" with tabs
Testnet rig, Connection failedandSimulator, Available. - Choosing again mid-connect. Choosing the demo while the TEST-NET probe was in flight connected the demo within 0.5 s, and the tab states were correct for 36 s.
- Tab actions. Disconnect from the tab kept the tab as "Available". Connect, Rename and Remove are disabled appropriately while connected.
- TX safety, by reading the code.
requestRadioTargetandperformRadioSwitchboth re-checkisTransmitting,isMoxandisTuning. Nothing restores TX on the new session. The confirm-dialog generation counter guards a stale accept. - Static gates. The engine-boundary and command-plane gates, compared per file between merge base and head, are flat. Every theme token used already exists on
main.
Depends on #6885.
Screenshots