fix(gui): make Profile Manager non-modal with persistent geometry (#2574)#2592
fix(gui): make Profile Manager non-modal with persistent geometry (#2574)#2592aethersdr-agent[bot] wants to merge 1 commit into
Conversation
) Convert the Profiles → Profile Manager... action from a stack-allocated QDialog::exec() to a persistent QPointer<QDialog> opened via show()/raise()/activateWindow(), matching the pattern used by NetworkDiagnosticsDialog and PropDashboardDialog. Re-triggering the menu raises the existing window instead of opening a duplicate. The dialog persists window position and size in AetherSDR.settings under the "ProfileManagerDialogGeometry" key, restored on construction and saved on move/resize/close. Underlying profile logic across all four tabs is untouched.
There was a problem hiding this comment.
Looks good. The non-modal pattern (QPointer + WA_DeleteOnClose + show/raise/activateWindow) matches NetworkDiagnosticsDialog and PropDashboardDialog in MainWindow.cpp, and the geometry save/restore implementation (Base64 round-trip via AppSettings, m_restoring guard, closeEvent/moveEvent/resizeEvent overrides chaining to the base class) matches the existing pattern used by StripCompPanel and friends. Scope is tight — only the four files needed.
One observation, not blocking: this dialog isn't yet wired into setFramelessMode() alongside the other modeless dialogs around MainWindow.cpp:11045, but PR #2580 appears to be doing exactly that for the Profile Manager, so the two changes should slot together cleanly.
Thanks for the focused fix.
ten9876
left a comment
There was a problem hiding this comment.
Claude here, reviewing on Jeremy's behalf.
Status — parallel to PR #2591
Heads-up before the review proper: AetherClaude picked up issue #2574 and implemented a fix here, but PR #2591 (from @Miriam-R-coder, with my follow-up fixes pushed to her branch) targets the same issue and is further along in review. Reviewing this one on its own merits anyway, then comparing the two.
What's good in this PR
The implementation has some genuinely nice touches over #2591:
-
m_restoringguard prevents the recursive save-during-restore loop whenrestoreGeometry()triggersmoveEvent/resizeEventcallbacks. Smart detail that #2591 doesn't have. -
Save on
moveEvent/resizeEvent/closeEvent— geometry persists continuously, so an unclean app exit (crash, force-quit, SIGTERM) doesn't lose the user's last-known position. #2591 only saves on close, so a crash loses geometry. -
Cleaner header — events forward-declared via standard Qt headers, no pollution.
-
Standard base64 round-trip matching the project's
MainWindowGeometryconvention — correct from the start. -
All 5 CI checks green on the pre-rebase commit.
What's missing
Frameless chrome integration with PR #2580. This is the same gap I had to fix in #2591. PR #2580 (which has since merged) added setFramelessMode(bool) to ProfileManagerDialog with an outer FramelessWindowTitleBar + body layout. This PR doesn't:
- Call
setFramelessMode(framelessWindowEnabled())on first construction - Wire
m_profileManagerDialog->setFramelessMode(on)intoMainWindow::setFramelessWindow()for runtime toggle propagation
Symptom if merged as-is: with the default FramelessWindow=True setting, the user opens Profile Manager and sees BOTH the custom frameless title bar AND the native OS window decoration — a double-title-bar visual that's not what either codebase intends.
Auto-merge against current main confirms this conflict:
$ git merge origin/main --no-commit
Auto-merging src/gui/ProfileManagerDialog.cpp
CONFLICT (content): Merge conflict in src/gui/ProfileManagerDialog.cpp
The conflict is in the constructor where this PR adds restoreGeometryFromSettings() and #2580 added the frameless chrome setup. Both need to coexist — chrome setup happens first, then geometry restore.
Missing s.save() call. setValue modifies the in-memory AppSettings; save() flushes to disk. Without explicit save calls, geometry only persists across runs via AetherSDR's clean-exit save path. If the user closes Profile Manager but then AetherSDR crashes before clean exit, geometry is lost. Worth one s.save() after each setValue.
Setter cosmetic notes (non-blocking):
setModal(false)is redundant — QDialog defaults to non-modal except when shown viaexec()setWindowModality(Qt::NonModal)is the same setting, doubled. Either is enough, both is just noise
Comparison with PR #2591 (Miriam + my pushed fixes)
| Aspect | #2591 (after my push) | #2592 (this PR) |
|---|---|---|
| Lazy-construct + QPointer slot | ✓ | ✓ |
| WA_DeleteOnClose | ✓ | ✓ |
| Base64 round-trip | ✓ | ✓ |
| Save on closeEvent | ✓ | ✓ |
| Save on move/resize | — | ✓ (crash-resilient) |
m_restoring guard |
— | ✓ (recursive-save prevention) |
s.save() for immediate persistence |
✓ | — |
| #2580 frameless integration on construct | ✓ | — |
#2580 frameless propagation via setFramelessWindow() |
✓ | — |
| Rebased onto current main | ✓ | — (needs rebase) |
| CI green | yes (after my push) | yes (on pre-rebase commit) |
Recommendation — two paths
Path A: merge #2591, close this PR as superseded. Lower friction since #2591 is already rebased and integrated with #2580. Lose the move/resize event saving and m_restoring guard — but those can be filed as a small follow-up issue (aetherclaude-eligible, 15-line change).
Path B: take this PR's good ideas onto #2591's branch. Pull the moveEvent/resizeEvent/m_restoring pattern from here, apply to #2591, close this PR. More work but lands the best of both in one merge.
I'd lean Path A — #2591 has been through human review and rebase, the move/resize crash-resilience is a real-but-modest improvement that can come in a follow-up. Path B asks AetherClaude (or me) to do another round trip when the marginal improvement is small.
Either way, this PR shouldn't merge as-is because of the frameless integration gap. If AetherClaude wants to keep going, the cleanest follow-up would be the small move/resize/m_restoring helper applied as an issue tagged for the agent.
Thanks @aethersdr-agent — the architectural awareness here (saving on move/resize is more robust than save-on-close) is good. The miss is the same one human contributors hit on #2580-coupled dialogs: the frameless chrome integration isn't documented anywhere central. Worth solving via PersistentDialog (#2605) so neither humans nor agents have to discover it independently.
73, Jeremy KK7GWY & Claude (AI dev partner)
…log-patterns doc Cherry-picks the robustness improvements from AetherClaude's parallel PR aethersdr#2592 onto this branch: - m_restoringGeometry guard prevents the recursive save-during-restore loop when restoreGeometry() triggers move/resize events - Save geometry on moveEvent and resizeEvent in addition to closeEvent. setValue() is in-memory only — close-time save() flushes to disk. Move/resize saves keep the in-memory store current so crash or force-quit preserves the last-known position Also adds docs/dialog-patterns.md capturing the canonical lazy-construct + non-modal + geometry-persist + frameless-chrome pattern in one place. Multiple recent PRs (aethersdr#2591, aethersdr#2592, aethersdr#2580) have each rediscovered the same pitfalls — the doc lists them in a single table with the wrong/right pairing and the PR that surfaced each one. CLAUDE.md now points contributors at the doc before they write or modify any QDialog. The longer-term cleanup is the PersistentDialog base class proposed in aethersdr#2605; this doc is the bridge until that lands. Co-Authored-By: aethersdr-agent <aethersdr-agent@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Closing as superseded by #2591, which now incorporates this PR's robustness improvements. What we took from this PRPushed onto #2591 (`3ec211b7`):
Credited @aethersdr-agent as Co-Authored-By on the commit. Why this one didn't merge directlyTwo gaps relative to current main:
Both are now in #2591. Why the next AetherClaude PR shouldn't hit thisThe frameless-chrome discovery gap is the recurring miss. Filed `docs/dialog-patterns.md` as a central reference for the dialog pattern, with a table of common pitfalls and the PRs that surfaced each one. CLAUDE.md now points at it before any QDialog work. Longer-term, #2605 will eliminate the boilerplate via a `PersistentDialog` base class. Thanks @aethersdr-agent — the m_restoring guard and move/resize event saving were the most valuable contribution; they make the geometry persistence crash-resilient in a way Miriam's PR didn't. Real improvement, kept on the landing branch. 73, Jeremy KK7GWY & Claude (AI dev partner) |
Pull request was closed
|
Acknowledged — closing in favor of #2591 makes sense, and thanks for keeping the The frameless-chrome miss is fair. I had Profile Manager set up as a plain I'll watch for 73, AetherClaude |
* Add geometry restoration and saving in ProfileManagerDialog Signed-off-by: Miriam <m058325352@gmail.com> * Add ProfileManagerDialog pointer to MainWindow Signed-off-by: Miriam <m058325352@gmail.com> * Update profile manager dialog handling Refactor profile manager dialog connection to allow multiple instances. Signed-off-by: Miriam <m058325352@gmail.com> * Add closeEvent method to ProfileManagerDialog Signed-off-by: Miriam <m058325352@gmail.com> * feat(profile-manager): adopt #2592 geometry robustness + dialog-patterns doc Cherry-picks the robustness improvements from AetherClaude's parallel PR #2592 onto this branch: - m_restoringGeometry guard prevents the recursive save-during-restore loop when restoreGeometry() triggers move/resize events - Save geometry on moveEvent and resizeEvent in addition to closeEvent. setValue() is in-memory only — close-time save() flushes to disk. Move/resize saves keep the in-memory store current so crash or force-quit preserves the last-known position Also adds docs/dialog-patterns.md capturing the canonical lazy-construct + non-modal + geometry-persist + frameless-chrome pattern in one place. Multiple recent PRs (#2591, #2592, #2580) have each rediscovered the same pitfalls — the doc lists them in a single table with the wrong/right pairing and the PR that surfaced each one. CLAUDE.md now points contributors at the doc before they write or modify any QDialog. The longer-term cleanup is the PersistentDialog base class proposed in #2605; this doc is the bridge until that lands. Co-Authored-By: aethersdr-agent <aethersdr-agent@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Signed-off-by: Miriam <m058325352@gmail.com> Co-authored-by: Jeremy Fielder <kk7gwy@aethersdr.com> Co-authored-by: aethersdr-agent <aethersdr-agent@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Fixes #2574
What was changed
fix(gui): make Profile Manager non-modal with persistent geometry (#2574)
Files modified
src/gui/MainWindow.cppsrc/gui/MainWindow.hsrc/gui/ProfileManagerDialog.cppsrc/gui/ProfileManagerDialog.hGenerated by AetherClaude (automated agent for AetherSDR)