search

gyunaev / birdtray

new mail system tray notification icon for Thunderbird
GNU General Public License v3.0
792 stars 60 forks source link

Allow showing/hiding Thunderbird when launched using Wayland platform #584

Open cptpcrd opened 9 months ago

cptpcrd commented 9 months ago

If the Wayland platform is in use, simply connect to the XWayland server directly, and perform queries using that connection.

As long as Thunderbird is running under XWayland, this allows Birdtray to find the window and show/hide it no matter what platform Birdtray is using.

This fixes some of the problems described in e.g. #426, but it is not full Wayland support.

cptpcrd commented 6 months ago

@Abestanis @gyunaev would you mind taking a look at this?

Abestanis commented 6 months ago

@cptpcrd I'm only maintaining the translations and the Windows support. I don't have the rights to approve this, and unfortunately I also don't have a system with XWayland to test this on.

But I have some questions which might help this get moving forward:

gyunaev commented 6 months ago

Honestly the change doesn't make much sense to me. If you're running birdtray and Thunderbird on wayland+X it should work as-is. If you're not running either under X, it won't work with this patch either. So what we are fixing here?

cptpcrd commented 6 months ago

Honestly the change doesn't make much sense to me. If you're running birdtray and Thunderbird on wayland+X it should work as-is. If you're not running either under X, it won't work with this patch either. So what we are fixing here?

Sorry, I didn't do a great job of explaining this in the description.

Birdtray uses Qt, and Thunderbird uses GTK. Qt and GTK each have an X11 "platform" and a Wayland "platform" that can be selected (though GDK seems to use the terminology "backend"). Thus it is entirely possible to:

  1. Have Birdtray using the X11 platform (i.e. XWayland), and Thunderbird using the Wayland platform, or
  2. Have Birdtray using the Wayland platform, and Thunderbird using the X11 platform (i.e. XWayland)

I only mention case (1) for completeness; it's a bit degenerate and would really require Birdtray to have full Wayland support.

My system, without any custom configuration, ended up in case (2). Thunderbird was connecting to the XWayland server, and thus could have been found and hidden by Birdtray. However, since Birdtray was using Qt's Wayland platform, it didn't connect to the XWayland server and didn't even try to find Thunderbird. I worked around this by running birdtray -platform xcb to force Birdtray to use the X11 platform. (On various issues on this repo, people have described having to use QT_QPA_PLATFORM=xcb which AFAICT is equivalent to -platform xcb, so I suspect that this isn't just me.)

With this change, Birdtray works out-of-the-box in that situation: it continues to use the Wayland platform to render its own window, but separately connects to the XWayland server to control Thunderbird. It's not a huge deal, but I think there's some value in having Birdtray "just work" when it's straightforward to do so, which it is in this case.

To address @Abestanis's comments as well:

@cptpcrd I'm only maintaining the translations and the Windows support. I don't have the rights to approve this, and unfortunately I also don't have a system with XWayland to test this on.

Ah, I wasn't aware.

But I have some questions which might help this get moving forward:

  • Why is using XOpenDisplay instead of QX11Info::display and DefaultRootWindow instead of QX11Info::appRootWindow better? Shouldn't the Qt functions just be wrappers around the xlib primitives? What's the difference?

When using the Wayland platform:

It looks like the Qt developers assume you won't try to use most of the QX11Info functions except when actually using the X11 platform, which is why this PR contains an explicit check for that.

  • Looking at the description, there seems to be a subtle difference between QX11Info::appRootWindow (Returns a handle for the applications root window on the given screen.) and DefaultRootWindow (return the root window for the default screen.). Can this become problematic?

Perhaps. I checked the implementation for QX11Info::appRootWindow(), and I'm not entirely sure what happens when you don't pass an argument (in which case screen defaults to -1); it calls into several other APIs I'm not familiar with. However I've never seen multiple X11 screens actually be used (especially under XWayland), so I doubt it will be an issue in practice. (Note: multiple monitors are generally not managed with X11 screens; there's still a single root window which stretches across all the monitors.)

  • What is the reason to introduce caching of the display an root window? Birdtray can run for a long time, what if the X server is restarted or if the root window changes? I don't know if this is an actual issue with, but maybe it would be better to revert this, we could instead just extract the getting the screen and root window into a small helper that is used everywhere.

I don't think the root window would change unless the X server restarts, and restarting the X server is fairly destructive. I don't think I've ever seen an X11 client which tries to reconnect when the server restarts; usually they just exit.

Under Wayland it's a bit different since the XWayland server can restart while the Wayland compositor keeps running, but it's still quite rare and IMO not worth the extra complexity. (Technically, there's one exception I'm aware of -- wlroots-based compositors sometimes stop the XWayland server if there are no connected clients -- but here we're keeping a connection open, so that won't happen.)

gyunaev commented 6 months ago

Thank you for the explanation. Would it be simpler just have a check in main() for whether QX11Info::appRootWindow() returns null, and in this case self-restart with -platform xcb (i.e. exec argv[0] -platform xcb )?

cptpcrd commented 6 months ago

Thank you for the explanation. Would it be simpler just have a check in main() for whether QX11Info::appRootWindow() returns null, and in this case self-restart with -platform xcb (i.e. exec argv[0] -platform xcb )?

That would work, though there's a small chance it'll break if Qt changes their platform selection logic. (Also you'd need to do some extra work to preserve other arguments like --settings, so it might not turn out to be simpler in the end.) IMO the change in this PR is a little more robust.