From 8743cdd292d24b8a1c9cf0fc40a0040b76dbf907 Mon Sep 17 00:00:00 2001 From: alex-ds13 <145657253+alex-ds13@users.noreply.github.com> Date: Thu, 28 Nov 2024 16:14:17 +0000 Subject: [PATCH] fix(wm): cross-monitor ops for floating windows This commit fixes an issue where when trying to move floating windows or windows on a floating workspace across boundaries to another monitor using the `move_container_in_direction` it wouldn't move the floating windows physically, although it moved them internally on komorebi, resulting in weird and wrong behavior. This commit creates a new method on `Monitor` to `add_container_with_direction` which takes a move direction and then uses the same logic that was previously on the `move_container_in_direction` function. It changes the `move_container_to_monitor` function to take an optional move direction which if it is some will have this function call the new method `add_container_with_direction` instead of just `add_container`. Lastly the `move_container_in_direction` function now when it realizes the move will be across monitors simply calls the `move_container_to_monitor` with the direction that was initially given to it. These changes require that all callers of `move_container_to_monitor` add an direction option, instead of passing `None` on all of them, a new helper function was created, named `direction_from_monitor_idx` which calculates the direction a move will have from the currently focused monitor and the target monitor return `None` if they are the same or returning `Some(direction)` if not. This way now all commands that call a move across monitor will use the logic to check from the direction if it should add the container on front or end. With these changes now all the code related to moving a window across monitors using a command should be on one place only making sure that in the future any change required only needs to be done on one place, instead of having to do it on `move_container_to_monitor` and `move_container_in_direction` as before! This commit is an interactive squashed rebase of the following commits from PR #1154: 8f4bc101bc746e50348ac109780636be0eb7355a fix(wm): move floats in direction across monitors This commit fixes an issue where when trying to move floating windows or windows on a floating workspace across boundaries to another monitor using the `move_container_in_direction` it wouldn't move the floating windows physically, although it moved them internally on komorebi, resulting in weird and wrong behavior. This commit creates a new method on `Monitor` to `add_container_with_direction` which takes a move direction and then uses the same logic that was previously on the `move_container_in_direction` function. It changes the `move_container_to_monitor` function to take an optional move direction which if it is some will have this function call the new method `add_container_with_direction` instead of just `add_container`. Lastly the `move_container_in_direction` function now when it realizes the move will be across monitors simply calls the `move_container_to_monitor` with the direction that was initially given to it. These changes require that all callers of `move_container_to_monitor` add an direction option, instead of passing `None` on all of them, a new helper function was created, named `direction_from_monitor_idx` which calculates the direction a move will have from the currently focused monitor and the target monitor return `None` if they are the same or returning `Some(direction)` if not. This way now all commands that call a move across monitor will use the logic to check from the direction if it should add the container on front or end. 3b20e4b2fe99f6c7db52dec6f13f8fd89039daa3 refactor(wm): use helper function on move to workspace Use the same `add_container_with_direction` function on `move_container_to_workspace` as it is being used on `move_container_to_monitor` or `move_container_in_direction`. This way we bring parity between all methods and make it easier to change the way a container is added on a monitor workspace when taking the move direction into consideration. --- komorebi/src/monitor.rs | 136 +++++++++++++++++++++----------- komorebi/src/process_command.rs | 38 +++++++-- komorebi/src/window_manager.rs | 125 ++++++++++------------------- 3 files changed, 161 insertions(+), 138 deletions(-) diff --git a/komorebi/src/monitor.rs b/komorebi/src/monitor.rs index 5e40467ec..d4a151a20 100644 --- a/komorebi/src/monitor.rs +++ b/komorebi/src/monitor.rs @@ -139,6 +139,86 @@ impl Monitor { Ok(()) } + /// Adds a container to this `Monitor` using the move direction to calculate if the container + /// should be added in front of all containers, in the back or in place of the focused + /// container, moving the rest along. The move direction should be from the origin monitor + /// towards the target monitor or from the origin workspace towards the target workspace. + pub fn add_container_with_direction( + &mut self, + container: Container, + workspace_idx: Option, + direction: OperationDirection, + ) -> Result<()> { + let workspace = if let Some(idx) = workspace_idx { + self.workspaces_mut() + .get_mut(idx) + .ok_or_else(|| anyhow!("there is no workspace at index {}", idx))? + } else { + self.focused_workspace_mut() + .ok_or_else(|| anyhow!("there is no workspace"))? + }; + + match direction { + OperationDirection::Left => { + // insert the container into the workspace on the monitor at the back (or rightmost position) + // if we are moving across a boundary to the left (back = right side of the target) + match workspace.layout() { + Layout::Default(layout) => match layout { + DefaultLayout::RightMainVerticalStack => { + workspace.add_container_to_front(container); + } + DefaultLayout::UltrawideVerticalStack => { + if workspace.containers().len() == 1 { + workspace.insert_container_at_idx(0, container); + } else { + workspace.add_container_to_back(container); + } + } + _ => { + workspace.add_container_to_back(container); + } + }, + Layout::Custom(_) => { + workspace.add_container_to_back(container); + } + } + } + OperationDirection::Right => { + // insert the container into the workspace on the monitor at the front (or leftmost position) + // if we are moving across a boundary to the right (front = left side of the target) + match workspace.layout() { + Layout::Default(layout) => { + let target_index = layout.leftmost_index(workspace.containers().len()); + + match layout { + DefaultLayout::RightMainVerticalStack + | DefaultLayout::UltrawideVerticalStack => { + if workspace.containers().len() == 1 { + workspace.add_container_to_back(container); + } else { + workspace.insert_container_at_idx(target_index, container); + } + } + _ => { + workspace.insert_container_at_idx(target_index, container); + } + } + } + Layout::Custom(_) => { + workspace.add_container_to_front(container); + } + } + } + OperationDirection::Up | OperationDirection::Down => { + // insert the container into the workspace on the monitor at the position + // where the currently focused container on that workspace is + workspace.insert_container_at_idx(workspace.focused_container_idx(), container); + } + }; + + Ok(()) + } + pub fn remove_workspace_by_idx(&mut self, idx: usize) -> Option { if idx < self.workspaces().len() { return self.workspaces_mut().remove(idx); @@ -215,54 +295,14 @@ impl Monitor { Some(workspace) => workspace, }; - match direction { - Some(OperationDirection::Left) => match target_workspace.layout() { - Layout::Default(layout) => match layout { - DefaultLayout::RightMainVerticalStack => { - target_workspace.add_container_to_front(container); - } - DefaultLayout::UltrawideVerticalStack => { - if target_workspace.containers().len() == 1 { - target_workspace.insert_container_at_idx(0, container); - } else { - target_workspace.add_container_to_back(container); - } - } - _ => { - target_workspace.add_container_to_back(container); - } - }, - Layout::Custom(_) => { - target_workspace.add_container_to_back(container); - } - }, - Some(OperationDirection::Right) => match target_workspace.layout() { - Layout::Default(layout) => { - let target_index = - layout.leftmost_index(target_workspace.containers().len()); - - match layout { - DefaultLayout::RightMainVerticalStack - | DefaultLayout::UltrawideVerticalStack => { - if target_workspace.containers().len() == 1 { - target_workspace.add_container_to_back(container); - } else { - target_workspace - .insert_container_at_idx(target_index, container); - } - } - _ => { - target_workspace.insert_container_at_idx(target_index, container); - } - } - } - Layout::Custom(_) => { - target_workspace.add_container_to_front(container); - } - }, - _ => { - target_workspace.add_container_to_back(container); - } + if let Some(direction) = direction { + self.add_container_with_direction( + container, + Some(target_workspace_idx), + direction, + )?; + } else { + target_workspace.add_container_to_back(container); } } diff --git a/komorebi/src/process_command.rs b/komorebi/src/process_command.rs index 7a65785cb..13a70feb8 100644 --- a/komorebi/src/process_command.rs +++ b/komorebi/src/process_command.rs @@ -536,7 +536,8 @@ impl WindowManager { self.move_container_to_workspace(workspace_idx, true, None)?; } SocketMessage::MoveContainerToMonitorNumber(monitor_idx) => { - self.move_container_to_monitor(monitor_idx, None, true)?; + let direction = self.direction_from_monitor_idx(monitor_idx); + self.move_container_to_monitor(monitor_idx, None, true, direction)?; } SocketMessage::SwapWorkspacesToMonitorNumber(monitor_idx) => { self.swap_focused_monitor(monitor_idx)?; @@ -548,7 +549,8 @@ impl WindowManager { .ok_or_else(|| anyhow!("there must be at least one monitor"))?, ); - self.move_container_to_monitor(monitor_idx, None, true)?; + let direction = self.direction_from_monitor_idx(monitor_idx); + self.move_container_to_monitor(monitor_idx, None, true, direction)?; } SocketMessage::SendContainerToWorkspaceNumber(workspace_idx) => { self.move_container_to_workspace(workspace_idx, false, None)?; @@ -570,7 +572,8 @@ impl WindowManager { self.move_container_to_workspace(workspace_idx, false, None)?; } SocketMessage::SendContainerToMonitorNumber(monitor_idx) => { - self.move_container_to_monitor(monitor_idx, None, false)?; + let direction = self.direction_from_monitor_idx(monitor_idx); + self.move_container_to_monitor(monitor_idx, None, false, direction)?; } SocketMessage::CycleSendContainerToMonitor(direction) => { let monitor_idx = direction.next_idx( @@ -579,22 +582,37 @@ impl WindowManager { .ok_or_else(|| anyhow!("there must be at least one monitor"))?, ); - self.move_container_to_monitor(monitor_idx, None, false)?; + let direction = self.direction_from_monitor_idx(monitor_idx); + self.move_container_to_monitor(monitor_idx, None, false, direction)?; } SocketMessage::SendContainerToMonitorWorkspaceNumber(monitor_idx, workspace_idx) => { - self.move_container_to_monitor(monitor_idx, Option::from(workspace_idx), false)?; + let direction = self.direction_from_monitor_idx(monitor_idx); + self.move_container_to_monitor( + monitor_idx, + Option::from(workspace_idx), + false, + direction, + )?; } SocketMessage::MoveContainerToMonitorWorkspaceNumber(monitor_idx, workspace_idx) => { - self.move_container_to_monitor(monitor_idx, Option::from(workspace_idx), true)?; + let direction = self.direction_from_monitor_idx(monitor_idx); + self.move_container_to_monitor( + monitor_idx, + Option::from(workspace_idx), + true, + direction, + )?; } SocketMessage::SendContainerToNamedWorkspace(ref workspace) => { if let Some((monitor_idx, workspace_idx)) = self.monitor_workspace_index_by_name(workspace) { + let direction = self.direction_from_monitor_idx(monitor_idx); self.move_container_to_monitor( monitor_idx, Option::from(workspace_idx), false, + direction, )?; } } @@ -602,7 +620,13 @@ impl WindowManager { if let Some((monitor_idx, workspace_idx)) = self.monitor_workspace_index_by_name(workspace) { - self.move_container_to_monitor(monitor_idx, Option::from(workspace_idx), true)?; + let direction = self.direction_from_monitor_idx(monitor_idx); + self.move_container_to_monitor( + monitor_idx, + Option::from(workspace_idx), + true, + direction, + )?; } } diff --git a/komorebi/src/window_manager.rs b/komorebi/src/window_manager.rs index d35afe805..941096f31 100644 --- a/komorebi/src/window_manager.rs +++ b/komorebi/src/window_manager.rs @@ -498,6 +498,35 @@ impl WindowManager { None } + /// Calculates the direction of a move across monitors given a specific monitor index + pub fn direction_from_monitor_idx( + &self, + target_monitor_idx: usize, + ) -> Option { + let current_monitor_idx = self.focused_monitor_idx(); + if current_monitor_idx == target_monitor_idx { + return None; + } + + let current_monitor_size = self.focused_monitor_size().ok()?; + let target_monitor_size = *self.monitors().get(target_monitor_idx)?.size(); + + if target_monitor_size.left + target_monitor_size.right == current_monitor_size.left { + return Some(OperationDirection::Left); + } + if current_monitor_size.right + current_monitor_size.left == target_monitor_size.left { + return Some(OperationDirection::Right); + } + if target_monitor_size.top + target_monitor_size.bottom == current_monitor_size.top { + return Some(OperationDirection::Up); + } + if current_monitor_size.top + current_monitor_size.bottom == target_monitor_size.top { + return Some(OperationDirection::Down); + } + + None + } + #[allow(clippy::too_many_arguments)] #[tracing::instrument(skip(self), level = "debug")] fn add_window_handle_to_move_based_on_workspace_rule( @@ -1383,6 +1412,7 @@ impl WindowManager { monitor_idx: usize, workspace_idx: Option, follow: bool, + move_direction: Option, ) -> Result<()> { self.handle_unmanaged_window_behaviour()?; @@ -1459,7 +1489,11 @@ impl WindowManager { .map(|w| w.hwnd) .collect::>(); - target_monitor.add_container(container, workspace_idx)?; + if let Some(direction) = move_direction { + target_monitor.add_container_with_direction(container, workspace_idx, direction)?; + } else { + target_monitor.add_container(container, workspace_idx)?; + } if let Some(workspace) = target_monitor.focused_workspace() { if !*workspace.tile() { @@ -1771,15 +1805,13 @@ impl WindowManager { .ok_or_else(|| anyhow!("there is no container or monitor in this direction"))?; { - // remove the container from the origin monitor workspace - let origin_container = self - .focused_workspace_mut()? - .remove_container_by_idx(origin_container_idx) - .ok_or_else(|| { - anyhow!("could not remove container at given origin index") - })?; - - self.focused_workspace_mut()?.focus_previous_container(); + // actually move the container to target monitor using the direction + self.move_container_to_monitor( + target_monitor_idx, + None, + true, + Some(direction), + )?; // focus the target monitor self.focus_monitor(target_monitor_idx)?; @@ -1799,79 +1831,6 @@ impl WindowManager { // get a mutable ref to the focused workspace on the target monitor let target_workspace = self.focused_workspace_mut()?; - match direction { - OperationDirection::Left => { - // insert the origin container into the focused workspace on the target monitor - // at the back (or rightmost position) if we are moving across a boundary to - // the left (back = right side of the target) - match target_workspace.layout() { - Layout::Default(layout) => match layout { - DefaultLayout::RightMainVerticalStack => { - target_workspace.add_container_to_front(origin_container); - } - DefaultLayout::UltrawideVerticalStack => { - if target_workspace.containers().len() == 1 { - target_workspace - .insert_container_at_idx(0, origin_container); - } else { - target_workspace - .add_container_to_back(origin_container); - } - } - _ => { - target_workspace.add_container_to_back(origin_container); - } - }, - Layout::Custom(_) => { - target_workspace.add_container_to_back(origin_container); - } - } - } - OperationDirection::Right => { - // insert the origin container into the focused workspace on the target monitor - // at the front (or leftmost position) if we are moving across a boundary to the - // right (front = left side of the target) - match target_workspace.layout() { - Layout::Default(layout) => { - let target_index = - layout.leftmost_index(target_workspace.containers().len()); - - match layout { - DefaultLayout::RightMainVerticalStack - | DefaultLayout::UltrawideVerticalStack => { - if target_workspace.containers().len() == 1 { - target_workspace - .add_container_to_back(origin_container); - } else { - target_workspace.insert_container_at_idx( - target_index, - origin_container, - ); - } - } - _ => { - target_workspace.insert_container_at_idx( - target_index, - origin_container, - ); - } - } - } - Layout::Custom(_) => { - target_workspace.add_container_to_front(origin_container); - } - } - } - OperationDirection::Up | OperationDirection::Down => { - // insert the origin container into the focused workspace on the target monitor - // at the position where the currently focused container on that workspace is - target_workspace.insert_container_at_idx( - target_workspace.focused_container_idx(), - origin_container, - ); - } - }; - // if there is only one container on the target workspace after the insertion // it means that there won't be one swapped back, so we have to decrement the // focused position