Repository navigation
feat(pgxl): spell out MEffA on the panel key (#5940). Principle VIII. - #5984
Conversation
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes, for the part claimed. #5940 asks for three things: spell the caption out, show fan speed as "Fan: S", and mimic the vendor's ---- MEffa --- label above the PWR band (plus "tighten up overall height"). The triage comment split those into items 1–4 and you claimed 1, 2 and 4 explicitly in the issue thread before opening this — the diff is exactly that, and the title says (#5940) rather than "Closes", so the issue correctly stays open for the fan key and the hardware-mimicking label. That is the honest framing; no complaint.
One thing the PR body doesn't mention and is worth pointing out because it argues for the change: the comment already in AmpApplet.cpp:1366 says "the visible text stays MEffA in all three", which was false on main for the panel key (it read "ME") and this PR makes true. The two presentations of one control agreeing is the stronger justification than the width argument.
Scope
| File / group | What it changes | Claimed by the issue/body? | Verdict |
|---|---|---|---|
src/gui/AmpApplet.cpp:594-597 |
"ME" → "MEffA", stale width comment replaced |
Yes (issue text, triage item 1) | In scope |
src/gui/AmpApplet.cpp:612-621 |
seed width measured over both letterbox captions | Yes (triage item 2) | In scope |
src/gui/AmpApplet.cpp:803,886 |
two comments re-worded ("two glyphs" → "a word") | Follows from item 1 | In scope |
tests/amp_applet_test.cpp |
one new test + two includes, registered in main() |
Yes (triage item 4) | In scope |
Everything in the diff is explained by the issue. Two files, 46 insertions; no CHANGELOG.md entry (correct — it must not have one), no build/CI churn, no settings key, no capability, no protocol surface, no removed guard, no changed default. git diff bf97bc7f..a51830c1 --stat confirms the two-file footprint, and the single commit's message matches the issue.
Sibling surfaces: the only other caption for this control is the rail button at AmpApplet.cpp:481, which already read "MEffA" — that is the peer this change aligns to, so nothing is left half-fixed. grep -rn '"ME"' src/ tests/ docs/ finds one unrelated hit (EibiClient.cpp:75, the Middle East region code). No docs or screenshots reference the "ME" key.
Socket tests: none added, modified or removed. The new test constructs widgets and reads QFontMetrics; no QTcpServer/QUdpSocket/QLocalServer/bind/listen/connectToHost, no peer process, no Fake* device. amp_applet_test is a registered socket-free CTest (tests/tests.cmake:6333-6351).
Blockers
None.
Nits (non-blocking)
- The second assertion in the new test cannot fail — see the inline comment on
tests/amp_applet_test.cpp:473-476. The caption assertion is a real regression guard; the width one has ~2× slack built into the geometry and so does not pin the seed-width change it sits next to. - The seed
qMax()is documentation, not arithmetic that can bind — see the inline comment onsrc/gui/AmpApplet.cpp:619-621. Correct and worth keeping; the comment could just say so outright. m_stbyKey's caption istr("STBY")while the MEffA key isQStringLiteral("MEffA"). Consistent with the rail button and defensible for a vendor feature name, so I'd leave it — but it does meanseedMetrics.horizontalAdvance(m_meffaKey->text())is measuring an untranslated string next to a translated one.
What I tried to break
- "The name fits inside the letterbox the key already takes, so abbreviating it bought no width." This is the load-bearing claim and it holds.
panelKeySize()(AccessoryPanelWidgets.cpp:57-61) returnsmax(round(h * 16/9), round(seed * scale))withh = round(kKeyDesignHeight * scale)andkKeyDesignHeight = 44— a 78 px floor at scale 1.0. "MEffA" atkKeyFontDesignPx = 13measures ~40 px, so seed ≈ 58 withkKeyPaddingDesignPx = 18, against 51 for "STBY". Both are under 78, so the widened seed changes no key's size and the panel'sminimumSizeHint()(which goes throughpanelMinimumSize(kDesignWidth, natural), not the keys) is untouched. Reasoned from the constants, not measured at runtime. - The widened seed hitting the other key.
applyKeySize()feedsm_keySeedWidthto bothm_stbyKeyandm_meffaKey(AmpApplet.cpp:882,888), so a seed that bound would silently widen STBY too and squeeze the twoAccessoryPortRowstrips insidekDesignWidth = 420. It cannot: both terms of theqMaxscale linearly by the samescale, so the 78 : 58 ratio is fixed at every scale includingkPanelMinScale. The port row is unchanged at any size. - Existing assertions that the caption or the geometry would break.
pgxl_panel_test.cppselects keys byaccessibleName().contains("STBY")(:398) — the MEffA key's accessible name is"MEffA on — idle in class AAB", which contains noSTBYsubstring, so the selector still resolves to the right key. Its only width assertion (:269) is the fan key's squareness, which this diff does not touch.grep -rn 'MEffA' -i tests/finds no assertion on the old"ME"text. - Whether the new test can fail at all.
report()incrementsg_failedandmain()returns non-zero (amp_applet_test.cpp:29-37,727-730), the test is wired intomain(), and the not-found path reportsfalserather than returning silently — so it is not a vacuous pass. Reverting the caption to"ME"turns the first assertion red. Reverting the seed change does not turn anything red; that is the nit above. - Lifetime and harness fit.
setFloating(true)schedulesQTimer::singleShot(0, this, …)forcalibrateNaturalHeight(); the applet is stack-local and the context-object overload auto-disconnects on destruction, so the deferred lambda cannot fire on a deadthis. The test is the firstsetFloatinginamp_applet_test.cpp, butpgxl_panel_test.cpp:207already floats anAmpApplettoday, so the path is exercised.main()forcesQT_QPA_PLATFORM=offscreenand usesTestSettingsProfile, and the target already carries${AETHER_SETTINGS_SOURCES},AccessoryPanelWidgets.cppandAUTOMOC ON— sofindChildren<PanelKey*>()links and the CMake/settings contract is satisfied. - What I could not check. I am headless: no build, no test run, no GUI. Every statement above is reasoned from the code in
/tmp/aetherclaude/pr-5984, not reproduced at runtime. In particular I could not confirm the new test passes — all 5 checks ona51830c1are green, but.github/ci-test-gate.txtis frozen andamp_applet_testis not in it, so noctest -Rstep on this PR executes it. The Linuxbuildjob does build the full tree (ci.yml:274), so green does establish that it compiles; execution happens post-merge infull-suite.yml. Also unchecked: how "MEffA" actually reads atkPanelMinScaleon a real display, which only eyes can settle. - Preference vs fix. This modifies existing UI, so I classified it: the reporter filed it as a defect in the label, the triage comment showed the abbreviation's stated justification ("one key's width") was arithmetically false, and the in-tree comment at
:1366already asserted the behaviour this PR implements. That is an authority to point at, so it is a fix, not a smuggled preference. No default value, keyboard/mouse behaviour or confirmation changed.
One maintainer note rather than a finding: #5940 still carries the maintainer-review label and no maintainer has ruled in the thread — the scope split here comes from the triage bot's comment and your own claim. I found no sentence in GOVERNANCE.md or CONTRIBUTING.md that makes that label merge-gating, so it is a convention at most; flagging it only so the call is visibly the maintainer's.
Recommendation
Approve with nits. The change is one caption plus a defensive seed measurement, the geometry claim is provable from the constants rather than hopeful, the scope is exactly what was claimed in the issue thread, and no existing assertion or sibling surface is disturbed. Next step: optionally tighten the new test's width assertion (or drop it and keep the caption check), then a maintainer merge — the fan key and the vendor-style label stay on #5940 for their own change, which is the right split given the port row's width budget. Thanks for claiming this in the thread first and for keeping the diff to the part with no layout risk.
🤖 aethersdr-agent · cost: $7.5656 · model: claude-opus-5
Review follow-up on aethersdr#5984. - amp_applet_test: remove the width assertion from testMeffaKeyCarriesTheFullName. The 16:9 floor is about twice the caption at every scale, so it could not fail, and it stayed green with the seed change reverted. The caption check stays: it fails on "ME" (re-checked on this commit). The now-unused <QFontMetrics> include goes with it. - AmpApplet.cpp: the seed comment now says plainly that nothing binds today, and that the measurement over both captions guards a later retune of the font or the aspect. Refs aethersdr#5940 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#6036) **Merge order: #6008 and #6033 must be merged before this PR.** This branch is built on #6008 and merges in #6033 (with the conflicts resolved), so it carries their commits until they merge; it will then be rebased onto `main`. #6030 has merged. **Merge order for the PGXL / 4O3A PRs** (the same map is in each of them; #6030 has merged): ``` Stage 1: #6008 authenticate TGXL, PGXL and Antenna Genius Stage 2 (either order, after what each needs): #6027 Peripherals settings UI needs #6008 #6033 PGXL drain current (ID meter) needs #6008 Stage 3: #6036 PGXL docked layout needs #6008 and #6033 ``` **How to review:** this PR's own work is [`b4dcd8c9`](b4dcd8c) (the layout change) and [`4cc897c2`](4cc897c2) (the review findings: a neutral OPERATE placeholder with an accessible name, the temperature column measured in the drawn font, and two test fixes). `1b0de46e` merges #6030 in, and `3827ffea`, `4ef42eae` and `2186ef8e` merge #6033 in (the last also brings #6008's hostname-bound credential); open them only to check the resolutions. Everything else in the diff is carried from #6008, #6030 and #6033, which are reviewed in their own PRs. ## Summary Fixes #5903. Fixes #4885. #6008 puts the connection indicator on its own line and provides the direct connection that shows all of the readings and controls. In the docked (rail) view, the PGXL panel clipped its controls and readings: the 260 px rail had one row holding a stacked column of readings plus the MEffA, Fan and OPERATE controls. This PR gives the controls and the readings their own lines, and keeps the panel's shape whether the amplifier is offline or connected. Docked, the panel now reads: ``` [ MEffA ] [ Fan: Standard ] [ OPERATE ] PA 89.9 F Vac 122 V HL 89.2 F Vdd 51.9 V ● DIRECT ``` **Changes** - **Controls.** MEffA, Fan and OPERATE have the row to themselves and share its width, at one height (22 px). MEffA keeps its three state colours but takes OPERATE's 10 px bold lettering. - **Readings in a 2×2 grid**, in both presentations: PA over HL, and Vac over Vdd in a second column 10 px to the right. Docked, the connection indicator ends the HL/Vdd row; expanded, it keeps its own line below. Each is its own readout (`label value unit`) with its own screen-reader name. - **Vac and Vdd never move.** A grid column is as wide as its widest cell, so they start at one x; and the temperature column is held at the width of its widest possible reading, measured in the face it is drawn in, so they also stay put as the temperatures change. Fixed-length text alone was not enough: where no true fixed-width face is installed the fallback is proportional, and `9.9` and `106.8` differ in width. - **Temperature units.** PA and HL are both clickable, and a click on either switches both between Celsius and Fahrenheit and saves the choice, as the single readout did. HL shows `HL —` in grey until the direct connection sends a value, rather than disappearing, so the rows never move. The hover outline is gone (the pointing-hand cursor already says it can be clicked); the keyboard-focus outline stays. - **The connection indicator shares Vdd's line exactly.** Docked, it takes Vdd's height, so both centre the same way. At one pixel shorter, centring it needed a half-pixel offset that widget geometry rounds away, which lifted its text above Vdd's. - **Padding.** Every readout keeps a fixed length, but the spare spaces go at the end instead of in front of the number (`HL 89.8`, not `HL 89.8`). The temperature readouts have no left padding, so they line up with the controls above them. - **Spacing.** Docked, an 8 px pad separates the controls from the Id gauge, and the readings sit with equal space above and below. - **Fan labels** are spelled out: `Fan: Standard`, `Fan: Contest`, `Fan: Broadcast`. The pull-down reserves room for the longest. - **OPERATE's colours come from the theme.** It used a hardcoded green (`#6030`) under the theme's primary text colour, which is near-black under Default Light and unreadable there (#5903, item 4). It now uses the lit MEffA key's tokens, so the dark theme looks as before and the light theme gets its own green with white text. - **Before the amplifier reports.** The docked controls stay in place instead of hiding: MEffA is grey and disabled, OPERATE reads `—` and is grey and disabled (with the accessible name "Amplifier state not reported"), and Fan shows `Fan: —`, disabled. None of them claims a state. Clearing the fan selection is done with signals blocked, so it never sends a fan-mode command. PA and HL are grey until a reading arrives, like Vac and Vdd. The expanded panel's keys still wait for a state before appearing. **Maintainer rulings:** accepted as built. That covers the expanded (floating) panel's 2×2 grid, the removed hover outline, the right-justified readouts, and showing the controls disabled before the amplifier reports instead of hiding them. On a station without a direct PGXL connection, MEffA and fan stay dimmed, and their descriptions say they need a direct PGXL connection. **Against #5903's triage:** items 1 (controls on their own line), 2 (the rail MEffA button at 10 px) and 4 (OPERATE contrast) are done as proposed. Item 3 (the fan pull-down too wide when docked) is solved the other way: the controls share the full row, so the full labels fit, and they are spelled out rather than shortened to Std/Bcast. The docked-width test it asks for is added (below). **#4885** was left open until the docked `Fan: Contest` label could be checked on a current build: it reads in full here, and the new test checks all three fan labels at the rail's width. **Merging with #6030, #6033 and #5984:** #6030 is merged into this branch (commit 1b0de46), because the two conflicted in 10 places across `AmpApplet.cpp`, `AmpApplet.h` and `tests/amp_applet_test.cpp`: #6030 renames the temperature setters and members (`setTemp` → `setPaHeatsinkTemp`, `m_hasTempA` → `m_hasPaHeatsinkTemp`, …) and changes `updateTempLabel` and `setDirectConnected`, all code this PR restructures into two readouts. The resolution keeps this PR's structure with #6030's names and behaviour: HL clears (and turns grey) when the direct connection drops, a late HL write is ignored, and each readout's tooltip names only its own label. #6030's fallback test is rewritten for the two readouts and now also checks the grey tone. #6033 is merged in too (`3827ffea`, `4ef42eae`, `2186ef8e`). It deletes the amplifier-status relay handler and `AmpApplet::setMeff`, and adds `setRadioPaHeatsinkTemp` and `setRadioDrainCurrent` beside the PA setter this PR restructures, so it conflicted here in three files. The resolution keeps this PR's two-readout layout and tooltips, takes #6033's deletions and evidence comment, and keeps the grey-to-live style change on the PGXL's own first PA reading. Its single-button tooltip assertions are dropped in favour of this PR's per-readout ones. #5984 (spelling out MEffA on the expanded panel's key) edits panel-key code this PR leaves alone. ## Constitution principle honored Principle XI (Fixes Are Demonstrated). `pgxl_panel_test` lays the panel out at the rail's real width (260 px) and checks that no control or readout is narrower than its text needs, and `testDockedCaptionsFitTheRail` repeats that for every caption the controls can show. Both fail against the old layout. The layout was also checked in the running app on the hardware below, offline and connected. ## Test plan - [x] Local build passes - [x] Behavior verified on a real radio: FLEX-8600 (SmartSDR 4.2.20.41343) with a PowerGeniusXL on firmware 3.9.8. Docked, offline and connected with the direct connection up; `Fan: Broadcast`, the longest label, fits beside MEffA and OPERATE. - [x] Tests pass: `pgxl_panel_test`, `amp_applet_test`, `pgxl_direct_protocol_test`, `amp_model_test`, `peripheral_auth_handshake_test`, `health_applet_test` - [x] Test changes: - `pgxl_panel_test`: in both presentations, PA sits over HL and Vac over Vdd, with Vac and Vdd at one x. Docked at 260 px, the indicator ends the HL/Vdd row at the right edge, at Vdd's height and top edge; nothing is narrower than its text; the controls share one height; the space above and below the readings matches; the controls clear the Id gauge. Expanded keeps the indicator on its own line below, and the layout survives docked → expanded → docked. - `amp_applet_test`: new `testDockedCaptionsFitTheRail` shows every caption the rail's controls can take (each operate state word, all three fan labels, all three MEffA states, and the before-report placeholders) at 260 and 300 px, and checks each control is at least as wide as its caption needs. It compares against the control's own size hint, so a CI host that falls back to other fonts cannot fail it on a caption that fits. Against the old layout it fails at both widths: the fan pull-down got 70 px of the 130 its label needs. New `testVoltagesStayAlignedWithTheTemperatures` runs PA and HL through 9.9, 106.8, 100.4, 120.0 and -5.0 and checks that Vac and Vdd stay at one x and never move; without the pinned column width they moved by up to 7 px. `testDualSensorToggle` checks that a click on either temperature switches both and that the unit chosen from HL is saved. Also the new readout text; fixed length across digit changes; the before-state controls are shown but disabled with no mode; clearing the fan selection sends no command; the PA placeholder tone. - [x] Reproduction steps documented in the linked issues ## Checklist - [x] Commits are signed (`docs/COMMIT-SIGNING.md`) - [x] No new flat-key `AppSettings` calls - [x] Code is clean-room - [x] All meter UI uses `MeterSmoother` (no meter changed) - [x] Documentation updated if user-visible behavior changed (no `docs/` page describes this panel) - [x] Security-sensitive changes reference a GHSA if applicable (not applicable) 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Review follow-up (2026-10-03) - The column-pin test forces a proportional face on the temperature buttons, drives 9.9 → 106.8, and asserts the column minimum covers the widest reading plus the border. With the `setColumnMinimumWidth(0, ...)` line removed it fails. - **Justification (see #5983):** only the numeric field is right-justified (`label + rightJustified(value, 5) + unit`), so neither the unit nor the number's right edge moves as the value changes. - When there is no direct connection, the MEffA and fan descriptions say they need a direct PGXL connection. - OPERATE gets an accessible description while disabled; Vac/Vdd say "not reported" before the first reading; each readout's name change is announced once (`setAccessibleName` posts `NameChanged` itself); with a direct connection and no fan mode yet, the fan pull-down says the amplifier has not reported one; `placeReadings()` skips re-flow when nothing changed. - Comments describe current behaviour; the caption check uses the combo's edit-field rect; placeholder-tone checks compare against the resolved disabled colour. - **Known:** `pgxl_panel_test` fails at `:553-554` (`source->height() == vdd->height()`) in my sandbox, where the theme resources fail to load. It fails identically at the previous head `f5eafa06`, so it predates this change. It passes for you per your review. - Merged #6033's branch. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Jeremy [KK7GWY] <kk7gwy@aethersdr.com>
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit: Partial, as explicitly scoped. At f8f09e90d5143ff955f0d3b0ad346761873b6e50, this implements #5940's full MEffA caption and measures the shared letterbox seed over both captions. It deliberately leaves the fan caption and panel height unchanged, so Refs is correct. The separate operating-panel width shortage is disclosed in #5983.
Scope
| File/group | Claimed? | Verdict |
|---|---|---|
src/gui/AmpApplet.cpp |
Yes: caption, shared seed width, related comments | Within the stated subset of #5940; the caption remains Jeremy's visual-design choice |
tests/amp_applet_test.cpp |
Yes: actual panel and rail caption agreement | Relevant, registered regression coverage |
Both authored commits concern this change/test. No unrelated behavior, persistence, wire command, dependency, or changelog change was found; the scope checklist holds.
Blockers
None found.
Non-blocking nits / maintainer decision
- Jeremy: accept the caption change independently of #5983's layout decision.
src/gui/AmpApplet.cpp:597changes an existing presentation. The issue requests it, but the PR correctly leaves the visual choice to the maintainer. This review does not settle the separate fan/height requests or claim the longer text fits every desktop font. tests/amp_applet_test.cpp:459adds an unbraced conditional, contrary to AGENTS.md's braces convention. The mechanical replacement is:if (k->accessibleName().startsWith(QStringLiteral("MEffA"))) { key = k; }
src/gui/AmpApplet.cpp:594retells why the old abbreviation was chosen; keep the present invariant (same caption/letterbox as the rail control) and leave the history in the PR body.
Verification and attacks
- Built and ran the exact-head
amp_applet_teston Linux, Qt 6.11.2, offscreen, with its isolated test profile: all checks passed. No socket-owning changed test was found; the test never connects to an amplifier. - Mutation: changed only the production constructor caption back to
ME, rebuilt, and got exit 1 with[FAIL] the panel key reads MEffA, as the rail button does ME. Restored the source and reran successfully. The new regression genuinely detects the change. - Existing tests exercised the three MEffA states, unavailable control, operator toggle bit, and meter-source ownership; they passed. Source inspection confirms the command wiring and fonts are unchanged and the seed uses the larger caption measurement.
- Boundary: no live amplifier or operator application was touched. The demo has no PGXL session to verify, so I used the production-widget test rather than claiming a demo bridge verified PGXL behavior. Real-device operation and platform/font-specific clipping remain the author's evidence, not independently reproduced here. No model-invocable
code-reviewskill was available; this was a manual audit plus local tests. - Required CI checks succeeded for this head, but the earliest PR run was created 2026-09-27 20:27:26 UTC, before current main
8df6db2f(2026-10-04 18:26:40 UTC). Thosepull_requestruns tested an older merge result; protection isstrict: false. This is not a claim that the complete current-main suite passes.
Recommendation: Needs maintainer decision on the explicitly requested caption; no code blocker found. If Jeremy accepts that presentation, the focused regression and mutation evidence support landing after the normal current integration checks. Keep #5940 open for its deferred requests and #5983 for the width budget.
— Jeremy KK7GWY + Codex
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Independent re-review at head f8f09e90d5143ff955f0d3b0ad346761873b6e50. Same head as the earlier ten9876 + Codex review. I read that review and re-checked its claims rather than adopting them.
Issue fit: partial, as explicitly scoped. This delivers #5940's items 1, 2 and 4: the full "MEffA" caption, a seed measured over both letterbox captions, and the test. The fan caption and the panel height stay on #5940, and Refs (not Closes) is correct. The panel key now says what the rail button (AmpApplet.cpp:481) and the in-tree comment already said it should, so this is a fix with an authority behind it, not a restyle.
Scope
| File | Change | Claimed? | Verdict |
|---|---|---|---|
src/gui/AmpApplet.cpp:594-597 |
"ME" → "MEffA" |
Yes | In scope |
src/gui/AmpApplet.cpp:612-623 |
Seed = max(STBY, MEffA) advance + padding | Yes | In scope (defensive; does not bind today) |
src/gui/AmpApplet.cpp:804,887 |
Comment wording | Follows from the caption | In scope |
tests/amp_applet_test.cpp |
testMeffaKeyCarriesTheFullName |
Yes | In scope, registered, socket-free |
Everything in the diff is explained by the issue. Both commits are on-topic and signed. There is no CHANGELOG entry and no settings, capability or protocol surface.
Blockers
- This no longer merges with
main.git merge-tree origin/main <head>reports a conflict intests/amp_applet_test.cpp:4-10. Since this branch was cut,mainadded#include "models/AmpModel.h"and#include "core/backends/AmpDelta.h"at the same spot as this PR's#include "gui/AccessoryPanelWidgets.h". The fix is mechanical: keep all three includes. I checked thatmain'sAmpAppletstill builds the key with"ME"(main:src/gui/AmpApplet.cpp:623) and still sizes it through the samepanelKeySize(h, m_keySeedWidth, scale)seed. So the PR's sizing argument survives #6036/#6133, and only the include line needs resolving.
Nits (non-blocking)
tests/amp_applet_test.cpp:459: the one-lineif (...) key = k;is unbraced. AGENTS.md uses braces. Drop-in:if (k->accessibleName().startsWith(QStringLiteral("MEffA"))) { key = k; }
src/gui/AmpApplet.cpp:594-596: "abbreviating it bought no width" retells why the old caption existed. The present-tense invariant is enough ("same caption as the rail button; fits the shared letterbox"). Leave the history in the PR body.- The seed-width change at
:619-622cannot be pinned by a test today, because the 16:9 floor (78 px at scale 1.0) out-binds both captions (~51 px STBY, ~58 px MEffA with padding). That is honest as written. Just don't read the new test as coverage for the seed.
Verification and what I tried to break
- Built the exact head (Linux, GCC 16, Qt 6 offscreen).
amp_applet_testandpgxl_panel_testboth pass. The new check reports[ OK ] the panel key reads MEffA, as the rail button does MEffA. - Mutation: I put the production caption back to
"ME"and rebuilt. Result:[FAIL] the panel key reads MEffA, as the rail button does ME. I then restored the source (sha256 matchesgit show HEAD:, touched, rebuilt) and it passed again. The test pins the defect it claims to pin. - Selector collision:
pgxl_panel_test.cpp:398picks keys byaccessibleName().contains("STBY"). The MEffA key's accessible name has no "STBY", andpgxl_panel_testpasses at this head. - Both letterbox keys share the seed (
applyKeySize), so a binding seed would widen STBY too. Both terms of theqMaxscale linearly by the same factor, so the 78 : 58 ratio holds at every scale includingkPanelMinScale, and no key changes size. - Not driven in the app: the demo simulator has no PGXL, so the amp panel can't be reached on the bridge. Real-display clipping at the panel's minimum size (#5983) is the author's hardware evidence plus the disclosure, and I did not reproduce it.
- CI: the checks on this head ran 2026-09-27, before
main's current tip. Green describes a merge that no longer exists, which the conflict above confirms.
Recommendation: request changes for the rebase only. The code itself is ready, and the caption is the presentation choice #5940 asked for. Resolve the include conflict, and it is a maintainer merge.
— Jeremy KK7GWY + Claude
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Issue fit
Partial, as the PR says. Reviewed f8f09e90d5143ff955f0d3b0ad346761873b6e50. #5940 asks to spell the PGXL key "MEffA", show fan speed as "Fan: S", and mimic the vendor caption above the PWR band (and tighten panel height). This delivers the caption, the shared letterbox seed, and the regression. Refs is correct. Fan caption and height stay open.
Yes, "MEffA" fits the tuner applet's packed key row. The amp keys copy that letterbox on purpose (kKeyDesignHeight 44, 16:9, 13 px bold). Measured here on macOS with .AppleSystemUIFont and the panel-key stylesheet:
| Caption | 13 px bold | 10 px bold (scale 0.8) |
|---|---|---|
| MEffA | 39 px | 32 px |
| TUNE | 36 px | 29 px |
| STBY / STOP | 35 px | 28 px |
| ME | 20 px | 16 px |
The seed is that advance plus kKeyPaddingDesignPx (18). Bold "MEffA" makes 57, under the 78 px floor, so panelKeySize stays on the aspect term and no key grows. A styled 78×44 key gives a 76 px contents rect (37 px spare). At the 0.8 box (62×35) the spare is 28 px. The tuner row itself is the tight one: three dials plus three letterbox keys add up to about 422 px inside the 420 design width, and about 342 px inside the 336 px floor. That shortfall is a few pixels across the whole row. The word still has about 30 px of spare, and the stretch between the dials and the keys is what keeps a caption from being the thing that gives.
It does not fit the square fan key. At 44×44 the same 13 px word has 3 px of spare, and the fan face is 17 px, where the advance is 47 against a 42 px contents rect.
The amp port row is a different squeeze, already filed as #5983. While the A/B strips are up, the author measured the letterbox keys at 68 px given / 78 asked (design) and 53 / 62 (minimum). On this Mac a 53×35 styled key still has 19 px of spare for "MEffA". The #5983 desktop screenshots show the caption cut off at that minimum; that clip is real on that font and is wider than "ME", and it is a layout decision, not a reason to keep the two-letter caption. Spelling the key out does not change the key's size.
Visual design stays the maintainer's (GOVERNANCE.md: visual design is maintainer authority). The spelling is the change #5940 asked for, and it fits the shared letterbox.
Scope
| File | Change | Verdict |
|---|---|---|
src/gui/AmpApplet.cpp |
Caption ME → MEffA; seed is max(STBY, MEffA); comments |
In the claimed subset. Auto-merges onto current origin/main (4249b71). |
tests/amp_applet_test.cpp |
testMeffaKeyCarriesTheFullName |
In scope, already-registered, socket-free. Conflicts with main. |
No settings, protocol, changelog, or dependency change. Both commits are this caption and its test. AmpApplet.cpp on current main still builds the key as "ME" and still sizes it with panelKeySize.
Blocker
git merge-tree origin/main f8f09e90 conflicts only in tests/amp_applet_test.cpp. main added #include "core/ThemeManager.h", #include "models/AmpModel.h", and #include "core/backends/AmpDelta.h" beside this PR's #include "gui/AccessoryPanelWidgets.h". Keep all of them. AmpApplet.cpp auto-merges.
Required checks on this head succeeded (build, check-macos, check-windows, Static checks, run 36348093813 / 36348093905, 2026-09-27). They describe the merge with the base from that day (bf97bc7f), which is no longer main.
Nits
- Unbraced
ifin the new test. AGENTS.md wants braces. - The new caption comment retells why "ME" existed. The present rule is enough: same caption as the rail button, and it fits the shared letterbox.
- The seed comment says a too-wide caption would elide.
PanelKeyis aQPushButtonwhosesizeHintis the target; the style clips. The guard is still the right one. - The open thread on the seed comment (line 622) matches the wording already committed in
f8f09e90. The outdated width-assertion thread was dropped in that commit. Both can be resolved.
Verification
- Full diff read (2 files, +36/−7). Caption, seed, and
setMeffaaccessible-name path traced: aftersetMeffa("STANDBY")the key's accessible name is"MEffA on — idle in class AAB", so the test'sstartsWith("MEffA")finds it, andtext()stays"MEffA". - Fit numbers above are from a standalone Qt 6 probe (Fusion offscreen and macOS Cocoa), system font, the panel-key stylesheet,
SE_PushButtonContents. No AetherSDR configure oramp_applet_testrun in this review. CI on this SHA is the suite evidence. No app launch. The #5983 clip was not re-rendered here. - Changed test does not bind or listen.
PgxlConnection'sQTcpSocketis linked into the existing target and is not constructed by this case.
Recommendation: request changes for the rebase only. After the include conflict is resolved, the caption fits the tuner letterbox and is ready for a maintainer merge. Keep #5940 open for the fan caption and the height, and #5983 for the port-row width.
Non-blocking follow-up
A resize of the floating PGXL panel to its minimum with the port strips visible, asserting each letterbox key's laid-out width is at least the bold caption advance, would pin #5983. That is an operator follow-up, not a merge gate for this caption. The tuner row is a different layout (stretch between dials and keys) and this PR does not touch it.
The popped-out PGXL panel labelled its MEffA key "ME" while the docked rail button reads "MEffA". The key takes the same 16:9 letterbox as STBY, and "MEffA" fits that box, so the key does not grow. The letterbox seed is measured over both captions. Nothing binds today: the 16:9 floor is wider than either caption, and the measurement keeps that true if the font or the aspect is retuned. Refs aethersdr#5940 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Patrick Jensen <patjensen@gmail.com>
f8f09e9 to
362897f
Compare
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Rebased onto current main (4249b71) and the caption test is removed. The change is the panel key text ME → MEffA plus the shared letterbox seed, in src/gui/AmpApplet.cpp only. The word fits the existing 16:9 key, so nothing else moves.
Approving this head for merge.
|
Rebased onto current Head: |
The include conflict is gone: the caption test was removed and the branch was rebased onto current main. The remaining change is the MEffA label in AmpApplet.cpp, approved on 362897f.
3 participants
Summary
Refs #5940. It does not close it: #5940 also asks for a "Fan: S" caption and a shorter panel. Both are left for the maintainer (see Scope below), so the issue stays open for that call.
The popped-out PGXL panel labelled its MEffA key "ME", while the docked rail button for the same control reads "MEffA". The comment behind the abbreviation said it kept the key to one key's width. It did not need to: the MEffA key takes the same letterbox as STBY, whose 16:9 floor is 78 × 44 at scale 1.0, and "MEffA" fits inside it. The key keeps exactly the size it had.
Changes, all in
src/gui/AmpApplet.cpp, plus one test:"ME"to"MEffA", the same text as the rail button.m_keySeedWidth) is measured over both captions that use it (STBY and MEffA) instead of STBY alone. APanelKeynever grows to fit its text (sizeHint()returns its target size), so a caption wider than the seed would be cut off if the font or the aspect were ever retuned. Nothing binds today: the 16:9 floor is wider than either caption, so no key changes size. The measurement guards that retune.Scope is items 1, 2 and 4 of the triage comment on #5940. The fan key caption (item 3) and the panel height are not touched. The caption text is a visual choice, so it is the maintainer's call. The key's font is unchanged, at the 13 px caption face STBY uses.
One thing this PR makes more visible, filed separately as #5983. While the A/B port strips are on screen, the port row squeezes all three keys below the size they ask for, even at the design width (68 px given for a 78 px key at 420 wide). With a desktop's real fonts, at the panel's smallest size, the captions of MEffA, fan and STBY are cut off. That happens on
maintoo: the fan and STBY keys are untouched by this PR and clip the same way. But "MEffA" is wider than "ME", so on such a desktop it clips where "ME" still fitted. The fix belongs in the row layout, which is a maintainer decision, so it is in its own issue with measurements and pictures, not in this PR.Constitution principle honored
Principle VIII — Evidence Over Assertion. The old comment asserted that "ME" saved width. Measured, it saved none: the key is the same size with either caption (pictures below).
Test plan
Local build passes: full
cmake --build build, 0 errors, Linux Mint 22.3, Qt 6.8.3, branch based onbf97bc7f.Behavior verified on a real radio. FLEX-8600, firmware 4.2.20.41343, with a PGXL, operated by G6PWY. PGXL panel popped out:
Automation bridge, on the same live session with the amplifier in standby:
dumpTreereports the panel key withvalue"MEffA", geometry 62 × 35 (its full target size at the panel's minimum scale, 0.8), and accessible name "MEffA on — idle in class AAB".grab AmpApplet:Before and after, the same panel rendered offscreen at 420 × 300 by the
amp_applet_testharness, onmainand on this branch. Every key is the same size. (The pale background is the offscreen harness running without a theme file.)main("ME"):This branch ("MEffA"):
Existing tests pass: the full local suite passes, 562 of 562 (6 skip themselves by design:
crdv_quarantined_test,app_settings_safety_explicit-profile-path-isolation,weather_radar_texture_gl_test,range_slider_a11y_test,relay_bar_a11y_test,tgxl_panel_widgets_test). CI result to follow.amp_applet_test:testMeffaKeyCarriesTheFullName. It checks the panel key reads "MEffA", matching the rail button. (A width check that could not fail was removed inf8f09e90after review.)"ME"restored, "the panel key reads MEffA, as the rail button does" fails; with the change, it passes.full-suite.ymlandsanitizers.yml. No change to the per-PR gate.Reproduction steps documented: pop the PGXL applet out with a PGXL on the direct connection. On
mainthe key reads "ME"; on this branch it reads "MEffA".Static checks, run locally as
static-checks.ymlruns them:--strict/--checktools all pass.audit_colours.py --strictagainst the merge base: +0 on all three counts.test_check_capability_records.pyandtest_gen_seam_probe_table.pypass.check_a11y.pyonsrc/gui/AmpApplet.cpp: 0 findings.Checklist
docs/COMMIT-SIGNING.md)AppSettingscalls (Principle V)MeterSmoother: no meter UI touchedCHANGELOG.mdentry, as AGENTS.md requires.Written and tested by Claude Code. Hardware test: G6PWY, FLEX-8600 with a PGXL.
🤖 Generated with Claude Code