diff options
| -rw-r--r-- | CHANGELOG.md | 5 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 80 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 61 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 15 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 62 |
5 files changed, 162 insertions, 61 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 6701fae..816f03b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,11 @@ point at which they are stable. naming what it had been showing, so selecting that thread again was read as "already displayed" and never loaded it. This is why an `id:` query copied out of a message's own details dialog produced a card that would not open. +- An automatic sync skipped because another sync was already running gave up + instead of trying again, so an edit the running sync had already passed sat + pending until a manual sync or the next cron run. It now re-arms at the + configured delay. Skipping a concurrent run is unchanged: two mbsync runs + cannot share the lock. ## [0.23.0] - 2026-08-15 diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index d5f58dd..393279e 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -5338,3 +5338,83 @@ how the first version of it was green. It asserts the ids are cleared, because that is the fix's contract, and then that the pane leaves the placeholder, which is what the user sees. Mutation check: reverting the fix fails it with "runQuery blanked the pane but still names a current thread". + +## 89. A sync moves the list under the user's hands, and the auto-sync skips rather than retries + +**Observed (user, notes):** "the auto sync after a delay needs to be reviewed, +its behavior is not exactly right." Then, more broadly: "the sync in general is +worth rethinking, as it is now is not polished and shows too many moving parts. +messages disappearing from views, lists changing while the user is +interacting." And the workaround the user already found: "setting Inbox as the +default view mitigates the problem as messages are not removed from the list +after being read for 2s." + +**Two separate faults sit under one complaint**, and only the first is small. + +**Cause, the concrete half.** `MainWindow::runAutoSync()` +(`src/mainwindow.cpp:3213`) returns without rescheduling when a sync is already +in flight: + +```cpp +if (m_externalSyncBusy || (m_sync && m_sync->isRunning())) + return; +``` + +The comment beside it argues the edits are not lost, because they reached the +mail store at edit time and the running sync is "very likely" to carry them. +Very likely is not always: an edit made after the running mbsync has already +passed that account's mailbox is not carried, the timer has fired and is not +re-armed, and nothing arms it again until the next edit. The pending count then +sits non-zero until a manual sync or the cron job. That is exactly "not exactly +right", and it is a missing `m_autoSyncTimer->start(delay)` on the skip path +rather than a redesign. `scheduleAutoSync()` (`:3184`) already re-checks +everything on the way in, so restarting the timer there is safe by its own +argument. + +**Cause, the larger half.** Nothing to do with the auto-sync: it is what a +refresh does to the list. The refresh after a sync replaces the result set, and +a query like `tag:unread` no longer matches a thread the user has just read, so +rows vanish from under the pointer. Item 35 built the refresh to keep the user's +place, and it does, but keeping the selection is not the same as keeping the +row: a thread that has left the result set has nowhere to be kept. The user's +own mitigation, using an `tag:inbox` view where reading does not change +membership, is the real diagnosis. + +**Approach.** Ship the timer restart on its own, as an XS fix with a test that +arms the timer while a sync is running and asserts it is still active. Then +treat the list-churn half as a design question and put it to the user before +building: the plausible answers (defer a refresh while the pointer is over the +list, keep a read thread visible until the next explicit query, refresh only +rows rather than the result set) differ enough in feel that guessing wastes the +work. + +**Constraints.** + +- The skip itself must stay. Item 71 requires it and mbsync fails on a second + concurrent run; this item restarts the timer, it does not queue a sync. +- A restart must not turn into a spin against a long external sync. The delay is + the debounce interval, and `SyncMonitor` polls `/proc/locks`, so an + `m_externalSyncBusy` that never clears would re-arm indefinitely at that + interval. Cheap, but say so in the test. +- Do not restore the pre-0.16.0 behaviour by making the delay negative for the + user. `auto_sync_delay_ms` is theirs to set. + +**Outcome, 2026-08-15.** Split, and only half was built. + +**The timer half shipped.** `runAutoSync()` now calls `scheduleAutoSync()` on +the skip path instead of returning. The skip itself is unchanged, as item 71 +requires. `scheduleAutoSync()` re-checks the delay, the sync command and the +pending count on the way in, so it cannot arm a sync for nothing, and against a +long external sync it re-arms once per debounce interval, which is a timer and +not a sync. + +**The list-churn half is dropped rather than deferred, at the user's own +reading of it:** "this part I think I solved by using inbox as main view and +checking unread from time to time. It's more of my mental model that was +causing the issue. Unread is supposed to be volatile because once you read a +mail it no longer belongs there." A view defined by a tag stops showing a +thread when the thread loses that tag, and that is the view working. Three +designs were drafted before asking (keep non-matching rows until an explicit +query, defer the refresh while the pointer is over the list, append-only +refresh) and none is worth building against a complaint that resolved itself. +Reopen only if the churn is reported somewhere it is NOT the view doing its job. 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 c376f6e..14a242a 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 @@ -153,7 +153,7 @@ taking that too literally. | 84 | A config problem blocks `test_mainwindow` on a modal nobody can dismiss | testing | S | **done** 2026-08-14, unreleased. `showWarnings()` split: the status label stays in the constructor, `main.cpp` raises the modal after `show()` | | 85 | Nothing on screen can be searched for by right-clicking it | workflow | M | **done** 2026-08-14, unreleased; see `specs/2026-08-14-search-from-message-design.md`. Split from 78; rebuilt the details dialog as rows | | 86 | A right-click search can replace or narrow, but never exclude | workflow | S | **done** 2026-08-14, unreleased; see `specs/2026-08-14-exclude-from-search-design.md`. Follows 85. The `extend` bool became a `SearchMode` enum across four signatures | -| 89 | A sync moves the list under the user's hands, and the auto-sync skips rather than retries | workflow | M | open; from the 2026-08-15 notes pass. Two faults under one complaint, and the larger half is a design question | +| 89 | A sync moves the list under the user's hands, and the auto-sync skips rather than retries | workflow | XS | **done** 2026-08-15, unreleased. The timer half only: a skipped auto-sync re-arms instead of giving up. The list-churn half is **dropped**, not built: the user resolved it as a mental-model question, an Unread view is SUPPOSED to be volatile | | 90 | A saved-query button clears the account selection | workflow | S | **folded into 93** 2026-08-15. Not fixed in place: the button that misbehaves stops being a saved query at all. See `specs/2026-08-15-builtin-filters-design.md` | | 91 | Double-clicking a thread could open it in its own window | workflow | ? | open, unspecified; the user marked it "(?) UX not sure" | | 92 | Nothing distinguishes a tag written by a rule from one the user applied | information | ? | open, unspecified; the user asked it as a question, and the answer decides whether it is a display item or a format change across two repos | @@ -447,65 +447,6 @@ in CLAUDE.md. **Size: S**, down from M now that item 85 has built the menus and item 81 the seeded dialog. -## 89. A sync moves the list under the user's hands, and the auto-sync skips rather than retries - -**Observed (user, notes):** "the auto sync after a delay needs to be reviewed, -its behavior is not exactly right." Then, more broadly: "the sync in general is -worth rethinking, as it is now is not polished and shows too many moving parts. -messages disappearing from views, lists changing while the user is -interacting." And the workaround the user already found: "setting Inbox as the -default view mitigates the problem as messages are not removed from the list -after being read for 2s." - -**Two separate faults sit under one complaint**, and only the first is small. - -**Cause, the concrete half.** `MainWindow::runAutoSync()` -(`src/mainwindow.cpp:3213`) returns without rescheduling when a sync is already -in flight: - -```cpp -if (m_externalSyncBusy || (m_sync && m_sync->isRunning())) - return; -``` - -The comment beside it argues the edits are not lost, because they reached the -mail store at edit time and the running sync is "very likely" to carry them. -Very likely is not always: an edit made after the running mbsync has already -passed that account's mailbox is not carried, the timer has fired and is not -re-armed, and nothing arms it again until the next edit. The pending count then -sits non-zero until a manual sync or the cron job. That is exactly "not exactly -right", and it is a missing `m_autoSyncTimer->start(delay)` on the skip path -rather than a redesign. `scheduleAutoSync()` (`:3184`) already re-checks -everything on the way in, so restarting the timer there is safe by its own -argument. - -**Cause, the larger half.** Nothing to do with the auto-sync: it is what a -refresh does to the list. The refresh after a sync replaces the result set, and -a query like `tag:unread` no longer matches a thread the user has just read, so -rows vanish from under the pointer. Item 35 built the refresh to keep the user's -place, and it does, but keeping the selection is not the same as keeping the -row: a thread that has left the result set has nowhere to be kept. The user's -own mitigation, using an `tag:inbox` view where reading does not change -membership, is the real diagnosis. - -**Approach.** Ship the timer restart on its own, as an XS fix with a test that -arms the timer while a sync is running and asserts it is still active. Then -treat the list-churn half as a design question and put it to the user before -building: the plausible answers (defer a refresh while the pointer is over the -list, keep a read thread visible until the next explicit query, refresh only -rows rather than the result set) differ enough in feel that guessing wastes the -work. - -**Constraints.** - -- The skip itself must stay. Item 71 requires it and mbsync fails on a second - concurrent run; this item restarts the timer, it does not queue a sync. -- A restart must not turn into a spin against a long external sync. The delay is - the debounce interval, and `SyncMonitor` polls `/proc/locks`, so an - `m_externalSyncBusy` that never clears would re-arm indefinitely at that - interval. Cheap, but say so in the test. -- Do not restore the pre-0.16.0 behaviour by making the delay negative for the - user. `auto_sync_delay_ms` is theirs to set. ## 94. `pinned` has nothing left to decide once the buttons are built-in diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 2858514..3c2dca6 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -3441,8 +3441,21 @@ void MainWindow::runAutoSync() // m_externalSyncBusy covers the cron job SyncMonitor can see. A lock taken // between that poll and now is not visible here, and does not need to be: // MailSync::start() fails on a second run and startSync() reports it. - if (m_externalSyncBusy || (m_sync && m_sync->isRunning())) + // + // Re-armed rather than abandoned. Skipping is right; giving up is not. The + // running sync is only VERY LIKELY to carry the edit, since an edit made + // after mbsync has already passed that account's mailbox is not carried by + // it, and before this the timer had fired, nothing re-armed it, and the + // count sat non-zero until a manual sync or the next cron run. + // + // scheduleAutoSync() re-checks the delay, the sync command and the pending + // count on the way in, so this cannot arm a sync for nothing. Against a + // long external sync it re-arms once per debounce interval until the lock + // clears, which is the user's own interval and a timer, not a sync. + if (m_externalSyncBusy || (m_sync && m_sync->isRunning())) { + scheduleAutoSync(); return; + } startSync(); } diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index b96b158..08b2f84 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -201,6 +201,7 @@ private slots: void aSingleMessageIdQuerysCardOpensInTheMessagePane(); void autoSyncIsNotArmedWhenDisabledOrWithNothingPending(); void autoSyncSkipsWhileABackgroundSyncIsRunning(); + void aSkippedAutoSyncRearmsRatherThanGivingUp(); void aSuccessfulSyncRefreshesRatherThanRerunningTheQuery(); void markReadCanBeDisabled(); void pendingEditCountSurvivesAQuery(); @@ -4044,6 +4045,67 @@ void TestMainWindow::autoSyncSkipsWhileABackgroundSyncIsRunning() // table, and handing the real one back would re-expose the next test. } +void TestMainWindow::aSkippedAutoSyncRearmsRatherThanGivingUp() +{ + // Item 89, the concrete half. Skipping is correct and must stay, but the + // skip used to be the END of the attempt: the timer had fired, nothing + // re-armed it, and the edit waited for a manual sync or the next cron run. + // + // The comment defending it said the running sync was "very likely" to carry + // the edit, since it reached the mail store at edit time. Very likely is not + // always: an edit made after mbsync has already passed that account's + // mailbox is not carried by it, and the pending count then sits non-zero + // with nothing scheduled to clear it. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const QString locks = dir.filePath(QStringLiteral("locks")); + { + QFile f(locks); + QVERIFY(f.open(QIODevice::WriteOnly)); + } + MainWindow::setLocksPathForTesting(locks); + + Config config; + config.load(writeSyncConfig(dir)); + + MainWindow window(config); + auto *timer = window.findChild<QTimer *>(QStringLiteral("autoSyncTimer")); + QVERIFY(timer); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + + TagChange change; + change.messageIds = { QStringLiteral("m1") }; + change.added = { QStringLiteral("flagged") }; + QVERIFY(QMetaObject::invokeMethod(&window, "onTagsApplied", + Q_ARG(TagChange, change))); + QVERIFY2(timer->isActive(), "the edit did not arm the debounce at all"); + + // Fire it by hand rather than waiting out the delay. A QTimer that has + // fired is no longer active, so this is also what makes the assertion + // below mean something: without the re-arm it is inactive here. + timer->stop(); + QVERIFY(QMetaObject::invokeMethod(&window, "runAutoSync")); + + QVERIFY2(timer->isActive(), + "a skipped automatic sync left nothing armed to carry the edit"); + + // Re-armed at the configured debounce, not at some shorter interval that + // would spin against a long external sync. SyncMonitor polls /proc/locks, + // so an m_externalSyncBusy that never clears re-arms at this interval + // indefinitely, which is cheap only because the interval is the user's own. + QCOMPARE(timer->interval(), config.autoSyncDelayMs()); + + // The edit is still pending throughout: a retry must not look like a + // completed sync to the indicator. + auto *label = window.findChild<QLabel *>(QStringLiteral("pendingEdits")); + QVERIFY(label); + QVERIFY2(!label->isHidden(), + "the re-armed sync cleared the pending indicator"); +} + void TestMainWindow::aSuccessfulSyncRefreshesRatherThanRerunningTheQuery() { // Reported by hand against item 71: reading a message in the Unread view, |