From 4825e7a18b745dcee8a560308e99a761d3c85559 Mon Sep 17 00:00:00 2001 From: "Jeremy [KK7GWY]" Date: Sat, 24 Oct 2026 10:19:32 -0700 Subject: [PATCH 1/5] feat(ctr2): the CTR2 window's questions in the canon style. Principle VIII. The CTR2 window is a CanonWindow, but its questions and notices were plain QMessageBoxes: an OS icon and native buttons over the canon ground. CanonMessageBox is the canon version: a CanonWindow dialog with the title as a canon header, the text in canon ink, optional further text in the softer ink, and canon buttons at the bottom right. The default button is outlined in cyan; a destructive one (Restore) is drawn in gold. clickedButton() names the button clicked and stays null when the box is closed any other way (corner button, Escape, Ctrl+W), so callers keep their "clicked != go" checks. All eleven move to it: the five firmware questions (install, install over the cable, update the stock firmware, restore a backup, download mode) and the six Linux USB-access notices in the relay panel. Restore now opens with Cancel as the default, so Return never overwrites the device. The file picker stays the platform's own. Tests: canon_message_box_test covers a click on each role, Escape, the close button, Return on the default (which opens with the focus), the further text and the destructive marker. Not recording the click fails four checks; not focusing the default fails three. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WvUonxmfcg6yopRJnXx9VG --- CMakeLists.txt | 1 + docs/style/dialog-patterns.md | 7 ++ src/gui/CanonMessageBox.cpp | 108 +++++++++++++++++++++++ src/gui/CanonMessageBox.h | 51 +++++++++++ src/gui/Ctr2ProxyPanel.cpp | 53 +++++------ src/gui/FirmwareInstallDialog.cpp | 56 ++++++------ tests/canon_message_box_test.cpp | 141 ++++++++++++++++++++++++++++++ tests/tests.cmake | 22 +++++ 8 files changed, 387 insertions(+), 52 deletions(-) create mode 100644 src/gui/CanonMessageBox.cpp create mode 100644 src/gui/CanonMessageBox.h create mode 100644 tests/canon_message_box_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index cbf04dd8c1..25de277c2c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1425,6 +1425,7 @@ set(GUI_SOURCES src/gui/HealthApplet.cpp src/gui/PersistentDialog.cpp src/gui/CanonWindow.cpp + src/gui/CanonMessageBox.cpp src/gui/CanonSurface.cpp src/gui/PanadapterHeader.cpp src/gui/FramelessMessageBox.cpp diff --git a/docs/style/dialog-patterns.md b/docs/style/dialog-patterns.md index 01920a8980..50f5d25cac 100644 --- a/docs/style/dialog-patterns.md +++ b/docs/style/dialog-patterns.md @@ -269,6 +269,13 @@ What it keeps: is set. They animate only while visible and tick slowly while the window is not exposed. +A question or notice from a canon window uses `CanonMessageBox` +(`src/gui/CanonMessageBox.{h,cpp}`) in place of `QMessageBox`: a canon dialog +with the title as a canon header, the text, optional further text, and the +buttons at the bottom right. `clickedButton()` names the button clicked and is +null when the box is closed any other way; the default button is outlined in +cyan and a destructive one is drawn in gold. The CTR2 window's questions use it. + It is translucent, so the rounded corners need a compositor; an X11 session without one shows square black corners. Use `CanonWindow` only for windows the style guide covers: short-lived windows, and the tool windows the maintainer diff --git a/src/gui/CanonMessageBox.cpp b/src/gui/CanonMessageBox.cpp new file mode 100644 index 0000000000..1e6114544b --- /dev/null +++ b/src/gui/CanonMessageBox.cpp @@ -0,0 +1,108 @@ +#include "CanonMessageBox.h" + +#include "CanonIndicators.h" +#include "ModemChrome.h" + +#include +#include +#include +#include + +namespace AetherSDR { + +namespace { + +constexpr int kTextWidth = 420; + +} // namespace + +CanonMessageBox::CanonMessageBox(const QString& title, const QString& text, QWidget* parent) + : CanonWindow(title, parent) +{ + setObjectName(QStringLiteral("canonMessageBox")); + setModal(true); + applyCanonSheet(bodyWidget(), [] { + return ModemChrome::styleSheet(ModemChrome::Scale::Dialog, ModemChrome::Look::Canon) + + QStringLiteral( + "QLabel#canonMessageText { color: {{color.canon.ink}}; }" + "QLabel#canonMessageInfo { color: {{color.canon.inkSoft}}; }" + "QPushButton { padding: 6px 18px; }" + "QPushButton:default { color: {{color.canon.cyan}}; border-color: {{color.canon.cyan}}; }" + "QPushButton[role=\"destructive\"] { color: {{color.canon.sparkGold}}; }"); + }); + + auto* root = new QVBoxLayout(bodyWidget()); + root->setContentsMargins(18, 14, 18, 16); + root->setSpacing(12); + root->addWidget(makeCanonHeader(title)); + + m_text = new QLabel(text); + m_text->setObjectName(QStringLiteral("canonMessageText")); + m_text->setWordWrap(true); + m_text->setFixedWidth(kTextWidth); + root->addWidget(m_text); + + m_info = new QLabel; + m_info->setObjectName(QStringLiteral("canonMessageInfo")); + m_info->setWordWrap(true); + m_info->setFixedWidth(kTextWidth); + m_info->hide(); + root->addWidget(m_info); + + root->addSpacing(4); + m_buttons = new QHBoxLayout; + m_buttons->setSpacing(8); + m_buttons->addStretch(1); + root->addLayout(m_buttons); +} + +void CanonMessageBox::setInformativeText(const QString& text) +{ + m_info->setText(text); + m_info->setVisible(!text.isEmpty()); +} + +void CanonMessageBox::setTextSelectable(bool on) +{ + const Qt::TextInteractionFlags flags = on ? Qt::TextSelectableByMouse : Qt::NoTextInteraction; + m_text->setTextInteractionFlags(flags); + m_info->setTextInteractionFlags(flags); +} + +QPushButton* CanonMessageBox::addButton(const QString& text, Role role) +{ + auto* button = new QPushButton(text); + button->setAutoDefault(false); + if (role == Role::Destructive) { + button->setProperty("role", QStringLiteral("destructive")); + } + connect(button, &QPushButton::clicked, this, [this, button, role] { + m_clicked = button; + if (role == Role::Reject) { + reject(); + } else { + accept(); + } + }); + m_buttons->addWidget(button); + m_added.append(button); + return button; +} + +void CanonMessageBox::setDefaultButton(QPushButton* button) +{ + for (QPushButton* b : std::as_const(m_added)) { + b->setDefault(b == button); + } + m_default = button; +} + +void CanonMessageBox::showEvent(QShowEvent* event) +{ + CanonWindow::showEvent(event); + if (m_default) { + m_default->setFocus(); + } +} + +} // namespace AetherSDR diff --git a/src/gui/CanonMessageBox.h b/src/gui/CanonMessageBox.h new file mode 100644 index 0000000000..6e4db796b9 --- /dev/null +++ b/src/gui/CanonMessageBox.h @@ -0,0 +1,51 @@ +#pragma once + +#include "CanonWindow.h" + +#include + +class QLabel; +class QHBoxLayout; +class QPushButton; + +namespace AetherSDR { + +// A question or a notice in the style guide's window, for windows built on +// CanonWindow in place of QMessageBox: the title as a canon header, the text in +// canon ink, optional further text below it in the softer ink, and the buttons +// in a row at the bottom right. Run it with exec() and read clickedButton(), +// which stays null when the operator closes it instead (the corner button, +// Escape, ⌘W / Ctrl+W). +class CanonMessageBox : public CanonWindow { + Q_OBJECT + +public: + // Accept and Reject buttons close it as accepted or rejected; Destructive + // accepts too, and is drawn in the warning gold, for a step that replaces + // what is on the device. + enum class Role { Accept, Reject, Destructive }; + + CanonMessageBox(const QString& title, const QString& text, QWidget* parent = nullptr); + + void setInformativeText(const QString& text); + // Lets the operator select and copy the texts (commands to paste). + void setTextSelectable(bool on); + + QPushButton* addButton(const QString& text, Role role); + // The button Return clicks; also takes the focus when the box opens. + void setDefaultButton(QPushButton* button); + QPushButton* clickedButton() const { return m_clicked; } + +protected: + void showEvent(QShowEvent* event) override; + +private: + QLabel* m_text{nullptr}; + QLabel* m_info{nullptr}; + QHBoxLayout* m_buttons{nullptr}; + QList m_added; + QPushButton* m_default{nullptr}; + QPushButton* m_clicked{nullptr}; +}; + +} // namespace AetherSDR diff --git a/src/gui/Ctr2ProxyPanel.cpp b/src/gui/Ctr2ProxyPanel.cpp index 3cb9700d0b..aa2e20bb1d 100644 --- a/src/gui/Ctr2ProxyPanel.cpp +++ b/src/gui/Ctr2ProxyPanel.cpp @@ -1,5 +1,6 @@ #include "Ctr2ProxyPanel.h" +#include "CanonMessageBox.h" #include "GuardedSlider.h" #include "ScopedChildWidget.h" #include "models/Ctr2ProxyModel.h" @@ -12,7 +13,6 @@ #include #include #include -#include #include #include #include @@ -294,15 +294,15 @@ bool ruleAlreadyInstalled(const QString& rule) void Ctr2ProxyPanel::offerUsbAccessRule() { if (ruleAlreadyInstalled(loadRule())) { - ScopedChildWidget box( - QMessageBox::Information, tr("CTR2 USB access"), + ScopedChildWidget box( + tr("CTR2 USB access"), tr("The CTR2 access rule is already installed, but this login still cannot " - "open the CTR2."), - QMessageBox::Ok, this); + "open the CTR2."), this); box.get()->setInformativeText( tr("Unplug the CTR2, plug it back in, then press Start. The rule only opens " "the CTR2 to whoever is logged in at this computer's own screen, so a " "remote or background session cannot use it.")); + box.get()->setDefaultButton(box.get()->addButton(tr("OK"), CanonMessageBox::Role::Accept)); box.get()->exec(); return; } @@ -311,18 +311,18 @@ void Ctr2ProxyPanel::offerUsbAccessRule() return; } - ScopedChildWidget box( - QMessageBox::Question, tr("Allow AetherSDR to use the CTR2?"), + ScopedChildWidget box( + tr("Allow AetherSDR to use the CTR2?"), tr("Linux only lets the administrator open USB devices such as the CTR2, so " "AetherSDR cannot talk to it yet."), - QMessageBox::NoButton, this); + this); box.get()->setInformativeText( tr("AetherSDR can install a small system rule that lets whoever is logged in " "at this computer open CTR2 controllers, and only CTR2 controllers. You " "will be asked for your administrator password once; after that the CTR2 " "works every time.\n\nThe rule is written to %1.").arg(kRuleTarget)); - QPushButton* install = box.get()->addButton(tr("Install rule"), QMessageBox::AcceptRole); - box.get()->addButton(tr("Not now"), QMessageBox::RejectRole); + box.get()->addButton(tr("Not now"), CanonMessageBox::Role::Reject); + QPushButton* install = box.get()->addButton(tr("Install rule"), CanonMessageBox::Role::Accept); box.get()->setDefaultButton(install); box.get()->exec(); if (box && box.get()->clickedButton() == install) { @@ -342,28 +342,28 @@ void Ctr2ProxyPanel::showManualRuleInstructions(const QString& why) out.close(); if (!wrote) { qCWarning(lcDevices) << "CTR2: could not save the udev rule to" << saved; - ScopedChildWidget box( - QMessageBox::Warning, tr("CTR2 USB access"), + ScopedChildWidget box( + tr("CTR2 USB access"), tr("Linux only lets the administrator open USB devices such as the CTR2. " "AetherSDR could not ask for permission (%1), and could not save the " - "access rule to %2 for you to install by hand.").arg(why, saved), - QMessageBox::Ok, this); + "access rule to %2 for you to install by hand.").arg(why, saved), this); + box.get()->setDefaultButton(box.get()->addButton(tr("OK"), CanonMessageBox::Role::Accept)); box.get()->exec(); return; } - ScopedChildWidget box( - QMessageBox::Warning, tr("CTR2 USB access"), + ScopedChildWidget box( + tr("CTR2 USB access"), tr("Linux only lets the administrator open USB devices such as the CTR2, " "and AetherSDR could not ask for permission: %1.\n\n" "To allow it, run these commands in a terminal, then unplug and " - "replug the CTR2:").arg(why), - QMessageBox::Ok, this); + "replug the CTR2:").arg(why), this); box.get()->setInformativeText( QStringLiteral("sudo cp '%1' '%2'\n" "sudo udevadm control --reload-rules\n" "sudo udevadm trigger --subsystem-match=hidraw") .arg(saved, kRuleTarget)); - box.get()->setTextInteractionFlags(Qt::TextSelectableByMouse); + box.get()->setTextSelectable(true); + box.get()->setDefaultButton(box.get()->addButton(tr("OK"), CanonMessageBox::Role::Accept)); box.get()->exec(); } @@ -375,12 +375,13 @@ void Ctr2ProxyPanel::installUsbAccessRule() auto fail = [this](const QString& detail) { m_installingRule = false; syncConfiguration(); - ScopedChildWidget box( - QMessageBox::Warning, tr("CTR2 USB access"), - tr("The CTR2 access rule could not be installed."), QMessageBox::Ok, this); + ScopedChildWidget box( + tr("CTR2 USB access"), + tr("The CTR2 access rule could not be installed."), this); if (!detail.isEmpty()) { box.get()->setInformativeText(detail); } + box.get()->setDefaultButton(box.get()->addButton(tr("OK"), CanonMessageBox::Role::Accept)); box.get()->exec(); }; @@ -458,11 +459,11 @@ void Ctr2ProxyPanel::startAfterRuleInstalled(int attemptsLeft) return; // the Start was for USB; never start another transport from here } if (m_model->usbDeviceNeedsAccessRule()) { - ScopedChildWidget box( - QMessageBox::Information, tr("CTR2 USB access"), + ScopedChildWidget box( + tr("CTR2 USB access"), tr("The access rule is installed. Unplug the CTR2, plug it back in, " - "then press Start."), - QMessageBox::Ok, this); + "then press Start."), this); + box.get()->setDefaultButton(box.get()->addButton(tr("OK"), CanonMessageBox::Role::Accept)); box.get()->exec(); return; } diff --git a/src/gui/FirmwareInstallDialog.cpp b/src/gui/FirmwareInstallDialog.cpp index 42f6abac3e..5111765823 100644 --- a/src/gui/FirmwareInstallDialog.cpp +++ b/src/gui/FirmwareInstallDialog.cpp @@ -1,6 +1,7 @@ #include "FirmwareInstallDialog.h" #include "CanonIndicators.h" +#include "CanonMessageBox.h" #include "ComboStyle.h" #include "Ctr2ProxyPanel.h" #include "ModemChrome.h" @@ -19,7 +20,6 @@ #include #include #include -#include #include #include #include @@ -686,29 +686,31 @@ bool FirmwareInstallDialog::askDownloadMode(bool buttons) if (!buttons) { return true; } - ScopedChildWidget ask(QMessageBox::Information, - tr("Put the CTR2-Max in download mode"), - tr("Put the CTR2-Max in download mode now."), - QMessageBox::Ok | QMessageBox::Cancel, this); + ScopedChildWidget ask(tr("Put the CTR2-Max in download mode"), + tr("Put the CTR2-Max in download mode now."), this); ask.get()->setInformativeText( tr("Hold BOOT (through the hole in the back), press and release RESET, then release " "BOOT. Its screen goes dark; that is normal. AetherSDR carries on by itself once it " "finds it there.")); - ask.get()->setDefaultButton(QMessageBox::Ok); - return ask.get()->exec() == QMessageBox::Ok; + ask.get()->addButton(tr("Cancel"), CanonMessageBox::Role::Reject); + QPushButton* ok = ask.get()->addButton(tr("OK"), CanonMessageBox::Role::Accept); + ask.get()->setDefaultButton(ok); + ask.get()->exec(); + return ask && ask.get()->clickedButton() == ok; } void FirmwareInstallDialog::confirmRestore(const QString& path) { FirmwareInstallModel::Target target = m_fw->target(); - ScopedChildWidget ask(QMessageBox::Warning, tr("Restore the backup?"), - backupText(path), QMessageBox::Cancel, this); + ScopedChildWidget ask(tr("Restore the backup?"), backupText(path), this); ask.get()->setInformativeText( tr("A backup goes back only onto the CTR2-Max it came from, and only if the file is " "unchanged since it was made.")); - QPushButton* go = ask.get()->addButton(tr("Restore"), QMessageBox::DestructiveRole); + // Return keeps what is on the device; restoring takes a click. + ask.get()->setDefaultButton(ask.get()->addButton(tr("Cancel"), CanonMessageBox::Role::Reject)); + QPushButton* go = ask.get()->addButton(tr("Restore"), CanonMessageBox::Role::Destructive); ask.get()->exec(); - if (ask.get()->clickedButton() != go || !askDownloadMode(needsButtons(*m_fw, target))) + if (!ask || ask.get()->clickedButton() != go || !askDownloadMode(needsButtons(*m_fw, target))) return; target = afterAlert(*m_fw, target); const QString done = tr("The backup is restored and the CTR2-Max is restarting."); @@ -737,18 +739,19 @@ void FirmwareInstallDialog::onInstall() const QString product = m_fw->releaseProduct(); const QString version = m_fw->releaseVersion(); const QString folder = QDir::toNativeSeparators(FirmwareInstallModel::backupFolder()); - ScopedChildWidget ask(QMessageBox::Question, tr("Install %1?").arg(product), - tr("Install %1 %2 on this CTR2-Max?").arg(product, version), - QMessageBox::Cancel, this); + ScopedChildWidget ask(tr("Install %1?").arg(product), + tr("Install %1 %2 on this CTR2-Max?").arg(product, version), + this); ask.get()->setInformativeText( tr("Its whole flash is backed up first, to %1; that takes about a minute. The install " "writes only what the firmware's manifest lists: the stock firmware, its settings and " "its registration are kept.") .arg(folder)); - QPushButton* go = ask.get()->addButton(tr("Install"), QMessageBox::AcceptRole); + ask.get()->addButton(tr("Cancel"), CanonMessageBox::Role::Reject); + QPushButton* go = ask.get()->addButton(tr("Install"), CanonMessageBox::Role::Accept); ask.get()->setDefaultButton(go); ask.get()->exec(); - if (ask.get()->clickedButton() != go || !askDownloadMode(needsButtons(*m_fw, target))) + if (!ask || ask.get()->clickedButton() != go || !askDownloadMode(needsButtons(*m_fw, target))) return; target = afterAlert(*m_fw, target); const QString done = @@ -770,18 +773,19 @@ void FirmwareInstallDialog::installOverLink() } const QString product = m_fw->releaseProduct(); const QString version = m_fw->releaseVersion(); - ScopedChildWidget ask( - QMessageBox::Question, tr("Install %1?").arg(product), + ScopedChildWidget ask( + tr("Install %1?").arg(product), tr("Install %1 %2 over the USB cable? The device runs %3 now.") .arg(product, version, m_fw->linkVersion()), - QMessageBox::Cancel, this); + this); ask.get()->setInformativeText( tr("It takes about three minutes. The device checks the firmware's signature, and keeps " "the firmware it has if anything goes wrong.")); - QPushButton* go = ask.get()->addButton(tr("Install"), QMessageBox::AcceptRole); + ask.get()->addButton(tr("Cancel"), CanonMessageBox::Role::Reject); + QPushButton* go = ask.get()->addButton(tr("Install"), CanonMessageBox::Role::Accept); ask.get()->setDefaultButton(go); ask.get()->exec(); - if (ask.get()->clickedButton() != go) + if (!ask || ask.get()->clickedButton() != go) return; const QString done = tr("%1 %2 is installed and running.").arg(product, version); m_step->setText(tr("Starting the update…")); @@ -830,9 +834,8 @@ void FirmwareInstallDialog::installStockFirmware() } const QString name = m_fw->stockFirmwareName(); const QString folder = QDir::toNativeSeparators(FirmwareInstallModel::backupFolder()); - ScopedChildWidget ask(QMessageBox::Question, tr("Update the CTR2-Max?"), - tr("Install %1 on this CTR2-Max?").arg(name), - QMessageBox::Cancel, this); + ScopedChildWidget ask(tr("Update the CTR2-Max?"), + tr("Install %1 on this CTR2-Max?").arg(name), this); QString info = tr("Only the firmware itself is written: the CTR2-Max keeps its settings, its " "registration and AetherCTR2. Its whole flash is backed up first, to %1; " "that takes about a minute.") @@ -842,10 +845,11 @@ void FirmwareInstallDialog::installStockFirmware() + tr("AetherCTR2 restarts into download mode for the update; the relay stops " "while it runs."); ask.get()->setInformativeText(info); - QPushButton* go = ask.get()->addButton(tr("Update"), QMessageBox::AcceptRole); + ask.get()->addButton(tr("Cancel"), CanonMessageBox::Role::Reject); + QPushButton* go = ask.get()->addButton(tr("Update"), CanonMessageBox::Role::Accept); ask.get()->setDefaultButton(go); ask.get()->exec(); - if (ask.get()->clickedButton() != go || !askDownloadMode(needsButtons(*m_fw, target))) + if (!ask || ask.get()->clickedButton() != go || !askDownloadMode(needsButtons(*m_fw, target))) return; target = afterAlert(*m_fw, target); const QString done = diff --git a/tests/canon_message_box_test.cpp b/tests/canon_message_box_test.cpp new file mode 100644 index 0000000000..b4ee72322b --- /dev/null +++ b/tests/canon_message_box_test.cpp @@ -0,0 +1,141 @@ +// CanonMessageBox (RFC #6226): the canon window's question box. The caller +// acts on clickedButton(), so it must name the button the operator clicked +// (or Return clicked for them), and be null when the box is closed instead. + +#include "TestSettingsProfile.h" +#include "core/AppSettings.h" +#include "core/ThemeManager.h" +#include "gui/CanonMessageBox.h" + +#include +#include +#include +#include +#include +#include +#include + +using namespace AetherSDR; + +static int g_failures = 0; + +#define EXPECT_TRUE(cond) do { \ + if (!(cond)) { \ + std::fprintf(stderr, "FAIL %s:%d expected (%s) to be true\n", \ + __FILE__, __LINE__, #cond); \ + ++g_failures; \ + } \ +} while (0) + +namespace { + +struct Box { + CanonMessageBox box{QStringLiteral("Install AetherCTR2?"), + QStringLiteral("Install AetherCTR2 0.9.6 over the USB cable?")}; + QPushButton* cancel{box.addButton(QStringLiteral("Cancel"), CanonMessageBox::Role::Reject)}; + QPushButton* install{box.addButton(QStringLiteral("Install"), CanonMessageBox::Role::Accept)}; + + Box() { box.setDefaultButton(install); } + + // Runs the box modally, doing `act` once it is showing. + int run(std::function act) + { + QTimer::singleShot(0, &box, [this, act] { + QTest::qWaitForWindowExposed(&box); + act(box); + }); + return box.exec(); + } +}; + +void pressKey(CanonMessageBox& box, Qt::Key key) +{ + QWidget* target = QApplication::focusWidget() ? QApplication::focusWidget() : &box; + QTest::keyClick(target, key); +} + +} // namespace + +int main(int argc, char** argv) +{ + TestSettingsProfile settingsProfile(QStringLiteral("aether-canon-message-box-test")); + if (!settingsProfile.isValid()) { + std::fprintf(stderr, "FAIL could not create isolated settings profile\n"); + return 1; + } + if (qEnvironmentVariableIsEmpty("QT_QPA_PLATFORM")) { + qputenv("QT_QPA_PLATFORM", "offscreen"); + } + QApplication app(argc, argv); + AppSettings::instance().load(); + ThemeManager::instance(); + + // ---- a click on an accepting button names it ---- + { + Box b; + const int result = b.run([&b](CanonMessageBox&) { b.install->click(); }); + EXPECT_TRUE(result == QDialog::Accepted); + EXPECT_TRUE(b.box.clickedButton() == b.install); + } + + // ---- a click on a rejecting button names it, and rejects ---- + { + Box b; + const int result = b.run([&b](CanonMessageBox&) { b.cancel->click(); }); + EXPECT_TRUE(result == QDialog::Rejected); + EXPECT_TRUE(b.box.clickedButton() == b.cancel); + } + + // ---- Escape closes it with no button clicked ---- + { + Box b; + const int result = b.run([](CanonMessageBox& box) { pressKey(box, Qt::Key_Escape); }); + EXPECT_TRUE(result == QDialog::Rejected); + EXPECT_TRUE(b.box.clickedButton() == nullptr); + } + + // ---- the corner close button closes it with no button clicked ---- + { + Box b; + const int result = b.run([](CanonMessageBox& box) { box.close(); }); + EXPECT_TRUE(result == QDialog::Rejected); + EXPECT_TRUE(b.box.clickedButton() == nullptr); + } + + // ---- Return clicks the default button, which opens with the focus ---- + { + Box b; + bool focused = false; + const int result = b.run([&b, &focused](CanonMessageBox& box) { + focused = b.install->hasFocus(); + pressKey(box, Qt::Key_Return); + }); + EXPECT_TRUE(focused); + EXPECT_TRUE(result == QDialog::Accepted); + EXPECT_TRUE(b.box.clickedButton() == b.install); + EXPECT_TRUE(b.install->isDefault() && !b.cancel->isDefault()); + } + + // ---- the further text shows only when there is some; a destructive + // button accepts and is marked for the warning color ---- + { + CanonMessageBox box(QStringLiteral("Restore the backup?"), QStringLiteral("Restore it?")); + auto* info = box.findChild(QStringLiteral("canonMessageInfo")); + EXPECT_TRUE(info && info->isHidden()); + box.setInformativeText(QStringLiteral("Only onto the CTR2-Max it came from.")); + EXPECT_TRUE(info && !info->isHidden()); + QPushButton* restore = + box.addButton(QStringLiteral("Restore"), CanonMessageBox::Role::Destructive); + EXPECT_TRUE(restore->property("role").toString() == QStringLiteral("destructive")); + QTimer::singleShot(0, &box, [restore] { restore->click(); }); + EXPECT_TRUE(box.exec() == QDialog::Accepted); + EXPECT_TRUE(box.clickedButton() == restore); + } + + if (g_failures == 0) { + std::printf("canon_message_box_test: all checks passed\n"); + return 0; + } + std::fprintf(stderr, "canon_message_box_test: %d failure(s)\n", g_failures); + return 1; +} diff --git a/tests/tests.cmake b/tests/tests.cmake index 6741f9d36c..843a80ff10 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -2344,6 +2344,27 @@ add_test(NAME canon_window_test COMMAND canon_window_test) set_tests_properties(canon_window_test PROPERTIES ENVIRONMENT "QT_QPA_PLATFORM=offscreen") +# CanonMessageBox (RFC #6226): the canon question box reports the button +# clicked, and none when it is closed instead. +add_executable(canon_message_box_test + tests/canon_message_box_test.cpp + src/gui/CanonMessageBox.cpp + src/gui/CanonWindow.cpp + src/gui/ModemChrome.cpp + src/core/ThemeManager.cpp + src/core/ThemeSeedGenerated.cpp + $ + $ + $ + ${THEME_TEST_RESOURCES} +) +target_include_directories(canon_message_box_test PRIVATE src) +target_link_libraries(canon_message_box_test PRIVATE Qt6::Core Qt6::Gui Qt6::Widgets Qt6::Test) +set_target_properties(canon_message_box_test PROPERTIES AUTOMOC ON) +add_test(NAME canon_message_box_test COMMAND canon_message_box_test) +set_tests_properties(canon_message_box_test PROPERTIES + ENVIRONMENT "QT_QPA_PLATFORM=offscreen") + # CanonApplet, CanonPan and CanonCanvas (RFC #6226): the canon ground with # square corners inside the main window; the close button only requests. add_executable(canon_surface_test @@ -9823,6 +9844,7 @@ set(AETHER_SETTINGS_CONSUMERS aether_rx_profiles_test theme_manager_test canon_window_test + canon_message_box_test canon_surface_test kpa500_applet_test kxpa100_applet_test From 7ef5ba430d70f56e263de590dd3e1c83648d4a4e Mon Sep 17 00:00:00 2001 From: "Jeremy [KK7GWY]" Date: Fri, 23 Oct 2026 23:48:50 -0700 Subject: [PATCH 2/5] fix(ctr2): a device that leaves the bus for a restart it was asked for has restarted. Principle XI. Restart into (SWITCH) and download mode (BOOTLOADER) waited for the device's STATUS, and a port loss before it arrived read as "Lost the device ... The device keeps the firmware it had". Lynovation's CTR2-Max v1.02.00 betas answer SWITCH with the op as a character ('5', 0x35), which AetherSDR ignores, then leave the bus two seconds later, so a switch that worked was reported as a failure. A port loss while the answer is outstanding now finishes the request as the OK answer does: "The device is restarting into its other firmware" (or download mode). An answer that refuses the request still fails it. Tests: a simulated device that names the wrong op, and one that leaves for download mode without answering, both finish as restarts; with the new case removed both checks fail. The design doc states the rule. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WvUonxmfcg6yopRJnXx9VG --- docs/ctr2-usb-relay-design.md | 5 +++++ src/core/Ctr2FirmwareUpdater.cpp | 24 ++++++++++++++++++------ src/core/Ctr2FirmwareUpdater.h | 2 ++ tests/ctr2_firmware_updater_test.cpp | 20 +++++++++++++++++++- 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/docs/ctr2-usb-relay-design.md b/docs/ctr2-usb-relay-design.md index c1aa8ebe18..c4ad0c56c4 100644 --- a/docs/ctr2-usb-relay-design.md +++ b/docs/ctr2-usb-relay-design.md @@ -415,6 +415,11 @@ maker's firmware and its settings survive every update. SWITCH asks the device to restart into that firmware; how a device selects its image is its own business. +A device answers SWITCH and BOOTLOADER with STATUS before it leaves the bus. +AetherSDR also counts the request done if the device leaves while the answer +is outstanding, so a restart that beats its answer, or an answer naming the +wrong op, is not reported as a lost device. + **Host implementation.** `Ctr2FirmwareUpdater` runs the session and checks the image before sending anything: an ESP32-S3 application image with an application description (`readImage`), not a merged flash image or a diff --git a/src/core/Ctr2FirmwareUpdater.cpp b/src/core/Ctr2FirmwareUpdater.cpp index 83ccab3e36..ced5cf9c5a 100644 --- a/src/core/Ctr2FirmwareUpdater.cpp +++ b/src/core/Ctr2FirmwareUpdater.cpp @@ -358,13 +358,8 @@ void Ctr2FirmwareUpdater::onStatus(const firmware::Reply& reply) : tr("The device has no other firmware to start.")); return; } - m_timeout->stop(); closePort(); // it is leaving the bus - m_phase = Phase::Idle; - m_restart.reset(); - emit finished(true, loader ? tr("The device is restarting into download mode.") - : tr("The device is restarting into its other firmware."), - QString()); + restarted(); return; } if (reply.status != firmware::Status::Ok) { @@ -462,12 +457,29 @@ void Ctr2FirmwareUpdater::onPortFailed(const QString& message) fail(tr("The device left during the final check (%1). Reconnect it: it runs " "either the new firmware or the old one.").arg(message)); break; + case Phase::Commanding: + // Leaving the bus is what the request asks for. A device that does it + // before its STATUS arrives, or whose STATUS names another op, has + // still restarted. + restarted(); + break; default: fail(tr("Lost the device: %1.").arg(message) + kKeepsFirmware); break; } } +void Ctr2FirmwareUpdater::restarted() +{ + const bool loader = m_restart && *m_restart == Restart::Bootloader; + m_timeout->stop(); + m_phase = Phase::Idle; + m_restart.reset(); + emit finished(true, loader ? tr("The device is restarting into download mode.") + : tr("The device is restarting into its other firmware."), + QString()); +} + void Ctr2FirmwareUpdater::onTimeout() { switch (m_phase) { diff --git a/src/core/Ctr2FirmwareUpdater.h b/src/core/Ctr2FirmwareUpdater.h index a1b90c5593..0351b15f94 100644 --- a/src/core/Ctr2FirmwareUpdater.h +++ b/src/core/Ctr2FirmwareUpdater.h @@ -110,6 +110,8 @@ class Ctr2FirmwareUpdater : public QObject { void lookForDevice(); void fail(const QString& message); void succeed(const QString& version); + // A restart request done: the device is leaving the bus for its target. + void restarted(); void armTimeout(int ms); DeviceSource m_devices; diff --git a/tests/ctr2_firmware_updater_test.cpp b/tests/ctr2_firmware_updater_test.cpp index c3ec87c070..b49e7b9cb4 100644 --- a/tests/ctr2_firmware_updater_test.cpp +++ b/tests/ctr2_firmware_updater_test.cpp @@ -73,6 +73,8 @@ struct Device { bool hasOtherFirmware{true}; // SWITCH has somewhere to go bool hasBootloaderOp{true}; // firmware that answers BOOTLOADER QString restartedInto; // "other" or "bootloader" after a restart request + std::uint8_t restartStatusOp{0}; // not 0: the op its restart STATUS names instead + bool restartsSilently{false}; // leaves for a restart without answering it int dropsOnOpen{0}; // this open (1 = the first) fails before it negotiates }; @@ -272,7 +274,8 @@ class SimPort final : public Ctr2HidPort { status(d[0], CTR2_FW_BAD_STATE); break; } - status(d[0], CTR2_FW_OK); + if (!m_dev->restartsSilently) + status(m_dev->restartStatusOp ? m_dev->restartStatusOp : d[0], CTR2_FW_OK); m_dev->restartedInto = loader ? QStringLiteral("bootloader") : QStringLiteral("other"); m_dev->gone = true; QTimer::singleShot(5, this, [this] { @@ -581,6 +584,21 @@ void testRestartRequests() run = restart(alone, R::OtherFirmware); check(!run.ok && alone.restartedInto.isEmpty(), "SWITCH with no other firmware fails"); check(alone.closes == 1, "and the session is closed"); + + // Lynovation's v1.02.00 betas named the op as a character ('5', 0x35) + // and left the bus two seconds later. + Device misnamed; + misnamed.restartStatusOp = 0x30 | CTR2_FW_SWITCH; + run = restart(misnamed, R::OtherFirmware); + check(run.ok && misnamed.restartedInto == QStringLiteral("other") + && run.message.contains(QStringLiteral("restarting")), + "a device that leaves after a STATUS naming another op has restarted"); + + Device silent; + silent.restartsSilently = true; + run = restart(silent, R::Bootloader); + check(run.ok && run.message.contains(QStringLiteral("download mode")), + "a device that leaves for download mode without answering has restarted"); } // AetherSDR quitting mid-update destroys the updater: the device must still From a284c31922dcf7b01defd958f5a9c32c38954ca9 Mon Sep 17 00:00:00 2001 From: "Jeremy [KK7GWY]" Date: Sat, 24 Oct 2026 10:47:16 -0700 Subject: [PATCH 3/5] fix(ctr2): a restart request counts as done only once it was written. Principle XI. Review of #6736: any port failure while the SWITCH or BOOTLOADER answer was outstanding finished the request as a restart, including a failed write of the request itself (the USB worker's "CTR2 USB write failed"), so a request the device never received read as "The device is restarting". The updater now counts the reports it queues on each port and the ones the port reports written (reportsSent, which the worker delivers before any later failure). A port loss counts as the restart only once the request has been written; before that it is "Lost the device ... The device keeps the firmware it had." as it was. Tests: the simulated port acknowledges its writes like the worker, and can fail the write of a FIRMWARE message. That case is reported lost, not restarted, and fails with the written-check removed; the two departure cases fail with the check forced false. The user guide's Backups section now says the restore question has Cancel as its default (review nit). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WvUonxmfcg6yopRJnXx9VG --- docs/ctr2-usb-relay-design.md | 6 +++-- docs/user/docs/ctr2-proxy.md | 4 +++- src/core/Ctr2FirmwareUpdater.cpp | 18 ++++++++++++--- src/core/Ctr2FirmwareUpdater.h | 6 +++++ tests/ctr2_firmware_updater_test.cpp | 34 ++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+), 6 deletions(-) diff --git a/docs/ctr2-usb-relay-design.md b/docs/ctr2-usb-relay-design.md index c4ad0c56c4..7647b26639 100644 --- a/docs/ctr2-usb-relay-design.md +++ b/docs/ctr2-usb-relay-design.md @@ -417,8 +417,10 @@ own business. A device answers SWITCH and BOOTLOADER with STATUS before it leaves the bus. AetherSDR also counts the request done if the device leaves while the answer -is outstanding, so a restart that beats its answer, or an answer naming the -wrong op, is not reported as a lost device. +is outstanding, once the port has written the request, so a restart that beats +its answer, or an answer naming the wrong op, is not reported as a lost +device. A port that fails before writing it (a write error) still reports the +device lost. **Host implementation.** `Ctr2FirmwareUpdater` runs the session and checks the image before sending anything: an ESP32-S3 application image with an diff --git a/docs/user/docs/ctr2-proxy.md b/docs/user/docs/ctr2-proxy.md index 3e67d2c8bc..089c014d33 100644 --- a/docs/user/docs/ctr2-proxy.md +++ b/docs/user/docs/ctr2-proxy.md @@ -263,7 +263,9 @@ date, including any saved with **Back Up…** in earlier versions of AetherSDR it. Installing one restores it: the whole flash goes back exactly as it was, both firmwares, the settings and the registration included, and anything since is lost. A backup restores only onto the unit it came from (matched by -its MAC address), and only if the file is unchanged since it was made. +its MAC address), and only if the file is unchanged since it was made. The +question before a restore has **Cancel** as its default, so pressing Return +keeps what is on the unit; restoring takes a click on **Restore**. ### Cancelling diff --git a/src/core/Ctr2FirmwareUpdater.cpp b/src/core/Ctr2FirmwareUpdater.cpp index ced5cf9c5a..2e44926af1 100644 --- a/src/core/Ctr2FirmwareUpdater.cpp +++ b/src/core/Ctr2FirmwareUpdater.cpp @@ -202,7 +202,11 @@ bool Ctr2FirmwareUpdater::openPort(QString* error) port->setParent(nullptr); m_reassembler.reset(); m_reassembler.setExtensions(capabilityBit(MessageType::Firmware)); + m_reportsQueued = 0; + m_reportsWritten = 0; + m_restartQueuedTo = 0; connect(port, &Ctr2HidPort::extensionsNegotiated, this, &Ctr2FirmwareUpdater::onExtensions); + connect(port, &Ctr2HidPort::reportsSent, this, [this](int count) { m_reportsWritten += count; }); connect(port, &Ctr2HidPort::reportsReceived, this, &Ctr2FirmwareUpdater::onReports); connect(port, &Ctr2HidPort::failed, this, &Ctr2FirmwareUpdater::onPortFailed); return true; @@ -270,6 +274,7 @@ void Ctr2FirmwareUpdater::sendHello() std::vector out; m_encoder.reset(); m_encoder.encodeControl(MessageType::Hello, &out); + m_reportsQueued += qint64(out.size()); m_port->send(out); } @@ -277,6 +282,7 @@ void Ctr2FirmwareUpdater::sendExtension(const QByteArray& payload) { std::vector out; if (m_port && m_encoder.encodeExtension(MessageType::Firmware, payload, &out)) { + m_reportsQueued += qint64(out.size()); m_port->send(out); } } @@ -317,6 +323,7 @@ void Ctr2FirmwareUpdater::onMessage(const Message& message) setPhase(Phase::Commanding, loader ? tr("Restarting into download mode") : tr("Restarting into the other firmware")); sendExtension(loader ? firmware::encodeBootloader() : firmware::encodeSwitch()); + m_restartQueuedTo = m_reportsQueued; armTimeout(m_timing.answerTimeoutMs); } else if (m_phase == Phase::Calling) { m_retry->stop(); @@ -459,9 +466,14 @@ void Ctr2FirmwareUpdater::onPortFailed(const QString& message) break; case Phase::Commanding: // Leaving the bus is what the request asks for. A device that does it - // before its STATUS arrives, or whose STATUS names another op, has - // still restarted. - restarted(); + // after the request was written, before its STATUS arrives or with a + // STATUS naming another op, has still restarted. A port that failed + // before writing it (a write error) has not. + if (m_reportsWritten >= m_restartQueuedTo) { + restarted(); + break; + } + fail(tr("Lost the device: %1.").arg(message) + kKeepsFirmware); break; default: fail(tr("Lost the device: %1.").arg(message) + kKeepsFirmware); diff --git a/src/core/Ctr2FirmwareUpdater.h b/src/core/Ctr2FirmwareUpdater.h index 0351b15f94..ef88ed41ef 100644 --- a/src/core/Ctr2FirmwareUpdater.h +++ b/src/core/Ctr2FirmwareUpdater.h @@ -132,6 +132,12 @@ class Ctr2FirmwareUpdater : public QObject { qint64 m_sent{0}; // image bytes sent qint64 m_stored{0}; // image bytes the device has acknowledged int m_inFlight{0}; + // Reports this port has queued and written, and the queued count once the + // restart request was queued: it has reached the device's endpoint when + // the written count gets there. + qint64 m_reportsQueued{0}; + qint64 m_reportsWritten{0}; + qint64 m_restartQueuedTo{0}; QString m_previousVersion; }; diff --git a/tests/ctr2_firmware_updater_test.cpp b/tests/ctr2_firmware_updater_test.cpp index b49e7b9cb4..df39ef6a6c 100644 --- a/tests/ctr2_firmware_updater_test.cpp +++ b/tests/ctr2_firmware_updater_test.cpp @@ -22,6 +22,7 @@ extern "C" { #include #include +#include #include #include #include @@ -75,6 +76,7 @@ struct Device { QString restartedInto; // "other" or "bootloader" after a restart request std::uint8_t restartStatusOp{0}; // not 0: the op its restart STATUS names instead bool restartsSilently{false}; // leaves for a restart without answering it + bool failsFirmwareWrite{false}; // the write of a FIRMWARE message fails (EIO) int dropsOnOpen{0}; // this open (1 = the first) fails before it negotiates }; @@ -120,11 +122,34 @@ class SimPort final : public Ctr2HidPort { bool isOpen() const override { return m_open; } void send(const std::vector& reports) override { + if (!m_open) { + return; + } + // As the USB worker does: a write that fails closes the port before + // the device sees the reports, and nothing is counted as sent. + const bool firmware = std::any_of(reports.begin(), reports.end(), [](const Report& r) { + return r.data()[0] == 0xFF && r.data()[3] == std::uint8_t(MessageType::Firmware); + }); + if (firmware && m_dev->failsFirmwareWrite) { + m_open = false; + QTimer::singleShot(0, this, [this] { + emit failed(QStringLiteral("CTR2 USB write failed: injected EIO")); + }); + return; + } for (const Report& r : reports) { if (m_open) { onReport(r.data()); } } + // Written reports are acknowledged before anything that follows. + const int count = int(reports.size()); + QPointer self(this); + QTimer::singleShot(0, this, [self, count] { + if (self) { + emit self->reportsSent(count); + } + }); } void discardQueued() override {} void shutdown(const std::vector& finalReports) override @@ -599,6 +624,15 @@ void testRestartRequests() run = restart(silent, R::Bootloader); check(run.ok && run.message.contains(QStringLiteral("download mode")), "a device that leaves for download mode without answering has restarted"); + + // A port that fails writing the request has not sent it: no restart. + Device unwritten; + unwritten.failsFirmwareWrite = true; + run = restart(unwritten, R::OtherFirmware); + check(!run.ok && unwritten.restartedInto.isEmpty() + && run.message.contains(QStringLiteral("write failed")) + && run.message.contains(QStringLiteral("keeps the firmware")), + "a restart request whose write fails is reported as lost, not as a restart"); } // AetherSDR quitting mid-update destroys the updater: the device must still From 5269db7b1181452c2c7c086b27d5bb2cb041ed5a Mon Sep 17 00:00:00 2001 From: "Jeremy [KK7GWY]" Date: Sat, 24 Oct 2026 11:00:41 -0700 Subject: [PATCH 4/5] fix(ctr2): canon box keyboard, width and screen reader; count every write. Principle XI. From the automated review of #6736: - CanonMessageBox buttons are auto-default, as QMessageBox's are, so Return on a focused button clicks that button. With them not auto-default, a platform theme that leaves Return out of its button-press keys would send Return on a focused Cancel to the default Install. - The texts are at least 420 px wide and may grow to 720, so a long path in a command to copy wraps or widens the box instead of being clipped. - Opening it sends the screen reader an Alert, as QMessageBox does, and its description carries both texts. - setModal() dropped: every caller runs it with exec(). - Ctr2FirmwareUpdater: a restart request that could not be queued fails at once, instead of counting as written. - Ctr2HidThreadPort: writes that went out before a failing one in the same tick are reported sent, before the failure, so the updater's written count never misses a request the device received. - dialog-patterns.md: the CanonMessageBox paragraph follows the CanonWindow ones instead of splitting them. Tests: Return on a focused Cancel clicks Cancel; the real corner close button leaves no button clicked; the description holds both texts; writes before a failing one in the same tick are counted before the failure (fails with that acknowledgement removed). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WvUonxmfcg6yopRJnXx9VG --- docs/style/dialog-patterns.md | 14 +++++++------- src/core/Ctr2FirmwareUpdater.cpp | 15 ++++++++++----- src/core/Ctr2FirmwareUpdater.h | 3 ++- src/core/Ctr2HidThreadPort.cpp | 7 +++++++ src/gui/CanonMessageBox.cpp | 23 ++++++++++++++++++---- tests/canon_message_box_test.cpp | 30 ++++++++++++++++++++++++++++- tests/ctr2_hid_thread_port_test.cpp | 21 ++++++++++++++++++++ 7 files changed, 95 insertions(+), 18 deletions(-) diff --git a/docs/style/dialog-patterns.md b/docs/style/dialog-patterns.md index 50f5d25cac..831bc4b2eb 100644 --- a/docs/style/dialog-patterns.md +++ b/docs/style/dialog-patterns.md @@ -269,13 +269,6 @@ What it keeps: is set. They animate only while visible and tick slowly while the window is not exposed. -A question or notice from a canon window uses `CanonMessageBox` -(`src/gui/CanonMessageBox.{h,cpp}`) in place of `QMessageBox`: a canon dialog -with the title as a canon header, the text, optional further text, and the -buttons at the bottom right. `clickedButton()` names the button clicked and is -null when the box is closed any other way; the default button is outlined in -cyan and a destructive one is drawn in gold. The CTR2 window's questions use it. - It is translucent, so the rounded corners need a compositor; an X11 session without one shows square black corners. Use `CanonWindow` only for windows the style guide covers: short-lived windows, and the tool windows the maintainer @@ -283,6 +276,13 @@ has moved to it (Waveforms, Network Diagnostics, AetherRX, AetherTX, AetherModem installs `FramelessResizer`, since `CanonWindow` itself only moves. Other tool and workspace dialogs keep the persistent pattern above. +A question or notice from a canon window uses `CanonMessageBox` +(`src/gui/CanonMessageBox.{h,cpp}`) in place of `QMessageBox`: a canon dialog +with the title as a canon header, the text, optional further text, and the +buttons at the bottom right. `clickedButton()` names the button clicked and is +null when the box is closed any other way; the default button is outlined in +cyan and a destructive one is drawn in gold. The CTR2 window's questions use it. + ## Common pitfalls These are the issues that have hit real PRs in the project's history. diff --git a/src/core/Ctr2FirmwareUpdater.cpp b/src/core/Ctr2FirmwareUpdater.cpp index 2e44926af1..34e51ee2f2 100644 --- a/src/core/Ctr2FirmwareUpdater.cpp +++ b/src/core/Ctr2FirmwareUpdater.cpp @@ -278,13 +278,15 @@ void Ctr2FirmwareUpdater::sendHello() m_port->send(out); } -void Ctr2FirmwareUpdater::sendExtension(const QByteArray& payload) +bool Ctr2FirmwareUpdater::sendExtension(const QByteArray& payload) { std::vector out; - if (m_port && m_encoder.encodeExtension(MessageType::Firmware, payload, &out)) { - m_reportsQueued += qint64(out.size()); - m_port->send(out); + if (!m_port || !m_encoder.encodeExtension(MessageType::Firmware, payload, &out)) { + return false; } + m_reportsQueued += qint64(out.size()); + m_port->send(out); + return true; } void Ctr2FirmwareUpdater::onReports(const QByteArray& reports) @@ -322,7 +324,10 @@ void Ctr2FirmwareUpdater::onMessage(const Message& message) const bool loader = *m_restart == Restart::Bootloader; setPhase(Phase::Commanding, loader ? tr("Restarting into download mode") : tr("Restarting into the other firmware")); - sendExtension(loader ? firmware::encodeBootloader() : firmware::encodeSwitch()); + if (!sendExtension(loader ? firmware::encodeBootloader() : firmware::encodeSwitch())) { + fail(tr("The restart request could not be sent.")); + return; + } m_restartQueuedTo = m_reportsQueued; armTimeout(m_timing.answerTimeoutMs); } else if (m_phase == Phase::Calling) { diff --git a/src/core/Ctr2FirmwareUpdater.h b/src/core/Ctr2FirmwareUpdater.h index ef88ed41ef..36dacc2823 100644 --- a/src/core/Ctr2FirmwareUpdater.h +++ b/src/core/Ctr2FirmwareUpdater.h @@ -105,7 +105,8 @@ class Ctr2FirmwareUpdater : public QObject { void onPortFailed(const QString& message); void onTimeout(); void sendHello(); - void sendExtension(const QByteArray& payload); + // False when nothing could be queued (no port, or the payload would not encode). + bool sendExtension(const QByteArray& payload); void sendChunks(); void lookForDevice(); void fail(const QString& message); diff --git a/src/core/Ctr2HidThreadPort.cpp b/src/core/Ctr2HidThreadPort.cpp index bd9553e130..04d5e0c480 100644 --- a/src/core/Ctr2HidThreadPort.cpp +++ b/src/core/Ctr2HidThreadPort.cpp @@ -255,6 +255,13 @@ class Ctr2HidIoWorker : public QObject { } const int n = writeReports(m_outbox, 0); if (n < 0) { + // What this tick did write reached the device: count it + // before the failure, which arrives after it. + if (sent > 0) { + QMetaObject::invokeMethod(m_port, [port = m_port, sent, generation] { + port->deliverSent(sent, generation); + }, Qt::QueuedConnection); + } fail(QStringLiteral("CTR2 USB write failed: %1").arg(errorText())); return; } diff --git a/src/gui/CanonMessageBox.cpp b/src/gui/CanonMessageBox.cpp index 1e6114544b..58ee64a645 100644 --- a/src/gui/CanonMessageBox.cpp +++ b/src/gui/CanonMessageBox.cpp @@ -3,6 +3,7 @@ #include "CanonIndicators.h" #include "ModemChrome.h" +#include #include #include #include @@ -12,7 +13,10 @@ namespace AetherSDR { namespace { +// The texts wrap at the narrower width; one long unbreakable token (a path in +// a command to copy) widens the box up to the wider one rather than clipping. constexpr int kTextWidth = 420; +constexpr int kMaxTextWidth = 720; } // namespace @@ -20,7 +24,6 @@ CanonMessageBox::CanonMessageBox(const QString& title, const QString& text, QWid : CanonWindow(title, parent) { setObjectName(QStringLiteral("canonMessageBox")); - setModal(true); applyCanonSheet(bodyWidget(), [] { return ModemChrome::styleSheet(ModemChrome::Scale::Dialog, ModemChrome::Look::Canon) + QStringLiteral( @@ -36,16 +39,21 @@ CanonMessageBox::CanonMessageBox(const QString& title, const QString& text, QWid root->setSpacing(12); root->addWidget(makeCanonHeader(title)); + // A screen reader announces a QMessageBox's text when it opens; this + // dialog is told the same through its description. + setAccessibleDescription(text); m_text = new QLabel(text); m_text->setObjectName(QStringLiteral("canonMessageText")); m_text->setWordWrap(true); - m_text->setFixedWidth(kTextWidth); + m_text->setMinimumWidth(kTextWidth); + m_text->setMaximumWidth(kMaxTextWidth); root->addWidget(m_text); m_info = new QLabel; m_info->setObjectName(QStringLiteral("canonMessageInfo")); m_info->setWordWrap(true); - m_info->setFixedWidth(kTextWidth); + m_info->setMinimumWidth(kTextWidth); + m_info->setMaximumWidth(kMaxTextWidth); m_info->hide(); root->addWidget(m_info); @@ -60,6 +68,8 @@ void CanonMessageBox::setInformativeText(const QString& text) { m_info->setText(text); m_info->setVisible(!text.isEmpty()); + setAccessibleDescription(text.isEmpty() ? m_text->text() + : m_text->text() + QLatin1Char('\n') + text); } void CanonMessageBox::setTextSelectable(bool on) @@ -71,8 +81,10 @@ void CanonMessageBox::setTextSelectable(bool on) QPushButton* CanonMessageBox::addButton(const QString& text, Role role) { + // Auto-default, as in QMessageBox: a focused button takes Return itself, + // and the default button takes it only while no other button has focus. auto* button = new QPushButton(text); - button->setAutoDefault(false); + button->setAutoDefault(true); if (role == Role::Destructive) { button->setProperty("role", QStringLiteral("destructive")); } @@ -103,6 +115,9 @@ void CanonMessageBox::showEvent(QShowEvent* event) if (m_default) { m_default->setFocus(); } + // As QMessageBox does, so a screen reader reads the texts as it opens. + QAccessibleEvent alert(this, QAccessible::Alert); + QAccessible::updateAccessibility(&alert); } } // namespace AetherSDR diff --git a/tests/canon_message_box_test.cpp b/tests/canon_message_box_test.cpp index b4ee72322b..45b51a5fdb 100644 --- a/tests/canon_message_box_test.cpp +++ b/tests/canon_message_box_test.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -97,7 +98,13 @@ int main(int argc, char** argv) // ---- the corner close button closes it with no button clicked ---- { Box b; - const int result = b.run([](CanonMessageBox& box) { box.close(); }); + const int result = b.run([](CanonMessageBox& box) { + // The corner button is the window's only QToolButton. + if (auto* close = box.findChild()) + close->click(); + else + box.done(-1); + }); EXPECT_TRUE(result == QDialog::Rejected); EXPECT_TRUE(b.box.clickedButton() == nullptr); } @@ -116,14 +123,35 @@ int main(int argc, char** argv) EXPECT_TRUE(b.install->isDefault() && !b.cancel->isDefault()); } + // ---- Return on a focused button clicks that button, not the default ---- + // Buttons are auto-default for this. A platform theme whose button-press + // keys include Return (as here, offscreen) gives the same result without + // it; one that leaves Return out would click the default instead. + { + Box b; + bool cancelFocused = false; + const int result = b.run([&b, &cancelFocused](CanonMessageBox& box) { + b.cancel->setFocus(); + cancelFocused = QApplication::focusWidget() == b.cancel; + pressKey(box, Qt::Key_Return); + }); + EXPECT_TRUE(cancelFocused); + EXPECT_TRUE(result == QDialog::Rejected); + EXPECT_TRUE(b.box.clickedButton() == b.cancel); + } + // ---- the further text shows only when there is some; a destructive // button accepts and is marked for the warning color ---- { CanonMessageBox box(QStringLiteral("Restore the backup?"), QStringLiteral("Restore it?")); auto* info = box.findChild(QStringLiteral("canonMessageInfo")); EXPECT_TRUE(info && info->isHidden()); + EXPECT_TRUE(box.accessibleDescription() == QStringLiteral("Restore it?")); box.setInformativeText(QStringLiteral("Only onto the CTR2-Max it came from.")); EXPECT_TRUE(info && !info->isHidden()); + // A screen reader reads both texts when the box opens. + EXPECT_TRUE(box.accessibleDescription() + == QStringLiteral("Restore it?\nOnly onto the CTR2-Max it came from.")); QPushButton* restore = box.addButton(QStringLiteral("Restore"), CanonMessageBox::Role::Destructive); EXPECT_TRUE(restore->property("role").toString() == QStringLiteral("destructive")); diff --git a/tests/ctr2_hid_thread_port_test.cpp b/tests/ctr2_hid_thread_port_test.cpp index 0a62342a51..1394880057 100644 --- a/tests/ctr2_hid_thread_port_test.cpp +++ b/tests/ctr2_hid_thread_port_test.cpp @@ -54,6 +54,7 @@ struct FakeDevice { std::atomic writeDelayMs{0}; std::atomic writesBegun{0}; std::atomic failWrites{false}; + std::atomic failWritesAfter{-1}; // not -1: writes after this many fail std::atomic closed{false}; std::function onWritten; // after a write completes, on the I/O thread QByteArray featureReply; // Get Feature 0x02 answer, ID first; empty = stall @@ -83,6 +84,9 @@ struct FakeDevice { return -1; } std::lock_guard g(lock); + if (failWritesAfter >= 0 && static_cast(writes.size()) >= failWritesAfter) { + return -1; + } writes.emplace_back(reinterpret_cast(buffer), size); if (onWritten) { onWritten(); @@ -224,6 +228,23 @@ void testReadsAndFailures() "a failed write fails the port with the device's reason"); port2->shutdown({}); waitUntil([] { return false; }, 100); + + // Writes that went out before a failing one in the same tick are counted, + // and reported before the failure: the firmware updater relies on it. + auto dev3 = std::make_shared(); + dev3->failWritesAfter = 2; + auto* port3 = new Ctr2HidThreadPort(dev3->io(), QStringLiteral("fake")); + int sent3 = 0; + int sentAtFailure = -1; + QObject::connect(port3, &Ctr2HidPort::reportsSent, port3, [&](int n) { sent3 += n; }); + QObject::connect(port3, &Ctr2HidPort::failed, port3, + [&](const QString&) { sentAtFailure = sent3; }); + port3->send(reports(3, 0x50)); + check(waitUntil([&] { return sentAtFailure >= 0; }) && dev3->writeCount() == 2 + && sentAtFailure == 2, + "writes before a failing one in the same tick are reported sent, before the failure"); + port3->shutdown({}); + waitUntil([] { return false; }, 100); } // Stop then Start on one device: the old port's slow final CLOSED and the new From 182af47bc835e92b3e9b0adbb3703fbd04bed01e Mon Sep 17 00:00:00 2001 From: "Jeremy [KK7GWY]" Date: Sat, 24 Oct 2026 11:18:23 -0700 Subject: [PATCH 5/5] fix(ctr2): the canon box shows its texts as plain text, and forgets an old answer. Principle VII. From aethersdr-agent's review of #6736: - The title, text and further text are Qt::PlainText. They carry firmware-manifest and device strings (a product name, a version, a path); markup in them is shown, never rendered. - clickedButton() is cleared each time the box opens, so a box shown again cannot report the previous showing's button. - setDefaultButton() says every box sets one. Tests: markup in all three texts stays plain (fails with the main text's format removed); a box shown again after a click and then dismissed with Escape reports no button (fails without the reset). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01WvUonxmfcg6yopRJnXx9VG --- src/gui/CanonMessageBox.cpp | 10 +++++++++- src/gui/CanonMessageBox.h | 1 + tests/canon_message_box_test.cpp | 22 ++++++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/gui/CanonMessageBox.cpp b/src/gui/CanonMessageBox.cpp index 58ee64a645..ce80b38146 100644 --- a/src/gui/CanonMessageBox.cpp +++ b/src/gui/CanonMessageBox.cpp @@ -37,13 +37,19 @@ CanonMessageBox::CanonMessageBox(const QString& title, const QString& text, QWid auto* root = new QVBoxLayout(bodyWidget()); root->setContentsMargins(18, 14, 18, 16); root->setSpacing(12); - root->addWidget(makeCanonHeader(title)); + // Texts carry manifest and device strings (a product name, a version, a + // path): never render them as rich text, the title included. + QWidget* header = makeCanonHeader(title); + if (auto* heading = header->findChild(QStringLiteral("canonHeaderTitle"))) + heading->setTextFormat(Qt::PlainText); + root->addWidget(header); // A screen reader announces a QMessageBox's text when it opens; this // dialog is told the same through its description. setAccessibleDescription(text); m_text = new QLabel(text); m_text->setObjectName(QStringLiteral("canonMessageText")); + m_text->setTextFormat(Qt::PlainText); m_text->setWordWrap(true); m_text->setMinimumWidth(kTextWidth); m_text->setMaximumWidth(kMaxTextWidth); @@ -51,6 +57,7 @@ CanonMessageBox::CanonMessageBox(const QString& title, const QString& text, QWid m_info = new QLabel; m_info->setObjectName(QStringLiteral("canonMessageInfo")); + m_info->setTextFormat(Qt::PlainText); m_info->setWordWrap(true); m_info->setMinimumWidth(kTextWidth); m_info->setMaximumWidth(kMaxTextWidth); @@ -112,6 +119,7 @@ void CanonMessageBox::setDefaultButton(QPushButton* button) void CanonMessageBox::showEvent(QShowEvent* event) { CanonWindow::showEvent(event); + m_clicked = nullptr; // this showing's answer, not a previous one's if (m_default) { m_default->setFocus(); } diff --git a/src/gui/CanonMessageBox.h b/src/gui/CanonMessageBox.h index 6e4db796b9..916dde2556 100644 --- a/src/gui/CanonMessageBox.h +++ b/src/gui/CanonMessageBox.h @@ -33,6 +33,7 @@ class CanonMessageBox : public CanonWindow { QPushButton* addButton(const QString& text, Role role); // The button Return clicks; also takes the focus when the box opens. + // Every box sets one; without it nothing decides where the focus starts. void setDefaultButton(QPushButton* button); QPushButton* clickedButton() const { return m_clicked; } diff --git a/tests/canon_message_box_test.cpp b/tests/canon_message_box_test.cpp index 45b51a5fdb..d88e682b82 100644 --- a/tests/canon_message_box_test.cpp +++ b/tests/canon_message_box_test.cpp @@ -123,6 +123,28 @@ int main(int argc, char** argv) EXPECT_TRUE(b.install->isDefault() && !b.cancel->isDefault()); } + // ---- a box shown again forgets the previous answer ---- + { + Box b; + b.run([&b](CanonMessageBox&) { b.install->click(); }); + const int result = b.run([](CanonMessageBox& box) { pressKey(box, Qt::Key_Escape); }); + EXPECT_TRUE(result == QDialog::Rejected); + EXPECT_TRUE(b.box.clickedButton() == nullptr); + } + + // ---- markup in a manifest string is shown as text, never rendered ---- + { + CanonMessageBox box(QStringLiteral("Install X?"), + QStringLiteral("Install X 1.0?")); + box.setInformativeText(QStringLiteral("y")); + int plain = 0; + for (QLabel* l : box.findChildren()) { + if (l->text().contains(QLatin1Char('<'))) + plain += l->textFormat() == Qt::PlainText ? 1 : -100; + } + EXPECT_TRUE(plain == 3); // title, text and further text + } + // ---- Return on a focused button clicks that button, not the default ---- // Buttons are auto-default for this. A platform theme whose button-press // keys include Return (as here, offscreen) gives the same result without