-
Notifications
You must be signed in to change notification settings - Fork 145
shell/grabs: Adjust reactive popups when window is moved #1369
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: master
Are you sure you want to change the base?
Conversation
toplevel, | ||
// TODO: give some thought to round vs floor | ||
(location + offset.as_global().to_f64()).to_i32_round(), | ||
self.window_outputs.iter(), |
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 outputs are for finding the anchor point and the anchor point must be within the surface, so it will be within the same outputs as the window, right?
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 outputs are for figuring out, which output contains the toplevel/popup to constrain that accordingly (and not let them go off-screen). window_outputs
should be fine here, as they contain every output the window currently overlaps with. 👍
Reactive XDG popups should be reconstrained if the window is moved: > When set reactive, the surface is reconstrained if the conditions used for constraining changed, e.g. the parent window moved.
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.
Thanks and sorry for looking at this so late.
Code-wise this seems all fine, can you update the code-comments I commented on to reflect the state of things better for merging? (Or remove them, where they were just questions.)
src/shell/grabs/moving.rs
Outdated
.get::<SeatMoveGrabState>() | ||
.map(|s| s.lock().unwrap()); | ||
if let Some(grab_state) = borrow.as_mut().and_then(|s| s.as_mut()) { | ||
// is this workspace location or output location? |
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.
Looking at the code, it should be Global
(I wonder why this piece of code isn't storing it that way), which means global coordinate space across outputs. update_reactive_popups
wants global, so this should be correct.
src/shell/grabs/moving.rs
Outdated
WindowSurface::Wayland(toplevel) => { | ||
update_reactive_popups( | ||
toplevel, | ||
// TODO: give some thought to round vs floor |
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.
Round is fine here. We use that in most of the code-base, so this would be consistent when fractional scaling is applied.
toplevel, | ||
// TODO: give some thought to round vs floor | ||
(location + offset.as_global().to_f64()).to_i32_round(), | ||
self.window_outputs.iter(), |
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 outputs are for figuring out, which output contains the toplevel/popup to constrain that accordingly (and not let them go off-screen). window_outputs
should be fine here, as they contain every output the window currently overlaps with. 👍
src/shell/grabs/moving.rs
Outdated
self.window_outputs.iter(), | ||
); | ||
} | ||
// Not sure what to do wth X11 surfaces. Perhaps their popups don't need adjustments. |
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.
They do, but since they have access to the global coordinate space unlike wayland clients, they do the re-adjustment themselves.
Thanks for taking a look, I pushed a version with improved comments (though my explanations are surface-deep). If you feel like more popup-related contributions, I can do them as soon as smithay gets Smithay/smithay#1745 |
XDG popups with positioner::reactive should be reconstrained if the window is moved:
This can be tested with the example from https://codeberg.org/dcz/stiwri :
This is what it looks like. Laggy because of my computer, but also I think I'm redrawing the surface one frame late.
The popup is allowed to slide to the right if the space to the left is not big enough. Its allowed to flip down if the space above is not big enough.
Screencast_20250423_145018.webm
Before this change, you needed to drag the edge and resize for the popup to move.
This will be very useful for the input method protocol update I'm working on.