Repository navigation
Conversation
GitHub Actions
Error
github-actions
Bot
added
kind/protocol
AKolenda
changed the title
…#2007) Windows lists every dynamic channel it intends to move in its Soft-Sync request, including the ones the client declined with NO_LISTENER. Against a Windows 11 host the request lists channels 2, 6, 7, 8, 9, 10, 11 and 12 (CoreInput, MouseCursor, Graphics, Video, Geometry, ...), and only channel 7, the graphics pipeline, is open. `process_soft_sync_request` dropped a whole channel list as soon as one ID in it was not open. The tunnel was then never switched, and the channels the client had opened stayed on TCP while the server was already sending them on the tunnel (MS-RDPEDYC 3.2.5.3.1). Unopened channels are now skipped one by one, and the tunnel is switched for the rest. ## Testing - New `dvc::client::soft_sync_skips_channels_the_client_did_not_open` in `ironrdp-testsuite-core`. - Live, against a Windows 11 host over RDP-UDP version 2, with the viewer built from a branch that also carries the tunnel and client PRs of this series: the Soft-Sync request above now switches the tunnel, and the graphics pipeline moves onto it. ## Checks - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings` - `cargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra`, plus the lib tests of the crates touched here - `cargo test --workspace --locked` on a branch that merges this PR with the other Windows interop PRs from this series - `typos` on the changed files ## Series These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on `master` and can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order. - #2007 fix(dvc): Soft-Sync tunnel with declined channels - #2008 fix(session)!: channels and graphics on the tunnel - #2009 fix(rdpeudp): auto-detect on the tunnel - #2010 fix(graphics)!: SRL streams from Windows - #2011 fix(egfx): bitmap cache across ResetGraphics - #2012 feat(session): bandwidth measurements during the session - #2013 feat(client): graphics pipeline and RDP-UDP version options - #2014 fix(client): resize reconnects on the graphics pipeline - #2015 feat(client): transport event - #2016 feat(cliprdr): Linux clipboard backend - #2017 feat(rdpdr): printer on Linux and macOS Co-authored-by: AKolenda <testedemail2222@gmail.com>
|
A Linux backend fills a real gap, since the Linux client falls back to the stub today. Two of the design reasons in the description do not hold for the protocols, though, and they drive the parts of this I would push back on. "Neither clipboard protocol notifies a non-owner of selection changes": both ext-data-control-v1 and wlr-data-control-unstable-v1 send a selection event to every data-control client whenever the selection changes, and on X11, XFixes (XFixesSelectSelectionInput) gives the same notification. The 750 ms poll, which re-reads the full text and image on every tick, is a limit of arboard's API rather than of the protocols. "Neither protocol lets a client offer data it only produces on demand": both are on-demand by design. A data-control source gets a send event only when something pastes, and an X11 selection owner answers SelectionRequest only when asked. That is the model MS-RDPECLIP's delayed rendering is built for: announce the formats, then fetch the data only when a local application pastes. Fetching every remote copy as soon as it is announced moves every copied image across the connection whether or not it is ever pasted. The worker's check that it is not reading back its own write is also what On the server side of this same bridge I use data-control directly, with the selection event for change detection and on-demand sources for delayed rendering, which avoids both the polling and the eager transfer. Is there a reason to stay on arboard for the Linux backend rather than using data-control directly, with X11 as the fallback? |
|
You're right on both counts, thanks for spelling them out. The description blamed the protocols for what are really arboard's limits: it only exposes whole-value get and set, so there's no selection event to listen to and no way to serve a paste on demand. That's where the 750 ms poll and the eager fetch come from. There's no reason to stay on arboard beyond getting something working, so I'll move this onto data-control directly (ext-data-control-v1, then wlr-data-control), using the selection event and delayed rendering, with XFixes and selection ownership for X11. The echo check will go through One question before I write that. |
|
Yes, I'll do that, and I'll put it in |
|
Thanks for the patience on this. The data-control clipboard client is up as #2055, as a linux-only data_control module in ironrdp-cliprdr-native. It is the client only, with no CLIPRDR wiring, so the backend in this PR can be built on it. DataControl::connect picks ext-data-control-v1 or wlr-data-control, and read(mime) returns the current selection with charset-tolerant type matching. set_selection takes a Content whose types are either given data or only advertised. A paste of an advertised type without data raises a TransferRequest through on_transfer, and complete() or fail() answers it, which lines up with the Format Data Request and Response. on_change reports selections made by other clients only, so a backend does not see its own clipboard come back. The loop detector stays the backend's concern. GNOME has no data-control protocol, so connect returns Error::Unsupported there, and X11 is not covered, which is where XFixes comes in. Both PRs touch crates/ironrdp-cliprdr-native/Cargo.toml and src/lib.rs, so whichever merges second will need a small rebase. |
Add the data_control module to ironrdp-cliprdr-native so a Linux CLIPRDR backend can read and set the clipboard without a window. It speaks ext-data-control-v1 and wlr-data-control-unstable-v1 and uses whichever the compositor offers. One thread owns the Wayland connection and DataControl is the handle to call from anywhere. A type advertised without data raises a TransferRequest when something pastes it, which maps onto the CLIPRDR Format Data Response. A selection the client set itself is not reported back as a local copy. Reads are capped at 100 MiB and time out when the source stalls for 5 seconds. The state tests live in ironrdp-testsuite-core behind the __test feature, because inline tests are not built for this crate.
Benoît Cortier (CBenoit)
added
automation-failed
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #2016 adds a Linux CLIPRDR backend (arboard-based, X11/Wayland) plus client wiring. The mechanics are largely sound: capability advertisement, format list/data PDU handling, PNG/DIB conversion, thread isolation, and tests check out, and the client gracefully falls back to the stub. The weaknesses are a design whose justification rests on factually incorrect doc-comment claims about X11/Wayland capabilities (driving the 750 ms poll and eager remote fetch the author already agreed to rework onto data-control/XFixes per #2055), a hand-rolled echo guard duplicating ironrdp_cliprdr::loop_detector, a heavy Linux-only dependency stack slated for removal, a duplicated PNG/RGBA codec missing bitmap's allocation guard on a remote-reachable path, a redundant PendingPaste enum, triplicated cfg blocks in the client, and a low-severity protocol hazard: Format Data Responses cannot be correlated to requests, so a late reply after the pending-paste timeout can apply superseded content to the newer…
05ed96a to
e6a0cc6
Compare
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
|
Pushed e6a0cc6, rebased onto current master and #2055, addressing all seven review findings. The Linux backend now uses event-driven selection notifications and on-demand rendering with the shared Wayland client and native X11/XFixes support. Follow-up review also fixed ownership-event races, late-response handling, and bounded descriptor/writer/INCR lifetimes. Validation on this exact revision: 1,764 core tests and 103 extra tests pass; all three private X11 integration tests pass, including xclip interoperability, large transfers and backpressure; full workspace Clippy, formatting, changed-file typos, and diff checks pass. Wayland state and real-pipe tests pass; no live compositor interoperability run was performed. Fresh platform CI is running. The shared public API dependency-resolution failure is addressed separately by #2071 and requires that workflow repair on the base branch. |
github-actions
Bot
added
automation-failed
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
|
Follow-up d826491 fixes two lints exposed by building the native crate without the test-only feature: removed the now-unused LocalChanged payload and an obsolete lint expectation. Both production and __test native Clippy configurations pass, as does Clippy for the affected test target. Re-ran all 48 clipboard tests and all three private X11 integration tests successfully; formatting, typo and diff checks also pass. The final normal CI run passed all 27 checks on d826491: https://github.com/Devolutions/IronRDP/actions/runs/36975991083. The separate API check still needs the shared workflow repair in #2071 merged into the base branch. |
|
Thanks for building on #2055 and for the fixes in these commits. I pushed an update to #2055 that changes state.rs, client.rs, worker.rs and dispatch.rs, so your next rebase will conflict in those files. It overlaps with your commits in the paste cap and deadline (the same 16 and 5 seconds), the Arc<[u8]> cached data, the removal of the expect(unreachable_pub) on the state module, and the foreign copy that arrives before the cancel for the client's source, which is handled by counting the echoes of the client's own selections. dispatch.rs is now one macro that generates the handlers for both protocols, so is_current_source goes in one place. Synchronize, read_pipe and the total read deadline aren't in #2055. |
|
A correction to my note above. The foreign copy that arrives before the cancel for the client's source is no longer handled by counting echoes. The client now advertises one private MIME type on its own selections and treats a selection that carries it as its own, so a copy by another client is reported when it arrives, whatever types it offers. The rest of my note stands. |
|
Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes. |
github-actions
Bot
added
risk/high
|
This pull request may overlap with #2055. PR 2055 adds a Wayland data-control clipboard client in ironrdp-cliprdr-native as a data_control module; this PR adds the same data_control module (client, dispatch, state, worker, mime, options) in that crate plus a Linux CLIPRDR backend built on it, matching the scope PR 2055 describes. This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
There was a problem hiding this comment.
PR 2016 adds an event-driven Linux CLIPRDR backend (shared Wayland data-control client plus X11/XFixes fallback) with delayed rendering, a single outstanding Format Data Request, generation-based stale rejection, and bounded transfers. Independent review confirms the wire-visible machinery is sound and the maintainer's objections (polling, eager fetch, bespoke echo guard) are addressed. Remaining published issues: the channel answers Format List OK synchronously while the OS mirror is asynchronous and fallible, so CB_RESPONSE_FAIL is never retracted; the PR ships a data_control module that verifiably diverges from #2055's updated versions of the same four files (rebase required before merge); DataControl::update_data is unreachable dead plumbing for the removed eager-fetch flow; and the MIME-set own-selection heuristic misclassifies foreign same-format copies. The rest are behavior-preserving compressions: duplicated Wayland dispatch impls, a redundant Preference variant, a repeated l…
| Command::RemoteCopy(formats) => { | ||
| self.invalidate(); | ||
| self.remote = formats; | ||
| self.owns_remote = true; | ||
| let mut mimes = Vec::new(); | ||
| if self.remote.iter().any(|f| f.id() == ClipboardFormatId::CF_UNICODETEXT) { | ||
| mimes.extend([TEXT.to_owned(), "text/plain".to_owned()]); | ||
| } | ||
| if self | ||
| .remote | ||
| .iter() | ||
| .any(|f| matches!(f.id(), ClipboardFormatId::CF_DIB | ClipboardFormatId::CF_DIBV5)) | ||
| { | ||
| mimes.push(PNG.to_owned()); | ||
| } | ||
| if let Err(error) = self.os.offer(&mimes, self.generation) { | ||
| warn!(%error, "Could not advertise the remote clipboard"); | ||
| let _ = self.os.clear(); | ||
| self.owns_remote = false; | ||
| self.remote.clear(); | ||
| self.local_mimes = self.os.mime_types(); | ||
| if self.ready { | ||
| self.advertise_local(); | ||
| } | ||
| } |
There was a problem hiding this comment.
[protocol] Success Format List Response is never retracted when the local clipboard mirror fails — low 🟡 — The peer's Format List is answered OK synchronously by the cliprdr layer, while this backend mirrors the formats onto the OS clipboard asynchronously via Command::RemoteCopy. That mirror is fallible (Wayland ownership may not be acquired; X11 set_selection_owner may lose the race), and the failure path (lines 129-138) silently clears and re-advertises local formats without ever sending a CB_RESPONSE_FAIL Format List Response, so the peer is told the formats are available when they are not (MS-RDPECLIP 3.1.5.2.2). Contained: subsequent pastes are served CB_RESPONSE_FAIL Format Data Responses per 3.1.5.4.3, so it is an acknowledgment deviation rather than stale-data delivery.
| //! A Wayland clipboard client built on the data-control protocols. | ||
| //! | ||
| //! Data-control lets a client that has no window read and set the clipboard. | ||
| //! This module speaks both `ext-data-control-v1` and | ||
| //! `wlr-data-control-unstable-v1` and picks whichever the compositor offers. | ||
| //! | ||
| //! It has no async runtime. One thread owns the Wayland connection, and | ||
| //! [`DataControl`] is the handle to call from anywhere. | ||
| //! | ||
| //! # Reading | ||
| //! | ||
| //! ```no_run | ||
| //! use ironrdp_cliprdr_native::data_control::DataControl; | ||
| //! | ||
| //! # fn main() -> ironrdp_cliprdr_native::data_control::Result<()> { | ||
| //! let clipboard = DataControl::connect()?; | ||
| //! if let Some(bytes) = clipboard.read("text/plain;charset=utf-8")? { | ||
| //! println!("{}", String::from_utf8_lossy(&bytes)); | ||
| //! } | ||
| //! # Ok(()) | ||
| //! # } | ||
| //! ``` | ||
| //! | ||
| //! # Owning the clipboard, with delayed rendering | ||
| //! | ||
| //! A type advertised without data raises a [`TransferRequest`] when something | ||
| //! pastes it, so the data is produced only if it is wanted. That maps onto | ||
| //! CLIPRDR, where the Format Data Response follows the paste. | ||
| //! | ||
| //! ```no_run | ||
| //! use ironrdp_cliprdr_native::data_control::{Content, DataControl}; | ||
| //! | ||
| //! # fn main() -> ironrdp_cliprdr_native::data_control::Result<()> { | ||
| //! let clipboard = DataControl::connect()?; | ||
| //! clipboard.on_transfer(|request| { | ||
| //! let _ = request.complete("rendered on demand"); | ||
| //! }); | ||
| //! clipboard.set_selection(Content::new().advertise("text/plain;charset=utf-8"))?; | ||
| //! # Ok(()) | ||
| //! # } | ||
| //! ``` | ||
| //! | ||
| //! GNOME's Mutter offers no data-control protocol, so [`DataControl::connect`] | ||
| //! returns [`Error::Unsupported`] there. | ||
|
|
||
| mod client; | ||
| mod dispatch; | ||
| mod error; | ||
| mod mime; | ||
| mod options; | ||
| #[cfg(feature = "__test")] | ||
| pub mod state; | ||
| #[cfg(not(feature = "__test"))] | ||
| pub(crate) mod state; | ||
| mod worker; | ||
|
|
||
| #[cfg(feature = "__test")] | ||
| pub use self::client::read_pipe; | ||
| pub use self::client::{Content, DataControl, TransferRequest}; | ||
| pub use self::error::{Error, Result}; | ||
| pub use self::mime::find_mime_match; | ||
| pub use self::options::{Options, Preference, Protocol}; |
There was a problem hiding this comment.
[skeptical] Divergent duplicate of the shared data-control client from #2055 — medium 🟠 — The PR builds on the data-control client extracted into ironrdp-cliprdr-native by #2055, but ships its own client.rs, state.rs, worker.rs and dispatch.rs. The PR conversation confirms the head predates #2055's 2026-10-06 updates to those exact files, which conflict by design: a single macro-generated dispatch pair, private-MIME self-selection detection replacing is_own_selection, Arc<[u8]> cached data, aligned paste caps/deadlines, and no Synchronize/read_pipe. Merging d826491 as-is creates a second divergent implementation of a module published on the 0.7 crate, with unclear ownership; a rebase onto #2055 is required before merge.
| /// Add data for a type after [`set_selection`](Self::set_selection), | ||
| /// without replacing the selection. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// [`Error::Stopped`] if the worker has shut down. | ||
| pub fn update_data(&self, mime_type: impl Into<String>, data: impl Into<Vec<u8>>) -> Result<()> { | ||
| self.link.send(Command::UpdateSourceData { | ||
| mime_type: mime_type.into(), | ||
| data: data.into(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
[skeptical] update_data and its command plumbing are dead code for a removed flow — low 🟡 — DataControl::update_data, Command::UpdateSourceData (state.rs 152), and State::update_source_data (state.rs 513, called only from worker.rs 225) have no caller anywhere in the head tree: the Linux backend advertises formats without data and serves pastes via TransferRequest/CompleteTransfer. The path exists for the eager-fetch flow the PR removed; delete the method, command variant, and state method rather than carry untested public plumbing.
| /// Whether a selection event reports the selection this client set itself. | ||
| /// | ||
| /// Another client taking the selection cancels our source first, so while | ||
| /// our source is live the selection is still ours. The offered MIME set must | ||
| /// also match what we advertised: that keeps a foreign copy from being | ||
| /// swallowed on a compositor that delivers the selection before the cancel. | ||
| #[cfg_attr(feature = "__test", visibility::make(pub))] | ||
| pub(crate) fn is_own_selection(source_live: bool, advertised: &[String], offered: &[String]) -> bool { | ||
| if !source_live || advertised.len() != offered.len() { | ||
| return false; | ||
| } | ||
| offered.iter().all(|mime| advertised.contains(mime)) | ||
| } |
There was a problem hiding this comment.
[skeptical] MIME-set equality cannot distinguish a foreign same-format copy from our echo — low 🟡 — is_own_selection treats any selection event as our own while our source is live and the offered MIME set matches what we advertised. A clipboard manager or another client copying the same type set (common right after we publish remote content) is misclassified: on_change is suppressed and shared.own_source_live stays true for a selection we no longer own until the compositor's cancelled event republishes, and render_local refuses with 'local clipboard selection was replaced' in the interim. #2055 replaces the heuristic with an unambiguous private-MIME marker; adopt that rather than shipping the weaker heuristic.
| // === ext-data-control-v1 === | ||
|
|
||
| impl Dispatch<ExtDataControlManagerV1, ()> for Client { | ||
| fn event( | ||
| _state: &mut Self, | ||
| _proxy: &ExtDataControlManagerV1, | ||
| _event: <ExtDataControlManagerV1 as wayland_client::Proxy>::Event, | ||
| _data: &(), | ||
| _conn: &Connection, | ||
| _qh: &QueueHandle<Self>, | ||
| ) { | ||
| // The manager has no events. | ||
| } | ||
| } | ||
|
|
||
| impl Dispatch<ExtDataControlDeviceV1, ()> for Client { | ||
| fn event( | ||
| state: &mut Self, | ||
| _proxy: &ExtDataControlDeviceV1, | ||
| event: <ExtDataControlDeviceV1 as wayland_client::Proxy>::Event, | ||
| _data: &(), | ||
| _conn: &Connection, | ||
| _qh: &QueueHandle<Self>, | ||
| ) { | ||
| match event { | ||
| ext_data_control_device_v1::Event::DataOffer { id } => { | ||
| state.data_control.on_data_offer_ext(id); | ||
| } | ||
| ext_data_control_device_v1::Event::Selection { id } => { | ||
| if id.is_some() { | ||
| state.data_control.on_selection(); | ||
| } else { | ||
| state.data_control.on_selection_cleared(); | ||
| } | ||
| } | ||
| ext_data_control_device_v1::Event::Finished => { | ||
| state.data_control.on_device_finished(); | ||
| } | ||
| ext_data_control_device_v1::Event::PrimarySelection { .. } => { | ||
| // Only the regular clipboard is handled, not the primary selection. | ||
| tracing::trace!("ext data control primary selection event (ignored)"); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| // The `data_offer` event creates a child offer object; without this, | ||
| // wayland-client's default panics. | ||
| wayland_client::event_created_child!(Client, ExtDataControlDeviceV1, [ | ||
| ext_data_control_device_v1::EVT_DATA_OFFER_OPCODE => (ExtDataControlOfferV1, ()), | ||
| ]); | ||
| } | ||
|
|
||
| impl Dispatch<ExtDataControlSourceV1, ()> for Client { | ||
| fn event( | ||
| state: &mut Self, | ||
| proxy: &ExtDataControlSourceV1, | ||
| event: <ExtDataControlSourceV1 as wayland_client::Proxy>::Event, | ||
| _data: &(), | ||
| _conn: &Connection, | ||
| _qh: &QueueHandle<Self>, | ||
| ) { | ||
| if !state.data_control.is_current_source(&proxy.id()) { | ||
| return; | ||
| } | ||
| match event { | ||
| ext_data_control_source_v1::Event::Send { mime_type, fd } => { | ||
| state.data_control.on_source_send(&mime_type, fd); | ||
| } | ||
| ext_data_control_source_v1::Event::Cancelled => { | ||
| state.data_control.on_source_cancelled(); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Dispatch<ExtDataControlOfferV1, ()> for Client { | ||
| fn event( | ||
| state: &mut Self, | ||
| _proxy: &ExtDataControlOfferV1, | ||
| event: <ExtDataControlOfferV1 as wayland_client::Proxy>::Event, | ||
| _data: &(), | ||
| _conn: &Connection, | ||
| _qh: &QueueHandle<Self>, | ||
| ) { | ||
| if let ext_data_control_offer_v1::Event::Offer { mime_type } = event { | ||
| state.data_control.on_offer_mime_type(mime_type); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // === wlr-data-control-unstable-v1 === | ||
|
|
||
| impl Dispatch<ZwlrDataControlManagerV1, ()> for Client { | ||
| fn event( | ||
| _state: &mut Self, | ||
| _proxy: &ZwlrDataControlManagerV1, | ||
| _event: <ZwlrDataControlManagerV1 as wayland_client::Proxy>::Event, | ||
| _data: &(), | ||
| _conn: &Connection, | ||
| _qh: &QueueHandle<Self>, | ||
| ) { | ||
| // The manager has no events. | ||
| } | ||
| } | ||
|
|
||
| impl Dispatch<ZwlrDataControlDeviceV1, ()> for Client { | ||
| fn event( | ||
| state: &mut Self, | ||
| _proxy: &ZwlrDataControlDeviceV1, | ||
| event: <ZwlrDataControlDeviceV1 as wayland_client::Proxy>::Event, | ||
| _data: &(), | ||
| _conn: &Connection, | ||
| _qh: &QueueHandle<Self>, | ||
| ) { | ||
| match event { | ||
| zwlr_data_control_device_v1::Event::DataOffer { id } => { | ||
| state.data_control.on_data_offer_wlr(id); | ||
| } | ||
| zwlr_data_control_device_v1::Event::Selection { id } => { | ||
| if id.is_some() { | ||
| state.data_control.on_selection(); | ||
| } else { | ||
| state.data_control.on_selection_cleared(); | ||
| } | ||
| } | ||
| zwlr_data_control_device_v1::Event::Finished => { | ||
| state.data_control.on_device_finished(); | ||
| } | ||
| zwlr_data_control_device_v1::Event::PrimarySelection { .. } => { | ||
| tracing::trace!("wlr data control primary selection event (ignored)"); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| wayland_client::event_created_child!(Client, ZwlrDataControlDeviceV1, [ | ||
| zwlr_data_control_device_v1::EVT_DATA_OFFER_OPCODE => (ZwlrDataControlOfferV1, ()), | ||
| ]); | ||
| } | ||
|
|
||
| impl Dispatch<ZwlrDataControlSourceV1, ()> for Client { | ||
| fn event( | ||
| state: &mut Self, | ||
| proxy: &ZwlrDataControlSourceV1, | ||
| event: <ZwlrDataControlSourceV1 as wayland_client::Proxy>::Event, | ||
| _data: &(), | ||
| _conn: &Connection, | ||
| _qh: &QueueHandle<Self>, | ||
| ) { | ||
| if !state.data_control.is_current_source(&proxy.id()) { | ||
| return; | ||
| } | ||
| match event { | ||
| zwlr_data_control_source_v1::Event::Send { mime_type, fd } => { | ||
| state.data_control.on_source_send(&mime_type, fd); | ||
| } | ||
| zwlr_data_control_source_v1::Event::Cancelled => { | ||
| state.data_control.on_source_cancelled(); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Dispatch<ZwlrDataControlOfferV1, ()> for Client { | ||
| fn event( | ||
| state: &mut Self, | ||
| _proxy: &ZwlrDataControlOfferV1, | ||
| event: <ZwlrDataControlOfferV1 as wayland_client::Proxy>::Event, | ||
| _data: &(), | ||
| _conn: &Connection, | ||
| _qh: &QueueHandle<Self>, | ||
| ) { | ||
| if let zwlr_data_control_offer_v1::Event::Offer { mime_type } = event { | ||
| state.data_control.on_offer_mime_type(mime_type); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[code-compressor] ext and wlr dispatch handlers are written out twice by hand — medium 🟠 — The ExtDataControl* and ZwlrDataControl* Dispatch impl blocks (lines 60-148 vs 152-237) are line-for-line identical apart from type names, forwarding to the same State methods; the module doc itself admits each handler 'is written twice'. A single macro_rules! parameterized over (proxy type, event module, event enum) can generate the four event-bearing impls plus the event_created_child! invocations, roughly halving the file and guaranteeing the two protocols stay in lockstep. No behavior change.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| #[non_exhaustive] | ||
| pub enum Preference { | ||
| /// Prefer `ext-data-control-v1`, then `wlr-data-control-unstable-v1`. | ||
| #[default] | ||
| Auto, | ||
| /// Prefer `ext-data-control-v1`. | ||
| Ext, | ||
| /// Prefer `wlr-data-control-unstable-v1`. | ||
| Wlr, | ||
| } | ||
|
|
There was a problem hiding this comment.
[code-compressor] Preference::Ext is behaviorally identical to Auto — low 🟡 — candidates() (options.rs 90-100) maps Preference::Auto and Preference::Ext to the same (Ext, Wlr) ordering, and allow_fallback — not the preference — decides whether the second candidate exists. Ext adds a third public state indistinguishable from Auto. Dropping the variant (or Auto) removes speculative generality from a brand-new public API; a true ext-only mode would be preference(Wlr-style) plus allow_fallback(false).
| // An OS event queued before offer() acknowledged our ownership | ||
| // must not invalidate the selection that was just installed. | ||
| if self.os.is_owner() { | ||
| return; | ||
| } | ||
| self.invalidate(); | ||
| self.owns_remote = false; | ||
| self.remote.clear(); | ||
| self.local_mimes = self.os.mime_types(); | ||
| if self.ready { | ||
| self.advertise_local(); | ||
| } | ||
| } | ||
| Command::RemoteCopy(formats) => { | ||
| self.invalidate(); | ||
| self.remote = formats; | ||
| self.owns_remote = true; | ||
| let mut mimes = Vec::new(); | ||
| if self.remote.iter().any(|f| f.id() == ClipboardFormatId::CF_UNICODETEXT) { | ||
| mimes.extend([TEXT.to_owned(), "text/plain".to_owned()]); | ||
| } | ||
| if self | ||
| .remote | ||
| .iter() | ||
| .any(|f| matches!(f.id(), ClipboardFormatId::CF_DIB | ClipboardFormatId::CF_DIBV5)) | ||
| { | ||
| mimes.push(PNG.to_owned()); | ||
| } | ||
| if let Err(error) = self.os.offer(&mimes, self.generation) { | ||
| warn!(%error, "Could not advertise the remote clipboard"); | ||
| let _ = self.os.clear(); | ||
| self.owns_remote = false; | ||
| self.remote.clear(); | ||
| self.local_mimes = self.os.mime_types(); | ||
| if self.ready { | ||
| self.advertise_local(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[code-compressor] Local-selection refresh sequence repeated in three handlers — low 🟡 — Command::LocalChanged (107-112), the failure path of Command::RemoteCopy (132-137), and Command::Reset (151-152 area) each repeat owns_remote=false, remote.clear(), local_mimes = os.mime_types(), and advertise_local() when ready. A single fn revert_to_local(&mut self) capturing that sequence shrinks all three handlers and prevents one from drifting (e.g. forgetting the ready gate). Behavior is preserved: Reset keeps its extra pending/cache/loop-detector clearing and LocalChanged keeps its is_owner early return and invalidate().
| request.target | ||
| } else { | ||
| request.property | ||
| }; | ||
| let Some(pending) = self.pending_pastes.remove(&(request.requestor, property)) else { | ||
| return Ok(()); | ||
| }; | ||
| if pending.expired { | ||
| return Ok(()); | ||
| } | ||
| if generation != self.generation || !self.owns { | ||
| self.notify(request, NONE)?; | ||
| } else if let Some(data) = data.filter(|d| d.len() <= MAX_TRANSFER_BYTES) { | ||
| let property = if request.property == NONE { | ||
| request.target | ||
| } else { | ||
| request.property | ||
| }; | ||
| if data.len() <= CHUNK_BYTES { |
There was a problem hiding this comment.
[code-compressor] Effective-property expression computed three times — low 🟡 — Request::Complete computes 'if request.property == NONE { request.target } else { request.property }' at lines 359-363 for the pending_pastes key, then shadows it with the identical expression at 373-377 inside the data branch; selection_request computes the same expression a third time at 692-696. Delete the inner recomputation and add a small fn effective_property(&SelectionRequestEvent) -> Atom used by both sites. All three expressions evaluate to the same atom for the same request, so this is a pure refactor.
| self.set_pending_offer(DataControlOffer::Ext(offer)); | ||
| } | ||
|
|
||
| /// Handle a `data_offer` event from the device (wlr variant). | ||
| pub(crate) fn on_data_offer_wlr(&mut self, offer: ZwlrDataControlOfferV1) { | ||
| self.set_pending_offer(DataControlOffer::Wlr(offer)); | ||
| } | ||
|
|
There was a problem hiding this comment.
[code-compressor] on_data_offer_ext and on_data_offer_wlr are the same one-line body — low 🟡 — Both methods exist only to wrap their protocol's offer in DataControlOffer (which already has Ext and Wlr variants) and call set_pending_offer. A single pub(crate) fn on_data_offer(offer: impl Into<DataControlOffer>) with From impls for both proxy types replaces the pair and removes one protocol-specific seam from State. Behavior is unchanged; the dispatch callers just pass the typed offer through.
github-actions
Bot
added
ai-reviewed/2
This adds the data-control clipboard client I committed to on #2016. It lives in ironrdp-cliprdr-native as the data_control module, so a Linux CLIPRDR backend can be built on it. The client speaks ext-data-control-v1 and wlr-data-control-unstable-v1 and uses whichever the compositor offers. One thread owns the Wayland connection and DataControl is the handle to call from anywhere. It has no async runtime. It supports delayed rendering. A type advertised without data raises a TransferRequest when something pastes it, which maps onto the CLIPRDR Format Data Response. Dropping the request without answering it closes the paste with no data. A selection this client set itself isn't reported back as a local copy, so a backend doesn't echo its own clipboard to the server. The client tells its own selection from another client's copy by one private MIME type that it advertises next to the real ones, so other clients see that type in the list they are offered. Reads are capped at 100 MiB and fail with a timeout if the source stalls for 5 seconds. Pastes are bounded too. At most 16 are served at once, a paste whose data isn't supplied within 5 seconds is closed with no data (an answer that comes later is kept for the next paste), and a reader that takes nothing for 5 seconds is dropped. MIME matching tolerates charset parameters. GNOME's Mutter offers no data-control protocol, so connecting returns an Unsupported error there.
|
#2055 merged on 2026-10-08 as a1efb0b, so the data-control client I said this PR would build on is on master now. The first commit here, d57e221, is an older copy of it from before review. Git can't drop it on its own, since master has the squashed, reviewed version. When you rebase, drop that commit and take data_control/ from master. The calls your backend makes into it all exist on master under the same names: DataControl::connect, on_change, owns_selection, selection_mime_types, serial, on_transfer, set_selection and clear_selection, Content::new and advertise, and find_mime_match. I compared names only, not signatures, and I haven't built this branch on master. Your own edits to data_control/ in the other three commits overlap with what merged, as I described in my note on 10-06. Synchronize, read_pipe and the total read deadline aren't on master, so those are yours to keep here or propose against the module. Six of the nine open review threads here are on data_control/ files, which come from master after the rebase. |
Adds native Linux clipboard redirection for Unicode text and images, using the shared Wayland data-control client from #2055 and an X11/XFixes fallback. This PR is stacked on #2055 and current master.
Remote format lists are advertised immediately; contents are requested only when a local application pastes. Local selection changes are event-driven, and local content is read only when the RDP peer requests it. The backend uses the existing clipboard loop detector and bounded bitmap converters, removing the polling arboard backend, direct PNG dependency, and duplicate image codec.
CLIPRDR responses have no request identifier. Only one Format Data Request is outstanding at a time: a timed-out OS paste is released, while the wire request remains reserved until its late response is drained. Selection generations reject obsolete data. If the peer never answers, remote pastes fail until channel reinitialization instead of applying stale content to a later copy.
Wayland and X11 bound pending transfers and use absolute deadlines. X11 supports INCR for large content and isolates reads with separate requestor windows. Ownership acknowledgments and current-owner checks prevent queued events from replacing a newer remote selection. The localized data-control changes also bound held descriptors and active writers and ignore events from replaced sources.
File transfer and HTML are not implemented. Desktops without data-control use X11/XWayland when available, otherwise the existing stub backend.
Validation
helper,__bench, locked dependencies, and warnings denied passes. Workspace formatting, changed-file typo checks, and diff checks pass.