IPC: Accept multiple actions from stdin

[bbb651]

This needs some testing, io::stdin().lines() behaves a little weird with EOF, notably pressing Ctrl + D twice (apparently that's a unix thing, you need to hit it twice when not at the start of a line), you still need to hit it an additional time for some reason.. with echo -e "one\ntwo" | niri msg action it works as expected (by the way, it seems like NIRI_SOCKET isn't replaced correctly in the winit backend).

I made the serialization prefer keeping 1-length vectors as single values to maximize compatibility. The amount of tests for one_or_many is a bit excessive (I thought a visitor was needed at first, and then got the order of the enum variants wrong, so they were helpful), should I only keep the derive ones?

Also I'm not sure what should happen if you pass no actions, currently the cli prints a warning and does nothing but it's allowed in the ipc, should niri respond with an error instead?

Also, this currently splits on spaces, meaning it handles quoting and multiple spaces incorrectly, should this use e.g. shlex instead?

[YaLTeR]

Also, this currently splits on spaces, meaning it handles quoting and multiple spaces incorrectly, should this use e.g. shlex instead?

I understand the problem but I feel like more complex lexing is a worse trade-off. Especially when actions are generated programmatically from an existing shell, the last thing you want is some weird extra splitting.

[YaLTeR]

Couldn't we just add a separate Actions variant and avoid the whole custom parsing?

[bbb651]

Yeah that makes sense, it's simpler and also more ergonomic for niri-ipc. The "compactness" of the json representation doesn't really matter.

[bbb651]

I wish rust allowed somehow matching over both a T and Vec<T> as &[T], the code in the match arm ends up duplicated and it's awkward to break into a helper since it's async and environment capturing, it's just short enough that it's probably fine..?

        Request::Action(action) => {
            let (tx, rx) = async_channel::bounded(1);

            ctx.event_loop.insert_idle(move |state| {
                // Make sure some logic like workspace clean-up has a chance to run before doing
                // actions.
                state.niri.advance_animations();
                state.do_action(action.into(), false);
                let _ = tx.send_blocking(());
            });

            // Wait until the action has been processed before returning. This is important for a
            // few actions, for instance for DoScreenTransition this wait ensures that the screen
            // contents were sampled into the texture.
            let _ = rx.recv().await;
            Response::Handled
        }
        Request::Actions(actions) => {
            if actions.is_empty() {
                return Ok(Response::Handled);
            }

            let (tx, rx) = async_channel::bounded(1);

            ctx.event_loop.insert_idle(move |state| {
                state.niri.advance_animations();
                for action in actions.into_iter().map(niri_config::Action::from) {
                    state.do_action(action, false);
                }
                let _ = tx.send_blocking(());
            });

            let _ = rx.recv().await;
            Response::Handled
        }

[YaLTeR]

I guess you could try something like async fn do_actions(ctx, actions: impl Iterator<Item=Action>)?

[YaLTeR]

Similarly, could this remain as non-optional, with a separate niri msg actions that does the stdin reading?

[bbb651]

To me this feels a bit weird to me, because it is the same subcommand in terms of functionality, just with different input, unlike how niri msg outputs and niri msg output are different.
Most commands that takes a positional arguments fallback to stdin if it's missing, e.g. cat, jq, nvim, etc. so it feels logical to follow the same convention. Granted this is a bit different since it takes it normally takes a single literal action and not a file of actions (and a flag like -i - feels redundant since it's mutually exclusive with a literal action and the majority of the time you'd use stdin). It might be a bit confusing to have niri msg action not print the help message though...

[YaLTeR]

It might be a bit confusing to have niri msg action not print the help message though...

Yeah, I want that to keep printing help.

Maybe then something more descriptive like niri msg multiple-actions or niri msg actions-from-stdin or something?

[bbb651]

Yeah, I want that to keep printing help.

Another option is checking if it's a terminal and printing the help message only in that case, then the help message can show a echo -e '...' | niri msg action example

[YaLTeR]

Nah, I think an explicit separate subcommand is better. Also I guess if we pick multiple-actions or do-multiple-actions, then the IPC enum variant should also be renamed to MultipleActions for consistency.

[YaLTeR]

Or maybe ActionBatch?