Created attachment 4385 [details]
testscale with decorations

Created attachment 4386 [details]
testsprite2 with decorations

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).

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

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.

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

(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.

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.

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?

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.

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.

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

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.

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

patch set for client-side decorations

(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.

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

(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, ...).

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.

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

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

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.

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!

(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?

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

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

This looks pretty good.

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

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

>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.

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.

(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.

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.

>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!

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

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?

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

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.

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.

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

(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.

Created attachment 4762 [details]
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.

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.

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.)

(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.

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.

(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.

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.

Updated TODO:

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

(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?

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.

(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.

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