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

Make create_output more useful #8381

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kennylevinsen
Copy link
Member

Gives a name argument to create_output and adds a new destroy_output.

This makes scripting adding, configuring and removing temporary headless outputs fairly reasonable, which has its uses. Controlled multi-window screen sharing sessions for one.

Does not yet address the manpage problem if we want to expose this as "official" commands.

@emersion
Copy link
Member

Hm, not a huge fan of allowing users to set an arbitrary name. This allows for conflicts, and naming an output something that'll get used by any backend in the future will result in fireworks...

@kennylevinsen
Copy link
Member Author

This allows for conflicts, and naming an output something that'll get used by any backend in the future will result in fireworks...

I intentionally caused some conflicts, and it doesn't do anything exciting.

It just means that there's a risk of one config applying to two outputs and causing, say, overlaps. So I think it's suboptimal to have conflicts, but no more harmful than accidentally configuring overlaps between two normal outputs.

Due to how our output handling works, I think we need a way to use a fixed name, but it doesn't have to be entirely arbitrary. We could restrict prefixes or suffixes, e.g. disallowing the normal output name prefixes.

To be honest, it's not really a footgun so we could also just document that it's up to the user to pick a unique name that won't conflict.

@emersion
Copy link
Member

The protocol disallows two outputs with the same name.

Can't we return the new output name in the command response instead?

@kennylevinsen
Copy link
Member Author

kennylevinsen commented Oct 11, 2024

Ah dang, I wasn't aware of the protocol requirement so thought it was just an internal issue. wl_output haunting us as usual...

Returning the name might be helpful regardless, but if it's incremented every time then one still needs to apply new configuration each time an output is created (say, if you want to change the size or position of the headless output), with each unique configuration piling up indefinitely in sway. Not great :/

Thoughts off the top of my head:

  1. We can do a uniqueness check against current outputs. Combined with a prefix, this gives us a guarantee as good as what the backends currently provide. Allow configuration ahead of time for the user's various use-cases.

  2. We could make the backends reuse names so e.g. HEADLESS-0 would be reused as long as you never create two outputs in a row. Requires the backend to keep a reference of created outputs. It only allows configuration ahead of time in some cases, but avoids pile-up of configuration.

  3. We could add a --wipe-config flag to the destroy_output command or otherwise make output config removable. Doesn't allow configuration ahead of time, but allows just-in-time configuration without any pile-up.

@emersion
Copy link
Member

BTW, I think bumping the number in the name is actually a good thing - it prevent race conditions.

@kennylevinsen
Copy link
Member Author

I have split out the destroy command for now: #8390.

Can't we return the new output name in the command response instead?

Hmm, we don't currently have a way for a command to return a message that isn't an error. We would have to expand the IPC protocol to allow that. It wouldn't be an i3 compat issue as i3 tools would never use a sway-specific command, but it does raise some questions such as how messages from multiple successful commands in a row are handled.

BTW, I think bumping the number in the name is actually a good thing - it prevent race conditions.

Hmm, what kind of race? I don't imagine there being a lot of things that would rapidly create and remove outputs, but even then, a configurable but uniqueness-checked name acts as a convenient semaphore for any scripting. E.g.,:

#!/bin/sh
swaymsg create_output HEADLESS-SHARE || exit 1
swaymsg output HEADLESS-SHARE mode 1280x720@30Hz
wl-mirror HEADLESS-SHARE
swaymsg destroy_output HEADLESS-SHARE

If we can't let user control the name one way or another (which I still think is preferable for UX), we'd need to sort out the return value problem above. If you have to subscribe or call get_outputs, then there's definitely race conditions, but more importantly it would just be really inconvenient. :/

Allow specifying a name to be set for the newly created output, which
avoids the need for traversing the output list to find out what was
created. This is particularly useful for reusing configuration.
@emersion
Copy link
Member

emersion commented Oct 15, 2024

Hmm, we don't currently have a way for a command to return a message that isn't an error. We would have to expand the IPC protocol to allow that. It wouldn't be an i3 compat issue as i3 tools would never use a sway-specific command, but it does raise some questions such as how messages from multiple successful commands in a row are handled.

I wonder if this should be a CREATE_OUTPUT IPC message then…?

Hmm, what kind of race?

A tool receives a notification that a new output has been created, then starts a client and passes the output name as argument. The client would run twice on the same output.

Or maybe the client is spawned right before the output is destroyed.

We've had many output-related races with docks. I'd expect similar stuff if people integrate create_output in scripts and such.

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.

2 participants