From 3b0a52b8620d3cead2f527d108ba64bfec8ee273 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 4 Aug 2026 11:16:52 +0200 Subject: feat(sync): show unsynced edits and offer to sync on exit Tagging changes the notmuch index at once, but the mail store only hears about it on the next sync, and nothing said so. Quitting with tagging outstanding was silent. Items 18 and 19 of the usability backlog, built together because the second needs the first's counter. The counter cannot be QUndoStack::isClean(), which is the obvious candidate and the wrong one: the undo stack is cleared on every query, since its entries refer to rows the new result set discards. Tag a thread, run any query, and the stack is empty while the change is still unsynced. m_pendingEdits is its own count, incremented where a write is CONFIRMED rather than where one is sent, so an optimistic update the worker later rejects cannot leave the indicator claiming an edit that never landed. Only a successful sync resets it: clearing on failure would assert the changes had reached the mail store when the sync is exactly what failed to put them there. It is shown in the status bar, hidden entirely at zero, and described as a lower bound rather than a guarantee, since an external notmuch run can carry changes over without this application noticing. On exit, sync_on_exit in [general] takes ask, always or never. Three values rather than a bool because "prompt me", "just do it" and "do nothing" are three behaviours and true/false expresses two; an unknown value warns by name, since a typo there silently changes what happens to unsynced work. The prompt offers three buttons for the same reason: a user who hit Quit by mistake needs a way back that is not "sync". A sync started at exit holds the window open until it finishes rather than being killed mid-run, and a sync that FAILS does not quit, because quitting there would discard the user's choice silently. With no sync command configured the prompt degrades to a plain warning instead of offering a sync that cannot run. This is not a destructive-action confirmation of the kind CLAUDE.md forbids. Those cover tag mutations, which keep undo instead of a dialog. This asks about losing work at the one point where undo cannot help. The tagsApplied lambda became a named slot, which is better structure and also what lets a test drive it: the worker is deliberately parentless because it moves to its own thread, so reaching it with findChild to emit the real signal cannot work, and contorting the test to try was the wrong instinct. Testing a modal needed its own care. A test that sends a close event hangs forever if an unexpected dialog opens, because the modal spins its own event loop; CloseProbe polls for activeModalWidget, closes it and records that one appeared, turning "a dialog opened" into an assertion rather than a hang. Also removes a stray qDebug left in the open_thread action by the earlier Enter-key investigation, which had reached two commits. Co-Authored-By: Claude Opus 5 --- tests/test_config.cpp | 47 +++++++++++++ tests/test_mainwindow.cpp | 163 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 210 insertions(+) (limited to 'tests') diff --git a/tests/test_config.cpp b/tests/test_config.cpp index a09738b..4d92891 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -45,6 +45,9 @@ private slots: void markReadDelayIsActuallyRead(); void markReadDelayAcceptsZeroAndNegative(); void markReadDelayRejectsGarbage(); + void syncOnExitDefaultsToAsk(); + void syncOnExitReadsAllThreeValues(); + void syncOnExitWarnsOnGarbage(); void extraMimetypesAppendToBuiltins(); void extraMimetypeDescriptionMayContainComma(); void malformedExtraMimetypeIsSkipped(); @@ -429,6 +432,50 @@ void TestConfig::markReadDelayRejectsGarbage() QVERIFY(!config.problems().isEmpty()); } +void TestConfig::syncOnExitDefaultsToAsk() +{ + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral("[general]\n"))); + QCOMPARE(config.syncOnExit(), Config::SyncOnExit::Ask); + QVERIFY(config.problems().isEmpty()); +} + +void TestConfig::syncOnExitReadsAllThreeValues() +{ + // A bool could only carry two of these. Each is a distinct behaviour at + // exit, so each has to round-trip. + const QList> cases = { + { QStringLiteral("ask"), Config::SyncOnExit::Ask }, + { QStringLiteral("always"), Config::SyncOnExit::Always }, + { QStringLiteral("never"), Config::SyncOnExit::Never }, + // Case and surrounding space are the user's, not the parser's problem. + { QStringLiteral(" Always "), Config::SyncOnExit::Always }, + }; + + for (const auto &testCase : cases) { + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral("[general]\nsync_on_exit=%1\n") + .arg(testCase.first))); + QCOMPARE(config.syncOnExit(), testCase.second); + QVERIFY2(config.problems().isEmpty(), + qPrintable(QStringLiteral("'%1' warned").arg(testCase.first))); + } +} + +void TestConfig::syncOnExitWarnsOnGarbage() +{ + // Silently falling back would change what happens to unsynced work without + // telling the user, so a typo has to be named. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral("[general]\n" + "sync_on_exit=maybe\n"))); + QCOMPARE(config.syncOnExit(), Config::SyncOnExit::Ask); + QVERIFY(!config.problems().isEmpty()); +} + void TestConfig::extraMimetypesAppendToBuiltins() { QTemporaryDir dir; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index fb33462..d307a99 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -20,8 +20,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -35,6 +37,7 @@ #include "keymap.h" #include "mainwindow.h" #include "messageview.h" +#include "notmuchworker.h" #include "threadlistmodel.h" /// MainWindow is mostly wiring, and the parts that need a real database are @@ -59,6 +62,10 @@ private slots: void markReadTimerRestartsRatherThanStacking(); void markReadTimerIsNotArmedForAReadThread(); void markReadCanBeDisabled(); + void pendingEditCountSurvivesAQuery(); + void aFailedSyncDoesNotClearThePendingCount(); + void closingWithNoPendingEditsDoesNotPrompt(); + void syncOnExitNeverClosesSilently(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -428,6 +435,162 @@ void TestMainWindow::markReadCanBeDisabled() "a negative mark_read_delay_ms must disable the timer"); } +void TestMainWindow::pendingEditCountSurvivesAQuery() +{ + // The defining property, and the reason this is a counter of its own rather + // than QUndoStack::isClean(): the undo stack is cleared on every query, + // because its entries refer to rows the new result set discards. Tag a + // thread, run any query, and the stack is empty while the change is still + // sitting unsynced in the database. + const Config config; + MainWindow window(config); + + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(label); + QVERIFY2(label->isHidden(), "the indicator must start hidden at zero"); + + // Confirm a write the way the worker really does, by emitting the signal + // the window listens to. No test-only entry point on MainWindow. + TagChange change; + change.added = { QStringLiteral("deleted") }; + change.description = QStringLiteral("Delete"); + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, change))); + + QVERIFY2(!label->isHidden(), "a confirmed edit must show the indicator"); + const QString afterEdit = label->text(); + QVERIFY(!afterEdit.isEmpty()); + + // Now run a query, which clears the undo stack. The indicator must not + // follow it down. + window.findChild()->setText(QStringLiteral("tag:inbox")); + QMetaObject::invokeMethod(&window, "runCurrentQuery"); + + QVERIFY2(!label->isHidden(), + "the indicator was cleared by a query, so it is tracking the undo " + "stack rather than unsynced state"); + QCOMPARE(label->text(), afterEdit); +} + +void TestMainWindow::aFailedSyncDoesNotClearThePendingCount() +{ + // A failed sync means the edits are still unsynced. Clearing here would + // assert the opposite, and the user would quit believing their tagging had + // been carried over. + const Config config; + MainWindow window(config); + + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(label); + + TagChange change; + change.added = { QStringLiteral("flagged") }; + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, change))); + QVERIFY(!label->isHidden()); + const QString afterEdit = label->text(); + + QMetaObject::invokeMethod(&window, "onSyncFinished", + Q_ARG(bool, false), Q_ARG(int, 1)); + QVERIFY2(!label->isHidden(), "a FAILED sync cleared the pending count"); + QCOMPARE(label->text(), afterEdit); + + // A successful one does clear it, so this is not "never clears". + QMetaObject::invokeMethod(&window, "onSyncFinished", + Q_ARG(bool, true), Q_ARG(int, 0)); + QVERIFY2(label->isHidden(), "a successful sync must clear the indicator"); +} + +/// Closes a window and reports whether it accepted, failing rather than hanging +/// if a modal appears. +/// +/// A modal spins its own event loop, so a test that simply sends a close event +/// blocks forever when a dialog it did not expect opens. This arms a timer that +/// closes any active modal and records that one was there, which turns "a +/// dialog appeared" into an assertion instead of a hung run. +struct CloseProbe +{ + bool accepted = false; + bool sawModal = false; + + void run(MainWindow *window) + { + QTimer poll; + poll.setInterval(50); + int ticks = 0; + QObject::connect(&poll, &QTimer::timeout, [this, &poll, &ticks]() { + if (QWidget *modal = QApplication::activeModalWidget()) { + sawModal = true; + modal->close(); + poll.stop(); + return; + } + if (++ticks > 20) // one second is ample for a synchronous close + poll.stop(); + }); + poll.start(); + + QCloseEvent event; + QApplication::sendEvent(window, &event); + accepted = event.isAccepted(); + poll.stop(); + } +}; + +void TestMainWindow::closingWithNoPendingEditsDoesNotPrompt() +{ + // Nothing outstanding means nothing to ask about. If a prompt fires here it + // is keying off something other than there being work to lose, and every + // quit would carry a dialog. + const Config config; + MainWindow window(config); + + CloseProbe probe; + probe.run(&window); + + QVERIFY2(!probe.sawModal, "a clean window prompted on close"); + QVERIFY2(probe.accepted, "a clean window refused to close"); +} + +void TestMainWindow::syncOnExitNeverClosesSilently() +{ + // "never" is the behaviour that existed before the prompt did, and it has + // to stay reachable for anyone who does not want to be asked. It must hold + // whether or not a sync command is configured, so this covers both: the + // no-command path has its own dialog, and "never" must skip that one too. + QTemporaryDir dir; + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + { + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("[general]\nsync_on_exit=never\n\n[sync]\ncommand=/bin/true\n"); + } + + Config config; + config.load(path); + QCOMPARE(config.syncOnExit(), Config::SyncOnExit::Never); + + MainWindow window(config); + + // Give it something to lose, so this is not passing for the same reason + // the previous test does. + TagChange change; + change.added = { QStringLiteral("deleted") }; + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, change))); + auto *label = window.findChild(QStringLiteral("pendingEdits")); + QVERIFY(label); + QVERIFY(!label->isHidden()); + + CloseProbe probe; + probe.run(&window); + + QVERIFY2(!probe.sawModal, + "sync_on_exit=never prompted anyway"); + QVERIFY2(probe.accepted, + "sync_on_exit=never must close without prompting"); +} + // 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