Issues on Android-dev.1137 - #788
Conversation
Issue #782 reported two UX problems that share a root cause of a hidden or stale UI affordance: 1. Tapping the waterfall/spectrum moved the blue tuning cursor but the red TX-bandwidth markers didn't follow, and were sometimes offset from the cursor. Both views computed the tap frequency inside onDraw() and read it back from getFreq_hz(), so an ACTION_UP that committed the base frequency read the previous frame's value. Extract SpectrumTouchMath so ColumnarView and WaterfallView share the same pixel<->Hz math, compute freq_hz eagerly in setTouch_x, draw the blue cursor at the pixel that maps back from that frequency (so the red +/- 25 Hz markers bracket it symmetrically), and have WaterfallScreen feed the touched frequency to the TX markers during the drag so the reds follow the blue live. 2. The CQ options bottom sheet (opened via long-press / "more" chevron) had no visible close affordance. Scrim tap, drag handle, and hardware Back all dismissed, but a tester who never discovered any of those saw no way out. Add an explicit close icon to FT8AFBottomSheet's header so every sheet that uses it — CQ options, band picker, hound setup, etc. — gets a plainly tappable Close. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Edge taps can commit invalid TX frequencies, while the new marker path and close target also need coverage and accessibility corrections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes waterfall cursor/TX-marker alignment and adds a visible close control to shared bottom sheets.
Changes:
- Centralizes spectrum pixel/frequency conversion and updates markers live.
- Adds an accessible label and close icon to bottom sheets.
- Adds geometry and close-button tests.
File summaries
| File | Description |
|---|---|
SpectrumTouchMath.java |
Adds shared coordinate conversion. |
ColumnarView.java |
Uses eager touch-frequency calculation. |
WaterfallView.java |
Aligns cursor and TX markers. |
WaterfallScreen.kt |
Displays markers at the touched frequency. |
FT8AFBottomSheet.kt |
Adds the close control. |
strings_compose.xml |
Adds its accessibility label. |
SpectrumTouchMathTest.java |
Tests spectrum geometry. |
FT8AFBottomSheetCloseButtonTest.kt |
Tests close rendering and dismissal. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Both views suppress the blue cursor for the newly accepted left-edge touch.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
…licts) Both hunks were additive: dev added the redesigned TX-strip strings and the ruler-tick helpers (rulerTicks/RulerTick) at the same spots where this PR added the sheet_close string and displayTxFrequencyHz. Kept both sides in each file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnrELYBhBMQ9RAmQ9cQoyT
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #788 +/- ##
============================================
+ Coverage 42.34% 42.40% +0.06%
- Complexity 226 227 +1
============================================
Files 267 267
Lines 31834 31862 +28
Branches 3650 3649 -1
============================================
+ Hits 13479 13510 +31
+ Misses 18097 18087 -10
- Partials 258 265 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
A touch at exactly x == 0 is valid: touchToFreqHz clamps it to 100 Hz and the handlers commit it, but both views gated the blue cursor on touch_x > 0 as well, so the red markers moved to 100 Hz with no blue line between them. Gate on the selected frequency alone via a shared SpectrumTouchMath.hasTapCursor helper (the cleared state is setTouch_x(-1), which maps to -1), with tests for the edge touch and the cleared/unlaid states. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnrELYBhBMQ9RAmQ9cQoyT
There was a problem hiding this comment.
🟡 Changes recommended
Rejected off-view drags retain stale marker state, and the critical view-setter behavior lacks direct tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
#788) - Both AndroidView listeners only forwarded a touch when the resolved frequency was positive, so the -1 an off-view drag resolves to never reached touchedFreqHz and the red TX markers stayed parked at the last on-view column until the timeout. Extract the routing into dispatchSpectrumTouch(): DOWN/MOVE always forward the frequency (the view has already hidden its cursor for that event), UP still commits only a valid one. Four pure tests in WaterfallScreenTest. - Add SpectrumViewTouchTest (Robolectric): lays out ColumnarView and WaterfallView for real and asserts setTouch_x updates getFreq_hz() immediately with no draw pass — the #782 regression on the actual views, plus the left-edge clamp, the -1 clear/off-view cases and the unlaid view. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EnrELYBhBMQ9RAmQ9cQoyT
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are focused and thoroughly tested; only the Hound setup claim in the test instructions needs clarification.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
ft8af/app/src/main/kotlin/radio/ks3ckc/ft8af/ui/components/FT8AFBottomSheet.kt:134
- The PR’s test instructions say the Hound setup sheet inherits this close icon, but
HoundSetupSheet.kt:41renders anAlertDialograther thanFT8AFBottomSheet, so that step will show its existing Cancel button—not the new ✕. Either remove Hound setup from the propagation claim/test steps or migrate it if the uniform icon is intended.
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Re Copilot's suppressed comment on the latest review (no inline thread): the PR description claimed the Hound setup sheet inherits the new close icon, but |
Closes #782
Scoped to two of the issues in the report — the waterfall tap-cursor
misalignment and the missing back button on the CQ options / "more" sheet
— because they share a small, clearly-diagnosable root cause and can be
verified with unit tests. The remaining items (FT2/FT4 cycle length,
"waiting for messages" panel layout, decode list toggle, WfView CAT
reliability, radio-linked chip UX) touch broader subsystems and belong in
separate PRs.
What changed
Waterfall tap alignment (
ColumnarView.java,WaterfallView.java,WaterfallScreen.kt, newSpectrumTouchMath.java)Two views computed the tap frequency in their
onDraw()and read it backvia
getFreq_hz(). A tap handler that ransetTouch_x(x)and then readgetFreq_hz()onACTION_UPtherefore committed the previous frame'sfrequency — that's the "blue line not in the middle of the red ones" the
issue describes. The red TX bandwidth markers also lagged behind the blue
cursor during a drag because they were driven by the base frequency, which
only updated on release.
SpectrumTouchMathso both views share the pixel↔Hz math.freq_hzeagerly insetTouch_x, sogetFreq_hz()returns thefresh tap position.
freq_hzand thered markers at
freq ± halfBwin the same coordinate system, so theybracket the cursor symmetrically.
WaterfallScreen, feed the currently touched frequency tosetTxFrequencywhile a cursor is active, so the reds track the blue inreal time instead of snapping only on
ACTION_UP.CQ options / bottom sheet back button (
FT8AFBottomSheet.kt,strings_compose.xml)The "more" panel opened from Call CQ (and every other
FT8AFBottomSheet)had three dismissal paths — scrim tap, drag-handle drag, hardware Back —
but no visible tappable close, so a user who didn't discover any of those
saw no way out. Added an explicit close icon to the sheet header. All
existing sheets that use
FT8AFBottomSheet— the band & mode picker, CQoptions, frequency picker, hunt options, the QSO sheet, export log and the
park picker — inherit the affordance for free. (Hound setup is an
AlertDialog, not a bottom sheet, and keeps its own Cancel button.)Tests
SpectrumTouchMathTest— 10 pure-JVM cases covering the tap→Hz mapping,the inverse Hz→pixel mapping, invalid-input handling, and the
regression case that the red ± halfBw markers stay centered on the blue
tap column for a range of tap positions.
FT8AFBottomSheetCloseButtonTest— 2 Compose/Robolectric casesasserting the close icon renders and dismisses.
assembleDebugcompletes.How to test
waterfall canvas. Verify that the red TX-bandwidth markers move with
the blue tap cursor in real time and that the blue line sits exactly
between the two reds when you release.
sheet header dismisses it. Repeat for the band & mode picker and the
frequency picker sheets to confirm the affordance propagates.