x11: Send window maximization hints correctly (#363)

Previously, the maximization hints were being sent as two separate client messages: one for
horizontal, and one for vertical. That resulted in the window only being maximized
horizontally (at least with my WM). The corrected client message sets both of these hints at
once.

In the process of implementing that, the relevant components were refactored to use the util
module, as we gradually move towards a hopeful future of a more readable X11 backend.

src/platform/linux/x11/dnd.rs

@@ -55,7 +55,9 @@ impl DndAtoms {
             );
         }
         xconn.check_errors()?;
-        unsafe { atoms.set_len(DND_ATOMS_LEN); }
+        unsafe {
+            atoms.set_len(DND_ATOMS_LEN);
+        }
         Ok(DndAtoms {
             aware: atoms[0],
             enter: atoms[1],
@@ -137,7 +139,7 @@ impl Dnd {
         this_window: c_ulong,
         target_window: c_ulong,
         state: DndState,
-    ) {
+    ) -> Result<(), XError> {
         let (accepted, action) = match state {
             DndState::Accepted => (1, self.atoms.action_private as c_long),
             DndState::Rejected => (0, self.atoms.none as c_long),
@@ -145,9 +147,11 @@ impl Dnd {
         util::send_client_msg(
             &self.xconn,
             target_window,
+            target_window,
             self.atoms.status,
+            None,
             (this_window as c_long, accepted, 0, 0, action),
-        );
+        )
     }
 
     pub unsafe fn send_finished(
@@ -155,7 +159,7 @@ impl Dnd {
         this_window: c_ulong,
         target_window: c_ulong,
         state: DndState,
-    ) {
+    ) -> Result<(), XError> {
         let (accepted, action) = match state {
             DndState::Accepted => (1, self.atoms.action_private as c_long),
             DndState::Rejected => (0, self.atoms.none as c_long),
@@ -163,9 +167,11 @@ impl Dnd {
         util::send_client_msg(
             &self.xconn,
             target_window,
+            target_window,
             self.atoms.finished,
+            None,
             (this_window as c_long, accepted, action, 0, 0),
-        );
+        )
     }
 
     pub unsafe fn get_type_list(

src/platform/linux/x11/mod.rs

@@ -276,12 +276,15 @@ impl EventsLoop {
                                 // This results in the SelectionNotify event below
                                 self.dnd.convert_selection(xwindow, time);
                             }
-                            self.dnd.send_status(xwindow, source_window, DndState::Accepted);
+                            self.dnd.send_status(xwindow, source_window, DndState::Accepted)
+                                .expect("Failed to send XDnD status message.");
                         }
                     } else {
                         unsafe {
-                            self.dnd.send_status(xwindow, source_window, DndState::Rejected);
+                            self.dnd.send_status(xwindow, source_window, DndState::Rejected)
-                            self.dnd.send_finished(xwindow, source_window, DndState::Rejected);
+                                .expect("Failed to send XDnD status message.");
+                            self.dnd.send_finished(xwindow, source_window, DndState::Rejected)
+                                .expect("Failed to send XDnD finished message.");
                         }
                         self.dnd.reset();
                     }
@@ -296,7 +299,8 @@ impl EventsLoop {
                             }
                         }
                         unsafe {
-                            self.dnd.send_finished(xwindow, source_window, DndState::Accepted);
+                            self.dnd.send_finished(xwindow, source_window, DndState::Accepted)
+                                .expect("Failed to send XDnD finished message.");
                         }
                     }
                     self.dnd.reset();

src/platform/linux/x11/util.rs

@@ -6,16 +6,24 @@ use libc::{c_char, c_int, c_long, c_short, c_uchar, c_ulong};
 
 use super::{ffi, XConnection, XError};
 
+pub unsafe fn get_atom(xconn: &Arc<XConnection>, name: &[u8]) -> Result<ffi::Atom, XError> {
+    let atom_name: *const c_char = name.as_ptr() as _;
+    let atom = (xconn.xlib.XInternAtom)(xconn.display, atom_name, ffi::False);
+    xconn.check_errors().map(|_| atom)
+}
+
 pub unsafe fn send_client_msg(
     xconn: &Arc<XConnection>,
-    target_window: c_ulong,
+    window: c_ulong,        // the window this is "about"; not necessarily this window
+    target_window: c_ulong, // the window we're sending to
     message_type: ffi::Atom,
+    event_mask: Option<c_long>,
     data: (c_long, c_long, c_long, c_long, c_long),
-) {
+) -> Result<(), XError> {
     let mut event: ffi::XClientMessageEvent = mem::uninitialized();
     event.type_ = ffi::ClientMessage;
     event.display = xconn.display;
-    event.window = target_window;
+    event.window = window;
     event.message_type = message_type;
     event.format = 32;
     event.data = ffi::ClientMessageData::new();
@@ -25,13 +33,17 @@ pub unsafe fn send_client_msg(
     event.data.set_long(3, data.3);
     event.data.set_long(4, data.4);
 
+    let event_mask = event_mask.unwrap_or(ffi::NoEventMask);
+
     (xconn.xlib.XSendEvent)(
         xconn.display,
         target_window,
         ffi::False,
-        ffi::NoEventMask,
+        event_mask,
         &mut event.into(),
     );
+
+    xconn.check_errors().map(|_| ())
 }
 
 #[derive(Debug)]

src/platform/linux/x11/window.rs

@@ -19,8 +19,7 @@ use window::MonitorId as RootMonitorId;
 
 use platform::x11::monitor::get_available_monitors;
 
-use super::{ffi};
+use super::{ffi, util, XConnection, WindowId, EventsLoop};
-use super::{XConnection, WindowId, EventsLoop};
 
 // TODO: remove me
 fn with_c_str<F, T>(s: &str, f: F) -> T where F: FnOnce(*const libc::c_char) -> T {
@@ -29,6 +28,24 @@ fn with_c_str<F, T>(s: &str, f: F) -> T where F: FnOnce(*const libc::c_char) ->
     f(c_str.as_ptr())
 }
 
+#[derive(Debug)]
+enum StateOperation {
+    Remove = 0, // _NET_WM_STATE_REMOVE
+    Add = 1, // _NET_WM_STATE_ADD
+    #[allow(dead_code)]
+    Toggle = 2, // _NET_WM_STATE_TOGGLE
+}
+
+impl From<bool> for StateOperation {
+    fn from(b: bool) -> Self {
+        if b {
+            StateOperation::Add
+        } else {
+            StateOperation::Remove
+        }
+    }
+}
+
 pub struct XWindow {
     display: Arc<XConnection>,
     window: ffi::Window,
@@ -145,8 +162,8 @@ impl Window2 {
 
             // Enable drag and drop
             unsafe {
-                let atom_name: *const libc::c_char = b"XdndAware\0".as_ptr() as _;
+                let atom = util::get_atom(display, b"XdndAware\0")
-                let atom = (display.xlib.XInternAtom)(display.display, atom_name, ffi::False);
+                    .expect("Failed to call XInternAtom (XdndAware)");
                 let version = &5; // Latest version; hasn't changed since 2002
                 (display.xlib.XChangeProperty)(
                     display.display,
@@ -275,49 +292,32 @@ impl Window2 {
         Ok(window)
     }
 
-    fn set_netwm(display: &Arc<XConnection>, window: ffi::Window, root: ffi::Window, property: &str, val: bool) {
+    fn set_netwm(
-        let state_atom = unsafe {
+        xconn: &Arc<XConnection>,
-            with_c_str("_NET_WM_STATE", |state|
+        window: ffi::Window,
-                (display.xlib.XInternAtom)(display.display, state, 0)
+        root: ffi::Window,
-            )
+        properties: (c_long, c_long, c_long, c_long),
-        };
+        operation: StateOperation
-        display.check_errors().expect("Failed to call XInternAtom");
+    ) {
-        let atom = unsafe {
+        let state_atom = unsafe { util::get_atom(xconn, b"_NET_WM_STATE\0") }
-            with_c_str(property, |state|
+            .expect("Failed to call XInternAtom (_NET_WM_STATE)");
-                (display.xlib.XInternAtom)(display.display, state, 0)
-            )
-        };
-        display.check_errors().expect("Failed to call XInternAtom");
-
-        let client_message_event = ffi::XClientMessageEvent {
-            type_: ffi::ClientMessage,
-            serial: 0,
-            send_event: 1,            // true because we are sending this through `XSendEvent`
-            display: display.display,
-            window: window,
-            message_type: state_atom, // the _NET_WM_STATE atom is sent to change the state of a window
-            format: 32,               // view `data` as `c_long`s
-            data: {
-                let mut data = ffi::ClientMessageData::new();
-                // This first `long` is the action; `1` means add/set following property.
-                data.set_long(0, val as c_long);
-                // This second `long` is the property to set (fullscreen)
-                data.set_long(1, atom as c_long);
-                data
-            }
-        };
-        let mut x_event = ffi::XEvent::from(client_message_event);
 
         unsafe {
-            (display.xlib.XSendEvent)(
+            util::send_client_msg(
-                display.display,
+                xconn,
+                window,
                 root,
-                0,
+                state_atom,
-                ffi::SubstructureRedirectMask | ffi::SubstructureNotifyMask,
+                Some(ffi::SubstructureRedirectMask | ffi::SubstructureNotifyMask),
-                &mut x_event as *mut _
+                (
-            );
+                    operation as c_long,
-            display.check_errors().expect("Failed to call XSendEvent");
+                    properties.0,
-        }
+                    properties.1,
+                    properties.2,
+                    properties.3,
+                )
+            )
+        }.expect("Failed to send NET_WM hint.");
     }
 
     pub fn set_fullscreen(&self, monitor: Option<RootMonitorId>) {
@@ -374,12 +374,35 @@ impl Window2 {
     }
 
     pub fn set_maximized(&self, maximized: bool) {
-        Window2::set_netwm(&self.x.display, self.x.window, self.x.root, "_NET_WM_STATE_MAXIMIZED_HORZ", maximized);
+        let xconn = &self.x.display;
-        Window2::set_netwm(&self.x.display, self.x.window, self.x.root, "_NET_WM_STATE_MAXIMIZED_VERT", maximized);
+
+        let horz_atom = unsafe { util::get_atom(xconn, b"_NET_WM_STATE_MAXIMIZED_HORZ\0") }
+            .expect("Failed to call XInternAtom (_NET_WM_STATE_MAXIMIZED_HORZ)");
+        let vert_atom = unsafe { util::get_atom(xconn, b"_NET_WM_STATE_MAXIMIZED_VERT\0") }
+            .expect("Failed to call XInternAtom (_NET_WM_STATE_MAXIMIZED_VERT)");
+
+        Window2::set_netwm(
+            xconn,
+            self.x.window,
+            self.x.root,
+            (horz_atom as c_long, vert_atom as c_long, 0, 0),
+            maximized.into()
+        );
     }
 
     fn set_fullscreen_hint(&self, fullscreen: bool) {
-        Window2::set_netwm(&self.x.display, self.x.window, self.x.root, "_NET_WM_STATE_FULLSCREEN", fullscreen);
+        let xconn = &self.x.display;
+
+        let fullscreen_atom = unsafe { util::get_atom(xconn, b"_NET_WM_STATE_FULLSCREEN\0") }
+            .expect("Failed to call XInternAtom (_NET_WM_STATE_FULLSCREEN)");
+
+        Window2::set_netwm(
+            xconn,
+            self.x.window,
+            self.x.root,
+            (fullscreen_atom as c_long, 0, 0, 0),
+            fullscreen.into()
+        );
     }
 
     pub fn set_title(&self, title: &str) {