Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,7 @@ set(GUI_SOURCES
src/gui/MainWindow.cpp
src/gui/MainWindowHelpers.cpp
src/gui/WindowGeometryRestore.cpp
src/gui/WindowShowState.cpp
src/gui/MainWindow_Controllers.cpp
src/gui/MainWindow_DspApplets.cpp
src/gui/MainWindow_Menus.cpp
Expand Down
12 changes: 8 additions & 4 deletions src/gui/MainWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@
#include "models/XvtrPolicy.h"
#include "core/BandStackSettings.h"
#include "gui/BandStackPanel.h"
#include "gui/WindowShowState.h"
#include "models/TunerModel.h"
#include "models/TransmitModel.h"
#include "models/EqualizerModel.h"
Expand Down Expand Up @@ -9378,12 +9379,15 @@ void MainWindow::toggleAetherialStrip()
m_aetherialStrip->setMicInputReady(ready);
m_aetherialStrip->setTxActive(ready && tx.isTransmitting());
}
if (m_aetherialStrip->isVisible()) {
// windowIsShowing() rather than isVisible(): a minimized strip still
// reports isVisible(), so the bare check sent it down the hide() branch and
// the next press called show(), which restores it straight back to
// minimized. The strip could then only be recovered from the taskbar/Dock,
// never from its own button.
if (windowIsShowing(m_aetherialStrip)) {
m_aetherialStrip->hide();
} else {
m_aetherialStrip->show();
m_aetherialStrip->raise();
m_aetherialStrip->activateWindow();
showAndRaiseWindow(m_aetherialStrip);
}
}

Expand Down
16 changes: 5 additions & 11 deletions src/gui/MainWindow_Nets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include "NetReminderBanner.h"
#include "NetSchedulerDialog.h"
#include "WindowShowState.h"

#include "core/AppSettings.h"
#include "core/LogManager.h"
Expand Down Expand Up @@ -222,13 +223,9 @@ void MainWindow::onNetReminderDue(const NetEntry& entry, const QDateTime& occurr
break;
}
}
// Bring the window forward without disturbing its state.
// showNormal() would clear a Maximized/FullScreen window
// (#3918) — only un-minimize if actually minimized.
if (isMinimized())
showNormal();
raise();
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.)

});
}
m_netReminderBanner->showReminder(entry.id, headline, detail, canTune);
Expand All @@ -243,10 +240,7 @@ void MainWindow::onNetReminderDue(const NetEntry& entry, const QDateTime& occurr
m_trayIcon->show();
connect(m_trayIcon, &QSystemTrayIcon::messageClicked, this, [this] {
// Raise without un-maximizing the window (#3918).
if (isMinimized())
showNormal();
raise();
activateWindow();
showAndRaiseWindow(this);
});
}
if (m_trayIcon) {
Expand Down
27 changes: 27 additions & 0 deletions src/gui/WindowShowState.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#include "gui/WindowShowState.h"

#include <QWidget>

namespace AetherSDR {

bool windowIsShowing(const QWidget* w)
{
return w && w->isVisible() && !w->isMinimized();
}

void showAndRaiseWindow(QWidget* w)
{
if (!w)
return;
Comment on lines +14 to +15

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;
}

Comment on lines +14 to +15

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;
}

// Clear ONLY the minimized bit. showNormal() would also clear Maximized
// and FullScreen, so a strip that was maximized, then minimized, would come
// back at normal size (#3918). On a hidden widget this is a pending state
// that show() applies; on a visible one it takes effect immediately and
// show() is a no-op.
w->setWindowState(w->windowState() & ~Qt::WindowMinimized);
w->show();
w->raise();
w->activateWindow();
}

} // namespace AetherSDR
25 changes: 25 additions & 0 deletions src/gui/WindowShowState.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#pragma once

class QWidget;

namespace AetherSDR {

// Window show-state helpers for press-to-open / press-again-to-close buttons.
//
// QWidget::isVisible() stays TRUE for a minimized window, and QWidget::show()
// on a minimized window restores it to its saved state — which is minimized.
// A toggle written as `if (w->isVisible()) w->hide(); else w->show();` is
// therefore unrecoverable once the window is minimized: the first press hides
// it, and the second press "shows" it straight back into the taskbar/Dock.
// Both behaviours are pinned by window_show_state_test.

// True when w is actually on screen for the user — visible AND not minimized.
[[nodiscard]] bool windowIsShowing(const QWidget* w);

// Bring w to the front, un-minimizing it first if needed. Only the Minimized
// bit is cleared, so a Maximized or FullScreen window keeps that state — both
// when it is merely raised and when it is restored from the taskbar/Dock
// (#3918). showNormal() would drop it in either case.
void showAndRaiseWindow(QWidget* w);

} // namespace AetherSDR
14 changes: 14 additions & 0 deletions tests/tests.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,20 @@ add_test(NAME window_geometry_restore_test COMMAND window_geometry_restore_test)
set_tests_properties(window_geometry_restore_test PROPERTIES
ENVIRONMENT "QT_QPA_PLATFORM=offscreen")

# Aetherial Audio Channel Strip toggle: a minimized window still reports
# isVisible() and show() does not un-minimize it, so the old toggle could not
# reopen the strip once minimized. Drives a real QWidget offscreen so both Qt
# behaviours are pinned rather than assumed.
add_executable(window_show_state_test
tests/window_show_state_test.cpp
src/gui/WindowShowState.cpp
)
target_include_directories(window_show_state_test PRIVATE src)
target_link_libraries(window_show_state_test PRIVATE Qt6::Widgets)
add_test(NAME window_show_state_test COMMAND window_show_state_test)
set_tests_properties(window_show_state_test PROPERTIES
ENVIRONMENT "QT_QPA_PLATFORM=offscreen")

# Workspace canvas (RFC #4887) phase 1 — normalized geometry. Pure logic, no
# widgets: the edge-rounding rule that keeps tiled items seam-free, and the
# resolution independence the whole RFC rests on, are pinned on every platform.
Expand Down
129 changes: 129 additions & 0 deletions tests/window_show_state_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
// Regression test for the Aetherial Audio Channel Strip toggle, which could
// not reopen the window once it had been minimized.
//
// Two Qt behaviours combine into the bug, and both are pinned here against a
// REAL QWidget rather than asserted from memory:
//
// 1. QWidget::isVisible() stays TRUE while a window is minimized. A toggle
// written `if (w->isVisible()) w->hide(); else w->show();` therefore
// treats a minimized window as "showing" and hides it.
// 2. QWidget::show() on a minimized window restores its SAVED state, which
// is still minimized — so the follow-up press does not recover it either.
//
// windowIsShowing() fixes (1) and showAndRaiseWindow() fixes (2), while the
// isMinimized() guard inside it keeps a maximized window maximized (#3918).
//
// Runs on the offscreen platform, where both behaviours reproduce.

#include "gui/WindowShowState.h"

#include <QApplication>
#include <QWidget>

#include <cstdio>

using AetherSDR::showAndRaiseWindow;
using AetherSDR::windowIsShowing;

namespace {

int g_failures = 0;

void report(const char* name, bool ok)
{
std::printf("%s %s\n", ok ? "[ OK ]" : "[FAIL]", name);
if (!ok) {
++g_failures;
}
}

// The toggle exactly as MainWindow::toggleAetherialStrip() runs it.
void toggle(QWidget* w)
{
if (windowIsShowing(w))
w->hide();
else
showAndRaiseWindow(w);
}

} // namespace

int main(int argc, char** argv)
{
QApplication app(argc, argv);

// --- Case 1: null is never "showing", and raising it must not crash.
report("windowIsShowing(nullptr) is false", !windowIsShowing(nullptr));
showAndRaiseWindow(nullptr);
report("showAndRaiseWindow(nullptr) is a no-op", true);

QWidget w;
w.resize(320, 240);

// --- Case 2: a hidden window is not showing; the toggle opens it.
report("hidden window is not showing", !windowIsShowing(&w));
toggle(&w);
app.processEvents();
report("toggle opens a hidden window", windowIsShowing(&w));

// --- Case 3: the toggle closes an ordinary open window.
toggle(&w);
app.processEvents();
report("toggle hides a showing window", !w.isVisible());

// --- Case 4: THE BUG. Qt reports a minimized window as visible, so the
// old bare isVisible() check would take the hide() branch here.
w.show();
app.processEvents();
w.showMinimized();
app.processEvents();
report("Qt: minimized window still reports isVisible()", w.isVisible());
report("Qt: minimized window reports isMinimized()", w.isMinimized());
report("windowIsShowing() treats minimized as not showing",
!windowIsShowing(&w));

// --- Case 5: why the second press never recovered it either — show() on a
// minimized window leaves it minimized.
w.show();
app.processEvents();
report("Qt: show() does not un-minimize", w.isMinimized());

// --- Case 6: the toggle restores a minimized window in ONE press.
toggle(&w);
app.processEvents();
report("toggle restores a minimized window", windowIsShowing(&w));
report("restored window is no longer minimized", !w.isMinimized());

// --- Case 7: the isMinimized() guard — raising a maximized window must
// not drop it out of maximized state (#3918).
w.showMaximized();
app.processEvents();
if (w.isMaximized()) {
showAndRaiseWindow(&w);
app.processEvents();
report("showAndRaiseWindow keeps a maximized window maximized",
w.isMaximized());

// --- Case 8: the reopen path itself must not drop maximize. This is
// what showNormal() got wrong: it clears Minimized AND Maximized, so a
// maximized strip that was minimized came back at normal size.
w.showMinimized();
app.processEvents();
report("Qt: minimizing keeps the Maximized bit alongside Minimized",
w.isMinimized() && (w.windowState() & Qt::WindowMaximized));
toggle(&w);
app.processEvents();
report("toggle restores a minimized-from-maximized window",
windowIsShowing(&w));
report("restored window is still maximized", w.isMaximized());
} 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");
Comment on lines +119 to +122

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.

}

if (g_failures == 0) {
std::printf("All window show-state tests passed.\n");
}
return g_failures == 0 ? 0 : 1;
}
Loading