Saving monitor position when disabling or reenabling a monitor #2

Henriklmao · 2025-12-15T16:12:04Z

This pull request introduces persistent saving and loading of monitor states (positions and scales), improving user experience by remembering display configurations across sessions. It also enhances the enable/disable logic for monitors and updates the documentation for clarity.

Persistent Monitor State Management:

Added MonitorState struct and methods (save_monitor_state, load_monitor_state) in configuration.rs to serialize and deserialize monitor positions and scales to a JSON file in the user's config directory. Includes unit tests for this functionality. [1] [2] [3]
On application startup, loads saved monitor states and applies them to detected monitors in main.rs. Saves the current state on exit and after relevant actions (move/scale mode exit, configuration save). [1] [2] [3] [4] [5] [6] [7]

Monitor Enable/Disable Enhancements:

Modified enable/disable logic in list.rs to remember previous position and scale when disabling, and restore them when enabling a monitor.
Updated Monitor struct to include saved_position and saved_scale fields, ensuring these are not serialized.

Testing and Utilities:

Adjusted test utilities to account for new saved_position and saved_scale fields in Monitor. [1] [2]

Documentation Improvements:

Updated README.md for improved formatting, clarified installation steps, and added code block formatting for commands. [1] [2] [3]

These changes collectively make display configuration more robust and user-friendly by persisting monitor layouts and improving the clarity of project documentation.

- Implement snap-to-boundary behavior for default move controls (Arrow keys, hjkl). - Add Shift modifier (Shift+Arrow, Shift+HJKL) for 10px incremental movement. - Add get_geometry to Monitor struct for bounds calculation. - Update Map::handle_events to support new move logic. - Fix render_map test expectation to match zoom behavior. - Add/Update tests for move mode.

Introduces functionality to save and load monitor positions and scales to a JSON file in the user's config directory. Updates monitor enable/disable logic to preserve and restore state, and ensures state is saved on exit and after relevant mode changes. Includes tests for monitor state persistence and updates documentation for clarity.

Removes resetting monitor position to (0,0) when no saved position is found, preserving the current position which may come from persistent state. Also updates scale assignment to prefer saved_scale, then existing scale, then default to 1.0.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

commit b810c1d Author: Henrik <57109108+Henriklmao@users.noreply.github.com> Date: Mon Dec 15 17:09:37 2025 +0100 Preserve monitor position if no saved position exists Removes resetting monitor position to (0,0) when no saved position is found, preserving the current position which may come from persistent state. Also updates scale assignment to prefer saved_scale, then existing scale, then default to 1.0. commit 0a33ca5 Author: Henrik <57109108+Henriklmao@users.noreply.github.com> Date: Mon Dec 15 17:05:38 2025 +0100 Add persistent monitor state saving and loading Introduces functionality to save and load monitor positions and scales to a JSON file in the user's config directory. Updates monitor enable/disable logic to preserve and restore state, and ensures state is saved on exit and after relevant mode changes. Includes tests for monitor state persistence and updates documentation for clarity.

Gitisshit

Henriklmao · 2025-12-15T16:50:01Z

Just to update a little. I added Dan Kingsley's Changes to my fork, to avoid any merge conflicts.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 15 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-15T16:55:50Z

src/monitor.rs

+        let (width, height) = if rotation == Rotation::Deg90 || rotation == Rotation::Deg270 {
+            (mode.unwrap().height, mode.unwrap().width)
+        } else {
+            (mode.unwrap().width, mode.unwrap().height)


Similar to the issue above, calling unwrap() twice on the same mode value (lines 208 and 210) is redundant. The unwrapped mode should be stored in a variable to avoid repeated unwrapping and improve code clarity.

Suggested change

let (width, height) = if rotation == Rotation::Deg90 || rotation == Rotation::Deg270 {

(mode.unwrap().height, mode.unwrap().width)

} else {

(mode.unwrap().width, mode.unwrap().height)

let mode = mode.unwrap();

let (width, height) = if rotation == Rotation::Deg90 || rotation == Rotation::Deg270 {

(mode.height, mode.width)

} else {

(mode.width, mode.height)

Copilot · 2025-12-15T16:55:51Z

src/map.rs

+    fn snap_vertical(app:&mut App, direction: i32) {
+        let selected_index = app.selected_monitor;
+        let mut targets = vec![0.0];
+
+        for (i, monitor) in app.monitors.iter().enumerate() {
+            if i == selected_index || !monitor.enabled { continue; }
+            let (_, y, _, h) = monitor.get_geometry();
+            targets.push(y);
+            targets.push(y + h);
+            targets.push(y + h / 2.0);
+        }
+
+        let (_, sy, _, sh) = app.monitors[selected_index].get_geometry();
+        let sources = vec![sy, sy + sh, sy + sh / 2.0];
+
+        let mut best_delta: Option<f64> = None;
+
+        for s in &sources {
+            for t in &targets {
+                let diff = t - s;
+                if (direction < 0 && diff < -0.1) || (direction > 0 && diff > 0.1) {
+                     match best_delta {
+                         None => best_delta = Some(diff),
+                         Some(current) => {
+                             if diff.abs() < current.abs() {
+                                 best_delta = Some(diff);
+                             }
+                         }
+                     }
+                }
+            }
+        }
+
+        if let Some(delta) = best_delta {
+            app.monitors[selected_index].move_vertical(delta.round() as i32);
+        }
+    }

    fn move_horizontal(app:&mut App, direction: i32) {
        app.monitors[app.selected_monitor].move_horizontal(direction);
    }
+    fn snap_horizontal(app:&mut App, direction: i32) {
+        let selected_index = app.selected_monitor;
+        let mut targets = vec![0.0];
+
+        for (i, monitor) in app.monitors.iter().enumerate() {
+            if i == selected_index || !monitor.enabled { continue; }
+            let (x, _, w, _) = monitor.get_geometry();
+            targets.push(x);
+            targets.push(x + w);
+            targets.push(x + w / 2.0);
+        }
+
+        let (sx, _, sw, _) = app.monitors[selected_index].get_geometry();
+        let sources = vec![sx, sx + sw, sx + sw / 2.0];
+
+        let mut best_delta: Option<f64> = None;
+
+        for s in &sources {
+            for t in &targets {
+                let diff = t - s;
+                if (direction < 0 && diff < -0.1) || (direction > 0 && diff > 0.1) {
+                     match best_delta {
+                         None => best_delta = Some(diff),
+                         Some(current) => {
+                             if diff.abs() < current.abs() {
+                                 best_delta = Some(diff);
+                             }
+                         }
+                     }
+                }
+            }
+        }
+
+        if let Some(delta) = best_delta {
+            app.monitors[selected_index].move_horizontal(delta.round() as i32);
+        }
+    }


The snap_vertical and snap_horizontal functions have significant code duplication (lines 111-147 and 152-188). Consider extracting the common snapping logic into a helper function that takes an axis parameter and coordinate extractors to reduce duplication and improve maintainability.

Copilot · 2025-12-15T16:55:51Z

src/map.rs

+        for s in &sources {
+            for t in &targets {
+                let diff = t - s;
+                if (direction < 0 && diff < -0.1) || (direction > 0 && diff > 0.1) {


The magic number -0.1 and 0.1 are used as epsilon values for floating point comparison. Consider defining these as named constants (e.g., SNAP_EPSILON) at the module level to make the code more maintainable and the purpose clearer.

Suggested change

if (direction < 0 && diff < -0.1) || (direction > 0 && diff > 0.1) {

if (direction < 0 && diff < -SNAP_EPSILON) || (direction > 0 && diff > SNAP_EPSILON) {

Copilot · 2025-12-15T16:55:52Z

src/rotation.rs

+use serde::Deserialize;
+
+#[derive(Debug, Clone, Deserialize, PartialEq)]
+pub enum Rotation {
+    Normal,
+    Deg90,
+    Deg180,
+    Deg270,
+}
+
+impl Default for Rotation {
+    fn default() -> Self {
+        Rotation::Normal
+    }
+}
+
+impl Rotation {
+    pub fn from_transform(transform: &Option<String>) -> Self {
+        match transform.as_deref() {
+            Some("90") => Rotation::Deg90,
+            Some("180") => Rotation::Deg180,
+            Some("270") => Rotation::Deg270,
+            _ => Rotation::Normal,
+        }
+    }
+
+    pub fn to_transform(&self) -> &str {
+        match self {
+            Rotation::Normal => "normal",
+            Rotation::Deg90 => "90",
+            Rotation::Deg180 => "180",
+            Rotation::Deg270 => "270",
+        }
+    }
+
+    pub fn to_hyprland(&self) -> i32 {
+        match self {
+            Rotation::Normal => 0,
+            Rotation::Deg90 => 1,
+            Rotation::Deg180 => 2,
+            Rotation::Deg270 => 3,
+        }
+    }
+
+    pub fn cycle(&self) -> Self {
+        match self {
+            Rotation::Normal => Rotation::Deg90,
+            Rotation::Deg90 => Rotation::Deg180,
+            Rotation::Deg180 => Rotation::Deg270,
+            Rotation::Deg270 => Rotation::Normal,
+        }
+    }
+}


The new Rotation module lacks any test coverage. Given the comprehensive testing in this project, tests should be added to verify the rotation cycle, transform conversions (from_transform, to_transform, to_hyprland), and the default behavior.

Copilot · 2025-12-15T16:55:52Z

src/monitor.rs

+            let rotation = Rotation::from_transform(&self.transform);
            format!(
-                "monitor = {}, {}x{}@{}, {}x{}, {}",
+                "monitor = {}, {}x{}@{}, {}x{}, {}, transform,{}",


The format string appears to have incorrect syntax with "transform," appearing as a literal string followed by a placeholder. According to Hyprland documentation, the monitor configuration format should be: "monitor = name, resolution@refresh, position, scale, transform, value". The current format has an extra comma and the word "transform" as a literal. This should likely be: "monitor = {}, {}x{}@{}, {}x{}, {}, transform, {}" with proper spacing.

Suggested change

"monitor = {}, {}x{}@{}, {}x{}, {}, transform,{}",

"monitor = {}, {}x{}@{}, {}x{}, {}, {}, {}",

Copilot · 2025-12-15T16:55:54Z

src/monitor.rs

+        let x = self.position.clone().unwrap().x as f64;
+        let y = self.position.clone().unwrap().y as f64;


Similar to line 164, the position is cloned and unwrapped separately for x and y. Consider cloning once or using a reference to avoid redundant operations.

Suggested change

let x = self.position.clone().unwrap().x as f64;

let y = self.position.clone().unwrap().y as f64;

let pos = self.position.clone().unwrap();

let x = pos.x as f64;

let y = pos.y as f64;

Copilot · 2025-12-15T16:55:54Z

src/map.rs

+    fn snap_vertical(app:&mut App, direction: i32) {
+        let selected_index = app.selected_monitor;
+        let mut targets = vec![0.0];
+
+        for (i, monitor) in app.monitors.iter().enumerate() {
+            if i == selected_index || !monitor.enabled { continue; }
+            let (_, y, _, h) = monitor.get_geometry();
+            targets.push(y);
+            targets.push(y + h);
+            targets.push(y + h / 2.0);
+        }
+
+        let (_, sy, _, sh) = app.monitors[selected_index].get_geometry();
+        let sources = vec![sy, sy + sh, sy + sh / 2.0];
+
+        let mut best_delta: Option<f64> = None;
+
+        for s in &sources {
+            for t in &targets {
+                let diff = t - s;
+                if (direction < 0 && diff < -0.1) || (direction > 0 && diff > 0.1) {
+                     match best_delta {
+                         None => best_delta = Some(diff),
+                         Some(current) => {
+                             if diff.abs() < current.abs() {
+                                 best_delta = Some(diff);
+                             }
+                         }
+                     }
+                }
+            }
+        }
+
+        if let Some(delta) = best_delta {
+            app.monitors[selected_index].move_vertical(delta.round() as i32);
+        }
+    }

    fn move_horizontal(app:&mut App, direction: i32) {
        app.monitors[app.selected_monitor].move_horizontal(direction);
    }
+    fn snap_horizontal(app:&mut App, direction: i32) {
+        let selected_index = app.selected_monitor;
+        let mut targets = vec![0.0];
+
+        for (i, monitor) in app.monitors.iter().enumerate() {
+            if i == selected_index || !monitor.enabled { continue; }
+            let (x, _, w, _) = monitor.get_geometry();
+            targets.push(x);
+            targets.push(x + w);
+            targets.push(x + w / 2.0);
+        }
+
+        let (sx, _, sw, _) = app.monitors[selected_index].get_geometry();
+        let sources = vec![sx, sx + sw, sx + sw / 2.0];
+
+        let mut best_delta: Option<f64> = None;
+
+        for s in &sources {
+            for t in &targets {
+                let diff = t - s;
+                if (direction < 0 && diff < -0.1) || (direction > 0 && diff > 0.1) {
+                     match best_delta {
+                         None => best_delta = Some(diff),
+                         Some(current) => {
+                             if diff.abs() < current.abs() {
+                                 best_delta = Some(diff);
+                             }
+                         }
+                     }
+                }
+            }
+        }
+
+        if let Some(delta) = best_delta {
+            app.monitors[selected_index].move_horizontal(delta.round() as i32);
+        }
+    }


The new snap_vertical and snap_horizontal functions implementing smart monitor positioning lack dedicated test coverage. While there's a basic snap test in main.rs (lines 401-408), comprehensive tests should verify edge cases such as snapping to multiple monitor edges, snapping when no valid target exists, and snapping in both directions.

Copilot · 2025-12-15T16:55:55Z

src/list.rs

+            });
+        }
+        // If no saved_position (from disable), keep the current position
+        // which might have been loaded from the persistent state file


When enabling a monitor, if saved_position exists it's restored, but if it doesn't exist, the comment says "keep the current position which might have been loaded from the persistent state file". However, there's no fallback to set a default position if neither saved_position nor a current position exists. This could leave a monitor in an undefined state if position is None. Consider adding a fallback to set a default position like (0, 0) if no position is available.

Suggested change

// which might have been loaded from the persistent state file

// which might have been loaded from the persistent state file.

// If position is still None, set a default position (0, 0)

if monitor.position.is_none() {

monitor.position = Some(Position { x: 0, y: 0 });

}

Copilot · 2025-12-15T16:55:55Z

src/monitor.rs

+                "monitor = {}, {}x{}@{}, {}x{}, {}, transform,{}",
                self.name,
                mode.width, mode.height, mode.refresh,
                self.position.clone().unwrap().x, self.position.clone().unwrap().y,


The position is cloned and unwrapped twice on the same line for x and y coordinates. This is inefficient. Consider cloning once and accessing both fields, or better yet, using a reference to avoid the unnecessary clones.

Copilot · 2025-12-15T16:55:55Z

src/list.rs

    fn disable_monitor(app:&mut App) {
-        app.monitors[app.selected_monitor].enabled = false;
+        let monitor = &mut app.monitors[app.selected_monitor];
+        monitor.enabled = false;
+        monitor.saved_position = monitor.position.clone();
+        monitor.saved_scale = monitor.scale;
    }

    fn enable_monitor(app:&mut App) {
-        app.monitors[app.selected_monitor].enabled = true;
-        app.monitors[app.selected_monitor].position = Some(
-            Position {
-                x: 0,
-                y: 0,
-            }
-        );
-        app.monitors[app.selected_monitor].scale = Some(1.0);
+        let monitor = &mut app.monitors[app.selected_monitor];
+        monitor.enabled = true;
+        if let Some(saved_pos) = &monitor.saved_position {
+            monitor.position = Some(Position {
+                x: saved_pos.x,
+                y: saved_pos.y,
+            });
+        }
+        // If no saved_position (from disable), keep the current position
+        // which might have been loaded from the persistent state file
+        monitor.scale = monitor.saved_scale.or_else(|| monitor.scale).or(Some(1.0));
    }


The updated disable_monitor and enable_monitor functions now save and restore position/scale, but lack test coverage for this new behavior. Given the comprehensive testing in the project, tests should verify that disabling a monitor saves its position and scale, and enabling it restores these values.

Added 'set display rotation' to the feature list and corrected the example monitors_config_path in the setup instructions according to https://wiki.hypr.land/Configuring/Start/

Dan-Kingsley and others added 7 commits December 2, 2025 12:17

feat: add display rotation functionality

80df23a

fix: use display resolutions as the example monitors shapes

1dadd0e

Feat: Add arrow key support for navigation

6c6a7c8

fix: remove unused KeyModifiers import.

4b5bbba

Copilot AI review requested due to automatic review settings December 15, 2025 16:12

Copilot AI reviewed Dec 15, 2025

View reviewed changes

Henriklmao added 4 commits December 15, 2025 17:29

Update map.rs

2f21736

Merge branch 'master' into gitisshit

0166aa2

Merge pull request #2 from Henriklmao/gitisshit

7f18b5f

Gitisshit

Copilot started reviewing on behalf of Henriklmao December 15, 2025 16:46 View session

Henriklmao requested a review from Copilot December 15, 2025 16:51

Copilot AI reviewed Dec 15, 2025

View reviewed changes

Update README with rotation feature and config path fix

68773bc

Added 'set display rotation' to the feature list and corrected the example monitors_config_path in the setup instructions according to https://wiki.hypr.land/Configuring/Start/

	if (direction < 0 && diff < -0.1) \|\| (direction > 0 && diff > 0.1) {
	if (direction < 0 && diff < -SNAP_EPSILON) \|\| (direction > 0 && diff > SNAP_EPSILON) {

	"monitor = {}, {}x{}@{}, {}x{}, {}, transform,{}",
	"monitor = {}, {}x{}@{}, {}x{}, {}, {}, {}",

		let x = self.position.clone().unwrap().x as f64;
		let y = self.position.clone().unwrap().y as f64;

-        let x = self.position.clone().unwrap().x as f64;
-        let y = self.position.clone().unwrap().y as f64;
+        let pos = self.position.clone().unwrap();
+        let x = pos.x as f64;
+        let y = pos.y as f64;

-        // which might have been loaded from the persistent state file
+        // which might have been loaded from the persistent state file.
+        // If position is still None, set a default position (0, 0)
+        if monitor.position.is_none() {
+            monitor.position = Some(Position { x: 0, y: 0 });
+        }

Saving monitor position when disabling or reenabling a monitor #2

Are you sure you want to change the base?

Saving monitor position when disabling or reenabling a monitor #2

Uh oh!

Conversation

Henriklmao commented Dec 15, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Henriklmao commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Henriklmao commented Dec 15, 2025 •

edited

Loading