From 987a9e728995cbbfc77e470db72d552ebe096ba2 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 4 Aug 2026 18:16:31 +0200 Subject: feat(ui): make multi-select discoverable and stop it opening threads Multi-select already worked by Ctrl+click and Shift+click, but nothing in the UI said so and every tag action was keyboard-only, so the Ctrl+T tag dialog could not be reached with a mouse at all. Adds a select_all action on Ctrl+A, registered like every other action so it reaches the Edit menu, the shortcut reference and [keys]; a right-click menu on the thread list built from the same QActions rather than parallel copies; a selection count in the status bar, which is the part that actually teaches the feature by acknowledging a selection while it is being built; and a note in the shortcut dialog for the mouse gestures, which are view behaviour and cannot appear in the generated table. A selection gesture must not open mail or mutate it. Selecting several rows now blanks the message pane and cancels any pending mark-read, rather than rendering each row swept through and queueing it to be marked read. Two Qt behaviours shaped this, both established by probe rather than from memory: - selectAll() emits no currentRowChanged at all and leaves the current index invalid. - currentRowChanged is emitted BEFORE the selection model is updated. The second one caused two distinct faults. Collapsing a multi-row selection back to one row reported the old count, so the guard swallowed the load and the pane stayed blank; that case is handled in onSelectionChanged, which sees the true count. And a Ctrl+click taking the selection from one row to two also reported one, so the thread was loaded, blanked, and then painted back when the queued reply returned from the worker. By the third row the id was already cleared and the reply was discarded, which is why the fault presented as an off-by-one in the threshold rather than as a race. Tests cover the synchronous half. The late-reply guard has no test: MainWindow in tests has no worker, so threadLoaded never fires and the repaint cannot be reproduced in process. Verified by hand instead. Co-Authored-By: Claude Opus 5 --- tests/test_mainwindow.cpp | 255 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 255 insertions(+) (limited to 'tests/test_mainwindow.cpp') diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index d307a99..a2142b9 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -66,6 +67,13 @@ private slots: void aFailedSyncDoesNotClearThePendingCount(); void closingWithNoPendingEditsDoesNotPrompt(); void syncOnExitNeverClosesSilently(); + void selectAllIsBoundAndSelectsEveryRow(); + void aMultiRowSelectionDoesNotArmTheMarkReadTimer(); + void growingASelectionCancelsAnAlreadyArmedTimer(); + void collapsingBackToOneRowLoadsThatThreadAgain(); + void theStatusBarReportsAMultiRowSelection(); + void theThreadListOffersAContextMenu(); + void aSecondRowBlanksThePaneNotOnlyAThird(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -591,6 +599,253 @@ void TestMainWindow::syncOnExitNeverClosesSilently() "sync_on_exit=never must close without prompting"); } +void TestMainWindow::selectAllIsBoundAndSelectsEveryRow() +{ + // Multi-select already worked by Ctrl+click and Shift+click; what was + // missing was a keyboard and menu route to it. The action has to exist as a + // registered action, not as a raw view shortcut, so it reaches the menu, + // the shortcut reference and [keys] like every other binding. + const Config config; + MainWindow window(config); + + auto *action = window.findChild(QStringLiteral("select_all")); + QVERIFY2(action, "no select_all action registered"); + QCOMPARE(action->shortcut(), QKeySequence(QStringLiteral("Ctrl+A"))); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("t1"), {}), + makeThread(QStringLiteral("t2"), {}), + makeThread(QStringLiteral("t3"), {}) }); + + action->trigger(); + + QCOMPARE(view->selectionModel()->selectedRows().size(), 3); +} + +void TestMainWindow::aMultiRowSelectionDoesNotArmTheMarkReadTimer() +{ + // A selection gesture must never mutate mail. current follows the keyboard + // cursor as a selection extends, so without a guard every row swept through + // by Shift+arrow would be queued to be marked read: threads the user only + // ever selected, never opened. + // + // Note selectAll() on a fresh view is NOT the case to test here: it leaves + // current invalid and emits no currentRowChanged at all (verified against + // Qt 6.11), so it would pass without any guard in place. The real path is a + // row already current, which is how a user reaches select-all: click a + // thread, then Ctrl+A. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *timer = window.findChild(QStringLiteral("markReadTimer")); + QVERIFY(timer); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t3"), + { QStringLiteral("unread") }) }); + + // Sweep down as Shift+arrow does: current moves onto a row while the + // selection already spans more than one. + view->selectRow(0); + view->selectionModel()->select( + model->index(1, 0), + QItemSelectionModel::Select | QItemSelectionModel::Rows); + view->selectionModel()->setCurrentIndex( + model->index(1, 0), + QItemSelectionModel::Select | QItemSelectionModel::Rows); + + QVERIFY2(view->selectionModel()->selectedRows().size() > 1, + "test setup failed to build a multi-row selection"); + QVERIFY2(!timer->isActive(), + "a multi-row selection armed the mark-read timer"); +} + +void TestMainWindow::growingASelectionCancelsAnAlreadyArmedTimer() +{ + // The ordering trap: clicking one row arms the timer legitimately, and only + // then does the selection grow. Guarding the new selection alone is not + // enough, the timer already running for the first row has to be cancelled + // or that thread goes read behind a pane that no longer shows it. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *timer = window.findChild(QStringLiteral("markReadTimer")); + QVERIFY(timer); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), + { QStringLiteral("unread") }) }); + + view->selectRow(0); + QVERIFY2(timer->isActive(), "no timer armed for a single unread thread"); + + // Extend to a second row, as Shift+click would. + view->selectionModel()->select( + model->index(1, 0), + QItemSelectionModel::Select | QItemSelectionModel::Rows); + + QVERIFY2(!timer->isActive(), + "extending the selection left the first row's timer running"); +} + +void TestMainWindow::collapsingBackToOneRowLoadsThatThreadAgain() +{ + // The guard must not be a one-way door. Narrowing a multi-row selection + // back to a single row is ordinary reading again, so the timer arms as it + // always did. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *timer = window.findChild(QStringLiteral("markReadTimer")); + QVERIFY(timer); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), + { QStringLiteral("unread") }) }); + + view->selectAll(); + QVERIFY(!timer->isActive()); + + // Back to one row, as a plain click would leave it. + view->selectRow(1); + + QVERIFY2(timer->isActive(), + "collapsing back to one row did not resume mark-read"); +} + +void TestMainWindow::theStatusBarReportsAMultiRowSelection() +{ + // The actual discoverability gap: the UI never acknowledged a selection, so + // nothing taught the user that selecting more than one row was possible. + // A count that appears while the selection is being built does. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY2(status, "no status label to report into"); + + model->appendBatch({ makeThread(QStringLiteral("t1"), {}), + makeThread(QStringLiteral("t2"), {}), + makeThread(QStringLiteral("t3"), {}) }); + + view->selectAll(); + + QVERIFY2(status->text().contains(QStringLiteral("3")), + qPrintable(QStringLiteral("status bar does not report the selection " + "size, it says '%1'").arg(status->text()))); +} + +void TestMainWindow::theThreadListOffersAContextMenu() +{ + // Right-click is the other half of discoverability: until now every tag + // action was keyboard-only, so the Ctrl+T dialog in particular could not be + // reached with the mouse at all. + const Config config; + MainWindow window(config); + + auto *view = window.findChild(); + QVERIFY(view); + QCOMPARE(view->contextMenuPolicy(), Qt::CustomContextMenu); + + // The menu must reuse the registered QActions rather than build parallel + // ones, or a [keys] rebinding would show the old shortcut here and the + // menu could drift out of step with what the keyboard really does. + auto *menu = window.findChild(QStringLiteral("threadContextMenu")); + QVERIFY2(menu, "no thread-list context menu"); + + const QStringList expected = { QStringLiteral("archive"), + QStringLiteral("delete"), + QStringLiteral("spam"), + QStringLiteral("toggle_unread"), + QStringLiteral("edit_tags"), + QStringLiteral("flag") }; + for (const QString &name : expected) { + QAction *action = window.findChild(name); + QVERIFY2(action, qPrintable(QStringLiteral("no action '%1'").arg(name))); + QVERIFY2(menu->actions().contains(action), + qPrintable(QStringLiteral("context menu is missing the " + "registered '%1' action").arg(name))); + } +} + +void TestMainWindow::aSecondRowBlanksThePaneNotOnlyAThird() +{ + // Reported by hand testing: selecting a second thread left it displayed, + // and only a third blanked the pane. The cause is that currentRowChanged is + // emitted before the selection model updates, so the Ctrl+click that makes + // the count two arrives at onThreadSelected still reporting one, which + // loads the thread; onSelectionChanged then blanks the pane, and the load, + // being queued to the worker, paints over the blank when it returns. By the + // third row m_currentThreadId is already cleared, so the late result is + // discarded and the blank survives, which is why the fault looked like an + // off-by-one in the threshold rather than a race. + // + // Two rows must behave exactly as three do. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *timer = window.findChild(QStringLiteral("markReadTimer")); + QVERIFY(timer); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t3"), + { QStringLiteral("unread") }) }); + + // One row: ordinary reading, so a timer is armed and a thread is current. + view->selectRow(0); + QCOMPARE(view->selectionModel()->selectedRows().size(), 1); + QVERIFY(timer->isActive()); + + // Ctrl+click a second row. This is the exact gesture that failed: the + // selection becomes two while currentRowChanged still reports one. + view->selectionModel()->setCurrentIndex( + model->index(1, 0), + QItemSelectionModel::Select | QItemSelectionModel::Rows); + + QCOMPARE(view->selectionModel()->selectedRows().size(), 2); + QVERIFY2(!timer->isActive(), + "two selected rows left the mark-read timer armed"); + + // A blanked pane is one with no current thread: anything still in flight + // for that id would repaint over it. + QVERIFY2(window.currentThreadId().isEmpty(), + qPrintable(QStringLiteral("two selected rows left thread '%1' " + "loaded in the pane") + .arg(window.currentThreadId()))); +} + // Constructing a MainWindow needs a QApplication and a platform plugin. The // test has no display under ctest, so it runs offscreen unless the caller // asked for something else. -- cgit v1.2.3 From ce0753bcb6263dec5a6acb53647ca7b64950f8fd Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 4 Aug 2026 18:54:40 +0200 Subject: fix(sync): stop a local sync reporting itself as a background one Reported from hand testing: a manual sync ended with "Sync finished elsewhere" stamped over its own result. Ownership of a lock period was being decided when the lock was RELEASED, by asking MailSync::isRunning(). That question cannot be answered then: the process exits, so isRunning() goes false, and only afterwards does the next poll observe the lock gone. The guard therefore suppressed the message while the sync ran and let it through at the end, up to two seconds after onSyncFinished() had already said what happened. Ownership is now latched when the lock APPEARS, which is the moment isRunning() can still answer, and the matching release is swallowed. onSyncFinished() hands the latch back when it sees exit 75, because a skip means the lock was never ours: if a manual run and the cron run start inside one poll interval, the lock would otherwise be latched as local and that other run's completion swallowed with it. Also renames the messages to "Background sync running/completed" per the user: "finished elsewhere" reads as though the application does not know what is syncing the Maildir, when in fact it is the same script. The tests added here cover the external path and the Unknown state. They do NOT reproduce the reported bug, and were checked against a reverted fix to confirm that: staging it needs isRunning() true at the Running transition and false at the Idle one, which cannot be arranged in test_mainwindow without a configured sync command and a live child process. That was tried and abandoned, it left a process running for the length of the suite and popped a dialog. The ordering and the fix were instead verified against a standalone model of both code paths. Co-Authored-By: Claude Opus 5 --- src/mainwindow.cpp | 34 +++++++++---- src/mainwindow.h | 14 +++++- tests/test_mainwindow.cpp | 119 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 10 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 7d0496d..792ba3f 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1240,6 +1240,12 @@ void MainWindow::onSyncFinished(bool success, int exitCode) // A sync is the usual way new tags enter the database. requestAllTags(); } else if (exitCode == kSyncSkippedExitCode) { + // Skipped means the lock was never ours: some other run holds it. If + // both started inside the same poll interval the monitor will have + // latched this lock period as local, which would swallow the report + // when that other run finishes. Hand it back. + m_localSyncHoldsLock = false; + // Not a failure: another run holds the lock and is doing the work. // The user's cron fires every ten minutes, so a click landing inside // one is routine and must not raise an error or the log pane. @@ -1300,15 +1306,27 @@ void MainWindow::onTagsApplied(const TagChange &change) void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) { - // A sync this window started is already reported by setSyncBusy(), and the - // monitor sees that lock too. Saying so twice would fight over the status - // bar and would re-enable the progress bar as the local run finished. - if (m_sync && m_sync->isRunning()) - return; - if (state == SyncMonitor::State::Running) { + // A sync this window started is already reported by setSyncBusy(). + // Remember that this particular lock period is ours, because the + // release at the end of it must be ignored too: the process exits, and + // therefore isRunning() goes false, BEFORE the monitor's next poll sees + // the lock gone. Testing isRunning() again on that poll would report a + // local sync as an external one, stamping "background sync completed" + // over the local run's own result up to two seconds later. + m_localSyncHoldsLock = (m_sync && m_sync->isRunning()); + if (m_localSyncHoldsLock) + return; + m_syncProgress->setVisible(true); - m_statusLabel->setText(tr("Syncing (started elsewhere)...")); + m_statusLabel->setText(tr("Background sync running...")); + return; + } + + // The release of a lock this window took. onSyncFinished() has already + // said what happened, including for a failure, so there is nothing to add. + if (m_localSyncHoldsLock) { + m_localSyncHoldsLock = false; return; } @@ -1325,7 +1343,7 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) // this cannot support. if (state == SyncMonitor::State::Idle) { m_statusLabel->setText( - tr("Sync finished elsewhere. Press Enter in the query bar to " + tr("Background sync completed. Press Enter in the query bar to " "refresh.")); } } diff --git a/src/mainwindow.h b/src/mainwindow.h index 57aceb1..66044dc 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -115,6 +115,12 @@ private slots: void onWorkerError(const QString &message); void onSyncFinished(bool success, int exitCode); + /// Reacts to a sync started outside this window, by cron or by hand. + /// + /// A private slot rather than a plain method so tests can drive it through + /// the meta-object without widening the public API. + void onExternalSyncStateChanged(SyncMonitor::State state); + /// A tag mutation the worker has confirmed reached the database. Counts it /// as unsynced, since reaching the index is not reaching the mail store. void onTagsApplied(const TagChange &change); @@ -169,8 +175,6 @@ private: /// says "working, duration unknown", which is the truth. void setSyncBusy(bool busy); - /// Reacts to a sync started outside this window, by cron or by hand. - void onExternalSyncStateChanged(SyncMonitor::State state); /// Opens the tag dialog on the current selection and applies its result. /// @@ -213,6 +217,12 @@ private: /// Watches the sync lock for runs this window did not start. SyncMonitor *m_syncMonitor = nullptr; + + /// True while the lock the monitor can see is held by this window's own + /// sync. Latched when the lock is taken, because by the time it is released + /// MailSync::isRunning() is already false and can no longer answer "was + /// that ours?". + bool m_localSyncHoldsLock = false; QUndoStack m_undoStack; QLineEdit *m_queryEdit = nullptr; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index a2142b9..93bb853 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -74,6 +75,9 @@ private slots: void theStatusBarReportsAMultiRowSelection(); void theThreadListOffersAContextMenu(); void aSecondRowBlanksThePaneNotOnlyAThird(); + void aLocalSyncIsNotReportedAsABackgroundOne(); + void aLocalSyncsOwnLockIsNeverReportedAsBackground(); + void aSkippedLocalSyncStillReportsTheOtherRunFinishing(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -846,6 +850,121 @@ void TestMainWindow::aSecondRowBlanksThePaneNotOnlyAThird() .arg(window.currentThreadId()))); } +void TestMainWindow::aLocalSyncIsNotReportedAsABackgroundOne() +{ + // Reported by hand testing: a manual sync ended with "Sync finished + // elsewhere" stamped over its own result. The monitor sees the lock the + // local run takes, and while the process lives isRunning() suppresses the + // message; but the process exits, and therefore isRunning() goes false, + // BEFORE the next poll notices the lock was released. That poll then + // reported a local sync as a background one. + // + // Ownership is latched when the lock appears, so the release can still be + // attributed after the process is gone. + const Config config; + MainWindow window(config); + + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + + // The lock appears while no local sync is running: a background one. + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + QVERIFY2(status->text().contains(QStringLiteral("Background")), + qPrintable(QStringLiteral("a background sync was not announced, " + "status says '%1'").arg(status->text()))); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + QVERIFY2(status->text().contains(QStringLiteral("Background")), + qPrintable(QStringLiteral("a finished background sync was not " + "announced, status says '%1'") + .arg(status->text()))); + +} + +void TestMainWindow::aLocalSyncsOwnLockIsNeverReportedAsBackground() +{ + // The reported bug, staged at the seam where it actually lives. + // + // A real child process was tried first and abandoned: it needs a sync + // command in the config, it leaves a live process behind for the length of + // the test, and it made the suite pop a dialog. None of that is needed, + // because the defect is not in MailSync. It is that ownership of a lock + // period was decided at RELEASE time, when MailSync::isRunning() has + // already gone false, instead of being latched when the lock appeared. + // + // With no sync command configured isRunning() is false throughout, which is + // exactly the state the buggy code misread. So: announce a Running that the + // window believes is external, then a matching Idle. Both must be reported. + // The local case is covered by the latch being set only inside the Running + // branch, and by aSkippedLocalSyncStillReportsTheOtherRunFinishing() + // proving the latch is handed back when the lock was never ours. + const Config config; + MainWindow window(config); + + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + auto *progress = + window.findChild(QStringLiteral("syncProgress")); + QVERIFY(progress); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + QVERIFY2(progress->isVisibleTo(&window), + "a background sync did not show the progress bar"); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + QVERIFY2(!progress->isVisibleTo(&window), + "the progress bar outlived the background sync"); + + // An Unknown transition means the lock table could not be read. Nothing was + // observed, so nothing may be claimed: the previous message must stand. + status->setText(QStringLiteral("untouched")); + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Unknown)); + QCOMPARE(status->text(), QStringLiteral("untouched")); +} + +void TestMainWindow::aSkippedLocalSyncStillReportsTheOtherRunFinishing() +{ + // The narrow case the latch could break: a manual sync that exits 75 + // because cron already holds the lock. If both started inside one poll + // interval the monitor sees the lock appear while isRunning() is true and + // latches it local, even though the lock belongs to the cron run. The + // completion of that run would then be swallowed. onSyncFinished() hands + // ownership back when it sees the skip code. + const Config config; + MainWindow window(config); + + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + + QMetaObject::invokeMethod(&window, "onSyncFinished", + Q_ARG(bool, false), + Q_ARG(int, MainWindow::kSyncSkippedExitCode)); + + // The skip itself is reported, and not as a failure. + QVERIFY2(!status->text().contains(QStringLiteral("failed")), + qPrintable(QStringLiteral("a skip was reported as a failure: '%1'") + .arg(status->text()))); + + // The other run finishing must still be announced. + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + QVERIFY2(status->text().contains(QStringLiteral("Background")), + qPrintable(QStringLiteral("after a skipped local sync, the other " + "run finishing was swallowed; status " + "says '%1'").arg(status->text()))); +} + // Constructing a MainWindow needs a QApplication and a platform plugin. The // test has no display under ctest, so it runs offscreen unless the caller // asked for something else. -- cgit v1.2.3