镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Wrap cursor around viewport during G/R/S (#3255) - #4612

Open
TTyChud wants to merge 23 commits into
GraphiteEditor:masterfrom
TTyChud:cursor-wrap-grs-3255
Open

TTyChud wants to merge 23 commits into
GraphiteEditor:masterfrom
TTyChud:cursor-wrap-grs-3255

Conversation

@TTyChud

@TTyChud TTyChud commented Sep 26, 2026 •

Copy link
Copy Markdown

Closes #3255

Wrap cursor around viewport during G/R/S. While grabbing/rotating/scaling, hide OS cursor and show a Graphite fake that wraps within viewport bounds. Uses relative pointer-lock deltas for infinite drag on desktop and web. Works on Wayland where OS warp is not supported.

I have also add tests.

Like Blender GHOST_kGrabWrap.

web

cursor_wrap_web.mp4

Desktop

cursor_wrap_desktop.mp4

Hide the OS cursor and draw a Graphite cursor that wraps inside the viewport bounds while grabbing, rotating or scaling. Relative pointer-lock deltas drive the transform so the drag is unbounded on desktop and web, and the grab still works on Wayland where the OS pointer cannot be warped.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 15 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread frontend/src/utility-functions/input.ts Outdated
Comment thread frontend/src/utility-functions/viewports.ts Outdated
A locked pointer keeps reporting the frozen or warp-back OS location, and a lock held without a mouse button routes those reports down the UI path, so they still reached the editor as absolute moves alongside the locked deltas. Drop the event while the pointer is really locked, which leaves the absolute path live exactly when the locked deltas are not.

Also move the software cursor state out of a utility-functions module, which must not hold state, into a store so the drawn cursor and the hit-tested position share one source.
@TrueDoctor

Copy link
Copy Markdown
Member

Just a heads up this might unfortunately take a long time to get merged since it touches frontend code and @Keavon has been historically been pretty opinionated about frontend code style

@TTyChud

TTyChud commented Sep 26, 2026

Copy link
Copy Markdown
Author

Thanks for the context. While 4612 sits in the frontend queue — is there anything on the Rust side
you'd want me to work on?

@0HyperCube 0HyperCube left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The desktop implementation is very glitchy (gnome + wayland) when you start with a resize cursor):

desktop_glitches.mp4

Comment thread desktop/src/input.rs Outdated
modifiers: ModifiersState,
pointer_position: PhysicalPosition<f64>,
pointer_state: PointerState,
software_cursor: Option<(f64, f64)>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could more usefully be e.g. LogicalPosition or glam::DVec2.

Comment thread desktop/src/input.rs Outdated
matches!(self.pointer_state, PointerState::Locked { .. })
}

pub(crate) fn window_position(&self, x: f64, y: f64) -> Option<PhysicalPosition<f64>> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document what coordinate space you are transforming from.

Comment thread desktop/src/input.rs Outdated
Comment on lines +100 to +106
// Software cursor shown while G/R/S wraps the pointer around the viewport
software_cursor_active: bool,
software_cursor_pos: ViewportPosition,
// A locked pointer delta waiting to be applied by the next `PointerMove`
pointer_lock_delta: Option<ViewportPosition>,
last_absolute_pointer: ViewportPosition,
tracking_locked_deltas: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be tracked by the InputPreprocessorMessageHandler not by the TransformLayerMessageHandler.

This reduces the stupidly large number of fields in the TransformLayerMessageHandler.

It also makes more logical sense and allows other message handlers to use the wrapping functionality.

@@ -191,11 +199,10 @@ impl MessageHandler<TransformLayerMessage, TransformLayerMessageContext<'_>> for
}
}

*mouse_position = input.mouse.position;
*start_mouse = input.mouse.position;
// `mouse_position` already includes any locked deltas accumulated so far

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it isn't really the mouse position is it? Consider giving a more informative name.

Comment thread desktop/src/input.rs Outdated
return None;
}

let (x, y) = (viewport.x + x * viewport.scale, viewport.y + y * viewport.scale);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you work with glam::DVec2 then there would be no need to do component wise operations in tuples.

@TTyChud

TTyChud commented Oct 1, 2026

Copy link
Copy Markdown
Author

hmmmmmmmm,
my first contribution so we can let this slide
I will send u a better pr

@0HyperCube

Copy link
Copy Markdown
Contributor

Thanks for your interest in contributing. I'm sorry if the code review seemed overly negative; I think this is a reasonable start to the problem. Feel free to keep this PR open and commit any improvements to the same branch.

# Conflicts:
#	editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs
- Software cursor state moves from TransformLayerMessageHandler to InputPreprocessorMessageHandler, dropping five fields from the transform layer and letting any message handler use the wrapping.
- The desktop software cursor is a glam::DVec2 rather than an (f64, f64) tuple, and window_position documents the coordinate space it converts from.
- wrap_software_cursor uses DVec2::rem_euclid instead of a hand-rolled positive modulo.
- Locked-pointer movement folds into the existing pointermove/pointerlockchange pipeline instead of adding a second pair of window listeners.
- The locked-delta forwarding is a named helper, keeping onPointerMove's cyclomatic complexity under the limit.

Verified: cargo check -p graphite-editor --lib clean, cargo fmt --all -- --check clean, eslint clean on the changed frontend files.
…ovement

- A granted lock that never reports movement no longer freezes the pointer: the reported position keeps driving it until a locked delta proves the deltas exist, and the pointer routes like an unlocked one until then.
- The OS cursor stays hidden for the whole transform while the software cursor is drawn, instead of hover changes revealing it.
- Web only: the lock is not re-requested once the viewport owns it, and the Escape that released the lock no longer cancels the transform a second time.

Verified: cargo check -p graphite-editor --tests clean, cargo fmt --all -- --check clean, eslint clean and svelte-check reports no problems in the changed frontend files.
- `mouse_position` is now `previous_pointer_position`, which is what it holds once G/R/S wraps the tracked pointer away from the mouse.
- `tracking_deltas` is now `locked_delta_seen`, which is what the flag records.
- The `CursorChange` arm collapses into the let chain the rest of `app.rs` already uses, so it stops tripping clippy.
- The web pointer lock is requested once per transform, so a refused lock is not re-requested on every pointer move the transform reports.
- An Escape from before the lock can no longer be consumed as the one that releases it.

Verified: cargo fmt --all -- --check clean, clippy clean on the changed crates, the input preprocessor and transform layer editor suites passing (7 and 18 tests), eslint and svelte-check clean on the changed frontend files.
@TTyChud
TTyChud marked this pull request as draft October 4, 2026 12:34
- Same facts in fewer words, without the trailing clauses the code below already spells out.
- The "owns the pointer" phrasing is gone from all four places it appeared.
- Two doc comments got the trailing period the rest of the branch uses; line comments stay period-free.

Verified: cargo fmt --all -- --check clean, eslint clean on the changed frontend files, and cargo check clean on graphite-editor, graphite-desktop and graphite-wasm-wrapper.
- Route every pointer event through the tracked pointer, so the click that
  confirms a G/R/S transform stops reporting the frozen OS position. This also
  covers double clicks, which had the same problem.
- Cancel the transform when the desktop window loses focus. The lock and the
  drawn cursor could previously survive an alt-tab with nothing to release them.
- Release the lock with a plain cursor instead of the last shape applied, so a
  transform started over a window resize edge doesn't flash that arrow on the
  way out.
- Rename the scaling branch's mouse offsets to previous/current, matching how
  they were already used.

Also reworded the comments this branch adds so they read like notes rather than
design docs, and shortened the test failure messages.

Tested with cargo test -p graphite-editor --lib (282 passing), cargo check
-Dwarnings on graphite-editor and graphite-desktop, cargo fmt, and eslint.
- Drop the unreachable arm in the locked-pointer routing. The early return just
  above it already rules out a lock that delivers movement, so the extra case and
  the tuple it switched on were dead.
- begin_operation no longer takes the pointer position as a write-only parameter.
  It reads the tracked pointer from self, where it already lives.
- Rename viewport_to_window_position (name converters by direction, like
  document_to_viewport) and software_cursor_position (it holds a position, not a
  cursor). App::unlock_pointer becomes release_pointer_lock so it doesn't shadow
  the InputState method it calls.
- Move UpdateSoftwareCursor next to UpdateMouseCursor, out of the cfg-gated
  window message block it was sitting in.
- Cut the comments that only repeat the name of the thing below them.

Checked with cargo test -p graphite-editor --lib (282 passing), cargo check
-Dwarnings on both crates, cargo fmt, clippy, and eslint.
@TTyChud
TTyChud marked this pull request as ready for review October 4, 2026 19:58
@TTyChud
TTyChud requested a review from 0HyperCube October 4, 2026 19:59

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 18 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread frontend/src/utility-functions/input.ts Outdated
The flag claimed the transform had already been cancelled by the Escape that
dropped the pointer lock, but it was set before the redirect check ran. When a
dialog is visible or a floating menu is open, shouldRedirectKeyboardEventToBackend
returns false and that Escape only dismisses the dialog, so the editor never saw
it. The lock-loss handler then skipped its synthetic Escape and left the
transform running with no lock.

Capture the pointer lock state synchronously so it can't change across the
awaits, and set the flag where the key is handed to the editor instead. For
Escape that path awaits nothing but microtasks, so the flag is still set before
the queued pointerlockchange can be handled.
- Deactivating the tools or making a document active ends any transform, so its pointer lock and drawn cursor cannot outlive it.
- A pointer lock granted after the transform already ended is handed back.
- Pointer moves are only swallowed while the software cursor drives them, and the locked device deltas go through in the units the platform reports.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 6 files (changes from recent commits).

Confidence score: 3/5

  • In desktop/src/app.rs, locked-pointer deltas bypass the viewport scaling used for absolute positions, so pointer movement can be mis-scaled on high-DPI platforms. Convert the deltas to viewport units before adding them.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="desktop/src/app.rs">

<violation number="1" location="desktop/src/app.rs:753">
P2: Locked device deltas are added directly to viewport-space cursor coordinates, unlike absolute desktop pointer positions, which `InputState::pointer_state` scales into viewport units. On high-DPI platforms reporting physical-pixel deltas, G/R/S movement is scaled relative to ordinary pointer movement; convert the delta to viewport units before scheduling it.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread editor/src/messages/tool/tool_message_handler.rs Outdated
Comment thread editor/src/messages/portfolio/portfolio_message_handler.rs Outdated
Comment thread desktop/src/app.rs Outdated
&& (x != 0. || y != 0.)
{
self.input_state.record_locked_delta();
// The device delta units are platform-defined, so they go through as they come

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Locked device deltas are added directly to viewport-space cursor coordinates, unlike absolute desktop pointer positions, which InputState::pointer_state scales into viewport units. On high-DPI platforms reporting physical-pixel deltas, G/R/S movement is scaled relative to ordinary pointer movement; convert the delta to viewport units before scheduling it.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At desktop/src/app.rs, line 753:

<comment>Locked device deltas are added directly to viewport-space cursor coordinates, unlike absolute desktop pointer positions, which `InputState::pointer_state` scales into viewport units. On high-DPI platforms reporting physical-pixel deltas, G/R/S movement is scaled relative to ordinary pointer movement; convert the delta to viewport units before scheduling it.</comment>

<file context>
@@ -750,9 +750,7 @@ impl ApplicationHandler for App {
-			// Device deltas are in physical pixels, the transform layer works in logical units
-			let scale = self.input_state.viewport_scale();
-			let (x, y) = if scale != 0. { (x / scale, y / scale) } else { (x, y) };
+			// The device delta units are platform-defined, so they go through as they come
 			let message = DesktopWrapperMessage::PointerLockMove { x, y };
 			self.app_event_scheduler.schedule(AppEvent::DesktopWrapperMessage(message));
</file context>

@TTyChud
TTyChud marked this pull request as draft October 5, 2026 04:14
- Queue the cancel before the document switch so its abort lands in the outgoing document
- Skip the cancel until a document is loaded, since a tool message without an active document stalls the dispatch and the switch
- Only cancel on tool deactivation when a transform is running, so an unfinished pen path survives
- Cover the document-switch cancel with a test and read a document's layer transform directly
- Clarify the pointer-lock delta comment
@TTyChud
TTyChud marked this pull request as ready for review October 5, 2026 04:41

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 22 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread frontend/src/components/panels/Document.svelte Outdated
- Reset the shared cursor store on teardown, so reopening the tab can't show a stale fake cursor
- Guard the async UpdateSoftwareCursor callback against resuming after teardown and re-showing it
- Release the pointer lock if the panel still holds it when it goes away

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread frontend/src/components/panels/Document.svelte Outdated
- The document is removed before SelectDocument runs, so its guard skipped the cancel and the transform leaked its software cursor and pointer lock
- Cancel in CloseDocument while the document is still loaded
- Add a regression test for closing the active document mid-transform
- Narrow InputState::viewport_scale back to module-private since only pointer_state uses it
- A request made just before teardown can still grant, and removing the listener left the page locked
- Keep the pointerlockchange listener until that late grant is handed back
- Hoist pointerLockRequested so teardown can tell a pending request from a settled one
Number inputs also lock the native pointer, so gating on pointer_locked cancelled unrelated operations when the window lost focus. A software cursor is only ever created by a transform, so gating on it restricts the synthetic Escape to the case it was added for.
Move the drawn cursor into the views components, keep the cursor store state-only with its client-position helper beside its only caller, and read the tracked pointer through a test accessor instead of the dispatcher internals.
@TTyChud

TTyChud commented Oct 5, 2026

Copy link
Copy Markdown
Author

Done ,
now u can do the final review of the whole codebase.

@0HyperCube 0HyperCube left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi; thanks for your continued work on this.

The change has got to be rather larger than I'd like (although a decent part of that is tests). To address this, I've suggested a major change regarding removing the frontend (see comment for details).

Comment thread desktop/src/app.rs Outdated
Comment on lines 621 to 625
// Focus loss drops the pointer lock underneath us, and this is the only notice we get
// Only a transform holds a software cursor, so a lock held elsewhere (a number input) isn't cancelled
if matches!(event, WindowEvent::Focused(false)) && self.input_state.software_cursor_active() {
self.send_escape_key();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very ugly as it is not clear from the tool code why you might see a fake escape event.

Comment thread desktop/src/input.rs Outdated
Comment on lines +127 to +128
if right > left { position.x.clamp(left, right) } else { position.x },
if bottom > top { position.y.clamp(top, bottom) } else { position.y },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest just assert!(viewport.width >= 0. && viewport_height >= 0.) unless you have some reason to expect negative sizes?

Comment thread desktop/src/input.rs Outdated
self.pointer_position = *position;

// A locked pointer freezes the OS cursor at the lock origin, so its reported position isn't movement
// Not every platform delivers locked deltas after accepting the lock, so the position keeps driving the pointer until one arrives

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the lock is already accepted then the pointer position will not change??

UpdateSoftwareCursor {
visible: bool,
x: f64,
y: f64,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows you to set the position of an invisible software cursor? This doesn't seem desirable.

Perhaps model as position: Option<(f64, f64)>.

Comment on lines +27 to +28
position: ViewportPosition,
last_reported: ViewportPosition,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names aren't particularly clear.

Could be like virtual_offset_position and position_before_lock?

Comment on lines +125 to +131
SelectDocument {
document_id: DocumentId,
},
// The switch itself, queued behind anything that has to run against the document being left
ActivateDocument {
document_id: DocumentId,
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like having two messages with very similar names.

HintData::clear_layout(responses);
}

// A transform belongs to the document being closed, so cancel it while that document is still loaded or its software cursor and pointer lock outlive it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can you switch documents whilst still transforming a layer??

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly suggest that you avoid any frontend changes for this PR if you wish for it to get merged per #4647. This would involve removing the support for web and using set_cursor_position for the desktop to warp the pointer to the other edge of the screen (no software cursor).

I'm sorry if this is requiring of a significant change in approach, however I think this is probably the most reasonable route forward for getting this merged.

Adding support for a fake cursor when the OS doesn't support moving the pointer (like on web) could be done as a follow up.

I suggest this because the PR is very long and the maintainers have not much time to review things.

I am not a maintainer myself so this is just my suggestion. The actual maintainers may wish to overrule this advice.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's also my fault for not consulting the maintainer first.

Comment on lines +1428 to +1436
let transform_before = get_layer_transform(&mut editor, layer).await.unwrap();
let pointer_before = editor.mouse_position();

let dropped = DVec2::new(75., 40.);
editor.handle_message(InputPreprocessorMessage::PointerLockMove { delta: dropped }).await;

assert_eq!(editor.mouse_position(), pointer_before + dropped, "pointer should still follow the delta the transform drops");
let transform_after = get_layer_transform(&mut editor, layer).await.unwrap();
assert!(transform_after.abs_diff_eq(transform_before, 1e-5), "dropped delta shouldn't move the layer");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't really do anything since a navigation message doesn't modify layer transforms. Events that trigger the transform layer message would need to run following the change to the viewport transform.

let transform_after = get_layer_transform(&mut editor, layer).await.unwrap();
assert!(transform_after.abs_diff_eq(transform_before, 1e-5), "dropped delta shouldn't move the layer");

editor.handle_message(TransformLayerMessage::CancelTransformOperation).await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to clean up?

Replace the software cursor, pointer lock, and web pointer-lock path with
a desktop-only warp. While a G/R/S transform runs, the desktop input state
tracks a continuous pointer position and moves the OS cursor to the
opposite edge of the viewport with `set_cursor_position` when it leaves
it. The editor only signals the start and end of the transform.

This drops the frontend, web, and software-cursor changes. Web and Wayland
cannot move the pointer, so they get no wrapping for now; a fake cursor
for platforms without a pointer warp can follow up.
- Keep the tracked pointer position continuous across a warp, so a G/R/S
  transform no longer jumps by the viewport size each time the pointer
  crosses an edge. Only point the next report at the warp target.
- Only hide the OS cursor when the pointer-lock grab actually succeeds,
  so a platform that refuses the lock isn't left with no visible cursor.
- Add a regression test for the continuous tracked position.
- Rename the scaling branch's mouse offsets to previous/current and drop
  dead scaffolding from the transform test helper.
@TTyChud
TTyChud force-pushed the cursor-wrap-grs-3255 branch from e820ea4 to bd4ce25 Compare October 6, 2026 14:37
- Route pointer moves to the editor while a G/R/S wrap is active, so the
  transform no longer stalls once the tracked position leaves the
  viewport. Adds a regression test.
- Have the window report whether the OS accepted the cursor move, and stop
  wrapping when it did not.
- Rename the transform layer's stored pointer position to
  previous_mouse_position, drop the closure parameter it replaced, and
  reuse the cached document-to-viewport matrix.
- Tighten the pointer wrap comments.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 25 files (changes from recent commits).

Confidence score: 3/5

  • In desktop/src/app.rs, losing focus during G/R/S clears the desktop wrap state but leaves the editor transform active, so dragging after refocus continues without wrapping. Cancel the transform or restore wrapping on focus loss.
  • The Cargo.toml revert downgrades convert_case from 0.12 to 0.8 and removes string_capitalization_test. Confirm both changes are intentional before merging.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="Cargo.toml">

<violation number="1" location="Cargo.toml:126">
P2: This revert downgrades convert_case from 0.12 (the state already on the target branch, `pr-base`) back to 0.8 and deletes `string_capitalization_test` from `node-graph/nodes/text/src/lib.rs`. When this PR merges, git will apply the 0.12→0.8 change cleanly over the target, silently reverting an upstream dependency bump and removing StringCapitalization regression tests that are unrelated to the cursor work. Keep the branch's diff scoped by rebasing onto the updated target so the tree already contains the 0.12 bump, instead of reverting changes already merged upstream.</violation>
</file>

<file name="desktop/src/app.rs">

<violation number="1" location="desktop/src/app.rs:585">
P2: When the window loses focus during G/R/S, this clears only the desktop wrap state; the editor’s transform remains active, so after refocus the drag continues without wrapping. Cancel the transform or restore wrapping when focus returns.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread Cargo.toml Outdated
bitflags = { version = "2.4", features = ["serde"] }
ctor = "0.2"
convert_case = "0.12"
convert_case = "0.8"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: This revert downgrades convert_case from 0.12 (the state already on the target branch, pr-base) back to 0.8 and deletes string_capitalization_test from node-graph/nodes/text/src/lib.rs. When this PR merges, git will apply the 0.12→0.8 change cleanly over the target, silently reverting an upstream dependency bump and removing StringCapitalization regression tests that are unrelated to the cursor work. Keep the branch's diff scoped by rebasing onto the updated target so the tree already contains the 0.12 bump, instead of reverting changes already merged upstream.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Cargo.toml, line 126:

<comment>This revert downgrades convert_case from 0.12 (the state already on the target branch, `pr-base`) back to 0.8 and deletes `string_capitalization_test` from `node-graph/nodes/text/src/lib.rs`. When this PR merges, git will apply the 0.12→0.8 change cleanly over the target, silently reverting an upstream dependency bump and removing StringCapitalization regression tests that are unrelated to the cursor work. Keep the branch's diff scoped by rebasing onto the updated target so the tree already contains the 0.12 bump, instead of reverting changes already merged upstream.</comment>

<file context>
@@ -123,7 +123,7 @@ env_logger = "0.11"
 bitflags = { version = "2.4", features = ["serde"] }
 ctor = "0.2"
-convert_case = "0.12"
+convert_case = "0.8"
 titlecase = "3.6"
 fancy-regex = "0.18.0"
</file context>

Comment thread desktop/src/app.rs Outdated
}

if matches!(event, WindowEvent::Focused(false)) {
self.input_state.set_pointer_wrap(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When the window loses focus during G/R/S, this clears only the desktop wrap state; the editor’s transform remains active, so after refocus the drag continues without wrapping. Cancel the transform or restore wrapping when focus returns.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At desktop/src/app.rs, line 585:

<comment>When the window loses focus during G/R/S, this clears only the desktop wrap state; the editor’s transform remains active, so after refocus the drag continues without wrapping. Cancel the transform or restore wrapping when focus returns.</comment>

<file context>
@@ -613,15 +571,18 @@ impl ApplicationHandler for App {
-		if matches!(event, WindowEvent::Focused(false)) && self.input_state.software_cursor_active() {
-			self.send_escape_key();
+		if matches!(event, WindowEvent::Focused(false)) {
+			self.input_state.set_pointer_wrap(false);
 		}
 
</file context>

Losing focus suspended the wrap for good while the transform stayed
active, so a drag after refocusing stopped wrapping at the viewport edge.
Keep the editor's request and re-apply it when focus returns, and cover
the suspend/resume and an already-ended transform with tests.
@TTyChud
TTyChud force-pushed the cursor-wrap-grs-3255 branch from 08b3ea4 to 157aa25 Compare October 6, 2026 17:33
@TTyChud

TTyChud commented Oct 6, 2026

Copy link
Copy Markdown
Author

@cubic review

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

@cubic review

@TTyChud I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 existing issue remains and 2 new issues found across 10 files

Confidence score: 3/5

  • app_window_message_handler.rs drops every PointerWrap request on wasm, so browser G/R/S cannot toggle viewport wrapping. Add a wasm-facing frontend message and handle it in the browser.
  • transform_layer_message_handler.rs enables wrapping on Wayland, but wrapping is then disabled when the desktop event path rejects the cursor warp; without a relative-pointer fallback, G/R/S stops receiving movement at the viewport edge. Add a fallback or avoid enabling wrapping when warping is unavailable.
  • app.rs can resume wrapping on Focused(true) while the OS cursor is still at the previous wrapped edge, causing a viewport-sized jump during an active G/R/S transform. Resynchronize the cursor position before resuming wrapping.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="editor/src/messages/app_window/app_window_message_handler.rs">

<violation number="1" location="editor/src/messages/app_window/app_window_message_handler.rs:24">
P1: This drops every `PointerWrap` request on wasm, so browser G/R/S never enables or disables viewport wrapping. Add a wasm-facing frontend message and handle it in the browser.</violation>
</file>

<file name="editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs">

<violation number="1" location="editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs:389">
P1: This enables native wrapping on Wayland, but the desktop event path disables it as soon as the platform rejects the cursor warp, with no relative-pointer fallback. G/R/S therefore stops receiving movement at the viewport edge on Wayland, contrary to the feature's stated Wayland support; keep the editor on a relative-pointer path when warping is unavailable instead of enabling this mode unconditionally.</violation>
</file>

Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.

Re-trigger cubic

#[cfg(not(target_family = "wasm"))]
responses.add(FrontendMessage::WindowPointerWrap { enabled });
#[cfg(target_family = "wasm")]
let _ = enabled;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: This drops every PointerWrap request on wasm, so browser G/R/S never enables or disables viewport wrapping. Add a wasm-facing frontend message and handle it in the browser.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At editor/src/messages/app_window/app_window_message_handler.rs, line 24:

<comment>This drops every `PointerWrap` request on wasm, so browser G/R/S never enables or disables viewport wrapping. Add a wasm-facing frontend message and handle it in the browser.</comment>

<file context>
@@ -17,6 +17,12 @@ impl MessageHandler<AppWindowMessage, ()> for AppWindowMessageHandler {
+				#[cfg(not(target_family = "wasm"))]
+				responses.add(FrontendMessage::WindowPointerWrap { enabled });
+				#[cfg(target_family = "wasm")]
+				let _ = enabled;
+			}
 			AppWindowMessage::DirectInput { enabled } => {
</file context>

responses.add(OverlaysMessage::AddProvider {
provider: TRANSFORM_GRS_OVERLAY_PROVIDER,
});
responses.add(AppWindowMessage::PointerWrap { enabled: true });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: This enables native wrapping on Wayland, but the desktop event path disables it as soon as the platform rejects the cursor warp, with no relative-pointer fallback. G/R/S therefore stops receiving movement at the viewport edge on Wayland, contrary to the feature's stated Wayland support; keep the editor on a relative-pointer path when warping is unavailable instead of enabling this mode unconditionally.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs, line 389:

<comment>This enables native wrapping on Wayland, but the desktop event path disables it as soon as the platform rejects the cursor warp, with no relative-pointer fallback. G/R/S therefore stops receiving movement at the viewport edge on Wayland, contrary to the feature's stated Wayland support; keep the editor on a relative-pointer path when warping is unavailable instead of enabling this mode unconditionally.</comment>

<file context>
@@ -385,6 +386,7 @@ impl MessageHandler<TransformLayerMessage, TransformLayerMessageContext<'_>> for
 				responses.add(OverlaysMessage::AddProvider {
 					provider: TRANSFORM_GRS_OVERLAY_PROVIDER,
 				});
+				responses.add(AppWindowMessage::PointerWrap { enabled: true });
 				// Find a way better than this hack
 				responses.add(TransformLayerMessage::PointerMove {
</file context>

Comment thread desktop/src/input.rs Outdated
The wrap decision used the tracked position, which is deliberately offset
from the OS cursor after a wrap, so the cursor could leave the viewport and
the transform could no longer reach the far side. Wrap from the reported
position and cover the reverse-direction escape with a test.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cursor should wrap around window when using G/R/S

3 participants