X11: split off Visual Info negotiation into a separate module (#177)

src/x11/mod.rs

@@ -6,3 +6,4 @@ pub use window::*;
 
 mod cursor;
 mod keyboard;
+mod visual_info;

src/x11/visual_info.rs

@@ -0,0 +1,94 @@
+use crate::x11::xcb_connection::XcbConnection;
+use std::error::Error;
+use x11rb::connection::Connection;
+use x11rb::protocol::xproto::{
+    Colormap, ColormapAlloc, ConnectionExt, Screen, VisualClass, Visualid,
+};
+use x11rb::COPY_FROM_PARENT;
+
+pub(super) struct WindowVisualConfig {
+    #[cfg(feature = "opengl")]
+    pub fb_config: Option<crate::gl::x11::FbConfig>,
+
+    pub visual_depth: u8,
+    pub visual_id: Visualid,
+    pub color_map: Option<Colormap>,
+}
+
+// TODO: make visual negotiation actually check all of a visual's parameters
+impl WindowVisualConfig {
+    #[cfg(feature = "opengl")]
+    pub fn find_best_visual_config_for_gl(
+        connection: &XcbConnection, gl_config: Option<crate::gl::GlConfig>,
+    ) -> Result<Self, Box<dyn Error>> {
+        let Some(gl_config) = gl_config else { return Self::find_best_visual_config(connection) };
+
+        // SAFETY: TODO
+        let (fb_config, window_config) = unsafe {
+            crate::gl::platform::GlContext::get_fb_config_and_visual(connection.dpy, gl_config)
+        }
+        .expect("Could not fetch framebuffer config");
+
+        Ok(Self {
+            fb_config: Some(fb_config),
+            visual_depth: window_config.depth,
+            visual_id: window_config.visual,
+            color_map: Some(create_color_map(connection, window_config.visual)?),
+        })
+    }
+
+    pub fn find_best_visual_config(connection: &XcbConnection) -> Result<Self, Box<dyn Error>> {
+        match find_visual_for_depth(connection.screen(), 32) {
+            None => Ok(Self::copy_from_parent()),
+            Some(visual_id) => Ok(Self {
+                #[cfg(feature = "opengl")]
+                fb_config: None,
+                visual_id,
+                visual_depth: 32,
+                color_map: Some(create_color_map(connection, visual_id)?),
+            }),
+        }
+    }
+
+    const fn copy_from_parent() -> Self {
+        Self {
+            #[cfg(feature = "opengl")]
+            fb_config: None,
+            visual_depth: COPY_FROM_PARENT as u8,
+            visual_id: COPY_FROM_PARENT,
+            color_map: None,
+        }
+    }
+}
+
+// For this 32-bit depth to work, you also need to define a color map and set a border
+// pixel: https://cgit.freedesktop.org/xorg/xserver/tree/dix/window.c#n818
+pub fn create_color_map(
+    connection: &XcbConnection, visual_id: Visualid,
+) -> Result<Colormap, Box<dyn Error>> {
+    let colormap = connection.conn.generate_id()?;
+    connection.conn.create_colormap(
+        ColormapAlloc::NONE,
+        colormap,
+        connection.screen().root,
+        visual_id,
+    )?;
+
+    Ok(colormap)
+}
+
+fn find_visual_for_depth(screen: &Screen, depth: u8) -> Option<Visualid> {
+    for candidate_depth in &screen.allowed_depths {
+        if candidate_depth.depth != depth {
+            continue;
+        }
+
+        for candidate_visual in &candidate_depth.visuals {
+            if candidate_visual.class == VisualClass::TRUE_COLOR {
+                return Some(candidate_visual.visual_id);
+            }
+        }
+    }
+
+    None
+}

src/x11/window.rs

@@ -14,9 +14,8 @@ use raw_window_handle::{
 
 use x11rb::connection::Connection;
 use x11rb::protocol::xproto::{
-    AtomEnum, ChangeWindowAttributesAux, ColormapAlloc, ConfigureWindowAux, ConnectionExt as _,
+    AtomEnum, ChangeWindowAttributesAux, ConfigureWindowAux, ConnectionExt as _, CreateGCAux,
-    CreateGCAux, CreateWindowAux, EventMask, PropMode, Screen, VisualClass, Visualid,
+    CreateWindowAux, EventMask, PropMode, Visualid, Window as XWindow, WindowClass,
-    Window as XWindow, WindowClass,
 };
 use x11rb::protocol::Event as XEvent;
 use x11rb::wrapper::ConnectionExt as _;
@@ -31,6 +30,7 @@ use super::keyboard::{convert_key_press_event, convert_key_release_event, key_mo
 
 #[cfg(feature = "opengl")]
 use crate::gl::{platform, GlContext};
+use crate::x11::visual_info::WindowVisualConfig;
 
 pub struct WindowHandle {
     raw_window_handle: Option<RawWindowHandle>,
@@ -187,11 +187,9 @@ impl<'a> Window<'a> {
         // FIXME: baseview error type instead of unwrap()
         let xcb_connection = XcbConnection::new()?;
 
-        // Get screen information (?)
+        // Get screen information
-        let setup = xcb_connection.conn.setup();
+        let screen = xcb_connection.screen();
-        let screen = &setup.roots[xcb_connection.screen];
+        let parent_id = parent.unwrap_or(screen.root);
-
-        let parent_id = parent.unwrap_or_else(|| screen.root);
 
         let gc_id = xcb_connection.conn.generate_id()?;
         xcb_connection.conn.create_gc(
@@ -207,39 +205,16 @@ impl<'a> Window<'a> {
 
         let window_info = WindowInfo::from_logical_size(options.size, scaling);
 
-        // Now it starts becoming fun. If we're creating an OpenGL context, then we need to create
-        // the window with a visual that matches the framebuffer used for the OpenGL context. So the
-        // idea is that we first retrieve a framebuffer config that matches our wanted OpenGL
-        // configuration, find the visual that matches that framebuffer config, create the window
-        // with that visual, and then finally create an OpenGL context for the window. If we don't
-        // use OpenGL, then we'll just take a random visual with a 32-bit depth.
-        let create_default_config = || {
-            Self::find_visual_for_depth(screen, 32)
-                .map(|visual| (32, visual))
-                .unwrap_or((x11rb::COPY_FROM_PARENT as u8, x11rb::COPY_FROM_PARENT as u32))
-        };
         #[cfg(feature = "opengl")]
-        let (fb_config, (depth, visual)) = match options.gl_config {
+        let visual_info =
-            Some(gl_config) => unsafe {
+            WindowVisualConfig::find_best_visual_config_for_gl(&xcb_connection, options.gl_config)?;
-                platform::GlContext::get_fb_config_and_visual(xcb_connection.dpy, gl_config)
-                    .map(|(fb_config, window_config)| {
-                        (Some(fb_config), (window_config.depth, window_config.visual))
-                    })
-                    .expect("Could not fetch framebuffer config")
-            },
-            None => (None, create_default_config()),
-        };
-        #[cfg(not(feature = "opengl"))]
-        let (depth, visual) = create_default_config();
 
-        // For this 32-bith depth to work, you also need to define a color map and set a border
+        #[cfg(not(feature = "opengl"))]
-        // pixel: https://cgit.freedesktop.org/xorg/xserver/tree/dix/window.c#n818
+        let visual_info = WindowVisualConfig::find_best_visual_config(&xcb_connection)?;
-        let colormap = xcb_connection.conn.generate_id()?;
-        xcb_connection.conn.create_colormap(ColormapAlloc::NONE, colormap, screen.root, visual)?;
 
         let window_id = xcb_connection.conn.generate_id()?;
         xcb_connection.conn.create_window(
-            depth,
+            visual_info.visual_depth,
             window_id,
             parent_id,
             0,                                         // x coordinate of the new window
@@ -248,7 +223,7 @@ impl<'a> Window<'a> {
             window_info.physical_size().height as u16, // window height
             0,                                         // window border
             WindowClass::INPUT_OUTPUT,
-            visual,
+            visual_info.visual_id,
             &CreateWindowAux::new()
                 .event_mask(
                     EventMask::EXPOSURE
@@ -263,7 +238,7 @@ impl<'a> Window<'a> {
                 )
                 // As mentioned above, these two values are needed to be able to create a window
                 // with a depth of 32-bits when the parent window has a different depth
-                .colormap(colormap)
+                .colormap(visual_info.color_map)
                 .border_pixel(0),
         )?;
         xcb_connection.conn.map_window(window_id)?;
@@ -292,7 +267,7 @@ impl<'a> Window<'a> {
         //       no error handling anymore at this point. Everything is more or less unchanged
         //       compared to when raw-gl-context was a separate crate.
         #[cfg(feature = "opengl")]
-        let gl_context = fb_config.map(|fb_config| {
+        let gl_context = visual_info.fb_config.map(|fb_config| {
             use std::ffi::c_ulong;
 
             let window = window_id as c_ulong;
@@ -308,7 +283,7 @@ impl<'a> Window<'a> {
             xcb_connection,
             window_id,
             window_info,
-            visual_id: visual,
+            visual_id: visual_info.visual_id,
             mouse_cursor: MouseCursor::default(),
 
             frame_interval: Duration::from_millis(15),
@@ -387,22 +362,6 @@ impl<'a> Window<'a> {
     pub fn gl_context(&self) -> Option<&crate::gl::GlContext> {
         self.inner.gl_context.as_ref()
     }
-
-    fn find_visual_for_depth(screen: &Screen, depth: u8) -> Option<Visualid> {
-        for candidate_depth in &screen.allowed_depths {
-            if candidate_depth.depth != depth {
-                continue;
-            }
-
-            for candidate_visual in &candidate_depth.visuals {
-                if candidate_visual.class == VisualClass::TRUE_COLOR {
-                    return Some(candidate_visual.visual_id);
-                }
-            }
-        }
-
-        None
-    }
 }
 
 impl WindowInner {

src/x11/xcb_connection.rs

@@ -5,7 +5,7 @@ use x11::{xlib, xlib::Display, xlib_xcb};
 
 use x11rb::connection::Connection;
 use x11rb::cursor::Handle as CursorHandle;
-use x11rb::protocol::xproto::Cursor;
+use x11rb::protocol::xproto::{Cursor, Screen};
 use x11rb::resource_manager;
 use x11rb::xcb_ffi::XCBConnection;
 
@@ -76,8 +76,7 @@ impl XcbConnection {
     // If neither work, I guess just assume 96.0 and don't do any scaling.
     fn get_scaling_screen_dimensions(&self) -> f64 {
         // Figure out screen information
-        let setup = self.conn.setup();
+        let screen = self.screen();
-        let screen = &setup.roots[self.screen];
 
         // Get the DPI from the screen struct
         //
@@ -115,4 +114,8 @@ impl XcbConnection {
             }
         }
     }
+
+    pub fn screen(&self) -> &Screen {
+        &self.conn.setup().roots[self.screen]
+    }
 }