aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/mainwindow.cpp19
-rw-r--r--tests/test_mainwindow.cpp129
2 files changed, 146 insertions, 2 deletions
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index ee883b0..594535b 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -860,8 +860,23 @@ void MainWindow::registerActions()
// &I rather than &S: the Message menu already has "Mark &spam", so
// "Starred" would have needed an accelerator from inside the word.
addAction(QStringLiteral("flag"), tr("&Important"),
- tr("Mark the selected threads as important"), [this]() {
- tagSelected({ QStringLiteral("flagged") }, {}, tr("Mark important"));
+ tr("Add or remove the important tag"), [this]() {
+ // Item 98. A toggle, like Delete and Toggle unread beside it: adding a
+ // tag that is already there is a no-op the user cannot see, so a
+ // one-way add read as a dead key on anything already important.
+ //
+ // everySelectedRowHasTag() rather than a loop of its own. Two separate
+ // bugs went into that logic on 2026-08-16 (items 88 and 105), and a
+ // copy of the then-current Delete loop would have inherited both:
+ // resolving a reply's row number to the wrong thread, and asking a
+ // reply's THREAD where the write is message-scoped, which makes a
+ // toggle one-way.
+ const bool allFlagged = everySelectedRowHasTag(QStringLiteral("flagged"));
+
+ if (allFlagged)
+ tagSelected({}, { QStringLiteral("flagged") }, tr("Unmark important"));
+ else
+ tagSelected({ QStringLiteral("flagged") }, {}, tr("Mark important"));
});
addAction(QStringLiteral("toggle_unread"), tr("Toggle &unread"),
tr("Toggle the unread tag"), [this]() {
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index 07cc56b..b188ef4 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -276,6 +276,9 @@ private slots:
void deleteOnAMixedSelectionDeletesRatherThanSplittingIt();
void deleteOnAReplyReadsItsOwnThreadNotTheFirstInTheList();
void toggleUnreadOnAReplyReadsItsOwnThreadNotTheFirstInTheList();
+ void importantOnAnAlreadyImportantThreadRemovesTheTag();
+ void importantOnAPlainThreadStillAddsTheTag();
+ void importantOnAReplyReadsItsOwnStateNotItsThreads();
void editTagsOnAReplyCountsItsOwnThreadNotTheFirstInTheList();
void markCurrentThreadReadResolvesTheThreadThroughTheIndex();
void deletingAReplyRepaintsThatReplyRow();
@@ -4798,6 +4801,132 @@ void TestMainWindow::toggleUnreadOnAReplyReadsItsOwnThreadNotTheFirstInTheList()
.arg(window.undoTextForTesting())));
}
+void TestMainWindow::importantOnAnAlreadyImportantThreadRemovesTheTag()
+{
+ // Item 98. `flag` was a one-way add, so pressing it on a thread that is
+ // already important re-sent a tag the thread had: a no-op write, and a
+ // no-op repaints nothing, so the key read as dead. Its two neighbours,
+ // Delete and Toggle unread, had been toggles for a long time.
+ const Config config;
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTreeView *>();
+ QVERIFY(view);
+ auto *action = window.findChild<QAction *>(QStringLiteral("flag"));
+ QVERIFY(action);
+
+ model->appendBatch({ makeThread(QStringLiteral("t1"),
+ QStringList{ QStringLiteral("inbox"),
+ QStringLiteral("flagged") }) });
+ QApplication::processEvents();
+
+ view->setCurrentIndex(model->index(0, 0));
+ view->selectionModel()->select(model->index(0, 0),
+ QItemSelectionModel::ClearAndSelect
+ | QItemSelectionModel::Rows);
+ QApplication::processEvents();
+
+ action->trigger();
+
+ QCOMPARE(window.undoDepthForTesting(), 1);
+ QVERIFY2(window.undoTextForTesting().contains(
+ QStringLiteral("Unmark important")),
+ qPrintable(QStringLiteral(
+ "Important on an already-important thread did not "
+ "remove the tag: %1. A one-way add is a no-op the "
+ "user cannot see.")
+ .arg(window.undoTextForTesting())));
+}
+
+void TestMainWindow::importantOnAPlainThreadStillAddsTheTag()
+{
+ // The other direction, so a mutation inverting the test above cannot pass.
+ const Config config;
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTreeView *>();
+ QVERIFY(view);
+ auto *action = window.findChild<QAction *>(QStringLiteral("flag"));
+ QVERIFY(action);
+
+ model->appendBatch({ makeThread(QStringLiteral("t1"),
+ QStringList{ QStringLiteral("inbox") }) });
+ QApplication::processEvents();
+
+ view->setCurrentIndex(model->index(0, 0));
+ view->selectionModel()->select(model->index(0, 0),
+ QItemSelectionModel::ClearAndSelect
+ | QItemSelectionModel::Rows);
+ QApplication::processEvents();
+
+ action->trigger();
+
+ QCOMPARE(window.undoDepthForTesting(), 1);
+ QVERIFY2(window.undoTextForTesting().contains(
+ QStringLiteral("Mark important"))
+ && !window.undoTextForTesting().contains(
+ QStringLiteral("Unmark")),
+ qPrintable(QStringLiteral(
+ "Important on a plain thread did not add the tag: "
+ "%1")
+ .arg(window.undoTextForTesting())));
+}
+
+void TestMainWindow::importantOnAReplyReadsItsOwnStateNotItsThreads()
+{
+ // The trap items 88 and 105 each fixed once, which is why item 98 says to
+ // call everySelectedRowHasTag() rather than copy the then-current Delete
+ // loop.
+ //
+ // THREE states, all different, which is what the test needs to distinguish
+ // the two wrong answers from the right one. t1 (the first thread in the
+ // list) is unflagged, t2 (the reply's own thread) is flagged, and the
+ // REPLY is unflagged. Reading t1 by row number answers "not flagged" and
+ // reading the reply's THREAD answers "flagged", so only a read of the
+ // message itself gives "not flagged" for the right reason.
+ //
+ // Leaving the reply's tags defaulted to its thread's is the trap: a reply
+ // in the same state as its thread answers identically whichever of the two
+ // the code reads, and the mutation putting item 105's bug back stays green.
+ // Measured: it did.
+ const Config config;
+ MainWindow window(config);
+
+ auto *model = window.findChild<ThreadListModel *>();
+ QVERIFY(model);
+ auto *view = window.findChild<QTreeView *>();
+ QVERIFY(view);
+ auto *action = window.findChild<QAction *>(QStringLiteral("flag"));
+ QVERIFY(action);
+
+ const QModelIndex reply = expandSecondThreadAndSelectItsReply(
+ view, model, {}, { QStringLiteral("flagged") },
+ QStringList{});
+ QVERIFY2(reply.isValid(),
+ "the fixture did not produce a reply row at row 0, so this test "
+ "would assert nothing");
+
+ action->trigger();
+
+ QVERIFY2(window.pendingMessageIdsForTesting().contains(
+ QStringLiteral("m1@example.org")),
+ "Important on a reply did not act on that reply");
+ QCOMPARE(window.undoDepthForTesting(), 1);
+ QVERIFY2(window.undoTextForTesting().contains(
+ QStringLiteral("Mark important"))
+ && !window.undoTextForTesting().contains(
+ QStringLiteral("Unmark")),
+ qPrintable(QStringLiteral(
+ "Important on an unflagged reply chose the wrong "
+ "direction: %1. Its own THREAD is flagged, so "
+ "reading the thread gives Unmark.")
+ .arg(window.undoTextForTesting())));
+}
+
void TestMainWindow::editTagsOnAReplyCountsItsOwnThreadNotTheFirstInTheList()
{
// The tag dialog is modal, so what is tested is the count it is BUILT