search

lxqt / lxqt-panel

The LXQt desktop panel
https://lxqt-project.org
GNU Lesser General Public License v2.1
193 stars 135 forks source link

panel: Use the wlroots backend with unknown Wayland compositors #2161

Open Conan-Kudo opened 3 weeks ago

Conan-Kudo commented 3 weeks ago

The description of the LXQtPanelApplicationPrivate::loadBackend() method already states that if we cannot identify the correct backend for a Wayland session, we should fall back to the wlroots backend module. This is reasonable given how broadly the wlr protocols are implemented. However, this was not happening due to a small error where we checked for the wlroots string in XDG_CURRENT_DESKTOP, which is not a valid option.

This change fixes this by checking XDG_SESSION_TYPE for "wayland" instead. As this is fallback logic, this is safe as the preferred backend logic using XDG_CURRENT_DESKTOP still wins over this.

tsujan commented 3 weeks ago

As this is fallback logic

Only for compositors that are compatible with wlroots, as far as lxqt-panel is concerned. Otherwise:

https://github.com/lxqt/lxqt-panel/blob/5ce16acdd7f0ab11c6df212e0e708548a74dcc3b/panel/lxqtpanelapplication.cpp#L306-L313

Conan-Kudo commented 3 weeks ago

That's not what the method description says:

https://github.com/lxqt/lxqt-panel/blob/246b33f12cda7ab54054e9a6a1404fe05737431f/panel/lxqtpanelapplication.cpp#L193-L203

tsujan commented 3 weeks ago

It's a code comment :) What exists other than X11 and Wayland?

The code isn't final of course — more generic backends might be added in future — but for now, several good compositors are compatible with wlroots (except for kwin_wayland, which has its own backend; I like it very much).

Conan-Kudo commented 3 weeks ago

Yeah. I like KWin too. 😄

But yeah, I think it's reasonable to use the wlroots backend as a wayland fallback because outside of KWin; Mutter; and Weston, pretty much all compositors offer the wlr protocols that the backend depends on.

And we already have a KWin backend, if someone actually wants to use Mutter or Weston, then backends can be made for them. 😂

tsujan commented 3 weeks ago

it's reasonable to use the wlroots backend as a wayland fallback

Until the current disarray comes to an end in the Wayland world, we should thread carefully, IMO.

Two years ago we didn't imagine that we could support 7 well-known Wayland compositors so soon. Yes, the situation of Wayland has got better but not completely.

Also see how "wlroots" is handled in another part of the code:

https://github.com/lxqt/lxqt-panel/blob/5ce16acdd7f0ab11c6df212e0e708548a74dcc3b/panel/backends/wayland/wlroots/lxqtwmbackend_wlr.cpp#L609-L643

Conan-Kudo commented 3 weeks ago

Why does this method exist? We already select to load it from the panel frontend side?

tsujan commented 3 weeks ago

The backend is chosen in two ways: by checking the key preferred_backend in panel's config file (its value is a string list), or if it doesn't exist, by checking the scores of what is included in XDG_CURRENT_DESKTOP. This logic will work fine if more backends are added.

BTW, we need to add a dedicated backend for labwc because it supports cosmic-workspace-unstable-v1 now.

Conan-Kudo commented 3 weeks ago

Sure, but does that mean that the wlroots backend isn't going to let itself be used as the least-common-denominator panel backend?

tsujan commented 3 weeks ago

As the lowest common denominator for the compositors that are announced to be compatible with wlroots by lxqt-wayland-session — without raising false hopes.

Conan-Kudo commented 3 weeks ago

Then we should use a separate variable for this, because XDG_CURRENT_DESKTOP isn't the right place for it.

tsujan commented 3 weeks ago

This logic was a result of our developers' hard works and long discussions. It's proven to work fine and flexibly.

That being said, it isn't written in stone. If another method is shown to be better in practice, it could be replaced.

Anyway, LXQt 2.1.0 will be released with it the day after tomorrow.

stefonarch commented 2 weeks ago

Then we should use a separate variable for this, because XDG_CURRENT_DESKTOP isn't the right place for it.

We could just read the compositor in use as in startlxqtwayland.

tsujan commented 2 weeks ago

We could just read the compositor in use as in startlxqtwayland.

No, it's not that simple. We've used XDG_CURRENT_DESKTOP in other parts of the code because we agreed on it. For example:

https://github.com/lxqt/lxqt-panel/blob/5ce16acdd7f0ab11c6df212e0e708548a74dcc3b/panel/lxqtpanel.cpp#L262-L270

tsujan commented 2 weeks ago

Using XDG_CURRENT_DESKTOP is necessary because it isn't limited to compositors. For example, one may use LXQt panel inside Plasma Wayland. See

https://github.com/lxqt/lxqt-panel/blob/5ce16acdd7f0ab11c6df212e0e708548a74dcc3b/panel/backends/wayland/kwin_wayland/lxqtwmbackend_kwinwayland.cpp#L800-L803

Moreover, a Wayland compositor is a DE too.

EDIT: Please remind me to remove the case-sensitivity after 2.1.0 if I forget it, although it isn't a problem for us.