x11: More robust geometry calculations (#438)

Tested on the following window managers:
* Xfwm4 4.12.4
* Mutter 3.26.2
* Muffin 3.6.0
* Budgie 10.4
* Marco 1.20.0
* Compiz 0.9.13.1
* KWin 5.12.3
* Enlightenment 0.22.2
* FVWM 2.6.7
* Awesome 4.2
* i3 4.15
* xmonad 0.13
* dwm 6.1
* Openbox 3.6.1
* Fluxbox 1.3.7
* Blackbox 0.70.1
* IceWM 1.3.8
* IceWM 1.4.2

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Unreleased
 
+- Overhauled X11 window geometry calculations. `get_position` and `set_position` are more universally accurate across different window managers, and `get_outer_size` actually works now.
+
 # Version 0.12.0 (2018-04-06)
 
 - Added subclass to macos windows so they can be made resizable even with no decorations.

src/platform/linux/x11/util.rs

@@ -98,6 +98,16 @@ pub enum GetPropertyError {
     NothingAllocated,
 }
 
+impl GetPropertyError {
+    pub fn is_actual_property_type(&self, t: ffi::Atom) -> bool {
+        if let GetPropertyError::TypeMismatch(actual_type) = *self {
+            actual_type == t
+        } else {
+            false
+        }
+    }
+}
+
 pub unsafe fn get_property<T>(
     xconn: &Arc<XConnection>,
     window: c_ulong,
@@ -299,6 +309,62 @@ pub unsafe fn lookup_utf8(
     str::from_utf8(&buffer[..count as usize]).unwrap_or("").to_string()
 }
 
+
+#[derive(Debug)]
+pub struct FrameExtents {
+    pub left: c_ulong,
+    pub right: c_ulong,
+    pub top: c_ulong,
+    pub bottom: c_ulong,
+}
+
+impl FrameExtents {
+    pub fn new(left: c_ulong, right: c_ulong, top: c_ulong, bottom: c_ulong) -> Self {
+        FrameExtents { left, right, top, bottom }
+    }
+
+    pub fn from_border(border: c_ulong) -> Self {
+        Self::new(border, border, border, border)
+    }
+}
+
+#[derive(Debug)]
+pub struct WindowGeometry {
+    pub x: c_int,
+    pub y: c_int,
+    pub width: c_uint,
+    pub height: c_uint,
+    pub frame: FrameExtents,
+}
+
+impl WindowGeometry {
+    pub fn get_position(&self) -> (i32, i32) {
+        (self.x as _, self.y as _)
+    }
+
+    pub fn get_inner_position(&self) -> (i32, i32) {
+        (
+            self.x.saturating_add(self.frame.left as c_int) as _,
+            self.y.saturating_add(self.frame.top as c_int) as _,
+        )
+    }
+
+    pub fn get_inner_size(&self) -> (u32, u32) {
+        (self.width as _, self.height as _)
+    }
+
+    pub fn get_outer_size(&self) -> (u32, u32) {
+        (
+            self.width.saturating_add(
+                self.frame.left.saturating_add(self.frame.right) as c_uint
+            ) as _,
+            self.height.saturating_add(
+                self.frame.top.saturating_add(self.frame.bottom) as c_uint
+            ) as _,
+        )
+    }
+}
+
 // Important: all XIM calls need to happen from the same thread!
 pub struct Ime {
     xconn: Arc<XConnection>,

src/platform/linux/x11/window.rs

@@ -5,7 +5,7 @@ use libc;
 use std::borrow::Borrow;
 use std::{mem, cmp, ptr};
 use std::sync::{Arc, Mutex};
-use std::os::raw::{c_int, c_long, c_uchar, c_ulong, c_void};
+use std::os::raw::{c_int, c_long, c_uchar, c_uint, c_ulong, c_void};
 use std::thread;
 use std::time::Duration;
 
@@ -99,6 +99,140 @@ pub struct Window2 {
     pub x: Arc<XWindow>,
     cursor: Mutex<MouseCursor>,
     cursor_state: Mutex<CursorState>,
+    supported_hints: Vec<ffi::Atom>,
+    wm_name: Option<String>,
+}
+
+fn get_supported_hints(xwin: &Arc<XWindow>) -> Vec<ffi::Atom> {
+    let supported_atom = unsafe { util::get_atom(&xwin.display, b"_NET_SUPPORTED\0") }
+        .expect("Failed to call XInternAtom (_NET_SUPPORTED)");
+    unsafe {
+        util::get_property(
+            &xwin.display,
+            xwin.root,
+            supported_atom,
+            ffi::XA_ATOM,
+        )
+    }.unwrap_or_else(|_| Vec::with_capacity(0))
+}
+
+fn get_wm_name(xwin: &Arc<XWindow>, _supported_hints: &[ffi::Atom]) -> Option<String> {
+    let check_atom = unsafe { util::get_atom(&xwin.display, b"_NET_SUPPORTING_WM_CHECK\0") }
+        .expect("Failed to call XInternAtom (_NET_SUPPORTING_WM_CHECK)");
+    let wm_name_atom = unsafe { util::get_atom(&xwin.display, b"_NET_WM_NAME\0") }
+        .expect("Failed to call XInternAtom (_NET_WM_NAME)");
+
+    // Mutter/Muffin/Budgie doesn't have _NET_SUPPORTING_WM_CHECK in its _NET_SUPPORTED, despite
+    // it working and being supported. This has been reported upstream, but due to the
+    // inavailability of time machines, we'll just try to get _NET_SUPPORTING_WM_CHECK
+    // regardless of whether or not the WM claims to support it.
+    //
+    // Blackbox 0.70 also incorrectly reports not supporting this, though that appears to be fixed
+    // in 0.72.
+    /*if !supported_hints.contains(&check_atom) {
+        return None;
+    }*/
+
+    // IceWM (1.3.x and earlier) doesn't report supporting _NET_WM_NAME, but will nonetheless
+    // provide us with a value for it. Note that the unofficial 1.4 fork of IceWM works fine.
+    /*if !supported_hints.contains(&wm_name_atom) {
+        return None;
+    }*/
+
+    // Of the WMs tested, only xmonad and dwm fail to provide a WM name.
+
+    // Querying this property on the root window will give us the ID of a child window created by
+    // the WM.
+    let root_window_wm_check = {
+        let result = unsafe {
+            util::get_property(
+                &xwin.display,
+                xwin.root,
+                check_atom,
+                ffi::XA_WINDOW,
+            )
+        };
+
+        let wm_check = result
+            .ok()
+            .and_then(|wm_check| wm_check.get(0).cloned());
+
+        if let Some(wm_check) = wm_check {
+            wm_check
+        } else {
+            return None;
+        }
+    };
+
+    // Querying the same property on the child window we were given, we should get this child
+    // window's ID again.
+    let child_window_wm_check = {
+        let result = unsafe {
+            util::get_property(
+                &xwin.display,
+                root_window_wm_check,
+                check_atom,
+                ffi::XA_WINDOW,
+            )
+        };
+
+        let wm_check = result
+            .ok()
+            .and_then(|wm_check| wm_check.get(0).cloned());
+
+        if let Some(wm_check) = wm_check {
+            wm_check
+        } else {
+            return None;
+        }
+    };
+
+    // These values should be the same.
+    if root_window_wm_check != child_window_wm_check {
+        return None;
+    }
+
+    // All of that work gives us a window ID that we can get the WM name from.
+    let wm_name = {
+        let utf8_string_atom = unsafe { util::get_atom(&xwin.display, b"UTF8_STRING\0") }
+            .unwrap_or(ffi::XA_STRING);
+
+        let result = unsafe {
+            util::get_property(
+                &xwin.display,
+                root_window_wm_check,
+                wm_name_atom,
+                utf8_string_atom,
+            )
+        };
+
+        // IceWM requires this. IceWM was also the only WM tested that returns a null-terminated
+        // string. For more fun trivia, IceWM is also unique in including version and uname
+        // information in this string (this means you'll have to be careful if you want to match
+        // against it, though).
+        // The unofficial 1.4 fork of IceWM still includes the extra details, but properly
+        // returns a UTF8 string that isn't null-terminated.
+        let no_utf8 = if let Err(ref err) = result {
+            err.is_actual_property_type(ffi::XA_STRING)
+        } else {
+            false
+        };
+
+        if no_utf8 {
+            unsafe {
+                util::get_property(
+                    &xwin.display,
+                    root_window_wm_check,
+                    wm_name_atom,
+                    ffi::XA_STRING,
+                )
+            }
+        } else {
+            result
+        }
+    }.ok();
+
+    wm_name.and_then(|wm_name| String::from_utf8(wm_name).ok())
 }
 
 impl Window2 {
@@ -179,15 +313,24 @@ impl Window2 {
             win
         };
 
+        let x_window = Arc::new(XWindow {
+            display: display.clone(),
+            window,
+            root,
+            screen_id,
+        });
+
+        // These values will cease to be correct if the user replaces the WM during the life of
+        // the window, so hopefully they don't do that.
+        let supported_hints = get_supported_hints(&x_window);
+        let wm_name = get_wm_name(&x_window, &supported_hints);
+
         let window = Window2 {
-            x: Arc::new(XWindow {
+            x: x_window,
-                display: display.clone(),
-                window,
-                root,
-                screen_id,
-            }),
             cursor: Mutex::new(MouseCursor::Default),
             cursor_state: Mutex::new(CursorState::Normal),
+            supported_hints,
+            wm_name,
         };
 
         // Title must be set before mapping, lest some tiling window managers briefly pick up on
@@ -531,80 +674,281 @@ impl Window2 {
         }
     }
 
-    fn get_geometry(&self) -> Option<(i32, i32, u32, u32, u32)> {
+    fn get_frame_extents(&self) -> Option<util::FrameExtents> {
-        unsafe {
+        let extents_atom = unsafe { util::get_atom(&self.x.display, b"_NET_FRAME_EXTENTS\0") }
-            use std::mem;
+            .expect("Failed to call XInternAtom (_NET_FRAME_EXTENTS)");
 
+        if !self.supported_hints.contains(&extents_atom) {
+            return None;
+        }
+
+        // Of the WMs tested, xmonad, i3, dwm, IceWM (1.3.x and earlier), and blackbox don't
+        // support this. As this is part of EWMH (Extended Window Manager Hints), it's likely to
+        // be unsupported by many smaller WMs.
+        let extents: Option<Vec<c_ulong>> = unsafe {
+            util::get_property(
+                &self.x.display,
+                self.x.window,
+                extents_atom,
+                ffi::XA_CARDINAL,
+            )
+        }.ok();
+
+        extents.and_then(|extents| {
+            if extents.len() >= 4 {
+                Some(util::FrameExtents {
+                    left: extents[0],
+                    right: extents[1],
+                    top: extents[2],
+                    bottom: extents[3],
+                })
+            } else {
+                None
+            }
+        })
+    }
+
+    fn is_top_level(&self, id: ffi::Window) -> Option<bool> {
+        let client_list_atom = unsafe { util::get_atom(&self.x.display, b"_NET_CLIENT_LIST\0") }
+            .expect("Failed to call XInternAtom (_NET_CLIENT_LIST)");
+
+        if !self.supported_hints.contains(&client_list_atom) {
+            return None;
+        }
+
+        let client_list: Option<Vec<ffi::Window>> = unsafe {
+            util::get_property(
+                &self.x.display,
+                self.x.root,
+                client_list_atom,
+                ffi::XA_WINDOW,
+            )
+        }.ok();
+
+        client_list.map(|client_list| {
+            client_list.contains(&id)
+        })
+    }
+
+    fn get_geometry(&self) -> Option<util::WindowGeometry> {
+        // Position relative to root window.
+        // With rare exceptions, this is the position of a nested window. Cases where the window
+        // isn't nested are outlined in the comments throghout this function, but in addition to
+        // that, fullscreen windows sometimes aren't nested.
+        let (inner_x_rel_root, inner_y_rel_root, child) = unsafe {
+            let mut inner_x_rel_root: c_int = mem::uninitialized();
+            let mut inner_y_rel_root: c_int = mem::uninitialized();
+            let mut child: ffi::Window = mem::uninitialized();
+
+            (self.x.display.xlib.XTranslateCoordinates)(
+                self.x.display.display,
+                self.x.window,
+                self.x.root,
+                0,
+                0,
+                &mut inner_x_rel_root,
+                &mut inner_y_rel_root,
+                &mut child,
+            );
+
+            (inner_x_rel_root, inner_y_rel_root, child)
+        };
+
+        let (inner_x, inner_y, width, height, border) = unsafe {
             let mut root: ffi::Window = mem::uninitialized();
-            let mut x: libc::c_int = mem::uninitialized();
+            // The same caveat outlined in the comment above for XTranslateCoordinates applies
-            let mut y: libc::c_int = mem::uninitialized();
+            // here as well. The only difference is that this position is relative to the parent
-            let mut width: libc::c_uint = mem::uninitialized();
+            // window, rather than the root window.
-            let mut height: libc::c_uint = mem::uninitialized();
+            let mut inner_x: c_int = mem::uninitialized();
-            let mut border: libc::c_uint = mem::uninitialized();
+            let mut inner_y: c_int = mem::uninitialized();
-            let mut depth: libc::c_uint = mem::uninitialized();
+            // The width and height here are for the client area.
+            let mut width: c_uint = mem::uninitialized();
+            let mut height: c_uint = mem::uninitialized();
+            // xmonad and dwm were the only WMs tested that use the border return at all.
+            // The majority of WMs seem to simply fill it with 0 unconditionally.
+            let mut border: c_uint = mem::uninitialized();
+            let mut depth: c_uint = mem::uninitialized();
 
-            // Get non-positioning data from winit window
+            let status = (self.x.display.xlib.XGetGeometry)(
-            if (self.x.display.xlib.XGetGeometry)(self.x.display.display, self.x.window,
+                self.x.display.display,
-                &mut root, &mut x, &mut y, &mut width, &mut height,
+                self.x.window,
-                &mut border, &mut depth) == 0
+                &mut root,
-            {
+                &mut inner_x,
+                &mut inner_y,
+                &mut width,
+                &mut height,
+                &mut border,
+                &mut depth,
+            );
+
+            if status == 0 {
                 return None;
             }
 
-            let width_out = width;
+            (inner_x, inner_y, width, height, border)
-            let height_out = height;
+        };
-            let border_out = border;
 
-            // Some window managers like i3wm will actually nest application
+        // The first condition is only false for un-nested windows, but isn't always false for
-            // windows (like those opened by winit) within other windows to, for
+        // un-nested windows. Mutter/Muffin/Budgie and Marco present a mysterious discrepancy:
-            // example, add decorations. Initially when debugging this method on
+        // when y is on the range [0, 2] and if the window has been unfocused since being
-            // i3, the x and y positions were always returned as "2".
+        // undecorated (or was undecorated upon construction), the first condition is true,
-            //
+        // requiring us to rely on the second condition.
-            // The solution that other xlib abstractions use is to climb up the
+        let nested = !(self.x.window == child || self.is_top_level(child) == Some(true));
-            // window hierarchy until just below the root window, and that
+
-            // window must be used to determine the appropriate position.
+        // Hopefully the WM supports EWMH, allowing us to get exact info on the window frames.
+        if let Some(mut extents) = self.get_frame_extents() {
+            // Mutter/Muffin/Budgie and Marco preserve their decorated frame extents when
+            // decorations are disabled, but since the window becomes un-nested, it's easy to
+            // catch.
+            if !nested {
+                extents = util::FrameExtents::new(0, 0, 0, 0);
+            }
+
+            // The difference between the nested window's position and the outermost window's
+            // position is equivalent to the frame size. In most scenarios, this is equivalent to
+            // manually climbing the hierarchy as is done in the case below. Here's a list of
+            // known discrepancies:
+            // * Mutter/Muffin/Budgie gives decorated windows a margin of 9px (only 7px on top) in
+            //   addition to a 1px semi-transparent border. The margin can be easily observed by
+            //   using a screenshot tool to get a screenshot of a selected window, and is
+            //   presumably used for drawing drop shadows. Getting window geometry information
+            //   via hierarchy-climbing results in this margin being included in both the
+            //   position and outer size, so a window positioned at (0, 0) would be reported as
+            //   having a position (-10, -8).
+            // * Compiz has a drop shadow margin just like Mutter/Muffin/Budgie, though it's 10px
+            //   on all sides, and there's no additional border.
+            // * Enlightenment otherwise gets a y position equivalent to inner_y_rel_root.
+            //   Without decorations, there's no difference. This is presumably related to
+            //   Enlightenment's fairly unique concept of window position; it interprets
+            //   positions given to XMoveWindow as a client area position rather than a position
+            //   of the overall window.
+            let abs_x = inner_x_rel_root - extents.left as c_int;
+            let abs_y = inner_y_rel_root - extents.top as c_int;
+
+            Some(util::WindowGeometry {
+                x: abs_x,
+                y: abs_y,
+                width,
+                height,
+                frame: extents,
+            })
+        } else if nested {
+            // If the position value we have is for a nested window used as the client area, we'll
+            // just climb up the hierarchy and get the geometry of the outermost window we're
+            // nested in.
             let window = {
                 let root = self.x.root;
                 let mut window = self.x.window;
                 loop {
-                    let candidate = self.x.get_parent_window(window).unwrap();
+                    let candidate = unsafe {
+                        self.x.get_parent_window(window).unwrap()
+                    };
                     if candidate == root {
                         break window;
                     }
-
                     window = candidate;
                 }
             };
 
-            if (self.x.display.xlib.XGetGeometry)(self.x.display.display, window,
+            let (outer_x, outer_y, outer_width, outer_height) = unsafe {
-                &mut root, &mut x, &mut y, &mut width, &mut height,
+                let mut root: ffi::Window = mem::uninitialized();
-                &mut border, &mut depth) == 0
+                let mut outer_x: c_int = mem::uninitialized();
-            {
+                let mut outer_y: c_int = mem::uninitialized();
-                return None;
+                let mut outer_width: c_uint = mem::uninitialized();
-            }
+                let mut outer_height: c_uint = mem::uninitialized();
+                let mut border: c_uint = mem::uninitialized();
+                let mut depth: c_uint = mem::uninitialized();
 
-            Some((x as i32, y as i32, width_out as u32, height_out as u32, border_out as u32))
+                let status = (self.x.display.xlib.XGetGeometry)(
+                    self.x.display.display,
+                    window,
+                    &mut root,
+                    &mut outer_x,
+                    &mut outer_y,
+                    &mut outer_width,
+                    &mut outer_height,
+                    &mut border,
+                    &mut depth,
+                );
+
+                if status == 0 {
+                    return None;
+                }
+
+                (outer_x, outer_y, outer_width, outer_height)
+            };
+
+            // Since we have the geometry of the outermost window and the geometry of the client
+            // area, we can figure out what's in between.
+            let frame = {
+                let diff_x = outer_width.saturating_sub(width);
+                let diff_y = outer_height.saturating_sub(height);
+                let offset_y = inner_y_rel_root.saturating_sub(outer_y) as c_uint;
+
+                let left = diff_x / 2;
+                let right = left;
+                let top = offset_y;
+                let bottom = diff_y.saturating_sub(offset_y);
+
+                util::FrameExtents::new(left.into(), right.into(), top.into(), bottom.into())
+            };
+
+            Some(util::WindowGeometry {
+                x: outer_x,
+                y: outer_y,
+                width,
+                height,
+                frame,
+            })
+        } else {
+            // This is the case for xmonad and dwm, AKA the only WMs tested that supplied a
+            // border value. This is convenient, since we can use it to get an accurate frame.
+            let frame = util::FrameExtents::from_border(border.into());
+            Some(util::WindowGeometry {
+                x: inner_x,
+                y: inner_y,
+                width,
+                height,
+                frame,
+            })
         }
     }
 
     #[inline]
     pub fn get_position(&self) -> Option<(i32, i32)> {
-        self.get_geometry().map(|(x, y, _, _, _)| (x, y))
+        self.get_geometry().map(|geo| geo.get_position())
     }
 
-    pub fn set_position(&self, x: i32, y: i32) {
+    pub fn set_position(&self, mut x: i32, mut y: i32) {
-        unsafe { (self.x.display.xlib.XMoveWindow)(self.x.display.display, self.x.window, x as libc::c_int, y as libc::c_int); }
+        if let Some(ref wm_name) = self.wm_name {
+            // There are a few WMs that set client area position rather than window position, so
+            // we'll translate for consistency.
+            if ["Enlightenment", "FVWM"].contains(&wm_name.as_str()) {
+                if let Some(extents) = self.get_frame_extents() {
+                    x += extents.left as i32;
+                    y += extents.top as i32;
+                }
+            }
+        }
+        unsafe {
+            (self.x.display.xlib.XMoveWindow)(
+                self.x.display.display,
+                self.x.window,
+                x as c_int,
+                y as c_int,
+            );
+        }
         self.x.display.check_errors().expect("Failed to call XMoveWindow");
     }
 
     #[inline]
     pub fn get_inner_size(&self) -> Option<(u32, u32)> {
-        self.get_geometry().map(|(_, _, w, h, _)| (w, h))
+        self.get_geometry().map(|geo| geo.get_inner_size())
     }
 
     #[inline]
     pub fn get_outer_size(&self) -> Option<(u32, u32)> {
-        self.get_geometry().map(|(_, _, w, h, b)| (w + b, h + b))       // TODO: is this really outside?
+        self.get_geometry().map(|geo| geo.get_outer_size())
     }
 
     #[inline]