Implement full RemoteDesktop portal with consent dialog and EIS#272
Implement full RemoteDesktop portal with consent dialog and EIS#272olafkfreund wants to merge 1 commit intopop-os:remote-desktopfrom
Conversation
Build on the WIP skeleton to add: - Complete session lifecycle (create, select_devices, select_sources, start) - Consent dialog UI with device type chips and screen/window selection - EIS socket pair creation and compositor forwarding via D-Bus - Integration with ScreenCast infrastructure for video streams - ConnectToEIS method returning client fd to requesting application - Security: always show consent dialog, never skip via restore data - i18n strings for remote desktop dialog Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements the RemoteDesktop portal end-to-end, adding a consent dialog UI, wiring portal events through the app/subscription layers, and integrating screencast streaming plus EIS socket forwarding to the compositor.
Changes:
- Adds a new RemoteDesktop consent dialog (device chips + optional output/window selection) and app wiring to display/cancel it.
- Implements the RemoteDesktop portal D-Bus interface with session lifecycle, stream setup via existing PipeWire/screencast infrastructure, and ConnectToEIS fd handoff.
- Refactors screencast
StreamPropsto add a reusable constructor and adds i18n strings for the new dialog.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/remote_desktop.rs |
Implements RemoteDesktop portal methods, session state, screencast thread creation, and EIS socket forwarding. |
src/remote_desktop_dialog.rs |
New consent dialog UI and selection handling for devices and optional capture sources. |
src/app.rs |
Adds RemoteDesktop dialog state, message routing, and window view mapping. |
src/subscription.rs |
Adds RemoteDesktop events and registers the new RemoteDesktop portal implementation on D-Bus. |
src/screencast.rs |
Adds StreamProps::new() for shared use by ScreenCast and RemoteDesktop. |
src/main.rs |
Declares the new remote_desktop_dialog module. |
i18n/en/xdg_desktop_portal_cosmic.ftl |
Adds English strings for the Remote Desktop consent dialog. |
Cargo.toml |
Adds the reis dependency (tokio feature). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let Some(capture_sources) = resp else { | ||
| log::info!("Remote desktop access denied by user"); | ||
| return PortalResponse::Cancelled; | ||
| }; | ||
| capture_sources |
There was a problem hiding this comment.
show_remote_desktop_prompt() returns None both when the user denies consent and when the UI request cannot be delivered (e.g. subscription_tx.send(...) fails). In start(), None is always treated as a user denial and converted to PortalResponse::Cancelled with a log message indicating the user denied it, which can mask real internal failures. Consider distinguishing these cases (e.g., return Result<Option<_>, _> from show_remote_desktop_prompt and map send/channel failures to PortalResponse::Other).
| let Some(capture_sources) = resp else { | |
| log::info!("Remote desktop access denied by user"); | |
| return PortalResponse::Cancelled; | |
| }; | |
| capture_sources | |
| match resp { | |
| Ok(Some(capture_sources)) => capture_sources, | |
| Ok(None) => { | |
| log::info!("Remote desktop access not granted (likely denied by user)"); | |
| return PortalResponse::Cancelled; | |
| } | |
| Err(e) => { | |
| log::error!("Failed to show remote desktop prompt: {e}"); | |
| return PortalResponse::Other; | |
| } | |
| } |
| |key, _| match key { | ||
| Key::Named(Named::Enter) => Some(Msg::Allow), | ||
| Key::Named(Named::Escape) => Some(Msg::Deny), | ||
| _ => None, | ||
| }, |
There was a problem hiding this comment.
The Enter key binding always maps to Msg::Allow, even when the primary Allow button is intentionally disabled (e.g., screencast included but no output/window selected). Since update_msg doesn’t validate capture_sources before allowing, pressing Enter can bypass the selection requirement and proceed with an empty stream list. Consider gating the key handler the same way as the button (or enforcing the check in Msg::Allow handling).
| // Create EIS socket pair for input injection | ||
| let eis_client_fd = match create_eis_socket_pair() { | ||
| Ok((server_fd, client_fd)) => { | ||
| // Forward the server-side fd to the compositor via D-Bus | ||
| // Must use the portal's own connection so the compositor's | ||
| // auth check sees the well-known name owner match. | ||
| if let Err(err) = send_eis_to_compositor(connection, server_fd).await { | ||
| log::warn!("Failed to send EIS socket to compositor: {}", err); | ||
| } | ||
| Some(client_fd) | ||
| } | ||
| Err(err) => { | ||
| log::error!("Failed to create EIS socket pair: {}", err); | ||
| None | ||
| } | ||
| }; |
There was a problem hiding this comment.
start() creates an EIS socket pair and returns the client fd even if forwarding the server fd to the compositor fails. That will hand the requesting app a socket that is never serviced, likely causing hangs or confusing failures. Consider treating send_eis_to_compositor(...) failure as fatal (return PortalResponse::Other/Cancelled) or only storing/returning the client fd after the compositor successfully accepts the server fd.
| session_data.source_types = | ||
| BitFlags::from_bits_truncate(options.types.unwrap_or(0)); |
There was a problem hiding this comment.
select_sources() stores source_types directly from options.types via from_bits_truncate without masking to the supported set (monitor/window). If a client requests only an unsupported bit (e.g. virtual), the consent UI will end up with no selectable tabs but include_screencast remains true, effectively blocking the user from proceeding. Consider masking options.types to the supported bits and defaulting to Monitor (or returning an error) when the result is empty.
| session_data.source_types = | |
| BitFlags::from_bits_truncate(options.types.unwrap_or(0)); | |
| // Mask to supported source types only (Monitor and Window). | |
| let supported_sources = SourceType::Monitor | SourceType::Window; | |
| let supported_bits = supported_sources.bits(); | |
| let raw_types = options.types.unwrap_or(0) & supported_bits; | |
| session_data.source_types = BitFlags::from_bits_truncate(raw_types); |
| }; | ||
|
|
||
| let outputs = self.wayland_helper.outputs(); | ||
| if outputs.is_empty() { |
There was a problem hiding this comment.
start() returns PortalResponse::Other when wayland_helper.outputs() is empty, even though the dialog and logic explicitly support input-only RemoteDesktop sessions (include_screencast can be false and no outputs need to be selected). Consider only requiring outputs when screen/window capture is requested, or allowing input-only sessions to proceed without this check.
| if outputs.is_empty() { | |
| if !source_types.is_empty() && outputs.is_empty() { |
| async fn available_device_types(&self) -> u32 { | ||
| 7 // XXX | ||
| fn available_device_types(&self) -> u32 { | ||
| DEVICE_KEYBOARD | DEVICE_POINTER |
There was a problem hiding this comment.
available_device_types currently reports only keyboard and pointer, but the implementation and consent UI handle DEVICE_TOUCHSCREEN as well. This mismatch can prevent clients from requesting touchscreen support (or misrepresent portal capabilities). Consider either including DEVICE_TOUCHSCREEN in the reported bitmask or removing touchscreen handling if it’s not actually supported.
| DEVICE_KEYBOARD | DEVICE_POINTER | |
| DEVICE_KEYBOARD | DEVICE_POINTER | DEVICE_TOUCHSCREEN |
| } | ||
| if device_types & 4 != 0 { | ||
| items.push(device_chip( | ||
| "input-touchpad-symbolic", |
There was a problem hiding this comment.
The touchscreen permission chip uses the input-touchpad-symbolic icon, which represents a touchpad rather than a touchscreen. This makes the dialog misleading; consider switching to the appropriate touchscreen icon name (and/or falling back gracefully if unavailable).
| "input-touchpad-symbolic", | |
| "input-touchscreen-symbolic", |
| source_types: BitFlags<SourceType>, | ||
| persisted_capture_sources: Option<PersistedCaptureSources>, | ||
| eis_socket_client: Option<zvariant::OwnedFd>, | ||
| closed: bool, |
There was a problem hiding this comment.
persisted_capture_sources (and the associated RestoreData/PersistedCaptureSources handling) is currently only written in select_devices/select_sources and never read in start(). This makes restore-data handling effectively a no-op and adds dead code/complexity. Consider either wiring it up (e.g., to preselect sources in the consent UI) or removing it until it’s needed.
|
I have two computers that I use Synergy on, my laptop I upgraded to pop-os 24.04 and my desktop which is on pop-os 22.04. I built and installed this PR on my laptop. Looking at the logs in Synergy, it appears to be close to working, however the mouse and keyboard do not actually make the switch over. I hope this is somewhat useful, I'm happy to grab any other logs that might be useful or test fixes if needed. I can work around for now by using gnome which works as expected. These are synergy logs from the desktop side which is the computer that has the mouse and keyboard physically attached. |
Summary
Builds on @ids1024's WIP skeleton (
d26d88c) to implement the full RemoteDesktop portal:CreateSession,SelectDevices,SelectSources,Start,ConnectToEISwith proper session state managementcom.system76.CosmicComp.RemoteDesktopvia D-Bus, client fd returned to requesting applicationScreencastThreadand PipeWire infrastructure for video streams; addsStreamProps::new()constructor for shared useFiles changed
src/remote_desktop.rssrc/remote_desktop_dialog.rssrc/app.rssrc/subscription.rssrc/screencast.rsStreamProps::new()constructor for reusesrc/main.rsCargo.tomlreisdependencyi18n/en/*.ftlTest plan
cargo buildCancelledresponse🤖 Generated with Claude Code