Skip to content

Commit f0ba4e8

Browse files
ten9876claude
andcommitted
fix(slice): review nits — off-screen pill letter, dialog signal, memoize, tests (#2606)
Addresses follow-up items raised in the PR self-review: 1. **Spectrum off-screen pill + context menus** now resolve the slice letter via SliceLabel::unicodeForm(sliceId, perClientLetter) so the displayed letter follows the same display mode as the marker colour. Previously the pill could read "C" while the marker was painted in cyan because the user's slice was A₂. 2. **Serial-shortcut RadioSetupDialog** (the Settings → Flex Control path) now also connects the sliceLetterDisplayModeChanged signal to the slice-letter refresh fan-out so toggling the mode from that dialog repaints live, not on the next slice event. 3. **RxApplet tab-row stylesheet memoization** — store the last applied colour index per button and skip setStyleSheet when nothing colour-relevant changed. updateSliceButtons fires on every slot occupancy + letter change; cheap, but no reason to re-parse the stylesheet when the palette is unchanged. 4. **slice_model_letter_test** — 6 cases covering letter() fallback, index_letter parse + letterChanged emission, no-op re-apply, letter change, emitLetterRefresh, and unrelated status messages not disturbing the letter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 9ae7fcc commit f0ba4e8

5 files changed

Lines changed: 151 additions & 7 deletions

File tree

CMakeLists.txt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,6 +1274,14 @@ target_include_directories(slice_label_test PRIVATE src)
12741274
target_link_libraries(slice_label_test PRIVATE Qt6::Gui)
12751275
add_test(NAME slice_label_test COMMAND slice_label_test)
12761276

1277+
add_executable(slice_model_letter_test
1278+
tests/slice_model_letter_test.cpp
1279+
src/models/SliceModel.cpp
1280+
)
1281+
target_include_directories(slice_model_letter_test PRIVATE src)
1282+
target_link_libraries(slice_model_letter_test PRIVATE Qt6::Core Qt6::Test)
1283+
add_test(NAME slice_model_letter_test COMMAND slice_model_letter_test)
1284+
12771285
add_executable(client_quindar_test
12781286
tests/client_quindar_test.cpp
12791287
src/core/ClientQuindarTone.cpp

src/gui/MainWindow.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6295,6 +6295,14 @@ void MainWindow::buildMenuBar()
62956295
dlg->setAttribute(Qt::WA_DeleteOnClose);
62966296
connect(dlg, &RadioSetupDialog::txBandSettingsRequested,
62976297
m_txBandAction, &QAction::trigger);
6298+
// Same Multi-Flex display-mode refresh wiring as the primary
6299+
// Radio Setup entrypoint — keep both code paths in sync so a
6300+
// toggle from the Serial shortcut still triggers a live repaint.
6301+
connect(dlg, &RadioSetupDialog::sliceLetterDisplayModeChanged,
6302+
this, [this]() {
6303+
for (auto* s : m_radioModel.slices())
6304+
s->emitLetterRefresh();
6305+
});
62986306
if (auto* tabs = dlg->findChild<QTabWidget*>()) {
62996307
for (int i = 0; i < tabs->count(); ++i) {
63006308
if (tabs->tabText(i) == "Serial") {

src/gui/RxApplet.cpp

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1394,6 +1394,18 @@ void RxApplet::updateSliceButtons(const QList<SliceModel*>& slices, int activeSl
13941394
.arg(color);
13951395
};
13961396

1397+
// Cache the last applied colour index per button so we skip the
1398+
// stylesheet rebuild + setStyleSheet() on every refresh when nothing
1399+
// colour-relevant changed (the loop runs on slot occupancy + letter
1400+
// signals, both of which fire often).
1401+
auto applyStyleIfChanged = [](QToolButton* btn, int colourIdx,
1402+
const QString& stylesheet) {
1403+
const QVariant prev = btn->property("colourIdx");
1404+
if (prev.isValid() && prev.toInt() == colourIdx) return;
1405+
btn->setProperty("colourIdx", colourIdx);
1406+
btn->setStyleSheet(stylesheet);
1407+
};
1408+
13971409
for (int btnPos = 0; btnPos < m_sliceBtns.size(); ++btnPos) {
13981410
auto* btn = m_sliceBtns[btnPos];
13991411

@@ -1447,7 +1459,7 @@ void RxApplet::updateSliceButtons(const QList<SliceModel*>& slices, int activeSl
14471459
btn->setChecked(slotId == activeSliceId);
14481460
// Colour pairs with the displayed letter in RadioIndexed mode.
14491461
const int colourIdx = SliceLabel::displayColorIndex(slotId, ourSlice->letter());
1450-
btn->setStyleSheet(buildButtonStyle(colourIdx));
1462+
applyStyleIfChanged(btn, colourIdx, buildButtonStyle(colourIdx));
14511463
} else if (qstrcmp(state, "foreign") == 0) {
14521464
// Foreign slots always render as "—" so they're visually
14531465
// distinguishable from empty slots even with the dim styling
@@ -1468,7 +1480,7 @@ void RxApplet::updateSliceButtons(const QList<SliceModel*>& slices, int activeSl
14681480
.arg(QChar('A' + slotId)).arg(owner));
14691481
// Foreign keeps its global slot colour as the base palette;
14701482
// the slotState QSS override paints it in the dim grey scheme.
1471-
btn->setStyleSheet(buildButtonStyle(slotId));
1483+
applyStyleIfChanged(btn, slotId, buildButtonStyle(slotId));
14721484
} else {
14731485
// Empty / available slot. In Global mode show the global
14741486
// letter for the slot. In RadioIndexed mode show the
@@ -1492,7 +1504,7 @@ void RxApplet::updateSliceButtons(const QList<SliceModel*>& slices, int activeSl
14921504
btn->setProperty("sliceId", QVariant());
14931505
btn->setProperty("slotState", "empty");
14941506
btn->setToolTip(QString("Slot %1 — empty").arg(QChar('A' + colourPos)));
1495-
btn->setStyleSheet(buildButtonStyle(colourIdx));
1507+
applyStyleIfChanged(btn, colourIdx, buildButtonStyle(colourIdx));
14961508
}
14971509
// Force a stylesheet re-evaluation so the slotState property change
14981510
// is picked up by the QSS attribute selectors.

src/gui/SpectrumWidget.cpp

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2667,7 +2667,9 @@ void SpectrumWidget::mousePressEvent(QMouseEvent* ev)
26672667
if (!m_offScreenRects[oi].isNull() &&
26682668
m_offScreenRects[oi].contains(QPoint(mx, y))) {
26692669
const auto& so = m_sliceOverlays[oi];
2670-
const QChar letter = QChar('A' + (so.sliceId % kSliceColorCount));
2670+
// Follow display mode so menu labels match the pill above (#2606).
2671+
const QString letter =
2672+
SliceLabel::unicodeForm(so.sliceId, so.perClientLetter);
26712673
QMenu menu(this);
26722674
menu.addAction(QString("Close Slice %1").arg(letter), this,
26732675
[this, id = so.sliceId]{ emit sliceCloseRequested(id); });
@@ -2808,12 +2810,18 @@ void SpectrumWidget::mousePressEvent(QMouseEvent* ev)
28082810
menu.addSeparator();
28092811
int closestSlice = -1;
28102812
int closestDist = INT_MAX;
2813+
QString closestLetter;
28112814
for (const auto& so : m_sliceOverlays) {
28122815
int dist = std::abs(mx - mhzToX(so.freqMhz));
2813-
if (dist < closestDist) { closestDist = dist; closestSlice = so.sliceId; }
2816+
if (dist < closestDist) {
2817+
closestDist = dist;
2818+
closestSlice = so.sliceId;
2819+
closestLetter = so.perClientLetter;
2820+
}
28142821
}
28152822
if (closestSlice >= 0) {
2816-
const QChar letter = QChar('A' + (closestSlice & 3));
2823+
const QString letter =
2824+
SliceLabel::unicodeForm(closestSlice, closestLetter);
28172825
menu.addAction(QString("Close Slice %1").arg(letter), this,
28182826
[this, closestSlice]{ emit sliceCloseRequested(closestSlice); });
28192827
}
@@ -6651,7 +6659,12 @@ void SpectrumWidget::drawOffScreenSlices(QPainter& p, const QRect& specRect)
66516659

66526660
const bool isRight = (so.freqMhz > endMhz);
66536661
const QColor col = sliceColorForOverlay(so);
6654-
const QChar letter = QChar('A' + (so.sliceId % kSliceColorCount));
6662+
// Letter on the off-screen pill follows the same display mode as
6663+
// the marker colour: per-client letter (with Unicode subscript)
6664+
// in RadioIndexed mode, global letter in Global mode (#2606).
6665+
QString letter = SliceLabel::unicodeForm(so.sliceId, so.perClientLetter);
6666+
if (letter.isEmpty())
6667+
letter = QString(QChar('A' + (so.sliceId % kSliceColorCount)));
66556668

66566669
long long hz = static_cast<long long>(std::round(so.freqMhz * 1e6));
66576670
int mhzPart = static_cast<int>(hz / 1000000);

tests/slice_model_letter_test.cpp

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
#include "models/SliceModel.h"
2+
3+
#include <QCoreApplication>
4+
#include <QMap>
5+
#include <QSignalSpy>
6+
#include <QString>
7+
#include <cstdio>
8+
9+
using namespace AetherSDR;
10+
11+
static int g_failures = 0;
12+
13+
#define EXPECT_EQ(actual, expected) do { \
14+
auto a_ = (actual); auto e_ = (expected); \
15+
if (a_ != e_) { \
16+
const QString a_str = QString("%1").arg(a_); \
17+
const QString e_str = QString("%1").arg(e_); \
18+
std::fprintf(stderr, "FAIL %s:%d expected %s, got %s\n", \
19+
__FILE__, __LINE__, \
20+
e_str.toUtf8().constData(), \
21+
a_str.toUtf8().constData()); \
22+
++g_failures; \
23+
} \
24+
} while (0)
25+
26+
static QMap<QString, QString> kv(std::initializer_list<std::pair<QString, QString>> pairs)
27+
{
28+
QMap<QString, QString> m;
29+
for (const auto& p : pairs) m.insert(p.first, p.second);
30+
return m;
31+
}
32+
33+
int main(int argc, char** argv)
34+
{
35+
QCoreApplication app(argc, argv);
36+
37+
// ── Fallback: letter() returns 'A' + sliceId when index_letter hasn't
38+
// arrived yet (older firmware, early in status sequence).
39+
{
40+
SliceModel s(2);
41+
EXPECT_EQ(s.letter(), QString("C")); // 'A' + 2
42+
}
43+
44+
// ── applyStatus("index_letter=A") populates the radio-given letter
45+
// and emits letterChanged with the resolved value.
46+
{
47+
SliceModel s(3);
48+
QSignalSpy spy(&s, &SliceModel::letterChanged);
49+
s.applyStatus(kv({{"index_letter", "A"}}));
50+
EXPECT_EQ(s.letter(), QString("A"));
51+
EXPECT_EQ(spy.count(), 1);
52+
EXPECT_EQ(spy.takeFirst().at(0).toString(), QString("A"));
53+
}
54+
55+
// ── Re-applying the same letter does NOT re-emit letterChanged.
56+
{
57+
SliceModel s(1);
58+
s.applyStatus(kv({{"index_letter", "B"}}));
59+
QSignalSpy spy(&s, &SliceModel::letterChanged);
60+
s.applyStatus(kv({{"index_letter", "B"}}));
61+
EXPECT_EQ(spy.count(), 0);
62+
}
63+
64+
// ── Letter change emits exactly once and the resolved value follows.
65+
{
66+
SliceModel s(2);
67+
s.applyStatus(kv({{"index_letter", "A"}}));
68+
QSignalSpy spy(&s, &SliceModel::letterChanged);
69+
s.applyStatus(kv({{"index_letter", "C"}}));
70+
EXPECT_EQ(s.letter(), QString("C"));
71+
EXPECT_EQ(spy.count(), 1);
72+
EXPECT_EQ(spy.takeFirst().at(0).toString(), QString("C"));
73+
}
74+
75+
// ── emitLetterRefresh() forces a re-emission with the current value
76+
// (used when a display preference changes).
77+
{
78+
SliceModel s(0);
79+
s.applyStatus(kv({{"index_letter", "A"}}));
80+
QSignalSpy spy(&s, &SliceModel::letterChanged);
81+
s.emitLetterRefresh();
82+
EXPECT_EQ(spy.count(), 1);
83+
EXPECT_EQ(spy.takeFirst().at(0).toString(), QString("A"));
84+
}
85+
86+
// ── Status messages without index_letter don't disturb the stored
87+
// letter or emit on letterChanged.
88+
{
89+
SliceModel s(2);
90+
s.applyStatus(kv({{"index_letter", "A"}}));
91+
QSignalSpy spy(&s, &SliceModel::letterChanged);
92+
s.applyStatus(kv({{"in_use", "1"}, {"RF_frequency", "14.250"}}));
93+
EXPECT_EQ(s.letter(), QString("A"));
94+
EXPECT_EQ(spy.count(), 0);
95+
}
96+
97+
if (g_failures == 0) {
98+
std::printf("slice_model_letter_test: all checks passed\n");
99+
return 0;
100+
}
101+
std::printf("slice_model_letter_test: %d failure(s)\n", g_failures);
102+
return 1;
103+
}

0 commit comments

Comments
 (0)