diff options
| -rw-r--r-- | README.md | 11 | ||||
| -rwxr-xr-x | assets/mailsync.sh | 9 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 68 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 93 | ||||
| -rw-r--r-- | src/mainwindow.h | 25 |
5 files changed, 197 insertions, 9 deletions
@@ -306,6 +306,17 @@ Two things any replacement has to get right, both learned the hard way: it reports success, clears the unsynced-changes count, and will quit on it during a sync-on-exit. The previous version ended in an unconditional `exit 0`, so a failed `mbsync` was indistinguishable from a clean run. +- **Exit 75 when another run holds the lock**, rather than 1. A skip is not a + failure: the other run is doing the work, and with a timer every ten minutes + a click landing inside one is routine. qtmaildir reports 75 as "a sync is + already running" and leaves the log pane alone, where any other non-zero code + raises an error. + +While a sync this window started is running, the status bar shows an +indeterminate progress bar. It is deliberately not a percentage: `mbsync` +reports no progress, so a bar filling left to right would be inventing one. A +sync started outside the application, by your cron timer, is not currently +visible here at all. ## Unsynced changes diff --git a/assets/mailsync.sh b/assets/mailsync.sh index d5f51ee..2134439 100755 --- a/assets/mailsync.sh +++ b/assets/mailsync.sh @@ -51,9 +51,12 @@ if ! flock -n 200; then # Both streams again: a caller that skipped because the cron run holds # the lock needs to be told, not left with silence and an error code. msg="$(date -Iseconds) === SKIPPED: previous run still in progress ===" - echo "$msg" >> "$LOGFILE" - echo "$msg" >&2 - exit 1 + echo "$msg" | tee -a "$LOGFILE" >&2 + # 75 (EX_TEMPFAIL), not 1. A skip is not a failure: the other run is + # doing the work. qtmaildir reports 1 as "sync failed" and shows its log + # pane, which is wrong for a click that landed during the cron run, and + # cron fires every ten minutes so that overlap is routine. + exit 75 fi # Statuses are written to files rather than shell variables because the diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 4df48dd..5fcd6d8 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -65,6 +65,7 @@ taking that too literally. | 24 | No right-click actions on the thread list | discoverability | S | open | | 25 | No select-all, and bulk actions are undiscoverable | workflow | S | open | | 26 | No way to add or remove an arbitrary tag from the UI | workflow | S | **done** | +| 27 | The UI cannot see a sync it did not start | feedback | S | open | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1192,6 +1193,73 @@ character. The test was corrected, not the validator. Rendered and inspected rather than only asserted. +## 27. The UI cannot see a sync it did not start + +**Observed (user, 2026-08-04), as a question:** "will the status bar be aware of +cronjob fired syncs?" It is not. The progress bar and the busy state are driven +by `MailSync`'s own `QProcess`, so they know only about syncs this window +started. The user's cron timer fires every ten minutes and the application is +blind to it. + +**The lock file is already the signal; no new file is needed.** `mailsync.sh` +does `exec 200>"$LOCKFILE"` and then `flock -n 200` on `/tmp/mbsync.lock`. +Another process can test whether that lock is held, without disturbing it, by +opening the same path on its own descriptor and attempting `flock(LOCK_EX | +LOCK_NB)`: the attempt fails exactly when someone holds it, and succeeds +otherwise, in which case the tester drops it again immediately. + +This is better than the status file first proposed. A kernel lock **cannot go +stale**: it is released when the holding process dies, however it dies. A status +file written by the script survives a `kill -9` and would leave the UI claiming +a sync is running forever, which then needs a heartbeat and a staleness +timeout, none of which the lock needs. + +**Decided (user, 2026-08-04): poll continuously, not only while quitting.** A +narrower version that polled only during the exit prompt was offered and +declined: the user wants the status bar informed at all times, not only at the +one moment it changes a decision. + +**Approach.** + +- A `syncLockHeld()` helper: `open()` the lock path, `flock(LOCK_EX|LOCK_NB)`, + close. Held when the attempt fails with `EWOULDBLOCK`. +- A `QTimer` polling it. **One second is finer than this needs**; a sync runs + for tens of seconds, so two seconds is plenty and halves a wakeup that never + stops. +- When the lock is held and `MailSync` is NOT the holder, show the busy state + with wording that says so: "Syncing (started elsewhere)". The existing + `setSyncBusy()` covers the widgets; this adds a third state between "idle" + and "we are syncing". +- On the exit path, this replaces a hedge with a fact. Today, quitting while + cron syncs says the window "cannot see it finish". With the lock watched it + can wait for the lock to clear and then quit, which is what the user actually + wants to happen. + +**Verify before building, both cheap and both the kind of assumption this +project has been bitten by twice:** + +- **That the lock is observable at all.** `flock` semantics across processes, + where the holder opened the file with `exec 200>`, are an assumption about + Linux behaviour, not a certainty. A twenty-line probe settles it. Do not + design on top of it unproven. +- **That polling it cannot disturb notmuch.** `mailsync.sh` runs `notmuch new`, + and notmuch's write lock is process-exclusive. Touching `/tmp/mbsync.lock` + should be unrelated, but the interaction is worth one look rather than an + assumption. + +**Constraints.** + +- The lock path becomes a contract between the script and the application, + where today the application knows nothing about it. It has to be documented + on both sides, and the script cannot move or rename it casually afterwards. +- The poll must not run a query or touch the database. It reports what another + process is doing; the existing `runCurrentQuery()` on a completed sync of our + own already handles refreshing, and a cron sync that finishes will be picked + up the next time the user runs a query. +- Do not report an externally-started sync as one this window can cancel. The + Sync button should be disabled while the lock is held, since starting one + would only produce the `EX_TEMPFAIL` skip. + --- ## Deferred, unsized, or split out diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 000be85..0a9f9d8 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -36,6 +36,7 @@ #include <QMenuBar> #include <QMessageBox> #include <QPlainTextEdit> +#include <QProgressBar> #include <QPushButton> #include <QSettings> #include <QSplitter> @@ -193,6 +194,8 @@ void MainWindow::closeEvent(QCloseEvent *event) if (box.clickedButton() == sync) { if (m_sync->start()) { m_syncingForExit = true; + m_syncLog->clear(); + setSyncBusy(true); m_statusLabel->setText(tr("Syncing before quitting...")); event->ignore(); return; @@ -212,6 +215,8 @@ void MainWindow::closeEvent(QCloseEvent *event) } else if (m_config.syncOnExit() == Config::SyncOnExit::Always) { if (m_sync->start()) { m_syncingForExit = true; + m_syncLog->clear(); + setSyncBusy(true); m_statusLabel->setText(tr("Syncing before quitting...")); event->ignore(); return; @@ -330,6 +335,18 @@ void MainWindow::buildUi() m_pendingLabel->hide(); statusBar()->addPermanentWidget(m_pendingLabel); + // Indeterminate: setRange(0, 0). A sync has no measurable progress, since + // mbsync reports no percentage and the script's output is unstructured, so + // a bar filling left to right would be inventing a fraction. This one + // animates to say "working, duration unknown". + m_syncProgress = new QProgressBar(this); + m_syncProgress->setObjectName(QStringLiteral("syncProgress")); + m_syncProgress->setRange(0, 0); + m_syncProgress->setTextVisible(false); + m_syncProgress->setMaximumWidth(120); + m_syncProgress->hide(); + statusBar()->addPermanentWidget(m_syncProgress); + // Query row. auto *queryRow = new QHBoxLayout; m_accountBox = new QComboBox(central); @@ -361,10 +378,35 @@ void MainWindow::buildUi() connect(m_markReadTimer, &QTimer::timeout, this, &MainWindow::markCurrentThreadRead); - m_syncLog = new QPlainTextEdit(central); + // The pane and its close button travel together: a QPlainTextEdit has + // nowhere to put one, and a pane that appears on a failed sync and can + // never be dismissed is worse than one that does not appear at all. + m_syncLogPane = new QWidget(central); + m_syncLogPane->setObjectName(QStringLiteral("syncLogPane")); + auto *syncLogLayout = new QVBoxLayout(m_syncLogPane); + syncLogLayout->setContentsMargins(0, 0, 0, 0); + syncLogLayout->setSpacing(2); + + auto *syncLogHeader = new QHBoxLayout; + syncLogHeader->addWidget(new QLabel(tr("Sync output"), m_syncLogPane)); + syncLogHeader->addStretch(); + + auto *closeSyncLog = new QPushButton(tr("Close"), m_syncLogPane); + closeSyncLog->setObjectName(QStringLiteral("closeSyncLog")); + closeSyncLog->setToolTip(tr("Hide the sync output until the next failure")); + connect(closeSyncLog, &QPushButton::clicked, + m_syncLogPane, &QWidget::hide); + syncLogHeader->addWidget(closeSyncLog); + syncLogLayout->addLayout(syncLogHeader); + + m_syncLog = new QPlainTextEdit(m_syncLogPane); m_syncLog->setReadOnly(true); - m_syncLog->setMaximumHeight(120); - m_syncLog->hide(); + // 200 rather than 120: mbsync's output is wide and repetitive, and the + // shorter pane showed too little of it to read. + m_syncLog->setMaximumHeight(200); + syncLogLayout->addWidget(m_syncLog); + + m_syncLogPane->hide(); m_syncButton = new QPushButton(tr("Sync"), central); m_sync = new MailSync(m_config.syncCommand(), this); @@ -374,8 +416,14 @@ void MainWindow::buildUi() tr("No sync command configured ([sync] command in qtmaildir.conf)")); } connect(m_syncButton, &QPushButton::clicked, this, [this]() { - if (!m_sync->start()) + if (!m_sync->start()) { m_statusLabel->setText(tr("Sync already running")); + return; + } + // Fresh run, fresh output: leaving the previous run's lines in place + // makes a stale failure look like the current one. + m_syncLog->clear(); + setSyncBusy(true); }); connect(m_sync, &MailSync::finished, this, &MainWindow::onSyncFinished); connect(m_sync, &MailSync::outputReceived, this, [this](const QString &chunk) { @@ -451,7 +499,7 @@ void MainWindow::buildUi() m_splitter->setStretchFactor(1, 2); layout->addWidget(m_splitter, 1); - layout->addWidget(m_syncLog); + layout->addWidget(m_syncLogPane); setCentralWidget(central); @@ -1013,6 +1061,8 @@ void MainWindow::onWorkerError(const QString &message) void MainWindow::onSyncFinished(bool success, int exitCode) { + setSyncBusy(false); + if (success) { // Only a SUCCESSFUL sync clears the count. Clearing on failure would // assert the edits had reached the mail store when the sync is exactly @@ -1033,9 +1083,28 @@ void MainWindow::onSyncFinished(bool success, int exitCode) runCurrentQuery(); // A sync is the usual way new tags enter the database. requestAllTags(); + } else if (exitCode == kSyncSkippedExitCode) { + // 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. + m_statusLabel->setText(tr("A sync is already running (started " + "elsewhere); this one was skipped")); + + if (m_syncingForExit) { + // The other run is syncing, but this application cannot see when + // it finishes, so it cannot promise the changes are across. Leave + // the window open and say so rather than quitting on a guess. + m_syncingForExit = false; + QMessageBox::information( + this, tr("Sync already running"), + tr("Another sync was already in progress, so this one was " + "skipped. Your changes are most likely being carried over " + "by that run, but this window cannot see it finish, so it " + "has been left open.")); + } } else { m_statusLabel->setText(tr("Sync failed (exit %1)").arg(exitCode)); - m_syncLog->show(); + m_syncLogPane->show(); if (m_syncingForExit) { // Do NOT quit: the edits are still unsynced and quitting now would @@ -1073,6 +1142,18 @@ void MainWindow::onTagsApplied(const TagChange &change) } } +void MainWindow::setSyncBusy(bool busy) +{ + m_syncProgress->setVisible(busy); + // Disabled rather than left clickable: MailSync::start() already refuses a + // second run, but a button that looks live and does nothing is worse than + // one that shows it is unavailable. + m_syncButton->setEnabled(!busy && m_sync && m_sync->isAvailable()); + + if (busy) + m_statusLabel->setText(tr("Syncing...")); +} + void MainWindow::updatePendingIndicator() { if (m_pendingEdits <= 0) { diff --git a/src/mainwindow.h b/src/mainwindow.h index 7897038..d1f7b1c 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -39,6 +39,7 @@ class QPushButton; class QComboBox; class QPlainTextEdit; class QSplitter; +class QProgressBar; class QTimer; class ThreadListModel; @@ -66,6 +67,15 @@ public: /// cid: references from resolving to another's. static QString cidPrefixForIndex(int index); + /// What the sync command returns when another run already holds the lock. + /// + /// EX_TEMPFAIL from sysexits.h. A skip is not a failure: the other run is + /// doing the work, and with a cron timer every ten minutes a click landing + /// inside one is routine. Reporting it as an error would show a log pane + /// and an alarming status for a situation that needs neither. + /// `assets/mailsync.sh` is the reference implementation of this contract. + static constexpr int kSyncSkippedExitCode = 75; + /// Path of the machine-written UI state file. Deliberately not /// Config::defaultPath(): the config is hand-edited and must never gain a /// base64 geometry blob, nor be rewritten on exit (QSettings does not @@ -134,6 +144,15 @@ private: /// Redraws the unsynced-edits indicator from m_pendingEdits. void updatePendingIndicator(); + /// Shows or hides the "syncing" state: the progress bar and a disabled + /// Sync button. + /// + /// The bar is INDETERMINATE by design. mbsync reports no percentage and + /// the script's output is unstructured, so a bar that filled from left to + /// right would be inventing a fraction nobody knows. An indeterminate one + /// says "working, duration unknown", which is the truth. + void setSyncBusy(bool busy); + /// 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 @@ -185,6 +204,12 @@ private: /// Says how many tag changes have not been seen to reach the mail store. /// Hidden entirely at zero rather than reading "0 unsynced", which is noise. QLabel *m_pendingLabel = nullptr; + + /// Indeterminate, shown only while a sync runs. See setSyncBusy(). + QProgressBar *m_syncProgress = nullptr; + + /// Holds the sync log and its close button, so the pane can be dismissed. + QWidget *m_syncLogPane = nullptr; QPlainTextEdit *m_syncLog = nullptr; /// Action name (as used in [keys]) to the QAction implementing it. Owned |