-
-
Notifications
You must be signed in to change notification settings - Fork 832
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
Add get_position lua method for window #3006
base: main
Are you sure you want to change the base?
Conversation
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 for diving in!
Please split this into two PRs; one for the pane direction stuff which I think looks mostly fine, and the other for the window positioning stuff which will need some more discussion and fleshing out to keep the build working on all platforms!
2c0099a
to
b6f9de7
Compare
I have split the This one I guess requires implementation for window systems other than MacOS. Any suggestions how to test/proceed? |
For Windows, you can use GetWindowRect to get the position. There are a couple of places that call that already that you can probably copypasta. For X11, there is a GetGeometry request that yields a GetGeometryReply. You can find an example of that in the x11/window.rs file. Note that on some window managers it might be necessary to translate the result to get the correct coordinates. There's an example of that at the bottom of the x11/connection.rs file. I'd suggest skipping that complexity to start and we can see if we need to add it in later. For Wayland I'd suggest always returning If you don't have linux and windows machines available, I'm fine with you iterating by pushing to this PR to see if things compile; I think that complexity of these methods is low enough that that will probably be fine. |
/// | ||
/// This is only implemented on backends that allow | ||
/// windows to move themselves (not Wayland). | ||
fn get_window_position(&self) -> Future<ScreenPoint>; |
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.
I wonder, should this be get_window_geometry
instead, and return a ScreenRect
; since the implementation on all platforms will have the window rectangle, it feels like it might simplify some cases for callers in the future to return it all, especially because right now the window size is tracked in TermWindow based on WindowEvents.
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.
That way, the wayland implementation could return the dimensions but leave the position at 0,0 and this method would be half-usable. Not super convinced that that is the best idea though.
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.
I guess the current API choice favors get_position
as we have set_position
and get_dimensions
calls
b6f9de7
to
f59e41a
Compare
Looks like freebsd-12 failure in CI is unrelated to the commit |
b4d29a9
to
8057fbd
Compare
I have tested that round-tripping coordinates through
|
The code looks good! Following up on my earlier comment, can you try your round tripping test on X11 running i3? Then all that's left is adding a doc page for this new method! |
d48228c
to
a936615
Compare
@wez given I test it on i3 is this ready to merge (with all the recent wayland changes)? |
These methods allow to implement in Lua moving focus to adjacent windows (aka ActivateWindowDirection). Not sure if this is the right implementation - so I will open an issue to discuss and will use this PR as an illustration.
See #3007 for more context.