-
Notifications
You must be signed in to change notification settings - Fork 107
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
(#1762) Allow shells to enable XWayland by default #3133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
debian/libmiral6.symbols
Outdated
@@ -424,3 +424,4 @@ libmiral.so.6 libmiral6 #MINVER# | |||
MIRAL_4.1@MIRAL_4.1 4.1.0 | |||
(c++)"miral::WaylandExtensions::zwp_input_method_v1@MIRAL_4.1" 4.1.0 | |||
(c++)"miral::WaylandExtensions::zwp_input_panel_v1@MIRAL_4.1" 4.1.0 | |||
(c++)"miral::X11Support::X11Support(bool)@MIRAL_4.2" 4.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a MIRAL_4.2 for any new entry point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping MIRAL_VERSION_MINOR
and running check-and-update-debian-symbols.py
will do that
include/miral/miral/x11_support.h
Outdated
@@ -32,6 +32,7 @@ class X11Support | |||
void operator()(mir::Server& server) const; | |||
|
|||
X11Support(); | |||
explicit X11Support(bool x11_enabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if a named constructor would be clearer? I don't see much point in X11Support{false}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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),
...
src/miral/symbols.map
Outdated
MIRAL_4.2 { | ||
global: | ||
extern "C++" { | ||
miral::X11Support::X11Support*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs more hackery: their are two identical globs that match both constructors
Codecov ReportAttention:
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. |
79c02d8
to
9e426e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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). IfMIR_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