issue #129 - improving handling of which windows are managed

Penrose still doesn't correctly manage all window types but this seems
to allow for sorting out windows that don't set a window type (such as
'st' or 'mpv') and also prevents transients and 'unviewable' windows
from being mapped on restart.

I suspect that handling around floating clients is still wonky but I
need to get some clean reproducible test cases to work with. Firefox is
behaving very strangely when being closed at the moment and I'm not sure
if it's due to how penrose is closing the window (only Firefox seems
affected) or if it is something wrong with my system...

src/contrib/extensions/scratchpad.rs

@@ -140,7 +140,7 @@ impl<X: XConn> Hook<X> for Scratchpad {
             self.client.replace(Some(c.id()));
             c.externally_managed();
             c.set_floating(true);
-            self.toggle_client(wm).unwrap();
+            return self.toggle_client(wm);
         }
 
         Ok(())

src/core/xconnection/mod.rs

@@ -19,8 +19,6 @@ use crate::{
 
 use penrose_proc::stubbed_companion_trait;
 
-use std::str::FromStr;
-
 pub mod atom;
 pub mod event;
 pub mod property;
@@ -32,7 +30,10 @@ pub use event::{
     ClientEventMask, ClientMessage, ClientMessageKind, ConfigureEvent, ExposeEvent, PointerChange,
     PropertyEvent, XEvent,
 };
-pub use property::{Prop, WmHints, WmNormalHints, WmNormalHintsFlags};
+pub use property::{
+    MapState, Prop, WindowAttributes, WindowClass, WindowState, WmHints, WmNormalHints,
+    WmNormalHintsFlags,
+};
 
 /// An X resource ID
 pub type Xid = u32;
@@ -320,6 +321,10 @@ pub trait XClientConfig {
     #[stub(Ok(()))]
     fn set_client_attributes(&self, id: Xid, data: &[ClientAttr]) -> Result<()>;
 
+    /// Get the [WindowAttributes] for this client
+    #[stub(Err(XError::Raw("mocked".into())))]
+    fn get_window_attributes(&self, id: Xid) -> Result<WindowAttributes>;
+
     /*
      *  The following default implementations should used if possible.
      *
@@ -537,25 +542,37 @@ pub trait XConn:
     }
 
     /// Check to see if this client is one that we should be handling or not
+    #[tracing::instrument(level = "trace", skip(self))]
     fn is_managed_client(&self, id: Xid) -> bool {
         if self.get_prop(id, Atom::WmTransientFor.as_ref()).is_ok() {
-            false
-        } else if let Ok(Prop::Atom(atoms)) = self.get_prop(id, Atom::NetWmWindowType.as_ref()) {
-            atoms.iter().any(|atom| match Atom::from_str(atom) {
-                Ok(a) => !UNMANAGED_WINDOW_TYPES.contains(&a),
-                _ => false,
-            })
-        } else {
-            false
+            trace!("window is transient: don't manage");
+            return false;
+        }
+
+        if let Ok(Prop::Atom(types)) = self.get_prop(id, Atom::NetWmWindowType.as_ref()) {
+            let unmanaged_types: Vec<String> = UNMANAGED_WINDOW_TYPES
+                .iter()
+                .map(|t| t.as_ref().to_string())
+                .collect();
+            trace!(ty = ?types, "checking window type to see we should manage");
+            return types.iter().all(|ty| !unmanaged_types.contains(ty));
         }
+
+        trace!("unable to find type: defaulting to manage");
+        return true;
     }
 
     /// The subset of active clients that are considered managed by penrose
     fn active_managed_clients(&self) -> Result<Vec<Xid>> {
         Ok(self
             .active_clients()?
             .into_iter()
-            .filter(|&id| self.is_managed_client(id))
+            .filter(|&id| {
+                let attrs_ok = self.get_window_attributes(id).map_or(true, |a| {
+                    !a.override_redirect && a.map_state == MapState::Viewable
+                });
+                attrs_ok && self.is_managed_client(id)
+            })
             .collect())
     }
 }
@@ -658,6 +675,8 @@ mod mock_conn {
 mod tests {
     use super::*;
 
+    use std::str::FromStr;
+
     struct WmNameXConn {
         wm_name: bool,
         net_wm_name: bool,

src/core/xconnection/property.rs

@@ -93,6 +93,30 @@ pub enum WindowState {
     Iconic,
 }
 
+/// The mapping states a window can be in
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[derive(Debug, PartialEq, Eq, Hash, Clone)]
+pub enum MapState {
+    /// The window is unmapped
+    Unmapped,
+    /// The window is never viewable
+    UnViewable,
+    /// The window is currently viewable
+    Viewable,
+}
+
+/// The input class for a window
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[derive(Debug, PartialEq, Eq, Hash, Clone)]
+pub enum WindowClass {
+    /// Class is copied from parent window
+    CopyFromParent,
+    /// Window can be displayed
+    InputOutput,
+    /// Window can only be used for queries
+    InputOnly,
+}
+
 /// Client requested hints about information other than window geometry.
 ///
 /// See the ICCCM [spec][1] for further details.
@@ -101,14 +125,14 @@ pub enum WindowState {
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[derive(Debug, PartialEq, Eq, Hash, Clone)]
 pub struct WmHints {
-    flags: WmHintsFlags,
-    accepts_input: bool,
-    initial_state: WindowState,
-    icon_pixmap: u32,
-    icon_win: Xid,
-    icon_position: Point,
-    icon_mask: u32,
-    window_group: u32,
+    pub(crate) flags: WmHintsFlags,
+    pub(crate) accepts_input: bool,
+    pub(crate) initial_state: WindowState,
+    pub(crate) icon_pixmap: u32,
+    pub(crate) icon_win: Xid,
+    pub(crate) icon_position: Point,
+    pub(crate) icon_mask: u32,
+    pub(crate) window_group: u32,
 }
 
 impl WmHints {
@@ -201,11 +225,11 @@ impl WmHints {
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[derive(Debug, PartialEq, Eq, Hash, Clone)]
 pub struct WmNormalHints {
-    flags: WmNormalHintsFlags,
-    base: Option<Region>,
-    min: Option<Region>,
-    max: Option<Region>,
-    user_specified: Option<Region>,
+    pub(crate) flags: WmNormalHintsFlags,
+    pub(crate) base: Option<Region>,
+    pub(crate) min: Option<Region>,
+    pub(crate) max: Option<Region>,
+    pub(crate) user_specified: Option<Region>,
 }
 
 impl WmNormalHints {
@@ -286,3 +310,50 @@ impl WmNormalHints {
         })
     }
 }
+
+/// Window Attributes honoured by penose.
+///
+/// Only a small subset of window attributes are checked and honoured by penrose. This list may be
+/// extended in future.
+///
+/// ```C
+/// typedef struct xcb_get_window_attributes_reply_t {
+///     uint8_t        response_type;
+///     uint8_t        backing_store;
+///     uint16_t       sequence;
+///     uint32_t       length;
+///     xcb_visualid_t visual;
+///     uint16_t       _class;
+///     uint8_t        bit_gravity;
+///     uint8_t        win_gravity;
+///     uint32_t       backing_planes;
+///     uint32_t       backing_pixel;
+///     uint8_t        save_under;
+///     uint8_t        map_is_installed;
+///     uint8_t        map_state;
+///     uint8_t        override_redirect;
+///     xcb_colormap_t colormap;
+///     uint32_t       all_event_masks;
+///     uint32_t       your_event_mask;
+///     uint16_t       do_not_propagate_mask;
+///     uint8_t        pad0[2];
+/// } xcb_get_window_attributes_reply_t;
+/// ```
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[derive(Debug, PartialEq, Eq, Hash, Clone)]
+pub struct WindowAttributes {
+    pub(crate) override_redirect: bool,
+    pub(crate) map_state: MapState,
+    pub(crate) window_class: WindowClass,
+}
+
+impl WindowAttributes {
+    /// Create a new instance from component parts
+    pub fn new(override_redirect: bool, map_state: MapState, window_class: WindowClass) -> Self {
+        Self {
+            override_redirect,
+            map_state,
+            window_class,
+        }
+    }
+}

src/x11rb/xconn.rs

@@ -14,8 +14,8 @@ use crate::{
         screen::Screen,
         xconnection::{
             Atom, ClientAttr, ClientConfig, ClientEventMask, ClientMessage, ClientMessageKind,
-            Prop, Result, WmHints, WmNormalHints, XAtomQuerier, XClientConfig, XClientHandler,
-            XClientProperties, XConn, XEvent, XEventHandler, XState, Xid,
+            Prop, Result, WindowAttributes, WmHints, WmNormalHints, XAtomQuerier, XClientConfig,
+            XClientHandler, XClientProperties, XConn, XError, XEvent, XEventHandler, XState, Xid,
         },
     },
     x11rb::{atom::Atoms, X11rbError},
@@ -30,7 +30,7 @@ use x11rb::{
         xproto::{
             AtomEnum, ButtonIndex, ChangeWindowAttributesAux, ClientMessageData,
             ClientMessageEvent, ConfigureWindowAux, ConnectionExt as _, CreateWindowAux, EventMask,
-            Grab, GrabMode, InputFocus, ModMask, PropMode, StackMode, WindowClass,
+            Grab, GrabMode, InputFocus, MapState, ModMask, PropMode, StackMode, WindowClass,
             CLIENT_MESSAGE_EVENT,
         },
     },
@@ -172,6 +172,39 @@ impl<C: Connection> XClientConfig for X11rbConnection<C> {
         self.conn.change_window_attributes(id, &aux)?;
         Ok(())
     }
+
+    fn get_window_attributes(&self, id: Xid) -> Result<WindowAttributes> {
+        let win_attrs = self.conn.get_window_attributes(id)?.reply()?;
+        let override_redirect = win_attrs.override_redirect;
+        let map_state = match win_attrs.map_state {
+            MapState::UNMAPPED => crate::core::xconnection::MapState::Unmapped,
+            MapState::UNVIEWABLE => crate::core::xconnection::MapState::UnViewable,
+            MapState::VIEWABLE => crate::core::xconnection::MapState::Viewable,
+            _ => {
+                return Err(XError::Raw(format!(
+                    "invalid map state: {:?}",
+                    win_attrs.map_state
+                )))
+            }
+        };
+        let window_class = match win_attrs.class {
+            WindowClass::COPY_FROM_PARENT => crate::core::xconnection::WindowClass::CopyFromParent,
+            WindowClass::INPUT_OUTPUT => crate::core::xconnection::WindowClass::InputOutput,
+            WindowClass::INPUT_ONLY => crate::core::xconnection::WindowClass::InputOnly,
+            _ => {
+                return Err(XError::Raw(format!(
+                    "invalid window class: {:?}",
+                    win_attrs.class
+                )))
+            }
+        };
+
+        Ok(WindowAttributes::new(
+            override_redirect,
+            map_state,
+            window_class,
+        ))
+    }
 }
 
 impl<C: Connection> XClientHandler for X11rbConnection<C> {