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

Provide a configuration mechanism for enabling the supported Wayland extensions #538

Merged
merged 5 commits into from Aug 13, 2018

Conversation

AlanGriffiths
Copy link
Contributor

This provides a foundation for future development.

But this is likely to gain complexity over time:

  • A shell may want to validate the selected extensions.
  • Some extensions may be mutually incompatible.
  • We may want to change the default from "enable everything".

@wmww
Copy link
Contributor

wmww commented Aug 13, 2018

Is this intended to include a WLCS sync?

@AlanGriffiths
Copy link
Contributor Author

Is this intended to include a WLCS sync?

Oops! No. Let me fix...

Copy link
Contributor

@wmww wmww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor stuff, non blocking.

std::set<std::string> selected_extension;
auto extensions_ = extensions + ':';

for (char const* start = extensions_.c_str(); char const* end = strchr(start, ':'); start = end+1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a problem, but seems a bit hacky to do this with C strings and strchr, instead of std::string::find.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes the C API leads to clearer code. IMO the C++ version is too verbose.

protected:
virtual void custom_extensions(
wl_display* display,
std::shared_ptr<mf::Shell> const& shell,
mf::WlSeat* seat,
mf::OutputManager* const output_manager)
{
if (!getenv("MIR_DISABLE_XDG_SHELL_V6_UNSTABLE"))
add_extension("zxdg_shell_v6", std::make_shared<mf::XdgShellV6>(display, shell, *seat, output_manager));
if (extension.find(wl_shell) != extension.end())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another extension could, theoretically, have a name that contains "wl_shell".

if ((":" + extension + ":").find(":" + wl_shel +":") != extension.end())

Would be uglier, but more technically correct IMO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably also a leaner way to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wmww That code is looking for a value "wl_shell" in a set. It doesn't match a value containing "wl_shell".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, oops. I thought I read that carefully.

@wmww
Copy link
Contributor

wmww commented Aug 13, 2018

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 13, 2018

Merge conflict

@wmww
Copy link
Contributor

wmww commented Aug 13, 2018

bors r+

bors bot added a commit that referenced this pull request Aug 13, 2018
538: Provide a configuration mechanism for enabling the supported Wayland extensions r=wmww a=AlanGriffiths

This provides a foundation for future development.

But this is likely to gain complexity over time:

  * A shell may want to validate the selected extensions.
  * Some extensions may be mutually incompatible.
  * We may want to change the default from "enable everything".

Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
Co-authored-by: William Wold <9331264+wmww@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Aug 13, 2018

Build failed

@wmww
Copy link
Contributor

wmww commented Aug 13, 2018

3rd times the charm
bors r+

bors bot added a commit that referenced this pull request Aug 13, 2018
538: Provide a configuration mechanism for enabling the supported Wayland extensions r=wmww a=AlanGriffiths

This provides a foundation for future development.

But this is likely to gain complexity over time:

  * A shell may want to validate the selected extensions.
  * Some extensions may be mutually incompatible.
  * We may want to change the default from "enable everything".

Co-authored-by: Alan Griffiths <alan@octopull.co.uk>
Co-authored-by: William Wold <9331264+wmww@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Aug 13, 2018

Build succeeded

@bors bors bot merged commit ee7a210 into master Aug 13, 2018
@bors bors bot deleted the select-wl-extensions branch August 13, 2018 23:29
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.

None yet

2 participants