fix set-position constraints

Author: Ktrompfl

jay-config/src/_private/client.rs

@@ -34,8 +34,8 @@ use {
             connector_type::{CON_UNKNOWN, ConnectorType},
         },
         window::{
-            ContentType, MatchedWindow, TileState, Window, WindowCriterion, WindowMatcher,
-            WindowType,
+            ContentType, Coordinate, MatchedWindow, TileState, Window, WindowCriterion,
+            WindowMatcher, WindowType,
         },
         workspace::WorkspaceDisplayOrder,
         xwayland::XScalingMode,
@@ -611,12 +611,12 @@ impl ConfigClient {
     pub fn set_window_position(
         &self,
         window: Window,
-        x1: Option<i32>,
-        y1: Option<i32>,
-        x2: Option<i32>,
-        y2: Option<i32>,
-        width: Option<i32>,
-        height: Option<i32>,
+        x1: Option<Coordinate>,
+        y1: Option<Coordinate>,
+        x2: Option<Coordinate>,
+        y2: Option<Coordinate>,
+        width: Option<Coordinate>,
+        height: Option<Coordinate>,
     ) {
         self.send(&ClientMessage::SetWindowPosition {
             window,

jay-config/src/_private/ipc.rs

@@ -19,7 +19,7 @@ use {
             BlendSpace, ColorSpace, Connector, DrmDevice, Eotf, Format, GfxApi, ScalingFilter,
             TearingMode, Transform, VrrMode, connector_type::ConnectorType,
         },
-        window::{ContentType, TileState, Window, WindowMatcher, WindowType},
+        window::{ContentType, Coordinate, TileState, Window, WindowMatcher, WindowType},
         workspace::WorkspaceDisplayOrder,
         xwayland::XScalingMode,
     },
@@ -986,12 +986,12 @@ pub enum ClientMessage<'a> {
     },
     SetWindowPosition {
         window: Window,
-        x1: Option<i32>,
-        y1: Option<i32>,
-        x2: Option<i32>,
-        y2: Option<i32>,
-        width: Option<i32>,
-        height: Option<i32>,
+        x1: Option<Coordinate>,
+        y1: Option<Coordinate>,
+        x2: Option<Coordinate>,
+        y2: Option<Coordinate>,
+        width: Option<Coordinate>,
+        height: Option<Coordinate>,
     },
     GetWorkspacePosition {
         workspace: Workspace,

jay-config/src/input.rs

@@ -16,7 +16,7 @@ use {
         },
         keyboard::{Keymap, mods::Modifiers, syms::KeySym},
         video::Connector,
-        window::Window,
+        window::{Coordinate, Window},
     },
     serde::{Deserialize, Serialize},
     std::time::Duration,
@@ -718,12 +718,12 @@ impl Seat {
     /// See [`Window::set_position`](crate::window::Window::set_position) for details.
     pub fn set_position(
         self,
-        x1: Option<i32>,
-        y1: Option<i32>,
-        x2: Option<i32>,
-        y2: Option<i32>,
-        width: Option<i32>,
-        height: Option<i32>,
+        x1: Option<Coordinate>,
+        y1: Option<Coordinate>,
+        x2: Option<Coordinate>,
+        y2: Option<Coordinate>,
+        width: Option<Coordinate>,
+        height: Option<Coordinate>,
     ) {
         self.window().set_position(x1, y1, x2, y2, width, height);
     }

jay-config/src/window.rs

@@ -2,9 +2,9 @@
 
 use {
     crate::{
-        Axis, Direction, Workspace,
         client::{Client, ClientCriterion},
         input::Seat,
+        Axis, Direction, Workspace,
     },
     serde::{Deserialize, Serialize},
     std::ops::Deref,
@@ -66,6 +66,15 @@ pub enum TileState {
     Floating,
 }
 
+/// A constraint on a coordinate or size passed to [`Window::set_position`].
+#[derive(Serialize, Deserialize, Copy, Clone, Debug, Hash, Eq, PartialEq)]
+pub enum Coordinate {
+    /// Constrain this field to the window's current value for it.
+    Keep,
+    /// Constrain this field to this value.
+    Set(i32),
+}
+
 /// A window created by a client.
 ///
 /// This is the same as `XDG_TOPLEVEL | X_WINDOW`.
@@ -256,25 +265,17 @@ impl Window {
     ///
     /// This only has an effect if the window is floating.
     ///
-    /// Every argument that is `None` is left unconstrained by this call. Every argument that
-    /// is `Some(_)` is constrained to that value.
-    ///
-    /// Since `x2 = x1 + width` (and analogously for the y axis), not every combination of
-    /// constraints is satisfiable. If `x1`, `x2`, and `width` are all constrained but not
-    /// self-consistent (and analogously for `y1`, `y2`, `height`), this call has no effect.
-    ///
-    /// If fewer than two of `x1`, `x2`, `width` are constrained (and analogously for `y1`,
-    /// `y2`, `height`), the missing values default to preserving the window's current size,
-    /// e.g. setting only `width` keeps `x1` fixed and moves `x2`, while setting only `x1` keeps
-    /// the width fixed and moves `x2` along with it.
+    /// For each axis, missing constraints (fewer than two given) are inferred,
+    /// preserving the window's current position rather than its size; if all
+    /// three are given but unsatisfiable for an axis, this is a no-op.
     pub fn set_position(
         self,
-        x1: Option<i32>,
-        y1: Option<i32>,
-        x2: Option<i32>,
-        y2: Option<i32>,
-        width: Option<i32>,
-        height: Option<i32>,
+        x1: Option<Coordinate>,
+        y1: Option<Coordinate>,
+        x2: Option<Coordinate>,
+        y2: Option<Coordinate>,
+        width: Option<Coordinate>,
+        height: Option<Coordinate>,
     ) {
         get!().set_window_position(self, x1, y1, x2, y2, width, height);
     }

src/config/handler.rs

@@ -83,7 +83,7 @@ use {
             Format as ConfigFormat, GfxApi, ScalingFilter as ConfigScalingFilter,
             TearingMode as ConfigTearingMode, Transform, VrrMode as ConfigVrrMode,
         },
-        window::{TileState as ConfigTileState, Window, WindowMatcher},
+        window::{Coordinate, TileState as ConfigTileState, Window, WindowMatcher},
         workspace::WorkspaceDisplayOrder,
         xwayland::XScalingMode,
     },
@@ -3049,12 +3049,12 @@ impl ConfigProxyHandler {
     fn handle_set_window_position(
         &self,
         window: Window,
-        x1: Option<i32>,
-        y1: Option<i32>,
-        x2: Option<i32>,
-        y2: Option<i32>,
-        width: Option<i32>,
-        height: Option<i32>,
+        x1: Option<Coordinate>,
+        y1: Option<Coordinate>,
+        x2: Option<Coordinate>,
+        y2: Option<Coordinate>,
+        width: Option<Coordinate>,
+        height: Option<Coordinate>,
     ) -> Result<(), CphError> {
         let tl = self.get_window(window)?;
         let pos = tl.node_absolute_position(LiveTL);
@@ -4179,24 +4179,22 @@ impl ConfigProxyHandler {
     }
 }
 
-/// Resolves the constraints on a single axis (x1/x2/width or y1/y2/height) of
-/// [`ConfigProxyHandler::handle_set_window_position`] into a concrete `(c1, c2)` pair.
-///
-/// `c1`, `c2` and `size` are constrained to their contained value if they are `Some(_)` and
-/// are otherwise unconstrained.
-///
-/// If two of the three are constrained, the third is derived from them. If only one is
-/// constrained, the window keeps its current size (if `c1` or `c2` is constrained) or its
-/// current position (if only `size` is constrained). If none are constrained, the axis is
-/// left unchanged. If all three are constrained but not self-consistent, an error is returned.
 fn resolve_geometry_axis(
-    c1: Option<i32>,
-    c2: Option<i32>,
-    size: Option<i32>,
+    c1: Option<Coordinate>,
+    c2: Option<Coordinate>,
+    size: Option<Coordinate>,
     cur_c1: i32,
     cur_c2: i32,
 ) -> Result<(i32, i32), CphError> {
     let cur_size = cur_c2.saturating_sub(cur_c1);
+    let resolve = |c: Option<Coordinate>, cur: i32| match c {
+        Some(Coordinate::Set(v)) => Some(v),
+        Some(Coordinate::Keep) => Some(cur),
+        None => None,
+    };
+    let c1 = resolve(c1, cur_c1);
+    let c2 = resolve(c2, cur_c2);
+    let size = resolve(size, cur_size);
     let res = match (c1, c2, size) {
         (Some(c1), Some(c2), Some(size)) => {
             if c2.saturating_sub(c1) != size {
@@ -4207,8 +4205,8 @@ fn resolve_geometry_axis(
         (Some(c1), Some(c2), None) => (c1, c2),
         (Some(c1), None, Some(size)) => (c1, c1.saturating_add(size)),
         (None, Some(c2), Some(size)) => (c2.saturating_sub(size), c2),
-        (Some(c1), None, None) => (c1, c1.saturating_add(cur_size)),
-        (None, Some(c2), None) => (c2.saturating_sub(cur_size), c2),
+        (Some(c1), None, None) => (c1, cur_c2),
+        (None, Some(c2), None) => (cur_c1, c2),
         (None, None, Some(size)) => (cur_c1, cur_c1.saturating_add(size)),
         (None, None, None) => (cur_c1, cur_c2),
     };

toml-config/src/config.rs

@@ -42,7 +42,7 @@ use {
             BlendSpace, ColorSpace, Connector, Eotf, Format, GfxApi, ScalingFilter, TearingMode,
             Transform, VrrMode,
         },
-        window::{ContentType, TileState, WindowType},
+        window::{ContentType, Coordinate, TileState, WindowType},
         workspace::WorkspaceDisplayOrder,
         xwayland::XScalingMode,
     },
@@ -215,12 +215,12 @@ pub enum Action {
         ws: Rc<WorkspaceSlot>,
     },
     SetPosition {
-        x1: Option<i32>,
-        y1: Option<i32>,
-        x2: Option<i32>,
-        y2: Option<i32>,
-        width: Option<i32>,
-        height: Option<i32>,
+        x1: Option<Coordinate>,
+        y1: Option<Coordinate>,
+        x2: Option<Coordinate>,
+        y2: Option<Coordinate>,
+        width: Option<Coordinate>,
+        height: Option<Coordinate>,
     },
 }
 

toml-config/src/config/parsers.rs

@@ -18,6 +18,7 @@ pub mod config;
 mod connector;
 mod connector_match;
 mod content_type;
+mod coordinate;
 mod drm_device;
 mod drm_device_match;
 mod egui;

toml-config/src/config/parsers/action.rs

@@ -8,6 +8,7 @@ use {
             parsers::{
                 StringParser, StringParserError,
                 connector::{ConnectorParser, ConnectorParserError},
+                coordinate::{CoordinateParser, CoordinateParserError},
                 drm_device::{DrmDeviceParser, DrmDeviceParserError},
                 drm_device_match::{DrmDeviceMatchParser, DrmDeviceMatchParserError},
                 env::{EnvParser, EnvParserError},
@@ -97,6 +98,8 @@ pub enum ActionParserError {
     ShowOverlay(#[source] ShowWorkspaceError),
     #[error("Could not parse a toggle-overlay action")]
     ToggleOverlay(#[source] ShowWorkspaceError),
+    #[error("Could not parse a coordinate")]
+    Coordinate(#[source] CoordinateParserError),
     #[error("Unknown direction {0}")]
     UnknownDirection(String),
     #[error("Exactly one of `output` or `direction` must be specified")]
@@ -111,28 +114,6 @@ pub enum ShowWorkspaceError {
     FallbackOutputModeParser(FallbackOutputModeParserError),
 }
 
-/// Extracts a field that should either be an integer or the string `"keep"`.
-///
-/// `"keep"` is represented as `None`, an integer `n` is represented as `Some(n)`.
-fn coordinate(
-    name: &'static str,
-) -> impl for<'v, 'w> FnOnce(
-    &mut Extractor<'v, 'w>,
-) -> Result<Spanned<Option<i32>>, Spanned<ExtractorError>> {
-    move |extractor: &mut Extractor| {
-        val(name)(extractor).and_then(|v| match v.value {
-            Value::String(s) if s.as_str() == "keep" => Ok(None.spanned(v.span)),
-            Value::Integer(i) => match i32::try_from(*i) {
-                Ok(n) => Ok(Some(n).spanned(v.span)),
-                Err(_) => Err(ExtractorError::I32.spanned(v.span)),
-            },
-            _ => Err(
-                ExtractorError::Expected("an integer or \"keep\"", v.value.name()).spanned(v.span),
-            ),
-        })
-    }
-}
-
 pub struct ActionParser<'a, 'b>(pub &'a Context<'b>);
 
 impl ActionParser<'_, '_> {
@@ -625,20 +606,27 @@ impl ActionParser<'_, '_> {
 
     fn parse_set_position(&mut self, ext: &mut Extractor<'_, '_>) -> ParseResult<Self> {
         let (x1, y1, x2, y2, width, height) = ext.extract((
-            opt(coordinate("x1")),
-            opt(coordinate("y1")),
-            opt(coordinate("x2")),
-            opt(coordinate("y2")),
-            opt(coordinate("width")),
-            opt(coordinate("height")),
+            opt(val("x1")),
+            opt(val("y1")),
+            opt(val("x2")),
+            opt(val("y2")),
+            opt(val("width")),
+            opt(val("height")),
         ))?;
+        let coordinate = |v: Option<Spanned<&Value>>| match v {
+            None => Ok(None),
+            Some(v) => v
+                .parse_map(&mut CoordinateParser)
+                .map_spanned_err(ActionParserError::Coordinate)
+                .map(Some),
+        };
         Ok(Action::SetPosition {
-            x1: x1.despan().flatten(),
-            y1: y1.despan().flatten(),
-            x2: x2.despan().flatten(),
-            y2: y2.despan().flatten(),
-            width: width.despan().flatten(),
-            height: height.despan().flatten(),
+            x1: coordinate(x1)?,
+            y1: coordinate(y1)?,
+            x2: coordinate(x2)?,
+            y2: coordinate(y2)?,
+            width: coordinate(width)?,
+            height: coordinate(height)?,
         })
     }
 

toml-config/src/config/parsers/coordinate.rs

@@ -0,0 +1,40 @@
+use {
+    crate::{
+        config::parser::{DataType, ParseResult, Parser, UnexpectedDataType},
+        toml::toml_span::{Span, SpannedExt},
+    },
+    jay_config::window::Coordinate,
+    thiserror::Error,
+};
+
+#[derive(Debug, Error)]
+pub enum CoordinateParserError {
+    #[error(transparent)]
+    Expected(#[from] UnexpectedDataType),
+    #[error("Expected the string \"keep\" but found \"{0}\"")]
+    UnknownKeyword(String),
+    #[error("Value must fit in a i32")]
+    NotI32,
+}
+
+pub struct CoordinateParser;
+
+impl Parser for CoordinateParser {
+    type Value = Coordinate;
+    type Error = CoordinateParserError;
+    const EXPECTED: &'static [DataType] = &[DataType::String, DataType::Integer];
+
+    fn parse_string(&mut self, span: Span, string: &str) -> ParseResult<Self> {
+        match string {
+            "keep" => Ok(Coordinate::Keep),
+            _ => Err(CoordinateParserError::UnknownKeyword(string.to_string()).spanned(span)),
+        }
+    }
+
+    fn parse_integer(&mut self, span: Span, integer: i64) -> ParseResult<Self> {
+        match i32::try_from(integer) {
+            Ok(n) => Ok(Coordinate::Set(n)),
+            Err(_) => Err(CoordinateParserError::NotI32.spanned(span)),
+        }
+    }
+}