summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/mainwindow.cpp34
-rw-r--r--src/mainwindow.h14
-rw-r--r--tests/test_mainwindow.cpp119
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 <QLabel>
#include <QLineEdit>
#include <QMenu>
+#include <QProgressBar>
#include <QFile>
#include <QSettings>
#include <QStandardPaths>
@@ -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<QLabel *>(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<QLabel *>(QStringLiteral("statusMessage"));
+ QVERIFY(status);
+ auto *progress =
+ window.findChild<QProgressBar *>(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<QLabel *>(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.