Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wlroots taskbar #2046

Draft
wants to merge 97 commits into
base: work/gfgit/wayland_taskbar
Choose a base branch
from

Conversation

marcusbritanicus
Copy link

@marcusbritanicus marcusbritanicus commented Mar 27, 2024

This PR adds a generic wlroots backend support for the the taskbar and desktop-switcher plugins.

@gfgit: I took lxqt-panel:git, then merged #2041 and #2043. I hope this is what I was supposed to do. If something's amiss, please do let me know.

One of the issues we face is the wlroots backend does not support virtual desktops. However, various compositors (wayfire, sway, hyprland, etc) have their own mechanisms to provide this feature.

  1. swaymsg provides a very fine-grained control to handle windows and virtual-desktops on sway.
  2. Wayfire:git has a rudimentary IPC support (not as developed as swaymsg) with which we can develop something similar to swaymsg. Alternatively, I am developing three protocols that provide virtual-desktop and toplevel management support via protocols.
  3. Hyprthing is for Hyprland what swaymsg is for sway? I do not know much about this.

So it would be beneficial if we provide per-compositor support. As discussed in #2531, the best way forward is to develop a plugin interface for the wayland backend. Currently, I have the following structure in mind:

panel/backends/
  - wayland/
    - plasma/
      * lxqtbackendplasma.cpp
      * lxqtbackendplasma.h
    - wlroots/
      * lxqtbackendwlroots.cpp
      * lxqtbackendwlroots.h
    - wayfire/
      * lxqtbackendwayfire.cpp
      * lxqtbackendwayfire.h
    - sway/
      * lxqtbackendsway.cpp
      * lxqtbackendsway.h
    - hyprland/
      * lxqtbackendhyprland.cpp
      * lxqtbackendhyprland.h
    - lxqtwaylandbackend.cpp
    - lxqtwaylandbackend.h
  - xcb/
    - lxqttaskbarbackend_x11.cpp
    - lxqttaskbarbackend_x11.h
  - ilxqttaskbarabstractbackend.cpp
  - ilxqttaskbarabstractbackend.h
  - lxqttaskbardummybackend.cpp
  - lxqttaskbardummybackend.h

Few points:

  1. lxqtwaylandbackend.cpp should attempt to detect the DE and load the suitable plugin. The DE detection can be based on a user settings or XDG_CURRENT_DESKTOP.
  2. If we're using a protocol (say, like wlr-foreign-toplevel, or wayfire-desktop-shell), we should also attempt to verify that the compositor is advertising the said protocol.
  3. If the compositor-specific protocol/support is unavailable (for example, when using wayfire, if ipc plugin is not loaded or similar), we should fallback to wlroots.
  4. If wlroots (i.e. wlr-foreign-toplevel) is not available (for example, foregin-toplevel plugin is not loaded in wayfire), the default dummy wayland backend (as provided by lxqtwaylandbackend.cpp will be used).

Changes made:

  1. I have moved all the files from panel/backends/wayland/ to panel/backends/wayland/plasma
  2. panel/backends/wayland/wlroots implements the wlroots backend.
  3. Line 50/51 panel/lxqtpanelapplication.cpp to use only wlroots backend. (This needs to be fixed)

Current state:

  1. On a wlroots-based compositor, following works:
    • Listing all the currently open windows.
    • Click to focus
    • Add newly opened windows
    • Remove closed windows
    • desktop-switcher plugin (It will simply show only one virtual desktop named ("Desktop 1" with id "desktop-1")
  2. On a wlroots-based compositor, following does not work:
    • Grouping does not work properly
    • Window icons
  3. On a wlroots-based compositor, what will not work:
    • Move to output, move to desktop, shade, pin on top/bottom (No support from the protocol)
    • Anything related to virtual desktops (at least until ext-workspace-* support gets added)
  4. In it's current state, taskbar and desktop-switcher plugins will crash the panel when run on any non-wlroots compositor (for example, plasma).

Note: For the purpose of our discussion here, by "non-wlroots compositor", what I mean is a compositor that does not implement wlr-foreign-toplevel protocol.

cc: @stefonarch @tsujan

gfgit added 30 commits March 26, 2024 12:35
This is an abstract interface to operate windows
and workspaces
- Move WindowProperty enum to lxqttaskbartypes.h
This model will manage the tasks shown
Also use it to get window icon
- Don't rely on global screen coordinates

- This will be needed for future Wayland port,
  Where we don't have global screen coordinates

- Keep compatible behavior on X11
This new window propery flag is needed to notify geometry changes
This will be used to avoid crashing panel in case no backend could be
created.
A warning message will be printed in this case.
@marcusbritanicus
Copy link
Author

Under labwc never saw one.

That's correct. Wlroots does not support urgency hints, except for probably what is being transmitted from xwayland.

@tsujan
Copy link
Member

tsujan commented Apr 23, 2024

Found the cause of incorrect icons.

For example, although the icon of Falkon is just falkon, its app_id is (probably) org.kde.falkon because it's taken from its desktop entry. Therefore, icon themes need to include an icon named org.kde.falkon to cover Falkon.

I think this approach is used in Plasma-Wayland. However, Waybar does it differently and finds the original icon.

All in all, this isn't an important problem. It can be seen as an approach, not a problem.

EDIT: On second thought, even if it's what Plasma-Wayland does too, it is a problem because it affects too many apps, especially when apps have weird app_id'a that aren't reflected by their desktop entries (like qpdfview and screengrab).

@marcusbritanicus
Copy link
Author

marcusbritanicus commented Apr 23, 2024

@tsujan I doubt you if you need an icon named org.kde.falkon. What you need is a desktop named org.kde.falkon.desktop. The icon to be used is defined inside that desktop. And as you rightly pointed out, it's simply falkon. The correct icon should be picked by XdgIcon or QIcon based on your theme. The breeze icon is this, or very similar to that. This is what I get on my system as well, as you can see below.

2024-04-23_23:04:54

Edit:
As a fallback, we can always search of an icon named app_id but I feel that's a bad precedent.

Edit2:
qpdfview reports its app-id as qpdfview.local.qpdfview which is incorrect. I have opened a bug report on launchpad. Let's see what happens. A solution to this would be to create a file qpdfview.local.qpdfview.desktop in ~/.local/share/applications/ and copy the contents of qpdfview.desktop in it. Don't forget to set NoDisplay=true to hide it from menus.

@tsujan
Copy link
Member

tsujan commented Apr 23, 2024

I doubt you if you need an icon named org.kde.falkon

org.kde.falkon belongs to breeze-icons. There are several other icons with weird names like that.

The icon to be used is defined inside that desktop

It is with Waybar, but not with this PR.

@tsujan
Copy link
Member

tsujan commented Apr 23, 2024

What you need is a desktop named org.kde.falkon.desktop. The icon to be used is defined inside that desktop.

According to a previous comment of yours, XdgIcon::fromTheme() is used here. The argument of that method is an icon name; it has nothing to do with desktop entries.

@marcusbritanicus
Copy link
Author

What you need is a desktop named org.kde.falkon.desktop. The icon to be used is defined inside that desktop.

According to a previous comment of yours, XdgIcon::fromTheme() is used here. The argument of that method is an icon name; it has nothing to do with desktop entries.

I see.. I was under the assumption that XdgIcon does this work. In that case, we have two ways we can do this work. (1) Add a method to XdgIcon: XdgIcon::fromAppId(...) or (2) Add a function QIcon getIconFromAppId(...) inside wayland backend, that can be used for all wayland backends. This is a typical implementation you're looking at, whether you choose (1) or (2) the code is most likely to be the same, but the place it resides will be different.

@tsujan
Copy link
Member

tsujan commented Apr 23, 2024

(1) isn't compatible with Qt. So, I guess the icon name should be extracted from the desktop entry, as is done in other places of lxqt-panel (like Quick Launch).

@marcusbritanicus
Copy link
Author

(1) isn't compatible with Qt. So, I guess the icon name should be extracted from the desktop entry, as is done in other places of lxqt-panel (like Quick Launch).

Yes, and no. With Quick Launch, things are easy: you know the desktop file name before hand. So all that you need to do is extract the icon name, and then the icon. In our case, we have a more complicated situation. As you've noticed, we have a lot of apps that do not report a "proper" app-id. So we need falbacks, and fallbacks for our fallbacks. That is the reason why that code is that long. With that, I have managed to get icons for most of the apps - QPdfView and VritualBox are exceptions. Their app-ids are so bad our fallbacks are just not good-enough.

@stefonarch
Copy link
Member

To complicate things when reporting missing icons to sfwbar we found that some app-id's on debian are different than on arch, for example thunderbird...

@marcusbritanicus
Copy link
Author

@tsujan I have updated the code to get the icon. It's essentially the code I linked above. You should see some improvements in the icons.

@stefonarch
Copy link
Member

stefonarch commented Apr 23, 2024

The latest commit fixed most generic icons of the list above, remaining only

  • colorpick
  • wshot (well, doesn't count as qarma window)
  • meteo-qt

This code should go also to the plasma plugin no?

@stefonarch
Copy link
Member

Found that the setting

Put buttons of the same class next to each other

does nothing now on wayland.

@tsujan
Copy link
Member

tsujan commented Apr 23, 2024

You should see some improvements in the icons.

I confirm @stefonarch's observation. Yes, it's definitely an improvement. Some icons that weren't shown are correct now. But, for example, those of qpdfview, speedcrunch and screengrab aren't yet (they are LXQt's fallback icon application-x-executable).

@tsujan
Copy link
Member

tsujan commented Apr 23, 2024

@marcusbritanicus, a word about your last commit: You don't need to search in directories for icons. All you need is find their correct names; the rest should be done by XdgIcon::fromTheme(), for the platform integration to be respected.

EDIT: I mean getPixmapIcon().

@stefonarch
Copy link
Member

But, for example, those of qpdfview, speedcrunch and screengrab aren't yet (they are LXQt's fallback icon application-x-executable).

Qpdfview has no issue here, but I'm not sure if I didn't make some hack for it.

@marcusbritanicus
Copy link
Author

@marcusbritanicus, a word about your last commit: You don't need to search in directories for icons. All you need is find their correct names; the rest should be done by XdgIcon::fromTheme(), for the platform integration to be respected.

EDIT: I mean getPixmapIcon().

I do not remember why exactly I had to add that code: if I remember correctly, it was some legacy x11 app which had its icon installed to /usr/share/pixmaps/. QIcon::fromTheme(...) (and hence, I believe XdgIcon::fromTheme(...) too) does not look up that particular folder to retrieve icons. Do note that getPixmapIcon(...) is used as the last of the fallbacks.

If you can confirm that XdgIcon::fromTheme(...) searches /usr/share/pixmaps then I can remove that code.

@marcusbritanicus
Copy link
Author

marcusbritanicus commented Apr 24, 2024

But, for example, those of qpdfview, speedcrunch and screengrab aren't yet (they are LXQt's fallback icon application-x-executable).

@tsujan @stefonarch Following are the app-ids (and desktop names) reported:

  • qpdfview: qpdfview.local.qpdfview (qpdfview.desktop)
  • speedcrunch: org.speedcrunch.speedcrunch (speedcrunch.desktop)
  • screengrab: org.https://lxqt.screengrab (screengrab.desktop)

You can see that in each case, the issue is with the app. The app-ids do not match the desktop file names as they are required to. screengrab's app id is so convoluted, I am wondering if it's a typo. If screengrab is an LXQt app (as it appears to be), I suggest we call app.setDesktopFileName( "screengrab" ) in the code to set the correct app-id.

Edit:
Alright... I think I know where the problem is. In screengrab, application name, org name and org domain are set. Since desktopFileName is not set, Qt in a misguided attempt inverts the org-domain (giving us org.https://lxqt) and then appends the application name, resulting in this beautiful expression: org.https://lxqt.screengrab

@stefonarch
Copy link
Member

As for screengrab it's a non-issue as it works actually only under x11.

We should or update it to include the relevant wayland protocols or show a message "Not supported actually under wayland".

As for qpdfview I see I've added some custom icons

$ locate qpdfview|grep icon
/home/stef/.local/share/icons/Plataro/apps/scalable/qpdfview.svg
/usr/share/icons/Papirus/16x16/apps/qpdfview.svg
/usr/share/icons/Papirus/22x22/apps/qpdfview.svg
/usr/share/icons/Papirus/24x24/apps/qpdfview.svg
/usr/share/icons/Papirus/32x32/apps/qpdfview.local.qpdfview
/usr/share/icons/Papirus/32x32/apps/qpdfview.local.qpdfview.svg
/usr/share/icons/Papirus/32x32/apps/qpdfview.svg
/usr/share/icons/Papirus/48x48/apps/qpdfview.local.qpdfview.svg
/usr/share/icons/Papirus/48x48/apps/qpdfview.svg
/usr/share/icons/Papirus/64x64/apps/qpdfview.svg
/usr/share/icons/hicolor/scalable/apps/qpdfview.local.qpdfview.svg

@marcusbritanicus
Copy link
Author

We should or update it to include the relevant wayland protocols or show a message "Not supported actually under wayland".

@stefonarch The wayland protocol is sadly insufficient at the moment. We can capture the whole screen or an area. But capturing windows is not yet possible. The speed at which work on wayland-protocols is progressing, I would not be surprised if it takes a decade for a wayland screengrab protocol to become available. 😆

But yes, I am willing to make a preliminary attempt at including wlr-screengrab protocol in the code. It is not very difficult to do that. It should work on all compositors that implement this protocol.

@stefonarch
Copy link
Member

But yes, I am willing to make a preliminary attempt at including wlr-screengrab protocol in the code. It is not very difficult to do that. It should work on all compositors that implement this protocol.

Nice! Opened lxqt/screengrab#366 before.

But capturing windows is not yet possible

My wshot (in bash) supports window capture on sway, wayfire and hyprland by using their cli tools.

@tsujan
Copy link
Member

tsujan commented Apr 24, 2024

If you can confirm that XdgIcon::fromTheme(...) searches /usr/share/pixmaps...

@marcusbritanicus, yes, it does. It follows Freedesktop standards.

You can see that in each case, the issue is with the app. The app-ids do not match the desktop file names

I know what you mean. But we can't tell users that they should create this or that desktop entry for this or that app to have a correct icon on the task-bar, because

  • Users see that the icon is correct in other places of LXQt Panel itself;
  • They know that the icon is correct under X11; and
  • Some of them know that the icon is correct with other Wayland task-bars like that of Waybar.

As for screengrab it's a non-issue as it works actually only under x11.

That's not the point. We need to find out how Waybar does it and improve our task-bar.

@tsujan
Copy link
Member

tsujan commented Apr 24, 2024

Anyway, IMHO, fixing of the problems mentioned at #2046 (comment) may have priority over this, namely,

  • The selected state of a task-bar button is lost when the window title changes (this may be the cause of no selection with some newly opened windows); and
  • The task-bar of a window remains selected on minimizing it when it's the only window on the current workspace.

@gfgit
Copy link
Contributor

gfgit commented Apr 25, 2024

@stefonarch The wayland protocol is sadly insufficient at the moment. We can capture the whole screen or an area. But capturing windows is not yet possible.

Can we use XDG Portal which should have a "Capture" protocol? Can't remember the exact name, maybe ScreenGrag or something.

@tsujan
Copy link
Member

tsujan commented Apr 25, 2024

Please don't add comments that are about another app.

@marcusbritanicus
Copy link
Author

marcusbritanicus commented Apr 25, 2024

If you can confirm that XdgIcon::fromTheme(...) searches /usr/share/pixmaps...

@marcusbritanicus, yes, it does. It follows Freedesktop standards.

That is great. In that case I can happily replace QIcon::fromTheme with XdgIcon::fromTheme, and drop this fallback (getPixmapIcon(...)).

You can see that in each case, the issue is with the app. The app-ids do not match the desktop file names

I know what you mean. But we can't tell users that they should create this or that desktop entry for this or that app to have a correct icon on the task-bar, because

  • Users see that the icon is correct in other places of LXQt Panel itself;
  • They know that the icon is correct under X11; and
  • Some of them know that the icon is correct with other Wayland task-bars like that of Waybar.

I know we cannot tell the users that it's the app bug and forget about it. But attempting to "fix" an app's problem by making special provisions in the code is rather too much. Instead, we can make a simple package (open a repo) which ships desktops or icons that fixes issues with these apps. @stefonarch has added a few icons with suitable names for qpdfview, including qpdfview.local.qpdfview.svg. Collect all these fixes in a single repo named lxqt-wayland-fixes. We can keep updating this as and when we encounter an app with issues. This will greatly simplify our code. We will not have to make a quarter dozen fallbacks because apps are misbehaving.

As for screengrab it's a non-issue as it works actually only under x11.

That's not the point. We need to find out how Waybar does it and improve our task-bar.

I would like to disagree. Our taskbar is near-perfect as it is. I would again like to re-iterate what I said previously: (1) Open a bug report for the particular app. (2) Until the app fixes it, ship an icon or desktop file that fixes the issue (lxqt-wayland-fixes). Let's be open about this - if an app misbehaves, we tell people that it's the problem with the app, and not try to cover it up using code in our task-bar.

@stefonarch
Copy link
Member

I just remember to have those generic-icon chasing lived already non only whit sfwbar but also together with @selairi for yatbfw time ago.

It's probably a lot easier ship fallback icons than make the code catchup every bug in apps for we are not responsible.

If waybar has a good system to catch everything up we could look at it, but as said every other panel suffer those issues too.

@stefonarch
Copy link
Member

At the end it's more or less the same: shipping an icon for an app or adding a lookup loop for it. It looks like for both meteo-qt and qpdfview the fallback needed is reading their .desktop file, the former uses weather-few-clouds and doesn't ship a own icon, only Plataro and Papirus themes do (afaik).

sudo cp /usr/share/icons/Papirus/48x48/apps/meteo-qt.svg /usr/share/icons/hicolor/scalable/apps/meteo-qt.python3.svg worked also here atm.

@stefonarch
Copy link
Member

Just noticed that the "active" window state is also reset when changing tab in browser, due to changing button text as seen in filemanager and fpad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Experimental/in progress
Development

Successfully merging this pull request may close these issues.

None yet

5 participants