2 files changed, 76 insertions, 10 deletions

diff --git a/src/tagdialog.cpp b/src/tagdialog.cpp
index 1fb3f18..fb2c629 100644
--- a/src/tagdialog.cpp
+++ b/src/tagdialog.cpp
@@ -268,16 +268,25 @@ void TagDialog::accept()
     QStringList add = splitTags(m_addEdit->text());
     QStringList remove = splitTags(m_removeEdit->text());
 
-    // Validate before applying anything: a partial change is worse than none,
-    // since the user cannot tell which half landed.
-    for (const QStringList &list : { add, remove }) {
-        for (const QString &tag : list) {
-            const TagNameProblem problem = validateTagName(tag);
-            if (problem != TagNameProblem::Ok) {
-                QMessageBox::warning(this, tr("Invalid tag"),
-                                     tagNameProblemText(problem, tag));
-                return;   // Stay open, with the text still there to fix.
-            }
+    // Validate what is being ADDED. A partial change is worse than none, since
+    // the user cannot tell which half landed, so this runs before anything is
+    // applied.
+    //
+    // REMOVAL is deliberately not validated. The rules here exist to stop a
+    // troublesome tag being CREATED; a tag that already exists is a fact, and
+    // refusing to remove it because it breaks a rule leaves the user with a
+    // tag they can see, cannot type, and cannot get rid of. That happened with
+    // `deleted-from:Inbox/SlackBuilds users`: an origin tag naming a Maildir
+    // folder whose name contains a space, rejected by the space rule, so the
+    // one dialog that could have cleared it refused the only text that names
+    // it. Whether such a tag SHOULD exist is a separate question from whether
+    // the user may delete it, and the answer to the second is always yes.
+    for (const QString &tag : add) {
+        const TagNameProblem problem = validateTagName(tag);
+        if (problem != TagNameProblem::Ok) {
+            QMessageBox::warning(this, tr("Invalid tag"),
+                                 tagNameProblemText(problem, tag));
+            return;   // Stay open, with the text still there to fix.
         }
     }
 
diff --git a/tests/test_tagdialog.cpp b/tests/test_tagdialog.cpp
index 5289599..9b12109 100644
--- a/tests/test_tagdialog.cpp
+++ b/tests/test_tagdialog.cpp
@@ -47,6 +47,7 @@ private slots:
     void acceptingACandidateKeepsTheOtherTags();
     void removeCompletesOnlyTheSelectionsOwnTags();
     void removeStillAcceptsATagItDoesNotSuggest();
+    void aTagWithASpaceCanStillBeRemoved();
 };
 
 void TestTagDialog::validNamesAreAccepted()
@@ -166,6 +167,62 @@ void TestTagDialog::multipleTagsSeparateOnComma()
                                                QStringLiteral("three") }));
 }
 
+void TestTagDialog::aTagWithASpaceCanStillBeRemoved()
+{
+    // validateTagName() rejects a space, and that rule is right: it stops a
+    // troublesome tag being CREATED. It ran on the removal list too, which is
+    // not the same question. A tag that already exists is a fact, and refusing
+    // to remove it because it breaks a naming rule leaves the user with a tag
+    // they can see and cannot get rid of.
+    //
+    // Reached by a real Maildir: a folder named "Inbox/SlackBuilds users"
+    // produced `deleted-from:Inbox/SlackBuilds users`, and the one dialog that
+    // could have cleared it refused the only text that names it.
+    //
+    // Only the TYPED route was blocked. Unchecking appends to the removal list
+    // after validation has run, so it worked throughout; that asymmetry is why
+    // both routes are asserted here rather than just the one that failed.
+    const QString spaced =
+        QStringLiteral("deleted-from:Inbox/SlackBuilds users");
+    QHash<QString, int> current;
+    current.insert(spaced, 1);
+
+    // Typed into the remove field, which is what a user does for a tag they
+    // can see on the message. Before the fix this raised a modal warning and
+    // returned without accepting, so the dialog simply would not close.
+    TagDialog typed({ spaced }, current, 1);
+    const QList<QLineEdit *> edits = typed.findChildren<QLineEdit *>();
+    QCOMPARE(edits.size(), 2);
+    edits.at(1)->setText(spaced);
+    typed.accept();
+
+    QCOMPARE(typed.tagsToRemove(), QStringList{ spaced });
+    QVERIFY(typed.tagsToAdd().isEmpty());
+
+    // And unchecking it in the list, the other way to the same place.
+    TagDialog unchecked({ spaced }, current, 1);
+    auto *list = unchecked.findChild<QListWidget *>();
+    QVERIFY(list);
+    QCOMPARE(list->count(), 1);
+    QCOMPARE(list->item(0)->data(Qt::UserRole).toString(), spaced);
+    list->item(0)->setCheckState(Qt::Unchecked);
+    unchecked.accept();
+
+    QCOMPARE(unchecked.tagsToRemove(), QStringList{ spaced });
+
+    // ADDING one is still refused, which is the rule this must not have
+    // weakened. accept() returns without setting the lists, so the dialog
+    // stays open with the text there to fix.
+    TagDialog added({}, {}, 1);
+    const QList<QLineEdit *> addEdits = added.findChildren<QLineEdit *>();
+    QCOMPARE(addEdits.size(), 2);
+    addEdits.at(0)->setText(QStringLiteral("two words"));
+    // Not calling accept(): it would raise a modal warning and block. The
+    // validator is the thing under test and is asked directly.
+    QVERIFY(validateTagName(QStringLiteral("two words"))
+            != TagNameProblem::Ok);
+}
+
 void TestTagDialog::uncheckingACurrentTagRemovesIt()
 {
     // Every selected thread carries "inbox", so its box starts checked.