diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-13 16:21:42 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-13 16:21:42 +0200 |
| commit | 0422205803c8f6980a447fdaa1dc270a486970cd (patch) | |
| tree | af9d8b005ad5ffae91860dfdadf727ce578fb60f | |
| parent | 3b771d03f2df572de29af3156817de3cb7ef6bef (diff) | |
| download | qtmaildir-0422205803c8f6980a447fdaa1dc270a486970cd.tar.gz qtmaildir-0422205803c8f6980a447fdaa1dc270a486970cd.zip | |
fix(rules): save the window size on Cancel and Save, not only on X
The geometry was saved from closeEvent, and neither dialog button sends
one: Cancel calls reject(), Save calls accept(), and only the window
manager's X button produces a QCloseEvent. So the size and the column
widths were kept for the one route out of three that a user almost never
takes, and a resize followed by Cancel came back forgotten.
The save moves to a done(int) override, which both buttons funnel
through and which QWidget::close() also reaches.
The test that covered this passed against the bug because it asserted
with close(). It now drives all three routes rather than trusting one to
stand for the others, and shows the dialog before the close leg:
close() on a widget that was never visible returns early without
reaching done(), so that assertion would otherwise prove nothing.
Both traps recorded in CLAUDE.md, since neither is specific to this
dialog.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| -rw-r--r-- | CLAUDE.md | 11 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 12 | ||||
| -rw-r--r-- | src/tagrulesdialog.cpp | 20 | ||||
| -rw-r--r-- | src/tagrulesdialog.h | 7 | ||||
| -rw-r--r-- | tests/test_tagrules.cpp | 58 |
5 files changed, 98 insertions, 10 deletions
@@ -317,6 +317,17 @@ current index invalid when nothing was current. A test that calls `selectAll()` view therefore passes against a missing selection guard, because no signal ever fires. Test multi-select from a row that is already current, which is also how a user reaches it. +**A `QDialog`'s buttons do not send a `QCloseEvent`.** `accept()` and `reject()` +go through `done(int)`, which hides the dialog without ever closing a window, so +a `closeEvent` override runs only for the window manager's X button. Anything a +dialog must persist on the way out belongs in a `done(int)` override, which both +buttons and `close()` reach. This shipped wrong in the rules dialog and the test +covering it passed, because the test used `close()` and the user used Cancel: +one route out of three. Assert every route. Underneath sits a second trap: +`close()` on a widget that was never shown returns early WITHOUT reaching +`done()`, so a test for the closed path has to `show()` the dialog first or it +asserts nothing at all. + **A queued load can outlive the state that started it.** `loadThread` crosses to the worker on a queued connection, so its reply lands after whatever the UI did in the meantime. The generation counter covers a superseded *query*, not a superseded *selection*: blanking the diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index d55336d..bce5742 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -4777,6 +4777,18 @@ delete. The rule shipped instead is that each column is auto-sized ONCE, on its first fill, after which its width belongs to the user however it was set. Two flags, because the count column is filled later by a reply from the worker. +**It then shipped broken once more, and the test that covered it passed.** The +save was written in `closeEvent`, and the test asserted with `close()`. Neither +button goes anywhere near either: Cancel calls `reject()`, Save calls +`accept()`, and only the window manager's X button sends a `QCloseEvent`. So +the size was kept for the one route out of three that the buttons never take, +and the user found it in one try by resizing and pressing Cancel. The save now +overrides `done(int)`, which both buttons funnel through and `close()` reaches, +and the test asserts all three routes rather than trusting one to stand for the +others. A second trap sits underneath: `close()` on a widget that was never +shown returns early without reaching `done()`, so that leg of the test has to +`show()` first or it proves nothing. + The **popup or primary window** question was put to the user and deliberately not taken: it stays a `QDialog`. Reopening it needs the unsaved-edit story that being modal currently sidesteps, and that is its own decision rather than part diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 61f2c21..54600c3 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -20,7 +20,6 @@ #include <QButtonGroup> #include <QCheckBox> -#include <QCloseEvent> #include <QComboBox> #include <QDialogButtonBox> #include <QDir> @@ -297,13 +296,22 @@ void TagRulesDialog::saveUiState() m_list->header()->saveState()); } -/// Saves on close rather than on accept, so a size the user chose is kept -/// whether they pressed Save or Cancel. The window's shape is not part of the -/// edit being confirmed. -void TagRulesDialog::closeEvent(QCloseEvent *event) +/// Saves on the way out, whichever way that is. +/// +/// done() rather than closeEvent, and this distinction shipped broken: Cancel +/// calls reject() and Save calls accept(), and NEITHER sends a QCloseEvent. +/// Only the window manager's X button does. Saving from closeEvent therefore +/// kept the size for the one route the buttons never take, which is how a +/// resize followed by Cancel came back forgotten. Both buttons funnel through +/// done(), and QWidget::close() reaches it too. +/// +/// On every route, not only on accept: the window's shape is not part of the +/// edit being confirmed, so Cancel should discard the rule changes and keep +/// the size. +void TagRulesDialog::done(int result) { saveUiState(); - QDialog::closeEvent(event); + QDialog::done(result); } int TagRulesDialog::columnWidthForTest(int column) const diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index f86d210..bdcaff9 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -123,8 +123,11 @@ private slots: void applyEditsToCurrentRule(); void onSave(); -protected: - void closeEvent(QCloseEvent *event) override; +public slots: + /// Saves the window's size and column widths on the way out. Overridden + /// here rather than closeEvent because Save and Cancel do not send a + /// close event at all, only the window manager's X button does. + void done(int result) override; private: void reloadList(); diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index af30938..0d81df7 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -52,6 +52,7 @@ private slots: void aFolderRowUsesTheDropdownAndKeepsItsSuffix(); void theTextModeToggleSurvivesBeingSwitchedOn(); void theWindowSizeAndColumnWidthsSurviveAReopen(); + void theWindowSizeIsSavedOnEveryWayOutOfTheDialog(); void aReloadDoesNotDiscardARestoredColumnWidth(); private: @@ -683,8 +684,13 @@ void TestTagRules::theWindowSizeAndColumnWidthsSurviveAReopen() TagRulesDialog dialog; dialog.resize(900, 640); dialog.setColumnWidthForTest(0, 123); - // The save is on close, matching where MainWindow writes its own. - dialog.close(); + // CANCEL, not close(). The first version of this saved from + // closeEvent and asserted with close(), which passes while the real + // dialog forgets everything: Cancel calls reject() and Save calls + // accept(), and neither sends a QCloseEvent. Only the window + // manager's X button does, so the test exercised the one path the + // buttons never take. The user found it by hand in one try. + dialog.reject(); } // Asserted on the stored VALUE, not on the reopened frame. Item 46: the @@ -705,6 +711,54 @@ void TestTagRules::theWindowSizeAndColumnWidthsSurviveAReopen() qputenv("XDG_STATE_HOME", previousState); } +void TestTagRules::theWindowSizeIsSavedOnEveryWayOutOfTheDialog() +{ + // There are three ways out and they take different code paths: Cancel + // calls reject(), Save calls accept(), and the window manager's X button + // sends a QCloseEvent. Saving from closeEvent alone covers only the + // third, which is how the first version of this shipped and forgot the + // size on both buttons. done(int) is the funnel the two buttons share and + // close() also reaches, so all three are asserted here rather than + // trusting one to stand for the others. + QTemporaryDir configHome; + QTemporaryDir stateHome; + QVERIFY(configHome.isValid()); + QVERIFY(stateHome.isValid()); + const QByteArray previousState = qgetenv("XDG_STATE_HOME"); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + qputenv("XDG_STATE_HOME", stateHome.path().toUtf8()); + writeTwoRules(configHome); + + const auto widthAfter = [&](int width, const char *how) { + QFile::remove(MainWindow::uiStatePath()); + TagRulesDialog dialog; + // Shown, because QWidget::close() on a widget that was never visible + // returns early without reaching done(). The X button it stands for + // only exists on a window that is on screen, so testing the closed + // path from a hidden dialog proves nothing about it. + dialog.show(); + dialog.setColumnWidthForTest(0, width); + if (qstrcmp(how, "reject") == 0) + dialog.reject(); + else if (qstrcmp(how, "accept") == 0) + dialog.saveForTest(); + else + dialog.close(); + + TagRulesDialog reopened; + return reopened.columnWidthForTest(0); + }; + + QCOMPARE(widthAfter(121, "reject"), 121); + QCOMPARE(widthAfter(122, "accept"), 122); + QCOMPARE(widthAfter(123, "close"), 123); + + if (previousState.isEmpty()) + qunsetenv("XDG_STATE_HOME"); + else + qputenv("XDG_STATE_HOME", previousState); +} + void TestTagRules::aReloadDoesNotDiscardARestoredColumnWidth() { // The width did not survive a close, and it did not survive an ADD or a |
