From ca89402d7e244e2616fdf13eb35c083afeb21826 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 6 Aug 2026 20:06:45 +0200 Subject: fix(ui): one Sync control, and a query bar that looks finished Sync had two controls that behaved differently. The QPushButton beside the query bar cleared the log, opened the pane and disabled itself; the QAction behind the toolbar, menu and shortcut called start() and did nothing else, discarding its return value so a rejected start was silent. Worse, item 29's "disable Sync during a background sync" set the button only, so the toolbar entry stayed clickable through a cron sync and could only produce the script's EX_TEMPFAIL skip. startSync() is now the single handler behind every route in, and the enabled state lives on the QAction, which reaches the toolbar, the menu and the shortcut at once. It also reports when no sync command is configured rather than doing nothing. The QPushButton is gone. It read as a Search button given it sat beside a text field, which is the user's own observation and the reason the toolbar one survives instead. Its unavailable-command tooltip moved to the action, since that is the only thing that says why the control is dead. Removing it left the query field running flush to the window edge, so the saved-query buttons move from their own row onto the query row. The bar is now framed by the account dropdown on the left and the saved queries on the right, the empty row is gone, and the thread list gains the space. The field also gains setClearButtonEnabled, which is Qt's own themed clear icon rather than a hand-rolled button. A "Search" button was considered and rejected: Return already runs the query. No overflow handling for [queries], which is unbounded. Three entries fit; item 23 already specifies buttons-plus-menu and is where that belongs. CLAUDE.md's architecture diagram named four widget classes that have never existed, QueryBar, SavedQueryBar, HeaderWidget and AttachmentBar. The query row and the message header are built inline. Corrected, and the components that do exist but were missing from it added. Tests: the new action test was verified red first and load-bearing by mutation. The old button test is deleted rather than repointed, being an exact duplicate of it, and the unobservable-lock test now drives the action. The clear button and the row layout were confirmed by hand; no test clicks the icon, which is a mouse path. Backlog: 45 done and reclassified as a defect rather than a cosmetic redundancy, 47 added for the bar. --- tests/test_mainwindow.cpp | 43 +++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 24 deletions(-) (limited to 'tests') diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 6a4f903..480be39 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -79,8 +79,8 @@ private slots: void aLocalSyncIsNotReportedAsABackgroundOne(); void aLocalSyncsOwnLockIsNeverReportedAsBackground(); void aSkippedLocalSyncStillReportsTheOtherRunFinishing(); - void theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock(); void anUnobservableLockTableLeavesTheSyncButtonUsable(); + void theSyncActionIsDisabledWhileABackgroundSyncHoldsTheLock(); void escapeBlanksTheMessagePane(); void deleteTogglesOnAnAlreadyDeletedThread(); void deleteOnAMixedSelectionDeletesRatherThanSplittingIt(); @@ -985,11 +985,13 @@ void TestMainWindow::aSkippedLocalSyncStillReportsTheOtherRunFinishing() "says '%1'").arg(status->text()))); } -void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock() +void TestMainWindow::theSyncActionIsDisabledWhileABackgroundSyncHoldsTheLock() { - // Item 27 specified this and it shipped unbuilt: while a cron sync holds - // the lock the button stayed clickable, and pressing it could only produce - // the EX_TEMPFAIL skip. + // Item 29 shipped for the QPushButton only: onExternalSyncStateChanged + // disabled m_syncButton and never touched the QAction, so the toolbar and + // menu Sync stayed clickable during a cron sync and could only produce the + // EX_TEMPFAIL skip. The button-based test passed throughout, because it + // drove the half that worked. QTemporaryDir dir; QVERIFY(dir.isValid()); QVERIFY(QDir().mkpath(dir.filePath(QStringLiteral("qtmaildir")))); @@ -999,10 +1001,6 @@ void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock() s.setValue(QStringLiteral("sync/command"), QStringLiteral("/bin/true")); } - // An empty lock table, so construction observes no sync. Against the real - // /proc/locks this assertion fails whenever the user's cron sync happens to - // be running: cron fires every ten minutes and a run lasts ~35s, so roughly - // 6% of runs landed inside one and the failure looked like flakiness. const QString locks = dir.filePath(QStringLiteral("locks")); { QFile f(locks); @@ -1014,25 +1012,22 @@ void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock() config.load(conf); MainWindow window(config); - auto *button = window.findChild(QStringLiteral("syncButton")); - QVERIFY2(button, "no sync button to check"); - QVERIFY2(button->isEnabled(), "the button starts disabled with a command set"); + auto *action = window.findChild(QStringLiteral("sync")); + QVERIFY2(action, "no sync action to check"); + QVERIFY2(action->isEnabled(), "the action starts disabled with a command set"); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Running)); - QVERIFY2(!button->isEnabled(), - "the sync button stayed enabled during a background sync"); + QVERIFY2(!action->isEnabled(), + "the sync action stayed enabled during a background sync"); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Idle)); - QVERIFY2(button->isEnabled(), - "the sync button was not re-enabled after the background sync"); + QVERIFY2(action->isEnabled(), + "the sync action was not re-enabled after the background sync"); - // The override is process-wide, and QTemporaryDir takes the file with it at - // the end of this scope: leaving it set would point every later window at a - // path that no longer exists. MainWindow::setLocksPathForTesting(QStringLiteral("/proc/locks")); } @@ -1054,19 +1049,19 @@ void TestMainWindow::anUnobservableLockTableLeavesTheSyncButtonUsable() config.load(conf); MainWindow window(config); - auto *button = window.findChild(QStringLiteral("syncButton")); - QVERIFY(button); + auto *action = window.findChild(QStringLiteral("sync")); + QVERIFY(action); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Running)); - QVERIFY(!button->isEnabled()); + QVERIFY(!action->isEnabled()); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Unknown)); - QVERIFY2(button->isEnabled(), - "an unobservable lock table left the sync button disabled"); + QVERIFY2(action->isEnabled(), + "an unobservable lock table left the sync action disabled"); } void TestMainWindow::escapeBlanksTheMessagePane() -- cgit v1.2.3