Mark windows with _NET_WM_STATE_SKIP_TASKBAR as popups

Also includes some light refactoring of the popup flow in general
and trimming down some unused code.

I suspect this may cause some windows to unexpectedly become popups when they
otherwise shouldn't, but that's a bridge we'll cross when we get there.

Fixes #110 and #112.

src/server/mod.rs

@@ -85,15 +85,14 @@ where
 }
 
 #[derive(Default, Debug)]
-pub struct WindowAttributes {
+struct WindowAttributes {
-    pub override_redirect: bool,
+    is_popup: bool,
-    pub popup_for: Option<x::Window>,
+    dims: WindowDims,
-    pub dims: WindowDims,
+    size_hints: Option<WmNormalHints>,
-    pub size_hints: Option<WmNormalHints>,
+    title: Option<WmName>,
-    pub title: Option<WmName>,
+    class: Option<String>,
-    pub class: Option<String>,
+    group: Option<x::Window>,
-    pub group: Option<x::Window>,
+    decorations: Option<Decorations>,
-    pub decorations: Option<Decorations>,
 }
 
 #[derive(Debug, Default, PartialEq, Eq, Copy, Clone)]
@@ -119,7 +118,6 @@ impl WindowData {
         window: x::Window,
         override_redirect: bool,
         dims: WindowDims,
-        parent: Option<x::Window>,
         activation_token: Option<String>,
     ) -> Self {
         Self {
@@ -128,9 +126,8 @@ impl WindowData {
             surface_serial: None,
             mapped: false,
             attrs: WindowAttributes {
-                override_redirect,
+                is_popup: override_redirect,
                 dims,
-                popup_for: parent,
                 ..Default::default()
             },
             output_offset: WindowOutputOffset::default(),
@@ -643,7 +640,6 @@ impl<C: XConnection> ServerState<C> {
         window: x::Window,
         override_redirect: bool,
         dims: WindowDims,
-        parent: Option<x::Window>,
         pid: Option<u32>,
     ) {
         let activation_token = pid
@@ -657,10 +653,19 @@ impl<C: XConnection> ServerState<C> {
             });
         self.windows.insert(
             window,
-            WindowData::new(window, override_redirect, dims, parent, activation_token),
+            WindowData::new(window, override_redirect, dims, activation_token),
         );
     }
 
+    pub fn set_popup(&mut self, window: x::Window) {
+        let Some(win) = self.windows.get_mut(&window) else {
+            debug!("not setting popup for unknown window {window:?}");
+            return;
+        };
+
+        win.attrs.is_popup = true;
+    }
+
     pub fn set_win_title(&mut self, window: x::Window, name: WmName) {
         let Some(win) = self.windows.get_mut(&window) else {
             debug!("not setting title for unknown window {window:?}");
@@ -798,7 +803,7 @@ impl<C: XConnection> ServerState<C> {
             return true;
         };
 
-        !win.mapped || win.attrs.override_redirect
+        !win.mapped || win.attrs.is_popup
     }
 
     pub fn reconfigure_window(&mut self, event: x::ConfigureNotifyEvent) {
@@ -1149,13 +1154,7 @@ impl<C: XConnection> ServerState<C> {
     }
 
     fn create_role_window(&mut self, window: x::Window, surface_key: ObjectKey) {
-        // Temporarily remove surface to placate borrow checker
+        let surface: &mut SurfaceData = self.objects.get_mut(surface_key).unwrap().as_mut();
-        let mut surface: SurfaceData = self.objects[surface_key]
-            .0
-            .take()
-            .unwrap()
-            .try_into()
-            .unwrap();
         surface.window = Some(window);
         surface.client.attach(None, 0, 0);
         surface.client.commit();
@@ -1164,77 +1163,28 @@ impl<C: XConnection> ServerState<C> {
             .xdg_wm_base
             .get_xdg_surface(&surface.client, &self.qh, surface_key);
 
-        let window_data = self.windows.get_mut(&window).unwrap();
+        let window = self.windows.get(&window).unwrap();
-        if window_data.attrs.override_redirect {
+        let mut popup_for = None;
-            // Override redirect is hard to convert to Wayland!
+        if window.attrs.is_popup {
-            if let Some(win) = self.last_hovered {
+            popup_for = self.last_hovered.or(self.last_focused_toplevel);
-                window_data.attrs.popup_for = Some(win);
-            } else if let Some(win) = self.last_focused_toplevel {
-                window_data.attrs.popup_for = Some(win);
-            }
         }
 
         let mut fullscreen = false;
-        let (width, height) = (window_data.attrs.dims.width, window_data.attrs.dims.height);
+        let (width, height) = (window.attrs.dims.width, window.attrs.dims.height);
         for (key, _) in &self.output_keys {
             let output: &Output = self.objects[key].as_ref();
             if output.dimensions.width == width as i32 && output.dimensions.height == height as i32
             {
                 fullscreen = true;
-                window_data.attrs.popup_for = None;
+                popup_for = None;
             }
         }
 
-        let surface_id = surface.client.id();
-        // Reinsert surface
-        self.objects[surface_key].0 = Some(surface.into());
-        let window = self.windows.get(&window).unwrap();
-
         let initial_scale;
-        let role = if let Some(parent) = window.attrs.popup_for {
+        let role = if let Some(parent) = popup_for {
-            let parent_window = self.windows.get(&parent).unwrap();
+            let data;
-            let parent_surface: &SurfaceData =
+            (initial_scale, data) = self.create_popup(window, surface_key, xdg_surface, parent);
-                self.objects[parent_window.surface_key.unwrap()].as_ref();
+            SurfaceRole::Popup(Some(data))
-            let parent_dims = parent_window.attrs.dims;
-
-            initial_scale = parent_surface.scale_factor;
-            debug!(
-                "creating popup ({:?}) {:?} {:?} {:?} {:?} {surface_key:?} (scale: {initial_scale})",
-                window.window, parent, window.attrs.dims, surface_id, xdg_surface.id()
-            );
-
-            let positioner = self.xdg_wm_base.create_positioner(&self.qh, ());
-            positioner.set_size(
-                1.max((window.attrs.dims.width as f64 / initial_scale) as i32),
-                1.max((window.attrs.dims.height as f64 / initial_scale) as i32),
-            );
-            let x = ((window.attrs.dims.x - parent_dims.x) as f64 / initial_scale) as i32;
-            let y = ((window.attrs.dims.y - parent_dims.y) as f64 / initial_scale) as i32;
-            positioner.set_offset(x, y);
-            positioner.set_anchor(Anchor::TopLeft);
-            positioner.set_gravity(Gravity::BottomRight);
-            positioner.set_anchor_rect(
-                0,
-                0,
-                (parent_window.attrs.dims.width as f64 / initial_scale) as i32,
-                (parent_window.attrs.dims.height as f64 / initial_scale) as i32,
-            );
-            let popup = xdg_surface.get_popup(
-                Some(&parent_surface.xdg().unwrap().surface),
-                &positioner,
-                &self.qh,
-                surface_key,
-            );
-            let popup = PopupData {
-                popup,
-                positioner,
-                xdg: XdgSurfaceData {
-                    surface: xdg_surface,
-                    configured: false,
-                    pending: None,
-                },
-            };
-            SurfaceRole::Popup(Some(popup))
         } else {
             initial_scale = 1.0;
             let data = self.create_toplevel(window, surface_key, xdg_surface, fullscreen);
@@ -1330,6 +1280,64 @@ impl<C: XConnection> ServerState<C> {
         }
     }
 
+    fn create_popup(
+        &self,
+        window: &WindowData,
+        surface_key: ObjectKey,
+        xdg: XdgSurface,
+        parent: x::Window,
+    ) -> (f64, PopupData) {
+        let parent_window = self.windows.get(&parent).unwrap();
+        let parent_surface: &SurfaceData =
+            self.objects[parent_window.surface_key.unwrap()].as_ref();
+        let parent_dims = parent_window.attrs.dims;
+        let initial_scale = parent_surface.scale_factor;
+
+        debug!(
+            "creating popup ({:?}) {:?} {:?} {:?} {surface_key:?} (scale: {initial_scale})",
+            window.window,
+            parent,
+            window.attrs.dims,
+            xdg.id()
+        );
+
+        let positioner = self.xdg_wm_base.create_positioner(&self.qh, ());
+        positioner.set_size(
+            1.max((window.attrs.dims.width as f64 / initial_scale) as i32),
+            1.max((window.attrs.dims.height as f64 / initial_scale) as i32),
+        );
+        let x = ((window.attrs.dims.x - parent_dims.x) as f64 / initial_scale) as i32;
+        let y = ((window.attrs.dims.y - parent_dims.y) as f64 / initial_scale) as i32;
+        positioner.set_offset(x, y);
+        positioner.set_anchor(Anchor::TopLeft);
+        positioner.set_gravity(Gravity::BottomRight);
+        positioner.set_anchor_rect(
+            0,
+            0,
+            (parent_window.attrs.dims.width as f64 / initial_scale) as i32,
+            (parent_window.attrs.dims.height as f64 / initial_scale) as i32,
+        );
+        let popup = xdg.get_popup(
+            Some(&parent_surface.xdg().unwrap().surface),
+            &positioner,
+            &self.qh,
+            surface_key,
+        );
+
+        (
+            initial_scale,
+            PopupData {
+                popup,
+                positioner,
+                xdg: XdgSurfaceData {
+                    surface: xdg,
+                    configured: false,
+                    pending: None,
+                },
+            },
+        )
+    }
+
     fn get_server_surface_from_client(
         &self,
         surface: client::wl_surface::WlSurface,

src/server/tests.rs

@@ -572,17 +572,11 @@ impl TestFixture {
             .insert(window, data);
     }
 
-    fn new_window(
+    fn new_window(&mut self, window: Window, override_redirect: bool, data: WindowData) {
-        &mut self,
-        window: Window,
-        override_redirect: bool,
-        data: WindowData,
-        parent: Option<Window>,
-    ) {
         let dims = data.dims;
         self.register_window(window, data);
         self.satellite
-            .new_window(window, override_redirect, dims, parent, None);
+            .new_window(window, override_redirect, dims, None);
     }
 
     fn map_window(
@@ -654,7 +648,7 @@ impl TestFixture {
             fullscreen: false,
         };
 
-        self.new_window(window, false, data, None);
+        self.new_window(window, false, data);
         self.map_window(comp, window, &surface.obj, &buffer);
         self.run();
         let id = self.check_new_surface();
@@ -724,7 +718,7 @@ impl TestFixture {
             dims,
             fullscreen: false,
         };
-        self.new_window(window, true, data, None);
+        self.new_window(window, true, data);
         self.map_window(comp, window, &surface.obj, &buffer);
         self.run();
 
@@ -1093,7 +1087,7 @@ fn override_redirect_window_after_toplevel_close() {
 
     let win2 = unsafe { Window::new(2) };
     let (buffer, surface) = comp.create_surface();
-    f.new_window(win2, true, WindowData::default(), None);
+    f.new_window(win2, true, WindowData::default());
     f.map_window(&comp, win2, &surface.obj, &buffer);
     let second = f.check_new_surface();
     let data = f.testwl.get_surface_data(second).unwrap();
@@ -1194,7 +1188,6 @@ fn window_group_properties() {
             ..Default::default()
         },
         None,
-        None,
     );
     f.satellite
         .set_win_title(prop_win, WmName::WmName("window".into()));
@@ -1214,7 +1207,7 @@ fn window_group_properties() {
     let (_, surface) = comp.create_surface();
     let dims = data.dims;
     f.register_window(win, data);
-    f.satellite.new_window(win, false, dims, None, None);
+    f.satellite.new_window(win, false, dims, None);
     f.satellite.set_win_hints(
         win,
         super::WmHints {
@@ -1415,7 +1408,6 @@ fn override_redirect_choose_hover_window() {
             },
             ..Default::default()
         },
-        None,
     );
     f.map_window(&comp, win3, &surface.obj, &buffer);
     f.run();
@@ -1765,7 +1757,7 @@ fn fullscreen_heuristic() {
             },
             fullscreen: false,
         };
-        f.new_window(window, override_redirect, data, None);
+        f.new_window(window, override_redirect, data);
         f.map_window(&comp, window, &surface.obj, &buffer);
         f.run();
         let id = f.check_new_surface();
@@ -2097,7 +2089,7 @@ fn toplevel_size_limits_scaled() {
         },
         fullscreen: false,
     };
-    f.new_window(window, false, data, None);
+    f.new_window(window, false, data);
     f.satellite.set_size_hints(
         window,
         super::WmNormalHints {

src/xstate/mod.rs

@@ -2,7 +2,7 @@ mod selection;
 use selection::{Selection, SelectionData};
 use wayland_protocols::xdg::decoration::zv1::client::zxdg_toplevel_decoration_v1;
 
-use crate::{server::WindowAttributes, XConnection};
+use crate::XConnection;
 use bitflags::bitflags;
 use log::{debug, trace, warn};
 use std::collections::HashMap;
@@ -301,33 +301,39 @@ impl XState {
             match event {
                 xcb::Event::X(x::Event::CreateNotify(e)) => {
                     debug!("new window: {:?}", e);
-                    let parent = e.parent();
-                    let parent = if parent.is_none() || parent == self.root {
-                        None
-                    } else {
-                        Some(parent)
-                    };
                     server_state.new_window(
                         e.window(),
                         e.override_redirect(),
                         (&e).into(),
-                        parent,
                         self.get_pid(e.window()),
                     );
                 }
                 xcb::Event::X(x::Event::ReparentNotify(e)) => {
                     debug!("reparent event: {e:?}");
                     if e.parent() == self.root {
+                        let geometry = self.connection.send_request(&x::GetGeometry {
+                            drawable: x::Drawable::Window(e.window()),
+                        });
+                        let attrs = self
+                            .connection
+                            .send_request(&x::GetWindowAttributes { window: e.window() });
+                        let geometry = unwrap_or_skip_bad_window_cont!(self
+                            .connection
+                            .wait_for_reply(geometry));
                         let attrs =
-                            unwrap_or_skip_bad_window_cont!(self.get_window_attributes(e.window()));
+                            unwrap_or_skip_bad_window_cont!(self.connection.wait_for_reply(attrs));
+
                         server_state.new_window(
                             e.window(),
-                            attrs.override_redirect,
+                            attrs.override_redirect(),
-                            attrs.dims,
+                            WindowDims {
-                            None,
+                                x: geometry.x(),
+                                y: geometry.y(),
+                                width: geometry.width(),
+                                height: geometry.height(),
+                            },
                             self.get_pid(e.window()),
                         );
-                        self.handle_window_attributes(server_state, e.window(), attrs);
                     } else {
                         debug!("destroying window since its parent is no longer root!");
                         server_state.destroy_window(e.window());
@@ -347,9 +353,9 @@ impl XState {
                             value_list: &[x::Cw::EventMask(x::EventMask::PROPERTY_CHANGE)],
                         }
                     ));
-                    let attrs =
+                    unwrap_or_skip_bad_window_cont!(
-                        unwrap_or_skip_bad_window_cont!(self.get_window_attributes(e.window()));
+                        self.handle_window_properties(server_state, e.window())
-                    self.handle_window_attributes(server_state, e.window(), attrs);
+                    );
                     server_state.map_window(e.window());
                 }
                 xcb::Event::X(x::Event::ConfigureNotify(e)) => {
@@ -484,68 +490,49 @@ impl XState {
         }
     }
 
-    fn get_window_attributes(&self, window: x::Window) -> XResult<WindowAttributes> {
+    fn handle_window_properties(
-        let geometry = self.connection.send_request(&x::GetGeometry {
+        &self,
-            drawable: x::Drawable::Window(window),
+        server_state: &mut super::RealServerState,
-        });
+        window: x::Window,
-        let attrs = self
+    ) -> XResult<()> {
-            .connection
-            .send_request(&x::GetWindowAttributes { window });
-
         let name = self.get_net_wm_name(window);
         let class = self.get_wm_class(window);
-        let wm_hints = self.get_wm_hints(window);
         let size_hints = self.get_wm_size_hints(window);
         let motif_wm_hints = self.get_motif_wm_hints(window);
+        let window_state = PropertyCookieWrapper {
+            connection: &self.connection,
+            cookie: self.get_property_cookie(window, self.atoms.net_wm_state, x::ATOM_ATOM, 10),
+            resolver: |reply: x::GetPropertyReply| reply.value::<x::Atom>().to_vec(),
+        };
 
-        let geometry = self.connection.wait_for_reply(geometry)?;
-        debug!("{window:?} geometry: {geometry:?}");
-        let attrs = self.connection.wait_for_reply(attrs)?;
         let mut title = name.resolve()?;
         if title.is_none() {
             title = self.get_wm_name(window).resolve()?;
         }
 
-        let class = class.resolve()?;
+        if let Some(name) = title {
-        let wm_hints = wm_hints.resolve()?;
-        let size_hints = size_hints.resolve()?;
-        let motif_wm_hints = motif_wm_hints.resolve()?;
-
-        Ok(WindowAttributes {
-            override_redirect: attrs.override_redirect(),
-            popup_for: None,
-            dims: WindowDims {
-                x: geometry.x(),
-                y: geometry.y(),
-                width: geometry.width(),
-                height: geometry.height(),
-            },
-            title,
-            class,
-            group: wm_hints.and_then(|h| h.window_group),
-            size_hints,
-            decorations: motif_wm_hints.and_then(|h| h.decorations),
-        })
-    }
-
-    fn handle_window_attributes(
-        &self,
-        server_state: &mut super::RealServerState,
-        window: x::Window,
-        attrs: WindowAttributes,
-    ) {
-        if let Some(name) = attrs.title {
             server_state.set_win_title(window, name);
         }
-        if let Some(class) = attrs.class {
+        if let Some(class) = class.resolve()? {
             server_state.set_win_class(window, class);
         }
-        if let Some(hints) = attrs.size_hints {
+        if let Some(hints) = size_hints.resolve()? {
             server_state.set_size_hints(window, hints);
         }
-        if let Some(decorations) = attrs.decorations {
+        if let Some(decorations) = motif_wm_hints.resolve()?.and_then(|m| m.decorations) {
             server_state.set_win_decorations(window, decorations);
         }
+
+        let mut is_popup = false;
+        if let Some(states) = window_state.resolve()? {
+            is_popup = states.contains(&self.atoms.skip_taskbar);
+        }
+
+        if is_popup {
+            server_state.set_popup(window);
+        }
+
+        Ok(())
     }
 
     fn get_property_cookie(
@@ -787,6 +774,7 @@ xcb::atoms_struct! {
         wm_pid => b"_NET_WM_PID" only_if_exists = false,
         net_wm_state => b"_NET_WM_STATE" only_if_exists = false,
         wm_fullscreen => b"_NET_WM_STATE_FULLSCREEN" only_if_exists = false,
+        skip_taskbar => b"_NET_WM_STATE_SKIP_TASKBAR" only_if_exists = false,
         active_win => b"_NET_ACTIVE_WINDOW" only_if_exists = false,
         client_list => b"_NET_CLIENT_LIST" only_if_exists = false,
         supported => b"_NET_SUPPORTED" only_if_exists = false,
@@ -803,12 +791,13 @@ xcb::atoms_struct! {
 }
 
 xcb::atoms_struct! {
-    pub struct WindowTypes {
+    struct WindowTypes {
-        pub normal => b"_NET_WM_WINDOW_TYPE_NORMAL" only_if_exists = false,
+        ty => b"_NET_WM_WINDOW_TYPE" only_if_exists = false,
-        pub dialog => b"_NET_WM_WINDOW_TYPE_DIALOG" only_if_exists = false,
+        normal => b"_NET_WM_WINDOW_TYPE_NORMAL" only_if_exists = false,
-        pub splash => b"_NET_WM_WINDOW_TYPE_SPLASH" only_if_exists = false,
+        dialog => b"_NET_WM_WINDOW_TYPE_DIALOG" only_if_exists = false,
-        pub menu => b"_NET_WM_WINDOW_TYPE_MENU" only_if_exists = false,
+        splash => b"_NET_WM_WINDOW_TYPE_SPLASH" only_if_exists = false,
-        pub utility => b"_NET_WM_WINDOW_TYPE_UTILITY" only_if_exists = false,
+        menu => b"_NET_WM_WINDOW_TYPE_MENU" only_if_exists = false,
+        utility => b"_NET_WM_WINDOW_TYPE_UTILITY" only_if_exists = false,
     }
 }
 

tests/integration.rs

@@ -301,6 +301,9 @@ xcb::atoms_struct! {
         wm_protocols => b"WM_PROTOCOLS",
         net_active_window => b"_NET_ACTIVE_WINDOW",
         wm_delete_window => b"WM_DELETE_WINDOW",
+        net_wm_state => b"_NET_WM_STATE",
+        skip_taskbar => b"_NET_WM_STATE_SKIP_TASKBAR",
+        transient_for => b"WM_TRANSIENT_FOR",
         clipboard => b"CLIPBOARD",
         targets => b"TARGETS",
         multiple => b"MULTIPLE",
@@ -1642,3 +1645,21 @@ fn forced_1x_scale_consistent_x11_size() {
     assert_eq!(geo.width(), 30);
     assert_eq!(geo.height(), 30);
 }
+
+#[test]
+fn popup_properties() {
+    let mut f = Fixture::new();
+    let mut connection = Connection::new(&f.display);
+
+    let win_toplevel = connection.new_window(connection.root, 0, 0, 20, 20, false);
+    f.map_as_toplevel(&mut connection, win_toplevel);
+
+    let win_popup_dialog = connection.new_window(connection.root, 10, 10, 50, 50, false);
+    connection.set_property(
+        win_popup_dialog,
+        x::ATOM_ATOM,
+        connection.atoms.net_wm_state,
+        &[connection.atoms.skip_taskbar],
+    );
+    f.map_as_popup(&mut connection, win_popup_dialog);
+}