mate-desktop / marco

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

Only enable compositing in UI code if it's actually available. #759

Closed Ionic closed 1 year ago

Ionic commented 1 year ago

Thus far, the UI code enabled internal compositing code if compositing has been requested (i.e., the compositing-manager preference is turned on).

Unfortunately, requesting compositing doesn't mean that it needs to be available. Meta core code handles this gracefully by checking if X server extensions related to compositing are available and only enabling the internal compositor if everything looks alright, but this is not exposed to UI code directly.

If UI code enables compositing parts merely based upon the preference and doesn't check if compositing is actually available, we end up with black borders around windows and other weird effects.

This can be reproduced via X2Go/NX or testing with nested X servers such as Xephyr and disabling the XComposite extension.

This PR introduces checks for the availability of compositing and fixes this bug.

However, I'm not sure if the way I implemented this is "correct". I've noticed that UI code generally does not have access to core code and mostly only uses GTK/GDK and Xlib interfaces. To implement this, I had to pass down MetaScreen or MetaDisplay objects into the UI code, which might be frowned upon.

An alternative would have been to create a global property (hidden preference?) indicating whether compositing is available or not, manage this in core code and check it in UI code, but this would have been more complicated/intrusive.

lukefromdc commented 1 year ago

I was unable to start Xephyr, and have never used any kind of remote desktop setup so I cannot test this for function, but it built without issue and I didn't notice any new build warnings. This is the error I got trying to launch Xephyr:

Fatal server error:
(EE) Server is already active for display 0
    If this server is no longer running, remove /tmp/.X0-lock
    and start again.
(EE) 

We need someone familar with things like X2go or with a working Xephyr install (and knowing how to turn xcomposite off without restarting their session) to test this for function.

Ionic commented 1 year ago

Testing it is easy, don't worry.

Xephyr-nocomposite.sh:
#!/bin/sh

Xephyr :2 -extension GLX -extension COMPOSITE -resizeable # -extension DAMAGE -extension XINERAMA
mate-Xephyr.sh:

#!/bin/sh

env -u SESSION_MANAGER -u DBUS_SESSION_BUS_ADDRESS DISPLAY=:2 dbus-launch mate-session

First launch Xephyr-nocomposite.sh, then mate-Xephyr.sh. Make sure that compositing is enabled in the window manager's settings to see the original issue.

lukefromdc commented 1 year ago

I got a black screen in the Xephyr window with no cursor

Ionic commented 1 year ago

Yes, you should be getting a black screen in Xephyr after starting it. Afterwards, starting MATE should populate it with actual content (and also make the cursor show up).

lukefromdc commented 1 year ago

I cannot interact with the Xephyr window, and trying to open the session in another terminal gets

** (mate-session:127755): WARNING **: 16:59:45.545: Cannot open display: 
luke@ubuntu:~$ 

Remember, I have zero experience with this, I have never done any remote GUI work beyond interacting with a file system using sshfs a decade ago

Ionic commented 1 year ago

Yep, you can't interact with anything in Xephyr, since it's a blank X server at that point. :)

Spawning mate-Xephyr.sh in a second terminal instance should make MATE appear within it. The warning message doesn't make sense, though. It looks like it's trying to open up an empty DISPLAY value, but note how mate-Xephyr.sh explicitly sets DISPLAY=:2 (so that it spawns in Xephyr, which we have started using :2 before).

lukefromdc commented 1 year ago

In my case, that didn't work. I may have to leave functional testing for someone with X2go, the intended use case

raveit65 commented 1 year ago

I can confirmed that compositor is disabled in a x2go session and the black frame is gone. @vkareh Can you please do a code review?

vkareh commented 1 year ago

Code looks fine for me, and it works locally (I didn't test in x2go). LGTM

raveit65 commented 1 year ago

@lukefromdc Is this ready to go from your side? X2go was tested by me.

lukefromdc commented 1 year ago

I had no issues with the panel running locally and was unable to test x2go at all due to some kind of issue with it, so I don't know of any problems with this PR. We have a report of a succesful test with x2go and another reviewer says the code is good.

I just looked at the code myself and it looks good to me too: we add the necessary variables to get the screen and replace

if (meta_prefs_get_compositing_manager ()) with

      if (meta_prefs_get_compositing_manager () && screen &&
          !!(meta_display_get_compositor (meta_screen_get_display (screen))))

which is simple and should be quite reliable

As best as I can tell good to go

cwendling commented 1 year ago

Cherry-picked to 1.26 as a67e582bc96aac4a779b0fe28e7a005ca166bd03 and fdfc8bdd01242a38d87684b9c5e80e06edd2adad