wayland client-side decorations

[SDLBugzilla]

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• set of patches for window decorations (sdl_decoration_patches.tar.xz, application/x-xz, 2020-06-15 22:33:47 +0000, 7332 bytes)
• testscale with decorations (sdl_decoration_scale.png, image/png, 2020-06-15 22:35:42 +0000, 166638 bytes)
• testsprite2 with decorations (sdl_decoration_sprite2.png, image/png, 2020-06-15 22:36:22 +0000, 34977 bytes)
• set of patches for window decorations (ver 2) (sdl_decoration_patches.tar.xz, text/plain, 2021-01-26 22:00:39 +0000, 7224 bytes)
• libdecoration Draft (libdecoration_dynamic_wip.diff, text/plain, 2021-01-30 07:24:42 +0000, 19723 bytes)
• libdecoration API fixes (0001-API-Avoid-stdbool-do-not-include-wayland-client.h.patch, text/plain, 2021-02-03 22:57:40 +0000, 3948 bytes)
• Whitespace fix (wlwhitespace.diff, text/plain, 2021-02-05 00:12:34 +0000, 796 bytes)
• libdecoration Support (libdecoration.diff, text/plain, 2021-02-05 21:01:47 +0000, 27458 bytes)

Reported in version: 2.0.15
Reported for operating system, platform: Linux, All

Comments on the original bug report:

On 2020-06-15 22:33:47 +0000, Christian Rauch wrote:

Created attachment 4384
set of patches for window decorations

The attached set of patches implement client-side window decorations on Wayland using 'libdecoration' from https://gitlab.gnome.org/jadahl/libdecoration.

All patches are compressed into a single archive as I am unable to add multiple attachments. You can follow the upstream work at https://github.com/christianrauch/SDL/tree/decoration.

One patch additionally enables AddressSanitizer for debug builds (remove if you don't want this) and wl_proxy tag support to distinguish between owners of a wl_surface in callbacks. The libdecoration dependency is downloaded directly from upstream at build-time.

This fixes https://bugzilla.libsdl.org/show_bug.cgi?id=2710

Cheers and thanks for making Linux gaming happen :-)

On 2020-06-15 22:35:42 +0000, Christian Rauch wrote:

Created attachment 4385
testscale with decorations

On 2020-06-15 22:36:22 +0000, Christian Rauch wrote:

Created attachment 4386
testsprite2 with decorations

On 2020-09-11 11:49:48 +0000, Lothar Serra Mari wrote:

Thanks a lot for this patch!

In my opinion, this is clearly a step in the right direction, since it seems unlikely that the GNOME developers will ever change their mind.

This provides a clean solution for all SDL2 applications, so I'd really appreciate to see this merged on day.

I gave this a try on my own - looking good so far! (Tested with the current master branch of ScummVM which is an SDL2 application).

On 2020-10-29 16:58:08 +0000, David Heidelberg (okias) wrote:

Do I understand right, this could make move ahead in case of:

Allowing apps run under Wayland backend ([patch] Wayland: SDL applications start in XWayland by default) [1]
and would close [2]?

Thank you guys!

[1] https://bugzilla.libsdl.org/show_bug.cgi?id=3948
[2] https://bugzilla.libsdl.org/show_bug.cgi?id=4845

On 2020-10-31 14:22:31 +0000, Christian Rauch wrote:

This would indeed implement the decorations and close bug 4845.

It will also bring SDL closer to full Wayland support, but if this will be enabled by default (bug 3948) is up to the developers. So far, I have not received any feedback from developers on the integration of client-side decorations, so I am not sure if there is even interest in fully supporting Wayland.

On 2020-11-18 15:46:40 +0000, Ryan C. Gordon wrote:

*** Bug 4845 has been marked as a duplicate of this bug. ***

On 2020-11-18 15:48:54 +0000, Ryan C. Gordon wrote:

(In reply to Christian Rauch from comment # 5)

decorations, so I am not sure if there is even interest in fully supporting
Wayland.

I haven't looked at these patches yet--and won't until after 2.0.14 ships--but I am definitely interested in this. It seems to me that libdecoration is the only reasonable way forward here for client-side decorations in SDL.

--ryan.

On 2021-01-23 20:16:36 +0000, Ethan Lee wrote:

Some feedback having looked at the Wayland driver recently...

For SDL, there are two functions in particular that are available in the X11 driver that libdecoration might care about: GetWindowBordersSize and SetWindowResizable. SetWindowResizable was thankfully easy to implement since all we have to do is ignore/react based on what our window flags are, but looking at the callbacks I wasn't sure if libdecoration was able to tell the difference between a fixed-size and resizable window, since all that work is handled in a callback rather than a readable/writeable attribute like in X11:

https://hg.libsdl.org/SDL/file/a3d4219798f6/src/video/x11/SDL_x11window.c#l1072

GetWindowBordersSize is less important to me but is worth noting for full X11 parity - is there a way for us to get the size of the window borders generated by libdecoration?

Aside from that I didn't notice anything major; the current patchset will have to be rewritten to replace ifdef-y stuff with dynamic loading of libdecoration (falling back when unavailable) but that's purely on the SDL side of things.

On 2021-01-23 21:26:59 +0000, Christian Rauch wrote:

Note that the most recent version will be available at https://github.com/christianrauch/SDL/tree/decoration as I will frequently rebase on SDL master and also adapt the implementation to libdecoration changes. I will create a new patch set once the changes look good to you. (I don't want to spam you with new patch sets every time I rebase or update the implementation.)

(In reply to Ethan Lee from comment # 8)

I wasn't sure if libdecoration was able to tell the difference between
a fixed-size and resizable window
libdecoration has "capabilities": https://gitlab.gnome.org/jadahl/libdecoration/-/blob/6150ef8f/src/libdecoration.h#L135-137 to set/unset MOVE/RESIZE/MINIMIZE actions. All this has to be handle client-side, so libdecoration will just ignore these requests. There is no protocol yet to prevent this server-side.

GetWindowBordersSize is less important to me but is worth noting for full
X11 parity - is there a way for us to get the size of the window borders
generated by libdecoration?
This information is definitely available internally, but it is not fully exposed to clients so far. The window size is stored in "libdecor_configuration", which is private, and the content size can be obtained via the public "libdecor_configuration_get_content_size".

I am sure we can add API for getting the decoration border size if there is demand.

The current patchset will have to be rewritten to replace ifdef-y stuff with
dynamic loading of libdecoration (falling back when unavailable) but that's
purely on the SDL side of things.
Is this something you could help me with?

On 2021-01-23 21:32:17 +0000, Ethan Lee wrote:

I can definitely help with the dynamic loading part, sure. The hard part will be adding it to configure.ac, while the rest is just adding it to the gelatinous blob of shell "stuff" we have already.

I may be wrong on this but I believe GetWindowBordersSize was among the additions to SDL that was specifically for GUI implementation support, with the primary consumer being Unreal Engine 4 - so while it's not used in a lot of different engines it is definitely popular if only because it's used in the most popular AAA-scale engine on the market.

On 2021-01-23 21:39:24 +0000, Ethan Lee wrote:

Just grepped the UE4 source code and can confirm it is used in both the editor as well as the end-user runtime, so GetWindowBordersSize is definitely actively used.

I also found the symbol in the Unity binary but couldn't confirm if it was in use, though I suspect the Unity Editor at minimum uses it for roughly the same purpose since they have since moved to SDL for Linux support.

On 2021-01-26 04:28:27 +0000, Sam Lantinga wrote:

Can you make libdecoration optional, dynamically loading and using it when available, like some of the other SDL dependencies?

On 2021-01-26 04:30:49 +0000, Ethan Lee wrote:

I believe that's the plan - Christian: Can you rebase your existing patchset against default? I may need some help with the autoconf side of things but I can try to work on the dynamic loading this week.

On 2021-01-26 22:00:39 +0000, Christian Rauch wrote:

Created attachment 4708
set of patches for window decorations (ver 2)

patch set for client-side decorations

On 2021-01-26 22:04:28 +0000, Christian Rauch wrote:

(In reply to Ethan Lee from comment # 13)

Can you rebase your existing patchset against default?

I rebased on master and uploaded a new (ver 2) patch set.

On 2021-01-27 19:49:22 +0000, Sam Lantinga wrote:

In case anyone has trouble with it, the attachment "set of patches for window decorations (ver 2)" is tar.xz format.

On 2021-01-27 20:23:13 +0000, Christian Rauch wrote:

(In reply to Sam Lantinga from comment # 16)

In case anyone has trouble with it, the attachment "set of patches for
window decorations (ver 2)" is tar.xz format.

The patch-set contains multiple commits that I compressed into a single archive. Let me know if you need the patches in a different format.

Also feel free to merge the patches that are unrelated to the decorations (AddressSanitizer, memory fixes, ...).

On 2021-01-29 13:53:08 +0000, Ellie wrote:

Would it be possible to integrate this such that libdecoration is found via dlopen() and picked up from the Linux system it is on, while otherwise not requiring it via full dynamic link to be present?

My apologies if it's already handled like that, I'm asking because this would make statically building SDL2 easier: it wouldn't require baking libdecoration too, and instead a static portable SDL2 app could then just pick it up & use it if present and otherwise still work on KDE, non-Wayland GNOME, Phosh, and others as expected.

If that's too much work that's fine, I just thought it wouldn't hurt to suggest this approach.

On 2021-01-29 15:28:18 +0000, Ethan Lee wrote:

dlopen is the only way we'd ever do it!

I'll try to take a look at this today.

On 2021-01-30 07:24:42 +0000, Ethan Lee wrote:

Created attachment 4736
libdecoration Draft

Attached is a diff that reworks patches 2 and 4 into something that would be more likely to land in upstream. I ignored patch 1, skipped 4 because the build system is a whole other issue, and skipped 3 because that should probably be sent in as its own patch separately from this issue.

In this diff I added a ton of 'FIXME libdecoration' blocks for someone else to assess - some of it is on the side of libdecoration while others are mostly dealing with the integration of libdecoration.h, which was surprisingly difficult to figure out without adding HAVE_LIBDECORATION_H all over the place (my guess is, we'll be adding HAVE_LIBDECORATION_H all over the place). It's possible that we'll have to do the dynamic loading of libdecoration separately from SDL_waylanddyn.h/SDL_waylandsym.h, which would suck but may be necessary since everything else is always available to us.

Hopefully this is enough for someone to go on for getting this wrapped up!

On 2021-01-30 15:14:25 +0000, Christian Rauch wrote:

(In reply to Ethan Lee from comment # 20)

Created attachment 4736 [details]
libdecoration Draft

Attached is a diff that reworks patches 2 and 4 into something that would be
more likely to land in upstream. I ignored patch 1, skipped 4 because the
build system is a whole other issue, and skipped 3 because that should
probably be sent in as its own patch separately from this issue.

Thanks!

I am unable to apply the patches on top of my branch / patch-set (neither via git nor via hg). This is probably because I am not very familiar with this system. I am using web interfaces like GitLab, Bitbucket or GitHub mostly.

Can you give me a hint in which order I have to apply the patches onto the current tip/master/default?

On 2021-01-30 16:36:44 +0000, Ethan Lee wrote:

Made a git mirror, should be easier to work with:

https://github.com/flibitijibibo/SDL-mirror/commit/7e7a961a927ce283584cd3fbf0c90c037921aa48

On 2021-01-30 20:44:54 +0000, Sam Lantinga wrote:

This looks pretty good.

Do you need to set data->shell.libdecoration = NULL after this call?
libdecor_unref(data->shell.libdecoration)

On 2021-01-30 20:45:20 +0000, Sam Lantinga wrote:

Also, have you done testing with and without libdecoration being available?

On 2021-01-30 21:45:03 +0000, Ethan Lee wrote:

Do you need to set data->shell.libdecoration = NULL after this call?
libdecor_unref(data->shell.libdecoration)

Yeah, we should - branch updated.

Also, have you done testing with and without libdecoration being available?

I've actually been working on this cleanup without even having the libdecoration source on my PC... I've been doing build tests by forward declaring what we use wherever I could (no spot was very good, which is probably the only big issue on the source side of things, maybe the build system changes will fix that). For the most part this patchset just corrects the ifdef goo so that the new backend doesn't delete the existing functionality.

On 2021-01-30 22:12:26 +0000, Ellie wrote:

I am wondering now, are there any plans to convince distributions to preinstall libdecoration as a sort of wayland core component with any wayland desktop? Because otherwise it might be actually interesting to possibly statically link it, although it seems more reasonable to me to treat it as external infrastructure component by the distro. But e.g. on Fedora 33 where I have GNOME installed there is no libdecoration to be found, even though I run GNOME Wayland - so pretty much the environment where it really would be necessary. Just throwing it in here in case that gives some useful ideas about whether dlopen() will be the only possible path or if it should be, and/or if some distributors might need to be poked once this is in.

On 2021-01-31 00:24:03 +0000, Christian Rauch wrote:

(In reply to Ellie from comment # 26)

I am wondering now, are there any plans to convince distributions to
preinstall libdecoration as a sort of wayland core component with any
wayland desktop? Because otherwise it might be actually interesting to
possibly statically link it, although it seems more reasonable to me to
treat it as external infrastructure component by the distro.

Yes, in the long term libdecoration should be packaged for distributions as probably many more projects will want to use a library to implement decorations on wayland. Until libdecoration in widely available, I guess projects have to provide their own copy of shared objects or just link statically. The plugins that actually draw the decorations are dynamically loaded, with the idea that this could be done dependant on the desktop environment.

Saying that, it would be useful to decide this now. I used #ifdefs all over the wayland backend assuming that the decorations are enabled/disabled at compile-time. If it is only used via dlopen, I assume that there should be no "#ifdef DECORATION" left and everything should be forward declared?

@Ethan Lee
I rebased your patch on top of my decoration branch:
https://github.com/christianrauch/SDL/tree/decoration
It does not compile, as you said, and it will take me a while to figure out how to transform the remaining code to the dlopen behaviour. If you want to continue on this, you can maybe just fork from there and send a PR to my repo, I will then upload the patches here once things work as expected.

On 2021-01-31 00:44:35 +0000, Ellie wrote:

I would prefer if distributions shipped it, I am just wondering if anyone has asked any if they plan to. Because it would need to be preinstalled with desktop environments and I imagine it might be harder to convince distributions this is the case. (No user will know this needs to be manually installed, and if it's just pulled in via deps of specific programs that use it then e.g. AppImage-based programs from the web will be severely handicapped.)

Now if distros DONT play along with this, the use is dlopen() only could be a bit problematic since dlopen() would prevent easy fully static link into the program, wouldn't it? So programs meant to be shipped without additional files (e.g. think of an installer using SDL2) would then be left out. The projects I am working on tend to be of this kind, this is why I brought it up.

On 2021-01-31 01:19:02 +0000, Ethan Lee wrote:

If you want to continue on this, you can maybe just fork from there and send a PR to my repo, I will then upload the patches here once things work as expected.

I can test patches as they roll in, but I won't be able to do much from here on out other than review minor style things. Hopefully the dlopen changes got a bunch of busy work out of the way though!

On 2021-02-01 16:18:17 +0000, Cameron Gutman wrote:

I'm glad this is getting closer to fruition.

I left some review comments on your latest commit: https://github.com/christianrauch/SDL/commit/cdd9ce0ab61119a33428e39fb8f958e2ed66f9b2

On 2021-02-03 00:13:56 +0000, Christian Rauch wrote:

I got the dlopen-ing working. I created a new branch for the time being:
https://github.com/christianrauch/SDL/tree/decoration_dlopen

The symbols are now resolved at runtime. If the "libdecoration.so" cannot be found it will fall back to the previous xdg surfaces without decorations.

I did not fix any of the FIXME since I wanted to get the basic dlopen function running first.

Two issues are left:

1. I had to set the library name "libdecoration.so" manually in "sdlchecks.cmake". "FindLibraryAndSONAME(decoration)" did not work for me.

2. For some reason, some of the symbol renaming ("WAYLAND_libdecor_" -> "libdecor_") causes conflicts. E.g.:
   "#define libdecor_unref (*WAYLAND_libdecor_unref)"
   works, but
   "#define libdecor_new (*WAYLAND_libdecor_new)"
   gives me "error: conflicting types for 'WAYLAND_libdecor_new'" (with the conflicting symbol in header "libdecoration.h"). It looks like all functions that return some value (non-void) cannot be renamed like this.
   @Ethan Is this supposed to work, or do I have to use the "WAYLAND_" symbols?

On 2021-02-03 22:03:41 +0000, Ethan Lee wrote:

Buildfixes: https://github.com/flibitijibibo/SDL-mirror/commit/7d2dbc529e4ece6bfb28bcb62902dddec607df19

On 2021-02-03 22:23:24 +0000, Ethan Lee wrote:

One more commit, this fully compiles now:

https://github.com/flibitijibibo/SDL-mirror/commit/1860847fdf38f61a5eb761b0d27f4ece5f96ba01

Linking is a different story... it's mysteriously double-defining the dynamic function pointers for some reason.

On 2021-02-03 22:39:15 +0000, Ethan Lee wrote:

Found the bug: libdecoration.h includes wayland-client.h, which basically means including libdecoration.h can't work at all! We need to include libdecoration after this line...

https://hg.libsdl.org/SDL/file/53efc2c1b4c8/src/video/wayland/SDL_waylanddyn.h#l40

... because if we don't we end up macro'ing over the libdecoration.h function declarations, leading to double definitions. Problem is, by including it there, we trip this error handler (or rather we should, but it doesn't check for WAYLAND_CLIENT_H, another unrelated bug):

https://hg.libsdl.org/SDL/file/53efc2c1b4c8/src/video/wayland/SDL_waylanddyn.h#l67

So the two things we need from libdecoration are:

1. Replace stdbool.h with uint8_t
2. Forward declare the related wayland-client.h structs and include this header in the libdecoration source, rather than the API header

Once that's done, all of the libdecoration.h includes can be deleted in favor of that one spot in waylanddyn.h, and everything should compile and link properly after that.

On 2021-02-03 22:57:40 +0000, Ethan Lee wrote:

Created attachment 4756
libdecoration API fixes

Apologies for the spam, but since I was proposing API changed I figured I may as well write them in...

With the attached patch to libdecoration, SDL-libdecoration now fully compiles and links with the following branch:

https://github.com/flibitijibibo/SDL-mirror/tree/decoration_dlopen

On 2021-02-03 23:26:31 +0000, Christian Rauch wrote:

(In reply to Ethan Lee from comment # 35)

Created attachment 4756 [details]
libdecoration API fixes

Apologies for the spam, but since I was proposing API changed I figured I
may as well write them in...

With the attached patch to libdecoration, SDL-libdecoration now fully
compiles and links with the following branch:

https://github.com/flibitijibibo/SDL-mirror/tree/decoration_dlopen

Thanks for your linking fixes.

Actually, your previous commit "Almost linking..." already worked in my setup. Meaning that it linked successfully and the test executables also worked as expected. On the contrary, your next commit "libdecoration links!" does not compile on my setup :-)

Is there a SDL CI somewhere, where these things can be tested on "neutral" ground?

Btw, feel free to send pull-request via GitHub directly to my branch to simplify the development there. We can then post the merged/squashed patches here again when they are ready for review. I will squash some of your commits already into my branch and rewrite the history. So you may have to fetch changed and rebase again.

On 2021-02-05 00:12:34 +0000, Ethan Lee wrote:

Created attachment 4762
Whitespace fix

On the code, side, this branch is now ready for review:

https://github.com/christianrauch/SDL/tree/decoration_dlopen

The CMake stuff may be different in the final patch, however - more than likely we'll wait for libdecoration's first official release (AKA the ABI freeze), then instead of having it as an external project we'll depend on the distribution to provide it when building.

There are a couple other small diffs in that branch, but I filed # 5531 and attached another quick patch here deal with those.

On 2021-02-05 01:33:19 +0000, Sam Lantinga wrote:

We definitely shouldn't ship with support for libdecoration before it reaches first stable release. Looking at it, I can definitely see that it's going to evolve quite a bit from here.

On 2021-02-05 01:38:04 +0000, Ellie wrote:

Maybe it could be merged a build option that is disabled by default for now? So curious people can manually build SDL2 and test libdecoration use. (My apologies if that was the plan all along.)

On 2021-02-05 01:40:46 +0000, Christian Rauch wrote:

(In reply to Ethan Lee from comment # 37)

On the code, side, this branch is now ready for review:

https://github.com/christianrauch/SDL/tree/decoration_dlopen

Nevermind my previous comment about linking issues. I overlooked that you attached a patch for libdecoration.
The "decoration_dlopen" now has two commits:

1. "dynamic libdecoration loading" this is with Ethan's initial dlopen changes and some minor changes from my side, and works with the upstream libdecoration
2. "use libdecoration header (with custom patch)" is the one that only works with Ethan's libdecoration patch

The CMake stuff may be different in the final patch, however - more than
likely we'll wait for libdecoration's first official release (AKA the ABI
freeze), then instead of having it as an external project we'll depend on
the distribution to provide it when building.

I agree with waiting for a released version with ABI freeze. However, I doubt that distributions will pick that up quickly, especially since nothing depends on it right now (chicken or egg problem) and the Wayland support is not even active by default.
I would still propose to depend on an upstream released version for now, so that the Wayland+Decoration functionality can be tested together in real applications.

On 2021-02-05 03:31:59 +0000, Ethan Lee wrote:

As soon as a tag is established we can apply the final version of this change, and proposing the new package should be pretty straightforward with SDL to show as a primary consumer. I imagine with Wayland approaching critical mass at this point it won't be that hard of a pitch, particularly for any distribution shipping GNOME by default.

For now we can just keep the libdecoration patch as a separate thing to be rebased every now and then. Just about the only thing we could do to polish this further...

• Change DECORATION to HAVE_LIBDECORATION_H
• Add ifdefs to libdecoration paths, to ensure building without libdecoration is still possible
• Add support for both dynamic loading and dynamic linking paths
• autoconf support
• SetWindowResizable, SetWindowBordered support
• GetWindowBordersSize support

Once all that's done we can squash the whole diff down to a single hg commit to be hosted here, and as long as someone keeps it up to date we can import it the same day the ABI is frozen.

On 2021-02-05 14:07:06 +0000, Sam Lantinga wrote:

(In reply to Ethan Lee from comment # 41)

Once all that's done we can squash the whole diff down to a single hg commit
to be hosted here, and as long as someone keeps it up to date we can import
it the same day the ABI is frozen.

That sounds like a good plan. I'll leave this bug here on WAITING until then.

On 2021-02-05 21:01:47 +0000, Ethan Lee wrote:

Created attachment 4765
libdecoration Support

Attaching an updated patch to obsolete the others, omits the CMake changes for now. This includes the 5531 patch, whitespace patch, and unused SDL_DisplayMode variable fix.

Updated TODO:

• Remaining 'FIXME libdecoration' lines, mostly API issues
• CMake support for both dynamic loading and dynamic linking
• autoconf support

On 2021-02-05 21:23:21 +0000, Christian Rauch wrote:

(In reply to Ethan Lee from comment # 43)

Created attachment 4765 [details]
libdecoration Support

Attaching an updated patch to obsolete the others, omits the CMake changes
for now. This includes the 5531 patch, whitespace patch, and unused
SDL_DisplayMode variable fix.

I am bit confused. This patch contains changes that are already part of the "decoration" / "decoration_dlopen" branch. Is this just an alternative version to these branches or does the patch represent the same version just with squashed commits?

On 2021-02-05 21:25:28 +0000, Ethan Lee wrote:

This is a squashed version that can be imported into Mercurial directly. It's the same thing as the branch I have submitted as a pull request to the Git version.

On 2021-02-05 21:49:50 +0000, Christian Rauch wrote:

(In reply to Ethan Lee from comment # 45)

This is a squashed version that can be imported into Mercurial directly.
It's the same thing as the branch I have submitted as a pull request to the
Git version.

Hm, I never received a PR to my GitHub branches. How would you like to proceed to integrate your changes into my branch? I am likely to apply more changes to my branch before an initial version of libdecoration gets released.
I would prefer to get PRs on GitHub or alternatively patches that I can apply on top of my branch. I also propose to keep non-decoration commits (e.g. white-space fixes, unused variables, ...) separate.

On 2021-02-05 21:51:20 +0000, Ethan Lee wrote:

You may need to watch the repo, for some reason it doesn't always happen. You're looking for this:

https://github.com/christianrauch/SDL/pull/1

[flibitijibibo]

CCing @christianrauch, we can replace this issue with a Pull Request marked as a draft now that SDL's migration is done.