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

Test Xres extension before using it. #747

Merged
merged 3 commits into from
Jan 14, 2023

Conversation

joakim-tjernlund
Copy link
Contributor

No description provided.

@raveit65
Copy link
Member

My crystal ball is broken, for what is your PR good for ?
:)

@joakim-tjernlund
Copy link
Contributor Author

joakim-tjernlund commented Dec 21, 2022

My crystal ball is broken, for what is your PR good for ? :)

:) It is related to #746
Marco should not rely on others(libwnck) to have done this for you.

Quote from man page:

XResQueryExtension returns True if the XRes extension is available on the given display. A client must call XResQueryExtension before calling any other XRes function in order to negotiate a compatible protocol version; otherwise the client will get undefined behavior (XRes may or may not work). 

Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

This builds and runs fine with marco in both compositing and noncomposing mode. Note that runtime test was short as I normally run compiz. Didn't get any issues with windows overlapping panels or anything like that.

NOT tested with X2go however as I simply don't have that sort of setup

@lukefromdc lukefromdc requested a review from a team December 23, 2022 07:12
@muktupavels
Copy link
Contributor

Marco should not rely on others(libwnck) to have done this for you.

Marco does not use libwnck...

Before using any Xres extension one must call XResQueryExtension()
Also make sure Xres 1.2 is available as marco need XResQueryClientIds()
Needed for X2Go as it does not have XRES 1.2 extension.
@joakim-tjernlund
Copy link
Contributor Author

joakim-tjernlund commented Dec 29, 2022

Turns out that X2Go does not tolerate any call to XResQueryClientIds().
Therefore impl. the test similar to libwnck using a global flag

@raveit65
Copy link
Member

raveit65 commented Jan 12, 2023

Problems with x2go seems to be fixed, but it doesn't fix the origin issue #301, which was fixed by #741

Edit: Tested with signal messenger from flathub.

@joakim-tjernlund
Copy link
Contributor Author

Problems with x2go seems to be fixed, but it doesn't fix the origin issue #301, which was fixed by #741

Edit: Tested with signal messenger from flathub.

Would be great if applied to 1.26.x too(and a release).
My testing was on 1.26

@lukefromdc
Copy link
Member

What is the status of this? It is ready to go?

@raveit65
Copy link
Member

.............but it doesn't fix the origin issue #301, which was fixed by #741

Edit: Tested with signal messenger from flathub.

For me this is open.

@lukefromdc
Copy link
Member

So this needs more work before merge?

@raveit65
Copy link
Member

No idea, i only wanted to say that origin issue isn't fixed by this PR.

@joakim-tjernlund
Copy link
Contributor Author

I didn't claim it solved any flatpak issues, just X2GO.
There is nothing more to be done here, if you have flatpak issues that is a different problem I think.

@lukefromdc
Copy link
Member

lukefromdc commented Jan 14, 2023

Thinking everyone who's going to review this already has, we have two different testers (one the writer) finding this fixes the X2go issue, and this is not about any flatpak bugs. Been almost a month and no problems caused by this have cropped up. The new variables are several int variables and a gboolean on the stack, so automatically managed and won't leak.

Will merge

@lukefromdc lukefromdc merged commit dcd5d21 into mate-desktop:master Jan 14, 2023
@joakim-tjernlund
Copy link
Contributor Author

Thank you, adding it to 1.26 too?

@lukefromdc
Copy link
Member

I normally only work on the master branch, it's three cherrypicks to make this work but since 4 commits back also related to this will try it

@lukefromdc
Copy link
Member

Someone else will have to handle releasing a 1.26 point release containing this: just pushed it to the branch but I have never managed the releases

@lukefromdc
Copy link
Member

lukefromdc commented Jan 14, 2023

Merged to 1.26, distros won't see it until someone releases 1.26.2

@mbkma
Copy link
Member

mbkma commented Jan 14, 2023

I will do point releases soon.

@joakim-tjernlund
Copy link
Contributor Author

I will do point releases soon.

I guess my soon is different from your soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants