mate-desktop / marco

MATE default window manager
https://mate-desktop.org
GNU General Public License v2.0
196 stars 87 forks source link

Test Xres extension before using it. #747

Closed joakim-tjernlund closed 1 year ago

raveit65 commented 1 year ago

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

joakim-tjernlund commented 1 year ago

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

:) It is related to https://github.com/mate-desktop/marco/issues/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). 
muktupavels commented 1 year ago

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

Marco does not use libwnck...

joakim-tjernlund commented 1 year ago

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

raveit65 commented 1 year ago

Problems with x2go seems to be fixed, but it doesn't fix the origin issue https://github.com/mate-desktop/marco/issues/301, which was fixed by https://github.com/mate-desktop/marco/pull/741

Edit: Tested with signal messenger from flathub.

joakim-tjernlund commented 1 year ago

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 commented 1 year ago

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

raveit65 commented 1 year ago

.............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 commented 1 year ago

So this needs more work before merge?

raveit65 commented 1 year ago

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

joakim-tjernlund commented 1 year ago

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 commented 1 year ago

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 severalint variables and a gboolean on the stack, so automatically managed and won't leak.

Will merge

joakim-tjernlund commented 1 year ago

Thank you, adding it to 1.26 too?

lukefromdc commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mbkma commented 1 year ago

I will do point releases soon.

joakim-tjernlund commented 1 year ago

I will do point releases soon.

I guess my soon is different from your soon :)