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 x11 Window::get_position wrong values #386

Merged
merged 1 commit into from
Jan 27, 2018

Conversation

jwilm
Copy link
Contributor

@jwilm jwilm commented Jan 22, 2018

Some window managers like i3wm will actually nest application windows
(like those opened by winit) within other windows to, for example, add
decorations. Initially when debugging this method on i3, the x and y
positions were always returned as "2".

The solution that other xlib abstractions use is to climb up the window
hierarchy until just below the root window, and that window must be used
to determine the appropriate position.

This patch doesn't take into account borders or the window's offset
within its parent, but it's much more usable than the original
implementation on certain WMs.

Some window managers like i3wm will actually nest application windows
(like those opened by winit) within other windows to, for example, add
decorations. Initially when debugging this method on i3, the x and y
positions were always returned as "2".

The solution that other xlib abstractions use is to climb up the window
hierarchy until just below the root window, and that window must be used
to determine the appropriate position.

This patch doesn't take into account borders or the window's offset
within its parent, but it's much more usable than the original
implementation on certain WMs.
@tomaka
Copy link
Contributor

tomaka commented Jan 22, 2018

Don't we require the parent window at initialization? I feel like this new function is a bit useless?

@jwilm
Copy link
Contributor Author

jwilm commented Jan 22, 2018

@tomaka get_position always returns (2, 2) on my system with the previous implementation. Do you have another idea how we might address that?

@tomaka
Copy link
Contributor

tomaka commented Jan 23, 2018

Sorry, I was thinking about the get_window_parent function which is maybe unnecessary? I'm on mobile so I can't really check the code at the moment.

@francesca64
Copy link
Member

@tomaka That function is vital for the new logic implemented here, as it's used to climb the window hierarchy in a loop.

@francesca64 francesca64 self-requested a review January 26, 2018 23:22
Copy link
Member

@francesca64 francesca64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks perfect.

In testing this, get_position used to always return (1, 28) for me, whereas now it returns accurate values, so this is a big improvement.

@tomaka tomaka merged commit 150d270 into rust-windowing:master Jan 27, 2018
alexheretic added a commit to alexheretic/winit that referenced this pull request Feb 7, 2018
sodiumjoe pushed a commit to sodiumjoe/winit that referenced this pull request Mar 19, 2018
Some window managers like i3wm will actually nest application windows
(like those opened by winit) within other windows to, for example, add
decorations. Initially when debugging this method on i3, the x and y
positions were always returned as "2".

The solution that other xlib abstractions use is to climb up the window
hierarchy until just below the root window, and that window must be used
to determine the appropriate position.

This patch doesn't take into account borders or the window's offset
within its parent, but it's much more usable than the original
implementation on certain WMs.
tmfink pushed a commit to tmfink/winit that referenced this pull request Jan 5, 2022
tmfink pushed a commit to tmfink/winit that referenced this pull request Jan 5, 2022
Implement gradient wrap/spread modes in SVG.

Closes rust-windowing#386.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants