Skip to content

fix(gui): reopen the Aetherial strip after minimize — Principle XI. - #5366

Open
crypticpy wants to merge 2 commits into
aethersdr:mainfrom
crypticpy:pr/5365-aetherial-strip-minimize-lockout
Open

fix(gui): reopen the Aetherial strip after minimize — Principle XI.#5366
crypticpy wants to merge 2 commits into
aethersdr:mainfrom
crypticpy:pr/5365-aetherial-strip-minimize-lockout

Conversation

@crypticpy

Copy link
Copy Markdown

Summary

Fixes #5365.

Minimizing the Aetherial Audio Channel Strip made it unrecoverable from its own
button: no number of presses on the AetherVoice button (or the chain applet's
nub) would bring the window back, leaving the Dock/taskbar as the only way to
reach it.

Two Qt behaviours combine to cause it, and toggleAetherialStrip() was written
as if neither existed:

  1. QWidget::isVisible() stays true while a window is minimized, so
    if (m_aetherialStrip->isVisible()) sent a minimized strip down the hide()
    branch.
  2. QWidget::show() on a minimized window restores its saved state — still
    minimized — so the follow-up press did not recover it either.

The decision is extracted into gui/WindowShowState.{h,cpp}:

  • windowIsShowing(w) — visible and not minimized.
  • showAndRaiseWindow(w)showNormal() when minimized, show() otherwise,
    then raise + activate.

showNormal() stays guarded on isMinimized() because calling it
unconditionally would clear a Maximized or FullScreen window — the same reason
the net-reminder and tray raise paths in MainWindow_Nets.cpp guard it (#3918).
That helper is where those two sites should eventually converge; they are
deliberately not touched here, to keep this change scoped to the reported
bug.

The docked CWX and DVK panels use the same bare isVisible() shape but are
child widgets that cannot be minimized independently, so they are unaffected and
are left alone.

Constitution principle honored

Principle XI — Fixes Are Demonstrated. New regression test
window_show_state_test drives a real QWidget through
hidden → shown → minimized → restored and runs the toggle exactly as
MainWindow does. Mutation-checked three ways, each failing a different
assertion:

Principle VIII — Evidence Over Assertion. Both Qt behaviours above are
pinned against a real QWidget on the offscreen platform, not asserted from
memory.

Test plan

  • Local build passes (cmake --build build) — clean, exit 0
  • Behavior verified on a real radio if applicable — N/A: pure window
    management in MainWindow, independent of the connected backend
  • Existing tests pass (CI) — window_show_state_test passes; full local
    suite run on this build tree shows only the two failures that reproduce
    unchanged on clean main (bridge_docs_check, hl2_state_restore_test),
    neither of which touches GUI window state
  • Reproduction steps documented if user-reported bug — in Aetherial Audio Channel Strip cannot be reopened after minimizing — its own button no longer restores it #5365

Checklist

  • Commits are signed (docs/COMMIT-SIGNING.md) — GPG, GitHub reports
    verified: true
  • No new flat-key AppSettings calls — this change adds no settings
  • Code is clean-room — AetherSDR's own widget code against public Qt API
    (Principle IV)
  • All meter UI uses MeterSmootherN/A, no meter UI is touched
  • Documentation updated if user-visible behavior changed — no doc change
    needed; no document describes the old toggle behaviour. CHANGELOG.md
    deliberately untouched
  • Security-sensitive changes reference a GHSA if applicable — N/A

Note on the claim protocol (AGENTS.md §Issue / PR Claim Protocol): assignee
changes are rejected for an account without write access to this repo, so the
Fixes #5365 link is the visible claim on the issue timeline instead.

Minimizing the Aetherial Audio Channel Strip made it unrecoverable from
its own button: no number of presses on the AetherVoice button (or the
chain applet's nub) would bring the window back, leaving the taskbar/Dock
as the only way to reach it.

Two Qt behaviours combine to cause it, and toggleAetherialStrip() was
written as if neither existed:

  1. QWidget::isVisible() stays TRUE while a window is minimized, so
     `if (m_aetherialStrip->isVisible())` sent a minimized strip down the
     hide() branch.
  2. QWidget::show() on a minimized window restores its SAVED state —
     still minimized — so the follow-up press did not recover it either.

Both are now pinned against a real QWidget rather than asserted from
memory, on the offscreen platform where they reproduce.

Extract the decision into gui/WindowShowState.{h,cpp}:

  * windowIsShowing(w)     — visible AND not minimized.
  * showAndRaiseWindow(w)  — showNormal() when minimized, show() otherwise,
                             then raise + activate.

showNormal() stays guarded on isMinimized() because calling it
unconditionally would clear a Maximized or FullScreen window — the same
reason the net-reminder and tray raise paths in MainWindow_Nets.cpp guard
it (aethersdr#3918). The helper is where those two sites should eventually
converge; not touched here to keep this change to the reported bug.

The docked CWX and DVK panels use the same bare isVisible() shape but are
child widgets that cannot be minimized independently, so they are
unaffected and are left alone.

New regression test window_show_state_test drives a real QWidget through
hidden → shown → minimized → restored and runs the toggle exactly as
MainWindow does. Mutation-checked three ways, each failing a different
assertion:

  * windowIsShowing() without the !isMinimized() term      → 3 failures
  * showAndRaiseWindow() with an unconditional show()      → 2 failures
  * showAndRaiseWindow() with an unconditional showNormal()→ 1 failure
    (the aethersdr#3918 maximized guard)

Found in local field use on macOS 26.5.2 (25F84) / Qt 6.11.1; filed as aethersdr#5365.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015HjpgedubkqsxhbYrsTzMR
@crypticpy
crypticpy requested review from a team as code owners September 1, 2026 04:04

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue fit

#5365: the Aetherial strip button treated a minimized strip window as "already showing", so pressing it did nothing — the operator had to un-minimize by hand. The fix makes windowIsShowing() require !isMinimized() and adds showAndRaiseWindow() that un-minimizes before raising. Correct and directly on-target, extracted into a testable free function with a mutation-checked regression test (window_show_state_test) that pins both Qt behaviors and the #3918 maximize guard. Principle XI honored.

Scope

Clean — the helper, its one call site, the test. CHANGELOG.md correctly untouched. Preflight: no sockets, pure widget-state test.

Blockers

None.

Nits (non-blocking)

  1. Minimize-from-maximized reopens un-maximized (inline). showNormal() clears both the Minimized and Maximized bits, so maximize the strip → minimize → reopen brings it back at normal size, not maximized — a corner of the exact reopen path this PR fixes, and one the test misses (case 7 only checks maximize is preserved when the window was never minimized). w->setWindowState(w->windowState() & ~Qt::WindowMinimized) clears only the minimized bit and preserves maximize.
  2. The isMinimized()/showNormal()/raise pattern now lives in three places (MainWindow_Nets.cpp:226-247 has two copies for #3918). The body acknowledges convergence "eventually"; this helper takes a QWidget* and could absorb those net-reminder/tray paths now, so a future fix (like nit 1) lands once.

What was verified vs read

  • Verified by me: showNormal() clears the maximized bit (nit 1's mechanism, uncovered by the test); the helper resolves unqualified in MainWindow.cpp's namespace.
  • From the automated pass, verified: first-press-creates-then-shows still works; the mutation test genuinely pins the two Qt behaviors.
  • Not run: no bridge session (the widget-state helper is unit-covered), tests read not executed — no CI yet, which is a merge gate.

Comment thread src/gui/WindowShowState.cpp Outdated
return;
// showNormal() only for a minimized window — see the header note on #3918.
if (w->isMinimized())
w->showNormal();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (non-blocking) — showNormal() drops a prior maximize. It clears both the Minimized and Maximized bits, so minimize a maximized strip and reopen → it returns un-maximized. Clear only the minimized bit to preserve the pre-minimize size:

Suggested change
w->showNormal();
if (w->isMinimized())
w->setWindowState(w->windowState() & ~Qt::WindowMinimized);
else
w->show();

Add a test row (maximize → minimize → reopen → still maximized); case 7 doesn't cover the minimized-from-maximized path.

… — Principle XI.

Review of aethersdr#5366 pointed out that showNormal() clears the Maximized and
FullScreen bits along with Minimized, so a strip that was maximized, then
minimized, then reopened from its button came back at normal size. That
is a corner of the exact reopen path the PR fixes. showAndRaiseWindow()
now clears only Qt::WindowMinimized via setWindowState(), which is a
pending state on a hidden widget and immediate on a visible one, so the
isMinimized() branch goes away too.

The same isMinimized()/showNormal()/raise sequence lived twice more in
MainWindow_Nets.cpp (the net-reminder tune and the tray-message click,
both aethersdr#3918). They carried the same latent un-maximize and now call the
helper, so the fix lands once. Braces restored on the strip toggle to
match the surrounding style.

Test: window_show_state_test gains case 8 — maximize, minimize, toggle —
and asserts the window is showing AND still maximized. Mutation-checked:
the old showNormal() body fails only that final assertion.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FhDXan9Qe86EbypHmxzHUy
@crypticpy

crypticpy commented Sep 2, 2026

Copy link
Copy Markdown
Author

@ten9876 Both nits taken, pushed as 5a48797.

Nit 1 — minimize-from-maximized. showAndRaiseWindow() now clears only Qt::WindowMinimized through setWindowState(), exactly as you suggested. That also removes the isMinimized() branch: on a hidden widget the cleared state is pending and show() applies it, on a visible one it takes effect at once and show() is a no-op. Case 8 in window_show_state_test pins it (maximize → minimize → toggle → still maximized). Mutation-checked against the old showNormal() body: only the final "restored window is still maximized" assertion fails, so the row is measuring what it claims.

Nit 2 — the two copies in MainWindow_Nets.cpp. Both (net-reminder tune, tray-message click) now call showAndRaiseWindow(this). They had the identical latent un-maximize, so the nit-1 fix reaches them in the same commit. Disclosing here since it widens the diff by one file beyond #5365's strip: no behaviour change other than preserving maximize on those two raise paths.

One style-only change of my own: the strip toggle in MainWindow.cpp gets its braces back to match the surrounding code.

@Ozy311 Ozy311 self-assigned this Sep 5, 2026

@Ozy311 Ozy311 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue fit

The fix resolves #5365. Independently reproduced the lockout in the actual pre-fix application and verified one-click recovery in the PR application on macOS 26.5.2 (25F84), arm64, Qt 6.11.1. The latest commit also addresses both points from the earlier maintainer review: restoring a minimized window preserves maximized/fullscreen state, and the two net-reminder raise paths share the helper.

Scope

Files Change and provenance Verdict
MainWindow.cpp, WindowShowState.{h,cpp} Correct the strip toggle and restore only the minimized state bit; described by #5365 and the review follow-up In scope
MainWindow_Nets.cpp Reuse the helper at both raise sites, explicitly requested in the earlier maintainer review and disclosed in the author's follow-up In scope
CMakeLists.txt, tests/tests.cmake, window_show_state_test.cpp Compile the helper and register socket-free Qt widget regression coverage In scope

No unrelated changes, new settings, protocol changes, dependencies, or changelog churn. The PR summary still describes the earlier showNormal() implementation and says the Nets sites are untouched; the follow-up comment accurately describes the final code.

Blockers

None found.

Nits — non-blocking

  1. Refresh the PR summary and test comments to describe clearing WindowMinimized and the two Nets callers; references to an isMinimized() guard are stale.
  2. Add braces to the new helper's null guard and the test's toggle branches to match AGENTS.md's control-flow convention (inline suggestion).

Independent verification

Built both the unchanged merge base 7f3eb98cd2df426dc4381e2a2203a1d59f600d1a and exact PR head 5a48797deeeef0057e761a68c652111215cd5ac7 as Mac applications. Both used RelWithDebInfo, Qt 6.11.1, and ENABLE_ASR=OFF; optional ASR is unrelated to this window path. The sandbox's icon-generation step failed identically on both; rerunning the builds outside the sandbox succeeded without source changes.

The full application checks used separate settings stores, explicit per-instance automation sockets, offscreen Qt, and only DEMO-0001 (AetherSDR Demo), with transmission pinned off. The actual AetherVoice launcher was invoked through VfoWidget/Aetherial Audio Channel Strip; restoration was not forced through the bridge's restore verb.

Recorded strip state:

Sequence Pre-fix application PR application
Open, minimize, press AetherVoice once visible:false, windowState:"minimized" visible:true, windowState:"normal"
Press AetherVoice again on pre-fix build visible:true, windowState:"minimized" — still inaccessible Normal hide/reopen behavior verified separately
Maximize, minimize, press AetherVoice visible:true, windowState:"maximized"
Fullscreen, minimize, press AetherVoice visible:true, windowState:"fullscreen"

The PR application also passed ordinary hide/show toggles, hide/show while maximized, and close/reopen while maximized. Both owned demo instances exited normally after validation; final demo model state reported transmitting:false and txPower:0.

The registered window_show_state_test passed through CTest. Its 15 assertions also passed independently on native Cocoa with no skips. Three deliberate mutations were caught: removing the minimized predicate (4 failures), omitting the minimized-state clear (3 failures), and using unconditional showNormal() (3 failures). An additional socket-free driver passed 11 assertions covering hidden/minimized restoration, fullscreen preservation, repeated geometry-preserving toggles, and a parent-owned top-level window matching the strip's ownership.

Inspected callers, ownership, surrounding code, and intervening main changes; no stale-base overwrite or overlapping fix found. Local test-registration and engine-boundary checks passed (existing tracked boundary warnings only). No separately invocable automated code-review skill was available; a second code audit was performed and its conclusions checked against the patch and runtime evidence.

Review snapshot and limits

Trusted governance snapshot: 50d70ebe9fdc4d6f01171b5cb17479bc89537be2, manifest PASS; canonical constitution and root mirror match. Both PR commits have valid GitHub-verified signatures. Approval remains subject to the repository's human CODEOWNERS requirements: infrastructure for CMakeLists.txt, reviewers for source/tests.

This review demonstrates the actual strip path on Mac plus native Cocoa helper behavior. Native Dock stacking, actual OS tray notifications, and physical-radio operation were not exercised. The full local suite was not run; the author's historical full-suite failure claims were not independently repeated.

Verdict

Approve with the non-blocking notes above. CI is green for Linux, macOS, and Windows, and Static Checks is green. Inspected the completed job logs; the platform test steps ran and passed. The head and existing discussion were rechecked immediately before submission. No merge action is included in this review.

Comment on lines +14 to +15
if (!w)
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking convention nit: AGENTS.md requires braces on all control flow. Please brace this guard (and the toggle branches in the new test).

Suggested change
if (!w)
return;
if (!w) {
return;
}

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Issue fit

Yes — this solves #5365. The issue names two Qt behaviours (isVisible() stays true while minimized; show() on a minimized window restores it to minimized) and the diff addresses both: windowIsShowing() fixes the branch selection at MainWindow.cpp:9387, showAndRaiseWindow() fixes the restore. I confirmed the premise holds for this widget specifically: the strip is promoted to a real top-level at MainWindow.cpp:9267 (setWindowFlag(Qt::Window, true)) — it is the only widget in src/gui that gets that promotion — so it is the one that can carry an independent minimize state, and the issue's claim that the docked CWX/DVK panels are unaffected checks out (they are child widgets).

I also walked the second entry point the issue mentions ("the chain applet's nub"): both MainWindow_Wiring.cpp:5861 and MainWindow_DspApplets.cpp:459 connect to the same toggleAetherialStrip() slot, and the session-restore path at MainWindow.cpp:2540 calls it too. All three go through the fixed branch — no second copy left behind. The restore path still works because a freshly-constructed strip is hidden, so windowIsShowing() is false and it takes the show branch.

2. Scope

File / group What it changes Claimed by the issue? Verdict
src/gui/WindowShowState.{h,cpp} New helper: the two predicates the fix needs Yes In scope
src/gui/MainWindow.cpp The toggle at 9387 + one include Yes — this is the reported bug In scope
CMakeLists.txt Adds WindowShowState.cpp to GUI_SOURCES Yes — required, not dead In scope
tests/window_show_state_test.cpp, tests/tests.cmake New registered CTest target Yes — Principle XI In scope
src/gui/MainWindow_Nets.cpp Replaces the two if (isMinimized()) showNormal(); raise(); activateWindow(); blocks with the helper No#5365 is strictly about the strip See below

The MainWindow_Nets.cpp hunk is the one thing the linked issue does not explain, and notably commit 1's own message explicitly deferred it: "The helper is where those two sites should eventually converge; not touched here to keep this change to the reported bug." Commit 2 then converged them anyway.

It is not a no-op refactor — it is a real behaviour change to the main window. The old code called showNormal(), which clears Minimized and Maximized and FullScreen; so a main window that was maximized, then minimized, came back un-maximized when a net reminder or tray click raised it. The new helper clears only the Minimized bit, so it now comes back maximized. That is a genuine improvement and it is arguably what the #3918 comment always intended, but it is an undisclosed user-visible change to a surface #5365 never mentions. Not a blocker in my read — it is small, correct, and consistent with the comment it sits under — but it should be stated in the PR body rather than arriving as a silent side effect, and it is the maintainer's call whether to keep it here or split it. (Secondary difference I checked: the helper also calls show() unconditionally where the old code did not. On an already-visible widget show() is a no-op, so this only matters if the main window were hidden — in which case revealing it is what the net reminder wants anyway.)

3. Blockers

None.

4. Nits (all non-blocking)

  • WindowShowState.cpp:14 — braces. @Ozy311 is right and it is canon, not taste: AGENTS.md:182"Braces on all control flow — even single-line if/else/for/while". Applies to the null guard and to toggle() in the new test. Suggestion inline.
  • The #3918 guard — the entire point of commit 2 — can silently go untested (tests/window_show_state_test.cpp:101). Cases 7 and 8 sit behind if (w.isMaximized()), and the else branch prints [SKIP] and exits 0. If any CI platform's offscreen QPA declines to maximize, the test still passes while proving nothing about maximize preservation. The skip comment's justification is also not accurate: it says "the guard is still pinned by case 6", but case 6 (minimize → restore) involves no maximized state at all — nothing else in the file covers it. Detail inline.
  • FullScreen preservation is claimed but never tested. WindowShowState.h says "a Maximized or FullScreen window keeps that state"; only the Maximized half has a case, and conditionally at that.
  • raise() / activateWindow() are untested. Removing both from the helper would leave every assertion green. Genuinely hard to test on offscreen — worth naming, not worth fixing.
  • Green CI does not mean this test ran. window_show_state_test matches none of the ctest -R filters in .github/workflows/ci.yml, so the four passing checks on 5a48797 prove it compiles, not that it passes. It is a widget-free-of-network, offscreen, fast test — cheap to fold into an existing filtered list if you want the Principle XI evidence actually enforced on merge.
  • @ten9876's showNormal() nit was already addressed by commit 2 — and correctly not applied verbatim: the suggested else w->show(); form skips show() on the minimized path, which would leave a strip that was hidden-while-minimized never actually shown. The version that landed is better than the suggestion.

5. What I tried to break

  • Sibling call sites. Grepped /tmp/aetherclaude/pr-5366/src/gui for the same toggle shape. CopyAssistController.cpp:224 is a literal shape-match (isVisible() ? hide() : show()+raise()+activateWindow()), and ClientEqEditor.cpp:454 has the related if (!isVisible()) show(); form that also cannot un-minimize. Both are plain QDialogs with no Qt::Window promotion and no minimize button under default dialog flags, so I could not construct a way for a user to minimize them — I do not consider these unfixed siblings, but flagging them so the next person doesn't rediscover them.
  • Would the test pass against the unfixed code? No, and I checked each mutation independently rather than trusting the commit message. Dropping !w->isMinimized() from windowIsShowing() breaks cases 4 and 6; replacing the helper body with a bare show() breaks case 6's two assertions; substituting showNormal() breaks case 8. The three mutations fail three different assertions, so no single assertion is carrying the file.
  • Qt semantics the test asserts. Rather than take them on faith I traced them: showMinimized() ORs in the Minimized bit (keeps Maximized — case 8's premise), showMaximized() clears Minimized/FullScreen and sets Maximized, showNormal() clears all three. The state & ~Qt::WindowMinimized idiom is well-formed C++ here (Q_DECLARE_OPERATORS_FOR_FLAGS supplies operator~ on the enum) and is the form Qt's own docs use.
  • Lifecycle and null paths. showAndRaiseWindow(nullptr) is guarded and case 1 covers it; the lazy-creation path at MainWindow.cpp:9263 cannot reach the toggle with a null m_aetherialStrip; a hidden-and-never-shown widget hits setWindowState(NoState), a no-op, before show(). Double-press, minimize-then-hide, and hide-while-minimized orderings all resolve sensibly — the button can no longer reach a state where it does nothing.
  • Dead additions / registration. WindowShowState.cpp is in GUI_SOURCES; the test target is a real add_executable + add_test with QT_QPA_PLATFORM=offscreen, not an unregistered stub.
  • CodeGuard. All 18 CG-PATH-001 hits are refuted for this PR: none land on a changed line. The four in MainWindow.cpp (8806, 8918, 9037–9039) are in untouched regions — this diff's only hunks there are at ~140 and ~9379 — and the rest are in files this PR does not touch at all.
  • Socket-test disclosure (§2). I read the added test source, not the working tree. tests/window_show_state_test.cpp includes only QApplication, QWidget and <cstdio>; no QTcpServer/QUdpSocket/QLocalServer, no bind/listen/connectToHost, no peer process, no Fake* device class. No socket test added, modified or removed by this PR.

Everything above is reasoned from the diff and the head checkout at /tmp/aetherclaude/pr-5366. I am headless: I did not build, run window_show_state_test, or drive the GUI, so I cannot confirm empirically that the strip restores on macOS 26.5.2 — the platform-specific half of the claim (that clearing the Minimized bit deminiaturizes a Cocoa window) is the one thing I can only reason about, not observe.

6. Recommendation

Approve with nits, with one item for a maintainer to rule on. The fix is correctly scoped to the real defect, the helper's semantics are better than either the original code or the suggestion it received in review, and the test is honestly mutation-checked rather than asserting the implementation back to itself. The one judgment call is the MainWindow_Nets.cpp hunk — a correct but undisclosed main-window behaviour change that commit 1 had deliberately deferred; keep it and document it in the body, or split it, your call.

Concrete next steps for @crypticpy: brace the two guards per AGENTS.md:182, make the case-7/8 skip loud (or fail) so the #3918 guard can't quietly go unenforced, and add a line to the PR body disclosing the net-reminder/tray behaviour change. Nice work on pinning the two Qt behaviours against a real QWidget instead of asserting them — that is what made this reviewable.


🤖 aethersdr-agent · cost: $3.8653 · model: claude-opus-5

Comment on lines +14 to +15
if (!w)
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming @Ozy311's nit as canon rather than taste — AGENTS.md:182: "Braces on all control flow — even single-line if/else/for/while". Same applies to toggle() in the new test.

Also worth recording why the version that landed is right: @ten9876's suggested if (isMinimized()) setWindowState(...); else show(); would skip show() on the minimized path, so a strip that was hidden while minimized would have its Minimized bit cleared and then never actually be shown. Clearing the bit unconditionally and always calling show() — what you did — handles both entry states in one path. Good call not applying that suggestion verbatim.

Suggested change
if (!w)
return;
if (!w) {
return;
}

Comment on lines +119 to +122
} else {
// Some platforms decline to maximize; the guard is still pinned by
// case 6, so skip rather than fail on a platform quirk.
std::printf("[SKIP] maximized state unavailable on this platform\n");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #3918 guard — the whole point of commit 2 — can silently go untested here.

Cases 7 and 8 are the only coverage of maximize preservation anywhere in the file, and they sit behind if (w.isMaximized()). If any CI platform's offscreen QPA declines to maximize, this branch prints [SKIP], g_failures stays 0, and the test exits green having proven nothing about the behaviour commit 2 exists to add.

The justification in the comment doesn't hold: case 6 is minimize → restore on a non-maximized window — it exercises ~Qt::WindowMinimized but never distinguishes it from showNormal(), which is exactly the distinction cases 7–8 are for. Substituting showNormal() back into the helper fails case 8 and nothing else, so on a platform that skips, that mutation goes undetected.

Since the skip is a real platform-capability question you can't resolve from here, the cheap fix is to make it loud rather than silent — count it, or fail if it skips on the platforms you know do support it:

Suggested change
} else {
// Some platforms decline to maximize; the guard is still pinned by
// case 6, so skip rather than fail on a platform quirk.
std::printf("[SKIP] maximized state unavailable on this platform\n");
} else {
// Maximize is unavailable on this platform, so the #3918 guard is NOT
// covered by this run — case 6 exercises ~WindowMinimized but cannot
// tell it apart from showNormal(). Fail rather than pass silently:
// every platform CI runs this on is expected to support maximize.
report("maximized state available (needed for the #3918 guard)", false);
}

Separately: WindowShowState.h claims the helper preserves "Maximized or FullScreen" — only the Maximized half has a case. A showFullScreen() sibling of case 8 would close that gap.

activateWindow();
// Bring the window forward without disturbing its
// Maximized/FullScreen state (#3918).
showAndRaiseWindow(this);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope note, not a blocker — see the review body's scope table.

This file isn't explained by #5365, which is strictly about the Aetherial strip, and commit 1's message explicitly set it aside: "The helper is where those two sites should eventually converge; not touched here to keep this change to the reported bug." Commit 2 converged them.

It also isn't behaviour-neutral. The old showNormal() cleared Minimized and Maximized and FullScreen, so a maximized-then-minimized main window came back un-maximized on a net-reminder raise; with the helper it now stays maximized. I think that's what the #3918 comment always meant to do and the change reads as correct — but it's a user-visible change to a surface the linked issue never mentions, so it should be stated in the PR body rather than inferred from the diff. Maintainer's call whether it rides along or splits out.

(The other difference is that the helper calls show() unconditionally where the old code did not. On a visible widget that's a no-op, so it only matters if the main window were hidden — where revealing it is the intent anyway. No concern from me.)

@aethersdr-agent

Copy link
Copy Markdown
Contributor

Cisco CodeGuard — static analysis of this PR (18 finding(s))

  • [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5366/.github/workflows/appimage.yml:200
  • [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/appimage.yml /tmp/aetherclaude/pr-5366/.github/workflows/appimage.yml:399
  • [MEDIUM] CG-PATH-001 — Potential path traversal in .github/workflows/macos-dmg.yml /tmp/aetherclaude/pr-5366/.github/workflows/macos-dmg.yml:107
  • [MEDIUM] CG-PATH-001 — Potential path traversal in plugins/ulanzi-aethersdr/com.g0jkn.aethersdr.ulanziPlugin/plugin/app.js /tmp/aetherclaude/pr-5366/plugins/ulanzi-aethersdr/com.g0jkn.aethersdr.ulanziPlugin/plugin/app.js:12
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/core/QsoRecorder.cpp /tmp/aetherclaude/pr-5366/src/core/QsoRecorder.cpp:6
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5366/src/gui/MainWindow.cpp:8806
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5366/src/gui/MainWindow.cpp:8918
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5366/src/gui/MainWindow.cpp:9037
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5366/src/gui/MainWindow.cpp:9038
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-5366/src/gui/MainWindow.cpp:9039
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MemoryDialog.cpp /tmp/aetherclaude/pr-5366/src/gui/MemoryDialog.cpp:1191
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MemoryDialog.cpp /tmp/aetherclaude/pr-5366/src/gui/MemoryDialog.cpp:1417
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tests/hl2_live_band_filter_probe.cpp /tmp/aetherclaude/pr-5366/tests/hl2_live_band_filter_probe.cpp:98
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tests/ulanzi_mapping_migration_test.cpp /tmp/aetherclaude/pr-5366/tests/ulanzi_mapping_migration_test.cpp:206
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tests/ulanzi_mapping_migration_test.cpp /tmp/aetherclaude/pr-5366/tests/ulanzi_mapping_migration_test.cpp:207
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tools/check_engine_boundary.py /tmp/aetherclaude/pr-5366/tools/check_engine_boundary.py:113
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tools/gen_touchpoint_manifest.py /tmp/aetherclaude/pr-5366/tools/gen_touchpoint_manifest.py:68
  • [MEDIUM] CG-PATH-001 — Potential path traversal in tools/gen_touchpoint_manifest.py /tmp/aetherclaude/pr-5366/tools/gen_touchpoint_manifest.py:132

Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them.


🤖 aethersdr-agent · cost: $3.9858 · model: claude-opus-5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Aetherial Audio Channel Strip cannot be reopened after minimizing — its own button no longer restores it

3 participants