Repository navigation
Conversation
Hyprland delivers selection offers only to the first wl_data_device a client creates. In Neomacs that is winit's drag-and-drop device, so smithay-clipboard's device never sees an offer and every CLIPBOARD read returns nil, even while Neomacs is focused. wlroots compositors send the selection to every device, which is why they were unaffected. When smithay-clipboard reports no selection offer or no text type, read the CLIPBOARD through ext-data-control-v1 on a private event queue of the same display. Each read creates a short-lived device, transfers the current selection with smithay-clipboard's MIME preference and line-end normalization, and destroys every object it received. Focus and seat errors still propagate, and PRIMARY reads are unchanged.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe Wayland clipboard backend adds a data-control reader. CLIPBOARD reads use it when smithay-clipboard reports no usable selection offer. PRIMARY reads continue to use smithay-clipboard alone. ChangesWayland clipboard fallback
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Caller
participant WaylandClipboard
participant SmithayClipboard
participant DataControlReader
participant SelectionOwner
Caller->>WaylandClipboard: Read CLIPBOARD
WaylandClipboard->>SmithayClipboard: Read selection
SmithayClipboard-->>WaylandClipboard: No usable offer
WaylandClipboard->>DataControlReader: Bind lazily if needed
WaylandClipboard->>DataControlReader: Read text
DataControlReader->>SelectionOwner: Request selected text MIME type
SelectionOwner-->>DataControlReader: Transfer text over pipe
DataControlReader-->>WaylandClipboard: Decoded text or no selection
WaylandClipboard-->>Caller: Return selection
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue remains established for this change; it is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The fallback limits transfer time but not memory consumption. A hostile clipboard owner could supply enough data to exhaust the editor process when the fallback is used. Access remains on the existing desktop connection, but behavior across multiple seats is not established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 55.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 4 files. (1 skipped: 1 unsupported.)
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 ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
|
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It involves unsafe FFI on a cross-thread-shared foreign wl_display, manual Wayland protocol object lifecycle management, and correctness that ultimately depends on live-compositor behavior not verifiable in this environment.
Review effort: Balanced
Findings: None
What changed in this PR
On Hyprland, wlroots-style selection routing only delivers the CLIPBOARD offer to winit's first wl_data_device, so smithay-clipboard's device never sees it and neomacs-clipboard-get returns nil even while focused. This PR adds a lazily-bound ext-data-control-v1 fallback that reads the selection directly (bypassing data-device focus routing) only when smithay-clipboard reports no usable offer, leaving writes, PRIMARY reads, and non-buggy compositors behaving as before.
Changes:
- New
wayland_data_controlmodule: a private event queue on the shared foreignwl_displaythat bindsext-data-control-v1, creates a short-lived device per read, transfers the selection over a pipe with a 3s deadline, and cleans up all offers/devices. WaylandClipboardnow holds a lazily-boundDataControlfallback;text()routes CLIPBOARD reads throughsmithay_text_or_fallback, consulting data-control only when smithay saw no offer while preserving focus/seat errors.rustixgains theeventfeature (workspace) forpoll; unit tests cover MIME selection, line-end normalization, lossy UTF-8, pipe read-to-EOF/timeout, and the fallback decision matrix.
| File | Description |
|---|---|
crates/neomacs-display-runtime/src/clipboard/wayland_data_control.rs |
New ext-data-control-v1 reader: MIME choice, decode, per-read device lifecycle, poll-based pipe transfer with deadline. |
crates/neomacs-display-runtime/src/clipboard.rs |
Integrates the lazy DataControl fallback and smithay_text_or_fallback/smithay_saw_no_offer helpers into text(). |
crates/neomacs-display-runtime/src/clipboard/tests/wayland_data_control_test.rs |
Tests TextMime selection/decoding and read_to_end_before EOF/timeout behavior. |
crates/neomacs-display-runtime/src/clipboard/tests/clipboard_test.rs |
Tests the fallback decision matrix (native wins, no-offer/no-MIME fall back, empty is text, errors not masked, failed fallback stays nil). |
Cargo.toml |
Adds the event feature to the workspace rustix dependency for poll. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
eval-exec
left a comment
eval-exec
left a comment
There was a problem hiding this comment.
Clean diagnosis and a well-scoped fix.
Verified against the vendored crate sources: the fallback only triggers when smithay-clipboard reports no selection offer (its focus and seat errors still propagate), and MIME choice, lossy UTF-8 decoding, and CRLF normalization match smithay-clipboard 0.7.3's behavior exactly — so both paths return the same text. The from_foreign_display call carries the same contract smithay-clipboard itself relies on, documented at the definition and the call site, and the field drop order holds (wayland-backend only disconnects a foreign display when it owns it).
Non-blocking follow-ups, fine for a later PR:
- A byte cap on the transfer — the 3 s deadline bounds time, not memory.
- A typed error at the backend boundary instead of matching smithay's
"selection is empty"string; the arboard path already matches on a type. - An upstream Hyprland issue: the single-device-per-client behavior is still present on their main, so this fallback can eventually be deleted rather than maintained.
PR #482's data-control fallback re-implemented smithay-clipboard's MIME preference and decode rules, and both the fallback trigger and the arboard path classified native errors inline, including a string match on smithay's "selection is empty" message. Move the MIME vocabulary, the decode rules, the fallback decision, and the two native-error classifiers into the clipboard text_policy module, and give reads a typed answer: TextRead::{Text, NoSelection, TargetUnavailable}. "No owner" and "an owner with no readable text type" are now distinct inside the runtime, and smithay's message strings exist in exactly one adapter. This is behaviour-preserving: execute_command maps the typed answer back to the evaluator-facing Option<String>, with both absences reading as nil, so batch semantics and the oracle pins are untouched.
On Hyprland,
neomacs-clipboard-getreturns nil while Neomacs has keyboard focus, even when another client (or Neomacs itself) owns a text CLIPBOARD selection. Copying out of Neomacs works; reading never does.Why
Neomacs has two
wl_data_deviceobjects for the seat on its one Wayland connection: winit's drag-and-drop device, which is created first, and smithay-clipboard's. wlroots sendsselection/data_offerto every data device of the focused client. Hyprland'sCWLDataDeviceProtocol::dataDeviceForClientreturns only the first device it finds for the client (src/protocols/core/DataDevice.cppin Hyprland 0.56.2), so the offer goes to winit's device and smithay-clipboard's device never gets one.Clipboard::load()then fails withselection is empty, which Neomacs reports as nil.WAYLAND_DEBUG=clienttraces from nested Hyprland 0.56.2 show everywl_data_device.selectiongoing to winit's device and none to smithay-clipboard's. The same client under a wlroots compositor (labwc 0.20.2) gets the selection on both devices. An input method (fcitx5) makes no difference.What changed
ext-data-control-v1, using a private event queue on the samewl_display.text/plain;charset=utf-8orUTF8_STRINGin offer order, elsetext/plain. Both paths return the same text.client doesn't have focus,no events received on any seat) still propagate. PRIMARY reads and all writes are unchanged.wl_data_device. On compositors without the bug, an empty or non-text CLIPBOARD costs one extra short-lived data-control device and roundtrip per read; a failed or timed-out fallback logs a warning and reads as nil.rustixgains theeventfeature forpoll.Testing
cargo nextest run -p neomacs-display-runtime -E 'test(/clipboard|wayland_data_control/)': 18 passed. This includes new tests for the fallback decision (native text wins, no-offer and no-MIME fall back, an empty transfer is text, focus/seat errors are not masked, a failed fallback stays nil) and for the transfer helpers (MIME choice, line-end normalization, lossy UTF-8, reading to EOF, timeout on a stalled owner).cargo xtask fresh-build --release) run in a nested headless Hyprland 0.56.2, with and without fcitx5, driven throughneomacsclient --eval:(neomacs-clipboard-get)returned nil afterwl-copywhile focused, and also nil for Neomacs's own selection.wl-copy, and Neomacs's own selection.wl-pastestill reads text set from Neomacs.This PR is agent-assisted.