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: Support xdg-shell as a fallback #1433

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pslldq
Copy link

@pslldq pslldq commented Jan 10, 2025

Support the xdg-shell wayland protocol as a fallback in case the wlr-layer-shell-unstable-v1 protocol is not present. This allows running dunst on wayland compositors not supporting the layer shell protocol.

Note that the xdg-shell protocol doesn't allow dunst to specify where it should be displayed on the screen. Therefore it is only chosen, when the layer-shell protocol is not available.

Currently this allows dunst to also run on the Gnome shell (using a different session D-Bus) and weston.

@pslldq pslldq force-pushed the supportxdgshell branch 4 times, most recently from a1bae20 to e293572 Compare January 10, 2025 16:06
@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 89 lines in your changes missing coverage. Please review.

Project coverage is 64.57%. Comparing base (5408dfc) to head (e293572).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/wayland/wl.c 0.00% 89 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1433      +/-   ##
==========================================
- Coverage   65.32%   64.57%   -0.75%     
==========================================
  Files          50       51       +1     
  Lines        8763     8864     +101     
  Branches     1034     1046      +12     
==========================================
  Hits         5724     5724              
- Misses       3039     3140     +101     
Flag Coverage Δ
unittests 64.57% <0.00%> (-0.75%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Qingwu-Li pushed a commit to Qingwu-Li/dunst that referenced this pull request Jan 22, 2025
Update the xdg-shell client protocol to the one included in
wayland-protocols 1.39 .

This prevent the following error when using the xdg-shell protocol:
  interface 'xdg_toplevel' has no event 3

It was updated using the relevant wayland-scanner commands from
the wayland-protocols make target in a Debian Buster container
against the xdg-shell.xml file included in version 1.39.

Upstream-Status: Submitted [dunst-project#1433]
Qingwu-Li pushed a commit to Qingwu-Li/dunst that referenced this pull request Jan 22, 2025
Support the xdg-shell wayland protocol as a fallback in case the
wlr-layer-shell-unstable-v1 protocol is not present. This allows running
dunst on wayland compositors not supporting the layer shell protocol.

Note that the xdg-shell protocol doesn't allow dunst to specify where
it should be displayed on the screen. Therefore it is only chosen, when
the layer-shell protocol is not available.

Upstream-Status: Submitted [dunst-project#1433]
@bynect
Copy link
Member

bynect commented Jan 23, 2025

@alebastr sorry to bother, I'm asking you since you did work a lot on the wayland code. from the review I don't see anything wrong but maybe you can spot some problem?

also why were the protocols changed @pslldq ?

@bynect bynect requested a review from fwsmit January 23, 2025 23:00
@pslldq
Copy link
Author

pslldq commented Jan 24, 2025

The xdg_shell protocol was changed due to the included version being outdated and printing the following error on my machine (Arch Linux):

interface 'xdg_toplevel' has no event 3

As the code generated with my wayland-scanner version (1.23.1) caused build errors on Debian buster and bullseye, I've used the wayland-scanner executable from a Debian buster docker container.

While this caused an older wayland-scanner executable being used (1.16 from Debian buster) for the code generation, it uses the xdg_shell protocol file from the current wayland-protocols 1.39 release.

Qingwu-Li pushed a commit to Qingwu-Li/dunst that referenced this pull request Jan 24, 2025
Update the xdg-shell client protocol to the one included in
wayland-protocols 1.39 .

This prevent the following error when using the xdg-shell protocol:
  interface 'xdg_toplevel' has no event 3

It was updated using the relevant wayland-scanner commands from
the wayland-protocols make target in a Debian Buster container
against the xdg-shell.xml file included in version 1.39.

Upstream-Status: Submitted [dunst-project#1433]
Qingwu-Li pushed a commit to Qingwu-Li/dunst that referenced this pull request Jan 24, 2025
Support the xdg-shell wayland protocol as a fallback in case the
wlr-layer-shell-unstable-v1 protocol is not present. This allows running
dunst on wayland compositors not supporting the layer shell protocol.

Note that the xdg-shell protocol doesn't allow dunst to specify where
it should be displayed on the screen. Therefore it is only chosen, when
the layer-shell protocol is not available.

Upstream-Status: Submitted [dunst-project#1433]
@fwsmit
Copy link
Member

fwsmit commented Jan 24, 2025

I made the initial Wayland version of dunst (mostly copy pasted from the Mako notification daemon). I dont have time right now to look at it, but i can take a look at it in a week.
The Wayland scanner situation is not ideal, but I don't know of a better solution. We could also take a look at how other projects handle this

@pslldq
Copy link
Author

pslldq commented Jan 24, 2025

The Wayland scanner situation is not ideal, but I don't know of a better solution. We could also take a look at how other projects handle this

Looking at the packages depending on the Arch Linux wayland-protocols package, the common way seems to depend on the wayland-protocols package of the given distribution and generate the protocol files as part of the build process. This has the advantage of not including generated files into the repository.

But it requires to check for the wayland-protocols version and dynamically set macros to support potentially missing fields (see wl-clipboard as an example)

@alebastr
Copy link
Contributor

I'm not sure if supporting GNOME is a worthy reason to complicate the code. GNOME has its own notification daemon as an integral part of the shell, and it actually works better than anything third-party.
Weston support sounds a bit more reasonable, until you try it and see the notifications appearing randomly positioned on the screen. And most of the remaining non-wlroots-based compositors (Cosmic, niri, jay, Mir, etc) seem to support layer-shell.


The xdg_shell protocol was changed due to the included version being outdated and printing the following error on my machine (Arch Linux):

interface 'xdg_toplevel' has no event 3

This could be solved without touching the protocol files by binding to xdg_wm_base_interface version 3 or lower. You don't use anything from versions 4 or 5 and thus don't need it.

Requesting version 5 also breaks dunst with older compositors (anything before wlroots 0.18):

wl_registry@2: error 0: invalid version for global xdg_wm_base (11): have 2, wanted 5

I suggest doing something like this:

                 ctx.xdg_shell = wl_registry_bind(registry, name,
-                                &xdg_wm_base_interface, 5);
+                                &xdg_wm_base_interface, MIN(version, 3));
                 xdg_wm_base_add_listener(ctx.xdg_shell, &xdg_wm_base_listener, NULL);

and removing the protocol change commit.


Some changes in wl.c look suspicious, but I haven't touched the dunst code for almost a year. I'll try to find time for a closer look within a week.

Support the xdg-shell wayland protocol as a fallback in case the
wlr-layer-shell-unstable-v1 protocol is not present. This allows running
dunst on wayland compositors not supporting the layer shell protocol.

Note that the xdg-shell protocol doesn't allow dunst to specify where
it should be displayed on the screen. Therefore it is only chosen, when
the layer-shell protocol is not available.
@pslldq
Copy link
Author

pslldq commented Jan 27, 2025

I'm not sure if supporting GNOME is a worthy reason to complicate the code. GNOME has its own notification daemon as an integral part of the shell, and it actually works better than anything third-party.
Weston support sounds a bit more reasonable, until you try it and see the notifications appearing randomly positioned on the screen.

The main use case is weston with the ivi-shell, which allows to programmatically control the window layout.

Without an external window positioning system, it is not really useful for a day to day use (resulting from the fact that the xdg-shell doesn't provide the means to set the global window positions). But it might be useful in case of development, where you're just interested in testing some input, D-Bus or rendering changes (e.g. avoid having to spawn a nested compositor on a GNOME/Weston desktop).

This could be solved without touching the protocol files by binding to xdg_wm_base_interface version 3 or lower. You don't use anything from versions 4 or 5 and thus don't need it.

Requesting version 5 also breaks dunst with older compositors (anything before wlroots 0.18):

oh, thanks. I was thinking "latest is greatest" when deciding what to use for the version and didn't realize that you want to use the lowest version number required for your application. As I also don't seem to use anything from version 2 or 3, I've now fixed it to version 1 for the broadest range of compability (and removed the protocol change commit).

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

Successfully merging this pull request may close these issues.

5 participants