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

Implement smooth resizing on X11 with _NET_WM_SYNC_REQUEST #3771

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

Conversation

Speykious
Copy link

@Speykious Speykious commented Jul 1, 2024

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users

Resolves #2153.

I only tested this on KDE so far, this probably needs more tests on other DEs and WMs.

One thing I'm unsure about is whether all the widely used DEs and WMs support the sync API. I know KDE and Gnome definitely do, and it seems that awesomewm also does, but I don't know if some other important ones don't. Searching around, I saw that Qt preemptively checks if the sync extension is supported through a hasXSync call, but I couldn't find a way to do that with x11rb so right now there's just no checks...

On KDE, there's also the subtly weird behavior of the window when resizing a certain way that I mentioned in this C gist that I shared in #2153, but it looks more like a KDE bug to me than a misuse of the API. ok it actually definitely is a KDE bug because it happens to all windows of all applications regardless of whether they support _NET_WM_SYNC_REQUEST.

@Speykious Speykious force-pushed the sync branch 2 times, most recently from ede459d to 44e0b2a Compare July 1, 2024 22:56
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Qt preemptively checks if the sync extension is supported

We can do this here. Do extension_information with sync::X11_EXTENSION_NAME and check for None.

I'll test this later once I have more time.

@@ -469,21 +472,40 @@ impl UnownedWindow {
leap!(window.set_icon_inner(icon.inner)).ignore_error();
}

// Opt into handling window close
// Opt into handling window close and resize synchronization
Copy link
Member

Choose a reason for hiding this comment

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

Is there a better source of documentation on resize synchronization?

Copy link
Author

Choose a reason for hiding this comment

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

There's https://www.x.org/releases/X11R7.7/doc/xextproto/sync.html and https://fishsoup.net/misc/wm-spec-synchronization.html, but outside of that there's pretty much just direct implementations like Qt's.

src/platform_impl/linux/x11/window.rs Outdated Show resolved Hide resolved
Comment on lines 442 to 445
let lo = (bytemuck::cast::<c_long, c_ulong>(xev.data.get_long(2)) & 0xffffffff) as u32;
#[allow(clippy::identity_op)]
let hi = (bytemuck::cast::<c_long, c_ulong>(xev.data.get_long(3)) & 0xffffffff) as u32;
let hi = bytemuck::cast::<u32, i32>(hi);
Copy link
Member

Choose a reason for hiding this comment

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

Do we lose anything by doing as instead of bytemuck::cast?

Copy link
Author

@Speykious Speykious Jul 3, 2024

Choose a reason for hiding this comment

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

The data format is 32 bit numbers. On 64 bit Linux we could just skip the c_ulong conversion entirely, but on 32 bit Linux the c_long and c_ulong types are 32 bit, so the last bit being the sign becomes a problem. So this is done to make sure both 64 bit and 32 bit interpret the data correctly.

Alternatively I could make 2 separate codepaths, one for 32 bit and another for 64 bit. Might be easier to read?
Edit: I did just that, I think it's a bit better.

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.

Feature request: X11 Clean Resizing (aka support _NET_WM_SYNC_REQUEST)
2 participants