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

IPC: Accept multiple actions from stdin #1017

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions niri-ipc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ use serde::{Deserialize, Serialize};
pub mod socket;
pub mod state;

mod utils;

/// Request from client to niri.
#[derive(Debug, Serialize, Deserialize, Clone)]
#[cfg_attr(feature = "json-schema", derive(schemars::JsonSchema))]
Expand All @@ -63,8 +65,8 @@ pub enum Request {
FocusedOutput,
/// Request information about the focused window.
FocusedWindow,
/// Perform an action.
Action(Action),
/// Perform actions.
Action(#[serde(with = "utils::one_or_many")] Vec<Action>),
Comment on lines +68 to +69
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
        }

Copy link
Owner

@YaLTeR YaLTeR Jan 30, 2025

Choose a reason for hiding this comment

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

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

/// Change output configuration temporarily.
///
/// The configuration is changed temporarily and not saved into the config file. If the output
Expand Down
118 changes: 118 additions & 0 deletions niri-ipc/src/utils.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
pub(crate) mod one_or_many {
use serde::{Deserialize, Deserializer, Serialize, Serializer};

pub(crate) fn serialize<T: Serialize, S: Serializer>(
value: &Vec<T>,
serializer: S,
) -> Result<S::Ok, S::Error> {
if value.len() == 1 {
value[0].serialize(serializer)
} else {
value.serialize(serializer)
}
}

pub(crate) fn deserialize<'de, T: Deserialize<'de>, D: Deserializer<'de>>(
deserializer: D,
) -> Result<Vec<T>, D::Error> {
#[derive(Deserialize)]
#[serde(untagged)]
enum OneOrMany<T> {
Many(Vec<T>),
One(T),
}

match OneOrMany::deserialize(deserializer)? {
OneOrMany::Many(v) => Ok(v),
OneOrMany::One(v) => Ok(vec![v]),
}
}

#[cfg(test)]
mod tests {
use std::fmt::Debug;

use serde_json::de::SliceRead;
use serde_json::{Deserializer, Serializer, Value};

use super::*;

fn test_serialize<T: Serialize>(value: &Vec<T>, expected: &str) {
let mut bytes = Vec::new();
let mut serializer = Serializer::new(&mut bytes);
serialize(value, &mut serializer).expect("failed to serialize");
assert_eq!(String::from_utf8_lossy(&bytes), expected);
}

fn test_deserialize<'de, T>(value: &'de str, expected: &Vec<T>)
where
T: Deserialize<'de> + Debug + PartialEq,
{
let mut deserailier = Deserializer::new(SliceRead::new(value.as_bytes()));
let result: Vec<T> = deserialize(&mut deserailier).expect("failed to deserialize");
assert_eq!(&result, expected);
}

#[test]
fn serialize_one() {
test_serialize(&vec![Value::Null], "null");
}

#[test]
fn deserialize_one() {
test_deserialize("null", &vec![Value::Null]);
}

#[test]
fn serialize_many() {
test_serialize(&vec![Value::Null, Value::Null], "[null,null]");
}

#[test]
fn deserialize_many() {
test_deserialize("[null,null]", &vec![Value::Null, Value::Null]);
}

#[test]
fn serialize_none() {
test_serialize(&Vec::<Value>::new(), "[]");
}

#[test]
fn deserialize_none() {
test_deserialize("[]", &Vec::<Value>::new());
}

#[test]
fn serialize_derive() {
#[derive(Debug, Serialize, PartialEq)]
enum Request {
Action(#[serde(with = "self")] Vec<String>),
}
let request = serde_json::to_string(&Request::Action(vec!["foo".to_string()]))
.expect("failed to serialize");
assert_eq!(request, r#"{"Action":"foo"}"#);
let request =
serde_json::to_string(&Request::Action(vec!["foo".to_string(), "bar".to_string()]))
.expect("failed to serialize");
assert_eq!(request, r#"{"Action":["foo","bar"]}"#);
}

#[test]
fn deserialize_derive() {
#[derive(Debug, Deserialize, PartialEq)]
enum Request {
Action(#[serde(with = "self")] Vec<String>),
}
let request: Request =
serde_json::from_str(r#"{"Action":"foo"}"#).expect("failed to deserialize");
assert_eq!(request, Request::Action(vec!["foo".to_string()]));
let request: Request =
serde_json::from_str(r#"{"Action":["foo","bar"]}"#).expect("failed to deserialize");
assert_eq!(
request,
Request::Action(vec!["foo".to_string(), "bar".to_string()])
);
}
}
}
6 changes: 5 additions & 1 deletion src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,13 @@ pub enum Msg {
FocusedWindow,
/// Perform an action.
Action {
// NOTE: This is only optional because of clap_derive limitations and will never be `None`.
// If an action is not provided it reads actions from stdin and turns to [`Msg::Actions`].
Comment on lines +77 to +78
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Or maybe ActionBatch?

#[command(subcommand)]
action: Action,
action: Option<Action>,
},
#[clap(skip)]
Actions { actions: Vec<Action> },
/// Change output configuration temporarily.
///
/// The configuration is changed temporarily and not saved into the config file. If the output
Expand Down
8 changes: 6 additions & 2 deletions src/ipc/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ pub fn handle_msg(msg: Msg, json: bool) -> anyhow::Result<()> {
Msg::Outputs => Request::Outputs,
Msg::FocusedWindow => Request::FocusedWindow,
Msg::FocusedOutput => Request::FocusedOutput,
Msg::Action { action } => Request::Action(action.clone()),
Msg::Action {
action: Some(action),
} => Request::Action(vec![action.clone()]),
Msg::Action { action: None } => unreachable!(),
Msg::Actions { actions } => Request::Action(actions.clone()),
Msg::Output { output, action } => Request::Output {
output: output.clone(),
action: action.clone(),
Expand Down Expand Up @@ -252,7 +256,7 @@ pub fn handle_msg(msg: Msg, json: bool) -> anyhow::Result<()> {
println!("No output is focused.");
}
}
Msg::Action { .. } => {
Msg::Action { .. } | Msg::Actions { .. } => {
let Response::Handled = response else {
bail!("unexpected response: expected Handled, got {response:?}");
};
Expand Down
7 changes: 4 additions & 3 deletions src/ipc/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,15 +309,16 @@ async fn process(ctx: &ClientCtx, request: Request) -> Reply {
let window = windows.values().find(|win| win.is_focused).cloned();
Response::FocusedWindow(window)
}
Request::Action(action) => {
Request::Action(actions) => {
let (tx, rx) = async_channel::bounded(1);

let action = niri_config::Action::from(action);
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, false);
for action in actions.into_iter().map(niri_config::Action::from) {
state.do_action(action, false);
}
let _ = tx.send_blocking(());
});

Expand Down
22 changes: 19 additions & 3 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ use std::io::{self, Write};
use std::os::fd::FromRawFd;
use std::path::PathBuf;
use std::process::Command;
use std::{env, mem};
use std::{env, iter, mem};

use clap::Parser;
use directories::ProjectDirs;
use niri::cli::{Cli, Sub};
use niri::cli::{Cli, Msg, Sub};
#[cfg(feature = "dbus")]
use niri::dbus;
use niri::ipc::client::handle_msg;
Expand All @@ -24,6 +24,7 @@ use niri::utils::watcher::Watcher;
use niri::utils::{cause_panic, version, IS_SYSTEMD_SERVICE};
use niri_config::Config;
use niri_ipc::socket::SOCKET_PATH_ENV;
use niri_ipc::Action;
use portable_atomic::Ordering;
use sd_notify::NotifyState;
use smithay::reexports::calloop::EventLoop;
Expand Down Expand Up @@ -104,7 +105,22 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
info!("config is valid");
return Ok(());
}
Sub::Msg { msg, json } => {
Sub::Msg { mut msg, json } => {
if let Msg::Action { action: None } = msg {
let actions = io::stdin()
.lines()
.map(|line| {
line.map(|line| {
Action::parse_from(iter::once("action").chain(line.split(' ')))
})
})
.collect::<Result<Vec<_>, _>>()?;
if actions.is_empty() {
warn!("read actions from stdin but no actions were provided");
return Ok(());
}
msg = Msg::Actions { actions }
}
handle_msg(msg, json)?;
return Ok(());
}
Expand Down