Desktop: Switch to winit clipboard API - #4519
Conversation
|
!build desktop (Run ID 34660824992) |
There was a problem hiding this comment.
4 issues found across 7 files
Confidence score: 2/5
desktop/src/input.rssilently drops the newMouseScrollDeltavariant, so scrolling can stop working on devices or backends that emit it; handle the variant and convert it into the canvas input path.desktop/src/app.rsoverwrites the pending paste serial when another asynchronous fetch starts, causing the earlier paste result to be ignored; track concurrent fetches independently and clear stalled requests so clipboard or drag-and-drop operations cannot leave the UI waiting indefinitely.Cargo.tomlroutes the workspace’s corewinitdependency through an unpinned personal git branch, creating supply-chain and reproducibility risk; use a trusted pinned release, commit, or approved source.
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="desktop/src/input.rs">
<violation number="1" location="desktop/src/input.rs:224">
P2: The newly added `_ => return` arms silently drop the new MouseScrollDelta variant, so scroll input reported in that unit is discarded entirely. On devices/backends that emit the new variant, canvas (in the editor route) and UI scrolling will both stop working. Convert the new variant into a scroll delta (e.g. treat it like PixelDelta, or clamp/conservative handling) rather than ignoring it, or at minimum route it to the existing scroll path so scrolling keeps working on those inputs.</violation>
</file>
<file name="Cargo.toml">
<violation number="1" location="Cargo.toml:263">
P2: This patch routes the entire workspace's `winit` dependency (version 0.31.0-beta.2) through an unpinned personal git fork branch, which is a supply-chain and reproducibility concern for a core dependency. Unlike the adjacent `rfd` patch, this line has no comment or removal plan referencing the upstream winit clipboard work (winit PR #4658) that will let it be dropped once a release ships. Because only `branch = "graphite"` is specified without a `rev`, any force-push/rewrite of that branch breaks every downstream Cargo.lock and silently changes the winit code each time a contributor re-resolves. Pin the commit and add a TODO.</violation>
</file>
<file name="desktop/src/app.rs">
<violation number="1" location="desktop/src/app.rs:479">
P2: When a second paste starts before the first asynchronous fetch completes, this assignment overwrites the first serial, so the first `DataTransferReceived` event is ignored and that paste is silently dropped. Track all outstanding serials or explicitly serialize clipboard reads.</violation>
<violation number="2" location="desktop/src/app.rs:646">
P2: A clipboard read (or dropped-file fetch) whose serial never produces a DataTransferReceived leaves pending_clipboard_fetch/pending_dnd_fetch set forever, so no ClipboardReadResult is ever dispatched and the UI awaits indefinitely; the Err(WouldBlock) arm provides no terminal fallback and there is no timeout or DragLeave cleanup. Add a guaranteed terminal path (e.g. clear the pending state and dispatch content:None if the transfer cannot resolve) rather than relying solely on winit re-delivering the event.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let input = match delta { | ||
| MouseScrollDelta::LineDelta(x, y) => InputEvent::pointer().scrolled_lines(f64::from(*x), f64::from(*y)), | ||
| MouseScrollDelta::PixelDelta(position) => InputEvent::pointer().scrolled_pixels(position.x, position.y), | ||
| _ => return, |
There was a problem hiding this comment.
P2: The newly added _ => return arms silently drop the new MouseScrollDelta variant, so scroll input reported in that unit is discarded entirely. On devices/backends that emit the new variant, canvas (in the editor route) and UI scrolling will both stop working. Convert the new variant into a scroll delta (e.g. treat it like PixelDelta, or clamp/conservative handling) rather than ignoring it, or at minimum route it to the existing scroll path so scrolling keeps working on those inputs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/input.rs, line 224:
<comment>The newly added `_ => return` arms silently drop the new MouseScrollDelta variant, so scroll input reported in that unit is discarded entirely. On devices/backends that emit the new variant, canvas (in the editor route) and UI scrolling will both stop working. Convert the new variant into a scroll delta (e.g. treat it like PixelDelta, or clamp/conservative handling) rather than ignoring it, or at minimum route it to the existing scroll path so scrolling keeps working on those inputs.</comment>
<file context>
@@ -221,6 +221,7 @@ impl InputState {
let input = match delta {
MouseScrollDelta::LineDelta(x, y) => InputEvent::pointer().scrolled_lines(f64::from(*x), f64::from(*y)),
MouseScrollDelta::PixelDelta(position) => InputEvent::pointer().scrolled_pixels(position.x, position.y),
+ _ => return,
};
ui_callback(input.modifiers(self.modifiers).build());
</file context>
| debug = true | ||
|
|
||
| [patch.crates-io] | ||
| winit = { git = "https://github.com/timon-schelling/winit", branch = "graphite" } |
There was a problem hiding this comment.
P2: This patch routes the entire workspace's winit dependency (version 0.31.0-beta.2) through an unpinned personal git fork branch, which is a supply-chain and reproducibility concern for a core dependency. Unlike the adjacent rfd patch, this line has no comment or removal plan referencing the upstream winit clipboard work (winit PR #4658) that will let it be dropped once a release ships. Because only branch = "graphite" is specified without a rev, any force-push/rewrite of that branch breaks every downstream Cargo.lock and silently changes the winit code each time a contributor re-resolves. Pin the commit and add a TODO.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Cargo.toml, line 263:
<comment>This patch routes the entire workspace's `winit` dependency (version 0.31.0-beta.2) through an unpinned personal git fork branch, which is a supply-chain and reproducibility concern for a core dependency. Unlike the adjacent `rfd` patch, this line has no comment or removal plan referencing the upstream winit clipboard work (winit PR #4658) that will let it be dropped once a release ships. Because only `branch = "graphite"` is specified without a `rev`, any force-push/rewrite of that branch breaks every downstream Cargo.lock and silently changes the winit code each time a contributor re-resolves. Pin the commit and add a TODO.</comment>
<file context>
@@ -260,6 +260,7 @@ lto = "thin"
debug = true
[patch.crates-io]
+winit = { git = "https://github.com/timon-schelling/winit", branch = "graphite" }
rfd = { git = "https://github.com/timon-schelling/rfd.git", branch = "graphite" } # TODO: Remove this once https://github.com/PolyMeilex/rfd/pull/317 is merged and released
cef = { git = "https://github.com/timon-schelling/cef-rs.git", branch = "graphite-151" }
</file context>
| winit = { git = "https://github.com/timon-schelling/winit", branch = "graphite" } | |
| winit = { git = "https://github.com/timon-schelling/winit", rev = "1226add1bb6473ebaae83c3e1a43b81f170aca54" } # TODO: Remove this and restore the crates.io release once the winit clipboard API (https://github.com/rust-windowing/winit/pull/4658) is in a released version |
| event_loop.fetch_data_transfer(id, &TypeHint::Plaintext).map(Some) | ||
| }); | ||
| match result { | ||
| Ok(Some(serial)) => self.pending_clipboard_fetch = Some(serial), |
There was a problem hiding this comment.
P2: When a second paste starts before the first asynchronous fetch completes, this assignment overwrites the first serial, so the first DataTransferReceived event is ignored and that paste is silently dropped. Track all outstanding serials or explicitly serialize clipboard reads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/app.rs, line 479:
<comment>When a second paste starts before the first asynchronous fetch completes, this assignment overwrites the first serial, so the first `DataTransferReceived` event is ignored and that paste is silently dropped. Track all outstanding serials or explicitly serialize clipboard reads.</comment>
<file context>
@@ -467,6 +466,32 @@ impl App {
+ event_loop.fetch_data_transfer(id, &TypeHint::Plaintext).map(Some)
+ });
+ match result {
+ Ok(Some(serial)) => self.pending_clipboard_fetch = Some(serial),
+ Ok(None) => self.dispatch_desktop_wrapper_message(DesktopWrapperMessage::ClipboardReadResult { content: None }),
+ Err(e) => {
</file context>
| let message = DesktopWrapperMessage::ClipboardReadResult { content: Some(content) }; | ||
| self.app_event_scheduler.schedule(AppEvent::DesktopWrapperMessage(message)); | ||
| } | ||
| Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => {} |
There was a problem hiding this comment.
P2: A clipboard read (or dropped-file fetch) whose serial never produces a DataTransferReceived leaves pending_clipboard_fetch/pending_dnd_fetch set forever, so no ClipboardReadResult is ever dispatched and the UI awaits indefinitely; the Err(WouldBlock) arm provides no terminal fallback and there is no timeout or DragLeave cleanup. Add a guaranteed terminal path (e.g. clear the pending state and dispatch content:None if the transfer cannot resolve) rather than relying solely on winit re-delivering the event.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/app.rs, line 646:
<comment>A clipboard read (or dropped-file fetch) whose serial never produces a DataTransferReceived leaves pending_clipboard_fetch/pending_dnd_fetch set forever, so no ClipboardReadResult is ever dispatched and the UI awaits indefinitely; the Err(WouldBlock) arm provides no terminal fallback and there is no timeout or DragLeave cleanup. Add a guaranteed terminal path (e.g. clear the pending state and dispatch content:None if the transfer cannot resolve) rather than relying solely on winit re-delivering the event.</comment>
<file context>
@@ -601,20 +626,53 @@ impl ApplicationHandler for App {
+ let message = DesktopWrapperMessage::ClipboardReadResult { content: Some(content) };
+ self.app_event_scheduler.schedule(AppEvent::DesktopWrapperMessage(message));
+ }
+ Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => {}
+ Err(e) => {
+ self.pending_clipboard_fetch = None;
</file context>
|
|
|
Upstream work in rust-windowing/winit#4658 (thanks to @0HyperCube for implementing the x11 backend).
For now pulling this from my winit fork.