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 --- src/keymap.cpp | 4 + src/mainwindow.cpp | 147 ++++++++++++++++++++++++++ src/mainwindow.h | 23 +++++ tests/test_mainwindow.cpp | 255 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 429 insertions(+) diff --git a/src/keymap.cpp b/src/keymap.cpp index 0901cfb..dcb63a8 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -35,6 +35,7 @@ QStringList KeyMap::knownActions() QStringLiteral("flag"), QStringLiteral("focus_query"), QStringLiteral("complete_query"), + QStringLiteral("select_all"), QStringLiteral("toggle_html"), QStringLiteral("load_remote"), QStringLiteral("message_details"), @@ -72,6 +73,9 @@ QList> KeyMap::defaultBindings() // shells and editors, and it is a named key rather than a symbol, so // no layout has to shift it. { QStringLiteral("Ctrl+Space"), QStringLiteral("complete_query") }, + // The conventional select-all key, and free here: the thread list is a + // read-only view, so nothing else in the window wants it. + { QStringLiteral("Ctrl+A"), QStringLiteral("select_all") }, { QStringLiteral("Ctrl+H"), QStringLiteral("toggle_html") }, { QStringLiteral("Ctrl+M"), QStringLiteral("load_remote") }, // Shifted because Ctrl+D is delete. Both are "D for details/delete" diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 0a9f9d8..b3b67c5 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -325,6 +325,7 @@ void MainWindow::buildUi() // The status label is created first: the sync wiring below can report into // it before the rest of the UI exists. m_statusLabel = new QLabel(this); + m_statusLabel->setObjectName(QStringLiteral("statusMessage")); statusBar()->addWidget(m_statusLabel); // Beside the sync status rather than as a widget competing with it: the two @@ -488,6 +489,16 @@ void MainWindow::buildUi() &QItemSelectionModel::currentRowChanged, this, &MainWindow::onThreadSelected); + // Separate from currentRowChanged: a selection can grow without current + // moving at all. Ctrl+click adds a row and leaves current where it was, and + // selectAll() emits no currentRowChanged whatsoever (verified against + // Qt 6.11). Both are multi-select gestures that have to blank the pane and + // cancel a pending mark-read, so neither can rely on the current-index + // signal to notice them. + connect(m_threadView->selectionModel(), + &QItemSelectionModel::selectionChanged, + this, &MainWindow::onSelectionChanged); + m_messageView = new MessageView(central); m_messageView->setTagColors(&m_tagColors); connect(m_messageView, &MessageView::statusMessage, @@ -659,6 +670,14 @@ void MainWindow::registerActions() m_queryEdit->setFocus(); m_queryCompleter->triggerCompletion(); }); + addAction(QStringLiteral("select_all"), tr("Select &all threads"), + tr("Select every thread in the current result list"), [this]() { + // A registered action rather than the view's built-in SelectAll key, so + // it reaches the Edit menu, the shortcut reference and [keys] the same + // way every other binding does. That is the whole point: multi-select + // already worked, it was simply invisible. + m_threadView->selectAll(); + }); addAction(QStringLiteral("quit"), tr("&Quit"), tr("Quit qtmaildir"), [this]() { close(); }); @@ -680,6 +699,8 @@ void MainWindow::buildMenus() editMenu->addSeparator(); editMenu->addAction(m_actions.value(QStringLiteral("focus_query"))); editMenu->addAction(m_actions.value(QStringLiteral("complete_query"))); + editMenu->addSeparator(); + editMenu->addAction(m_actions.value(QStringLiteral("select_all"))); auto *messageMenu = menuBar()->addMenu(tr("&Message")); messageMenu->addAction(m_actions.value(QStringLiteral("archive"))); @@ -731,6 +752,29 @@ void MainWindow::buildMenus() action->setIcon(icon); } + // Right-click on the thread list. Built from the same registered QActions + // as the menu bar, never from parallel copies: a [keys] override then shows + // the right shortcut here too, and an action cannot end up doing one thing + // from the menu bar and another from the context menu. + // + // Every entry applies to the whole selection already, since they all funnel + // through tagSelected(), so this needs no multi-row special casing. + m_threadContextMenu = new QMenu(this); + m_threadContextMenu->setObjectName(QStringLiteral("threadContextMenu")); + m_threadContextMenu->addAction(m_actions.value(QStringLiteral("archive"))); + m_threadContextMenu->addAction(m_actions.value(QStringLiteral("delete"))); + m_threadContextMenu->addAction(m_actions.value(QStringLiteral("spam"))); + m_threadContextMenu->addSeparator(); + m_threadContextMenu->addAction(m_actions.value(QStringLiteral("toggle_unread"))); + m_threadContextMenu->addAction(m_actions.value(QStringLiteral("flag"))); + m_threadContextMenu->addAction(m_actions.value(QStringLiteral("edit_tags"))); + m_threadContextMenu->addSeparator(); + m_threadContextMenu->addAction(m_actions.value(QStringLiteral("select_all"))); + + m_threadView->setContextMenuPolicy(Qt::CustomContextMenu); + connect(m_threadView, &QTableView::customContextMenuRequested, + this, &MainWindow::showThreadContextMenu); + // The frequent subset only. A toolbar holding every action is as // unreadable as no toolbar. auto *toolBar = addToolBar(tr("Main")); @@ -787,6 +831,18 @@ void MainWindow::showShortcutReference() "") .arg(left, right)); + // Mouse selection is view behaviour, not an action, so it cannot appear in + // the table above however the table is generated. Said here because it is + // otherwise undiscoverable: nothing in the UI hints that a thread list + // takes more than one row at a time. + auto *selectionNote = new QLabel( + tr("Thread list: Ctrl+click adds or removes a single " + "row, Shift+click extends the selection to a range. Tag, " + "archive and delete all apply to every selected thread."), + &dialog); + selectionNote->setTextFormat(Qt::RichText); + selectionNote->setWordWrap(true); + auto *note = new QLabel( tr("Rebind any of these in the [keys] section of " "qtmaildir.conf, using the action name."), @@ -799,6 +855,7 @@ void MainWindow::showShortcutReference() auto *layout = new QVBoxLayout(&dialog); layout->addWidget(label); + layout->addWidget(selectionNote); layout->addWidget(note); layout->addStretch(); layout->addWidget(buttons); @@ -973,12 +1030,94 @@ void MainWindow::onQueryFinished(int total, quint64 generation) m_statusLabel->setText(tr("%n thread(s)", "", total)); } +void MainWindow::showThreadContextMenu(const QPoint &pos) +{ + const QModelIndex index = m_threadView->indexAt(pos); + if (!index.isValid()) + return; // Right-click on empty space below the rows. + + // Right-clicking a row that is already part of the selection must leave + // that selection alone: the actions apply to every selected thread, so + // collapsing to the clicked row here would silently narrow a deliberate + // multi-row selection to one. Right-clicking outside it selects that row + // instead, which is what every other list does. + if (!m_threadView->selectionModel()->isRowSelected(index.row())) + m_threadView->selectRow(index.row()); + + m_threadContextMenu->popup(m_threadView->viewport()->mapToGlobal(pos)); +} + +void MainWindow::onSelectionChanged() +{ + const int selected = m_threadView->selectionModel()->selectedRows().size(); + if (selected <= 1) { + // Clearing the count here would wipe whatever the last action reported + // ("Archive: 3 threads"), which is the more useful message once the + // selection is gone. Only a count this function wrote is taken back. + if (m_statusLabel->text() == m_selectionMessage) + m_statusLabel->clear(); + m_selectionMessage.clear(); + + // Collapsing a multi-row selection back to one row has to load that + // row here, and cannot be left to onThreadSelected. currentRowChanged + // is emitted BEFORE the selection model is updated (verified against + // Qt 6.11), so when a click collapses three rows to one, that handler + // still sees three selected, takes the multi-select branch and returns + // without loading anything. Only this signal sees the real count. + const QModelIndex current = m_threadView->currentIndex(); + if (current.isValid() + && m_model->threadAt(current.row()).threadId != m_currentThreadId) { + onThreadSelected(current, QModelIndex()); + } + return; + } + + // The count is the part that actually teaches multi-select: it acknowledges + // the selection while it is being built, rather than only after an action + // has already been applied to it. + m_selectionMessage = tr("%n thread(s) selected", "", selected); + m_statusLabel->setText(m_selectionMessage); + + // Ctrl+click and selectAll() reach a multi-row selection without moving + // current, so onThreadSelected never runs and its guard never fires. The + // pane and the pending timer have to be dealt with here as well. + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + m_currentThreadId.clear(); + m_messageView->clear(); +} + void MainWindow::onThreadSelected(const QModelIndex ¤t, const QModelIndex &) { if (!current.isValid()) return; + // A selection spanning more than one row is aimed at a bulk action, not at + // reading. current follows the keyboard cursor as the selection extends, so + // without this every row swept through would be rendered and, worse, + // queued to be marked read: a selection gesture must not mutate mail. + // + // The count read here is deliberately not trusted on its own. This signal + // is emitted BEFORE the selection model is updated (verified against + // Qt 6.11), so a Ctrl+click that takes the selection from one row to two + // arrives here still reporting one. onSelectionChanged() always follows and + // sees the true count, and it is what finally blanks the pane and cancels + // the timer; this branch only catches the case where the count is already + // stale in the other direction. + // + // The stop() is not redundant with the guard. Clicking one row arms a timer + // legitimately and only then does the selection grow, so the timer already + // running for that first row has to be cancelled here or it fires behind a + // pane that no longer shows the thread. + if (m_threadView->selectionModel()->selectedRows().size() > 1) { + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + m_currentThreadId.clear(); + m_messageView->clear(); + return; + } + const ThreadSummary thread = m_model->threadAt(current.row()); m_currentThreadId = thread.threadId; m_messageView->setTags(thread.tags); @@ -995,6 +1134,14 @@ void MainWindow::onThreadLoaded(const QVector &messages, if (generation != m_generation || messages.isEmpty()) return; + // A load started while the selection was still a single row can land after + // it has grown: loadThread crosses to the worker on a queued connection, so + // the reply arrives after onSelectionChanged() has already blanked the + // pane. Without this it would paint a thread back over the blank, and the + // pane would only look right once a third row made the count stale-proof. + if (m_threadView->selectionModel()->selectedRows().size() > 1) + return; + MimeParser parser; QList items; items.reserve(messages.size()); diff --git a/src/mainwindow.h b/src/mainwindow.h index d1f7b1c..933c71b 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -33,6 +33,7 @@ class QAction; class QLineEdit; +class QMenu; class QTableView; class QLabel; class QPushButton; @@ -60,6 +61,12 @@ public: /// really registered. QStringList registeredActionNames() const; + /// The thread currently shown in the message pane, empty when it is blank. + /// + /// Empty is what "the pane is blanked" means internally: a late-arriving + /// load is discarded rather than painted, so no thread can reappear. + QString currentThreadId() const { return m_currentThreadId; } + /// The cid: namespace prefix for the nth message of a thread. /// /// MainWindow is the only producer of this value in the application. It @@ -95,6 +102,14 @@ private slots: void onThreadsReady(const QVector &threads, quint64 generation); void onQueryFinished(int total, quint64 generation); void onThreadSelected(const QModelIndex ¤t, const QModelIndex &previous); + + /// Keeps the status bar's selection count and the multi-select guard in + /// step with selections that never move the current index. + void onSelectionChanged(); + + /// Pops up the thread-list context menu, preserving a multi-row selection + /// the click lands inside. + void showThreadContextMenu(const QPoint &pos); void onThreadLoaded(const QVector &messages, quint64 generation); void onWorkerError(const QString &message); void onSyncFinished(bool success, int exitCode); @@ -196,6 +211,10 @@ private: QLineEdit *m_queryEdit = nullptr; QueryCompleter *m_queryCompleter = nullptr; QTableView *m_threadView = nullptr; + + /// Right-click menu for the thread list, holding the same QActions the + /// menu bar does. + QMenu *m_threadContextMenu = nullptr; QSplitter *m_splitter = nullptr; QComboBox *m_accountBox = nullptr; QPushButton *m_syncButton = nullptr; @@ -230,6 +249,10 @@ private: QString m_lastQuery; QString m_currentThreadId; + /// The selection count last written to the status bar, so it can be taken + /// back without clobbering a message some other action put there. + QString m_selectionMessage; + /// Confirmed tag mutations not yet known to have reached the mail store. /// /// A count of its own rather than QUndoStack::isClean(), which cannot serve 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 a86de2d924d68451c1db05c3055affe4eb654464 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 4 Aug 2026 18:17:54 +0200 Subject: docs: record the two selection-model traps found in 987a9e7 currentRowChanged fires before the selection model updates, and selectAll() emits it not at all. Both cost real debugging, and the first caused two distinct faults in one change, so they belong beside the setCompleter trap rather than only in commit history. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 9eb21a4..10c0846 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -119,6 +119,27 @@ letting QCompleter overwrite the field. This has been hit twice, in either class. A test that uses `setText()` passes against the bug, since `setText` does not drive a completer at all: the keys must be typed. +**`QItemSelectionModel::currentRowChanged` is emitted BEFORE the selection model is +updated.** A handler on it reading `selectedRows()` sees the *previous* selection, not the +one the user just made. Verified against Qt 6.11. This produced two separate faults in one +change (987a9e7): a Ctrl+click taking a selection from one row to two arrived reporting +one, and a click collapsing three rows to one arrived reporting three. Any decision that +depends on how many rows are selected belongs in a `selectionChanged` handler, which does +see the true count; `currentRowChanged` is only safe for "which row is current". + +The related trap: **`selectAll()` emits no `currentRowChanged` at all** and leaves the +current index invalid when nothing was current. A test that calls `selectAll()` on a fresh +view therefore passes against a missing selection guard, because no signal ever fires. Test +multi-select from a row that is already current, which is also how a user reaches it. + +**A queued load can outlive the state that started it.** `loadThread` crosses to the worker +on a queued connection, so its reply lands after whatever the UI did in the meantime. The +generation counter covers a superseded *query*, not a superseded *selection*: blanking the +pane and then receiving an in-flight thread repaints it. `onThreadLoaded` therefore drops a +reply that arrives while more than one row is selected. This class of bug cannot be +reproduced in `test_mainwindow`, which has no worker and never fires `threadLoaded`; it +needs the notmuch fixture or a hand test. + **Do not conclude a key binding is dead from `QTest::keyClick()`.** Whether a symbol needs Shift is a layout property, not a Qt one. `Ctrl++` is the shipped `zoom_in` default and is exactly what the `+` key emits on an Italian layout, while synthetic input never delivers -- cgit v1.2.3 From 61b85ed8a43971adba30be602902a3efb96c4ce7 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 4 Aug 2026 18:25:37 +0200 Subject: feat(sync): notice syncs this window did not start The user's cron runs mailsync.sh every ten minutes, so mail arrives and tags change while the window sits idle, and nothing here noticed. The script already holds an flock for the whole run, so that lock is the signal: no status file is needed, and a kernel lock cannot go stale because it dies with the process holding it. The observation method is the part that matters, and two of the three plausible ones are wrong. Both were probed on Linux 6.18 before any of this was written: - flock -n acquires in order to test, so polling every two seconds would open a window every two seconds in which a starting mailsync.sh is refused the lock and exits 75. It would cause the very skips the script reports. - fcntl(F_OFD_GETLK) never acquires and looks ideal, but reports UNLOCKED against a lock held by flock(2): separate lock namespaces in the kernel, which cannot see each other. A silent false negative. - /proc/locks is a pure read. It observes flock(2) correctly, and since it takes no lock at all it can never contend with the Xapian write lock notmuch new holds during the same run. Confirmed: 200 reads left the lock table unchanged and this process holding nothing. SyncMonitor keeps the parsing separate from the polling so the parsing is testable, and it reports Unknown rather than Idle where /proc/locks cannot be read: "no sync is running" is the claim that would let the window quit, so it must never be guessed. Verified against a real flock end to end, not only against synthetic content. It reports rather than refreshes. runCurrentQuery() clears the undo stack, the selection and the message pane, which is right for a query the user typed and hostile for one a cron timer fired: it would discard undo history and close the thread being read up to six times an hour, with no action from the user. Co-Authored-By: Claude Opus 5 --- src/CMakeLists.txt | 1 + src/mainwindow.cpp | 41 +++++++++++ src/mainwindow.h | 7 ++ src/syncmonitor.cpp | 151 +++++++++++++++++++++++++++++++++++++ src/syncmonitor.h | 106 ++++++++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/test_syncmonitor.cpp | 180 +++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 487 insertions(+) create mode 100644 src/syncmonitor.cpp create mode 100644 src/syncmonitor.h create mode 100644 tests/test_syncmonitor.cpp diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 5214f42..a71a6f1 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -12,6 +12,7 @@ add_library(qtmaildir_lib STATIC tagstrip.cpp threadlistmodel.cpp mailsync.cpp + syncmonitor.cpp threadcidmap.cpp messageview.cpp mainwindow.cpp diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index b3b67c5..7d0496d 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -431,6 +431,15 @@ void MainWindow::buildUi() m_syncLog->appendPlainText(chunk.trimmed()); }); + // Syncs this window did not start. The user's cron runs the same script + // every ten minutes, so mail arrives and tags change while the window sits + // idle, and until now nothing here noticed. + m_syncMonitor = new SyncMonitor(SyncMonitor::defaultLockPath(), + QStringLiteral("/proc/locks"), this); + connect(m_syncMonitor, &SyncMonitor::stateChanged, + this, &MainWindow::onExternalSyncStateChanged); + m_syncMonitor->start(); + queryRow->addWidget(m_accountBox); queryRow->addWidget(m_queryEdit, 1); queryRow->addWidget(m_syncButton); @@ -1289,6 +1298,38 @@ 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) { + m_syncProgress->setVisible(true); + m_statusLabel->setText(tr("Syncing (started elsewhere)...")); + return; + } + + m_syncProgress->setVisible(false); + + // Deliberately reports rather than refreshes. runCurrentQuery() clears the + // undo stack, the selection and the message pane, which is right for a + // query the user typed and hostile for one fired by a cron timer: with a + // sync every ten minutes it would discard undo history and close the thread + // being read, up to six times an hour, with no action from the user. + // + // Unknown is not worth reporting either. It means the lock table could not + // be read, so nothing was observed, and "sync finished" would be a claim + // this cannot support. + if (state == SyncMonitor::State::Idle) { + m_statusLabel->setText( + tr("Sync finished elsewhere. Press Enter in the query bar to " + "refresh.")); + } +} + void MainWindow::setSyncBusy(bool busy) { m_syncProgress->setVisible(busy); diff --git a/src/mainwindow.h b/src/mainwindow.h index 933c71b..57aceb1 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -28,6 +28,7 @@ #include "config.h" #include "keymap.h" +#include "syncmonitor.h" #include "tagcolors.h" #include "types.h" @@ -168,6 +169,9 @@ 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. /// /// The only route to an arbitrary tag: every other tag action writes a @@ -206,6 +210,9 @@ private: ThreadListModel *m_model = nullptr; MessageView *m_messageView = nullptr; MailSync *m_sync = nullptr; + + /// Watches the sync lock for runs this window did not start. + SyncMonitor *m_syncMonitor = nullptr; QUndoStack m_undoStack; QLineEdit *m_queryEdit = nullptr; diff --git a/src/syncmonitor.cpp b/src/syncmonitor.cpp new file mode 100644 index 0000000..e2883dc --- /dev/null +++ b/src/syncmonitor.cpp @@ -0,0 +1,151 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "syncmonitor.h" + +#include +#include + +#include + +namespace { + +/// Two seconds. The user chose continuous polling over a poll-only-while- +/// quitting variant, and this is a read of one small procfs file, so the cost +/// is negligible next to noticing a cron sync within a couple of seconds. +constexpr int kDefaultIntervalMs = 2000; + +} // namespace + +SyncMonitor::SyncMonitor(const QString &lockPath, const QString &locksPath, + QObject *parent) + : QObject(parent), m_lockPath(lockPath), m_locksPath(locksPath) +{ + m_timer.setInterval(kDefaultIntervalMs); + connect(&m_timer, &QTimer::timeout, this, &SyncMonitor::poll); +} + +void SyncMonitor::setInterval(int ms) +{ + m_timer.setInterval(ms); +} + +void SyncMonitor::start() +{ + // Poll once immediately: a window opened during a cron sync should say so + // at once rather than after the first interval. + poll(); + m_timer.start(); +} + +void SyncMonitor::stop() +{ + m_timer.stop(); +} + +QString SyncMonitor::defaultLockPath() +{ + // Must stay equal to LOCKFILE in assets/mailsync.sh. + return QStringLiteral("/tmp/mbsync.lock"); +} + +qint64 SyncMonitor::inodeOf(const QString &path) +{ + // Qt exposes no inode accessor, and /proc/locks identifies a file only by + // device and inode, so this has to come from stat(2) directly. That is also + // why the whole class is Linux-shaped; see the Unknown state for what + // happens where /proc/locks does not exist. + struct stat st; + if (::stat(QFile::encodeName(path).constData(), &st) != 0) + return -1; + + return static_cast(st.st_ino); +} + +bool SyncMonitor::lockHeldIn(const QString &content, qint64 inode) +{ + if (content.isEmpty() || inode < 0) + return false; + + // A /proc/locks line looks like: + // 82: FLOCK ADVISORY WRITE 9051 fc:00:12058676 0 EOF + // The inode is the last colon-separated part of the major:minor:inode + // field. Matching the raw number anywhere in the line would also match a + // pid or a byte range, so the field is located first and then split. + const QList lines = QStringView(content).split(u'\n', + Qt::SkipEmptyParts); + for (const QStringView &line : lines) { + const QList fields = + line.split(u' ', Qt::SkipEmptyParts); + + // Shortest real line still has: index, type, ADVISORY, WRITE, pid, + // dev:inode. Anything shorter is truncated or not a lock line, and is + // skipped rather than guessed at. + if (fields.size() < 6) + continue; + + // flock(2) only. mailsync.sh uses flock, and a POSIX record lock on + // the same file belongs to somebody else: the two namespaces cannot + // see each other, so treating a POSIX entry as ours would report a + // sync that is not running. + if (fields.at(1) != QLatin1String("FLOCK")) + continue; + + for (const QStringView &field : fields) { + const qsizetype lastColon = field.lastIndexOf(u':'); + if (lastColon < 0) + continue; + + bool ok = false; + const qint64 candidate = + field.mid(lastColon + 1).toLongLong(&ok); + if (ok && candidate == inode) + return true; + } + } + + return false; +} + +void SyncMonitor::poll() +{ + State next = State::Unknown; + + QFile locks(m_locksPath); + if (locks.open(QIODevice::ReadOnly | QIODevice::Text)) { + // Read in full rather than line by line: /proc/locks is small, and a + // partial read while the kernel is editing the table could truncate a + // line mid-field. + const QString content = QString::fromUtf8(locks.readAll()); + + const qint64 inode = inodeOf(m_lockPath); + if (inode < 0) { + // No lock file yet, before the first sync ever runs. The table was + // readable, so this is a real answer and not Unknown. + next = State::Idle; + } else { + next = lockHeldIn(content, inode) ? State::Running : State::Idle; + } + } + + if (next == m_state) + return; + + m_state = next; + emit stateChanged(m_state); +} diff --git a/src/syncmonitor.h b/src/syncmonitor.h new file mode 100644 index 0000000..3aca520 --- /dev/null +++ b/src/syncmonitor.h @@ -0,0 +1,106 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#pragma once + +#include +#include +#include + +/// Notices syncs this process did not start. +/// +/// The user's cron runs mailsync.sh every ten minutes, so mail can appear and +/// tags can change while the window sits idle. The script holds an flock for +/// the whole run, which is already the signal: no status file is needed, and a +/// kernel lock cannot go stale because it dies with the process holding it. +/// +/// **Read the lock, never take it.** Three ways to observe an flock look +/// plausible and two are wrong, both verified on Slackware, Linux 6.18: +/// +/// - `flock -n` acquires in order to test. Polling every two seconds would +/// open a window every two seconds in which a starting mailsync.sh is +/// refused the lock and exits 75. It would cause the very skips the sync +/// script reports. +/// - `fcntl(F_OFD_GETLK)` never acquires, and looks ideal, but reports +/// UNLOCKED against a lock held by flock(2): the two are separate lock +/// namespaces in the kernel and cannot see each other. A silent false +/// negative, which is the worst failure available here. +/// - /proc/locks is a pure read. It observes flock(2) entries correctly and +/// cannot acquire, steal, or contend, so it can also never disturb the +/// Xapian write lock notmuch new holds during the same run. +/// +/// Do not "simplify" this to flock -n. +class SyncMonitor : public QObject +{ + Q_OBJECT +public: + enum class State { + Unknown, ///< The lock table cannot be read; claim nothing. + Idle, ///< Readable, and nothing holds the lock. + Running, ///< Something holds the lock: a sync is in progress. + }; + Q_ENUM(State) + + /// @param lockPath the file mailsync.sh flocks, /tmp/mbsync.lock. + /// @param locksPath the kernel lock table; injectable so tests can drive + /// transitions without holding real locks. + explicit SyncMonitor(const QString &lockPath, + const QString &locksPath = QStringLiteral("/proc/locks"), + QObject *parent = nullptr); + + State state() const { return m_state; } + + /// True only for State::Running. Unknown is deliberately not "running": + /// callers use this to decide whether to wait, and waiting forever on a + /// platform with no /proc/locks would be worse than not noticing a sync. + bool isRunning() const { return m_state == State::Running; } + + void setInterval(int ms); + void start(); + void stop(); + + /// One observation. Public so tests can step it without a running timer. + void poll(); + + /// Whether @p content holds an flock(2) entry for @p inode. + /// + /// Static and content-based: this is the part worth testing, and it is + /// testable only while it is separate from reading the file. + static bool lockHeldIn(const QString &content, qint64 inode); + + /// The inode of @p path, or -1 when it does not exist. + static qint64 inodeOf(const QString &path); + + /// The lock file assets/mailsync.sh takes, and the only one worth watching. + /// + /// Hardcoded to match LOCKFILE in that script. Two sources of truth is the + /// standing hazard here: change one and the monitor silently reports Idle + /// forever, since a missing lock file is a legitimate "no sync running". + static QString defaultLockPath(); + +signals: + /// Emitted only when the state actually changes, never once per poll: the + /// status bar must not be repainted every two seconds forever. + void stateChanged(SyncMonitor::State state); + +private: + QString m_lockPath; + QString m_locksPath; + State m_state = State::Unknown; + QTimer m_timer; +}; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 7f7caa5..0f6ec5a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -16,6 +16,7 @@ add_qtmaildir_test(notmuchworker) add_qtmaildir_test(tagcolors) add_qtmaildir_test(threadlistmodel) add_qtmaildir_test(mailsync) +add_qtmaildir_test(syncmonitor) add_qtmaildir_test(threadcidmap) add_qtmaildir_test(mainwindow) add_qtmaildir_test(messageview) diff --git a/tests/test_syncmonitor.cpp b/tests/test_syncmonitor.cpp new file mode 100644 index 0000000..c120085 --- /dev/null +++ b/tests/test_syncmonitor.cpp @@ -0,0 +1,180 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include + +#include +#include +#include + +#include "syncmonitor.h" + +/// SyncMonitor answers one question: is a sync running that this process did +/// not start? The parsing of /proc/locks is the testable part and is kept +/// separate from the polling for exactly that reason. +class TestSyncMonitor : public QObject +{ + Q_OBJECT +private slots: + void anFlockOnTheWatchedInodeIsHeld(); + void anFlockOnAnotherInodeIsIgnored(); + void aPosixLockOnTheWatchedInodeIsIgnored(); + void emptyContentMeansNotHeld(); + void garbageLinesAreSkippedRatherThanMisread(); + void anUnreadableLockTableIsUnknownNotIdle(); + void aMissingLockFileIsNotHeld(); + void theStateChangeSignalFiresOnlyOnTransitions(); +}; + +void TestSyncMonitor::anFlockOnTheWatchedInodeIsHeld() +{ + // The real shape of a held lock, taken verbatim from /proc/locks while + // mailsync.sh held /tmp/mbsync.lock: the inode is the last colon-separated + // field of the device:inode column, not the whole column. + const QString content = + QStringLiteral("82: FLOCK ADVISORY WRITE 9051 fc:00:12058676 0 EOF\n"); + QVERIFY(SyncMonitor::lockHeldIn(content, 12058676)); +} + +void TestSyncMonitor::anFlockOnAnotherInodeIsIgnored() +{ + // A busy machine has many flocks. Matching anything but our own inode would + // report a sync whenever some unrelated program took a lock. + const QString content = + QStringLiteral("82: FLOCK ADVISORY WRITE 9051 fc:00:99999999 0 EOF\n"); + QVERIFY(!SyncMonitor::lockHeldIn(content, 12058676)); +} + +void TestSyncMonitor::aPosixLockOnTheWatchedInodeIsIgnored() +{ + // flock(2) and fcntl(2) are separate namespaces in the kernel and cannot + // see each other; mailsync.sh uses flock(2). A POSIX lock on the same file + // is somebody else's, and treating it as ours would report a sync that is + // not running. Verified: fcntl(F_OFD_GETLK) reports UNLOCKED against a + // held flock, which is why this distinction is not academic. + const QString content = + QStringLiteral("1: POSIX ADVISORY WRITE 1234 fc:00:12058676 0 EOF\n"); + QVERIFY(!SyncMonitor::lockHeldIn(content, 12058676)); +} + +void TestSyncMonitor::emptyContentMeansNotHeld() +{ + QVERIFY(!SyncMonitor::lockHeldIn(QString(), 12058676)); +} + +void TestSyncMonitor::garbageLinesAreSkippedRatherThanMisread() +{ + // /proc/locks gains fields across kernel versions, and a line can be + // truncated as it is read. A short line must not match by accident, and + // must not stop the lines after it from being read. + const QString content = QStringLiteral( + "not a lock line at all\n" + "3: FLOCK\n" + "82: FLOCK ADVISORY WRITE 9051 fc:00:12058676 0 EOF\n"); + QVERIFY(SyncMonitor::lockHeldIn(content, 12058676)); + + const QString onlyGarbage = QStringLiteral("nonsense\n3: FLOCK\n"); + QVERIFY(!SyncMonitor::lockHeldIn(onlyGarbage, 12058676)); +} + +void TestSyncMonitor::anUnreadableLockTableIsUnknownNotIdle() +{ + // /proc/locks is Linux-only. Where it cannot be read the honest answer is + // "unknown", and the indicator stays hidden. Reporting idle would be a + // claim the monitor cannot support, and it is the claim that matters: + // "no sync is running" is what lets the window quit. + SyncMonitor monitor(QStringLiteral("/nonexistent/mbsync.lock"), + QStringLiteral("/nonexistent/proc/locks")); + QCOMPARE(monitor.state(), SyncMonitor::State::Unknown); + monitor.poll(); + QCOMPARE(monitor.state(), SyncMonitor::State::Unknown); +} + +void TestSyncMonitor::aMissingLockFileIsNotHeld() +{ + // Before the first sync ever runs there is no lock file. That is not a + // sync in progress, and it must not read as unknown either: the lock table + // is perfectly readable, there is simply nothing holding anything. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + + QTemporaryFile locks; + QVERIFY(locks.open()); + locks.write("82: FLOCK ADVISORY WRITE 9051 fc:00:12058676 0 EOF\n"); + locks.flush(); + + SyncMonitor monitor(dir.filePath(QStringLiteral("never-created.lock")), + locks.fileName()); + monitor.poll(); + QCOMPARE(monitor.state(), SyncMonitor::State::Idle); +} + +void TestSyncMonitor::theStateChangeSignalFiresOnlyOnTransitions() +{ + // The UI reacts to a sync starting and finishing, so a signal on every + // poll would repaint the status bar every two seconds forever and would + // stamp over whatever else had been written there. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString lockPath = dir.filePath(QStringLiteral("mbsync.lock")); + { + QFile lock(lockPath); + QVERIFY(lock.open(QIODevice::WriteOnly)); + } + + const qint64 inode = SyncMonitor::inodeOf(lockPath); + QVERIFY(inode > 0); + + // A stand-in for /proc/locks whose contents the test controls. + const QString locksPath = dir.filePath(QStringLiteral("locks")); + auto writeLocks = [&locksPath](const QString &text) { + QFile f(locksPath); + QVERIFY(f.open(QIODevice::WriteOnly | QIODevice::Truncate)); + f.write(text.toUtf8()); + }; + writeLocks(QString()); + + SyncMonitor monitor(lockPath, locksPath); + QSignalSpy spy(&monitor, &SyncMonitor::stateChanged); + + monitor.poll(); + QCOMPARE(monitor.state(), SyncMonitor::State::Idle); + QCOMPARE(spy.count(), 1); // Unknown -> Idle is a real transition. + + monitor.poll(); + monitor.poll(); + QCOMPARE(spy.count(), 1); // Still idle: no further signals. + + writeLocks(QStringLiteral("82: FLOCK ADVISORY WRITE 9051 fc:00:%1 0 EOF\n") + .arg(inode)); + monitor.poll(); + QCOMPARE(monitor.state(), SyncMonitor::State::Running); + QCOMPARE(spy.count(), 2); + + monitor.poll(); + QCOMPARE(spy.count(), 2); // Still running. + + writeLocks(QString()); + monitor.poll(); + QCOMPARE(monitor.state(), SyncMonitor::State::Idle); + QCOMPARE(spy.count(), 3); +} + +QTEST_MAIN(TestSyncMonitor) + +#include "test_syncmonitor.moc" -- 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(-) 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