Skip to content

Commit

Permalink
fix(wm): cross-monitor ops for floating windows
Browse files Browse the repository at this point in the history 
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:

8f4bc10
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.

3b20e4b
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.
  • Loading branch information
@alex-ds13 @LGUG2Z
alex-ds13 authored and LGUG2Z committed Nov 30, 2024
1 parent 01367f5 commit 8743cdd
Show file tree
Hide file tree
Showing 3 changed files with 161 additions and 138 deletions.
136 changes: 88 additions & 48 deletions komorebi/src/monitor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<usize>,
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<Workspace> {
if idx < self.workspaces().len() {
return self.workspaces_mut().remove(idx);
Expand Down Expand Up @@ -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);
}
}

Expand Down
38 changes: 31 additions & 7 deletions komorebi/src/process_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand All @@ -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)?;
Expand All @@ -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(
Expand All @@ -579,30 +582,51 @@ 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,
)?;
}
}
SocketMessage::MoveContainerToNamedWorkspace(ref workspace) => {
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,
)?;
}
}

Expand Down
125 changes: 42 additions & 83 deletions komorebi/src/window_manager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<OperationDirection> {
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(
Expand Down Expand Up @@ -1383,6 +1412,7 @@ impl WindowManager {
monitor_idx: usize,
workspace_idx: Option<usize>,
follow: bool,
move_direction: Option<OperationDirection>,
) -> Result<()> {
self.handle_unmanaged_window_behaviour()?;

Expand Down Expand Up @@ -1459,7 +1489,11 @@ impl WindowManager {
.map(|w| w.hwnd)
.collect::<Vec<_>>();

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() {
Expand Down Expand Up @@ -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)?;
Expand All @@ -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
Expand Down

0 comments on commit 8743cdd

Please sign in to comment.