Don't use MULTIPLE target atom for getting selections

The ICCCM claims this is a "required" target for selection owners,
however several GTK clients (zenity, winecfg) don't seem to support it.
(So much for required.) Just manually grab all the supported targets
individually from the selection owner instead.
Fix for #50

src/xstate/mod.rs

@@ -4,7 +4,7 @@ use selection::{SelectionData, SelectionTarget};
 use crate::server::WindowAttributes;
 use bitflags::bitflags;
 use log::{debug, trace, warn};
-use std::ffi::{CString, CStr};
+use std::ffi::CString;
 use std::os::fd::{AsRawFd, BorrowedFd};
 use std::sync::Arc;
 use xcb::{x, Xid, XidNew};
@@ -405,16 +405,6 @@ impl XState {
         }
     }
 
-    fn get_atom_name(&self, atom: x::Atom) -> String {
-        match self
-            .connection
-            .wait_for_reply(self.connection.send_request(&x::GetAtomName { atom }))
-        {
-            Ok(reply) => reply.name().to_string(),
-            Err(err) => format!("<error getting atom name: {err:?}> {atom:?}"),
-        }
-    }
-
     fn get_window_attributes(&self, window: x::Window) -> XResult<WindowAttributes> {
         let geometry = self.connection.send_request(&x::GetGeometry {
             drawable: x::Drawable::Window(window),
@@ -637,7 +627,7 @@ impl XState {
                 if log::log_enabled!(log::Level::Debug) {
                     debug!(
                         "changed property {:?} for {:?}",
-                        self.get_atom_name(event.atom()),
+                        get_atom_name(&self.connection, event.atom()),
                         window
                     );
                 }
@@ -665,6 +655,7 @@ xcb::atoms_struct! {
         pub utf8_string => b"UTF8_STRING" only_if_exists = false,
         pub clipboard => b"CLIPBOARD" only_if_exists = false,
         pub targets => b"TARGETS" only_if_exists = false,
+        pub save_targets => b"SAVE_TARGETS" only_if_exists = false,
         pub multiple => b"MULTIPLE" only_if_exists = false,
         pub timestamp => b"TIMESTAMP" only_if_exists = false,
         pub selection_reply => b"_selection_reply" only_if_exists = false,
@@ -864,3 +855,10 @@ impl super::FromServerState<Arc<xcb::Connection>> for Atoms {
         state.atoms.as_ref().unwrap().clone()
     }
 }
+
+fn get_atom_name(connection: &xcb::Connection, atom: x::Atom) -> String {
+    match connection.wait_for_reply(connection.send_request(&x::GetAtomName { atom })) {
+        Ok(reply) => reply.name().to_string(),
+        Err(err) => format!("<error getting atom name: {err:?}> {atom:?}"),
+    }
+}

src/xstate/selection.rs

@@ -1,4 +1,4 @@
-use super::XState;
+use super::{get_atom_name, XState};
 use crate::server::ForeignSelection;
 use crate::{MimeTypeData, RealServerState};
 use log::{debug, trace, warn};
@@ -43,12 +43,28 @@ impl MimeTypeData for SelectionTarget {
     }
 }
 
+enum MimeTypes {
+    Temporary {
+        /// Temporary mime data, being built
+        data: Vec<SelectionTarget>,
+        /// Mime types we still need to receive feedback on
+        /// 2nd field is the destination property
+        to_grab: Vec<(SelectionTargetId, x::Atom)>,
+    },
+    /// Done grabbing mime data
+    Complete(Rc<Vec<SelectionTarget>>),
+}
+
+impl Default for MimeTypes {
+    fn default() -> Self {
+        Self::Complete(Default::default())
+    }
+}
+
 #[derive(Default)]
 pub(crate) struct SelectionData {
     clear_time: Option<u32>,
-    // Selection ID and destination atom
+    mime_types: MimeTypes,
-    tmp_mimes: Vec<(SelectionTargetId, x::Atom)>,
-    mime_types: Rc<Vec<SelectionTarget>>,
     foreign_data: Option<ForeignSelection>,
 }
 
@@ -100,7 +116,7 @@ impl XState {
             })
             .collect();
 
-        self.selection_data.mime_types = Rc::new(types);
+        self.selection_data.mime_types = MimeTypes::Complete(Rc::new(types));
         self.selection_data.foreign_data = Some(selection);
         trace!("Clipboard set from Wayland");
     }
@@ -111,11 +127,13 @@ impl XState {
         server_state: &mut RealServerState,
     ) -> bool {
         match event {
+            // Someone else is the clipboard owner - get the data from them and then reestablish
+            // ourselves as the owner
             xcb::Event::X(x::Event::SelectionClear(e)) => {
                 if e.selection() != self.atoms.clipboard {
                     warn!(
                         "Got SelectionClear for unexpected atom {}, ignoring",
-                        self.get_atom_name(e.selection())
+                        get_atom_name(&self.connection, e.selection())
                     );
                     return true;
                 }
@@ -141,12 +159,17 @@ impl XState {
 
                 match e.target() {
                     x if x == self.atoms.targets => self.handle_target_list(e.property()),
-                    x if x == self.atoms.multiple => self.handle_new_clipboard_data(server_state),
+                    atom => self.handle_clipboard_data(atom),
-                    atom => {
+                }
-                        warn!(
+
-                            "unexpected SelectionNotify type: {}",
+                if let MimeTypes::Temporary { data, to_grab } = &mut self.selection_data.mime_types
-                            self.get_atom_name(atom)
+                {
-                        )
+                    if to_grab.is_empty() {
+                        let data = Rc::new(std::mem::take(data));
+                        self.selection_data.mime_types = MimeTypes::Complete(data.clone());
+                        self.set_clipboard_owner(self.selection_data.clear_time.unwrap());
+                        server_state.set_copy_paste_source(data);
+                        trace!("Clipboard set from X11");
                     }
                 }
             }
@@ -171,7 +194,7 @@ impl XState {
                 let success = || send_notify(e.property());
 
                 if log::log_enabled!(log::Level::Debug) {
-                    let target = self.get_atom_name(e.target());
+                    let target = get_atom_name(&self.connection, e.target());
                     debug!("Got selection request for target {target}");
                 }
 
@@ -181,14 +204,15 @@ impl XState {
                     return true;
                 }
 
+                let MimeTypes::Complete(mime_data) = &self.selection_data.mime_types else {
+                    warn!("Got selection request, but mime data is incomplete");
+                    refuse();
+                    return true;
+                };
+
                 match e.target() {
                     x if x == self.atoms.targets => {
-                        let atoms: Box<[x::Atom]> = self
+                        let atoms: Box<[x::Atom]> = mime_data.iter().map(|t| t.id.atom).collect();
-                            .selection_data
-                            .mime_types
-                            .iter()
-                            .map(|t| t.id.atom)
-                            .collect();
 
                         self.connection
                             .send_and_check_request(&x::ChangeProperty {
@@ -203,14 +227,9 @@ impl XState {
                         success();
                     }
                     other => {
-                        let Some(target) = self
+                        let Some(target) = mime_data.iter().find(|t| t.id.atom == other) else {
-                            .selection_data
-                            .mime_types
-                            .iter()
-                            .find(|t| t.id.atom == other)
-                        else {
                             if log::log_enabled!(log::Level::Debug) {
-                                let name = self.get_atom_name(other);
+                                let name = get_atom_name(&self.connection, other);
                                 debug!("refusing selection request because given atom could not be found ({})", name);
                             }
                             refuse();
@@ -272,80 +291,88 @@ impl XState {
             .unwrap();
 
         let targets: &[x::Atom] = reply.value();
-        let target_props: Box<[x::Atom]> = targets
+        if log::log_enabled!(log::Level::Debug) {
+            let targets_str: Vec<String> = targets
+                .iter()
+                .map(|t| get_atom_name(&self.connection, *t))
+                .collect();
+            debug!("got targets: {targets_str:?}");
+        }
+
+        let to_grab = targets
             .iter()
             .copied()
-            .filter(|atom| ![self.atoms.targets, self.atoms.multiple].contains(atom))
+            .filter(|atom| {
+                ![
+                    self.atoms.targets,
+                    self.atoms.multiple,
+                    self.atoms.save_targets,
+                ]
+                .contains(atom)
+            })
             .enumerate()
-            .flat_map(|(idx, target)| {
+            .map(|(idx, target_atom)| {
-                let name = [b"dest", idx.to_string().as_bytes()].concat();
+                let dest_name = [b"dest", idx.to_string().as_bytes()].concat();
                 let reply = self
                     .connection
                     .wait_for_reply(self.connection.send_request(&x::InternAtom {
-                        name: &name,
+                        name: &dest_name,
                         only_if_exists: false,
                     }))
                     .unwrap();
                 let dest = reply.atom();
 
-                [target, dest]
-            })
-            .collect();
-
-        // Setup target list
-        self.connection
-            .send_and_check_request(&x::ChangeProperty {
-                mode: x::PropMode::Replace,
-                window: self.wm_window,
-                property: self.atoms.selection_reply,
-                r#type: x::ATOM_ATOM,
-                data: &target_props,
-            })
-            .unwrap();
-
-        // Request data for our targets
                 self.connection
                     .send_and_check_request(&x::ConvertSelection {
                         requestor: self.wm_window,
                         selection: self.atoms.clipboard,
-                target: self.atoms.multiple,
+                        target: target_atom,
-                property: self.atoms.selection_reply,
+                        property: dest,
                         time: self.selection_data.clear_time.as_ref().copied().unwrap(),
                     })
                     .unwrap();
 
-        let tmp = target_props
+                let target_name = get_atom_name(&self.connection, target_atom);
-            .chunks_exact(2)
+                (
-            .map(|atoms| {
+                    SelectionTargetId {
-                let [target, property] = atoms.try_into().unwrap();
+                        name: target_name,
-                let name = self
+                        atom: target_atom,
-                    .connection
+                    },
-                    .wait_for_reply(
+                    dest,
-                        self.connection
-                            .send_request(&x::GetAtomName { atom: target }),
                 )
-                    .unwrap();
-                let name = name.name().to_string();
-                let target = SelectionTargetId { atom: target, name };
-                (target, property)
             })
             .collect();
 
-        self.selection_data.tmp_mimes = tmp;
+        self.selection_data.mime_types = MimeTypes::Temporary {
+            to_grab,
+            data: Vec::new(),
+        };
     }
 
-    fn handle_new_clipboard_data(&mut self, server_state: &mut RealServerState) {
+    fn handle_clipboard_data(&mut self, atom: x::Atom) {
-        let mut mime_types = Vec::new();
+        let MimeTypes::Temporary { data, to_grab } = &mut self.selection_data.mime_types else {
-        for (id, dest) in std::mem::take(&mut self.selection_data.tmp_mimes) {
+            warn!("Got selection notify, but not awaiting selection data...");
-            let value = {
+            return;
-                if id.atom == self.atoms.timestamp {
+        };
-                    TargetValue::U32(vec![self
+
+        let Some(idx) = to_grab.iter().position(|(id, _)| id.atom == atom) else {
+            warn!(
+                "unexpected SelectionNotify type: {}",
+                get_atom_name(&self.connection, atom)
+            );
+            return;
+        };
+
+        let (id, dest) = to_grab.swap_remove(idx);
+
+        let value = match atom {
+            x if x == self.atoms.timestamp => TargetValue::U32(vec![self
                 .selection_data
                 .clear_time
                 .as_ref()
                 .copied()
-                        .unwrap()])
+                .unwrap()]),
-                } else {
+            _ => {
                 let reply = self
                     .connection
                     .wait_for_reply(self.connection.send_request(&x::GetProperty {
@@ -364,35 +391,23 @@ impl XState {
                     32 => TargetValue::U32(reply.value().to_vec()),
                     other => {
                         if log::log_enabled!(log::Level::Debug) {
-                                let atom = id.atom;
+                            let target_name = &id.name;
-                                let target = self.get_atom_name(atom);
                             let ty = if reply.r#type() == x::ATOM_NONE {
                                 "None".to_string()
                             } else {
-                                    self.get_atom_name(reply.r#type())
+                                get_atom_name(&self.connection, reply.r#type())
                             };
-                                debug!("unexpected format: {other} (atom: {target}, type: {ty:?}, property: {dest:?})");
+                            let dest = get_atom_name(&self.connection, dest);
+                            let value = reply.value::<u8>().to_vec();
+                            debug!("unexpected format: {other} (atom: {target_name}, type: {ty:?}, property: {dest}, value: {value:?})");
                         }
-                            continue;
+                        return;
                     }
                 }
             }
         };
 
         trace!("Selection data: {id:?} {value:?}");
-            mime_types.push(SelectionTarget { id, value });
+        data.push(SelectionTarget { id, value });
-        }
-
-        self.selection_data.mime_types = Rc::new(mime_types);
-        self.connection
-            .send_and_check_request(&x::DeleteProperty {
-                window: self.wm_window,
-                property: self.atoms.selection_reply,
-            })
-            .unwrap();
-
-        self.set_clipboard_owner(self.selection_data.clear_time.unwrap());
-        server_state.set_copy_paste_source(Rc::clone(&self.selection_data.mime_types));
-        trace!("Clipboard set from X11");
     }
 }

tests/integration.rs

@@ -575,7 +575,7 @@ fn copy_from_x11() {
         .unwrap();
     assert_eq!(window, owner.owner());
 
-    // wait for request to come through
+    // wait for requests to come through
     std::thread::sleep(std::time::Duration::from_millis(100));
     let request = match connection.poll_for_event().unwrap() {
         Some(xcb::Event::X(x::Event::SelectionRequest(r))) => r,
@@ -604,45 +604,28 @@ fn copy_from_x11() {
         })
         .unwrap();
 
-    std::thread::sleep(std::time::Duration::from_millis(100));
+    connection.await_event();
-    let request = match connection.poll_for_event().unwrap() {
+    let mut mime_data = vec![
-        Some(xcb::Event::X(x::Event::SelectionRequest(r))) => r,
+        (
-        other => panic!("Didn't get selection request event, instead got {other:?}"),
+            connection.atoms.mime1,
+            x::ATOM_STRING,
+            b"hello world".as_slice(),
+        ),
+        (connection.atoms.mime2, x::ATOM_INTEGER, &[1u8, 2, 3, 4]),
+    ];
+
+    while let Some(request) = connection.poll_for_event().unwrap() {
+        let xcb::Event::X(x::Event::SelectionRequest(request)) = request else {
+            continue;
         };
 
-    assert_eq!(request.target(), connection.atoms.multiple);
+        let target = request.target();
-    let pairs = connection
+        let Some(idx) = mime_data.iter().position(|(atom, _, _)| *atom == target) else {
-        .wait_for_reply(connection.send_request(&x::GetProperty {
+            panic!("Expected atom in {mime_data:?}, got {target:?}");
-            delete: true,
+        };
-            window: request.requestor(),
-            property: request.property(),
-            r#type: x::ATOM_ATOM,
-            long_offset: 0,
-            long_length: 4,
-        }))
-        .unwrap();
 
-    let pairs: &[x::Atom] = pairs.value();
+        let (_, ty, data) = mime_data.swap_remove(idx);
-    assert_eq!(pairs.len(), 4);
+        connection.set_property(request.requestor(), ty, request.property(), data);
-    assert!(pairs.contains(&connection.atoms.mime1));
-    assert!(pairs.contains(&connection.atoms.mime2));
-
-    let mime1data = b"hello world";
-    let mime2data = &[1u8, 2, 3, 4];
-    for [target, property] in pairs
-        .chunks_exact(2)
-        .map(|pair| <[x::Atom; 2]>::try_from(pair).unwrap())
-    {
-        match target {
-            x if x == connection.atoms.mime1 => {
-                connection.set_property(request.requestor(), x::ATOM_STRING, property, mime1data);
-            }
-            x if x == connection.atoms.mime2 => {
-                connection.set_property(request.requestor(), x::ATOM_INTEGER, property, mime2data);
-            }
-            _ => panic!("unexpected target: {target:?}"),
-        }
-    }
 
         connection
             .send_and_check_request(&x::SendEvent {
@@ -658,7 +641,13 @@ fn copy_from_x11() {
                 ),
             })
             .unwrap();
+        std::thread::sleep(std::time::Duration::from_millis(50));
+    }
 
+    assert!(
+        mime_data.is_empty(),
+        "Didn't get all mime types: {mime_data:?}"
+    );
     f.wait_and_dispatch();
 
     let owner = connection
@@ -684,10 +673,10 @@ fn copy_from_x11() {
     for testwl::PasteData { mime_type, data } in data {
         match mime_type {
             x if x == "text/plain" => {
-                assert_eq!(&data, mime1data);
+                assert_eq!(&data, b"hello world");
             }
             x if x == "blah/blah" => {
-                assert_eq!(&data, mime2data);
+                assert_eq!(&data, &[1, 2, 3, 4]);
             }
             other => panic!("unexpected mime type: {other} ({data:?})"),
         }
@@ -755,7 +744,7 @@ fn copy_from_wayland() {
         })
         .unwrap();
 
-    std::thread::sleep(std::time::Duration::from_millis(50));
+    connection.await_event();
     let request = match connection.poll_for_event().unwrap() {
         Some(xcb::Event::X(x::Event::SelectionNotify(r))) => r,
         other => panic!("Didn't get selection notify event, instead got {other:?}"),
@@ -887,8 +876,7 @@ fn bad_clipboard_data() {
         })
         .unwrap();
 
-    // wait for request to come through
+    connection.await_event();
-    std::thread::sleep(std::time::Duration::from_millis(100));
     let request = match connection.poll_for_event().unwrap() {
         Some(xcb::Event::X(x::Event::SelectionRequest(r))) => r,
         other => panic!("Didn't get selection request event, instead got {other:?}"),
@@ -915,26 +903,14 @@ fn bad_clipboard_data() {
         })
         .unwrap();
 
-    std::thread::sleep(std::time::Duration::from_millis(100));
+    connection.await_event();
     let request = match connection.poll_for_event().unwrap() {
         Some(xcb::Event::X(x::Event::SelectionRequest(r))) => r,
         other => panic!("Didn't get selection request event, instead got {other:?}"),
     };
-    assert_eq!(request.target(), connection.atoms.multiple);
+    assert_eq!(request.target(), connection.atoms.mime2);
-    let pairs = connection
-        .wait_for_reply(connection.send_request(&x::GetProperty {
-            delete: true,
-            window: request.requestor(),
-            property: request.property(),
-            r#type: x::ATOM_ATOM,
-            long_offset: 0,
-            long_length: 4,
-        }))
-        .unwrap();
 
-    let pairs: &[x::Atom] = pairs.value();
+    // Don't actually set any data as requested - just report success
-    assert_eq!(pairs.len(), 2);
-    assert!(pairs.contains(&connection.atoms.mime2));
 
     connection
         .send_and_check_request(&x::SendEvent {
@@ -951,7 +927,7 @@ fn bad_clipboard_data() {
         })
         .unwrap();
 
-    f.wait_and_dispatch();
+    std::thread::sleep(std::time::Duration::from_millis(50));
     let owner = connection
         .wait_for_reply(connection.send_request(&x::GetSelectionOwner {
             selection: connection.atoms.clipboard,
@@ -969,8 +945,7 @@ fn bad_clipboard_data() {
         })
         .unwrap();
 
-    std::thread::sleep(std::time::Duration::from_millis(100));
+    connection.await_event();
-
     let mut e = None;
     while let Some(event) = connection.poll_for_event().unwrap() {
         if let xcb::Event::X(x::Event::SelectionNotify(event)) = event {