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

Wayland taskbar support v2 #2043

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Wayland taskbar support v2 #2043

wants to merge 15 commits into from

Conversation

gfgit
Copy link
Member

@gfgit gfgit commented Mar 26, 2024

Depends on #2041 (WM abstraction)
Replaces #2031

This is an experimental implementation on taskbar, desktopswitch and showdesktop functionality for KWin Wayland compositor.
It uses following protocols:

  • org-kde-plasma-virtual-desktop
  • plasma-window-management

(Taken from plasma-wayland-protocols repository)

Code is inspired by libtaskmanager/waylandtasksmodel.cpp

Ideally the Wayland backend should have sub-backends targeting specific compositors when no standard protocol is available.

It also uses layer shell protocol to place panel on screen edge.

NOTE: panel auto-hide is not working yet

@gfgit gfgit mentioned this pull request Mar 26, 2024
@stefonarch
Copy link
Member

Nice! Taskbar under kwin_wayland works again I see.

NOTE: panel auto-hide is not working yet

It does here :) (had always worked on this branch).

@stefonarch
Copy link
Member

Something has gone wrong I think, I recompiled the changes this afternoon and taskbar is gone again.

plugin-tray/CMakeLists.txt Outdated Show resolved Hide resolved
@stefonarch
Copy link
Member

Taskbar is back here now I see, and crash on clicking showdesktop is avoided too.

@stefonarch
Copy link
Member

Just noticed a different behavior here compared to wlroots and X11 (any WM): the taskbar buttons only activates windows, but don't toggle minimize/restore.

@@ -0,0 +1,13 @@
[Desktop Entry]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get always 2 panels now after login as there is also the lxqt-panel.desktop from regular installation in autostart - in some way it ends up also in /etc/xdg/autostart/ with the _wayland name, didn't have time to investigate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's here, it installs any *.desktop file:

file(GLOB DESKTOP_FILES_IN *.desktop.in)

Copy link
Member

@stefonarch stefonarch Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO panel_wayland.conf.in could be panel_kwin.conf.in and be inside the plasma backend folder, as it's not generic for wayland but compositor-specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to install a single .desktop file (plus another or symlink inside autostart folder)
This is because KWin might get confused about which file the panel belongs and it needs to check the one with absolute path and KDE specific entries.
In the meantime we discuss this issue, I will solve the double install problem

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has the /usr/local/bin path again now:

screen_area_gio_16:10:14_

Because it's not named lxqt-panel.wayland.desktop.in it doesn't get configured I guess.

@stefonarch
Copy link
Member

From my tests it looks that export XDG_SESSION_DESKTOP="KDE in the startscript is not needed (anymore).

Another remaining issue is the "Maximize" item in menu which is always greyed out.

@marcusbritanicus
Copy link
Contributor

marcusbritanicus commented Apr 1, 2024

NOTE: panel auto-hide is not working yet

Is this not implemented yet, or, the implementation gives some problems?

Edit:

   // Emulate auto-hide on Wayland
   // NOTE: we cannot move window out of screen so we make it smaller

   // NOTE: a cleaner approach would be to use screen edge protocol
   // but it's specific to KWin

From the code it seems to me that you have problems about how. With layer-shell, we can set the margin to a negative value. For a bottom panel, setting the bottom margin to -PANEL_HEIGHT would mean that the panel is effectively hidden.

However, the issue of how to show the panel becomes a problem. With wayfire, we can use wayfire-shell or wayfire-desktop-shell, both of which provide an equivalent of screen-edge protocol.

But in general, we can partially hide the panel, instead of resizing the panel. For example, set the bottom margin to -PANEL_HEIGHT + PANEL_HIDE_SIZE. This would leave a few pixels sticking out. If the mouse pointer enters the panel, then it can be revealed completely. In place of QPropertyAnimation, you can apply QVariantAnimation and change the bottom margin value from -PANEL_HEIGHT + PANEL_HIDE_SIZE to 0 and other way around.

@gfgit
Copy link
Member Author

gfgit commented Apr 1, 2024

Are negative margin value allowed by layer shell?
Didn't know about it!
Current implementation sort of works but on my setup it triggers auto-show even if cursor is not yet at screen edge.
This makes it impossible to click for example statusbar of applications or other UI elements near screen edge because panel shows first and you end up click the panel instead.
It may be a bug in kwin mouse logic

@marcusbritanicus
Copy link
Contributor

Well... It's not specifically, disallowed. On wayfire, we simply use negative margins. I've not tested on other compositors, though.

@Consolatis
Copy link

Consolatis commented Apr 1, 2024

Negative layershell margins may end up on other outputs or don't receive frame / configure events anymore because the surface is not actually visible (which revealed a bug in the GTK layershell wrapper in the past), depending on the compositor. (Although I'd actually categorize the shows-up-on-other-output as a compositor bug). AFAIR a discussion in the ext-layershell protocol MR concluded to not allow negative margins at all (but memory is a bit fuzzy there).

Another option that should always work is hiding the panel and replacing it with a transparent hitbox without exclusive zone. On mouse movement on the hitbox that one is hidden and the panel is shown again.

This implements it in Python with GTK but something similar should be possible with Qt as well.
#!/usr/bin/env python3

from functools import partial

import gi
gi.require_version("GLib", "2.0")
gi.require_version("Gdk", "3.0")
gi.require_version("Gtk", "3.0")
gi.require_version('GtkLayerShell', '0.1')
from gi.repository import GLib, Gdk, Gtk, GtkLayerShell


class Context:
	def __init__(self):
		self.panel = Panel(self, auto_hide_seconds=5)
		self.dummy = HitBox(self)
		self.show_panel()

	def log(self, msg):
		print(f"[{self.__class__.__name__}] {msg}")

	def hide_panel(self):
		self.log("Hiding panel")
		self.dummy.show()
		self.panel.hide()

	def show_panel(self):
		self.log("Showing panel")
		self.dummy.hide()
		self.panel.show()


class Base(Gtk.Window):
	def __init__(self, context):
		super().__init__()
		self.context = context
		self.connect('motion-notify-event', self.on_mouse_move)
		self.set_events(Gdk.EventMask.POINTER_MOTION_MASK)
		self.set_size_request(-1, 30)
		GtkLayerShell.init_for_window(self)
		GtkLayerShell.set_anchor(self, GtkLayerShell.Edge.BOTTOM, True)
		GtkLayerShell.set_anchor(self, GtkLayerShell.Edge.LEFT, True)
		GtkLayerShell.set_anchor(self, GtkLayerShell.Edge.RIGHT, True)
		self.log = partial(context.__class__.log, self)
		self.log("init done")

	def on_mouse_move(self, src, event):
		pass

	def add_style(self, css):
		if isinstance(css, str):
			css = css.encode('utf-8')
		css_provider = Gtk.CssProvider()
		css_provider.load_from_data(css)
		style_context = Gtk.StyleContext()
		style_context.add_provider_for_screen(
			Gdk.Screen.get_default(),
			css_provider,
			Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION
		)


class Panel(Base):
	def __init__(self, *args, auto_hide_seconds=3, **kwargs):
		super().__init__(*args, **kwargs)
		GtkLayerShell.set_layer(self, GtkLayerShell.Layer.BOTTOM)
		GtkLayerShell.auto_exclusive_zone_enable(self)
		self.auto_hide_seconds = auto_hide_seconds
		self.timer = None

	def timer_rearm(self):
		if self.timer:
			GLib.source_remove(self.timer)
		self.timer = GLib.timeout_add_seconds(
			self.auto_hide_seconds, self.on_timer,
			priority=GLib.PRIORITY_LOW
		)

	def on_timer(self):
		self.log("Timer hit")
		self.context.hide_panel()
		self.timer = None
		return False

	def show(self):
		super().show()
		self.timer_rearm()

	def on_mouse_move(self, src, event):
		self.timer_rearm()


class HitBox(Base):
	def __init__(self, *args, **kwargs):
		super().__init__(*args, **kwargs)
		#self.set_size_request(-1, 2)
		GtkLayerShell.set_layer(self, GtkLayerShell.Layer.TOP)
		self.set_name('hitbox')
		#self.add_style('#hitbox { background-color: transparent; }')
		self.add_style('#hitbox { background-color: rgba(255, 128, 128, 0.5); }')

	def on_mouse_move(self, src, event):
		self.context.show_panel()


if __name__ == '__main__':
	context = Context()
	Gtk.main()

The variant also has the benefit that rather than just having a transparent non-exclusive hitbox, one could also anchor the replacement surface to e.g. the bottom-left edge and show a small button instead which - when triggered - hides the replacement surface and restores the panel one. That replacement surface would then be non-exclusive, only be the width of the button and using the top layer so its always visible on top of usual windows.

@stefonarch
Copy link
Member

Current implementation sort of works but on my setup it triggers auto-show even if cursor is not yet at screen edge.
This makes it impossible to click for example statusbar of applications or other UI elements near screen edge because panel shows first and you end up click the panel instead.
It may be a bug in kwin mouse logic

I didn't notice that as I don't really use it, but it's on all compositors the same, getting triggered at panel height and not screen border.

@Consolatis
Copy link

Consolatis commented Apr 1, 2024

Current implementation sort of works but on my setup it triggers auto-show even if cursor is not yet at screen edge.
This makes it impossible to click for example statusbar of applications or other UI elements near screen edge because panel shows first and you end up click the panel instead.
It may be a bug in kwin mouse logic

I didn't notice that as I don't really use it, but it's on all compositors the same, getting triggered at panel height and not screen border.

That could also be solved by using the transparent replacement surface, either via the button-only variant or by defining a 1px or 2px height of the surface (added a commented out call to the GTK example above).

@tsujan
Copy link
Member

tsujan commented Apr 1, 2024

Under X11, the panel is moved out of the screen appropriately, such that only 4 pixels (PANEL_HIDE_SIZE) of its height or width remain visible, depending on whether it's horizontal or vertical. Then its opacity is made zero. Those invisible 4 pixels serve to show the panel on mouse-over.

Since the panel is never positioned between screens, this method works fine. Of course, there are other details/options, like animation and auto-hiding on overlapping windows.

@stefonarch
Copy link
Member

Btw what about lxqt/lxqt#2531 (comment) ?

@gfgit
Copy link
Member Author

gfgit commented Apr 5, 2024

Btw what about lxqt/lxqt#2531 (comment) ?

I'm ok for extracting layer shell code and make a new PR

@gfgit gfgit mentioned this pull request Apr 8, 2024
@gfgit gfgit force-pushed the work/gfgit/wayland_taskbar branch 2 times, most recently from 5269af5 to 414e7d0 Compare April 11, 2024 14:48
@Sodivad
Copy link

Sodivad commented Apr 18, 2024

Code is inspired by libtaskmanager/waylandtasksmodel.cpp

Awesome that the code works for you but in this case the inspiration seems was rather strong and seems to include copied code from Plasma in multiple files as the commented out lines still references the original class and the general structure of the code (for example org_kde_plasma_window_icon_changed). We do not mind this as it's free software after all but would appreciate it if the original license headers could be preserved, thanks!

The original license headers are the two SPDX- lines at the top of the file, e.g. SPDX-License-Identifier: LGPL-2.1-only. Since lxqt is LGPL v2.1, you can specifically choose to re-use the code from KDE under the LGPL v2.1.

@tsujan
Copy link
Member

tsujan commented Apr 22, 2024

seems to include copied code from Plasma in multiple files

In that case, the original codes should be referenced by comments like "Adapted from...".

@gfgit
Copy link
Member Author

gfgit commented Apr 22, 2024

@Sodivad Hi, I've updated the license headers. Is it ok like that?

@gfgit gfgit force-pushed the work/gfgit/wayland_taskbar branch from 83634a0 to 1fc5594 Compare June 8, 2024 15:41
@gfgit
Copy link
Member Author

gfgit commented Jun 8, 2024

Rebased on current master

@stefonarch
Copy link
Member

stefonarch commented Jun 8, 2024

I notice 2 regressions now: desktop switch is the "n.d" one (not supported on this platform: wayland) and showdesktop doesn't work too anymore.

@marcusbritanicus
Copy link
Contributor

marcusbritanicus commented Jun 9, 2024

@gfgit @stefonarch @tsujan A request.

This PR introduces support for plasma wayland based taskbar and pager. However, before this can reviewed don't you think it's better to discuss how we will introduce support for wlroots-based taskbar too? Otherwise, we will unnecessarily have to refactor the code.

One of the ways we can go ahead is via modules - basically plugins for the taskbar plugin. The wayland backend code will load the suitable plugin based on the compositor and user's choice. We can use the ILXQtTaskbarAbstractBackend class as to develop the plugin interface. Then, the backends for all other platforms can be easily developed.

The biggest advantage in this approach is that we can easily change the backend code without affecting the rest of the project. Secondly, we can add support for different compositors based on their strengths - sway, wayfire, hyprland can use IPC, others can use what they provide. If nothing is available, the generic wlroots backend can be used.

If we introduce these modules (the framework for modules) before we review this PR, then we can refactor the plasma code into a module rather than merge this and ten go for a refactor later.

@stefonarch
Copy link
Member

I think that makes sense.
Labwc (generic wlroots backend), kwin and wayfire will be the priority atm as both qterminal dropdown and lxqt-runner are unusable on sway and hyprland is on a quite weird way probably afaik ("As we’re in the middle of slowly but surely moving off of depending on wlroots for our backend(s),").

@gfgit gfgit force-pushed the work/gfgit/wayland_taskbar branch from 2641999 to 059fe4c Compare August 20, 2024 11:46
gfgit added 2 commits August 20, 2024 13:49
- Split XDG_CURRENT_DESKTOP
- Skip LXQTPANEL_PLUGIN_PATH if empty
libwmbackend_<platform>.so
@gfgit gfgit force-pushed the work/gfgit/wayland_taskbar branch from 059fe4c to 93c038f Compare August 20, 2024 11:51
gfgit added 7 commits August 20, 2024 13:56
NOTE: works only on KWin

- Choose backend at runtime
- Windows filter logic is re-evaluated on window property changes

LXQtTaskBarPlasmaWindowManagment: implement showDesktop()

LXQtTaskbarWaylandBackend: do not show transient windows

LXQtTaskBarPlasmaWindowManagment: fix destructor TODO

TODO: is this correct?
Seems to call wl_proxy_destroy underneath

LXQtPanel: basic virtual desktop support on Plasma Wayland

Add desktop file to be recognized by KWin Wayland

NOTE: absolute path is needed inside .desktop file for this to work
      use CMake to get it.

- Prevent double dekstop file installed in autostart

LXQtTaskbarWaylandBackend: return only accepted windows

- reloadWindows() force removal and readding of windows

This fixes changing windows grouping settings and adding taskbar plugin
AFTER panel is started.
Both situations resulted in empty taskbar previously

LXQtTaskbarWaylandBackend: fix workspace logic

LXQtTaskbarWaylandBackend: fix workspace removal logic

LXQtTaskbarWaylandBackend: implement moving window to virtual desktop
workspace

LXQtPlasmaWaylandWorkspaceInfo: fix signedness comparison

CMake: move panel WM backends to separate libraries

LXQtTaskbarWaylandBackend: possibly fix crash on showDesktop for non-
KWin

Update license headers

LXQtTaskbarWaylandBackend: add dummy setDesktopLayout()

Implement LXQtWMBackendKWinWaylandLibrary

- Add Desktop Environment detection
TODO: show error message when not supported
- Add NoDisplay=true to .desktop file

CMake: rename autostart desktop variable
@gfgit gfgit force-pushed the work/gfgit/wayland_taskbar branch from 93c038f to 8f68516 Compare August 20, 2024 11:59
@gfgit
Copy link
Member Author

gfgit commented Aug 20, 2024

In the latter minimize/restore on click on the window button in the taskbar isn't working yet.

I think this is because when a window is deactivated we do not know previously active window so we return null WId.
It's tricky to debug since putting breakpoints activates IDE window and creates an infinite loop.

@gfgit
Copy link
Member Author

gfgit commented Aug 20, 2024

@stefonarch My intuition was correct. Avoiding null active window being reported by backend fixes minimize/restore on click on the window button in the taskbar.
To confirm can you test last commit?
I'm not sure it's the right approach but it doesn't seem to have side effects.

@stefonarch
Copy link
Member

@gfgit Nice, I'll test later today, already compiled several panels ;)
Btw do you have a dual monitor setup? As I remember the "send to monitor" menu item wasn't working in kwin (and wlroots too). Will look into that later too.

@marcusbritanicus
Copy link
Contributor

@gfgit Nice work. Thanks... :) Now I can rebase my work on top of this (#2043), and we can proceed.

@stefonarch
Copy link
Member

To confirm can you test last commit?

For some reason the taskbar didn't work for me in this branch, so I applied the change to Marcus' branch and can confirm it works (showing a quite terrible animation on default kwin...).

@stefonarch
Copy link
Member

Testing dual monitor: kwin detects it nicely and asks for how to use it, kanshictl works also fine once configured its setting.

Under wlroots one item is greyed out:

immagine

Under kwin_wayland both items are indistinct active.

@stefonarch
Copy link
Member

After some more testing windows button under kwin_wayland I found an issue (which isn't present in wlroots): if a window is minimized it's icon is still marked as active and it needs therefor 2 clicks to activate the window. If more windows are minimized only the one marked as active needs 2 clicks. Basically on an empty desktop still one window is marked as active in the taskbar.

@marcusbritanicus
Copy link
Contributor

I remember the "send to monitor" menu item wasn't working in kwin (and wlroots too).

In wlroots, we do not have any mechanism to send a window to a given output/screen. This will be possible on Wayfire, and Sway. Perhaps you can test if Hyprland supports it (via Hyprctl)

@stefonarch
Copy link
Member

So basically we need to grey them out in wlroots general backend?
Hyprland can surely do it with hyprctl dispatch action. In Labwc I've a titlebar menu item

    <item label="→ Other monitor">
    <action name="MoveToOutput" direction="left" wrap="yes" />
    <action name="FitToOutput" />
  </item>

Afaik configurable title bar menus exists only there.

@marcusbritanicus
Copy link
Contributor

So basically we need to grey them out in wlroots general backend?

That's right. Since there is no method in the protocol to do it, so wlroots will miss this feature.

Hyprland can surely do it with hyprctl dispatch action. In Labwc I've a titlebar menu item

So can wayfire. With the wayfire plugin, we will be able to easily implement it. In the same way, quite a few things can be done using the hyprland plugin.

@gfgit
Copy link
Member Author

gfgit commented Aug 22, 2024 via email

@gfgit
Copy link
Member Author

gfgit commented Aug 22, 2024 via email

@marcusbritanicus
Copy link
Contributor

marcusbritanicus commented Aug 22, 2024

If set it must contain the path to the plugin shared library and will take precedence over any other plugin regardless of current platform.

Why restrict to one plugin? This is cause issues, for example, if the user wants to use a single plugin, and is switching between different compositors.

Currently, in a single option you can support multiple compositors. Also, why give the path? We have support for LXQTPANEL_PLUGIN_PATH. Let the users specify all the paths using that env-var. We will simply assume that all the plugins are installed in the default location, and have a specific filename heuristic.

PS: All this is with the assumption that we will have users who will develop and use multiple custom plugins for different compositors.

@marcusbritanicus
Copy link
Contributor

I use "KDE" check to test panel in a Plasma Wayland session. I don't think it hurts keeping it there...

@gfgit FYI: If you set preferred_backend=KWIN:kde_wayland,KDE:kwin_wayland,wayfire:wfipc,sway=wlroots, we only check if the plugin loads (for the correct platform), and don't check the backend score. So, those if statements will be "just there".

Additionally, you can see the allure of writing it this way: you can use one single config file, and support multiple platforms. In this hypothetical example, KDE, KWIN, wayfire, and sway will load the given plugins (if available). We will do an auto-detect for all other compositors.

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

Successfully merging this pull request may close these issues.

9 participants