From b08923df88de7ba03135234aaf3c602f23e49e03 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 14 Aug 2026 11:01:58 +0200 Subject: fix(rules): stop a rule with a spaced name from vanishing on save A rule named "justeat orders" in the field labelled Name was written to rules.json correctly and then dropped by every reader, because load() required ^[a-z0-9][a-z0-9-]*$ and the save path validated nothing. The rule stayed in the file, invisible in the dialog, never applied by the post-new hook, and the next save from the dialog would have deleted it outright. The asymmetry was the defect, not the pattern. TagRules::validate() is now the single predicate: the dialog refuses to save against it, and load() uses it to repair rather than drop, so a rule that fails is visible and fixable instead of silently discarded. - The typed name is sanitised into an id when the field is committed, so the field shows what will reach the file. uniqueId() suffixes a collision, since sanitising is many-to-one and can manufacture the duplicate that load() then drops. - An already-legal id is never rewritten, including one like "a---b" that sanitising would otherwise collapse. Rewriting valid ids would churn a file mailctl also reads. - A bad id loads repaired, with the warning kept: what is on disk is not what the hook runs until the file is saved back. Deliberately not mirrored into mailrules.py. The hook tags real mail unattended every ten minutes, where silently renaming an id is worse than dropping the rule; the file converges as soon as the dialog saves. No format change, so no version bump and no two-repo commitment. The load warning was not missing: it had been showing "1 rule could not be read and was skipped" on every open, in the same font and colour as the intro prose two lines above it, and read as more explanation. It is now a red banner beside Save, with an icon and a dismiss button, and it says the rules need attention rather than that they were skipped, which is no longer true. Dismissal is per-appearance only; a persistent one would re-hide the problem that went unnoticed for a session. Both new dialog tests were confirmed to fail with the sanitiser reverted, and the banner's styling, position and dismissal each fail under mutation. 20 of 20 suites green, 34 tests in test_tagrules. Closes item 83. Co-Authored-By: Claude Opus 5 --- src/tagrules.cpp | 108 ++++++++++++++++++++++++++--- src/tagrules.h | 25 +++++++ src/tagrulesdialog.cpp | 184 +++++++++++++++++++++++++++++++++++++++++++++---- src/tagrulesdialog.h | 29 ++++++++ 4 files changed, 323 insertions(+), 23 deletions(-) (limited to 'src') diff --git a/src/tagrules.cpp b/src/tagrules.cpp index 39cc529..0049278 100644 --- a/src/tagrules.cpp +++ b/src/tagrules.cpp @@ -58,8 +58,96 @@ QStringList stringsOf(const QJsonValue &value) return out; } +/// The id pattern, in one place. mailrules.py enforces the same one; see +/// "Changing the shared rule format" in CLAUDE.md before touching it. +const QRegularExpression &idPattern() +{ + static const QRegularExpression pattern( + QStringLiteral("^[a-z0-9][a-z0-9-]*$")); + return pattern; +} + } // namespace +bool TagRules::isValidId(const QString &id) +{ + return idPattern().match(id).hasMatch(); +} + +QString TagRules::sanitiseId(const QString &name) +{ + // Untouched when already legal: sanitising every id on load would rewrite + // a good file and show mailctl a diff the user never made. + if (isValidId(name)) + return name; + + QString out; + out.reserve(name.size()); + for (const QChar ch : name.toLower()) { + if ((ch >= QLatin1Char('a') && ch <= QLatin1Char('z')) + || (ch >= QLatin1Char('0') && ch <= QLatin1Char('9'))) { + out.append(ch); + } else if (!out.isEmpty() && !out.endsWith(QLatin1Char('-'))) { + // One dash per run of anything else, so "Notify: PayPal!" does not + // become "notify--paypal-". + out.append(QLatin1Char('-')); + } + } + while (out.endsWith(QLatin1Char('-'))) + out.chop(1); + + // Empty rather than a manufactured id. The caller knows what the rule is + // and can name it; this function inventing "rule-1" would hide the fact + // that nothing of the typed name survived. + return out; +} + +QString TagRules::uniqueId(const QString &name, const QStringList &taken) +{ + QString base = sanitiseId(name); + if (base.isEmpty()) + base = QStringLiteral("rule"); + if (!taken.contains(base)) + return base; + + // Starts at 2: the unsuffixed id is the first, so "-2" reads as the second + // rule of that name rather than as an index. + for (int suffix = 2;; ++suffix) { + const QString candidate = + base + QLatin1Char('-') + QString::number(suffix); + if (!taken.contains(candidate)) + return candidate; + } +} + +QStringList TagRules::validate(const QList &rules) +{ + QStringList problems; + QStringList seen; + for (const TagRule &rule : rules) { + const QString where = + rule.id.isEmpty() ? QObject::tr("(unnamed)") : rule.id; + + if (!isValidId(rule.id)) { + problems.append( + QObject::tr("'%1': the name must be lowercase letters, digits " + "and dashes").arg(where)); + } else if (seen.contains(rule.id)) { + problems.append( + QObject::tr("'%1': another rule already has this name") + .arg(where)); + } + seen.append(rule.id); + + if (rule.query.trimmed().isEmpty()) + problems.append(QObject::tr("'%1': no query").arg(where)); + if (rule.add.isEmpty() && rule.remove.isEmpty()) + problems.append(QObject::tr("'%1': adds and removes nothing") + .arg(where)); + } + return problems; +} + QString TagRules::defaultPath() { // Not QStandardPaths::ConfigLocation: that appends the organization and @@ -121,9 +209,12 @@ void TagRules::load(const QString &path) } // An id is a handle: a UI selects on it and a diff tracks it. - static const QRegularExpression idPattern( - QStringLiteral("^[a-z0-9][a-z0-9-]*$")); - + // + // A bad id is REPAIRED rather than dropped. Dropping it made the rule + // invisible in the dialog while it still occupied the file, so the next + // save deleted it outright: the user wrote a rule, saw it vanish, wrote it + // again, and lost it again. The warning still fires, because what is on + // disk is not what the hook runs until the file is saved back. QStringList seen; const QJsonArray array = root.value(QStringLiteral("rules")).toArray(); for (int index = 0; index < array.size(); ++index) { @@ -132,12 +223,13 @@ void TagRules::load(const QString &path) TagRule rule; rule.id = object.value(QStringLiteral("id")).toString(); - if (!idPattern.match(rule.id).hasMatch()) { + if (!isValidId(rule.id)) { + const QString repaired = uniqueId(rule.id, seen); m_warnings.append( - QObject::tr("%1: id '%2' is missing or not lowercase letters, " - "digits and dashes; dropped") - .arg(where, rule.id)); - continue; + QObject::tr("%1: name '%2' is not lowercase letters, digits " + "and dashes; loaded as '%3'. Save to keep it.") + .arg(where, rule.id, repaired)); + rule.id = repaired; } if (seen.contains(rule.id)) { diff --git a/src/tagrules.h b/src/tagrules.h index 8f75b38..4d3f0ab 100644 --- a/src/tagrules.h +++ b/src/tagrules.h @@ -75,6 +75,31 @@ public: QStringList warnings() const { return m_warnings; } + /// The one predicate. load() drops or repairs by it, the dialog refuses to + /// save against it, and mailrules.py enforces the same pattern in the + /// companion repo. Anything failing this is invisible to the post-new hook. + static bool isValidId(const QString &id); + + /// A typed name reduced to a legal id: lowercased, every run of anything + /// else collapsed to one dash, dashes trimmed off both ends. + /// + /// Returns an EMPTY string when nothing legal survives ("!!!"), because an + /// empty id is not writable and the caller must decide the fallback rather + /// than have one invented here. Already-legal ids pass through untouched, + /// so loading a good file never rewrites it. + static QString sanitiseId(const QString &name); + + /// sanitiseId plus a numeric suffix when the result is already taken. + /// Sanitising is many-to-one, so it manufactures duplicates that load() + /// would then drop; this is what stops the second rule becoming the first. + static QString uniqueId(const QString &name, const QStringList &taken); + + /// Every reason these rules would not survive a reload, one string each, + /// empty when they all would. Written for the save path: the defect this + /// answers is that save() wrote anything and load() validated, so a rule + /// could reach the file and never come back. + static QStringList validate(const QList &rules); + /// No file yet, as distinct from a file that would not load. A fresh /// install is not an error and must not be reported as one. bool missing() const { return m_missing; } diff --git a/src/tagrulesdialog.cpp b/src/tagrulesdialog.cpp index 4fca971..226a564 100644 --- a/src/tagrulesdialog.cpp +++ b/src/tagrulesdialog.cpp @@ -97,11 +97,6 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) intro->setWordWrap(true); layout->addWidget(intro); - m_warningLabel = new QLabel(this); - m_warningLabel->setWordWrap(true); - m_warningLabel->setVisible(false); - layout->addWidget(m_warningLabel); - m_list = new QTreeWidget(this); m_list->setColumnCount(ColumnCount + 1); m_list->setHeaderLabels({ tr("On"), tr("Stage"), tr("Rule"), tr("Tags"), @@ -242,6 +237,62 @@ TagRulesDialog::TagRulesDialog(QWidget *parent) buttons->addWidget(refreshButton); layout->addLayout(buttons); + // Beside Save, not under the intro. It sat below the header in the same + // font and colour as the prose around it, so it read as more explanatory + // text: the user missed "1 rule could not be read and was skipped" on + // every open of this dialog while hunting the rule it was telling them + // about. Down here it is next to the button whose outcome it reports, and + // it is the only red thing in the window. + // + // A palette role would be theme-correct and is not usable: the warning has + // to stand out against BOTH a light and a dark desktop, and no role means + // "alarming" in both. The colours are therefore literal, chosen to pass + // contrast either way, which is the same reasoning the tag chips use. + // The label and its dismiss button share a banner widget, so hiding the + // warning takes the button with it. Visibility is the banner's; the label + // itself stays visible inside it and setWarning() is still the one route. + m_warningBanner = new QWidget(this); + m_warningBanner->setVisible(false); + m_warningBanner->setStyleSheet(QStringLiteral( + "QWidget { background-color: #b3261e; border-radius: 4px; }")); + + auto *warningRow = new QHBoxLayout(m_warningBanner); + warningRow->setContentsMargins(8, 6, 6, 6); + + m_warningLabel = new QLabel(m_warningBanner); + m_warningLabel->setWordWrap(true); + // Plain text: these strings interpolate ids and queries read from the + // file, and a query holding '<' would otherwise be eaten as markup. + m_warningLabel->setTextFormat(Qt::PlainText); + m_warningLabel->setStyleSheet(QStringLiteral( + "QLabel { color: #ffffff; font-weight: bold; background: transparent; }")); + warningRow->addWidget(m_warningLabel, 1); + + m_warningClose = new QPushButton(QStringLiteral("✕"), m_warningBanner); + m_warningClose->setToolTip(tr("Dismiss")); + m_warningClose->setFlat(true); + m_warningClose->setCursor(Qt::ArrowCursor); + m_warningClose->setFixedSize(22, 22); + // Focus would put a highlight ring on the banner and let Space dismiss a + // warning the user is only tabbing past. + m_warningClose->setFocusPolicy(Qt::NoFocus); + m_warningClose->setStyleSheet(QStringLiteral( + "QPushButton { color: #ffffff; background: transparent; border: none;" + " font-weight: bold; }" + "QPushButton:hover { background-color: rgba(255, 255, 255, 60);" + " border-radius: 11px; }")); + warningRow->addWidget(m_warningClose, 0, Qt::AlignTop); + + // Dismissed for THIS appearance only, never persistently. The message it + // most often carries is that the file on disk is not what the hook runs, + // and a stored "do not show again" would re-hide exactly the problem that + // went unnoticed for a whole session. The next warning shows it again, + // including on the next open with the file still unrepaired. + connect(m_warningClose, &QPushButton::clicked, + this, [this] { setWarning(QString()); }); + + layout->addWidget(m_warningBanner); + auto *box = new QDialogButtonBox(QDialogButtonBox::Save | QDialogButtonBox::Cancel, this); @@ -417,17 +468,35 @@ void TagRulesDialog::reloadListForTest() reloadList(); } +void TagRulesDialog::setWarning(const QString &text) +{ + if (text.isEmpty()) { + m_warningLabel->clear(); + m_warningBanner->setVisible(false); + return; + } + // One route in, so the icon and the styling cannot drift apart between the + // load path and the save refusal. The glyph is part of the string rather + // than a second widget: it has to survive word wrap without leaving an + // icon stranded beside an empty line. + m_warningLabel->setText(QStringLiteral("⚠ ") + text); + m_warningBanner->setVisible(true); +} + void TagRulesDialog::showWarnings() { const QStringList warnings = m_rules.warnings(); if (warnings.isEmpty()) { - m_warningLabel->setVisible(false); + setWarning(QString()); return; } - m_warningLabel->setText( - tr("%n rule(s) could not be read and were skipped: %1", "", - warnings.size()).arg(warnings.join(QStringLiteral("; ")))); - m_warningLabel->setVisible(true); + // Not "skipped" any more: a rule with a bad name is loaded repaired, and + // saying it was skipped would send the user looking for something that is + // in front of them. What is true of every warning here is that the file on + // disk is not yet what the hook will run. + setWarning(tr("%n rule(s) in the file need attention: %1. Save to write " + "them back.", "", warnings.size()) + .arg(warnings.join(QStringLiteral("; ")))); } void TagRulesDialog::fillItem(QTreeWidgetItem *item, const TagRule &rule) const @@ -541,7 +610,24 @@ void TagRulesDialog::applyEditsToCurrentRule() return; TagRule &rule = m_working[index]; - rule.id = m_id->text().trimmed(); + + // Sanitised as it is committed, not on save, so what the field shows is + // what reaches the file. The field is labelled "Name" and a person types + // "Justeat orders" into it; an id with a space is written happily and then + // dropped by every reader, which is the defect this answers. + QStringList taken; + for (int other = 0; other < m_working.size(); ++other) { + if (other != index) + taken.append(m_working.at(other).id); + } + const QString typed = m_id->text().trimmed(); + rule.id = TagRules::isValidId(typed) ? typed + : TagRules::uniqueId(typed, taken); + if (rule.id != typed) { + const QSignalBlocker blocker(m_id); + m_id->setText(rule.id); + } + rule.stage = m_stage->value(); rule.enabled = m_enabled->isChecked(); rule.add = splitTags(m_add->text()); @@ -629,6 +715,20 @@ void TagRulesDialog::onSave() { applyEditsToCurrentRule(); + // Validated against the same predicate load() uses. Writing a rule that + // cannot be read back is what made a rule disappear: the file was correct, + // every reader dropped it, and nothing said so at the point of the write. + // Reported through the warning label rather than a modal, as the text-mode + // refusal already is: a QMessageBox inside onSave() would hang the suite, + // which drives this path directly through saveForTest(). + const QStringList problems = TagRules::validate(m_working); + if (!problems.isEmpty()) { + setWarning(tr("%n rule(s) cannot be saved as they are: %1", "", + problems.size()) + .arg(problems.join(QStringLiteral("; ")))); + return; + } + m_rules.setRules(m_working); if (!m_rules.save()) { QMessageBox::warning(this, tr("Tagging rules"), @@ -667,6 +767,59 @@ void TagRulesDialog::setTextModeForTest(bool on) m_textMode->setChecked(on); } +void TagRulesDialog::setNameForTest(const QString &name) +{ + m_id->setText(name); + // editingFinished is what leaving the field emits, and it is where the + // sanitiser hangs. setText() alone does not emit it. + emit m_id->editingFinished(); +} + +QString TagRulesDialog::nameLineForTest() const +{ + return m_id->text(); +} + +QString TagRulesDialog::warningStyleForTest() const +{ + // Both: the fill is the banner's and the text colour is the label's, so + // reading only one of them would miss half the styling. + return m_warningBanner->styleSheet() + m_warningLabel->styleSheet(); +} + +void TagRulesDialog::dismissWarningForTest() +{ + m_warningClose->click(); +} + +Qt::TextFormat TagRulesDialog::warningTextFormatForTest() const +{ + return m_warningLabel->textFormat(); +} + +bool TagRulesDialog::warningIsBelowTheRuleListForTest() const +{ + // By layout position rather than by coordinates: the offscreen platform + // does not lay a dialog out the way a real one is, so a y() comparison + // would assert about the platform. indexOf() on the shared parent layout + // is exact and true in both. + auto *parent = qobject_cast(layout()); + if (!parent) + return false; + return parent->indexOf(m_warningBanner) > parent->indexOf(m_splitter); +} + +int TagRulesDialog::ruleCountForTest() const +{ + return m_working.size(); +} + +void TagRulesDialog::setTagsForTest(const QString &tags) +{ + m_add->setText(tags); + emit m_add->editingFinished(); +} + bool TagRulesDialog::textModeToggleIsReachableForTest() const { // isVisibleTo rather than isVisible: nothing is isVisible() on a dialog @@ -681,8 +834,10 @@ QString TagRulesDialog::warningTextForTest() const // 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(); + // The BANNER carries the visibility now: the label stays visible inside it + // and would report a dismissed warning as still showing. + return m_warningBanner->isVisibleTo(this) ? m_warningLabel->text() + : QString(); } void TagRulesDialog::selectRuleForTest(int index) @@ -922,10 +1077,9 @@ void TagRulesDialog::setTextMode(bool on) if (!parsed.parsed) { const QSignalBlocker block(m_textMode); m_textMode->setChecked(true); - m_warningLabel->setText( + setWarning( 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; } diff --git a/src/tagrulesdialog.h b/src/tagrulesdialog.h index e8979fe..8ba656b 100644 --- a/src/tagrulesdialog.h +++ b/src/tagrulesdialog.h @@ -89,6 +89,28 @@ public: void setTextModeForTest(bool on); QString warningTextForTest() const; + /// The warning's appearance and place, asserted as widget properties. A + /// render probe cannot carry this: see "Rendering probes lie" in CLAUDE.md. + QString warningStyleForTest() const; + Qt::TextFormat warningTextFormatForTest() const; + bool warningIsBelowTheRuleListForTest() const; + + /// Clicks the warning's dismiss button, so the test drives the same signal + /// the user's click does rather than calling setWarning() behind it. + void dismissWarningForTest(); + + /// Types a name and commits it the way leaving the field does. The commit + /// is the point: the sanitiser runs on editingFinished, so a test that + /// only calls setText() asserts against a field nothing has processed. + void setNameForTest(const QString &name); + QString nameLineForTest() const; + + /// Adding and filling a rule the way the buttons do, so a test can drive + /// the whole journey the user takes rather than only its last step. + int ruleCountForTest() const; + void addRuleForTest() { onAddRule(); } + void setTagsForTest(const QString &tags); + /// 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 @@ -169,6 +191,11 @@ private: void restoreUiState(); void saveUiState(); void showWarnings(); + + /// The only way the warning label is written. An empty string hides it. + /// One route in so the icon and the red styling cannot drift between the + /// load path, the save refusal and the text-mode notice. + void setWarning(const QString &text); int currentIndex() const; /// Writes one rule's summary onto its row. Shared by reloadList() and @@ -232,6 +259,8 @@ private: bool m_countColumnSized = false; QTreeWidget *m_list = nullptr; + QWidget *m_warningBanner = nullptr; + QPushButton *m_warningClose = nullptr; QLineEdit *m_id = nullptr; QLineEdit *m_add = nullptr; QLineEdit *m_remove = nullptr; -- cgit v1.2.3