From 34bf07db05e4cec036a74d13c9184084b784c418 Mon Sep 17 00:00:00 2001 From: En-En <39373446+En-En-Code@users.noreply.github.com> Date: Mon, 13 Oct 2025 20:38:20 +0000 Subject: [PATCH] fix: give clipboard/primary TARGETS unique atoms The clipboard and primary selections previously used the same atom to store TARGETS when queried for them by `handle_target_list`. When a program requests both selections simultaneously, problems such as one selection using the target list of the other, then deleting it and requiring the second target to get the selection targets again could occur. This was observed when using `wl-clip-persist` and could result in selection desyncs. This change gives a unique atom to each primary and clipboard selection targets so they no longer compete over the same property. It also adds updates to the `copy_from_x11` intergration to test for this issue. --- src/xstate/mod.rs | 3 +- src/xstate/selection.rs | 10 +++-- tests/integration.rs | 98 ++++++++++++++++++++++++++--------------- 3 files changed, 70 insertions(+), 41 deletions(-) diff --git a/src/xstate/mod.rs b/src/xstate/mod.rs index 0265b9c..1d69b50 100644 --- a/src/xstate/mod.rs +++ b/src/xstate/mod.rs @@ -998,15 +998,16 @@ xcb::atoms_struct! { motif_wm_hints => b"_MOTIF_WM_HINTS" only_if_exists = false, utf8_string => b"UTF8_STRING" only_if_exists = false, clipboard => b"CLIPBOARD" only_if_exists = false, + clipboard_targets => b"_clipboard_targets" only_if_exists = false, targets => b"TARGETS" only_if_exists = false, save_targets => b"SAVE_TARGETS" only_if_exists = false, multiple => b"MULTIPLE" only_if_exists = false, timestamp => b"TIMESTAMP" only_if_exists = false, - selection_reply => b"_selection_reply" only_if_exists = false, incr => b"INCR" only_if_exists = false, xsettings => b"_XSETTINGS_S0" only_if_exists = false, xsettings_settings => b"_XSETTINGS_SETTINGS" only_if_exists = false, primary => b"PRIMARY" only_if_exists = false, + primary_targets => b"_primary_targets" only_if_exists = false, moveresize => b"_NET_WM_MOVERESIZE" only_if_exists = false, } } diff --git a/src/xstate/selection.rs b/src/xstate/selection.rs index 865c6f8..f2cdbee 100644 --- a/src/xstate/selection.rs +++ b/src/xstate/selection.rs @@ -233,6 +233,7 @@ enum CurrentSelection { struct SelectionData { last_selection_timestamp: u32, atom: x::Atom, + targets_atom: x::Atom, current_selection: Option>, } @@ -269,10 +270,11 @@ trait SelectionDataImpl { } impl SelectionData { - fn new(atom: x::Atom) -> Self { + fn new(atom: x::Atom, targets_atom: x::Atom) -> Self { Self { last_selection_timestamp: x::CURRENT_TIME, atom, + targets_atom, current_selection: None, } } @@ -336,7 +338,7 @@ impl SelectionDataImpl for SelectionData { requestor: wm_window, selection: self.atom, target: atoms.targets, - property: atoms.selection_reply, + property: self.targets_atom, time: timestamp, }) { Ok(_) => { @@ -571,8 +573,8 @@ impl SelectionState { .expect("Couldn't create window for selections"); Self { target_window, - clipboard: SelectionData::new(atoms.clipboard), - primary: SelectionData::new(atoms.primary), + clipboard: SelectionData::new(atoms.clipboard, atoms.clipboard_targets), + primary: SelectionData::new(atoms.primary, atoms.primary_targets), } } } diff --git a/tests/integration.rs b/tests/integration.rs index 8a6f24c..93b3f18 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -994,17 +994,21 @@ fn copy_from_x11() { let window = connection.new_window(connection.root, 0, 0, 20, 20, false); f.map_as_toplevel(&mut connection, window); + + connection.set_selection_owner(window, connection.atoms.primary); connection.set_selection_owner(window, connection.atoms.clipboard); - let request = connection.await_selection_request(); - assert_eq!(request.target(), connection.atoms.targets); - connection.set_property( - request.requestor(), - x::ATOM_ATOM, - request.property(), - &[connection.atoms.mime1, connection.atoms.mime2], - ); - connection.send_selection_notify(&request); + for _ in [connection.atoms.primary, connection.atoms.clipboard] { + let request = connection.await_selection_request(); + assert_eq!(request.target(), connection.atoms.targets); + connection.set_property( + request.requestor(), + x::ATOM_ATOM, + request.property(), + &[connection.atoms.mime1, connection.atoms.mime2], + ); + connection.send_selection_notify(&request); + } f.wait_and_dispatch(); struct MimeData { @@ -1028,21 +1032,37 @@ fn copy_from_x11() { }, ]; - let advertised_mimes = f.testwl.data_source_mimes(); + // When requesting both primary and clipboard simultaneously, the first to be requested erases + // its TARGETS (which is the correct behavior of a GetProperty request), but the second still + // tried to use this data and would come up empty. + let primary_mimes = f.testwl.primary_source_mimes(); + let clipboard_mimes = f.testwl.data_source_mimes(); assert_eq!( - advertised_mimes.len(), + primary_mimes.len(), mimes_truth.len(), - "Wrong number of advertised mimes: {advertised_mimes:?}" + "Wrong number of advertised primary mimes: {primary_mimes:?}" + ); + assert_eq!( + clipboard_mimes.len(), + mimes_truth.len(), + "Wrong number of advertised clipboard mimes: {clipboard_mimes:?}" ); for MimeData { data, .. } in &mimes_truth { assert!( - advertised_mimes.contains(&data.mime_type), + primary_mimes.contains(&data.mime_type), + "Missing mime type {}", + data.mime_type + ); + assert!( + clipboard_mimes.contains(&data.mime_type), "Missing mime type {}", data.mime_type ); } - let data = f.testwl.clipboard_paste_data(|mime, _| { + // Type annotations hint the compiler to use HRTBs (needed since this closure is reused). + // See: https://users.rust-lang.org/t/implementation-of-fnonce-is-not-general-enough/68294/3 + let mut send_data_for_mime = |mime: &str, _: &mut testwl::Server| { let request = connection.await_selection_request(); let data = mimes_truth .iter() @@ -1056,29 +1076,35 @@ fn copy_from_x11() { ); connection.send_selection_notify(&request); true - }); - let mut found_mimes = Vec::new(); - for testwl::PasteData { mime_type, data } in data { - match &mime_type { - x if x == "text/plain" => { - assert_eq!(&data, b"hello world"); - } - x if x == "blah/blah" => { - assert_eq!(&data, &[1, 2, 3, 4]); - } - other => panic!("unexpected mime type: {other} ({data:?})"), - } - found_mimes.push(mime_type); - } + }; - assert!( - found_mimes.contains(&"text/plain".to_string()), - "Didn't get mime data for text/plain" - ); - assert!( - found_mimes.contains(&"blah/blah".to_string()), - "Didn't get mime data for blah/blah" - ); + let clipboard_data = f.testwl.clipboard_paste_data(&mut send_data_for_mime); + let primary_data = f.testwl.primary_paste_data(&mut send_data_for_mime); + + for data in [primary_data, clipboard_data] { + let mut found_mimes = Vec::new(); + for testwl::PasteData { mime_type, data } in data { + match &mime_type { + x if x == "text/plain" => { + assert_eq!(&data, b"hello world"); + } + x if x == "blah/blah" => { + assert_eq!(&data, &[1, 2, 3, 4]); + } + other => panic!("unexpected mime type: {other} ({data:?})"), + } + found_mimes.push(mime_type); + } + + assert!( + found_mimes.contains(&"text/plain".to_string()), + "Didn't get mime data for text/plain" + ); + assert!( + found_mimes.contains(&"blah/blah".to_string()), + "Didn't get mime data for blah/blah" + ); + } } #[test]