From 4361411957e3e4d0b4739cf784fc12dfa8654714 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 11:46:51 +0200 Subject: docs: changelog for the rule builder --- CHANGELOG.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'CHANGELOG.md') diff --git a/CHANGELOG.md b/CHANGELOG.md index fa9e7cf..03b5046 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,28 @@ point at which they are stable. ## [Unreleased] +### Added + +- The tagging rules dialog builds a rule from rows now: a field and operator + dropdown per condition, `+`/`-` to add and remove them, a match all/any + choice, and a separate "but not" block for exclusions. The notmuch query + 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. + +### Fixed + +- Opening the tagging rules dialog and saving destroyed the first rule in the + list, even with nothing edited. The rule lost its query and its tags, then + vanished on the next load, since a rule with an empty query is dropped as + malformed. Populating the form emitted a change signal that wrote the form + back over the rule before the query field had been filled. + ## [0.16.0] - 2026-08-13 The rules that tag your mail on arrival move out of a shell script and into the -- cgit v1.2.3 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(-) (limited to 'CHANGELOG.md') 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 From 1dd39414fd6139f2934554fc90b1c5721603d14b Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 16:35:37 +0200 Subject: docs: the window size cannot be restored under a tiling compositor Item 75 shipped claiming the rules window remembers its size. It does not, and no code here can make it. Hyprland tiles the window to fill its slot, so the size dragged belongs to the tile. saveGeometry stores frameGeometry beside normalGeometry and restoreGeometry restores the normal one, which stays at whatever resize() last set it to. Decoded from the real state file after a hand test: frame 2248x806, normal 760x664. The dialog restores 760 correctly and still opens tiled. Three diagnoses were tried before this one and each was disproved by a probe rather than argued away: that restoreGeometry rejected the blob as off-screen, that the layout overrode a geometry applied before the first show, and that a test could tell the broken and fixed versions apart. The last one matters most: the offscreen platform returns an identical frame for both, so a size assertion passed against the bug and a mutation restoring it left the suite green. That assertion is not reinstated. The column widths, which are what actually works, keep their test. The changelog and the backlog entry are corrected to say what ships, and CLAUDE.md gains both the tiling-compositor trap and the rule that the offscreen platform cannot test window sizing at all. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 ++++--- CLAUDE.md | 19 +++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 27 ++++++++++++++++++---- tests/test_tagrules.cpp | 8 +++++++ 4 files changed, 55 insertions(+), 7 deletions(-) (limited to 'CHANGELOG.md') diff --git a/CHANGELOG.md b/CHANGELOG.md index 0637cd9..bbcc854 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,9 +26,11 @@ point at which they are stable. 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. +- The tagging rules window remembers the widths of the rule list's columns. A + column you widen also survives adding or deleting a rule, which previously + reset it. The window's own size is saved too, but a tiling window manager + sizes the window itself, so there it opens at whatever size the tile gives + it. ### Fixed diff --git a/CLAUDE.md b/CLAUDE.md index 107fe28..6ab47b3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -328,6 +328,25 @@ one route out of three. Assert every route. Underneath sits a second trap: `done()`, so a test for the closed path has to `show()` the dialog first or it asserts nothing at all. +**Under a tiling compositor a window's size is not the application's to +restore, and the user's desktop is Hyprland.** `saveGeometry` stores +`frameGeometry` and `normalGeometry`; `restoreGeometry` restores the NORMAL +one. When the compositor tiles the window to fill its slot, the size the user +drags is the tile's, and `normalGeometry` keeps whatever the code last passed to +`resize()`. Measured against the real state file after a hand test: frame +2248x806, normal 760x664, so the dialog correctly restored 760 and correctly +looked broken. A whole session went into "the geometry restore is broken" before +the blob was decoded. Decode the stored geometry before theorising, and expect +`maximized` to read as a value no bool should hold, which is the tiled state Qt +records and does not round-trip. + +The corollary for tests: **the offscreen platform cannot test window sizing at +all.** It prints "This plugin does not support propagateSizeHints()" and returns +an identical frame for a correct restore and a broken one, verified in a +standalone program containing none of this project's code. A size assertion +there passes against both, and a mutation putting the bug back leaves the suite +green. Assert on the stored value, and leave the frame to a hand test. + **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 bce5742..e9f18d9 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 @@ -4764,10 +4764,29 @@ 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. +**Done 2026-08-13** on `rule-builder`, unreleased, for the COLUMN WIDTHS. +`saveGeometry()` and the list header's `saveState()` go to `tagrules/geometry` +and `tagrules/header` in `uistate.conf`, written on `done(int)` so they survive +Cancel as well as Save; `resize(760, 520)` stays as the first-run fallback. + +**The window SIZE does not come back, and that half of the item cannot be +fixed here.** The user's desktop is Hyprland, a tiling compositor. It tiles the +window to fill its slot, so the size dragged is the tile's; `saveGeometry` +records `frameGeometry` and `normalGeometry` and `restoreGeometry` restores the +NORMAL one, which stays at whatever `resize()` last set. Decoded from the real +state file after a hand test: frame 2248x806, normal 760x664. The code restores +760 faithfully and the window still opens tiled. + +Three wrong diagnoses were tried and each was disproved by a probe rather than +by argument: that `restoreGeometry` rejected the blob as off-screen (it returns +true on the real display; the negative y is the DP-1 origin), that the layout +overrode a geometry set before the first show (a `showEvent` restore produced +the identical size), and that the offscreen test could tell the two apart (it +returns the same frame for both, so the mutation survived). + +Nothing worth building remains unless the user wants the dialog to open at a +remembered size when floated, which needs a Hyprland window rule rather than +code here. **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", diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 0d81df7..8ca7671 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -696,6 +696,14 @@ void TestTagRules::theWindowSizeAndColumnWidthsSurviveAReopen() // 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. + // + // And on a TILING compositor the frame is not the dialog's to restore at + // all. saveGeometry stores frameGeometry beside normalGeometry, and + // restoreGeometry restores the NORMAL one; under Hyprland the window is + // tiled to fill its slot, so the size the user drags belongs to the tile + // while normalGeometry stays at whatever the code last resize()d it to. + // Measured against the real state file: frame 2248x806, normal 760x664. + // Restoring 760 there is correct behaviour, not the bug it looks like. QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); QCOMPARE(state.value(QStringLiteral("tagrules/geometry")).toByteArray() .isEmpty(), false); -- cgit v1.2.3 From 1ab12862fd47bf3816f7fad7b8a96eceb8f84272 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 16:44:16 +0200 Subject: fix(rules): a long rule no longer squeezes the rule list away Item 80. A rule with eight From conditions left the list showing about one and a half rows. The list was added with stretch 1 and the form below it with none, which looks decisive and is not: a stretch factor only distributes space above each widget's minimum, and the form's minimum grew with every condition row, so each row came straight out of the list. The builder asked for 120px with one row and 414px with eight. A QSplitter now divides the list from the editor, so the balance is the user's and is saved beside the column widths, and the condition rows sit in a QScrollArea capped at 190px so the editor cannot grow without bound however the splitter is set. The scroll area is what text mode hides; hiding the builder inside it would leave an empty frame. Three measures were tried in the test before one told the bug and the fix apart, and two passed against broken code: the dialog's minimumSizeHint does not track form rows and read 580 either way, and a qMin against the scroll area's own hint read small whether or not the cap was set, since an uncapped maximumHeight is QWIDGETSIZE_MAX. What survives mutation is the editor pane's minimum inside the splitter, plus the cap read directly, and both are asserted. A row's size hint is invalid until the event loop runs, so the test calls processEvents after selecting a rule or it measures the same height twice. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 ++ .../plans/2026-08-03-post-0.1.0-usability.md | 34 ++++++++++ src/tagrulesdialog.cpp | 76 ++++++++++++++++++++-- src/tagrulesdialog.h | 30 +++++++++ tests/test_tagrules.cpp | 72 ++++++++++++++++++++ 5 files changed, 211 insertions(+), 6 deletions(-) (limited to 'CHANGELOG.md') diff --git a/CHANGELOG.md b/CHANGELOG.md index bbcc854..e46584a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,11 @@ point at which they are stable. 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 rule list and the rule editor are now divided by a draggable splitter, + and the condition rows scroll instead of growing without limit. A rule with + eight senders used to squeeze the list to about one visible row, since the + editor grew with every condition and the list gave up the space. Where you + leave the divider is remembered. - The tagging rules window remembers the widths of the rule list's columns. A column you widen also survives adding or deleting a rule, which previously reset it. The window's own size is saved too, but a tiling window manager 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 e9f18d9..110938d 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 @@ -135,6 +135,7 @@ taking that too literally. | 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 | +| 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13 on `rule-builder`, unreleased. Follows item 76 | | 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 | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -4975,6 +4976,39 @@ in a released build. **Size: XS** for the fix. The reproduction and the field repair were the work. +## 80. A rule with many conditions squeezes the rule list to one visible row + +**Observed.** A rule with eight From conditions left the rule list showing +about one and a half rows, with the second rule half cut off under the first. +Reported with a screenshot; the builder filled the window and the list it sits +under kept almost nothing. + +**Cause.** `m_list` was added to the dialog's `QVBoxLayout` with stretch 1 +(`src/tagrulesdialog.cpp:109`) and the form below it with none, which looks +like the list wins. It does not: a stretch factor only distributes space ABOVE +each widget's minimum, and the form's minimum grows with every condition row, +so each row came directly out of the list. Measured on the builder's size hint: +120px with one row, 414px with eight. + +**Approach.** Done 2026-08-13. A `QSplitter` divides the list from the editor, +so the balance is the user's and is saved to `uistate.conf` beside the column +widths, and the condition rows sit in a `QScrollArea` capped at 190px so the +editor cannot grow without bound whatever the splitter is set to. The scroll +area rather than the builder is what text mode hides, since hiding the inner +widget would leave an empty frame. + +**Constraints.** Three measures were tried before one distinguished the bug +from the fix, and two passed against broken code: the dialog's +`minimumSizeHint` does not track form rows and read 580 either way, and a +`qMin` against the scroll area's own size hint read small whether or not the +cap was set, because an uncapped `maximumHeight` is `QWIDGETSIZE_MAX`. The +assertions that survive mutation are the editor pane's minimum inside the +splitter, and the cap read directly. A row's size hint is also invalid until +the event loop has run, so the test needs `processEvents` after selecting a +rule or it measures one row's height twice. + +**Size: XS.** + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 54600c3..285043a 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -33,8 +33,10 @@ #include #include #include +#include #include #include +#include #include #include @@ -106,7 +108,26 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) tr("Matches") }); m_list->setRootIsDecorated(false); m_list->setUniformRowHeights(true); - layout->addWidget(m_list, 1); + + // The list and the editor go in a splitter, and the editor's own widget + // holds the form. A plain QVBoxLayout gave the list stretch 1 and still + // let a rule with eight condition rows squeeze it to about one visible + // row: a stretch factor only shares out space ABOVE each widget's + // minimum, and the form's grew with every row. Measured before the fix, + // the editor asked for 120px with one row and 414px with eight. + auto *editor = new QWidget(this); + auto *editorLayout = new QVBoxLayout(editor); + editorLayout->setContentsMargins(0, 0, 0, 0); + + m_splitter = new QSplitter(Qt::Vertical, this); + m_splitter->addWidget(m_list); + m_splitter->addWidget(editor); + // Neither pane collapses to nothing by dragging the handle past the end, + // which would hide the thing the user was trying to make room for. + m_splitter->setChildrenCollapsible(false); + m_splitter->setStretchFactor(0, 1); + m_splitter->setStretchFactor(1, 0); + layout->addWidget(m_splitter, 1); auto *form = new QFormLayout; m_id = new QLineEdit(this); @@ -155,7 +176,19 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) m_addExclusion = new QPushButton(tr("Add e&xclusion"), m_builder); builderLayout->addWidget(m_addExclusion, 0, Qt::AlignLeft); - form->addRow(tr("Match"), m_builder); + // The rows scroll rather than growing without bound. The splitter alone + // fixes the squeeze, but only until the user drags the handle down; this + // caps what the editor can ever demand, so a rule with thirty senders + // stays as workable as one with two. + m_builderScroll = new QScrollArea(this); + m_builderScroll->setWidget(m_builder); + m_builderScroll->setWidgetResizable(true); + m_builderScroll->setFrameShape(QFrame::NoFrame); + m_builderScroll->setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff); + // Roughly four rows. Enough that the common rule needs no scrolling at + // all, small enough that the list keeps most of the window. + m_builderScroll->setMaximumHeight(190); + form->addRow(tr("Match"), m_builderScroll); // The toggle sits with the QUERY line, not inside m_builder, because // switching to text mode HIDES m_builder. A checkbox parented there @@ -187,7 +220,7 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) }); form->addRow(tr("Note"), m_note); - layout->addLayout(form); + editorLayout->addLayout(form); auto *buttons = new QHBoxLayout; auto *addButton = new QPushButton(tr("&New"), this); @@ -273,6 +306,11 @@ void TagRulesDialog::restoreUiState() if (!geometry.isEmpty()) restoreGeometry(geometry); + const QByteArray splitter = + state.value(QStringLiteral("tagrules/splitter")).toByteArray(); + if (!splitter.isEmpty()) + m_splitter->restoreState(splitter); + const QByteArray header = state.value(QStringLiteral("tagrules/header")).toByteArray(); if (!header.isEmpty()) { @@ -294,6 +332,8 @@ void TagRulesDialog::saveUiState() state.setValue(QStringLiteral("tagrules/geometry"), saveGeometry()); state.setValue(QStringLiteral("tagrules/header"), m_list->header()->saveState()); + state.setValue(QStringLiteral("tagrules/splitter"), + m_splitter->saveState()); } /// Saves on the way out, whichever way that is. @@ -314,6 +354,30 @@ void TagRulesDialog::done(int result) QDialog::done(result); } +int TagRulesDialog::heightDemandedBelowListForTest() const +{ + // The builder's PREFERRED height, which is what grows with each condition + // row and what the list ends up paying for. minimumSizeHint is the wrong + // measure and reads 580 either way: a QFormLayout's minimum does not + // track its rows, so a test on it passes against the bug. + // The EDITOR PANE's minimum, which is what the splitter refuses to + // shrink below and therefore what the rule list actually pays. Not the + // builder's size hint: that grows with every row by design and is capped + // by the scroll area rather than reduced. Not minimumSizeHint on the + // dialog either, which does not track form rows at all and reads the + // same whether the bug is present or not. + return m_splitter->widget(1)->minimumSizeHint().height(); +} + +int TagRulesDialog::conditionAreaHeightForTest() const +{ + // The CAP itself, not a qMin against the scroll area's own size hint: a + // QScrollArea reports a small hint whether or not it is capped, and an + // uncapped maximumHeight is QWIDGETSIZE_MAX, so qMin picked the hint and + // the assertion passed with the cap removed. + return m_builderScroll->maximumHeight(); +} + int TagRulesDialog::columnWidthForTest(int column) const { return m_list->columnWidth(column); @@ -434,7 +498,7 @@ void TagRulesDialog::onSelectionChanged() const QSignalBlocker blockTextMode(m_textMode); m_textMode->setChecked(!m_loadedQuery.parsed); } - m_builder->setVisible(m_loadedQuery.parsed); + m_builderScroll->setVisible(m_loadedQuery.parsed); m_query->setReadOnly(m_loadedQuery.parsed); if (m_loadedQuery.parsed) @@ -818,7 +882,7 @@ void TagRulesDialog::setTextMode(bool on) // Show what the rows currently mean, then hand the string over. if (m_loadedQuery.parsed) m_query->setText(currentQueryFromRows().compile()); - m_builder->setVisible(false); + m_builderScroll->setVisible(false); m_query->setReadOnly(false); return; } @@ -847,7 +911,7 @@ void TagRulesDialog::setTextMode(bool on) m_reloading = wasReloading; m_loadedQuery = parsed; - m_builder->setVisible(true); + m_builderScroll->setVisible(true); m_query->setReadOnly(true); // The refusal above writes into the same label the load warnings use, so diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index bdcaff9..de35a55 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -31,6 +31,8 @@ class QLineEdit; class QPlainTextEdit; class QPushButton; class QRadioButton; +class QScrollArea; +class QSplitter; class QSpinBox; class QTreeWidget; class QTreeWidgetItem; @@ -107,6 +109,26 @@ public: /// the width the user had dragged. void reloadListForTest(); + /// The height the condition-row editor asks for. This is what squeezed + /// the rule list: a stretch factor only shares out space ABOVE each + /// widget's minimum, so every row added here came out of the list. + /// Measured at 120px for one row and 414px for eight before the scroll + /// area capped it. + /// + /// Asserted on instead of the list's rendered height because the + /// offscreen platform does not honour a window size, so the rendered + /// height there is not evidence of anything (see CLAUDE.md). Note the + /// size HINT, not minimumSizeHint: a QFormLayout's minimum does not track + /// its rows and reads the same either way, which passed against the bug. + int heightDemandedBelowListForTest() const; + + /// How tall the condition-row area may become. The scroll area caps it; + /// without the cap the rows grow without bound and a long rule fills the + /// window again, scrolling instead of squeezing. Asserted separately + /// because removing the cap leaves heightDemandedBelowListForTest + /// unchanged, so that measure alone does not cover it. + int conditionAreaHeightForTest() const; + signals: /// Asks the owner to run countQueries() through the worker. void countsRequested(); @@ -211,6 +233,14 @@ private: QRadioButton *m_matchAny = nullptr; QCheckBox *m_textMode = nullptr; QWidget *m_builder = nullptr; + /// Scrolls the condition rows, so a rule with many of them cannot grow + /// the editor without bound. Shown and hidden in place of m_builder for + /// text mode: hiding the inner widget would leave an empty scroll area. + QScrollArea *m_builderScroll = nullptr; + /// Divides the rule list from the editor. The list had stretch 1 and was + /// still squeezed, because a stretch factor only shares out space above + /// each widget's minimum and the form's grew with every condition row. + QSplitter *m_splitter = nullptr; QVBoxLayout *m_rowsLayout = nullptr; QVBoxLayout *m_exclusionsLayout = nullptr; QLabel *m_exclusionsHeader = nullptr; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 8ca7671..268c41f 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -54,6 +54,7 @@ private slots: void theWindowSizeAndColumnWidthsSurviveAReopen(); void theWindowSizeIsSavedOnEveryWayOutOfTheDialog(); void aReloadDoesNotDiscardARestoredColumnWidth(); + void manyConditionRowsDoNotSqueezeTheRuleList(); private: QString writeRules(const QString &json); @@ -321,6 +322,11 @@ void TestTagRules::switchingRulesDoesNotLeakRowsBetweenThem() // And back, to prove the first rule was not overwritten by loading the // second. dialog.selectRuleForTest(0); + // The row widgets are created during the select and their size hints are + // not valid until the layout has run, which needs the event loop: without + // this the builder reports the same height for one row and for eight, and + // the test passes against the bug. + QCoreApplication::processEvents(); QCOMPARE(dialog.rowCountForTest(), 1); QCOMPARE(dialog.queryLineForTest(), QStringLiteral("from:one.example.org")); } @@ -795,5 +801,71 @@ void TestTagRules::aReloadDoesNotDiscardARestoredColumnWidth() qputenv("XDG_STATE_HOME", previousState); } +void TestTagRules::manyConditionRowsDoNotSqueezeTheRuleList() +{ + // A rule with eight senders left the rule list showing about one and a + // half rows: the list had stretch 1, but a stretch factor only shares out + // space ABOVE each widget's minimum, and the form below it has no ceiling, + // so every condition row added to the minimum the list had to give up. + // + // Measured as the height the layout demands below the list. A rule with + // many rows must not demand materially more than a rule with one; what it + // needs beyond that belongs in the builder's own scroll area. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + 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": "one", "query": "from:a.example.org", + "add": ["x"], "stage": 50, "enabled": true}, + {"id": "many", "query": + "from:a.example.org or from:b.example.org or from:c.example.org or from:d.example.org or from:e.example.org or from:f.example.org or from:g.example.org or from:h.example.org", + "add": ["y"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + dialog.show(); + + dialog.selectRuleForTest(0); + // The row widgets are created during the select and their size hints are + // not valid until the layout has run, which needs the event loop: without + // this the builder reports the same height for one row and for eight, and + // the test passes against the bug. + QCoreApplication::processEvents(); + QCOMPARE(dialog.rowCountForTest(), 1); + const int withOneRow = dialog.heightDemandedBelowListForTest(); + + dialog.selectRuleForTest(1); + QCoreApplication::processEvents(); + // Guard: the fixture must actually produce the many-row case, or this + // test passes by measuring the same rule twice. + QCOMPARE(dialog.rowCountForTest(), 8); + const int withEightRows = dialog.heightDemandedBelowListForTest(); + + // Seven extra rows at roughly 30px each would be over 200px of growth. + // A small increase is fine (the scroll area still has a minimum), a + // proportional one is the bug. + QVERIFY2(withEightRows - withOneRow < 100, + qPrintable(QStringLiteral("one row demands %1, eight demand %2") + .arg(withOneRow).arg(withEightRows))); + + // And the rows are CAPPED, not merely allowed to grow inside a scroll + // area that has no ceiling. Asserted separately because removing the cap + // leaves the assertion above green: the editor's minimum stays flat + // either way, so only the visible height of the row area distinguishes + // them. Without a cap a thirty-sender rule fills the window again, this + // time scrolling instead of squeezing. + QVERIFY2(dialog.conditionAreaHeightForTest() <= 200, + qPrintable(QStringLiteral("condition area is %1px tall") + .arg(dialog.conditionAreaHeightForTest()))); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3 From b8de7ce746718c7193e40b0f70cd3e2178f4ed8a Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 17:00:55 +0200 Subject: feat(rules): preview a rule's mail in the thread list Item 77. The dialog could say how many messages a rule matched and not which ones. A Preview in list button now runs the selected rule's query in the main window; the dialog stays open, since comparing the rule against its results is the point. Two constraints from the backlog entry, both now asserted and both mutation-checked. The query runs exactly as stored, with no tag:new and no wrapping parentheses. The post-new hook adds those when it applies a rule, and a preview that copied them would match nothing outside a sync window, since tag:new is set only on mail that has just arrived. The account selector is cleared first. runQuery() wraps the bar's text in the selected account's scope, and a rule query usually names its own path already, so previewing one with an account selected would scope it twice and show an empty list, which reads as "this rule collects no mail". The second mutation only fails once the test's config has an account to select: with the default empty config the selector sits on "All accounts" anyway, and asserting that a preview leaves it there passed against the mutation. Recorded in the test. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 ++ .../plans/2026-08-03-post-0.1.0-usability.md | 14 +++- src/mainwindow.cpp | 43 +++++++++++++ src/mainwindow.h | 26 ++++++++ src/tagrulesdialog.cpp | 24 +++++++ src/tagrulesdialog.h | 14 ++++ tests/test_tagrules.cpp | 74 ++++++++++++++++++++++ 7 files changed, 200 insertions(+), 1 deletion(-) (limited to 'CHANGELOG.md') diff --git a/CHANGELOG.md b/CHANGELOG.md index e46584a..901734d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,12 @@ point at which they are stable. 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. +- A **Preview in list** button in the tagging rules dialog runs the selected + rule's query in the main window, so you can see which mail a rule collects + rather than only how many messages it matches. The dialog stays open. The + query runs exactly as stored, without the `tag:new` scope the hook adds, and + the account selector is cleared first, since a rule query that names its own + folder would otherwise be scoped twice and match nothing. - The rule list and the rule editor are now divided by a draggable splitter, and the condition rows scroll instead of growing without limit. A rule with eight senders used to squeeze the list to about one visible row, since 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 110938d..eec544e 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 @@ -133,7 +133,7 @@ taking that too literally. | 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 | **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 | +| 77 | No way to see what a rule would collect, in the thread list | workflow | S | **done** 2026-08-13 on `rule-builder`, unreleased | | 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 | | 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13 on `rule-builder`, unreleased. Follows item 76 | | 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 | @@ -4891,6 +4891,18 @@ means the preview discards whatever thread load was in flight. **Size: S.** +**Done 2026-08-13** on `rule-builder`, unreleased. A **Preview in list** +button emits `previewRequested(query)`; `MainWindow::onRulePreviewRequested` +clears the account selector, puts the query in the bar and runs it, then +raises itself. The dialog stays open, which is the point. + +Both constraints above became assertions, and BOTH mutations were needed: a +test that emitted the hook's `tag:new and (...)` wrapping fails, and one that +skips the account reset fails. The second only bites once the test config +actually has an account to select, since the default empty config leaves the +selector on "All accounts" already and the assertion passed against the +mutation until that was fixed. + ## 78. No way to build a rule from something visible in a message **Observed.** The user would like to select an address or another piece of a diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index e8b4dda..e2df6de 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1324,6 +1324,9 @@ void MainWindow::showTagRulesDialog() // setFolders refills the rows that exist by then. QMetaObject::invokeMethod(m_worker, "requestFolders", Qt::QueuedConnection); + connect(dialog, &TagRulesDialog::previewRequested, + this, &MainWindow::onRulePreviewRequested); + connect(dialog, &TagRulesDialog::countsRequested, this, [this, dialog]() { QMetaObject::invokeMethod( m_worker, "requestMessageCounts", Qt::QueuedConnection, @@ -1570,6 +1573,46 @@ void MainWindow::onCountsReady(const QVector &counts, quint64 generation) m_messageView->showPlaceholder(placeholderHelpers()); } +QString MainWindow::queryTextForTesting() const +{ + return m_queryEdit->text(); +} + +QString MainWindow::selectedAccountForTesting() const +{ + return m_accountBox->currentData().toString(); +} + +void MainWindow::selectAccountForTesting(const QString &key) +{ + const int index = m_accountBox->findData(key); + if (index >= 0) + m_accountBox->setCurrentIndex(index); +} + +void MainWindow::onRulePreviewRequested(const QString &query) +{ + // Unscoped, deliberately. runQuery() wraps the bar's text in the selected + // account's scope, and a rule query usually names its own path already + // (path:"work/**" is what every account rule looks like), so previewing + // one with an account selected would scope it twice and match nothing. + // That reads as "this rule collects no mail", which is the opposite of + // what the preview is for. + m_accountBox->setCurrentIndex(0); + + // Through the query bar, like onPlaceholderQueryRequested: the bar then + // shows what is on screen and the user can edit the rule's query there + // before deciding to change the rule itself. + m_queryEdit->setText(query); + runCurrentQuery(); + + // The dialog is a separate window and may be covering this one or sitting + // beside it. Raising makes the result visible either way, and the dialog + // stays open so the two can be compared. + raise(); + activateWindow(); +} + void MainWindow::onPlaceholderQueryRequested(const QString &query) { // Through the query bar rather than straight to the worker, so the bar diff --git a/src/mainwindow.h b/src/mainwindow.h index adf63b0..18fbbb4 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -185,6 +185,27 @@ public: /// The generation the next counts reply must carry to be accepted. quint64 countsGenerationForTesting() const { return m_countsGeneration; } + /// The query bar's text, and the account the selector is scoped to + /// (empty for "All accounts"). Both are what a rule preview writes: the + /// bar so the user can see and edit what ran, and the selector because + /// runQuery() wraps the text in the selected account's scope, which would + /// double-scope a rule query that already names its own path. + QString queryTextForTesting() const; + QString selectedAccountForTesting() const; + + /// Scopes the view to one account, as choosing it in the selector does. + /// A test for the rule preview needs this: with no account selected the + /// box already sits at "All accounts", so asserting that a preview leaves + /// it there passes whether or not the preview clears it. + void selectAccountForTesting(const QString &key); + + /// Runs a rule preview without the dialog, which the offscreen platform + /// cannot click a button in. + void previewRuleQueryForTesting(const QString &query) + { + onRulePreviewRequested(query); + } + protected: void closeEvent(QCloseEvent *event) override; @@ -293,6 +314,11 @@ private slots: /// Runs a query the user clicked on the placeholder pane. void onPlaceholderQueryRequested(const QString &query); + /// Runs one tagging rule's query in the thread list, so the user can see + /// which mail it collects. The rules dialog stays open; the point is to + /// compare the rule against its results. + void onRulePreviewRequested(const QString &query); + /// Opens the auto-tagging rules editor, or raises the one already open. void showTagRulesDialog(); diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 285043a..4fca971 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -234,6 +234,11 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) buttons->addWidget(copyButton); buttons->addWidget(deleteButton); buttons->addStretch(); + m_previewButton = new QPushButton(tr("&Preview in list"), this); + m_previewButton->setToolTip( + tr("Run this rule's query in the main window, to see which mail it " + "collects. This does not tag anything.")); + buttons->addWidget(m_previewButton); buttons->addWidget(refreshButton); layout->addLayout(buttons); @@ -251,6 +256,8 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) this, &TagRulesDialog::onCopyRule); connect(deleteButton, &QPushButton::clicked, this, &TagRulesDialog::onDeleteRule); + connect(m_previewButton, &QPushButton::clicked, + this, &TagRulesDialog::previewForTest); connect(refreshButton, &QPushButton::clicked, this, &TagRulesDialog::countsRequested); connect(box, &QDialogButtonBox::accepted, @@ -369,6 +376,23 @@ int TagRulesDialog::heightDemandedBelowListForTest() const return m_splitter->widget(1)->minimumSizeHint().height(); } +void TagRulesDialog::previewForTest() +{ + // Flush any half-typed edit first, so previewing shows what the rule + // says NOW rather than what it said when the row was selected. + applyEditsToCurrentRule(); + + const int index = currentIndex(); + if (index < 0 || index >= m_working.size()) + return; + + const QString query = m_working.at(index).query; + if (query.isEmpty()) + return; + + emit previewRequested(query); +} + int TagRulesDialog::conditionAreaHeightForTest() const { // The CAP itself, not a qMin against the scroll area's own size hint: a diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index de35a55..e8979fe 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -109,6 +109,10 @@ public: /// the width the user had dragged. void reloadListForTest(); + /// Presses Preview, which is otherwise reachable only through a click on + /// a button whose geometry the offscreen platform does not guarantee. + void previewForTest(); + /// The height the condition-row editor asks for. This is what squeezed /// the rule list: a stretch factor only shares out space ABOVE each /// widget's minimum, so every row added here came out of the list. @@ -133,6 +137,15 @@ signals: /// Asks the owner to run countQueries() through the worker. void countsRequested(); + /// Asks the owner to run one rule's query in the main window, so the user + /// can see WHICH mail a rule collects rather than how much. + /// + /// The query goes out exactly as stored: no `tag:new`, and no wrapping + /// parentheses. The post-new hook adds both when it applies a rule, and a + /// preview that copied them would match nothing outside a sync window, + /// since `tag:new` is set only on mail that has just arrived. + void previewRequested(const QString &query); + public slots: /// Corpus counts, positionally paired with countQueries(). void setCounts(const QVector &counts); @@ -241,6 +254,7 @@ private: /// still squeezed, because a stretch factor only shares out space above /// each widget's minimum and the form's grew with every condition row. QSplitter *m_splitter = nullptr; + QPushButton *m_previewButton = nullptr; QVBoxLayout *m_rowsLayout = nullptr; QVBoxLayout *m_exclusionsLayout = nullptr; QLabel *m_exclusionsHeader = nullptr; diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 268c41f..ed3a06d 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -20,6 +20,7 @@ #include #include +#include "config.h" #include "mainwindow.h" #include "rulequery.h" #include "tagrules.h" @@ -55,6 +56,8 @@ private slots: void theWindowSizeIsSavedOnEveryWayOutOfTheDialog(); void aReloadDoesNotDiscardARestoredColumnWidth(); void manyConditionRowsDoNotSqueezeTheRuleList(); + void previewEmitsTheRuleQueryAsStored(); + void previewClearsTheAccountScope(); private: QString writeRules(const QString &json); @@ -867,5 +870,76 @@ void TestTagRules::manyConditionRowsDoNotSqueezeTheRuleList() .arg(dialog.conditionAreaHeightForTest()))); } +void TestTagRules::previewEmitsTheRuleQueryAsStored() +{ + // The query goes out EXACTLY as stored: no tag:new, no wrapping + // parentheses. The hook adds both when it applies a rule, and a preview + // that copied it would show nothing at all outside a sync window, since + // tag:new is only set on mail that has just arrived. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + 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": "promo", "query": "from:a.example.org or from:b.example.org", + "add": ["promo"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + QSignalSpy spy(&dialog, &TagRulesDialog::previewRequested); + + dialog.selectRuleForTest(0); + dialog.previewForTest(); + + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.first().at(0).toString(), + QStringLiteral("from:a.example.org or from:b.example.org")); +} + +void TestTagRules::previewClearsTheAccountScope() +{ + // runQuery() wraps the bar's text in the selected account's scope. A rule + // query usually names its own path already (path:"work/**"), so previewing + // one while an account is selected would scope it twice and show nothing, + // which reads as "the rule matches no mail" rather than as a UI fault. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + + // An account that can actually BE selected. With the default empty config + // the selector holds only "All accounts", so it sits at index 0 already + // and the assertion below passes whether or not the preview clears it: + // measured, the mutation removing the reset survived until this config + // was added. + const QString confPath = configHome.filePath(QStringLiteral("q.conf")); + QFile conf(confPath); + QVERIFY(conf.open(QIODevice::WriteOnly | QIODevice::Text)); + conf.write("[account.work]\nmaildir=work-mail\n"); + conf.close(); + + Config config; + config.load(confPath); + QCOMPARE(config.accounts().size(), 1); + + MainWindow window(config); + window.selectAccountForTesting(QStringLiteral("work")); + QCOMPARE(window.selectedAccountForTesting(), QStringLiteral("work")); + + window.previewRuleQueryForTesting(QStringLiteral("from:a.example.org")); + + QCOMPARE(window.queryTextForTesting(), + QStringLiteral("from:a.example.org")); + QVERIFY2(window.selectedAccountForTesting().isEmpty(), + "a preview must run unscoped, or an account-scoped rule query " + "is wrapped twice and matches nothing"); +} + QTEST_MAIN(TestTagRules) #include "test_tagrules.moc" -- cgit v1.2.3