Keep switch hit rect inside its row#27
Conversation
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/input.c">
<violation number="1" location="src/input.c:1299">
P2: Switch touch-rect clamp uses `layout.height` instead of row height, so row-bound hit-area containment can fail in normal layouts.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Next time, avoid using default branch ( |
Sorry about that, it was my first PR. I will use a feature branch next time. |
There was a problem hiding this comment.
The layout.height vs row_height issue has been noted by @cubic-dev-ai already. Beyond that, two more concerns:
-
nextafterf()is papering over a deeper issue.in_rect()ininternal.h:510-511uses closed intervals (<=on both edges), so two adjacent rows sharing a boundary pixel both match. That's the actual root cause of boundary ambiguity. The right fix is to makein_rect()use a half-open interval[y, y+height)— i.e., changepos.y <= rect->y + rect->heighttopos.y < rect->y + rect->height(and same for x). Thennextafterf()becomes unnecessary, and the fix simplifies to a plain row-height clamp. -
Edge case: if
ctx->row_height(orlayout.heightin the current code) is very small or zero,nextafterf(y + h, y)can produce values wheretouch_bottom - touch_rect.yis negative, yielding a negative height. This is unlikely in practice but indicates thenextafterfapproach is fragile.
Suggested fix for the clamp (after addressing in_rect):
if (touch_rect.height > ctx->row_height) {
touch_rect.y = ctx->layout.y;
touch_rect.height = ctx->row_height;
}|
Use |
Adjacent rows shared the same boundary, which could route a switch click to the wrong row. Use half-open hit tests, clamp the switch hit rect to row_height, and update the field-tracking test for the new boundary semantics.
|
Thank @deantee for contributing! |
|
@deantee , By the way, you can take some screenshots, put them in the |
Clicks near the boundary between vertically adjacent switches can hit the wrong row because the expanded touch target overlaps the adjacent row. Keep the switch hit rect within the current row so the enabled switch toggles correctly without replaying the off-to-on animation.
recording.mp4
Closes #26
Summary by cubic
Clamp the expanded switch touch target to its row so edge taps hit the correct switch and don’t replay the off‑to‑on animation. Switch hit tests now use half‑open bounds so each row exclusively owns its edges; tests were updated for the new boundary.
Closes #26.
Written for commit 0b5c129. Summary will update on new commits.