diff options
| -rw-r--r-- | CHANGELOG.md | 40 | ||||
| -rw-r--r-- | CLAUDE.md | 30 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 161 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-13-rule-builder.md | 39 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-08-13-rule-builder-design.md | 38 | ||||
| -rw-r--r-- | src/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 61 | ||||
| -rw-r--r-- | src/mainwindow.h | 26 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 37 | ||||
| -rw-r--r-- | src/notmuchworker.h | 17 | ||||
| -rw-r--r-- | src/rulequery.cpp | 414 | ||||
| -rw-r--r-- | src/rulequery.h | 65 | ||||
| -rw-r--r-- | src/tagrulesdialog.cpp | 689 | ||||
| -rw-r--r-- | src/tagrulesdialog.h | 164 | ||||
| -rw-r--r-- | tests/CMakeLists.txt | 1 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 77 | ||||
| -rw-r--r-- | tests/test_rulequery.cpp | 415 | ||||
| -rw-r--r-- | tests/test_tagrules.cpp | 700 |
18 files changed, 2946 insertions, 29 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index fa9e7cf..901734d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,46 @@ 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 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. +- 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 + 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 + sizes the window itself, so there it opens at whatever size the tile gives + it. + +### 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 @@ -317,6 +317,36 @@ 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. + +**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 1216542..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 @@ -131,10 +131,12 @@ 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` | -| 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 | +| 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 | **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 | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -4763,6 +4765,55 @@ the test asserts on the saved value rather than on the resulting frame. **Size: S.** +**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", +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. + +**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 +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 @@ -4840,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 @@ -4866,6 +4929,98 @@ in CLAUDE.md. **Size: M.** +## 79. Opening the rules dialog and saving destroys the first rule + +**Observed.** Open Tagging rules, press Save, change nothing. The first rule +in the list loses its query and its tags. It then vanishes entirely the next +time anything reads the file, because a rule with an empty query is dropped +as malformed on load. + +Found while building item 76, by a test written to catch a different problem. +Reproduced against the released tag rather than the branch, with a throwaway +worktree at 9585674 and a two-rule fixture: after constructing the dialog and +calling its save path, the store held one rule instead of two. + +**Cause.** `TagRulesDialog::onSelectionChanged` blocks signals for `m_note` +only. Two lines later, `m_enabled->setChecked(rule.enabled)` emits `toggled`, +which is connected to `applyEditsToCurrentRule()`. That handler writes every +field of the current rule from the widgets, and it runs BEFORE +`m_query->setText(rule.query)` has filled the query widget, so it writes the +previous rule's text. On the first open there is no previous rule and the +widgets are empty, so rule 0 gets an empty query and an empty tag list. +`TagRules::load` then drops it (`src/tagrules.cpp:150`). + +The existing comment above the `QSignalBlocker` shows the hazard was known for +`m_note` and simply not extended to `m_enabled`. A blocker per widget is the +wrong shape: the whole load needs one guard. + +**Fix.** Raise `m_reloading` for the duration of `onSelectionChanged` and +restore it afterwards, replacing the single-widget blocker. `m_reloading` +already exists for exactly this class of problem and +`applyEditsToCurrentRule()` already honours it. Also take the rule by value +rather than by const reference: the reference points into `m_working`, which +the handler mutates, so it could be read back half overwritten. + +Fixed on the `rule-builder` branch as part of item 76, with +`switchingRulesDoesNotLeakRowsBetweenThem` in `test_tagrules` as the +regression test. It fails against the unfixed code. + +**Damage in the field, and the repair.** The live rules file had exactly one +casualty: the account rule sitting first in the list, with its `query`, its +`add` and its `note` all empty while every sibling account rule was intact. + +**The note was missed on the first pass of the repair**, because the shell +backup was read for the tagging command and the note comes from the comment +block ABOVE it, which the migration had given to all five account rules +alike. The user spotted the gap. `applyEditsToCurrentRule()` writes every +field, so every field is equally exposed: repair work here must check the +whole rule, not the fields that first drew attention. Restored from the four +siblings, which carry byte-identical notes. +Restored from `post-new.shell-backup`, which item 44's migration kept, and +verified by loading the file through mailctl's own reader: 17 rules, correct +scoping. The rule had stopped tagging, but only one message had arrived in the +meantime (14968 of 14969 in that account still carried the tag); it was tagged +by hand and the account is now complete. + +**Constraints.** The user chose to leave the fix on the branch rather than cut +a patch release, so 0.16.0 in the field still has it. Do not open that dialog +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/docs/superpowers/plans/2026-08-13-rule-builder.md b/docs/superpowers/plans/2026-08-13-rule-builder.md index fb39cca..96fddc6 100644 --- a/docs/superpowers/plans/2026-08-13-rule-builder.md +++ b/docs/superpowers/plans/2026-08-13-rule-builder.md @@ -376,9 +376,20 @@ bool needsQuotes(const RuleTerm &term) { if (term.field == RuleTerm::Folder) return true; - if (term.op == RuleTerm::Is || term.op == RuleTerm::IsNot) + if (term.value.contains(QLatin1Char(' '))) return true; - return term.value.contains(QLatin1Char(' ')); + // Is/IsNot means an exact phrase, and only the free-text fields need + // quotes to express one. A tag or an attachment name is a single bare + // token to notmuch, which reads `tag:inbox` and `tag:"inbox"` identically + // (both count 5322 against the live index). Quoting them would therefore + // change the stored string without changing what it matches, and this + // type's whole contract is that an unedited rule compiles back byte for + // byte. + if (term.op == RuleTerm::Is || term.op == RuleTerm::IsNot) { + return term.field == RuleTerm::From || term.field == RuleTerm::To + || term.field == RuleTerm::Cc || term.field == RuleTerm::Subject; + } + return false; } QString compileTerm(const RuleTerm &term) @@ -793,6 +804,9 @@ bool parseTerm(const QString &token, RuleTerm *out) return !value.isEmpty(); } + // Tag and Attachment compile unquoted (see needsQuotes in Task 2), so + // their operator must not be inferred from the quoting: reading a quoted + // tag back as Is would compile it unquoted and change the stored string. if (field == RuleTerm::Attachment) out->op = RuleTerm::Has; else if (field == RuleTerm::Tag) @@ -1099,6 +1113,11 @@ void TestRuleQuery::anUnrepresentableQueryRejectsWhole() QStringLiteral("date:2026-01-01..2026-02-01"), // two-sided range QStringLiteral("from:a.example.org xor subject:x"), }; + // NOT in this list: `from:((((`. It parses, as a From row whose value is + // the literal text `((((`, and round-trips byte for byte. That is exactly + // what the query means to notmuch, which treats the parens as characters + // to search for rather than as grouping, so the row tells the truth and + // rejecting it would buy nothing. See the test below. for (const QString &query : unrepresentable) { const RuleQuery q = RuleQuery::parse(query); @@ -1112,11 +1131,19 @@ void TestRuleQuery::anUnrepresentableQueryRejectsWhole() void TestRuleQuery::aMalformedQueryIsRejectedNotDiagnosed() { - // notmuch accepts `from:((((` cleanly and matches nothing, so there is no - // failure to observe and a test asserting one fails against correct code. - // The assertion is on OUR rejection only. + // notmuch accepts `from:((((` cleanly and matches nothing: the parens are + // characters it searches for, not grouping. So there is no failure to + // observe, and a test asserting one fails against correct code. + // + // This parser accepts it too, as a From row whose value is that literal + // text, which is what the query actually means. What must hold is the + // round trip, not a rejection: displaying it as a row and compiling it + // back must not alter the stored string. const RuleQuery q = RuleQuery::parse(QStringLiteral("from:((((")); - QVERIFY(!q.parsed); + QVERIFY(q.parsed); + QCOMPARE(q.terms.size(), 1); + QCOMPARE(q.terms.at(0).value, QStringLiteral("((((")); + QCOMPARE(q.compile(), QStringLiteral("from:((((")); } ``` diff --git a/docs/superpowers/specs/2026-08-13-rule-builder-design.md b/docs/superpowers/specs/2026-08-13-rule-builder-design.md index c11ef47..4d6c001 100644 --- a/docs/superpowers/specs/2026-08-13-rule-builder-design.md +++ b/docs/superpowers/specs/2026-08-13-rule-builder-design.md @@ -221,7 +221,12 @@ an empty query and must open in the builder ready to receive a row. Everything else sets `parsed = false`: nested parens beyond that one shape, mixed `and`/`or` without parens, `xor`, an unrecognised prefix (`body:`, `mid:`, -`folder:`), a bare word with no prefix, a `path:` not ending in `/**`. +`folder:`), a bare word with no prefix, a `path:` not ending in `/**`, a +two-sided `date:` range, a trailing operator, an unterminated quote. + +A parenthesis **inside a value** is not a shape at all: `from:((((` is a From +term whose value happens to contain parens, and it parses and round-trips like +any other. Only a parenthesis in grouping position is a shape question. ### The parser is strict, and that is the safety property @@ -270,7 +275,7 @@ The builder replaces the query line edit. Everything else in the form stays. ``` Id [ vendor-receipts ] Stage [ 50 ] [x] Applied on every sync -Match (o) all ( ) any [ ] Edit as text +Match (o) all ( ) any [From v] [contains v] [vendor.example.org ] [+] [-] [From v] [contains v] [vendor.example.net ] [+] [-] But not @@ -282,9 +287,22 @@ Add tags [ vendor, receipts ] Remove tags [ ] Note [ ... ] -Query (from:vendor.example.org or ...) and not subject:receipt [Count matches] +Query (from:vendor.example.org or ...) and not subject:receipt + [ ] Edit as text ``` +**The "Edit as text" toggle belongs to the QUERY row, not to the match row.** +An earlier draft of this sketch put it beside the all/any radios, which is +where it reads best and is also wrong: switching to text mode hides the +builder, and a checkbox living inside the builder disappears with it, leaving +no way back except closing the dialog. That shipped and a hand test found it +within minutes. The query row is visible in both modes, so a toggle there is +always reachable. + +The test for this must assert **reachability**, not the checked state. A +hidden checkbox reports its state perfectly well, so a state assertion passes +against the broken layout. + **The query line stays visible in builder mode, read-only.** It is what ships to the hook, and watching it update as rows change is what makes the builder trustworthy rather than a black box. In text mode the same widget becomes @@ -334,10 +352,16 @@ terms and tests exactly the same thing. **Rejection tests**, which carry the safety property. Queries that must set `parsed = false` and must not partially parse: nested `or` inside `or`, mixed -`and`/`or` without parens, `body:foo`, a bare word, a `path:` without `/**`, and -`from:((((`. That last asserts **our** rejection, never a provoked notmuch -failure: `CLAUDE.md` records twice that notmuch accepts it cleanly, and a test -expecting an error there fails against correct code. +`and`/`or` without parens, `body:foo`, a bare word, a `path:` without `/**`, a +two-sided `date:` range, and a trailing operator. + +**`from:((((` is not among them, and the reason is worth stating.** notmuch +treats those parens as characters to search for rather than as grouping, so the +query is meaningful, matches nothing, and reports no error. This parser accepts +it as a From row whose value is that literal text, which is what it means. The +assertion there is the **round trip**, never a rejection and never a provoked +notmuch failure: `CLAUDE.md` records twice that notmuch accepts it cleanly, and +a test expecting an error fails against correct code. **Compile tests** for every field and operator pair including both negations, and the parenthesisation rule at its boundary: `join == Any` with zero diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 926f6a9..6ae157b 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -23,6 +23,7 @@ add_library(qtmaildir_lib STATIC messageview.cpp mainwindow.cpp querycompleter.cpp + rulequery.cpp ) target_include_directories(qtmaildir_lib diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 331354f..e2df6de 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1313,8 +1313,20 @@ void MainWindow::showTagRulesDialog() auto *dialog = new TagRulesDialog(this); dialog->setAttribute(Qt::WA_DeleteOnClose); + m_tagRulesDialog = dialog; + // The Folder row's dropdown, filled from the Maildir tree on disk rather + // than from config. Config names one subtree per account and nothing + // below it, so the dropdown offered five entries and no way to say Drafts + // or Sent, which is a folder a rule wants to target as often as a whole + // account. The answer comes back queued, after the dialog is already up; + // 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, @@ -1414,6 +1426,15 @@ void MainWindow::wireWorker() connect(m_worker, &NotmuchWorker::messageCountsReady, this, &MainWindow::onRuleCountsReady); + // The rules dialog is the only consumer, and it may have been closed while + // the scan was in flight. No generation counter: the tree on disk does not + // change under a query, so a late answer is still the right one. + connect(m_worker, &NotmuchWorker::foldersReady, this, + [this](const QStringList &folders) { + if (m_tagRulesDialog) + m_tagRulesDialog->setFolders(folders); + }); + // A confirmed write clears the pending revert: without this, a later // unrelated error would roll back a change that actually succeeded. connect(m_worker, &NotmuchWorker::tagsApplied, @@ -1552,6 +1573,46 @@ void MainWindow::onCountsReady(const QVector<int> &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/notmuchworker.cpp b/src/notmuchworker.cpp index 94fb79c..6aae397 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -20,6 +20,9 @@ #include <notmuch.h> +#include <QDir> +#include <QDirIterator> +#include <QFileInfo> #include <QSet> #include <cstdlib> @@ -678,3 +681,37 @@ void NotmuchWorker::requestMessageCounts(const QStringList &queries, emit messageCountsReady(counts, generation); } + +void NotmuchWorker::requestFolders() +{ + if (!openReadOnly()) + return; + + const QString root = QString::fromUtf8(notmuch_database_get_path(m_db)); + if (root.isEmpty()) { + emit errorOccurred( + QStringLiteral("notmuch reports no database path.")); + return; + } + + // A Maildir folder is a directory holding cur/. Testing for that rather + // than listing every directory keeps the plumbing (cur, new, tmp) and an + // account's container directory out of the list; neither is somewhere mail + // is filed. Hidden directories are skipped, which is what excludes + // .notmuch itself. + QStringList folders; + QDirIterator it(root, QDir::Dirs | QDir::NoDotAndDotDot, + QDirIterator::Subdirectories); + const QDir rootDir(root); + while (it.hasNext()) { + const QString path = it.next(); + if (!QFileInfo::exists(path + QStringLiteral("/cur"))) + continue; + folders.append(rootDir.relativeFilePath(path)); + } + + // Sorted, so the dropdown keeps one order across openings. QDirIterator + // walks in filesystem order, which is neither stable nor alphabetical. + folders.sort(); + emit foldersReady(folders); +} diff --git a/src/notmuchworker.h b/src/notmuchworker.h index b3fbed3..9736ab8 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -151,6 +151,18 @@ public slots: /// called when the dialog is opened and never on a timer. void requestDatabaseStats(quint64 generation); + /// Every Maildir folder under the database root, as paths relative to it. + /// + /// From the DISK, not from the index: a folder mbsync created and nothing + /// has landed in yet is still a folder a tagging rule may target, and one + /// derived from indexed message paths would not offer it. + /// + /// Here rather than in MainWindow because the database root is + /// notmuch's `database.path` and this class owns the only handle that can + /// answer for it. Duplicating the path into config is exactly the second + /// source of truth the design refuses. + void requestFolders(); + signals: void threadsReady(const QVector<ThreadSummary> &threads, quint64 generation); void queryFinished(int totalThreads, quint64 generation); @@ -175,6 +187,11 @@ signals: /// renders as unknown rather than as zero. void databaseStatsReady(const DatabaseStats &stats, quint64 generation); + /// Maildir folders relative to the database root, sorted. No generation: + /// the tree on disk does not change under a query, and the one consumer + /// asks once when its dialog opens. + void foldersReady(const QStringList &folders); + void errorOccurred(const QString &message); private: diff --git a/src/rulequery.cpp b/src/rulequery.cpp new file mode 100644 index 0000000..31996fa --- /dev/null +++ b/src/rulequery.cpp @@ -0,0 +1,414 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "rulequery.h" + +#include <QPair> +#include <QStringList> +#include <QVector> + +namespace { + +/// The notmuch prefix each field compiles to. Wire format, never translated. +QString prefixFor(RuleTerm::Field field) +{ + switch (field) { + case RuleTerm::From: return QStringLiteral("from"); + case RuleTerm::To: return QStringLiteral("to"); + case RuleTerm::Cc: return QStringLiteral("cc"); + case RuleTerm::Subject: return QStringLiteral("subject"); + case RuleTerm::Tag: return QStringLiteral("tag"); + case RuleTerm::Folder: return QStringLiteral("path"); + case RuleTerm::Attachment: return QStringLiteral("attachment"); + case RuleTerm::Date: return QStringLiteral("date"); + } + return QString(); +} + +bool isNegated(RuleTerm::Op op) +{ + return op == RuleTerm::ContainsNot || op == RuleTerm::IsNot + || op == RuleTerm::HasNot; +} + +/// Quoted when the operator asks for an exact phrase, and ALWAYS when the +/// value holds a space: unquoted, the space ends the term and the remainder +/// becomes a bare word, which widens the rule instead of breaking it. +bool needsQuotes(const RuleTerm &term) +{ + if (term.field == RuleTerm::Folder) + return true; + if (term.value.contains(QLatin1Char(' '))) + return true; + // Is/IsNot means an exact phrase, and only the free-text fields need + // quotes to express one. A tag or an attachment name is a single bare + // token to notmuch, which reads `tag:inbox` and `tag:"inbox"` identically + // (both count 5322 against the live index). Quoting them would therefore + // change the stored string without changing what it matches, and this + // type's whole contract is that an unedited rule compiles back byte for + // byte. + if (term.op == RuleTerm::Is || term.op == RuleTerm::IsNot) { + return term.field == RuleTerm::From || term.field == RuleTerm::To + || term.field == RuleTerm::Cc || term.field == RuleTerm::Subject; + } + return false; +} + +QString compileTerm(const RuleTerm &term) +{ + QString value = term.value; + if (term.field == RuleTerm::Folder) + value += QStringLiteral("/**"); + + QString body; + if (term.field == RuleTerm::Date) { + body = prefixFor(term.field) + QLatin1Char(':') + + (term.op == RuleTerm::Before + ? QStringLiteral("..") + value + : value + QStringLiteral("..")); + } else if (needsQuotes(term)) { + body = prefixFor(term.field) + QStringLiteral(":\"") + value + + QLatin1Char('"'); + } else { + body = prefixFor(term.field) + QLatin1Char(':') + value; + } + + return isNegated(term.op) ? QStringLiteral("not ") + body : body; +} + +/// Splits on whitespace, keeping a double-quoted run as one token. Returns +/// false when a quote is left open, which is a query this builder will not +/// represent. +bool tokenise(const QString &query, QStringList *out) +{ + QString current; + bool inQuotes = false; + bool has = false; + + for (int i = 0; i < query.size(); ++i) { + const QChar c = query.at(i); + if (c == QLatin1Char('"')) { + inQuotes = !inQuotes; + current += c; + has = true; + } else if (!inQuotes && c.isSpace()) { + if (has) { + out->append(current); + current.clear(); + has = false; + } + } else { + current += c; + has = true; + } + } + + if (inQuotes) + return false; + if (has) + out->append(current); + return true; +} + +bool fieldForPrefix(const QString &prefix, RuleTerm::Field *out) +{ + static const QVector<QPair<QString, RuleTerm::Field>> table = { + {QStringLiteral("from"), RuleTerm::From}, + {QStringLiteral("to"), RuleTerm::To}, + {QStringLiteral("cc"), RuleTerm::Cc}, + {QStringLiteral("subject"), RuleTerm::Subject}, + {QStringLiteral("tag"), RuleTerm::Tag}, + {QStringLiteral("path"), RuleTerm::Folder}, + {QStringLiteral("attachment"), RuleTerm::Attachment}, + {QStringLiteral("date"), RuleTerm::Date}, + }; + + for (const auto &entry : table) { + if (entry.first == prefix) { + *out = entry.second; + return true; + } + } + return false; +} + +/// Parses ONE token into a term. Returns false for anything this builder does +/// not represent, which is not the same as invalid: notmuch accepts far more +/// than this. +bool parseTerm(const QString &token, RuleTerm *out) +{ + const int colon = token.indexOf(QLatin1Char(':')); + if (colon <= 0) + return false; + + RuleTerm::Field field; + if (!fieldForPrefix(token.left(colon), &field)) + return false; + + QString value = token.mid(colon + 1); + if (value.isEmpty()) + return false; + + bool quoted = false; + if (value.size() >= 2 && value.startsWith(QLatin1Char('"')) + && value.endsWith(QLatin1Char('"'))) { + value = value.mid(1, value.size() - 2); + quoted = true; + } + // A quote anywhere else means a shape this builder does not emit. + if (value.contains(QLatin1Char('"'))) + return false; + + out->field = field; + + if (field == RuleTerm::Date) { + if (value.startsWith(QStringLiteral(".."))) { + out->op = RuleTerm::Before; + out->value = value.mid(2); + } else if (value.endsWith(QStringLiteral(".."))) { + out->op = RuleTerm::After; + out->value = value.chopped(2); + } else { + return false; // A two-sided range is not a row. + } + return !out->value.isEmpty(); + } + + if (field == RuleTerm::Folder) { + // Only the recursive form is representable; a bare path means + // something different to notmuch and must not be silently rewritten. + if (!value.endsWith(QStringLiteral("/**"))) + return false; + value = value.chopped(3); + out->op = RuleTerm::Is; + out->value = value; + return !value.isEmpty(); + } + + // Tag and Attachment compile unquoted (see needsQuotes), so their + // operator must not be inferred from the quoting: reading a quoted tag + // back as a quoting operator would compile it unquoted and change the + // stored string. + if (field == RuleTerm::Attachment) + out->op = RuleTerm::Has; + else if (field == RuleTerm::Tag) + out->op = RuleTerm::Is; + else + out->op = quoted ? RuleTerm::Is : RuleTerm::Contains; + + out->value = value; + return true; +} + +/// Splits `(A or B) and not C and not D` into its group and its remainder. +/// Returns false when the query does not start with a balanced group. +bool splitLeadingGroup(const QString &query, QString *group, QString *rest) +{ + if (!query.startsWith(QLatin1Char('('))) + return false; + + int depth = 0; + bool inQuotes = false; + for (int i = 0; i < query.size(); ++i) { + const QChar c = query.at(i); + if (c == QLatin1Char('"')) + inQuotes = !inQuotes; + if (inQuotes) + continue; + if (c == QLatin1Char('(')) + ++depth; + else if (c == QLatin1Char(')')) { + --depth; + if (depth == 0) { + *group = query.mid(1, i - 1).trimmed(); + *rest = query.mid(i + 1).trimmed(); + return true; + } + } + } + return false; +} + +} // namespace + +bool operator==(const RuleTerm &a, const RuleTerm &b) +{ + return a.field == b.field && a.op == b.op && a.value == b.value; +} + +bool operator==(const RuleQuery &a, const RuleQuery &b) +{ + return a.parsed == b.parsed && a.join == b.join + && a.terms == b.terms && a.exclusions == b.exclusions; +} + +QString RuleQuery::compile() const +{ + if (terms.isEmpty()) + return QString(); + + QStringList parts; + for (const RuleTerm &term : terms) + parts.append(compileTerm(term)); + + const QString glue = join == Any ? QStringLiteral(" or ") + : QStringLiteral(" and "); + QString out = parts.join(glue); + + // An `or` group followed by `and not` must be parenthesised or the `and` + // binds tighter than the `or`: `a or b and not c` is `a or (b and not c)`, + // which matches every `a` whatever the exclusion says. + if (join == Any && !exclusions.isEmpty() && terms.size() > 1) + out = QLatin1Char('(') + out + QLatin1Char(')'); + + for (const RuleTerm &term : exclusions) { + // The block IS the negation, so its rows are stored un-negated and + // the `and not` is applied here. A row stored negated would compile + // to `and not not subject:x`. + out += QStringLiteral(" and not ") + compileTerm(term); + } + + return out; +} + +RuleQuery RuleQuery::parse(const QString &query) +{ + RuleQuery out; + + const QString trimmed = query.trimmed(); + if (trimmed.isEmpty()) { + // An empty query is a rule with no rows yet, not a failure. + out.parsed = true; + return out; + } + + QString group; + QString rest; + if (splitLeadingGroup(trimmed, &group, &rest)) { + // Only one nested shape is representable: an `or` group followed by + // `and not` exclusions. Anything else rejects whole. + if (group.contains(QLatin1Char('('))) + return RuleQuery(); + + const RuleQuery inner = parse(group); + if (!inner.parsed || inner.join != Any || !inner.exclusions.isEmpty()) + return RuleQuery(); + + out.join = Any; + out.terms = inner.terms; + + if (rest.isEmpty()) { + // A group with nothing after it compiles back WITHOUT parens, + // since compile() only adds them when exclusions follow. Round + // trip would break, so this is not representable. + return RuleQuery(); + } + + // The remainder must be nothing but `and not <term>` repetitions. + QStringList tail; + if (!tokenise(rest, &tail)) + return RuleQuery(); + + int i = 0; + while (i < tail.size()) { + if (tail.at(i).compare(QStringLiteral("and"), + Qt::CaseInsensitive) != 0) + return RuleQuery(); + ++i; + if (i >= tail.size() + || tail.at(i).compare(QStringLiteral("not"), + Qt::CaseInsensitive) != 0) + return RuleQuery(); + ++i; + if (i >= tail.size()) + return RuleQuery(); + + RuleTerm term; + if (!parseTerm(tail.at(i), &term)) + return RuleQuery(); + out.exclusions.append(term); + ++i; + } + + out.parsed = true; + return out; + } + + QStringList tokens; + if (!tokenise(trimmed, &tokens)) + return RuleQuery(); + + // Walk the chain: term, operator, term, ... Anything else rejects whole. + bool sawOr = false; + bool sawAnd = false; + int i = 0; + + while (i < tokens.size()) { + bool negated = false; + if (tokens.at(i).compare(QStringLiteral("not"), + Qt::CaseInsensitive) == 0) { + negated = true; + ++i; + if (i >= tokens.size()) + return RuleQuery(); + } + + RuleTerm term; + if (!parseTerm(tokens.at(i), &term)) + return RuleQuery(); + + if (negated) { + // `not date:` has no row form: "not before" is "after", which the + // unnegated operators already express. + if (term.field == RuleTerm::Date) + return RuleQuery(); + // The block IS the negation, so the row is stored un-negated and + // compile() re-applies the `and not`. + out.exclusions.append(term); + } else { + out.terms.append(term); + } + ++i; + + if (i >= tokens.size()) + break; + + const QString glue = tokens.at(i).toLower(); + if (glue == QStringLiteral("and")) { + sawAnd = true; + } else if (glue == QStringLiteral("or")) { + sawOr = true; + } else { + return RuleQuery(); // Not a joining word: unrepresentable. + } + ++i; + if (i >= tokens.size()) + return RuleQuery(); // Trailing operator. + } + + // Mixed and/or without parentheses is ambiguous to a reader and binds in + // a way the rows cannot show. Reject rather than guess. + if (sawAnd && sawOr) + return RuleQuery(); + if (out.terms.isEmpty()) + return RuleQuery(); + + out.join = sawOr ? Any : All; + out.parsed = true; + return out; +} diff --git a/src/rulequery.h b/src/rulequery.h new file mode 100644 index 0000000..9d487d2 --- /dev/null +++ b/src/rulequery.h @@ -0,0 +1,65 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#pragma once + +#include <QList> +#include <QString> + +/// One row in the builder: a field, an operator, and a value. +struct RuleTerm +{ + enum Field { From, To, Cc, Subject, Tag, Folder, Attachment, Date }; + + enum Op { + Contains, ContainsNot, ///< Unquoted term, optionally negated. + Is, IsNot, ///< Quoted phrase, optionally negated. + Has, HasNot, ///< attachment: only. + Before, After ///< date: only. "not before" is "after", so + ///< these carry no negated twin. + }; + + Field field = From; + Op op = Contains; + QString value; +}; + +bool operator==(const RuleTerm &a, const RuleTerm &b); + +/// A tagging rule's query, as rows. +/// +/// The STRING is the stored format, shared with mailctl and executed by the +/// notmuch post-new hook. This type is a view over it, never the store: a +/// query it cannot represent must still open, save and run unchanged. +struct RuleQuery +{ + enum Join { All, Any }; ///< and / or, over the positive terms only. + + Join join = All; + QList<RuleTerm> terms; ///< Positive section. + QList<RuleTerm> exclusions; ///< The "but not" block, joined `and not`. + + /// False when the query cannot be shown as rows. NOT an error and NOT a + /// claim that the query is invalid: the rule opens in text mode. + bool parsed = false; + + static RuleQuery parse(const QString &query); + QString compile() const; +}; + +bool operator==(const RuleQuery &a, const RuleQuery &b); diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 40ee66a..4fca971 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -18,8 +18,12 @@ #include "tagrulesdialog.h" +#include <QButtonGroup> #include <QCheckBox> +#include <QComboBox> #include <QDialogButtonBox> +#include <QDir> +#include <QFileInfo> #include <QFormLayout> #include <QHBoxLayout> #include <QHeaderView> @@ -28,10 +32,16 @@ #include <QMessageBox> #include <QPlainTextEdit> #include <QPushButton> +#include <QRadioButton> +#include <QScrollArea> +#include <QSettings> #include <QSpinBox> +#include <QSplitter> #include <QTreeWidget> #include <QVBoxLayout> +#include "mainwindow.h" + namespace { /// Columns of the rule list. @@ -50,12 +60,28 @@ QStringList splitTags(const QString &text) return out; } +struct FieldEntry { RuleTerm::Field field; const char *label; }; + +const FieldEntry kFields[] = { + {RuleTerm::From, QT_TR_NOOP("From")}, + {RuleTerm::To, QT_TR_NOOP("To")}, + {RuleTerm::Cc, QT_TR_NOOP("Cc")}, + {RuleTerm::Subject, QT_TR_NOOP("Subject")}, + {RuleTerm::Tag, QT_TR_NOOP("Tag")}, + {RuleTerm::Folder, QT_TR_NOOP("Folder")}, + {RuleTerm::Attachment, QT_TR_NOOP("Attachment")}, + {RuleTerm::Date, QT_TR_NOOP("Date")}, +}; + } // namespace 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(); @@ -82,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); @@ -104,9 +149,78 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) form->addRow(QString(), m_enabled); form->addRow(tr("Add tags"), m_add); form->addRow(tr("Remove tags"), m_remove); - form->addRow(tr("Query"), m_query); + m_builder = new QWidget(this); + auto *builderLayout = new QVBoxLayout(m_builder); + builderLayout->setContentsMargins(0, 0, 0, 0); + + auto *matchRow = new QHBoxLayout; + m_matchAll = new QRadioButton(tr("Match &all"), m_builder); + m_matchAny = new QRadioButton(tr("Match a&ny"), m_builder); + m_matchAll->setChecked(true); + auto *matchGroup = new QButtonGroup(this); + matchGroup->addButton(m_matchAll); + matchGroup->addButton(m_matchAny); + matchRow->addWidget(m_matchAll); + matchRow->addWidget(m_matchAny); + matchRow->addStretch(); + builderLayout->addLayout(matchRow); + + m_rowsLayout = new QVBoxLayout; + builderLayout->addLayout(m_rowsLayout); + + m_exclusionsHeader = new QLabel(tr("But not"), m_builder); + builderLayout->addWidget(m_exclusionsHeader); + m_exclusionsLayout = new QVBoxLayout; + builderLayout->addLayout(m_exclusionsLayout); + + m_addExclusion = new QPushButton(tr("Add e&xclusion"), m_builder); + builderLayout->addWidget(m_addExclusion, 0, Qt::AlignLeft); + + // 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 + // vanishes with the rows it governs, leaving no way back except closing + // the dialog, which is exactly what shipped in the first draft of this + // builder. The query row is visible in both modes, so the toggle is + // always reachable. + auto *queryRow = new QHBoxLayout; + m_textMode = new QCheckBox(tr("Edit as &text"), this); + m_textMode->setToolTip( + tr("Edit the notmuch query directly. A rule too complex to show as " + "rows opens this way.")); + queryRow->addWidget(m_query, 1); + queryRow->addWidget(m_textMode); + form->addRow(tr("Query"), queryRow); + + // The query line shows what the rows compile to. Read-only in builder + // mode: it is what actually ships to the hook, and watching it change is + // what makes the rows trustworthy. + m_query->setReadOnly(true); + + connect(m_matchAll, &QRadioButton::toggled, + this, &TagRulesDialog::syncQueryLine); + connect(m_textMode, &QCheckBox::toggled, + this, &TagRulesDialog::setTextMode); + connect(m_addExclusion, &QPushButton::clicked, this, [this] { + addRow(true); + syncQueryLine(); + }); + form->addRow(tr("Note"), m_note); - layout->addLayout(form); + editorLayout->addLayout(form); auto *buttons = new QHBoxLayout; auto *addButton = new QPushButton(tr("&New"), this); @@ -120,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); @@ -137,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, @@ -166,8 +287,134 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) connect(m_note, &QPlainTextEdit::textChanged, this, &TagRulesDialog::applyEditsToCurrentRule); + addRow(false); + updateExclusionsVisibility(); + 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 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()) { + 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()); + state.setValue(QStringLiteral("tagrules/splitter"), + m_splitter->saveState()); +} + +/// 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::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(); +} + +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 + // 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); +} + +void TagRulesDialog::setColumnWidthForTest(int column, int width) +{ + m_list->setColumnWidth(column, width); +} + +void TagRulesDialog::reloadListForTest() +{ + reloadList(); } void TagRulesDialog::showWarnings() @@ -210,8 +457,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()) @@ -230,12 +484,23 @@ void TagRulesDialog::onSelectionChanged() const int index = currentIndex(); if (index < 0 || index >= m_working.size()) return; - const TagRule &rule = m_working.at(index); - - // The note's textChanged fires from setPlainText below, which would then - // write the rule just loaded back over the rule now current. Harmless when - // they are the same rule, destructive when the selection is what changed. - const QSignalBlocker blockNote(m_note); + // By value: every setter below can reach applyEditsToCurrentRule(), which + // writes into m_working, and a reference into that list would then be read + // back half overwritten. + const TagRule rule = m_working.at(index); + + // Populating the form emits change signals whose handlers write the form + // back onto the working copy, so the rule just loaded would be written over + // whichever rule is now current. Harmless when they are the same rule, + // destructive when the selection is what changed. m_enabled::toggled and + // the note's textChanged both do this, and the widgets are populated in an + // order where m_enabled fires while m_query still holds the PREVIOUS rule's + // text, which emptied the query of the first rule opened. The reloading + // flag is the existing guard for exactly this, so it covers the whole load, + // including rebuildRows: populating combo boxes and line edits fires + // currentIndexChanged, which runs syncQueryLine. + const bool wasReloading = m_reloading; + m_reloading = true; m_id->setText(rule.id); m_stage->setValue(rule.stage); @@ -244,6 +509,26 @@ void TagRulesDialog::onSelectionChanged() m_remove->setText(rule.remove.join(QStringLiteral(", "))); m_query->setText(rule.query); m_note->setPlainText(rule.note); + + // Parse once, on load, and keep it: the save path compares against this to + // decide whether the stored string may be left alone, so that opening a + // rule and closing it cannot rewrite the file mailctl also reads. + m_loadedQuery = RuleQuery::parse(rule.query); + + // A rule the builder cannot show opens as text, and one it can show + // returns to the builder. Blocked, because letting setChecked run + // setTextMode() here would recompile and overwrite m_query mid-load. + { + const QSignalBlocker blockTextMode(m_textMode); + m_textMode->setChecked(!m_loadedQuery.parsed); + } + m_builderScroll->setVisible(m_loadedQuery.parsed); + m_query->setReadOnly(m_loadedQuery.parsed); + + if (m_loadedQuery.parsed) + rebuildRows(m_loadedQuery); + + m_reloading = wasReloading; } void TagRulesDialog::applyEditsToCurrentRule() @@ -261,7 +546,16 @@ void TagRulesDialog::applyEditsToCurrentRule() rule.enabled = m_enabled->isChecked(); rule.add = splitTags(m_add->text()); rule.remove = splitTags(m_remove->text()); - rule.query = m_query->text().trimmed(); + if (m_textMode->isChecked()) { + rule.query = m_query->text().trimmed(); + } else { + const RuleQuery current = currentQueryFromRows(); + // Unchanged rows mean the stored string is left exactly as it was + // read. Recompiling an untouched rule would churn a file the + // companion tool also reads, showing a diff the user never made. + if (!(current == m_loadedQuery)) + rule.query = current.compile(); + } rule.note = m_note->toPlainText(); fillItem(m_list->topLevelItem(index), rule); @@ -323,7 +617,12 @@ void TagRulesDialog::setCounts(const QVector<int> &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() @@ -339,3 +638,367 @@ void TagRulesDialog::onSave() } accept(); } + +QString TagRulesDialog::queryLineForTest() const +{ + return m_query->text(); +} + +bool TagRulesDialog::textModeForTest() const +{ + return m_textMode->isChecked(); +} + +void TagRulesDialog::setRowValueForTest(int index, const QString &value) +{ + if (index < 0 || index >= m_rows.size()) + return; + setRowValue(&m_rows[index], value); + syncQueryLine(); +} + +void TagRulesDialog::setQueryTextForTest(const QString &text) +{ + m_query->setText(text); +} + +void TagRulesDialog::setTextModeForTest(bool on) +{ + m_textMode->setChecked(on); +} + +bool TagRulesDialog::textModeToggleIsReachableForTest() const +{ + // isVisibleTo rather than isVisible: nothing is isVisible() on a dialog + // that was never shown, so that would report unreachable in both the + // working and the broken case. + return m_textMode->isVisibleTo(this); +} + +QString TagRulesDialog::warningTextForTest() const +{ + // isVisible() is false for every child of a dialog that was never shown, + // so it would report no warning whatever the label held. isVisibleTo() + // answers the question actually being asked: would this be on screen if + // the dialog were. + return m_warningLabel->isVisibleTo(this) ? m_warningLabel->text() + : QString(); +} + +void TagRulesDialog::selectRuleForTest(int index) +{ + if (index >= 0 && index < m_list->topLevelItemCount()) + m_list->setCurrentItem(m_list->topLevelItem(index)); +} + +QString TagRulesDialog::rowValue(const Row &row) const +{ + const bool isFolder = + RuleTerm::Field(row.field->currentData().toInt()) == RuleTerm::Folder; + return (isFolder ? row.folder->currentText() : row.value->text()).trimmed(); +} + +void TagRulesDialog::setRowValue(Row *row, const QString &value) +{ + if (RuleTerm::Field(row->field->currentData().toInt()) == RuleTerm::Folder) + row->folder->setCurrentText(value); + else + row->value->setText(value); +} + +void TagRulesDialog::setFolders(const QStringList &folders) +{ + m_folders = folders; + + // Rows already exist by the time this is called: the constructor loads the + // first rule and builds its rows before the caller can hand the list over. + // Repopulating them here rather than only in addRow() is what stops the row + // on screen from opening with an empty dropdown. The current text is + // preserved across the refill, since the combo is editable and may hold a + // folder the config does not list. + const auto refill = [&folders](const QList<Row> &rows) { + for (const Row &row : rows) { + const QString had = row.folder->currentText(); + QSignalBlocker block(row.folder); + row.folder->clear(); + row.folder->addItems(folders); + row.folder->setCurrentText(had); + } + }; + refill(m_rows); + refill(m_exclusionRows); +} + +TagRulesDialog::Row *TagRulesDialog::addRow(bool exclusion) +{ + Row row; + row.container = new QWidget(m_builder); + auto *layout = new QHBoxLayout(row.container); + layout->setContentsMargins(0, 0, 0, 0); + + row.field = new QComboBox(row.container); + for (const FieldEntry &entry : kFields) + row.field->addItem(tr(entry.label), int(entry.field)); + + row.op = new QComboBox(row.container); + row.value = new QLineEdit(row.container); + + row.folder = new QComboBox(row.container); + // Editable so a folder present in the file but absent from the config + // still displays and still saves, rather than being silently blanked. + row.folder->setEditable(true); + row.folder->addItems(m_folders); + row.folder->setVisible(false); + + auto *plus = new QPushButton(QStringLiteral("+"), row.container); + auto *minus = new QPushButton(QStringLiteral("-"), row.container); + plus->setFixedWidth(30); + minus->setFixedWidth(30); + + layout->addWidget(row.field); + layout->addWidget(row.op); + layout->addWidget(row.value, 1); + layout->addWidget(row.folder, 1); + layout->addWidget(plus); + layout->addWidget(minus); + + QList<Row> &rows = exclusion ? m_exclusionRows : m_rows; + QVBoxLayout *target = exclusion ? m_exclusionsLayout : m_rowsLayout; + rows.append(row); + target->addWidget(row.container); + + // The three widgets by pointer, never the Row by value: the row lives in a + // QList that reallocates as rows are added, so a copy taken here would be + // compared against, or written through, after that list has moved. + QComboBox *field = row.field; + QLineEdit *value = row.value; + QComboBox *folder = row.folder; + connect(row.field, &QComboBox::currentIndexChanged, this, + [this, exclusion, field, value, folder](int) { + populateOperators(exclusion); + const bool isFolder = + RuleTerm::Field(field->currentData().toInt()) + == RuleTerm::Folder; + value->setVisible(!isFolder); + folder->setVisible(isFolder); + syncQueryLine(); + }); + connect(row.op, &QComboBox::currentIndexChanged, + this, [this](int) { syncQueryLine(); }); + connect(row.value, &QLineEdit::textEdited, + this, [this](const QString &) { syncQueryLine(); }); + connect(row.folder, &QComboBox::currentTextChanged, + this, [this](const QString &) { syncQueryLine(); }); + connect(plus, &QPushButton::clicked, this, [this, exclusion] { + addRow(exclusion); + syncQueryLine(); + }); + + QWidget *container = row.container; + connect(minus, &QPushButton::clicked, this, [this, exclusion, container] { + const QList<Row> &list = exclusion ? m_exclusionRows : m_rows; + for (int i = 0; i < list.size(); ++i) { + if (list.at(i).container == container) { + removeRow(exclusion, i); + break; + } + } + syncQueryLine(); + }); + + populateOperators(exclusion); + updateExclusionsVisibility(); + return &rows.last(); +} + +void TagRulesDialog::removeRow(bool exclusion, int index) +{ + QList<Row> &rows = exclusion ? m_exclusionRows : m_rows; + if (index < 0 || index >= rows.size()) + return; + + // The positive section keeps at least one row: a rule with no rows has an + // empty query, which is reachable by clearing the value rather than by + // deleting the last row out from under the user. + if (!exclusion && rows.size() == 1) { + setRowValue(&rows[0], QString()); + return; + } + + delete rows.at(index).container; + rows.removeAt(index); + updateExclusionsVisibility(); +} + +void TagRulesDialog::updateExclusionsVisibility() +{ + // Most rules have no exclusions, so an empty block on every rule is noise. + const bool any = !m_exclusionRows.isEmpty(); + m_exclusionsHeader->setVisible(any); +} + +void TagRulesDialog::populateOperators(bool exclusion) +{ + const QList<Row> &rows = exclusion ? m_exclusionRows : m_rows; + for (const Row &row : rows) { + const auto field = RuleTerm::Field(row.field->currentData().toInt()); + const QString had = row.op->currentText(); + QSignalBlocker block(row.op); + row.op->clear(); + + switch (field) { + case RuleTerm::Tag: + case RuleTerm::Folder: + row.op->addItem(tr("is"), int(RuleTerm::Is)); + row.op->addItem(tr("is not"), int(RuleTerm::IsNot)); + break; + case RuleTerm::Attachment: + row.op->addItem(tr("has"), int(RuleTerm::Has)); + row.op->addItem(tr("has not"), int(RuleTerm::HasNot)); + break; + case RuleTerm::Date: + row.op->addItem(tr("before"), int(RuleTerm::Before)); + row.op->addItem(tr("after"), int(RuleTerm::After)); + break; + default: + row.op->addItem(tr("contains"), int(RuleTerm::Contains)); + row.op->addItem(tr("contains not"), int(RuleTerm::ContainsNot)); + row.op->addItem(tr("is"), int(RuleTerm::Is)); + row.op->addItem(tr("is not"), int(RuleTerm::IsNot)); + break; + } + + const int restored = row.op->findText(had); + if (restored >= 0) + row.op->setCurrentIndex(restored); + } +} + +RuleQuery TagRulesDialog::currentQueryFromRows() const +{ + RuleQuery query; + query.parsed = true; + query.join = m_matchAny->isChecked() ? RuleQuery::Any : RuleQuery::All; + + for (const Row &row : m_rows) { + const QString value = rowValue(row); + if (value.isEmpty()) + continue; + query.terms.append({RuleTerm::Field(row.field->currentData().toInt()), + RuleTerm::Op(row.op->currentData().toInt()), + value}); + } + for (const Row &row : m_exclusionRows) { + const QString value = rowValue(row); + if (value.isEmpty()) + continue; + query.exclusions.append( + {RuleTerm::Field(row.field->currentData().toInt()), + RuleTerm::Op(row.op->currentData().toInt()), + value}); + } + return query; +} + +void TagRulesDialog::setTextMode(bool on) +{ + if (on) { + // Show what the rows currently mean, then hand the string over. + if (m_loadedQuery.parsed) + m_query->setText(currentQueryFromRows().compile()); + m_builderScroll->setVisible(false); + m_query->setReadOnly(false); + return; + } + + // Going back needs the typed query to be representable. If it is not, the + // checkbox cannot clear: there are no rows that mean this query. + // + // Said in the warning label rather than a modal. A modal here would block + // any test that reaches this branch, which is how a refusal path ends up + // shipping unverified, and it interrupts someone who is mid-edit to tell + // them something the label can hold while they keep typing. + const RuleQuery parsed = RuleQuery::parse(m_query->text().trimmed()); + if (!parsed.parsed) { + const QSignalBlocker block(m_textMode); + m_textMode->setChecked(true); + m_warningLabel->setText( + tr("This query is more than the builder can show, so it stays as " + "text. It is still saved and applied normally.")); + m_warningLabel->setVisible(true); + return; + } + + const bool wasReloading = m_reloading; + m_reloading = true; + rebuildRows(parsed); + m_reloading = wasReloading; + + m_loadedQuery = parsed; + m_builderScroll->setVisible(true); + m_query->setReadOnly(true); + + // The refusal above writes into the same label the load warnings use, so + // a successful return to the rows must clear it or a stale complaint + // outlives the query that caused it. showWarnings() restores whatever the + // file itself had to say. + showWarnings(); +} + +void TagRulesDialog::rebuildRows(const RuleQuery &query) +{ + while (!m_rows.isEmpty()) + delete m_rows.takeLast().container; + while (!m_exclusionRows.isEmpty()) + delete m_exclusionRows.takeLast().container; + + m_matchAll->setChecked(query.join == RuleQuery::All); + m_matchAny->setChecked(query.join == RuleQuery::Any); + + for (const RuleTerm &term : query.terms) + applyTermToRow(addRow(false), term); + for (const RuleTerm &term : query.exclusions) + applyTermToRow(addRow(true), term); + + if (m_rows.isEmpty()) + addRow(false); // Always one row to type into. + + updateExclusionsVisibility(); +} + +void TagRulesDialog::applyTermToRow(Row *row, const RuleTerm &term) +{ + const QSignalBlocker blockField(row->field); + const QSignalBlocker blockOp(row->op); + const QSignalBlocker blockValue(row->value); + const QSignalBlocker blockFolder(row->folder); + + const int fieldIndex = row->field->findData(int(term.field)); + if (fieldIndex >= 0) + row->field->setCurrentIndex(fieldIndex); + + // The field's own handler is blocked here, so the swap it would have done + // has to happen explicitly or a Folder row opens showing the line edit. + const bool isFolder = term.field == RuleTerm::Folder; + row->value->setVisible(!isFolder); + row->folder->setVisible(isFolder); + + // The operator list depends on the field just set, so it must be rebuilt + // before the operator can be found in it. + populateOperators(false); + populateOperators(true); + + const int opIndex = row->op->findData(int(term.op)); + if (opIndex >= 0) + row->op->setCurrentIndex(opIndex); + + setRowValue(row, term.value); +} + +void TagRulesDialog::syncQueryLine() +{ + if (m_reloading) + return; + m_query->setText(currentQueryFromRows().compile()); + applyEditsToCurrentRule(); +} diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index 0350bd5..e8979fe 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -19,17 +19,24 @@ #pragma once #include <QDialog> +#include <QStringList> +#include "rulequery.h" #include "tagrules.h" class QCheckBox; +class QComboBox; class QLabel; class QLineEdit; class QPlainTextEdit; class QPushButton; +class QRadioButton; +class QScrollArea; +class QSplitter; class QSpinBox; class QTreeWidget; class QTreeWidgetItem; +class QVBoxLayout; /// Views and edits the shared auto-tagging rules. /// @@ -51,10 +58,94 @@ public: /// handle and notmuch permits one per process. QStringList countQueries() const; + /// 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 + /// synthetic clicks on widgets whose geometry the offscreen platform does + /// not guarantee. + int rowCountForTest() const { return m_rows.size(); } + QString queryLineForTest() const; + + /// Selects the rule at `index` as a click on the list would. + void selectRuleForTest(int index); + + bool textModeForTest() const; + void setRowValueForTest(int index, const QString &value); + /// Runs the Save path without showing the dialog. + void saveForTest() { onSave(); } + + /// Types into the query field and toggles the mode, so the refusal path + /// can be reached without a synthetic click. The refusal reports through + /// the warning label rather than a modal precisely so this is testable. + void setQueryTextForTest(const QString &text); + void setTextModeForTest(bool on); + QString warningTextForTest() const; + + /// Whether the text-mode toggle would be on screen. A toggle that hides + /// itself when switched on is a one-way trip, and asserting only on the + /// checked STATE passes against that, since the state is still readable + /// 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(); + + /// 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. + /// 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(); + /// 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<int> &counts); @@ -67,8 +158,16 @@ private slots: void applyEditsToCurrentRule(); void onSave(); +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(); + void restoreUiState(); + void saveUiState(); void showWarnings(); int currentIndex() const; @@ -76,6 +175,44 @@ private: /// applyEditsToCurrentRule() so the two cannot render a rule differently. void fillItem(QTreeWidgetItem *item, const TagRule &rule) const; + /// One builder row's widgets, so a row can be removed as a unit. + struct Row + { + QWidget *container = nullptr; + QComboBox *field = nullptr; + QComboBox *op = nullptr; + QLineEdit *value = nullptr; + /// Shown in place of `value` for a Folder row. Never visible together. + QComboBox *folder = nullptr; + }; + + /// Reads a row's value from whichever of its two widgets the field selects, + /// so the read and write paths cannot disagree about where it lives. + QString rowValue(const Row &row) const; + void setRowValue(Row *row, const QString &value); + + Row *addRow(bool exclusion); + void removeRow(bool exclusion, int index); + void populateOperators(bool exclusion); + void updateExclusionsVisibility(); + void syncQueryLine(); + + void setTextMode(bool on); + + void rebuildRows(const RuleQuery &query); + void applyTermToRow(Row *row, const RuleTerm &term); + RuleQuery currentQueryFromRows() const; + + /// The query as parsed when the current rule was loaded. Task 10's save + /// path compares against this to decide whether the stored string may be + /// left alone. + RuleQuery m_loadedQuery; + + QStringList m_folders; + + QList<Row> m_rows; + QList<Row> m_exclusionRows; + TagRules m_rules; QList<TagRule> m_working; ///< Edited copy; written only on Save. @@ -85,6 +222,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; @@ -95,4 +241,22 @@ private: QCheckBox *m_enabled = nullptr; QLabel *m_warningLabel = nullptr; QPushButton *m_saveButton = nullptr; + + QRadioButton *m_matchAll = nullptr; + 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; + QPushButton *m_previewButton = nullptr; + QVBoxLayout *m_rowsLayout = nullptr; + QVBoxLayout *m_exclusionsLayout = nullptr; + QLabel *m_exclusionsHeader = nullptr; + QPushButton *m_addExclusion = nullptr; }; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9c65abd..e5142b6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -53,3 +53,4 @@ add_qtmaildir_test(messageview) add_qtmaildir_test(querycompleter) add_qtmaildir_test(tagdialog) add_qtmaildir_test(tagrules) +add_qtmaildir_test(rulequery) diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index 1db26c7..9068ca3 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -80,6 +80,9 @@ private slots: void messageCountsCountMessagesNotThreads(); void messageCountsReportAnInvalidQueryAsMinusOne(); + void requestFoldersListsEveryMaildirFolder(); + void requestFoldersOnUnreadableConfigEmitsError(); + private: /// Tags of one message, read back through a fresh worker query. QStringList tagsOf(const QString &messageId); @@ -900,5 +903,79 @@ void TestNotmuchWorker::requestDatabaseStatsOnUnreadableConfigEmitsError() QVERIFY(ready.isEmpty()); } +void TestNotmuchWorker::requestFoldersListsEveryMaildirFolder() +{ + // Its own fixture rather than the shared one: this needs a NESTED folder, + // which is the shape a real account has (<account>/Drafts, not a flat + // "drafts"), and adding a message to the shared fixture would move seven + // count assertions in other tests for nothing. + NotmuchFixture fixture; + QVERIFY(fixture.isValid()); + QVERIFY(fixture.addMessage(QStringLiteral("work/INBOX"), + QStringLiteral("g1@example.org"), + QStringLiteral("Something"), + QStringLiteral("Alice <alice@example.org>"), + QStringLiteral("Mon, 1 Jun 2026 10:00:00 +0000"), + QStringLiteral("body"), false)); + QVERIFY(fixture.addMessage(QStringLiteral("work/Drafts"), + QStringLiteral("g2@example.org"), + QStringLiteral("Half written"), + QStringLiteral("You <you@example.org>"), + QStringLiteral("Tue, 2 Jun 2026 10:00:00 +0000"), + QStringLiteral("body"), false)); + QVERIFY2(fixture.index(), qPrintable(fixture.error())); + + // An EMPTY folder, created but never written to. mbsync makes these, and a + // list derived from indexed messages would not offer it. A rule may + // legitimately target a folder that has nothing in it yet. + QDir dir; + const QString empty = fixture.maildirPath() + QStringLiteral("/work/Archive"); + QVERIFY(dir.mkpath(empty + QStringLiteral("/cur"))); + QVERIFY(dir.mkpath(empty + QStringLiteral("/new"))); + QVERIFY(dir.mkpath(empty + QStringLiteral("/tmp"))); + + NotmuchWorker worker(fixture.configPath()); + QSignalSpy ready(&worker, &NotmuchWorker::foldersReady); + + worker.requestFolders(); + + QCOMPARE(ready.count(), 1); + const QStringList folders = ready.first().at(0).toStringList(); + + // Paths relative to the database root, which is what a Folder term + // compiles a path: against. Drafts is the whole point of the item: the + // dialog used to offer one entry per account and nothing below it. + QVERIFY(folders.contains(QStringLiteral("work/INBOX"))); + QVERIFY(folders.contains(QStringLiteral("work/Drafts"))); + QVERIFY(folders.contains(QStringLiteral("work/Archive"))); + + // Not the maildir plumbing, which is not a folder anyone files mail into, + // and not the account directory itself, which holds no cur/. + QVERIFY(!folders.contains(QStringLiteral("work/INBOX/cur"))); + QVERIFY(!folders.contains(QStringLiteral("work/INBOX/new"))); + QVERIFY(!folders.contains(QStringLiteral("work"))); + + // Sorted, so the dropdown does not reorder itself between openings with + // the same tree on disk. QDir's own order is filesystem order. + QStringList sorted = folders; + sorted.sort(); + QCOMPARE(folders, sorted); +} + +void TestNotmuchWorker::requestFoldersOnUnreadableConfigEmitsError() +{ + // Fails closed like every other entry point. The dialog then leaves the + // dropdown as it was rather than emptying it, since an empty list reads as + // "this account has no folders". + NotmuchWorker worker(QStringLiteral("/nonexistent/qtmaildir-test/config")); + QSignalSpy ready(&worker, &NotmuchWorker::foldersReady); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.requestFolders(); + + QCOMPARE(errors.size(), 1); + QVERIFY(ready.isEmpty()); +} + QTEST_MAIN(TestNotmuchWorker) #include "test_notmuchworker.moc" diff --git a/tests/test_rulequery.cpp b/tests/test_rulequery.cpp new file mode 100644 index 0000000..54f9477 --- /dev/null +++ b/tests/test_rulequery.cpp @@ -0,0 +1,415 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. <danix@danix.xyz> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include <QtTest> + +#include "rulequery.h" + +/// RuleQuery is a view over a string the notmuch post-new hook executes, so +/// the risk is a query that compiles to something subtly wider than the rows +/// say. These tests are about exact strings, not about whether notmuch would +/// accept the result: notmuch accepts almost anything, including `from:((((`. +class TestRuleQuery : public QObject +{ + Q_OBJECT + +private slots: + void aSingleContainsTermCompiles(); + void everyFieldCompilesToItsPrefix(); + void isQuotesAndContainsDoesNot(); + void negationPrefixesNot(); + void aValueWithASpaceIsAlwaysQuoted(); + void folderAppendsTheRecursiveSuffix(); + void dateCompilesToAOneSidedRange(); + void allJoinsWithAnd(); + void anyJoinsWithOr(); + void exclusionsAppendAsAndNot(); + void anyIsParenthesisedOnlyWhenExclusionsFollow(); + void anEmptyQueryCompilesToAnEmptyString(); + void aFlatAndChainParses(); + void aFlatOrChainParses(); + void anEmptyQueryParsesToNoRows(); + void quotedValuesLoseTheirQuotes(); + void aNegatedTermParsesAsANegatedOperator(); + void whatParsesCompilesBackUnchanged(); + void anOrGroupWithExclusionsParses(); + void anUnrepresentableQueryRejectsWhole(); + void aParenBearingValueIsARowNotAShape(); + void theRuleCorpusRoundTripsByteForByte(); +}; + +void TestRuleQuery::aSingleContainsTermCompiles() +{ + RuleQuery q; + q.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("sender@example.org")}); + + QCOMPARE(q.compile(), QStringLiteral("from:sender@example.org")); +} + +void TestRuleQuery::everyFieldCompilesToItsPrefix() +{ + const QVector<QPair<RuleTerm::Field, QString>> cases = { + {RuleTerm::From, QStringLiteral("from:x")}, + {RuleTerm::To, QStringLiteral("to:x")}, + {RuleTerm::Cc, QStringLiteral("cc:x")}, + {RuleTerm::Subject, QStringLiteral("subject:x")}, + }; + + for (const auto &c : cases) { + RuleQuery q; + q.terms.append({c.first, RuleTerm::Contains, QStringLiteral("x")}); + QCOMPARE(q.compile(), c.second); + } +} + +void TestRuleQuery::isQuotesAndContainsDoesNot() +{ + RuleQuery contains; + contains.terms.append({RuleTerm::Subject, RuleTerm::Contains, + QStringLiteral("receipt")}); + QCOMPARE(contains.compile(), QStringLiteral("subject:receipt")); + + RuleQuery is; + is.terms.append({RuleTerm::Subject, RuleTerm::Is, + QStringLiteral("receipt")}); + QCOMPARE(is.compile(), QStringLiteral("subject:\"receipt\"")); +} + +void TestRuleQuery::negationPrefixesNot() +{ + RuleQuery q; + q.terms.append({RuleTerm::Subject, RuleTerm::ContainsNot, + QStringLiteral("receipt")}); + QCOMPARE(q.compile(), QStringLiteral("not subject:receipt")); + + RuleQuery tag; + tag.terms.append({RuleTerm::Tag, RuleTerm::IsNot, + QStringLiteral("inbox")}); + QCOMPARE(tag.compile(), QStringLiteral("not tag:inbox")); + + RuleQuery att; + att.terms.append({RuleTerm::Attachment, RuleTerm::HasNot, + QStringLiteral("pdf")}); + QCOMPARE(att.compile(), QStringLiteral("not attachment:pdf")); +} + +void TestRuleQuery::aValueWithASpaceIsAlwaysQuoted() +{ + // Unquoted, a space would end the term and the rest would become a + // separate bare word, silently widening the rule. + RuleQuery q; + q.terms.append({RuleTerm::Subject, RuleTerm::Contains, + QStringLiteral("your receipt")}); + QCOMPARE(q.compile(), QStringLiteral("subject:\"your receipt\"")); +} + +void TestRuleQuery::folderAppendsTheRecursiveSuffix() +{ + // A path: without the suffix matches nothing, and notmuch reports no + // error when it happens. + RuleQuery q; + q.terms.append({RuleTerm::Folder, RuleTerm::Is, + QStringLiteral("account-one")}); + QCOMPARE(q.compile(), QStringLiteral("path:\"account-one/**\"")); +} + +void TestRuleQuery::dateCompilesToAOneSidedRange() +{ + RuleQuery before; + before.terms.append({RuleTerm::Date, RuleTerm::Before, + QStringLiteral("2026-01-01")}); + QCOMPARE(before.compile(), QStringLiteral("date:..2026-01-01")); + + RuleQuery after; + after.terms.append({RuleTerm::Date, RuleTerm::After, + QStringLiteral("2026-01-01")}); + QCOMPARE(after.compile(), QStringLiteral("date:2026-01-01..")); +} + +void TestRuleQuery::allJoinsWithAnd() +{ + RuleQuery q; + q.join = RuleQuery::All; + q.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("vendor.example.org")}); + q.terms.append({RuleTerm::Subject, RuleTerm::Contains, + QStringLiteral("receipt")}); + + QCOMPARE(q.compile(), + QStringLiteral("from:vendor.example.org and subject:receipt")); +} + +void TestRuleQuery::anyJoinsWithOr() +{ + RuleQuery q; + q.join = RuleQuery::Any; + q.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("one.example.org")}); + q.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("two.example.org")}); + + QCOMPARE(q.compile(), + QStringLiteral("from:one.example.org or from:two.example.org")); +} + +void TestRuleQuery::exclusionsAppendAsAndNot() +{ + RuleQuery q; + q.join = RuleQuery::All; + q.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("vendor.example.org")}); + q.exclusions.append({RuleTerm::Subject, RuleTerm::Contains, + QStringLiteral("receipt")}); + + QCOMPARE(q.compile(), + QStringLiteral("from:vendor.example.org " + "and not subject:receipt")); +} + +void TestRuleQuery::anyIsParenthesisedOnlyWhenExclusionsFollow() +{ + // Without the parens this binds as (a or (b and not c)), which matches + // everything from the first sender regardless of the exclusion. + RuleQuery guarded; + guarded.join = RuleQuery::Any; + guarded.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("one.example.org")}); + guarded.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("two.example.org")}); + guarded.exclusions.append({RuleTerm::Subject, RuleTerm::Contains, + QStringLiteral("receipt")}); + + QCOMPARE(guarded.compile(), + QStringLiteral("(from:one.example.org or from:two.example.org) " + "and not subject:receipt")); + + // No exclusion, no parens: they would be noise in the stored file. + RuleQuery bare; + bare.join = RuleQuery::Any; + bare.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("one.example.org")}); + bare.terms.append({RuleTerm::From, RuleTerm::Contains, + QStringLiteral("two.example.org")}); + + QCOMPARE(bare.compile(), + QStringLiteral("from:one.example.org or from:two.example.org")); +} + +void TestRuleQuery::anEmptyQueryCompilesToAnEmptyString() +{ + RuleQuery q; + QCOMPARE(q.compile(), QString()); +} + +void TestRuleQuery::aFlatAndChainParses() +{ + const RuleQuery q = RuleQuery::parse( + QStringLiteral("from:vendor.example.org and subject:receipt")); + + QVERIFY(q.parsed); + QCOMPARE(q.join, RuleQuery::All); + QCOMPARE(q.terms.size(), 2); + QCOMPARE(q.terms.at(0).field, RuleTerm::From); + QCOMPARE(q.terms.at(0).op, RuleTerm::Contains); + QCOMPARE(q.terms.at(0).value, QStringLiteral("vendor.example.org")); + QCOMPARE(q.terms.at(1).field, RuleTerm::Subject); + QVERIFY(q.exclusions.isEmpty()); +} + +void TestRuleQuery::aFlatOrChainParses() +{ + const RuleQuery q = RuleQuery::parse( + QStringLiteral("from:one.example.org or from:two.example.org")); + + QVERIFY(q.parsed); + QCOMPARE(q.join, RuleQuery::Any); + QCOMPARE(q.terms.size(), 2); +} + +void TestRuleQuery::anEmptyQueryParsesToNoRows() +{ + // One shipped rule has an empty query. It must open in the builder ready + // to receive a row, not fall back to text mode. + const RuleQuery q = RuleQuery::parse(QString()); + + QVERIFY(q.parsed); + QVERIFY(q.terms.isEmpty()); +} + +void TestRuleQuery::quotedValuesLoseTheirQuotes() +{ + const RuleQuery q = RuleQuery::parse( + QStringLiteral("subject:\"your receipt\"")); + + QVERIFY(q.parsed); + QCOMPARE(q.terms.size(), 1); + QCOMPARE(q.terms.at(0).value, QStringLiteral("your receipt")); + QCOMPARE(q.terms.at(0).op, RuleTerm::Is); +} + +void TestRuleQuery::aNegatedTermParsesAsANegatedOperator() +{ + const RuleQuery q = RuleQuery::parse( + QStringLiteral("from:vendor.example.org and not tag:inbox")); + + QVERIFY(q.parsed); + // A trailing negation on an `and` chain becomes an exclusion: that is how + // the user describes these rules, and the design records the preference. + QCOMPARE(q.terms.size(), 1); + QCOMPARE(q.exclusions.size(), 1); + QCOMPARE(q.exclusions.at(0).field, RuleTerm::Tag); + QCOMPARE(q.exclusions.at(0).op, RuleTerm::Is); +} + +void TestRuleQuery::whatParsesCompilesBackUnchanged() +{ + // The dialog decides whether to rewrite the stored string by comparing + // against what it parsed, so a compile that differs by so much as a quote + // would churn a file a second tool reads. + const QStringList queries = { + QStringLiteral("from:vendor.example.org"), + QStringLiteral("from:vendor.example.org and subject:receipt"), + QStringLiteral("from:one.example.org or from:two.example.org"), + QStringLiteral("subject:\"your receipt\""), + QStringLiteral("path:\"account-one/**\""), + QStringLiteral("tag:inbox"), + QStringLiteral("attachment:pdf"), + QStringLiteral("date:..2026-01-01"), + QStringLiteral("date:2026-01-01.."), + QStringLiteral("from:vendor.example.org and not tag:inbox"), + QStringLiteral("from:vendor.example.org and not subject:receipt " + "and not subject:refund"), + QStringLiteral("(from:one.example.org or from:two.example.org) " + "and not subject:receipt"), + }; + + for (const QString &query : queries) { + const RuleQuery parsed = RuleQuery::parse(query); + QVERIFY2(parsed.parsed, qPrintable(query)); + QCOMPARE(parsed.compile(), query); + } +} + +void TestRuleQuery::anOrGroupWithExclusionsParses() +{ + const RuleQuery q = RuleQuery::parse( + QStringLiteral("(from:vendor.example.org or from:vendor.example.net) " + "and not subject:receipt and not subject:refund")); + + QVERIFY(q.parsed); + QCOMPARE(q.join, RuleQuery::Any); + QCOMPARE(q.terms.size(), 2); + QCOMPARE(q.exclusions.size(), 2); + QCOMPARE(q.exclusions.at(0).field, RuleTerm::Subject); + QCOMPARE(q.exclusions.at(0).op, RuleTerm::Contains); + QCOMPARE(q.exclusions.at(0).value, QStringLiteral("receipt")); + + // The round trip is the point: this must come back as it went in. + QCOMPARE(q.compile(), + QStringLiteral("(from:vendor.example.org or " + "from:vendor.example.net) " + "and not subject:receipt and not subject:refund")); +} + +void TestRuleQuery::anUnrepresentableQueryRejectsWhole() +{ + const QStringList unrepresentable = { + QStringLiteral("from:a.example.org or (from:b.example.org " + "or from:c.example.org)"), // nested or inside or + QStringLiteral("from:a.example.org and subject:x " + "or subject:y"), // mixed, unparenthesised + QStringLiteral("body:receipt"), // unrecognised prefix + QStringLiteral("folder:Inbox"), // not the prefix we emit + QStringLiteral("receipt"), // bare word, no prefix + QStringLiteral("path:\"account-one\""), // no /** suffix + QStringLiteral("from:a.example.org and"), // trailing operator + QStringLiteral("date:2026-01-01..2026-02-01"), // two-sided range + QStringLiteral("from:a.example.org xor subject:x"), + QStringLiteral("subject:\"unterminated"), // open quote + QStringLiteral("(from:a.example.org or from:b.example.org)"), + // group, nothing after it + QStringLiteral("(from:a.example.org and from:b.example.org) " + "and not subject:x"), // group joined by and + }; + + for (const QString &query : unrepresentable) { + const RuleQuery q = RuleQuery::parse(query); + QVERIFY2(!q.parsed, qPrintable(QStringLiteral("parsed: ") + query)); + // Rejecting whole means keeping nothing: a half-parse is how a + // negation goes missing and a rule quietly matches more mail. + QVERIFY2(q.terms.isEmpty() && q.exclusions.isEmpty(), + qPrintable(QStringLiteral("kept rows: ") + query)); + } +} + +void TestRuleQuery::aParenBearingValueIsARowNotAShape() +{ + // notmuch reads these parens as characters to search for, not as + // grouping, so the query is meaningful and matches nothing. It parses + // here as a From row holding that literal text, which is what it means. + // The property to hold is the round trip, NOT a rejection: a test + // expecting an error here fails against correct code. + const RuleQuery q = RuleQuery::parse(QStringLiteral("from:((((")); + + QVERIFY(q.parsed); + QCOMPARE(q.terms.size(), 1); + QCOMPARE(q.terms.at(0).value, QStringLiteral("((((")); + QCOMPARE(q.compile(), QStringLiteral("from:((((")); +} + +void TestRuleQuery::theRuleCorpusRoundTripsByteForByte() +{ + // Every query SHAPE present in a real rules file, with placeholder + // values. Verified by hand against the live file when this was written: + // all seventeen rules parsed and compiled back byte for byte, none fell + // to text mode. + // + // A compile that differs by so much as a paren would rewrite the shared + // file on the next save, which the companion tool then sees as a diff + // nobody made. + const QStringList corpus = { + QString(), // a rule with no query yet + QStringLiteral("path:\"account-one/**\""), + QStringLiteral("path:\"account-two/Inbox/topic/**\""), + QStringLiteral("subject:\"[list-name]\""), + QStringLiteral("from:notifications@service.example.org"), + QStringLiteral("from:one@jobs.example.org or " + "from:two@jobs.example.org or " + "from:three@jobs.example.org"), + QStringLiteral("from:mail.vendor.example.org and " + "subject:\"Secure link\""), + QStringLiteral("(from:vendor.example.org or from:vendor.example.net) " + "and not subject:receipt and not subject:refund " + "and not subject:EUR"), + }; + + for (const QString &query : corpus) { + const RuleQuery parsed = RuleQuery::parse(query); + QVERIFY2(parsed.parsed, qPrintable(query)); + QCOMPARE(parsed.compile(), query); + + // And the value itself round-trips, which is what the dialog's + // "was this edited" comparison depends on. + QVERIFY2(RuleQuery::parse(parsed.compile()) == parsed, + qPrintable(query)); + } +} + +QTEST_MAIN(TestRuleQuery) +#include "test_rulequery.moc" diff --git a/tests/test_tagrules.cpp b/tests/test_tagrules.cpp index 8c7e1a9..ed3a06d 100644 --- a/tests/test_tagrules.cpp +++ b/tests/test_tagrules.cpp @@ -16,10 +16,15 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include <QSettings> #include <QTemporaryDir> #include <QtTest> +#include "config.h" +#include "mainwindow.h" +#include "rulequery.h" #include "tagrules.h" +#include "tagrulesdialog.h" /// The risk in TagRules is the format, not painting: a field silently dropped /// on save mis-tags real mail on the next sync, and does it quietly. These @@ -38,6 +43,21 @@ private slots: void aQueryWithQuotesRoundTrips(); void aMissingFileIsEmptyNotAnError(); void aNewerVersionIsRefused(); + void openingARuleFillsTheBuilderRows(); + void switchingRulesDoesNotLeakRowsBetweenThem(); + void openingARuleWithoutEditingLeavesItByteIdentical(); + void anUnrepresentableRuleOpensInTextMode(); + void editingARowRewritesTheQuery(); + void aTextModeRuleStaysTextWhenAnotherRuleIsVisited(); + void leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows(); + void aFolderRowUsesTheDropdownAndKeepsItsSuffix(); + void theTextModeToggleSurvivesBeingSwitchedOn(); + void theWindowSizeAndColumnWidthsSurviveAReopen(); + void theWindowSizeIsSavedOnEveryWayOutOfTheDialog(); + void aReloadDoesNotDiscardARestoredColumnWidth(); + void manyConditionRowsDoNotSqueezeTheRuleList(); + void previewEmitsTheRuleQueryAsStored(); + void previewClearsTheAccountScope(); private: QString writeRules(const QString &json); @@ -241,5 +261,685 @@ void TestTagRules::aNewerVersionIsRefused() QCOMPARE(rules.warnings().size(), 1); } +void TestTagRules::openingARuleFillsTheBuilderRows() +{ + // The dialog reads the shared store from its default path, so point the + // whole process at a temporary one. XDG_CONFIG_HOME is what + // TagRules::defaultPath() honours. + 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": "vendor", + "query": "from:vendor.example.org and subject:receipt", + "add": ["vendor"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + + QCOMPARE(dialog.rowCountForTest(), 2); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("from:vendor.example.org and subject:receipt")); +} + +void TestTagRules::switchingRulesDoesNotLeakRowsBetweenThem() +{ + 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:one.example.org", + "add": ["one"], "stage": 50, "enabled": true}, + {"id": "two", + "query": "from:two.example.org or from:three.example.org", + "add": ["two"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + + // The first rule is selected on open: one row, joined All by default. + QCOMPARE(dialog.rowCountForTest(), 1); + QCOMPARE(dialog.queryLineForTest(), QStringLiteral("from:one.example.org")); + + dialog.selectRuleForTest(1); + QCOMPARE(dialog.rowCountForTest(), 2); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("from:two.example.org or from:three.example.org")); + + // 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")); +} + +void TestTagRules::openingARuleWithoutEditingLeavesItByteIdentical() +{ + // Recompiling on open would rewrite the shared file for no reason, and + // the companion tool would see a diff the user never made. Semantically + // equal is not enough: the bytes must match. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "handwritten", + "query": "not subject:receipt and from:plain.example.net", + "add": ["handwritten"], "stage": 50, "enabled": true}, + {"id": "vendor", + "query": "(from:vendor.example.org or from:vendor.example.net) and not subject:receipt", + "add": ["vendor"], "stage": 50, "enabled": true}, + {"id": "plain", "query": "from:plain.example.org", + "add": ["plain"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + { + TagRulesDialog dialog; + dialog.selectRuleForTest(2); + dialog.selectRuleForTest(1); + dialog.selectRuleForTest(0); + dialog.saveForTest(); + } + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 3); + // Hand-written spacing and an exclusion ahead of the positive term. Both + // are things compile() normalises away, and this rule is deliberately the + // one left current at Save, since that is the only rule the save path + // writes at all. The two below round trip byte for byte on their own, so + // neither could catch a save path that recompiles regardless. + QCOMPARE(reloaded.rules().at(0).query, + QStringLiteral("not subject:receipt and " + "from:plain.example.net")); + QCOMPARE(reloaded.rules().at(1).query, + QStringLiteral("(from:vendor.example.org or " + "from:vendor.example.net) and not subject:receipt")); + QCOMPARE(reloaded.rules().at(2).query, + QStringLiteral("from:plain.example.org")); +} + +void TestTagRules::anUnrepresentableRuleOpensInTextMode() +{ + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + // body: is a perfectly good notmuch prefix this builder does not model. + out.write(R"({ + "version": 1, + "rules": [ + {"id": "deep", "query": "body:receipt", + "add": ["deep"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + { + TagRulesDialog dialog; + QVERIFY(dialog.textModeForTest()); + dialog.saveForTest(); + } + + // Unrepresentable is not invalid: it must survive a save untouched. + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 1); + QCOMPARE(reloaded.rules().at(0).query, QStringLiteral("body:receipt")); +} + +void TestTagRules::editingARowRewritesTheQuery() +{ + // The other half of the guarantee: when rows DO change, the stored query + // must follow. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "vendor", "query": "from:vendor.example.org", + "add": ["vendor"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + { + TagRulesDialog dialog; + dialog.setRowValueForTest(0, QStringLiteral("other.example.org")); + dialog.saveForTest(); + } + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 1); + QCOMPARE(reloaded.rules().at(0).query, + QStringLiteral("from:other.example.org")); +} + +void TestTagRules::aTextModeRuleStaysTextWhenAnotherRuleIsVisited() +{ + // The cross-rule question, asked directly. Text mode and m_loadedQuery are + // per-rule state on a dialog that has one set of widgets, so visiting a + // representable rule and coming back must not leave the unrepresentable one + // holding the other rule's mode or its parsed query. Getting that wrong + // recompiles a query the builder never modelled, which is data loss. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "deep", "query": "body:receipt", + "add": ["deep"], "stage": 50, "enabled": true}, + {"id": "plain", "query": "from:plain.example.org", + "add": ["plain"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + { + TagRulesDialog dialog; + QVERIFY(dialog.textModeForTest()); + + dialog.selectRuleForTest(1); + QVERIFY2(!dialog.textModeForTest(), + "a representable rule must return to the builder"); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("from:plain.example.org")); + + dialog.selectRuleForTest(0); + QVERIFY2(dialog.textModeForTest(), + "coming back to an unrepresentable rule must be text again"); + QCOMPARE(dialog.queryLineForTest(), QStringLiteral("body:receipt")); + + dialog.saveForTest(); + } + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 2); + QCOMPARE(reloaded.rules().at(0).query, QStringLiteral("body:receipt")); + QCOMPARE(reloaded.rules().at(1).query, + QStringLiteral("from:plain.example.org")); +} + +void TestTagRules::leavingTextModeIsRefusedWhenTheQueryCannotBeShownAsRows() +{ + // The refusal is the only path that can strand a user, so it is the one + // most worth pinning. It reports through the warning label rather than a + // modal, which is what lets this test exist at all: a modal would block + // here and the branch would ship unverified. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "vendor", "query": "from:vendor.example.org", + "add": ["vendor"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + QVERIFY(!dialog.textModeForTest()); + + dialog.setTextModeForTest(true); + QVERIFY(dialog.textModeForTest()); + + // Type something notmuch accepts and this builder does not model. + dialog.setQueryTextForTest(QStringLiteral("body:receipt")); + dialog.setTextModeForTest(false); + + QVERIFY2(dialog.textModeForTest(), + "the checkbox must refuse to clear: no rows mean this query"); + QVERIFY2(!dialog.warningTextForTest().isEmpty(), + "the refusal must say why, not fail silently"); + + // And a representable query lets the builder back, clearing the warning. + dialog.setQueryTextForTest(QStringLiteral("from:other.example.org")); + dialog.setTextModeForTest(false); + + QVERIFY2(!dialog.textModeForTest(), "a representable query must return"); + QCOMPARE(dialog.rowCountForTest(), 1); + QVERIFY2(dialog.warningTextForTest().isEmpty(), + "a stale refusal must not outlive the query that caused it"); +} + +void TestTagRules::aFolderRowUsesTheDropdownAndKeepsItsSuffix() +{ + // A path: without its suffix matches nothing and notmuch says nothing + // about it, so the suffix must never depend on the user typing it. + QTemporaryDir configHome; + QVERIFY(configHome.isValid()); + qputenv("XDG_CONFIG_HOME", configHome.path().toUtf8()); + QVERIFY(QDir().mkpath(configHome.filePath(QStringLiteral("mailrules")))); + + const QString stored = configHome.filePath( + QStringLiteral("mailrules/rules.json")); + QFile out(stored); + QVERIFY(out.open(QIODevice::WriteOnly)); + out.write(R"({ + "version": 1, + "rules": [ + {"id": "account", "query": "path:\"account-one/**\"", + "add": ["account-one"], "stage": 10, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + dialog.setFolders({QStringLiteral("account-one"), + QStringLiteral("account-two")}); + + // The stored rule round-trips: the row holds the bare name, and the + // query keeps the suffix. + QCOMPARE(dialog.rowCountForTest(), 1); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("path:\"account-one/**\"")); + + dialog.setRowValueForTest(0, QStringLiteral("account-two")); + QCOMPARE(dialog.queryLineForTest(), + QStringLiteral("path:\"account-two/**\"")); + + dialog.saveForTest(); + + TagRules reloaded; + reloaded.load(stored); + QCOMPARE(reloaded.rules().size(), 1); + QCOMPARE(reloaded.rules().at(0).query, + QStringLiteral("path:\"account-two/**\"")); +} + +void TestTagRules::theTextModeToggleSurvivesBeingSwitchedOn() +{ + // The toggle governs the builder, so it must not live INSIDE the builder: + // switching to text mode hides that widget, and a checkbox parented there + // disappears along with the rows, leaving no way back except closing the + // dialog. That shipped in the first draft and a user found it by hand. + // + // Asserting on the checked state alone passes against the bug, because a + // hidden widget still reports its state perfectly well. The question is + // reachability. + 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": "vendor", "query": "from:vendor.example.org", + "add": ["vendor"], "stage": 50, "enabled": true} + ] + })"); + out.close(); + + TagRulesDialog dialog; + QVERIFY(dialog.textModeToggleIsReachableForTest()); + + dialog.setTextModeForTest(true); + QVERIFY2(dialog.textModeToggleIsReachableForTest(), + "the toggle must survive switching to text, or there is no " + "way back to the rows"); + + // And the round trip works, which is the behaviour the user wanted. + dialog.setTextModeForTest(false); + QVERIFY(!dialog.textModeForTest()); + QVERIFY(dialog.textModeToggleIsReachableForTest()); + QCOMPARE(dialog.rowCountForTest(), 1); + QCOMPARE(dialog.queryLineForTest(), + 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); + // 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 + // 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); + + { + TagRulesDialog reopened; + QCOMPARE(reopened.columnWidthForTest(0), 123); + } + + if (previousState.isEmpty()) + qunsetenv("XDG_STATE_HOME"); + else + 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 + // 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); +} + +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()))); +} + +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" |