Fix drawing lags and allow drawing dots to canvas #107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shubertm
temporarily deployed
to
Development
GitHub Actions
Inactive
WalkthroughTouch handling in NotesCanvas was refactored: the finishDrawing flag and related branching were removed. A DrawPath is created and dispatched to the ViewModel on ACTION_DOWN; MOVE emits quadTo updates; UP/CANCEL may append a final lineTo, then reset the path. invalidate() is always called. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Canvas as NotesCanvas
participant VM as ViewModel
User->>Canvas: Touch ACTION_DOWN (x,y)
activate Canvas
Canvas->>Canvas: create DrawPath(path, paint)
Canvas->>VM: onDrawPath(DrawPath)
Canvas->>Canvas: invalidate()
deactivate Canvas
User->>Canvas: Touch ACTION_MOVE (x,y)
activate Canvas
Canvas->>Canvas: path.quadTo(prev, mid)
Canvas->>Canvas: emit AbsQuadTo command, update currentX/Y
Canvas->>Canvas: invalidate()
deactivate Canvas
alt ACTION_UP or ACTION_CANCEL
User->>Canvas: Touch end (x,y)
activate Canvas
opt if end == last point
Canvas->>Canvas: append AbsLineTo(x,y)
end
Canvas->>Canvas: path.reset()
Canvas->>Canvas: invalidate()
deactivate Canvas
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
shubertm
temporarily deployed
to
Development
GitHub Actions
Inactive
shubertm
temporarily deployed
to
Development
GitHub Actions
Inactive
shubertm
temporarily deployed
to
Development
GitHub Actions
Inactive
shubertm
temporarily deployed
to
Development
GitHub Actions
Inactive
shubertm
temporarily deployed
to
Development
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/dev/arkbuilders/arkmemo/ui/views/NotesCanvas.kt(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/dev/arkbuilders/arkmemo/ui/views/NotesCanvas.kt (1)
app/src/main/java/dev/arkbuilders/arkmemo/graphics/SVG.kt (1)
addCommand(34-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
Comment on lines
+79
to
+88
| if (x == currentX && y == currentY) { | ||
| path.lineTo(x, y) | ||
| viewModel.svg().apply { | ||
| addCommand( | ||
| SVGCommand.AbsLineTo(x, y).apply { | ||
| paintColor = viewModel.paint.color.getStrokeColor() | ||
| brushSizeId = viewModel.paint.strokeWidth.getBrushSizeId() | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Dot detection needs touch-jitter tolerance.
ACTION_UP coordinates often drift by a few pixels even on a stationary tap, so this strict equality rarely fires. When it doesn’t, we skip the lineTo/SVG command and the dot still vanishes after reload—undercutting the PR goal. Please gate this branch with a small tolerance (e.g., a few px or scaledTouchSlop) instead of raw equality.
- if (x == currentX && y == currentY) {
+ if (
+ kotlin.math.abs(x - currentX) <= DOT_TOLERANCE_PX &&
+ kotlin.math.abs(y - currentY) <= DOT_TOLERANCE_PX
+ ) {Add an appropriate constant (tuned for density/touch slop) near the class definition, for example:
private companion object {
private const val DOT_TOLERANCE_PX = 4f
}🤖 Prompt for AI Agents
In app/src/main/java/dev/arkbuilders/arkmemo/ui/views/NotesCanvas.kt around
lines 79-88, the current dot-detection uses strict x == currentX && y ==
currentY which fails due to touch jitter; add a small tolerance constant near
the class definition (e.g., in a private companion object, either a fixed
DOT_TOLERANCE_PX or obtain ViewConfiguration.get(context).scaledTouchSlop
converted to pixels) and replace the equality check with a distance/tolerance
check (e.g., if (abs(x - currentX) <= DOT_TOLERANCE_PX && abs(y - currentY) <=
DOT_TOLERANCE_PX) or squared-distance comparison) so the touch-up branch fires
for near-identical coordinates and the SVG dot command is preserved.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Before
Screen_Recording_20250928_114125_251002125459.mp4
After
Screen_Recording_20250928_114453_251002125252.mp4