feat: add pen icon when remote drawing#327
Conversation
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pencil badge variant and renderer, pre-renders and stores a pencil cursor image, introduces a controller visual-mode override, and wires drawing-mode events to update controller cursor visuals. ChangesPencil cursor support
Sequence Diagram(s)sequenceDiagram
participant DrawingModeEvent as UserEvent::DrawingMode
participant MainHandler as lib.rs Handler
participant CursorController
participant Controller as ControllerCursor
participant Renderer as ParticipantsManager/Renderer
DrawingModeEvent->>MainHandler: dispatch(DrawingMode)
MainHandler->>MainHandler: map DrawingMode -> Option<CursorMode>
MainHandler->>CursorController: set_controller_visual_mode(identity, visual_mode)
CursorController->>Controller: set_visual_mode_override(mode)
CursorController->>CursorController: update_cursors()
CursorController->>Controller: visual_mode()
Controller-->>CursorController: override or underlying mode
CursorController->>Renderer: set_cursor_mode(mode)
Renderer->>Renderer: draw cursor image using asset for mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/input/mouse.rs`:
- Around line 1042-1052: set_controller_visual_mode updates the controller
visual state via controllers_cursors and controller.set_visual_mode_override but
does not trigger a redraw, so the cursor badge can remain stale; after modifying
the controller (inside the if block or immediately after the loop) call the
appropriate redraw trigger on the UI surface (e.g. self.request_redraw() or
self.window.request_redraw()/self.queue_redraw(), depending on the available
API) so the cursor is repainted; reference set_controller_visual_mode,
controllers_cursors, and set_visual_mode_override when locating where to add the
redraw call.
In `@core/src/lib.rs`:
- Around line 1215-1229: The code incorrectly mutates pointer state when
drawing_mode is DrawingMode::Any; change the pointer update logic so
DrawingMode::Any is a no-op: compute visual_mode as currently done
(DrawingMode::Any -> None) but only call
cursor_controller.set_controller_pointer(...) for modes that explicitly set
pointer behavior (e.g., DrawingMode::Draw(_) -> true and
DrawingMode::ClickAnimation -> true, DrawingMode::Disabled -> false), and skip
calling set_controller_pointer for DrawingMode::Any; keep the subsequent call to
cursor_controller.set_controller_visual_mode(sid.as_str(), visual_mode) as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c4b0ef7-09d6-4bb0-b6df-38a6716cd4fb
📒 Files selected for processing (5)
core/src/graphics/cursor.rscore/src/graphics/participant.rscore/src/input/mouse.rscore/src/lib.rscore/src/utils/svg_renderer.rs
iparaskev
left a comment
iparaskev
left a comment
There was a problem hiding this comment.
@theodkp thanks for the PR. Generally looks good. One issue is that the pencil has offset from where the draw line ends.
We could either flip the pencil in the svg or maybe when the mode is pencil hardcode an offset in draw in cursor.rs.
| } | ||
|
|
||
| fn visual_mode(&self) -> CursorMode { | ||
| self.visual_mode_override.unwrap_or(self.mode) |
There was a problem hiding this comment.
I am fine with leaving this as is but I think we can do better. Instead of having two member for tracking the same thing we can just merge the visual mode with mode. This will need more refactoring as we have many functions who assume there are only two cursor modes. If you don't want to give it a go I am fine with leaving a todo comment here and we can improve later.
There was a problem hiding this comment.
In the implementation of set_drawing_mode we should also call set_cursor_mode.
|
@theodkp If the scope of this has grown and you don't have time to finish it, no worries, I'm happy to make the finishing touches and merge it. |
|
Yeah that would be great thanks! @iparaskev |
2 participants
closes #219
Summary by CodeRabbit
New Features
Improvements