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

Add xwayland command #3144

Merged
merged 1 commit into from
Jan 14, 2019
Merged

Add xwayland command #3144

merged 1 commit into from
Jan 14, 2019

Conversation

emersion
Copy link
Member

@emersion emersion commented Nov 17, 2018

This allows xwayland to be disabled at runtime.

Xwayland needs to be started in server_start, after the config has been read.

Closes #2965

@RyanDwyer
Copy link
Member

No docs?

Also, I assume you mean config-time and not runtime.

@emersion
Copy link
Member Author

No docs?

Oh right, I always forget those. Fixed.

Also, I assume you mean config-time and not runtime.

I meant "runtime" as the opposite of "compile-time". But yeah it can only be used in the config file.

@ddevault
Copy link
Contributor

Hmm, I don't think I like this. I would prefer it as -Dxwayland=no

@emersion
Copy link
Member Author

Disabling Wayland is not a debug option. It should be in the config file, you don't want to do it temporarily.

@ddevault
Copy link
Contributor

If you want to do it long-term, just uninstall Xwayland?

@emersion
Copy link
Member Author

The whole point is that uninstalling Wayland doesn't work. wlroots will still try to to launch it (which will fail, but set DISPLAY to a bad value). See the original issue.

@ddevault
Copy link
Contributor

wlroots will still try to to launch it (which will fail, but set DISPLAY to a bad value).

Seems like a bug.

@emersion
Copy link
Member Author

Seems like a bug.

Well, our only option would be to try to find Xwayland in $PATH which is not cool.

@emersion
Copy link
Member Author

It's also useful to be able to disable Xwayland even if you have it installed.

@ddevault
Copy link
Contributor

I dunno, I'm still -1 to this. We'll see what everyone else thinks.

@emersion
Copy link
Member Author

You haven't explained why you don't like it, though.

@ddevault
Copy link
Contributor

All features have a maintenance and complexity cost. I don't think this one pays its bill.

@emersion
Copy link
Member Author

I don't think a +72 −39 feature will have a high maintenance cost. Users can disable Xwayland anyway at build-time, so doing it at runtime doesn't add complexity. Eventually we'll want to have it anyway, as more applications migrate to Wayland. It's also already available in rootston, and the maintenance cost there has been zero.

@ddevault
Copy link
Contributor

rootston isn't a user-facing compositor, we don't have to deal with support.

@emersion
Copy link
Member Author

Sure, I only mentioned rootston because you mentioned code complexity and maintainability.

@ianyfan @RyanDwyer @RedSoxFan Thoughts?

@RyanDwyer
Copy link
Member

The only use case I can think of is a user who doesn't want to use Xwayland and who is using a distribution version of sway which has it enabled at compile time. It may be useful for someone who is running sway on a low memory device and wants as many resources as possible available for other things.

I don't see any maintenance or complexity overhead in having it, so +1 from me.

@RedSoxFan
Copy link
Member

wlroots will still try to to launch it (which will fail, but set DISPLAY to a bad value).

Seems like a bug.

I agree that this does not sound like the correct/ideal behaviour, but I am not currently familiar enough with related code to suggest a solution.

With that said, I don't think that this adds any maintenance or complexity overhead since it can only be adjusted on launch so +1 from me.

I would like to suggest a small addition/change to this implementation however. Currently on reload, the command will be executed but silently ignored. I think that if the config is being reloaded and the value changes, swaynag should be spawned to notify the user that the new value won't take effect until sway is restarted.

@ddevault ddevault merged commit 4879d40 into swaywm:master Jan 14, 2019
@ddevault
Copy link
Contributor

Thanks!

@emersion emersion deleted the cmd-xwayland branch January 14, 2019 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants