diff options
| -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 |