Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(wm): move floats in direction across monitors #1154

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alex-ds13
Copy link
Contributor

@alex-ds13 alex-ds13 commented Nov 28, 2024

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 adds a new WindowManagerEvent, LocationChange, which allows
komorebi to listen to and dispatch events to border_manager when
EVENT_OBJECT_LOCATIONCHANGE is emitted by the operating system.

This is required because there is no other event which fires on window
maximize/unmaximize.

fix LGUG2Z#1137
@alex-ds13 alex-ds13 force-pushed the fix/move-in-direction-across-monitors branch 2 times, most recently from 8798320 to 67c6a19 Compare November 28, 2024 16:58
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.
@alex-ds13 alex-ds13 force-pushed the fix/move-in-direction-across-monitors branch from 67c6a19 to ca27f4d Compare November 28, 2024 17:19
@alex-ds13
Copy link
Contributor Author

@LGUG2Z This is failing with the following error:

error: the following explicit lifetimes could be elided: 'a
  --> komorebi\src\com\interfaces.rs:47:6
   |
47 | impl<'a, T> Deref for ComIn<'a, T> {
   |      ^^                     ^^
   |

This PR doesn't touch that file at all. Also the previous PR I've submitted today didn't have this issue! Did anything happen in between with the Github actions? Could there have been an update on clippy or something like it that now reports this? If I run cargo clippy locally on my machine it doesn't show anything at all, it's all good!

@LGUG2Z
Copy link
Owner

LGUG2Z commented Nov 28, 2024

I think we're always tracking the latest Rust version and there was a new Rust release today 👀

I'll fix this on master and let you know when to rebase!

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.
@LGUG2Z
Copy link
Owner

LGUG2Z commented Nov 28, 2024

Should be fixed on master now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants