-
Notifications
You must be signed in to change notification settings - Fork 301
fix: Drop window on mouse position rather than window center #1156
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
base: main
Are you sure you want to change the base?
Conversation
state: &mut WmState, | ||
config: &UserConfig, | ||
) -> anyhow::Result<()> { | ||
moved_window.set_insertion_target(Some(InsertionTarget { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be simplified by setting insertion target to None
(will also use the workspace as target). we only set an explicit insertion target when transitioning from tiling -> floating, so that it'll go back into the same spot as previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent of the window needs to be changed manually, otherwise the window will be dropped into the workspace that containing the center point of the window, rather than the cursor position. Not sure how best to do that, as calling move_container_within_tree
errors since the window and the other monitors workspace have no common ancestor.
config, | ||
)?; | ||
|
||
Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove the Ok(())
by having update_window_state(...)
be the return
.captures(unparsed) | ||
.context(err_msg.to_string())?; | ||
let captures = | ||
units_regex.captures(unparsed).context(err_msg.clone())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a bit inconsistent but we typically use .to_string()
vs string cloning to be more explicit. would be good to add a linting rule for this at some point
.and_then(|monitor| monitor.displayed_workspace()) | ||
.unwrap_or_else(|| { | ||
warn!("No workspace at mouse position, using window workspace."); | ||
moved_window.workspace().expect("Window has no workspace") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have error logging for panics. would recommend:
.monitor_at_point(&mouse_pos)
.and_then(|monitor| monitor.displayed_workspace())
.or_else(|_| moved_window.workspace())
.context("Window has no workspace.")
6d85aa4
to
adddeb6
Compare
76fb418
to
6f3dadb
Compare
6f3dadb
to
ea29009
Compare
Fix to drop dragged windows onto the workspace the cursor is hovered over, rather than the workspace under the center of the window
Could also modify this to be an option in config, but using the cursor seems to be the more intuitive option.