From 0c5eea8f0d0ccc5b8eb6220814c9e212d6c1ccc2 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:19:58 +0200 Subject: feat(queries): save a query from the UI, and split the buttons off the query row Second half of item 23, on top of the storage change. A query can now be kept without hand-editing a file, and the row of buttons no longer grows without bound. Ctrl+S opens a dialog on whatever is in the query bar, taking a name, an optional account scope and whether the query is pinned. It preselects the account already chosen in the dropdown, since that is the scope the user is looking at, and it says so when a name is about to replace an existing query rather than refusing the name: overwriting a saved query on purpose is a normal edit, and the only thing worth preventing is doing it without noticing. Saving over an entry keeps the stored entry's unknown fields rather than the dialog's fresh value, so a field written by a later build survives being edited here. The saved queries move to a row of their own beneath the query bar, pinned ones as buttons and the rest behind a More queries menu that only exists when something is in it. The ponytail note that stood in the query row predicted exactly this: an unbounded list of buttons sharing the row squeezed the field. Sent moves down with them and is still not a saved query, for the reason already recorded there. A saved query's account scope goes through the account DROPDOWN rather than being baked into the query text. runQuery() already wraps the query in the selected account's path, so pre-scoping here would apply it twice, and setting the dropdown also shows the user which scope they are in. An unscoped query clears the selection rather than inheriting whatever the last one left, which is the same defect the rules preview had. Seven tests, three mutations. Ignoring the pinned flag fails two of them, pre-scoping the text instead of setting the dropdown fails two, and letting an unscoped query inherit the previous account fails one. The menu-absence test initially passed against no implementation at all, since it only asserted a widget was missing; it now proves the row was populated first, which is the guard that class of test needs. Two existing invariants caught real omissions rather than needing adjustment: every registered action must appear in KeyMap::knownActions(), which is what gives it a configurable binding, and every action needs its own icon. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) (limited to 'CHANGELOG.md') diff --git a/CHANGELOG.md b/CHANGELOG.md index e867f02..2d264e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,43 @@ point at which they are stable. ## [Unreleased] +### Added + +- A **Save query** action (`Ctrl+S`) keeps the query in the bar as a saved + query, naming it, optionally scoping it to one account, and choosing whether + it appears as a button or in a menu. Saved queries no longer have to be added + by hand-editing the config file (item 23). +- Saved queries live in `~/.config/qtmaildir/queries.json`, which carries their + **order**, a `pinned` flag and an optional account scope. The order in the + file is the order the buttons appear in, so rearranging them is a matter of + moving lines. + +### Changed + +- Saved queries have a **row of their own** beneath the query bar rather than + sharing it, and the unpinned ones sit behind a **More queries** menu, so the + query field is no longer squeezed by a long list of buttons. +- Saved-query buttons follow the file's order instead of appearing + alphabetically. + +### Upgrading + +Saved queries move out of the `[queries]` section of `qtmaildir.conf` and into +`~/.config/qtmaildir/queries.json`. **The first launch migrates them for you**: +the section is read, the JSON file is written from it, and every entry is +marked pinned so your buttons stay where they were. + +Your config file is left byte-for-byte alone. The old `[queries]` section stays +in it, ignored from then on, and can be deleted by hand whenever you like. It is +not removed automatically because rewriting the file would discard your comments +and reorder your keys. + +One behaviour changes with the move. Buttons used to appear in alphabetical +order and now follow the file. If `[general] startup_query` names a query that +does not exist, the fallback is likewise the first query in the file rather than +the alphabetically first one, so a config that relied on that fallback may open +on a different query than before. + ## [0.17.0] - 2026-08-13 A rule is built from rows now instead of typed into four free-text fields: -- cgit v1.2.3 From 14842cd136e45c551dde4f276af1176bfdf22023 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:29:18 +0200 Subject: fix(queries): put the Save query button beside the query bar The spec asked for "a Save query button beside the search bar" and what shipped was a menu entry and Ctrl+S. The user went looking for the button where the design said it would be and did not find it. Saving is a thing you decide on while looking at the results, so it belongs where the results came from rather than behind a menu or a remembered chord. The button takes the action through setDefaultAction rather than a second connect, so it inherits the text, icon, tooltip and enabled state and cannot end up offering to save an empty query while the menu entry correctly refuses. The mutation that replaces it with a plain clicked() connection fails the test. Also records item 82: a saved query cannot be edited, unpinned or deleted from the UI. Item 23 specified saving and nothing else, and that is exactly what was built, so the only way to unpin a query is a text editor or retyping it in full under the same name. An action that creates something the UI cannot then change or remove is incomplete, and this was found within minutes of the first hand test. It is filed as a defect rather than an enhancement, and the spec now says so where a reader would otherwise take the design for complete. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 +++-- .../plans/2026-08-03-post-0.1.0-usability.md | 44 ++++++++++++++++++++++ .../specs/2026-08-13-saved-queries-design.md | 11 ++++++ src/mainwindow.cpp | 17 +++++++++ src/mainwindow.h | 3 ++ tests/test_mainwindow.cpp | 34 +++++++++++++++++ 6 files changed, 114 insertions(+), 4 deletions(-) (limited to 'CHANGELOG.md') diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d264e2..428e3e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,10 +13,11 @@ point at which they are stable. ### Added -- A **Save query** action (`Ctrl+S`) keeps the query in the bar as a saved - query, naming it, optionally scoping it to one account, and choosing whether - it appears as a button or in a menu. Saved queries no longer have to be added - by hand-editing the config file (item 23). +- A **Save query** button beside the query bar, also on the Edit menu and bound + to `Ctrl+S`, keeps the query in the bar as a saved query: naming it, + optionally scoping it to one account, and choosing whether it appears as a + button or in a menu. Saved queries no longer have to be added by + hand-editing the config file (item 23). - Saved queries live in `~/.config/qtmaildir/queries.json`, which carries their **order**, a `pinned` flag and an optional account scope. The order in the file is the order the buttons appear in, so rearranging them is a matter of 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 268dbb4..9d19e4d 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 @@ -146,6 +146,7 @@ taking that too literally. | 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 | | 81 | No way to turn a saved query into a tagging rule | workflow | S | open; depends on 23, which builds the dialog, and is excluded from its spec on purpose. Writes to the shared rules file, so it spans this repo and `mailctl` | +| 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | open; found by hand-testing item 23 on 2026-08-13. Saving works, unsaving does not | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -574,6 +575,49 @@ failing silently. **Size: S** on top of 23, and not meaningful before it. +## 82. A saved query cannot be edited, unpinned or deleted from the UI + +**Observed (user, 2026-08-13):** hand-testing item 23. The user saved a query, +then asked how to unpin it, and there is no answer that does not involve either +a text editor or retyping the whole query. + +**Cause:** item 23 specified saving and nothing else, and that is exactly what +shipped. `SaveQueryDialog` opens on the contents of the query BAR, not on a +stored entry, so the only route to changing one field of an existing query is to +reconstruct the whole query, name it identically, and let +`MainWindow::saveCurrentQuery()` replace it by name. There is no delete at any +price: nothing in the UI removes an entry from `queries.json`. + +This is a defect rather than a missing enhancement. An action that creates +something the UI cannot then edit or remove is incomplete, and the user hit it +within minutes of the first hand test. + +**Approach.** A context menu on a saved-query button and on each **More +queries** entry, offering Edit, Unpin (or Pin) and Delete. + +- **Edit** opens `SaveQueryDialog` prefilled from the STORED entry rather than + from the query bar. The dialog already carries every field it needs; what it + lacks is a constructor that takes a `SavedQuery`. +- **Unpin** is a one-field write and does not need the dialog at all. +- **Delete** removes the entry and rewrites the file. + +**Constraints.** + +- `saveCurrentQuery()` already merges an existing entry's `unknown` fields over + the dialog's fresh value, and every one of these paths must do the same or a + field written by a later build is dropped by an edit here. +- Renaming through Edit is a rename, not a second entry: match on the name the + dialog was OPENED with, not the one it returns, or renaming silently creates a + duplicate and leaves the original behind. +- Delete is destructive and the file is user config, so it is one of the few + places in this application that wants a confirmation. The no-confirmation rule + in CLAUDE.md is about tag mutations, which are undoable through the undo + stack; this is not on that stack and cannot be undone. +- A test must exercise every route the way item 75's did not: the dialog's + Cancel goes through `done(int)` and never sends a `QCloseEvent`. + +**Size: S**, and it should land before the saved-query work is called done. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/docs/superpowers/specs/2026-08-13-saved-queries-design.md b/docs/superpowers/specs/2026-08-13-saved-queries-design.md index 6d0019f..ba6b23e 100644 --- a/docs/superpowers/specs/2026-08-13-saved-queries-design.md +++ b/docs/superpowers/specs/2026-08-13-saved-queries-design.md @@ -160,6 +160,17 @@ Sent stays where it is. It is not a saved query, it is built from would mean generating a per-account path query into stored config, which is the duplication the `account`-key decision just rejected. +## Editing and deleting are NOT here, and that is a defect + +This document specifies creating a saved query and says nothing about changing +or removing one. That gap shipped: the first hand test produced "how do I unpin +a query?", and the honest answer was a text editor. Recorded as **item 82**, +sized S, and it should land before this work is called finished. + +Anything built there must merge the stored entry's `unknown` fields the way +`saveCurrentQuery()` does, and must match a rename on the name the dialog was +opened with rather than the one it returns. + ## What is deliberately not here **Item 81, saving a query as a tagging rule.** A saved query is a view and costs diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 3966e9f..9eff3d5 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -45,6 +45,7 @@ #include #include #include +#include #include #include "mailsync.h" @@ -348,6 +349,12 @@ MainWindow::MainWindow(const Config &config, QWidget *parent) // also set programmatically, by the saved-query buttons and by // recoverStaleThread(), and the action must track those too. if (QAction *save = m_actions.value(QStringLiteral("save_query"))) { + // setDefaultAction, not a second connect: the button then takes the + // action's text, icon, tooltip and ENABLED state, so it cannot end up + // offering to save an empty query while the menu entry refuses. + m_saveQueryButton->setDefaultAction(save); + m_saveQueryButton->setToolButtonStyle(Qt::ToolButtonIconOnly); + auto updateSaveState = [this, save]() { save->setEnabled(!m_queryEdit->text().trimmed().isEmpty()); }; @@ -566,6 +573,16 @@ void MainWindow::buildUi() queryRow->addWidget(m_accountBox); queryRow->addWidget(m_sortOrder); queryRow->addWidget(m_queryEdit, 1); + + // Beside the field, where a user looks for it. The menu entry and Ctrl+S + // were not enough on their own: saving is a thing you decide on while + // looking at the results, so it needs to be visible at the query bar + // rather than remembered. Created here and given its action in the + // constructor, since registerActions() has not run yet. + m_saveQueryButton = new QToolButton(central); + m_saveQueryButton->setObjectName(QStringLiteral("saveQueryButton")); + queryRow->addWidget(m_saveQueryButton); + layout->addLayout(queryRow); buildSavedQueryRow(central, layout); diff --git a/src/mainwindow.h b/src/mainwindow.h index 8b43fd0..e6ee322 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -49,6 +49,7 @@ class QPlainTextEdit; class QSplitter; class QProgressBar; class QTimer; +class QToolButton; class QVBoxLayout; class ThreadListModel; @@ -625,6 +626,8 @@ private: QUndoStack m_undoStack; QLineEdit *m_queryEdit = nullptr; + /// Save query, beside the field. Driven by the save_query action. + QToolButton *m_saveQueryButton = nullptr; QueryCompleter *m_queryCompleter = nullptr; /// Its own type, not the QTreeView base. The strip painting and the /// expander column are ThreadListView's, and holding the base here only diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 2c70e0f..581daae 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -195,6 +195,7 @@ private slots: void aScopedSavedQuerySelectsItsAccount(); void anUnscopedSavedQueryClearsTheAccount(); void theSaveQueryActionIsDisabledOnAnEmptyQuery(); + void thereIsASaveButtonBesideTheQueryBar(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -5540,4 +5541,37 @@ void TestMainWindow::theSaveQueryActionIsDisabledOnAnEmptyQuery() QVERIFY(!save->isEnabled()); } +/// A menu entry and a shortcut are not a button. The spec asks for one beside +/// the query bar, and the user went looking for it there and did not find it. +void TestMainWindow::thereIsASaveButtonBesideTheQueryBar() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, "queries": [] + })")); + + MainWindow window(config); + auto *button = + window.findChild(QStringLiteral("saveQueryButton")); + QVERIFY2(button, "no Save query button beside the query bar"); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + // In the query row itself, not somewhere else in the window that a + // findChild would also reach. + QCOMPARE(button->parentWidget(), queryEdit->parentWidget()); + + // Follows the action, so it cannot offer to save an empty query while the + // menu entry correctly refuses. + queryEdit->clear(); + QVERIFY2(!button->isEnabled(), + "the button must follow the action's enabled state"); + queryEdit->setText(QStringLiteral("tag:inbox")); + QVERIFY(button->isEnabled()); +} + #include "test_mainwindow.moc" -- cgit v1.2.3 From 97c81f8cad571ce9ce724ddab8269e911df05a7c Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:52:13 +0200 Subject: feat(queries): make Sent a saved query rather than a fixed button The user asked whether the default queries could be unified with Sent. The answer runs the other way: Sent joins the saved queries rather than the saved queries becoming hardcoded. Inbox, Unread and Important are complete strings that depend on nothing and can never go stale, so generating them would buy nothing and would cost the four things the file just gained: reordering, unpinning, renaming and deleting. Hardcoding them would also make them undeletable, which is a regression for anyone who does not want one of them. Sent is different only in that its query CANNOT be stored: it is composed from every account's `sent` key, so a stored copy goes stale the moment a folder is renamed. That is a property of Sent, not of "default queries". Storing the GENERATOR rather than its output keeps both halves: `"generated": "sent"` still resolves from the accounts at click time, and the entry is an ordinary row that can be reordered, renamed, unpinned or removed. The row now follows one rule instead of carrying one member the user did not own. Two properties had to travel with the entry. The composed query, resolved through Config::resolvedQuery() so what lands in the bar is what actually ran; and FLAT mode, since a sent view lists messages and a threaded one folds every reply back into the conversation the user sent one message into. The sent generator implies flat rather than trusting the file to say so, because a hand-edited row would otherwise produce a threaded sent view. An unknown generator is reported but the row is KEPT: a later build may know it, and dropping it here would delete it from the file on the next save, which is the same data loss the unknown-field handling exists to prevent. A generator whose accounts configure nothing is skipped entirely, exactly as the hardcoded button was hidden rather than offering one that finds nothing. Eight new tests. The four pre-existing Sent tests reach this through migration and were left alone, which is what proves the migrated path still behaves; the new ones cover a STORED file, which is the path every launch after the first takes. Mutations: a generator resolving to nothing fails three, ignoring flat fails two, and not skipping an empty generator fails one. A rename test guards the property the change exists for, since anything keyed on the literal name "Sent" would break it. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 12 +++- README.md | 28 +++++++-- src/config.cpp | 66 +++++++++++++++++++- src/config.h | 21 +++++++ src/mainwindow.cpp | 50 ++++++++------- tests/test_config.cpp | 156 ++++++++++++++++++++++++++++++++++++++++++++++ tests/test_mainwindow.cpp | 111 +++++++++++++++++++++++++++++++++ 7 files changed, 413 insertions(+), 31 deletions(-) (limited to 'CHANGELOG.md') diff --git a/CHANGELOG.md b/CHANGELOG.md index 428e3e5..b384eda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,11 @@ point at which they are stable. **order**, a `pinned` flag and an optional account scope. The order in the file is the order the buttons appear in, so rearranging them is a matter of moving lines. +- **Sent is a saved query now**, carrying `"generated": "sent"` instead of a + stored query. It is still composed from your accounts' `sent` keys every time + you click it, so correcting a folder name still updates it with no edit, but + it can now be reordered, renamed, unpinned or deleted like any other entry + rather than being a fixed button you did not own. ### Changed @@ -30,13 +35,18 @@ point at which they are stable. query field is no longer squeezed by a long list of buttons. - Saved-query buttons follow the file's order instead of appearing alphabetically. +- The Sent button is no longer hardcoded beside the saved queries, so the whole + row now follows one rule instead of having one member that behaved + differently from its neighbours. ### Upgrading Saved queries move out of the `[queries]` section of `qtmaildir.conf` and into `~/.config/qtmaildir/queries.json`. **The first launch migrates them for you**: the section is read, the JSON file is written from it, and every entry is -marked pinned so your buttons stay where they were. +marked pinned so your buttons stay where they were. Sent is appended as a +`generated` entry, where its button already sat, provided an account configures +a sent folder. Your config file is left byte-for-byte alone. The old `[queries]` section stays in it, ignored from then on, and can be deleted by hand whenever you like. It is diff --git a/README.md b/README.md index 81ec7d8..749d89d 100644 --- a/README.md +++ b/README.md @@ -223,16 +223,36 @@ queries** menu, which keeps the row usable once you have more than a handful. same as choosing that account in the dropdown; leave it out for a query that spans every account. +**Sent is an entry like any other**, and the one that carries `generated` +instead of `query`: + +```json +{ "name": "Sent", "generated": "sent", "pinned": true } +``` + +A generated query is composed from your accounts every time you click it, +rather than stored. That is why Sent has no `query` of its own: it is built +from every account's `sent` key, so adding an account or correcting a folder +name updates the button with no edit here. A stored copy of the same string +would quietly go on naming the old folder. + +Being an ordinary entry, it can be reordered, renamed, unpinned or deleted like +the rest. Renaming it to `Posta inviata` changes only the label. `sent` is the +only generator today, and it is skipped entirely when no account configures a +sent folder, rather than offering a button that finds nothing. + The name is what the button says, so `Important` and `Flagged` can run the same query and differ only in the label. **Upgrading from 0.17.0 or earlier.** Saved queries used to live in a `[queries]` section of `qtmaildir.conf`. The first launch after upgrading reads that section, writes `queries.json` from it, and marks every entry pinned so -your buttons stay where they were. Your config file is not modified: the old -`[queries]` section is left exactly as it is, ignored from then on, and you can -delete it by hand whenever you like. The reason it is not removed for you is -that rewriting the file would drop your comments and reorder your keys. +your buttons stay where they were. Sent is added as a `generated` entry at the +end, where its button already sat, provided an account configures a sent +folder. Your config file is not modified: the old `[queries]` section is left +exactly as it is, ignored from then on, and you can delete it by hand whenever +you like. The reason it is not removed for you is that rewriting the file would +drop your comments and reorder your keys. One behaviour changes with the move. Buttons used to appear in alphabetical order, because the INI backend returns keys sorted and preserving file order diff --git a/src/config.cpp b/src/config.cpp index 8cd5e56..9edba50 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -51,6 +51,14 @@ constexpr int kMaxToolbarIconSize = 64; /// two-repo change and no hook stops tagging if it is half-deployed. constexpr int kQueriesFormatVersion = 1; +/// Generators a saved query may name in its `generated` field. +/// +/// A closed set, checked on load so a typo is reported rather than producing a +/// button that silently finds nothing. Adding one here needs no format bump: +/// an older build keeps the row and reports it, which is why an unknown +/// generator is a problem rather than a reason to drop the entry. +const QStringList kQueryGenerators = { QStringLiteral("sent") }; + } // namespace QString Account::scopedQuery(const QString &query) const @@ -457,9 +465,26 @@ void Config::loadSavedQueries(const QString &configPath, QSettings &settings) } settings.endGroup(); + // Sent was a hardcoded button beside the saved queries and becomes an + // ordinary row here, so it can be reordered, renamed, unpinned or + // removed like any other. It stays GENERATED, so it still follows the + // accounts. Appended last, where the button already sat. + // + // Only when an account actually configures a sent folder: the button + // was hidden entirely otherwise, and migrating a row that always finds + // nothing would be worse than what it replaces. + if (!allSentQuery().isEmpty()) { + SavedQuery sent; + sent.name = QStringLiteral("Sent"); + sent.generated = QStringLiteral("sent"); + sent.pinned = true; + sent.flat = true; + m_savedQueries.append(sent); + } + // Order is alphabetical here because childKeys() is genuinely all the // INI knows. The user reorders once and it sticks from then on. - if (!names.isEmpty() && !saveSavedQueries()) { + if (!m_savedQueries.isEmpty() && !saveSavedQueries()) { addProblem(QStringLiteral("Could not write saved queries to %1.") .arg(m_queriesPath)); } @@ -516,6 +541,26 @@ void Config::loadSavedQueries(const QString &configPath, QSettings &settings) query.query = object.value(QStringLiteral("query")).toString(); query.pinned = object.value(QStringLiteral("pinned")).toBool(false); query.account = object.value(QStringLiteral("account")).toString(); + query.generated = object.value(QStringLiteral("generated")).toString(); + // A generator carries its own view mode, so "sent" is flat whether or + // not the file says so. Storing it as a plain field would let a + // hand-edited or migrated-from-elsewhere row produce a THREADED sent + // view, which folds every reply back into the conversation the user + // sent one message into. The file may still set it for an ordinary + // query. + query.flat = object.value(QStringLiteral("flat")).toBool(false) + || query.generated == QStringLiteral("sent"); + + if (query.isGenerated() + && !kQueryGenerators.contains(query.generated)) { + // Reported but KEPT. A later build may know this generator, and + // dropping the row here would delete it from the file on the next + // save, which is the same data loss the unknown-field handling + // exists to prevent. + addProblem(QStringLiteral("Saved query '%1' uses an unknown " + "generator '%2' and will find nothing.") + .arg(query.name, query.generated)); + } if (query.name.isEmpty()) { addProblem(QStringLiteral("A saved query in %1 has no name and was " @@ -526,7 +571,8 @@ void Config::loadSavedQueries(const QString &configPath, QSettings &settings) for (auto it = object.begin(); it != object.end(); ++it) { static const QStringList known = { QStringLiteral("name"), QStringLiteral("query"), - QStringLiteral("pinned"), QStringLiteral("account") + QStringLiteral("pinned"), QStringLiteral("account"), + QStringLiteral("generated"), QStringLiteral("flat") }; if (!known.contains(it.key())) query.unknown.insert(it.key(), it.value()); @@ -550,6 +596,10 @@ bool Config::saveSavedQueries() const object.insert(QStringLiteral("pinned"), true); if (!query.account.isEmpty()) object.insert(QStringLiteral("account"), query.account); + if (query.isGenerated()) + object.insert(QStringLiteral("generated"), query.generated); + if (query.flat) + object.insert(QStringLiteral("flat"), true); for (auto it = query.unknown.begin(); it != query.unknown.end(); ++it) object.insert(it.key(), it.value()); array.append(object); @@ -572,6 +622,18 @@ bool Config::saveSavedQueries() const QString Config::resolvedQuery(const SavedQuery &query) const { + // Composed from the accounts every time it is asked for, which is the + // point: the answer follows the config rather than a copy of it taken when + // the entry was written. + if (query.isGenerated()) { + if (query.generated == QStringLiteral("sent")) + return allSentQuery(); + // An unknown generator was reported on load. Empty rather than the + // bare stored query, which for a generated entry is empty anyway and + // would otherwise run as "match everything". + return QString(); + } + if (query.account.isEmpty()) return query.query; diff --git a/src/config.h b/src/config.h index 4211b0c..b9ee9d6 100644 --- a/src/config.h +++ b/src/config.h @@ -122,6 +122,27 @@ struct SavedQuery /// user edits it. Resolve through Config::resolvedQuery(). QString account; + /// Names a builtin that COMPOSES this query from the accounts at run time, + /// rather than storing it. Empty for an ordinary query. + /// + /// "sent" is the only one today. Its query is built from every account's + /// `sent` key, so adding an account or correcting a folder name is a config + /// edit and nothing else; a stored copy of the same string would go stale + /// silently. That property is why Sent used to be hardcoded beside the + /// saved queries instead of living with them, which left one button on the + /// row that could not be reordered, renamed, unpinned or removed. + /// + /// Storing the GENERATOR rather than its output keeps both: the query stays + /// live, and the entry is an ordinary row the user owns. + QString generated; + + /// Lists messages rather than threads. Set for the sent view, where a + /// thread would fold every reply back into the conversation the user sent + /// one message into. + bool flat = false; + + bool isGenerated() const { return !generated.isEmpty(); } + /// Keys this build does not understand, preserved verbatim so a file /// written by a later version survives a save from this one. QJsonObject unknown; diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index f06b308..62f4751 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1673,40 +1673,34 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) auto *box = new QHBoxLayout(row); box->setContentsMargins(0, 0, 0, 0); + // Sent is an ordinary row here, not a hardcoded button beside the others. + // It is still GENERATED, so its query is composed from the accounts' `sent` + // keys at click time and correcting a folder name stays a config edit and + // nothing else; what changed is that the entry can now be reordered, + // renamed, unpinned or removed like every other, instead of being the one + // control on the row the user did not own. QList unpinned; for (const SavedQuery &saved : m_config.savedQueries()) { + // A generator whose accounts configure nothing produces a button that + // always finds nothing. Skipped entirely, which is what the hardcoded + // Sent button did and is worth keeping. + if (saved.isGenerated() && m_config.resolvedQuery(saved).isEmpty()) + continue; + if (!saved.pinned) { unpinned.append(saved); continue; } auto *button = new QPushButton(saved.name, row); + // The generated entries keep a stable object name so a test can find + // the sent button without depending on what the user renamed it to. + if (saved.generated == QStringLiteral("sent")) + button->setObjectName(QStringLiteral("sentButton")); connect(button, &QPushButton::clicked, this, [this, saved]() { runSavedQuery(saved); }); box->addWidget(button); } - // Sent sits with the saved queries and is not one: its query is COMPOSED - // from the accounts' `sent` keys at click time, so adding an account or - // correcting a folder name is a config edit and nothing else. A stored - // entry holding the same string would go stale silently, and could not - // narrow to the selected account the way this does through - // runCurrentQuery()'s existing scope wrap. - // - // Hidden entirely when no account configures a sent folder, rather than - // offering a button that always finds nothing. - if (!m_config.allSentQuery().isEmpty()) { - auto *sentButton = new QPushButton(tr("Sent"), row); - sentButton->setObjectName(QStringLiteral("sentButton")); - connect(sentButton, &QPushButton::clicked, this, [this]() { - m_queryEdit->setText(m_config.allSentQuery()); - // Flat for this query only. runCurrentQuery() clears it again for - // anything else, including the same query typed by hand, so the - // flag cannot outlive the button that set it. - runQuery(FlatResult::Yes); - }); - box->addWidget(sentButton); - } - // Everything above is left-aligned; the stretch here pushes what follows // to the right edge. The buttons are the row's content and read as a set, // while the overflow menu is a control over that set, so it sits apart @@ -1751,8 +1745,16 @@ void MainWindow::runSavedQuery(const SavedQuery &saved) if (index >= 0) m_accountBox->setCurrentIndex(index); - m_queryEdit->setText(saved.query); - runCurrentQuery(); + // A generated entry has no stored query: the text is composed from the + // accounts now, so what lands in the bar is what actually ran and the user + // can see and edit it. + m_queryEdit->setText(saved.isGenerated() ? m_config.resolvedQuery(saved) + : saved.query); + + // Flat for this query only. runQuery() sets the mode on EVERY run, so the + // flag cannot outlive the entry that asked for it, including for the same + // query typed by hand afterwards. + runQuery(saved.flat ? FlatResult::Yes : FlatResult::No); } void MainWindow::saveCurrentQuery() diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 8024728..dfe463b 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -62,6 +62,11 @@ private slots: void futureVersionIsRefusedAndReported(); void startupQueryFallsBackToDocumentOrder(); void scopedSavedQueryParenthesisesADisjunction(); + void aGeneratedQueryResolvesFromTheAccounts(); + void aGeneratedQueryTracksAConfigChange(); + void anUnknownGeneratorResolvesToNothingAndReports(); + void migrationAddsSentWhenAnAccountHasOne(); + void migrationAddsNoSentWithoutTheKey(); void generalSectionKeysAreActuallyRead(); void messageZoomDefaultsAndValidates(); void messageZoomOutOfRangeIsReported(); @@ -1416,5 +1421,156 @@ void TestConfig::scopedSavedQueryParenthesisesADisjunction() QCOMPARE(config.resolvedQuery(orphan), QStringLiteral("tag:inbox")); } +// --------------------------------------------------------------------------- +// Generated saved queries +// --------------------------------------------------------------------------- + +static QString twoAccountsWithSent() +{ + return QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + "sent=Sent\n" + "\n" + "[account.personal]\n" + "name=Test User\n" + "address=me@example.net\n" + "maildir=personal\n" + "sent=[Provider]/Posta inviata\n" + ); +} + +void TestConfig::aGeneratedQueryResolvesFromTheAccounts() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, twoAccountsWithSent()); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Sent", "generated": "sent", "pinned": true } + ] + })")); + + Config config; + config.load(path); + + const SavedQuery sent = config.savedQueries().at(0); + QVERIFY(sent.isGenerated()); + // The stored query is empty; the text comes from the accounts. + QVERIFY(sent.query.isEmpty()); + QCOMPARE(config.resolvedQuery(sent), config.allSentQuery()); + QVERIFY(config.resolvedQuery(sent).contains( + QStringLiteral("path:\"work-mail/Sent/**\""))); + // The quotes matter: "[" and "]" are Xapian syntax and an unquoted term + // is parsed rather than matched. + QVERIFY(config.resolvedQuery(sent).contains( + QStringLiteral("path:\"personal/[Provider]/Posta inviata/**\""))); + + // Flat, not threaded: a sent view lists messages, and that property has to + // travel with the entry or it is lost the moment Sent is a stored row. + QVERIFY(sent.flat); +} + +/// The whole reason Sent is generated rather than stored. A stored copy would +/// keep naming an account that has been renamed or a folder that has moved. +void TestConfig::aGeneratedQueryTracksAConfigChange() +{ + QTemporaryDir dir; + const QString queries = QStringLiteral(R"({ + "version": 1, + "queries": [ { "name": "Sent", "generated": "sent" } ] + })"); + + const QString before = writeIni(dir, twoAccountsWithSent()); + writeQueries(dir, queries); + Config first; + first.load(before); + const QString firstResolved = first.resolvedQuery(first.savedQueries().at(0)); + + // The user corrects a folder name. Nothing in queries.json changes. + QTemporaryDir second; + const QString after = writeIni(second, QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + "sent=Sent Items\n" + )); + writeQueries(second, queries); + Config later; + later.load(after); + const QString laterResolved = later.resolvedQuery(later.savedQueries().at(0)); + + QVERIFY(firstResolved != laterResolved); + QVERIFY(laterResolved.contains(QStringLiteral("Sent Items"))); +} + +void TestConfig::anUnknownGeneratorResolvesToNothingAndReports() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, twoAccountsWithSent()); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ { "name": "Future", "generated": "not_a_generator" } ] + })")); + + Config config; + config.load(path); + + // Kept rather than dropped: a later build may know this generator, and + // silently deleting the row on save would lose it. + QCOMPARE(config.savedQueries().size(), 1); + QVERIFY(config.resolvedQuery(config.savedQueries().at(0)).isEmpty()); + QVERIFY2(!config.problems().isEmpty(), + "an unknown generator must be reported, not silently inert"); +} + +void TestConfig::migrationAddsSentWhenAnAccountHasOne() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, twoAccountsWithSent() + + QStringLiteral( + "\n[queries]\n" + "Inbox=tag:inbox\n" + )); + + Config config; + config.load(path); + + const QList queries = config.savedQueries(); + QCOMPARE(queries.size(), 2); + // Last, where the button already sat: after the saved queries. + QCOMPARE(queries.at(1).name, QStringLiteral("Sent")); + QVERIFY(queries.at(1).isGenerated()); + QVERIFY(queries.at(1).pinned); + QVERIFY(queries.at(1).flat); +} + +/// Today the button is hidden entirely when no account configures a sent +/// folder, rather than offering one that always finds nothing. The migration +/// must not invent a row that would do exactly that. +void TestConfig::migrationAddsNoSentWithoutTheKey() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + "\n" + "[queries]\n" + "Inbox=tag:inbox\n" + )); + + Config config; + config.load(path); + + const QList queries = config.savedQueries(); + QCOMPARE(queries.size(), 1); + QCOMPARE(queries.at(0).name, QStringLiteral("Inbox")); +} + QTEST_MAIN(TestConfig) #include "test_config.moc" diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 42d7d78..883e9a7 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -200,6 +200,9 @@ private slots: void thereIsASaveButtonBesideTheQueryBar(); void theMenuIsRightAlignedAwayFromTheButtons(); void theRowSurvivesWithNothingButUnpinnedQueries(); + void aStoredGeneratedQueryRunsFlatAndComposed(); + void aRenamedSentEntryKeepsWorking(); + void aGeneratedQueryWithNothingToShowIsSkipped(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -5664,4 +5667,112 @@ void TestMainWindow::theRowSurvivesWithNothingButUnpinnedQueries() QCOMPARE(menuButton->menu()->actions().size(), 2); } +static QString oneAccountWithSent() +{ + return QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + "sent=Sent\n" + ); +} + +/// The existing Sent tests reach the generated entry through MIGRATION, since +/// their configs have no queries.json. This one starts from a stored file, so +/// it covers the path a user is on from the second launch onwards. +void TestMainWindow::aStoredGeneratedQueryRunsFlatAndComposed() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Sent", "generated": "sent", "pinned": true } + ] + })"), oneAccountWithSent()); + + MainWindow window(config); + auto *button = + window.findChild(QStringLiteral("sentButton")); + QVERIFY(button); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + auto *model = window.findChild(); + QVERIFY(model); + QVERIFY2(!model->flatMode(), "the model starts threaded"); + + button->click(); + + // Composed from the account, not read from the file: the entry stores no + // query at all. + QCOMPARE(queryEdit->text(), config.allSentQuery()); + QVERIFY(queryEdit->text().contains( + QStringLiteral("path:\"work-mail/Sent/**\""))); + QVERIFY2(model->flatMode(), + "a sent view must be flat, or replies fold back into the thread"); +} + +/// The point of the change: Sent is the user's row now. Renaming it must not +/// break it, which it would if anything keyed on the literal name "Sent". +void TestMainWindow::aRenamedSentEntryKeepsWorking() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Posta inviata", "generated": "sent", "pinned": true } + ] + })"), oneAccountWithSent()); + + MainWindow window(config); + const QStringList labels = savedQueryButtonLabels(window); + QCOMPARE(labels, QStringList{ QStringLiteral("Posta inviata") }); + + auto *button = + window.findChild(QStringLiteral("sentButton")); + QVERIFY2(button, "the generated entry lost its identity when renamed"); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + button->click(); + QCOMPARE(queryEdit->text(), config.allSentQuery()); +} + +/// The hardcoded button was hidden entirely when no account configured a sent +/// folder, rather than offering one that always finds nothing. A stored row +/// must behave the same way. +void TestMainWindow::aGeneratedQueryWithNothingToShowIsSkipped() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Sent", "generated": "sent", "pinned": true } + ] + })"), QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + )); + + MainWindow window(config); + + // The guard: the row was built and the other entry did get a button, so a + // missing Sent means it was skipped rather than that nothing was built. + QCOMPARE(savedQueryButtonLabels(window), + QStringList{ QStringLiteral("Inbox") }); + QVERIFY2(!window.findChild(QStringLiteral("sentButton")), + "a generated query with nothing to show must not get a button"); +} + #include "test_mainwindow.moc" -- cgit v1.2.3 From a7872cf56b7f313881f0d5e50d548ffdb5ba68b9 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 20:07:31 +0200 Subject: feat(queries): edit, pin and delete a saved query from the UI Item 82. Saving a query worked and nothing else did: changing one field meant retyping the whole query under the same name, and deleting one meant editing the file by hand. An action that creates something the UI cannot then change or remove is incomplete, and the user hit it within minutes of the first hand test. Right-clicking a saved query, on its button or its menu entry, now offers Edit, Move to menu / Show as a button, and Delete. Every path funnels through one replaceSavedQuery(), which matches on the name the dialog was OPENED with rather than the one it returns, so a rename replaces the entry instead of leaving the original behind beside a new one, and which merges the stored entry's unknown fields in a single place rather than in three. Delete confirms first: the rule against confirmation dialogs covers tag mutations, which the undo stack can take back, and this writes user config that it cannot. Two cases the item did not anticipate. A generated entry has no query to edit, so the dialog shows its composed query read-only rather than offering a field that changes nothing, and carries `generated` and `flat` through an edit rather than letting it decay into a plain entry holding a snapshot of what it resolved to today. And the overwrite notice had to learn to ignore the entry being edited, since warning that "Inbox" already exists while editing Inbox is noise. This also fixes a defect that predated it and was already reachable from the save path. rebuildSavedQueryRow() called deleteLater() on the old row, which defers destruction to the event loop, so the stale row went on answering findChild() and every lookup after a rebuild reported the state from before the edit. Nothing looked wrong on screen, which is why it surfaced only as three tests failing against a row that had in fact been rebuilt correctly. Five tests, three mutations. Matching on the returned name fails two, never writing the file fails three, and dropping the unknown-field merge fails one. That last one initially proved nothing: it drove UNPIN, which copies the stored entry and so carries `unknown` along by itself, and passed with the merge deleted. It now goes through the edit path with a replacement that has none, which is what the dialog actually returns. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 + README.md | 6 + .../plans/2026-08-03-post-0.1.0-usability.md | 26 ++- src/mainwindow.cpp | 113 ++++++++++++ src/mainwindow.h | 36 ++++ src/savequerydialog.cpp | 60 ++++++- src/savequerydialog.h | 16 ++ tests/test_mainwindow.cpp | 200 +++++++++++++++++++++ 8 files changed, 455 insertions(+), 8 deletions(-) (limited to 'CHANGELOG.md') diff --git a/CHANGELOG.md b/CHANGELOG.md index b384eda..5b14938 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,12 @@ point at which they are stable. it can now be reordered, renamed, unpinned or deleted like any other entry rather than being a fixed button you did not own. +- **Right-click a saved query** to edit, pin, unpin or delete it. A saved query + could previously be created and never changed: the only route to adjusting one + field was to retype the whole query under the same name, and there was no way + to delete one at all short of editing the file (item 82). Deleting asks first, + since it writes your config and is not undoable. + ### Changed - Saved queries have a **row of their own** beneath the query bar rather than diff --git a/README.md b/README.md index 749d89d..9148923 100644 --- a/README.md +++ b/README.md @@ -244,6 +244,12 @@ sent folder, rather than offering a button that finds nothing. The name is what the button says, so `Important` and `Flagged` can run the same query and differ only in the label. +**Right-click a saved query** (a button, or its entry in the menu) to edit it, +move it between the row and the menu, or delete it. Deleting asks first: it +rewrites this file and there is no undo for it. Editing a generated entry shows +its composed query read-only, since that one is built from your accounts rather +than stored. + **Upgrading from 0.17.0 or earlier.** Saved queries used to live in a `[queries]` section of `qtmaildir.conf`. The first launch after upgrading reads that section, writes `queries.json` from it, and marks every entry pinned so 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 9d19e4d..5e97337 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 @@ -146,7 +146,7 @@ taking that too literally. | 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 | | 81 | No way to turn a saved query into a tagging rule | workflow | S | open; depends on 23, which builds the dialog, and is excluded from its spec on purpose. Writes to the shared rules file, so it spans this repo and `mailctl` | -| 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | open; found by hand-testing item 23 on 2026-08-13. Saving works, unsaving does not | +| 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | **done** 2026-08-13 on `saved-queries`, unreleased. Right-click offers Edit, Pin/Unpin and Delete | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -618,6 +618,30 @@ queries** entry, offering Edit, Unpin (or Pin) and Delete. **Size: S**, and it should land before the saved-query work is called done. +**Done 2026-08-13.** A context menu on each button and each menu entry, with +Edit, Move to menu / Show as a button, and Delete. Every path goes through one +`replaceSavedQuery()`, matched on the name the dialog was OPENED with, so a +rename replaces rather than duplicating, and merging the stored entry's unknown +fields in one place rather than three. + +Two things the approach above did not anticipate. A GENERATED entry has no +query to edit, so the dialog shows its composed query read-only rather than +offering a field that changes nothing, and carries `generated` and `flat` +through an edit rather than letting it decay into a plain entry holding a +snapshot. And the overwrite notice had to learn to ignore the entry being +edited: warning that "Inbox" already exists while editing Inbox is noise. + +It also exposed a defect that predated it. `rebuildSavedQueryRow()` called +`deleteLater()` on the old row, which defers destruction to the event loop, so +the stale row went on answering `findChild()` and every lookup after a rebuild +saw the state from before the edit. It was already reachable from the save path. +Fixed by reparenting the row out immediately. + +The unknown-fields test initially passed against the merge being deleted: it +drove UNPIN, which copies the stored entry and therefore carries `unknown` +along by itself. It now goes through the edit path with a replacement that has +none, which is what the dialog actually returns. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 62f4751..14e4202 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1698,6 +1698,7 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) button->setObjectName(QStringLiteral("sentButton")); connect(button, &QPushButton::clicked, this, [this, saved]() { runSavedQuery(saved); }); + addSavedQueryActions(button, saved); box->addWidget(button); } @@ -1718,6 +1719,12 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) QAction *action = menu->addAction(saved.name); connect(action, &QAction::triggered, this, [this, saved]() { runSavedQuery(saved); }); + // A menu entry has no context menu of its own, so its own submenu + // carries the same three actions; an unpinned query would + // otherwise be the one thing that cannot be edited or deleted. + auto *entryMenu = new QMenu(menu); + addSavedQueryActions(entryMenu, saved); + action->setMenu(entryMenu); } menuButton->setMenu(menu); box->addWidget(menuButton); @@ -1733,6 +1740,105 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) row->hide(); } +void MainWindow::addSavedQueryActions(QWidget *target, const SavedQuery &saved) +{ + target->setContextMenuPolicy(Qt::ActionsContextMenu); + + auto *edit = new QAction(tr("Edit..."), target); + edit->setObjectName(QStringLiteral("editQuery")); + connect(edit, &QAction::triggered, this, + [this, saved]() { editSavedQuery(saved); }); + target->addAction(edit); + + auto *pin = new QAction(saved.pinned ? tr("Move to menu") + : tr("Show as a button"), + target); + pin->setObjectName(QStringLiteral("pinQuery")); + connect(pin, &QAction::triggered, this, [this, saved]() { + SavedQuery toggled = saved; + toggled.pinned = !saved.pinned; + replaceSavedQuery(saved.name, toggled); + }); + target->addAction(pin); + + auto *separator = new QAction(target); + separator->setSeparator(true); + target->addAction(separator); + + auto *remove = new QAction(tr("Delete"), target); + remove->setObjectName(QStringLiteral("deleteQuery")); + connect(remove, &QAction::triggered, this, + [this, saved]() { deleteSavedQuery(saved); }); + target->addAction(remove); +} + +void MainWindow::editSavedQuery(const SavedQuery &saved) +{ + SaveQueryDialog dialog(m_config, saved, this); + if (dialog.exec() != QDialog::Accepted) + return; + + // Matched on the name the dialog OPENED with. Using the returned name would + // leave the original entry in place and add a second one under the new + // name, which is a duplicate rather than a rename. + replaceSavedQuery(saved.name, dialog.savedQuery()); +} + +void MainWindow::deleteSavedQuery(const SavedQuery &saved) +{ + // One of the few places in this application that confirms. The rule against + // confirmation dialogs covers tag mutations, which are undoable through the + // undo stack; this writes user config, is not on that stack, and cannot be + // taken back. + if (m_confirmDelete) { + const auto answer = QMessageBox::question( + this, tr("Delete saved query"), + tr("Delete the saved query '%1'?").arg(saved.name), + QMessageBox::Yes | QMessageBox::No, QMessageBox::No); + if (answer != QMessageBox::Yes) + return; + } + + replaceSavedQuery(saved.name, SavedQuery()); +} + +void MainWindow::replaceSavedQuery(const QString &originalName, + const SavedQuery &replacement) +{ + QList queries = m_config.savedQueries(); + const bool removing = replacement.name.isEmpty(); + + for (int i = 0; i < queries.size(); ++i) { + if (queries.at(i).name.compare(originalName, Qt::CaseInsensitive) != 0) + continue; + + if (removing) { + queries.removeAt(i); + } else { + // The unknown fields belong to the STORED entry: a field written by + // a later build survives an edit made here rather than being + // dropped on the next save. + SavedQuery merged = replacement; + merged.unknown = queries.at(i).unknown; + queries[i] = merged; + } + break; + } + + m_config.setSavedQueries(queries); + if (!m_config.saveSavedQueries()) { + QMessageBox::warning(this, tr("Saved queries"), + tr("Could not write the saved queries file.")); + return; + } + + rebuildSavedQueryRow(); + statusBar()->showMessage( + removing ? tr("Deleted saved query '%1'.").arg(originalName) + : tr("Updated saved query '%1'.").arg(replacement.name), + kStatusMessageMs); +} + void MainWindow::runSavedQuery(const SavedQuery &saved) { // Through the dropdown, never by pre-scoping the text: runQuery() applies @@ -1817,6 +1923,13 @@ void MainWindow::rebuildSavedQueryRow() const int index = layout->indexOf(old); layout->removeWidget(old); + // Reparented out NOW, not merely scheduled for deletion. deleteLater() + // defers destruction to the event loop, so the old row goes on answering + // findChild() until it runs, and findChild returns the FIRST match: every + // lookup after a rebuild found the stale row and reported the state from + // before the edit. Nothing visible was wrong, which is why this only + // showed up as three tests failing on a row that had in fact been rebuilt. + old->setParent(nullptr); old->deleteLater(); buildSavedQueryRow(centralWidget(), layout); diff --git a/src/mainwindow.h b/src/mainwindow.h index e6ee322..6e8501a 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -125,6 +125,25 @@ public: static void setLocksPathForTesting(const QString &path); static QString locksPath(); + /// Suppresses the delete confirmation. + /// + /// A test seam. Deleting a saved query is destructive and not on the undo + /// stack, so it asks first; a test cannot answer a modal dialog without + /// hanging, and driving one through QTest would assert the dialog rather + /// than the deletion. + void setConfirmDeleteForTesting(bool confirm) { m_confirmDelete = confirm; } + + /// Renames or replaces a stored query, as the edit dialog would on accept. + /// + /// A test seam for the rename path specifically: the dialog is modal, and + /// the property worth asserting is that a rename REPLACES rather than + /// duplicating, which is decided after the dialog returns. + void replaceSavedQueryForTesting(const QString &originalName, + const SavedQuery &replacement) + { + replaceSavedQuery(originalName, replacement); + } + /// How many commands are on the undo stack. /// /// A test seam. The undo QAction is always enabled and checks canUndo() @@ -253,6 +272,21 @@ private: /// Rebuilds the saved-query row in place after the stored list changed. void rebuildSavedQueryRow(); + /// Hangs Edit, Pin/Unpin and Delete on a saved query's button or menu + /// entry. The only route to changing a stored query from the UI. + void addSavedQueryActions(QWidget *target, const SavedQuery &saved); + + /// Replaces the entry named `originalName`, writes the file and rebuilds + /// the row. An empty `replacement.name` deletes it instead. + /// + /// Matched on the ORIGINAL name, not the replacement's: a rename otherwise + /// leaves the old entry in place and adds a second one. + void replaceSavedQuery(const QString &originalName, + const SavedQuery &replacement); + + void editSavedQuery(const SavedQuery &saved); + void deleteSavedQuery(const SavedQuery &saved); + private slots: void runCurrentQuery() { runQuery(FlatResult::No); } @@ -628,6 +662,8 @@ private: QLineEdit *m_queryEdit = nullptr; /// Save query, beside the field. Driven by the save_query action. QToolButton *m_saveQueryButton = nullptr; + /// Whether deleting a saved query asks first. Always true outside tests. + bool m_confirmDelete = true; QueryCompleter *m_queryCompleter = nullptr; /// Its own type, not the QTreeView base. The strip painting and the /// expander column are ThreadListView's, and holding the base here only diff --git a/src/savequerydialog.cpp b/src/savequerydialog.cpp index 1ea97c2..2d09986 100644 --- a/src/savequerydialog.cpp +++ b/src/savequerydialog.cpp @@ -42,19 +42,51 @@ SaveQueryDialog::SaveQueryDialog(const Config &config, const QString &query, : QDialog(parent) , m_config(config) { + SavedQuery initial; + initial.query = query; + initial.account = accountKey; + initial.pinned = true; setWindowTitle(tr("Save query")); + build(initial); +} +SaveQueryDialog::SaveQueryDialog(const Config &config, + const SavedQuery &existing, QWidget *parent) + : QDialog(parent) + , m_config(config) + , m_originalName(existing.name) + , m_generated(existing.generated) + , m_flat(existing.flat) +{ + setWindowTitle(tr("Edit saved query")); + build(existing); +} + +void SaveQueryDialog::build(const SavedQuery &initial) +{ auto *layout = new QVBoxLayout(this); auto *form = new QFormLayout; - m_name = new QLineEdit(this); + m_name = new QLineEdit(initial.name, this); m_name->setObjectName(QStringLiteral("saveQueryName")); m_name->setPlaceholderText(tr("A name for this query")); form->addRow(tr("Name"), m_name); - m_query = new QLineEdit(query, this); + m_query = new QLineEdit(initial.query, this); m_query->setObjectName(QStringLiteral("saveQueryQuery")); - form->addRow(tr("Query"), m_query); + if (initial.isGenerated()) { + // A generated entry has no stored query: it is composed from the + // accounts every time it runs. Shown, so the user can see what it will + // do, but read-only, since editing it would change nothing. + m_query->setText(m_config.resolvedQuery(initial)); + m_query->setReadOnly(true); + m_query->setToolTip(tr("Built from your accounts and not editable. " + "It follows the sent folder each account " + "configures.")); + form->addRow(tr("Query"), m_query); + } else { + form->addRow(tr("Query"), m_query); + } // The scope is stored as an account KEY, so the entries carry the key as // data exactly as the main window's dropdown does. "All accounts" is the @@ -63,15 +95,15 @@ SaveQueryDialog::SaveQueryDialog(const Config &config, const QString &query, m_account = new QComboBox(this); m_account->setObjectName(QStringLiteral("saveQueryAccount")); m_account->addItem(tr("All accounts"), QString()); - for (const Account &account : config.accounts()) + for (const Account &account : m_config.accounts()) m_account->addItem(account.key, account.key); - const int index = m_account->findData(accountKey); + const int index = m_account->findData(initial.account); m_account->setCurrentIndex(index >= 0 ? index : 0); form->addRow(tr("Account"), m_account); m_pinned = new QCheckBox(tr("Show as a button"), this); m_pinned->setObjectName(QStringLiteral("saveQueryPinned")); - m_pinned->setChecked(true); + m_pinned->setChecked(initial.pinned); form->addRow(QString(), m_pinned); layout->addLayout(form); @@ -108,7 +140,14 @@ void SaveQueryDialog::updateOkState() const bool usable = !name.isEmpty() && !m_query->text().trimmed().isEmpty(); m_ok->setEnabled(usable); - if (!name.isEmpty() && namesAnExistingQuery(m_config, name)) { + // Ignores the entry being edited: warning that "Inbox" already exists + // while editing Inbox is noise, and the real case worth catching is a + // rename onto a name something else already holds. + const bool isItsOwnName = + !m_originalName.isEmpty() + && name.compare(m_originalName, Qt::CaseInsensitive) == 0; + if (!name.isEmpty() && !isItsOwnName + && namesAnExistingQuery(m_config, name)) { m_notice->setText( tr("A saved query named '%1' already exists and will be " "replaced.").arg(name)); @@ -124,5 +163,12 @@ SavedQuery SaveQueryDialog::savedQuery() const saved.query = m_query->text().trimmed(); saved.account = m_account->currentData().toString(); saved.pinned = m_pinned->isChecked(); + // Carried through rather than re-derived: an edit must not turn a + // generated entry into a plain one holding a snapshot of what it happened + // to resolve to today. + saved.generated = m_generated; + saved.flat = m_flat; + if (saved.isGenerated()) + saved.query.clear(); return saved; } diff --git a/src/savequerydialog.h b/src/savequerydialog.h index be5a2af..d859254 100644 --- a/src/savequerydialog.h +++ b/src/savequerydialog.h @@ -42,6 +42,11 @@ public: SaveQueryDialog(const Config &config, const QString &query, const QString &accountKey, QWidget *parent = nullptr); + /// Edits an entry that already exists, prefilled from it rather than from + /// the query bar. + SaveQueryDialog(const Config &config, const SavedQuery &existing, + QWidget *parent = nullptr); + /// The query as edited. Only meaningful after exec() returned Accepted. SavedQuery savedQuery() const; @@ -51,8 +56,19 @@ public: static bool namesAnExistingQuery(const Config &config, const QString &name); private: + void build(const SavedQuery &initial); void updateOkState(); + /// The name the dialog was opened on, empty when creating. The caller + /// matches on this rather than on the returned name, so a rename replaces + /// the entry instead of adding a second one beside it. + QString m_originalName; + + /// Set for a generated entry, whose query is composed from the accounts + /// and cannot be edited here. + QString m_generated; + bool m_flat = false; + const Config &m_config; QLineEdit *m_name = nullptr; QLineEdit *m_query = nullptr; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 883e9a7..d090016 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -203,6 +203,11 @@ private slots: void aStoredGeneratedQueryRunsFlatAndComposed(); void aRenamedSentEntryKeepsWorking(); void aGeneratedQueryWithNothingToShowIsSkipped(); + void aSavedQueryButtonOffersEditUnpinAndDelete(); + void unpinningMovesAQueryToTheMenu(); + void deletingRemovesTheQueryFromTheFile(); + void anEditedQueryKeepsItsUnknownFields(); + void renamingReplacesRatherThanDuplicating(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -5775,4 +5780,199 @@ void TestMainWindow::aGeneratedQueryWithNothingToShowIsSkipped() "a generated query with nothing to show must not get a button"); } +/// Reads queries.json back from disk, which is what "it was saved" means. +static QJsonArray storedQueries(const QTemporaryDir &dir) +{ + QFile f(dir.filePath(QStringLiteral("qtmaildir/queries.json"))); + if (!f.open(QIODevice::ReadOnly)) + return {}; + const QJsonObject root = QJsonDocument::fromJson(f.readAll()).object(); + return root.value(QStringLiteral("queries")).toArray(); +} + +static QAction *contextActionNamed(MainWindow &window, QWidget *target, + const QString &objectName) +{ + const QList actions = target->actions(); + for (QAction *action : actions) { + if (action->objectName() == objectName) + return action; + } + Q_UNUSED(window); + return nullptr; +} + +void TestMainWindow::aSavedQueryButtonOffersEditUnpinAndDelete() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true } + ] + })")); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + auto *button = row->findChild(); + QVERIFY(button); + + // A context menu, so the actions live on the widget itself. + QCOMPARE(button->contextMenuPolicy(), Qt::ActionsContextMenu); + QVERIFY(contextActionNamed(window, button, QStringLiteral("editQuery"))); + QVERIFY(contextActionNamed(window, button, QStringLiteral("pinQuery"))); + QVERIFY(contextActionNamed(window, button, QStringLiteral("deleteQuery"))); +} + +void TestMainWindow::unpinningMovesAQueryToTheMenu() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Other", "query": "tag:other", "pinned": true } + ] + })")); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + QCOMPARE(savedQueryButtonLabels(window).size(), 2); + QVERIFY(!window.findChild( + QStringLiteral("savedQueryMenuButton"))); + + auto *button = row->findChild(); + QVERIFY(button); + QAction *pin = contextActionNamed(window, button, QStringLiteral("pinQuery")); + QVERIFY(pin); + pin->trigger(); + + // Off the row, into the menu, and written to the file: an unpin that only + // redrew would come back pinned on the next launch. + QCOMPARE(savedQueryButtonLabels(window), QStringList{ QStringLiteral("Other") }); + auto *menuButton = + window.findChild(QStringLiteral("savedQueryMenuButton")); + QVERIFY(menuButton); + QCOMPARE(menuButton->menu()->actions().size(), 1); + + const QJsonArray stored = storedQueries(dir); + QCOMPARE(stored.size(), 2); + QCOMPARE(stored.at(0).toObject().value(QStringLiteral("name")).toString(), + QStringLiteral("Inbox")); + QVERIFY2(!stored.at(0).toObject().contains(QStringLiteral("pinned")), + "the unpin did not reach the file"); +} + +void TestMainWindow::deletingRemovesTheQueryFromTheFile() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Doomed", "query": "tag:doomed", "pinned": true }, + { "name": "Keeper", "query": "tag:keeper", "pinned": true } + ] + })")); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + auto *button = row->findChild(); + QVERIFY(button); + QCOMPARE(button->text(), QStringLiteral("Doomed")); + + QAction *del = + contextActionNamed(window, button, QStringLiteral("deleteQuery")); + QVERIFY(del); + // Destructive and not on the undo stack, so it confirms. Suppressed here + // rather than driven through the modal dialog, which would hang the test. + window.setConfirmDeleteForTesting(false); + del->trigger(); + + QCOMPARE(savedQueryButtonLabels(window), + QStringList{ QStringLiteral("Keeper") }); + + const QJsonArray stored = storedQueries(dir); + QCOMPARE(stored.size(), 1); + QCOMPARE(stored.at(0).toObject().value(QStringLiteral("name")).toString(), + QStringLiteral("Keeper")); +} + +/// A field a later build wrote must survive an edit here, or upgrading and +/// downgrading silently strips config the user set. +void TestMainWindow::anEditedQueryKeepsItsUnknownFields() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true, + "icon": "mail-inbox" } + ] + })")); + + MainWindow window(config); + + // Through the EDIT path, with a replacement carrying no unknown fields of + // its own, which is exactly what SaveQueryDialog returns. Driving this + // through unpin instead proved nothing: unpin copies the stored entry, so + // it carries `unknown` along by itself and the merge is never exercised. + // That version passed with the merge deleted. + SavedQuery edited; + edited.name = QStringLiteral("Inbox"); + edited.query = QStringLiteral("tag:inbox and not tag:muted"); + edited.pinned = true; + QVERIFY(edited.unknown.isEmpty()); + window.replaceSavedQueryForTesting(QStringLiteral("Inbox"), edited); + + const QJsonArray stored = storedQueries(dir); + QCOMPARE(stored.size(), 1); + const QJsonObject entry = stored.at(0).toObject(); + // The edit landed... + QCOMPARE(entry.value(QStringLiteral("query")).toString(), + QStringLiteral("tag:inbox and not tag:muted")); + // ...and did not take the unknown field down with it. + QCOMPARE(entry.value(QStringLiteral("icon")).toString(), + QStringLiteral("mail-inbox")); +} + +/// Renaming must match on the name the dialog OPENED with. Matching on the +/// returned name leaves the original in place and adds a second entry. +void TestMainWindow::renamingReplacesRatherThanDuplicating() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Old", "query": "tag:old", "pinned": true } + ] + })")); + + MainWindow window(config); + + SavedQuery renamed; + renamed.name = QStringLiteral("New"); + renamed.query = QStringLiteral("tag:old"); + renamed.pinned = true; + window.replaceSavedQueryForTesting(QStringLiteral("Old"), renamed); + + const QJsonArray stored = storedQueries(dir); + QCOMPARE(stored.size(), 1); + QCOMPARE(stored.at(0).toObject().value(QStringLiteral("name")).toString(), + QStringLiteral("New")); + QCOMPARE(savedQueryButtonLabels(window), QStringList{ QStringLiteral("New") }); +} + #include "test_mainwindow.moc" -- cgit v1.2.3