(#1762) Allow shells to enable XWayland by default

[hbatagelo]

Fixes #1762.

Allows the shell to alter the default behavior of XWayland (enabled or disabled).

• X11Support{}.default_to_enabled() allows the compositor to set the default state to enabled.

The default behavior can be overriden by setting MIR_SERVER_ENABLE_X11 or --enable-x11 to 1|0, on|off, yes|no, or true|false (boolean value from Boost.ProgramOptions). If MIR_SERVER_ENABLE_X11 is an empty string, it will default to 1.

Note to reviewers: this changes the behaviour of --enable-x11 on the command line. There now needs to be a value. Specifying the option in .config or via an environment variable is unchanged (so none of our snaps or downstreams seem to be affected). - alan_g

[AlanGriffiths]

Thanks for offering this.

You've got into a sticky area by adding an overload to a symbol that already exists in the symbols.map - a case that isn't handled by the regenerate-miral-symbols- map.py script.

There's a similar bit of hackery to what is needed in 2cb3fcf, (even using named constructor idiom would need to deal with a additional private constructor overload)

[AlanGriffiths]

We need a MIRAL_4.2 for any new entry point.

[AlanGriffiths]

Bumping MIRAL_VERSION_MINOR and running check-and-update-debian-symbols.py will do that

[AlanGriffiths]

I wonder if a named constructor would be clearer? I don't see much point in X11Support{false}

[hbatagelo]

Indeed. So, maybe something like that?

static X11Support x11_disabled_by_default();
static X11Support x11_enabled_by_default();

Would it be better to keep the default constructor as public for backwards compatibility? The downside is that it won't be clear to the user that it defaults to x11 as disabled.

[AlanGriffiths]

Think of the client code:

    ...
    X11Support::x11_enabled_by_default(),
    ...

would be rather tautological.

And, yes. We don't need to break ABI compatibility by removing the current constructor. (That would require an .so name change)

[hbatagelo]

I agree that it seems redundant, but X11Support doesn't imply X11 is enabled. In the current context, having support for X11 simply means providing a command line option to the user. Perhaps X11Support should more accurately be named X11EnableOption which could then be disabled_by_default or enabled_by_default based on the chosen named constructor. However, as we plan to keep the default constructor and only add a named one for enabling X11, I understand that we will end up with something like this:

• X11Support(): the default constructor, which doesn't enable X11. The "support" is there (i.e. the command line option), but X11 is disabled unless the user states otherwise.
• X11Support::enabled_by_default(): named constructor, which adds the command line option and also enables X11 by default.

I am open to suggestions.

[hbatagelo]

Oh, I must correct myself! Upon revisiting the code, I noticed that X11Support includes additional configuration options (x11-scale, x11-displayfd, xwayland-path): using X11EnableOption to describe its use would be incorrect.

Given that X11Support encompasses a set of initialization options with default values, I wonder if a named parameter idiom would enhance readability in this case. The client could write something like X11Support().set_enabled_by_default(), extensible to X11Support().set_enabled_by_default().set_default_scale(2)... if needed.

Another option would be to use a struct of default values and designated initializers; for instance, X11Support({.enabled = true}), extensible to X11Support({.enabled = true, scale = 1.0f, ...}).

What are your thoughts on these alternatives?

[AlanGriffiths]

I like the method chaining approach. Although I think dropping the "set_" wart, or replacing it with "with_" would be better:

    ...
    X11Support{}.default_to_enabled().default_to_scale(2),
    ...

[AlanGriffiths]

This needs more hackery: their are two identical globs that match both constructors

[codecov]

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (b8750ce) 77.82% compared to head (02ef738) 77.81%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
tests/miral/external_client.cpp	27.27%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3133      +/-   ##
==========================================
- Coverage   77.82%   77.81%   -0.01%     
==========================================
  Files        1064     1064              
  Lines       74431    74452      +21     
==========================================
+ Hits        57923    57934      +11     
- Misses      16508    16518      +10     

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

[AlanGriffiths]

Sorry to mess you around: I realise I asked you to move the symbols to Miral 4.2. But now it looks like the timing will allow this to get in the 2.16 release, so they can go in 4.1 after all.

The top comment is also wrong now, as it reflects an earlier iteration of the design. It is also worth calling out in that comment that there is a behavioural change: bin/miral-app --enable-x11 worked, now it fails. (I don't think this will break anything for anyone, but should be made explicit.)

Other than that, looks good. Thanks!