GNOME Bugzilla – Bug 693438
Make Gnome Shell not depend on Clutter using the X11 backend
Last modified: 2013-11-21 18:19:31 UTC
I'm hoping to make a series of patches to Gnome Shell to stop it from requiring Clutter to use the X11 backend. The intention is that then Mutter can eventually be made to use the KMS backend directly so it can be its own display server. This has advantages in itself even when acting as an X compositor, but it is also a step towards making Mutter a hybrid Wayland compositor as well. Most of this work was already done in the wip/wayland branch but the patches were a bit hacky so I'm trying to split them up into smaller less hacky patches. I will attach them to this bug as I think they are ready.
Created attachment 235532 [details] [review] st-clipboard: Use GDK for the event filters instead of Clutter Instead of using Clutter to add an event filter for X events it now uses the GDK API. The Clutter API won't work if Clutter is not using an X11-based backend such as if Mutter is directly running with the KMS backend. This is a step towards making Mutter be its own display server and a step towards being a Wayland compositor. In this case GDK will still be using the X backend because it will connect to the headless X server.
Review of attachment 235532 [details] [review]: Looks good.
Created attachment 236101 [details] [review] Don't use ClutterX11TexturePixmap directly to embed docked windows Previously when a client requests that a window should be docked the shell would reparent the socket window onto the stage's window and then use ClutterX11TexturePixmap to get a texture to represent the window. This will not work if Clutter is no longer using the X11 winsys for example if it becomes its own display server. Instead this patch leaves the socket window as a child of the root window and lets mutter create a MetaWindow out of it. If Mutter is acting as a display server then this mechanism will still work via the headless x server. The ShellGtkEmbed instance now registers for notification of the ‘window-created’ signal of the display so that it can find the MetaWindow that gets created to represent the socket window. When this window is found it is prevented from being displayed on the screen by setting the actor's opacity to 0. An input shape is then set on the window to prevent it receiving any input. Instead of being a subclass of ClutterX11TexturePixmap, ShellGtkEmbed is now a subclass of ClutterClone. When the MetaWindow is found for the socket window the clone's source is set to the invisible actor for the window so it can be displayed in the panel as before. The ShellEmbeddedWindow no longer needs to know what the stage is because it no longer reparents the socket window. Therefore the ShellTrayManager doesn't need to know the stage either so shell_tray_manager_manage_stage has been replaced with just shell_tray_manager_manage_screen.
Created attachment 236107 [details] [review] st-im-text: Extract the hardware keycode via the evdev code StImText passes the hardware keycodes from Clutter events through a GDK function to convert them to a keysym. When using a non-X11 input backend, the hardware keycodes don't necessarily match the hardware keycodes that X would generate so the wrong keysyms were being generated. This patch makes it query the evdev keycode if possible because the X keycodes are known to be the same as the evdev keycodes+8.
Review of attachment 236101 [details] [review]: Causes this crash here:
+ Trace 231514
Thanks for testing this. I have rebased it on top of the recent patches in Mutter and Gnome Shell but I can't replicate the crash. Is there a specific application that is causing this? I did however run into a separate bug while testing it which wasn't there before I rebased: https://bugzilla.gnome.org/show_bug.cgi?id=693905
(In reply to comment #3) > Previously when a client requests that a window should be docked the > shell would reparent the socket window onto the stage's window and > then use ClutterX11TexturePixmap to get a texture to represent the > window. > The ShellGtkEmbed instance now registers for notification of the > ‘window-created’ signal of the display so that it can find the > MetaWindow that gets created to represent the socket window. When this > window is found it is prevented from being displayed on the screen by > setting the actor's opacity to 0. An input shape is then set on the > window to prevent it receiving any input. I assume you're talking about legacy notification-area icons here. If you do this, won't app specific context menus set on the icons stop working? The issue is the stage won't have an X window associated with it anymore right? Maybe we just need a different X window to host the icons?
I think it's pretty clear legacy notifications are an X11-only feature. Maybe disabling that feature with a configure switch would be a better idea.
well i think in the proposed environment there's a headless X server running to support legacy applications. I think the objective is to make the core components not use legacy X interfaces, not to remove the ability to run legacy applications. (i'm assuming anyway)
Integrating with legacy X protocols means using X interfaces. I don't see the harm in interacting with a very X-specific protocol by using X-specific code, and I don't think there's much benefit to making it abstract.
i think we're talking past each other. I asked in comment 7 if the goal was to make the stage not have an X window associated with it anymore, and proposed, if so, to create a different X window to assume the role of hosting notification area icons. You then said we should just add a configure switch to remove the ability to have notification area icons. I then said I think there's going to be a headless server so legacy X things (like notification area icons) will still work, so disabling the features seems wrong. You then said the notification area should continue to use X (which I agree with and said in comment 7). Basically, it sounds like, Neil is trying to remove the X dependency in clutter and keep the X dependency in everything else routed through a headless X server.
(In reply to comment #11) > Basically, it sounds like, Neil is trying to remove the X dependency in clutter > and keep the X dependency in everything else routed through a headless X > server. Yes, that is the plan. In the Mutter process Clutter will be running using the KMS winsys so it can't use anything that depends on using the X winsys, which includes assuming the stage has an X window. Mutter still connects as the compositor to a headless X server and can move X windows around using Xlib calls. Also GTK will still be using the X backend so it is safe to access X via GTK. The main problem that the patch is trying to address is that it is using ClutterX11TexturePixmap which relies on Clutter using the X backend. Creating another window to parent the docked icons to won't help with that. The plan in the patch is to leave the windows parented by the root window so that the windows will turn into textures by the normal compositing mechanism. In the case where Mutter is its own display server that will mean the X server will create a Wayland surface for it and Mutter will get buffers for it via the Wayland protocol instead of texture-from-pixmap. I agree the code as it was before using direct X calls is totally fine when Mutter is a normal X compositor, but it seems like if we have a mechanism that we can get to work in both cases then we might as well use the same code unconditionally.
> I assume you're talking about legacy notification-area icons here. If you do > this, won't app specific context menus set on the icons stop working? X will still think there is a window for the status icon and it will still get positioned in the right place using the same mechanism as before. The popup menus appear as their own toplevel windows which are positioned relative to the icon's window so they should still work fine. Indeed I just tested it with teststatusicon and it does work.
(In reply to comment #8) > I think it's pretty clear legacy notifications are an X11-only feature. Maybe > disabling that feature with a configure switch would be a better idea. No, some apps are pretty much useless without them and I don't see them changing anytime soon. (the way we hide them in the tray is broken anyway but that's a different topic). (In reply to comment #6) > Thanks for testing this. I have rebased it on top of the recent patches in > Mutter and Gnome Shell but I can't replicate the crash. Is there a specific > application that is causing this? I have tested with pidgin and skype.
Created attachment 238726 [details] [review] Don't use ClutterX11TexturePixmap directly to embed docked windows [v2] Here's a second version of the patch which seems to fix drag01's crash. After discussing on IRC we found that the crash is likely triggered by an extension which he is using to move the status icons to the menu bar at the top. I think there was a race condition where Mutter might recognise that the toplevel window for the plug has been destroyed before the tray manager notices that the icon is removed. If a paint happens in-between then the CluterClone will have kept the window actor alive even though it has been disposed and then Gnome Shell will try to paint and it will crash because priv->window will be NULL. This second version of the patch fixes it by making the ShellGtkEmbed explicitly listen for the destroy signal on the actor window in which case it will set the clone source to NULL. drag01 also pointed out that with this patch and his extension the status icons no longer receive mouse clicks. Normally when you click on a status icon in the tray at the bottom, notificationDaemon.js will notice the click and cause it to be forwarded on by synthesizing an X event. However this doesn't work with the extension presumably because the status icons are no longer a child of the tray at the bottom and they are not themselves reactive. If the extension is used without my patch then the icons would still see the click events because their X window is still available and is positioned in the correct place. This only works when the icons are at the top of the screen and not at the bottom because when the bottom tray is visible all of the windows are not actually rendered at their correct position because the entire screen is shifted up. I was assuming that the intention was that the X windows for the status icons shouldn't receive events and so in the patch I explicitly made it set a zero-sized input shape on the window. This stops them from getting events with the extension. However, even if I remove that they still don't get events because Gnome Shell seems to block events to windows that are underneath the top menu bar. I'm assuming this works without the patch because the windows are parented to the stage window and so are higher up in the hierarchy. It doesn't seem like a good idea to allow the status icons to receive X events because otherwise it works in two different ways depending on where the icon is positioned which seems like a bit of a mess. Instead I think it could be better to make the extension forward on the click events like the tray at the bottom does. For that reason I haven't tried to remove the input shape on the windows in the patch.
(In reply to comment #15) > drago01 also pointed out that with this patch and his extension the status icons > no longer receive mouse clicks. Normally when you click on a status icon in the > tray at the bottom, notificationDaemon.js will notice the click and cause it to > be forwarded on by synthesizing an X event. Ahh, this is the answer to the mystery i was asking about in comment 7.
(In reply to comment #15) > Created an attachment (id=238726) [details] [review] > Don't use ClutterX11TexturePixmap directly to embed docked windows [v2] > > Here's a second version of the patch which seems to fix drag01's crash. After > discussing on IRC we found that the crash is likely triggered by an extension > which he is using to move the status icons to the menu bar at the top. I think > there was a race condition where Mutter might recognise that the toplevel > window for the plug has been destroyed before the tray manager notices that the > icon is removed. If a paint happens in-between then the CluterClone will have > kept the window actor alive even though it has been disposed and then Gnome > Shell will try to paint and it will crash because priv->window will be NULL. > This second version of the patch fixes it by making the ShellGtkEmbed > explicitly listen for the destroy signal on the actor window in which case it > will set the clone source to NULL. > > drag01 also pointed out that with this patch and his extension the status icons > no longer receive mouse clicks. Normally when you click on a status icon in the > tray at the bottom, notificationDaemon.js will notice the click and cause it to > be forwarded on by synthesizing an X event. However this doesn't work with the > extension presumably because the status icons are no longer a child of the tray > at the bottom and they are not themselves reactive. > > If the extension is used without my patch then the icons would still see the > click events because their X window is still available and is positioned in the > correct place. This only works when the icons are at the top of the screen and > not at the bottom because when the bottom tray is visible all of the windows > are not actually rendered at their correct position because the entire screen > is shifted up. > > I was assuming that the intention was that the X windows for the status icons > shouldn't receive events and so in the patch I explicitly made it set a > zero-sized input shape on the window. This stops them from getting events with > the extension. However, even if I remove that they still don't get events > because Gnome Shell seems to block events to windows that are underneath the > top menu bar. I'm assuming this works without the patch because the windows are > parented to the stage window and so are higher up in the hierarchy. > > It doesn't seem like a good idea to allow the status icons to receive X events > because otherwise it works in two different ways depending on where the icon is > positioned which seems like a bit of a mess. Instead I think it could be better > to make the extension forward on the click events like the tray at the bottom > does. For that reason I haven't tried to remove the input shape on the windows > in the patch. Yeah that one works way better, no more crashes and I can get clicks back after editing the extension.
Review of attachment 238726 [details] [review]: OK, I have been testing this for a while, seems to be fine. Code looks good to me as well.