From 3b771d03f2df572de29af3156817de3cb7ef6bef Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 16:15:57 +0200 Subject: feat(rules): the rules window keeps its size and column widths Item 75. saveGeometry() and the rule list header's saveState() go to uistate.conf under keys of their own, written on closeEvent so a size survives Cancel as well as Save. The 760x520 resize stays as the first-run fallback. The backlog's approach was wrong on one point and a test caught it. It said to drop the resizeColumnToContents calls once a saved header state exists, which fixes the restore and leaves the original defect standing: with nothing saved, a width the user had just dragged was still discarded by the next add or delete. Each column is instead auto-sized once, on its first fill, after which the width belongs to the user however it was set. Two flags, because the count column is filled later by a reply from the worker. The window stays a QDialog. Making it a top-level window needs the unsaved-edit story that being modal currently sidesteps, and that is its own decision rather than part of this item. Both tests redirect XDG_STATE_HOME as well as XDG_CONFIG_HOME, so they cannot write the real uistate.conf. The geometry is asserted on the stored value rather than the reopened frame, per item 46: the offscreen platform does not honour a resize. Also corrects setFolders' doc comment, which still described the folder list as coming from Config. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 15 ++-- .../plans/2026-08-03-post-0.1.0-usability.md | 22 ++++- src/tagrulesdialog.cpp | 93 +++++++++++++++++++- src/tagrulesdialog.h | 39 ++++++++- tests/test_tagrules.cpp | 99 ++++++++++++++++++++++ 5 files changed, 255 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03b5046..0637cd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,11 +19,16 @@ point at which they are stable. stays visible and is what gets saved, so a rule the builder cannot show opens as text and still works. Opening a rule without editing it leaves the stored query untouched. -- A Folder condition picks from your configured accounts rather than being - typed. A folder path with a typo matches nothing and notmuch reports no - error, so the rule would simply never fire. The list stays editable, so a - folder that is in the rules file but not in your config still opens and - still saves. +- A Folder condition picks from a list of every folder in your Maildir rather + than being typed, Drafts and Sent included, not only the top of each + account. A folder path with a typo matches nothing and notmuch reports no + error, so the rule would simply never fire. The list is read from the tree + on disk, so a folder that exists but has no mail in it yet is still offered. + It stays editable, so a folder in the rules file that is no longer on disk + still opens and still saves. +- The tagging rules window remembers its size and the widths of the rule + list's columns. A column you widen also survives adding or deleting a rule, + which previously reset it. ### Fixed 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 00a7f76..d55336d 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 @@ -131,8 +131,8 @@ taking that too literally. | 72 | No khard/khal integration | workflow | ? | open, unspecified; the user places it after send, so v2 at the earliest | | 73 | This backlog is past four thousand lines | maintenance | S | open | | 74 | "Searching..." keeps claiming a query is running while rows are already arriving | feedback | XS | open; cause measured 2026-08-11, the delay itself is the cold page cache and is not fixable here | -| 75 | The tagging rules window forgets its size and its column widths | persistence | S | open; follows item 44 | -| 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | open; design approved 2026-08-13, see `specs/2026-08-13-rule-builder-design.md` | +| 75 | The tagging rules window forgets its size and its column widths | persistence | S | **done** 2026-08-13 on `rule-builder`, unreleased. The window-kind question is left open, see below | +| 76 | Every field in the rules dialog is free text, so a rule is easy to get wrong | workflow | M | **done** 2026-08-13 on `rule-builder`, unreleased. See `specs/2026-08-13-rule-builder-design.md` | | 77 | No way to see what a rule would collect, in the thread list | workflow | S | open; the dialog counts matches, it cannot show them | | 78 | No way to build a rule from something visible in a message | workflow | M | open; wants 76 first, so the created rule lands in a form that can hold it | | 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | @@ -4764,6 +4764,24 @@ the test asserts on the saved value rather than on the resulting frame. **Size: S.** +**Done 2026-08-13** on `rule-builder`, unreleased. `saveGeometry()` and the +list header's `saveState()` go to `tagrules/geometry` and `tagrules/header` in +`uistate.conf`, written on `closeEvent` so a size survives Cancel as well as +Save; `resize(760, 520)` stays as the first-run fallback. + +**The approach above was wrong on one point, and a test caught it.** It said to +drop the `resizeColumnToContents` calls "once a saved header state exists", +which fixes the restore and leaves the original defect standing: with no saved +state, a width the user had just dragged was still discarded by the next add or +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. + +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 +of this item. + ## 76. Every field in the rules dialog is free text, so a rule is easy to get wrong **Observed.** A rule is written by typing into four line edits, and the user diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 75a9cbe..61f2c21 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -20,8 +20,11 @@ #include #include +#include #include #include +#include +#include #include #include #include @@ -31,10 +34,13 @@ #include #include #include +#include #include #include #include +#include "mainwindow.h" + namespace { /// Columns of the rule list. @@ -72,6 +78,9 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) : QDialog(parent) { setWindowTitle(tr("Tagging rules")); + // The fallback for a first run. restoreUiState() overwrites it when a + // size was saved, and is called at the end of this constructor because + // the header state cannot be restored before the columns exist. resize(760, 520); m_rules.load(); @@ -244,6 +253,72 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) reloadList(); showWarnings(); + + // Last, and after reloadList(): a header state cannot be applied before + // the columns it describes exist, and reloadList is what fills them. + restoreUiState(); +} + +/// Reads the window size and the rule list's header layout back. +/// +/// The same file MainWindow uses, under keys of its own. Machine-written +/// state, never the hand-edited config: a column width is not something +/// anyone edits by hand, and mixing the two puts a blob in a file the user +/// reads. +void TagRulesDialog::restoreUiState() +{ + QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); + + const QByteArray geometry = + state.value(QStringLiteral("tagrules/geometry")).toByteArray(); + if (!geometry.isEmpty()) + restoreGeometry(geometry); + + const QByteArray header = + state.value(QStringLiteral("tagrules/header")).toByteArray(); + if (!header.isEmpty()) { + m_list->header()->restoreState(header); + // Counts as the one auto-size each column gets, so the restore is not + // immediately overwritten. reloadList() runs before this in the + // constructor and has already sized the first two; the count column + // has not been filled yet, and would otherwise resize over the + // restored width as soon as the first counts arrived. + m_columnsSized = true; + m_countColumnSized = true; + } +} + +void TagRulesDialog::saveUiState() +{ + QDir().mkpath(QFileInfo(MainWindow::uiStatePath()).absolutePath()); + QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); + state.setValue(QStringLiteral("tagrules/geometry"), saveGeometry()); + state.setValue(QStringLiteral("tagrules/header"), + 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) +{ + saveUiState(); + QDialog::closeEvent(event); +} + +int TagRulesDialog::columnWidthForTest(int column) const +{ + return m_list->columnWidth(column); +} + +void TagRulesDialog::setColumnWidthForTest(int column, int width) +{ + m_list->setColumnWidth(column, width); +} + +void TagRulesDialog::reloadListForTest() +{ + reloadList(); } void TagRulesDialog::showWarnings() @@ -286,8 +361,15 @@ void TagRulesDialog::reloadList() auto *item = new QTreeWidgetItem(m_list); fillItem(item, rule); } - m_list->resizeColumnToContents(ColumnEnabled); - m_list->resizeColumnToContents(ColumnStage); + // Auto-sized on the FIRST fill only. After that the widths belong to the + // user, whether they came from a restored header or from a drag in this + // session, and resizing on every repopulate threw both away on the next + // add or delete. + if (!m_columnsSized) { + m_list->resizeColumnToContents(ColumnEnabled); + m_list->resizeColumnToContents(ColumnStage); + m_columnsSized = true; + } m_reloading = false; if (!m_working.isEmpty()) @@ -439,7 +521,12 @@ void TagRulesDialog::setCounts(const QVector &counts) counts.at(i) < 0 ? tr("invalid") : QString::number(counts.at(i))); } - m_list->resizeColumnToContents(ColumnCount); + // Once, like the columns in reloadList. The counts arrive after the first + // fill, so this column gets its own flag rather than sharing that one. + if (!m_countColumnSized) { + m_list->resizeColumnToContents(ColumnCount); + m_countColumnSized = true; + } } void TagRulesDialog::onSave() diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index 9559918..f86d210 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -56,9 +56,14 @@ public: /// handle and notmuch permits one per process. QStringList countQueries() const; - /// Account subdirectory names, for the Folder row's dropdown. Supplied by - /// the caller rather than read here: Config knows them, and this dialog - /// deliberately holds no Config of its own. + /// Maildir folder paths, for the Folder row's dropdown, relative to the + /// database root. Supplied by the caller rather than read here: they come + /// from a scan of the tree under notmuch's database.path, which only + /// NotmuchWorker can answer for, and this dialog reaches neither it nor + /// Config. + /// + /// Arrives AFTER the dialog is on screen, since the scan crosses to the + /// worker on a queued call, so this refills the rows that already exist. void setFolders(const QStringList &folders); /// Test seams. The builder's state is otherwise reachable only through @@ -88,6 +93,20 @@ public: /// when the widget is not. bool textModeToggleIsReachableForTest() const; + /// Width of one rule-list column. The geometry restore is asserted on the + /// saved and reread VALUE rather than on the resulting frame: item 46 + /// records that the offscreen platform does not honour a window resize, + /// so a test comparing frames there passes or fails for reasons that have + /// nothing to do with the code. + int columnWidthForTest(int column) const; + void setColumnWidthForTest(int column, int width); + + /// Repopulates the rule list, as adding or deleting a rule does. Exposed + /// because a restored column width has to survive one of these, not only + /// a close and reopen: `resizeColumnToContents` on every reload discarded + /// the width the user had dragged. + void reloadListForTest(); + signals: /// Asks the owner to run countQueries() through the worker. void countsRequested(); @@ -104,8 +123,13 @@ private slots: void applyEditsToCurrentRule(); void onSave(); +protected: + void closeEvent(QCloseEvent *event) override; + private: void reloadList(); + void restoreUiState(); + void saveUiState(); void showWarnings(); int currentIndex() const; @@ -160,6 +184,15 @@ private: /// row's text, which applyEditsToCurrentRule() has no chance to flush. bool m_reloading = false; + /// Each column is auto-sized to its contents ONCE, on its first fill. + /// After that its width belongs to the user, whether it came from a + /// restored header or from a drag, and resizeColumnToContents on every + /// repopulate threw both away on the next add or delete. Two flags rather + /// than one because the count column is filled later than the rest, by a + /// reply from the worker. + bool m_columnsSized = false; + bool m_countColumnSized = false; + QTreeWidget *m_list = nullptr; QLineEdit *m_id = nullptr; QLineEdit *m_add = nullptr; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index cf7dadb..af30938 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -16,9 +16,11 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include #include #include +#include "mainwindow.h" #include "rulequery.h" #include "tagrules.h" #include "tagrulesdialog.h" @@ -49,6 +51,8 @@ private slots: void leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows(); void aFolderRowUsesTheDropdownAndKeepsItsSuffix(); void theTextModeToggleSurvivesBeingSwitchedOn(); + void theWindowSizeAndColumnWidthsSurviveAReopen(); + void aReloadDoesNotDiscardARestoredColumnWidth(); private: QString writeRules(const QString &json); @@ -634,5 +638,100 @@ void TestTagRules::theTextModeToggleSurvivesBeingSwitchedOn() QStringLiteral("from:vendor.example.org")); } +namespace { + +/// Writes a two-rule file under a throwaway XDG_CONFIG_HOME. Two rules rather +/// than one because the column-width tests reload the list, and a list with a +/// single row hides an off-by-one in the repopulate. +void writeTwoRules(const QTemporaryDir &configHome) +{ + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + QFile out(configHome.filePath(QStringLiteral("mailrules/rules.json"))); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "vendor", "query": "from:vendor.example.org", + "add": ["vendor"], "stage": 50, "enabled": true}, + {"id": "lists", "query": "to:list.example.org", + "add": ["lists"], "stage": 60, "enabled": true} + ] + })"); + out.close(); +} + +} // namespace + +void TestTagRules::theWindowSizeAndColumnWidthsSurviveAReopen() +{ + // The window opened at 760x520 whatever size it was left at, and the + // columns reset to their computed widths on every open. + // + // XDG_STATE_HOME is redirected as well as XDG_CONFIG_HOME: the state file + // is where this writes, and a test must not touch the user's real + // ~/.local/state/qtmaildir/uistate.conf. + 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); + + { + TagRulesDialog dialog; + dialog.resize(900, 640); + dialog.setColumnWidthForTest(0, 123); + // The save is on close, matching where MainWindow writes its own. + dialog.close(); + } + + // Asserted on the stored VALUE, not on the reopened frame. Item 46: the + // offscreen platform does not honour a resize, so a frame comparison here + // would report a failure the code did not cause. + QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); + QCOMPARE(state.value(QStringLiteral("tagrules/geometry")).toByteArray() + .isEmpty(), false); + + { + TagRulesDialog reopened; + QCOMPARE(reopened.columnWidthForTest(0), 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 + // DELETE either: reloadList called resizeColumnToContents on every + // repopulate, so a restore was undone by the first thing the user did in + // the window. Restoring on open and reverting on the next click is worse + // than never restoring at all, because it looks like the setting is + // broken rather than absent. + 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); + + TagRulesDialog dialog; + dialog.setColumnWidthForTest(0, 137); + dialog.reloadListForTest(); + QCOMPARE(dialog.columnWidthForTest(0), 137); + + if (previousState.isEmpty()) + qunsetenv("XDG_STATE_HOME"); + else + qputenv("XDG_STATE_HOME", previousState); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3