From f424e8e0ad67119edfb1f5e59c08d5d6951f10c0 Mon Sep 17 00:00:00 2001 From: Arne Beer Date: Wed, 27 Nov 2024 11:51:46 +0100 Subject: [PATCH] change(edit): New way to edit tasks This change introduces a completely new way to edit tasks and allows to edit multiple tasks in one go. This is effectively a breaking change in all regards, since the API and the CLI changed due to this. --- CHANGELOG.md | 22 ++ pueue/src/client/cli.rs | 42 +-- pueue/src/client/client.rs | 25 +- pueue/src/client/commands/edit.rs | 292 ++++++++++-------- pueue/src/client/commands/restart.rs | 69 +++-- pueue/src/daemon/callbacks.rs | 2 + .../daemon/network/message_handler/edit.rs | 129 ++++---- .../src/daemon/network/message_handler/mod.rs | 6 +- .../daemon/network/message_handler/restart.rs | 32 +- .../daemon/network/message_handler/stash.rs | 4 +- pueue/tests/client/integration/edit.rs | 34 +- pueue/tests/client/integration/restart.rs | 52 ++-- pueue/tests/daemon/integration/aliases.rs | 9 +- pueue/tests/daemon/integration/edit.rs | 33 +- pueue/tests/daemon/integration/restart.rs | 20 +- pueue_lib/src/network/message.rs | 57 ++-- 16 files changed, 411 insertions(+), 417 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43c3472a..03e14123 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,27 @@ The new state design resolves this issue by allowing Pueue to manipulate subproc TLDR: Commands that start/stop/kill/pause tasks now only return when the task is actually started/stopped/killed/paused. +### New editing + +Task editing was a bit tedious until recently. +One could only edit a single task at a time and you had to specify which properties you wanted to add. +Each property was then opened in a new `$EDITOR` session, which meant that users had to open and close editors up to four times to edit a single task. + +After a lot of consideration, a new way of editing tasks has been designed that allows simple and convenient editing of multiple tasks at once. +For this, a temporary directory is created for every task to edit and a new file for every property, resulting in the following structure: + +``` + 📁 0/ + │ * command + │ * label + │ * path + └ * priority +``` + +You can then just navigate the resulting file structure and edit the properties you want in the editor of your choice. + +I'm aware that this might not be for everyone, so feedback is very much encouraged over [here](https://github.com/Nukesor/pueue/issues/553). + ### Runtime invariants Previously, various task-state related invariants were enforced during runtime. For example, a `Queued` task should not have a `start` or `enqueued_at` time set. @@ -46,6 +67,7 @@ TLDR: The new task state representation is more verbose but significantly cleane - Send log output to `stderr` instead of `stdout` [#562](https://github.com/Nukesor/pueue/issues/562). - Change default log level from error to warning [#562](https://github.com/Nukesor/pueue/issues/562). - Bumped MSRV to 1.70. +- **Breaking**: Redesigned task editing process [#553](https://github.com/Nukesor/pueue/issues/553). ### Add diff --git a/pueue/src/client/cli.rs b/pueue/src/client/cli.rs index df2d2096..d530352b 100644 --- a/pueue/src/client/cli.rs +++ b/pueue/src/client/cli.rs @@ -214,21 +214,9 @@ pub enum SubCommand { #[arg(long)] not_in_place: bool, - /// Edit the tasks' commands before restarting. + /// Edit the task before restarting. #[arg(short, long)] edit: bool, - - /// Edit the tasks' paths before restarting. - #[arg(short = 'p', long)] - edit_path: bool, - - /// Edit the tasks' labels before restarting. - #[arg(short = 'l', long)] - edit_label: bool, - - /// Edit the tasks' priorities before restarting. - #[arg(short = 'o', long)] - edit_priority: bool, }, #[command(about = "Either pause running tasks or specific groups of tasks.\n\ @@ -282,30 +270,12 @@ pub enum SubCommand { input: String, }, - #[command( - about = "Edit the command, path, label, or priority of a stashed or queued task.\n\ - By default only the command is edited.\n\ - Multiple properties can be added in one go." - )] + #[command(about = "Adjust editable properties of a task.\n\ + A temporary folder folder will be opened by your $EDITOR, which contains \n\ + a file for each editable property.")] Edit { - /// The task's id. - task_id: usize, - - /// Edit the task's command. - #[arg(short, long)] - command: bool, - - /// Edit the task's path. - #[arg(short, long)] - path: bool, - - /// Edit the task's label. - #[arg(short, long)] - label: bool, - - /// Edit the task's priority. - #[arg(short = 'o', long)] - priority: bool, + /// The ids of all tasks that should be edited. + task_ids: Vec, }, #[command(about = "Use this to add or remove groups.\n\ diff --git a/pueue/src/client/client.rs b/pueue/src/client/client.rs index d67b8c11..febdf2e4 100644 --- a/pueue/src/client/client.rs +++ b/pueue/src/client/client.rs @@ -203,23 +203,8 @@ impl Client { Ok(false) } - SubCommand::Edit { - task_id, - command, - path, - label, - priority, - } => { - let message = edit( - &mut self.stream, - &self.settings, - *task_id, - *command, - *path, - *label, - *priority, - ) - .await?; + SubCommand::Edit { task_ids } => { + let message = edit(&mut self.stream, &self.settings, task_ids).await?; self.handle_response(message)?; Ok(true) } @@ -243,9 +228,6 @@ impl Client { in_place, not_in_place, edit, - edit_path, - edit_label, - edit_priority, } => { // `not_in_place` superseeds both other configs let in_place = @@ -260,9 +242,6 @@ impl Client { *stashed, in_place, *edit, - *edit_path, - *edit_label, - *edit_priority, ) .await?; Ok(true) diff --git a/pueue/src/client/commands/edit.rs b/pueue/src/client/commands/edit.rs index dd88aa72..6d4cc9d3 100644 --- a/pueue/src/client/commands/edit.rs +++ b/pueue/src/client/commands/edit.rs @@ -1,10 +1,12 @@ use std::env; -use std::io::{Read, Seek, Write}; +use std::fs::{create_dir, read_to_string, File}; +use std::io::Write; use std::path::{Path, PathBuf}; use anyhow::{bail, Context, Result}; +use pueue_lib::error::Error; use pueue_lib::settings::Settings; -use tempfile::NamedTempFile; +use tempfile::tempdir; use pueue_lib::network::message::*; use pueue_lib::network::protocol::*; @@ -20,152 +22,75 @@ use pueue_lib::process_helper::compile_shell_command; pub async fn edit( stream: &mut GenericStream, settings: &Settings, - task_id: usize, - edit_command: bool, - edit_path: bool, - edit_label: bool, - edit_priority: bool, + task_ids: &[usize], ) -> Result { // Request the data to edit from the server and issue a task-lock while doing so. - let init_message = Message::EditRequest(task_id); + let init_message = Message::EditRequest(task_ids.to_vec()); send_message(init_message, stream).await?; let init_response = receive_message(stream).await?; - // In case we don't receive an EditResponse, something went wrong + // In case we don't receive an EditResponse, something went wrong. // Return the response to the parent function and let the client handle it // by the generic message handler. - let Message::EditResponse(init_response) = init_response else { + let Message::EditResponse(mut editable_tasks) = init_response else { return Ok(init_response); }; - // Edit the command if explicitly specified or if no flags are provided (the default) - let edit_command = edit_command || !edit_path && !edit_label && !edit_priority; - - // Edit all requested properties. - let edit_result = edit_task_properties( - settings, - &init_response.command, - &init_response.path, - &init_response.label, - init_response.priority, - edit_command, - edit_path, - edit_label, - edit_priority, - ); + let edit_result = edit_tasks(settings, &mut editable_tasks); // Any error while editing will result in the client aborting the editing process. // However, as the daemon moves tasks that're edited into the `Locked` state, we cannot simply // exit the client. We rather have to notify the daemon that the editing process was interrupted. - // In the following, we notify the daemon of any errors, so it can restore the task to its previous state. - let edited_props = match edit_result { - Ok(inner) => inner, - Err(error) => { - eprintln!("Encountered an error while editing. Trying to restore the task's status."); - // Notify the daemon that something went wrong. - let edit_message = Message::EditRestore(task_id); - send_message(edit_message, stream).await?; - let response = receive_message(stream).await?; - match response { - Message::Failure(message) | Message::Success(message) => { - eprintln!("{message}"); - } - _ => eprintln!("Received unknown response: {response:?}"), - }; - - return Err(error); - } - }; + // In the following, we notify the daemon of any errors, so it can restore the tasks to + // their previous state. + if let Err(error) = edit_result { + eprintln!("Encountered an error while editing. Trying to restore the task's status."); + // Notify the daemon that something went wrong. + let task_ids = editable_tasks.iter().map(|task| task.id).collect(); + let edit_message = Message::EditRestore(task_ids); + send_message(edit_message, stream).await?; + + let response = receive_message(stream).await?; + match response { + Message::Failure(message) | Message::Success(message) => { + eprintln!("{message}"); + } + _ => eprintln!("Received unknown response: {response:?}"), + }; + + return Err(error); + } // Create a new message with the edited properties. - let edit_message = EditMessage { - task_id, - command: edited_props.command, - path: edited_props.path, - label: edited_props.label, - delete_label: edited_props.delete_label, - priority: edited_props.priority, - }; - send_message(edit_message, stream).await?; + send_message(Message::Edit(editable_tasks), stream).await?; Ok(receive_message(stream).await?) } -#[derive(Default)] -pub struct EditedProperties { - pub command: Option, - pub path: Option, - pub label: Option, - pub delete_label: bool, - pub priority: Option, -} - -/// Takes several task properties and edit them if requested. -/// The `edit_*` booleans are used to determine which fields should be edited. -/// -/// Fields that have been edited will be returned as their `Some(T)` equivalent. -/// -/// The returned values are: `(command, path, label)` -#[allow(clippy::too_many_arguments)] -pub fn edit_task_properties( - settings: &Settings, - original_command: &str, - original_path: &Path, - original_label: &Option, - original_priority: i32, - edit_command: bool, - edit_path: bool, - edit_label: bool, - edit_priority: bool, -) -> Result { - let mut props = EditedProperties::default(); - - // Update the command if requested. - if edit_command { - props.command = Some(edit_line(settings, original_command)?); - }; +pub fn edit_tasks(settings: &Settings, editable_tasks: &mut [EditableTask]) -> Result<()> { + // Create the temporary directory that'll be used for all edits. + let temp_dir = tempdir().context("Failed to create temporary directory for edtiting.")?; + let temp_dir_path = temp_dir.path(); - // Update the path if requested. - if edit_path { - let str_path = original_path - .to_str() - .context("Failed to convert task path to string")?; - let changed_path = edit_line(settings, str_path)?; - props.path = Some(PathBuf::from(changed_path)); + for task in editable_tasks.iter() { + task.create_temp_dir(temp_dir_path)? } - // Update the label if requested. - if edit_label { - let edited_label = edit_line(settings, &original_label.clone().unwrap_or_default())?; + run_editor(settings, temp_dir_path)?; - // If the user deletes the label in their editor, an empty string will be returned. - // This is an indicator that the task should no longer have a label, in which case we - // set the `delete_label` flag. - if edited_label.is_empty() { - props.delete_label = true; - } else { - props.label = Some(edited_label); - }; + // Read the data back from disk into the struct. + for task in editable_tasks.iter_mut() { + task.read_temp_dir(temp_dir_path)? } - // Update the priority if requested. - if edit_priority { - props.priority = Some(edit_line(settings, &original_priority.to_string())?.parse()?); - }; - - Ok(props) + Ok(()) } -/// This function enables the user to edit a task's details. -/// Save any string to a temporary file, which is opened in the specified `$EDITOR`. -/// As soon as the editor is closed, read the file content and return the line. -fn edit_line(settings: &Settings, line: &str) -> Result { - // Create a temporary file with the command so we can edit it with the editor. - let mut file = NamedTempFile::new().expect("Failed to create a temporary file"); - writeln!(file, "{line}").context("Failed to write to temporary file.")?; - - // Get the editor that should be used from the environment. +/// Open the folder that contains all files for editing in the user's `$EDITOR`. +/// Returns as soon as the editor is closed again. +/// Get the editor that should be used from the environment. +pub fn run_editor(settings: &Settings, temp_dir: &Path) -> Result<()> { let editor = match env::var("EDITOR") { Err(_) => bail!("The '$EDITOR' environment variable couldn't be read. Aborting."), Ok(editor) => editor, @@ -173,9 +98,14 @@ fn edit_line(settings: &Settings, line: &str) -> Result { // Compile the command to start the editor on the temporary file. // We escape the file path for good measure, but it shouldn't be necessary. - let path = shell_escape::escape(file.path().to_string_lossy()); + let path = shell_escape::escape(temp_dir.to_string_lossy()); let editor_command = format!("{editor} {path}"); - let status = compile_shell_command(settings, &editor_command) + let mut modified_settings = settings.clone(); + modified_settings.daemon.env_vars.insert( + "PUEUE_EDIT_PATH".to_string(), + temp_dir.to_string_lossy().to_string(), + ); + let status = compile_shell_command(&modified_settings, &editor_command) .status() .context("Editor command did somehow fail. Aborting.")?; @@ -183,19 +113,117 @@ fn edit_line(settings: &Settings, line: &str) -> Result { bail!("The editor exited with a non-zero code. Aborting."); } - // Read the file. - let mut file = file.into_file(); - file.rewind() - .context("Couldn't seek to start of file. Aborting.")?; + Ok(()) +} + +/// Implements convenience functions to serialize and deserialize editable tasks to and from disk +/// so users can edit the task via their editor. +trait Editable { + fn create_temp_dir(&self, temp_dir: &Path) -> Result<()>; + fn read_temp_dir(&mut self, temp_dir: &Path) -> Result<()>; +} + +impl Editable for EditableTask { + /// Create a folder for this task that contains one file for each editable property. + fn create_temp_dir(&self, temp_dir: &Path) -> Result<()> { + let task_dir = temp_dir.join(self.id.to_string()); + create_dir(&task_dir) + .map_err(|err| Error::IoPathError(task_dir.clone(), "creating task dir", err))?; + + // Create command file + let cmd_path = task_dir.join("command"); + let mut output = File::create(&cmd_path) + .map_err(|err| Error::IoPathError(cmd_path.clone(), "creating command file", err))?; + write!(output, "{}", self.command) + .map_err(|err| Error::IoPathError(cmd_path.clone(), "writing command file", err))?; + + // Create cwd file + let cwd_path = task_dir.join("path"); + let mut output = File::create(&cwd_path).map_err(|err| { + Error::IoPathError(cwd_path.clone(), "creating temporary path file", err) + })?; + write!(output, "{}", self.path.to_string_lossy()) + .map_err(|err| Error::IoPathError(cwd_path.clone(), "writing path file", err))?; + + // Create label file. If there's no label, create an empty file. + let label_path = task_dir.join("label"); + let mut output = File::create(&label_path).map_err(|err| { + Error::IoPathError(label_path.clone(), "creating temporary label file", err) + })?; + if let Some(label) = &self.label { + write!(output, "{}", label) + .map_err(|err| Error::IoPathError(label_path.clone(), "writing label file", err))?; + } - let mut line = String::new(); - file.read_to_string(&mut line) - .context("Failed to read Command after editing")?; + // Create priority file. If there's no priority, create an empty file. + let priority_path = task_dir.join("priority"); + let mut output = File::create(&priority_path).map_err(|err| { + Error::IoPathError(priority_path.clone(), "creating priority file", err) + })?; + write!(output, "{}", self.priority).map_err(|err| { + Error::IoPathError(priority_path.clone(), "writing priority file", err) + })?; - // Remove any trailing newlines from the command. - while line.ends_with('\n') || line.ends_with('\r') { - line.pop(); + Ok(()) } - Ok(line.trim().to_string()) + /// Read the edited files of this task's temporary folder back into this struct. + /// + /// The user has finished editing at this point in time. + fn read_temp_dir(&mut self, temp_dir: &Path) -> Result<()> { + let task_dir = temp_dir.join(self.id.to_string()); + + // Create command file + let cmd_path = task_dir.join("command"); + let command = read_to_string(&cmd_path) + .map_err(|err| Error::IoPathError(cmd_path.clone(), "reading command file", err))?; + // Make sure the command isn't empty. + if command.trim().is_empty() { + bail!("Found empty command after edit for task {}", self.id); + } + self.command = command.trim().to_string(); + + // Create cwd file + let cwd_path = task_dir.join("path"); + let cwd = read_to_string(&cwd_path) + .map_err(|err| Error::IoPathError(cwd_path.clone(), "reading path file", err))?; + let cwd = cwd.trim(); + // Make sure the path isn't empty + if cwd.trim().is_empty() { + bail!("Found empty path after edit for task {}", self.id); + } + let path = PathBuf::from(&cwd); + // Make sure the path actually exists + if !self.path.exists() { + bail!( + "Found non-existing path '{:?}' after edit for task {}", + self.path, + self.id + ); + } + self.path = path; + + // Create label file. If file is empty, set the label to `None` + let label_path = task_dir.join("label"); + let label = read_to_string(&label_path) + .map_err(|err| Error::IoPathError(label_path.clone(), "reading label file", err))?; + self.label = if label.trim().is_empty() { + None + } else { + Some(label.trim().to_string()) + }; + + // Create priority file. If file is empty, set the priority to `None` + let priority_path = task_dir.join("priority"); + let priority = read_to_string(&priority_path).map_err(|err| { + Error::IoPathError(priority_path.clone(), "reading priority file", err) + })?; + // Parse the user input into a usize. + self.priority = priority.trim().parse().context(format!( + "Failed to parse priority string '{}' into an integer for task {}", + priority, self.id + ))?; + + Ok(()) + } } diff --git a/pueue/src/client/commands/restart.rs b/pueue/src/client/commands/restart.rs index ad9c28f1..fab3bca4 100644 --- a/pueue/src/client/commands/restart.rs +++ b/pueue/src/client/commands/restart.rs @@ -7,9 +7,10 @@ use pueue_lib::settings::Settings; use pueue_lib::state::FilteredTasks; use pueue_lib::task::{Task, TaskResult, TaskStatus}; -use crate::client::commands::edit::edit_task_properties; use crate::client::commands::get_state; +use super::edit::edit_tasks; + /// When restarting tasks, the remote state is queried and a [AddMessage] /// is create from the existing task in the state. /// @@ -25,10 +26,7 @@ pub async fn restart( start_immediately: bool, stashed: bool, in_place: bool, - edit_command: bool, - edit_path: bool, - edit_label: bool, - edit_priority: bool, + edit: bool, ) -> Result<()> { let new_status = if stashed { TaskStatus::Stashed { enqueue_at: None } @@ -89,35 +87,40 @@ pub async fn restart( start_immediately, }; - // Go through all Done commands we found and restart them - for task_id in &filtered_tasks.matching_ids { - let task = state.tasks.get(task_id).unwrap(); - let mut new_task = Task::from_task(task); - new_task.status = new_status.clone(); - - // Edit any properties, if requested. - let edited_props = edit_task_properties( - settings, - &task.command, - &task.path, - &task.label, - task.priority, - edit_command, - edit_path, - edit_label, - edit_priority, - )?; + // Get all tasks that should be restarted. + let mut tasks: Vec = filtered_tasks + .matching_ids + .iter() + .map(|task_id| Task::from_task(state.tasks.get(task_id).unwrap())) + .collect(); + + // If the tasks should be edited, edit them in one go. + if edit { + let mut editable_tasks: Vec = tasks.iter().map(EditableTask::from).collect(); + edit_tasks(settings, &mut editable_tasks)?; + + // Now merge the edited properties back into the tasks. + // We simply zip the task and editable task vectors, as we know that they have the same + // order. + tasks + .iter_mut() + .zip(editable_tasks.into_iter()) + .for_each(|(task, edited)| edited.into_task(task)); + } + + // Go through all restartable commands we found and process them. + for mut task in tasks { + task.status = new_status.clone(); // Add the tasks to the singular message, if we want to restart the tasks in-place. // And continue with the next task. The message will then be sent after the for loop. if in_place { restart_message.tasks.push(TaskToRestart { - task_id: *task_id, - command: edited_props.command, - path: edited_props.path, - label: edited_props.label, - delete_label: edited_props.delete_label, - priority: edited_props.priority, + task_id: task.id, + command: task.command, + path: task.path, + label: task.label, + priority: task.priority, }); continue; @@ -126,16 +129,16 @@ pub async fn restart( // In case we don't do in-place restarts, we have to add a new task. // Create a AddMessage to send the task to the daemon from the updated info and the old task. let add_task_message = AddMessage { - command: edited_props.command.unwrap_or_else(|| task.command.clone()), - path: edited_props.path.unwrap_or_else(|| task.path.clone()), + command: task.command, + path: task.path, envs: task.envs.clone(), start_immediately, stashed, group: task.group.clone(), enqueue_at: None, dependencies: Vec::new(), - priority: edited_props.priority.or(Some(task.priority)), - label: edited_props.label.or_else(|| task.label.clone()), + priority: Some(task.priority), + label: task.label, print_task_id: false, }; diff --git a/pueue/src/daemon/callbacks.rs b/pueue/src/daemon/callbacks.rs index baa08fc0..0e05513f 100644 --- a/pueue/src/daemon/callbacks.rs +++ b/pueue/src/daemon/callbacks.rs @@ -132,6 +132,8 @@ pub fn check_callbacks(state: &mut LockedState) { finished.reverse(); for id in finished.iter() { + // Explicitly allow this lint since we did a try_wait above and know that it finished. + #[allow(clippy::zombie_processes)] state.callbacks.remove(*id); } } diff --git a/pueue/src/daemon/network/message_handler/edit.rs b/pueue/src/daemon/network/message_handler/edit.rs index 6f160acc..44e19785 100644 --- a/pueue/src/daemon/network/message_handler/edit.rs +++ b/pueue/src/daemon/network/message_handler/edit.rs @@ -11,91 +11,96 @@ use crate::ok_or_save_state_failure; /// Invoked when calling `pueue edit`. /// If a user wants to edit a message, we need to send him the current command. /// Lock the task to prevent execution, before the user has finished editing the command. -pub fn edit_request(state: &SharedState, task_id: usize) -> Message { +pub fn edit_request(state: &SharedState, task_ids: Vec) -> Message { // Check whether the task exists and is queued/stashed. Abort if that's not the case. let mut state = state.lock().unwrap(); - match state.tasks.get_mut(&task_id) { - Some(task) => { - if !task.is_queued() && !task.is_stashed() { - return create_failure_message("You can only edit a queued/stashed task"); - } - task.status = TaskStatus::Locked { - previous_status: Box::new(task.status.clone()), - }; + let mut editable_tasks: Vec = Vec::new(); + for task_id in task_ids { + match state.tasks.get_mut(&task_id) { + Some(task) => { + if !task.is_queued() && !task.is_stashed() { + return create_failure_message("You can only edit a queued/stashed task"); + } + task.status = TaskStatus::Locked { + previous_status: Box::new(task.status.clone()), + }; - EditResponseMessage { - task_id: task.id, - command: task.original_command.clone(), - path: task.path.clone(), - label: task.label.clone(), - priority: task.priority, + editable_tasks.push(EditableTask::from(&*task)); } - .into() + None => return create_failure_message("No task with this id."), } - None => create_failure_message("No task with this id."), } + + Message::EditResponse(editable_tasks) } /// Invoked after closing the editor on `pueue edit`. /// Now we actually update the message with the updated command from the client. -pub fn edit(settings: &Settings, state: &SharedState, message: EditMessage) -> Message { +pub fn edit( + settings: &Settings, + state: &SharedState, + editable_tasks: Vec, +) -> Message { // Check whether the task exists and is locked. Abort if that's not the case. let mut state = state.lock().unwrap(); - match state.tasks.get_mut(&message.task_id) { - Some(task) => { - let TaskStatus::Locked { previous_status } = &task.status else { - return create_failure_message("Task is no longer locked."); - }; + for editable_task in editable_tasks { + match state.tasks.get_mut(&editable_task.id) { + Some(task) => { + let TaskStatus::Locked { previous_status } = &task.status else { + return create_failure_message(format!( + "Task {} is no longer locked.", + editable_task.id + )); + }; - // Restore the task to its previous state. - task.status = *previous_status.clone(); - - // Update command if applicable. - if let Some(command) = message.command { - task.original_command = command.clone(); - task.command = insert_alias(settings, command); - } - // Update path if applicable. - if let Some(path) = message.path { - task.path = path; - } - // Update label if applicable. - if message.label.is_some() { - task.label = message.label; - } else if message.delete_label { - task.label = None; - } - // Update priority if applicable. - if let Some(priority) = message.priority { - task.priority = priority; - } + // Restore the task to its previous state. + task.status = *previous_status.clone(); - ok_or_save_state_failure!(save_state(&state, settings)); + // Update all properties to the edited values. + task.original_command = editable_task.command.clone(); + task.command = insert_alias(settings, editable_task.command); + task.path = editable_task.path; + task.label = editable_task.label; + task.priority = editable_task.priority; - create_success_message("Command has been updated") + ok_or_save_state_failure!(save_state(&state, settings)); + } + None => return failure_msg!("Task to edit has gone away: {}", editable_task.id), } - None => failure_msg!("Task to edit has gone away: {}", message.task_id), } + + create_success_message("All tasks have been updated") } /// Invoked if a client fails to edit a task and asks the daemon to restore the task's status. -pub fn edit_restore(state: &SharedState, task_id: usize) -> Message { +pub fn edit_restore(state: &SharedState, task_ids: Vec) -> Message { // Check whether the task exists and is queued/stashed. Abort if that's not the case. let mut state = state.lock().unwrap(); - match state.tasks.get_mut(&task_id) { - Some(task) => { - let TaskStatus::Locked { previous_status } = &task.status else { - return create_failure_message("The requested task isn't locked"); - }; + let mut failed_tasks = Vec::new(); + for task_id in &task_ids { + match state.tasks.get_mut(task_id) { + Some(task) => { + let TaskStatus::Locked { previous_status } = &task.status else { + failed_tasks.push(format!("Task {} isn't locked! Cannot be unlocked", task_id)); + continue; + }; - // Restore the task to its previous state. - task.status = *previous_status.clone(); - - success_msg!( - "The requested task's status has been restored to '{}'", - task.status - ) + // Restore the task to its previous state. + task.status = *previous_status.clone(); + } + None => failed_tasks.push(format!("No task with id {}! Cannot be unlocked.", task_id)), } - None => create_failure_message("No task with this id."), } + + // Return an error if any tasks couldn't be restored. + if !failed_tasks.is_empty() { + let mut error_msg = String::from("Some tasks couldn't be unlocked:\n"); + error_msg.push_str(&failed_tasks.join("\n")); + return create_failure_message(error_msg); + } + + success_msg!( + "The requested task ids have been restored their previous state: {:?}", + task_ids + ) } diff --git a/pueue/src/daemon/network/message_handler/mod.rs b/pueue/src/daemon/network/message_handler/mod.rs index adf4626b..24d29c67 100644 --- a/pueue/src/daemon/network/message_handler/mod.rs +++ b/pueue/src/daemon/network/message_handler/mod.rs @@ -31,9 +31,9 @@ pub fn handle_message(message: Message, state: &SharedState, settings: &Settings match message { Message::Add(message) => add::add_task(settings, state, message), Message::Clean(message) => clean::clean(settings, state, message), - Message::Edit(message) => edit::edit(settings, state, message), - Message::EditRequest(task_id) => edit::edit_request(state, task_id), - Message::EditRestore(task_id) => edit::edit_restore(state, task_id), + Message::Edit(editable_tasks) => edit::edit(settings, state, editable_tasks), + Message::EditRequest(task_ids) => edit::edit_request(state, task_ids), + Message::EditRestore(task_ids) => edit::edit_restore(state, task_ids), Message::Enqueue(message) => enqueue::enqueue(settings, state, message), Message::Group(message) => group::group(settings, state, message), Message::Kill(message) => kill::kill(settings, state, message), diff --git a/pueue/src/daemon/network/message_handler/restart.rs b/pueue/src/daemon/network/message_handler/restart.rs index 2d337dab..51eefef1 100644 --- a/pueue/src/daemon/network/message_handler/restart.rs +++ b/pueue/src/daemon/network/message_handler/restart.rs @@ -33,7 +33,7 @@ pub fn restart_multiple( ); // Restart a tasks in-place - for task in message.tasks.iter() { + for task in message.tasks { restart(&mut state, task, message.stashed, settings); } @@ -52,7 +52,7 @@ pub fn restart_multiple( /// new task, which is completely handled on the client-side. fn restart( state: &mut MutexGuard, - to_restart: &TaskToRestart, + to_restart: TaskToRestart, stashed: bool, settings: &Settings, ) { @@ -75,26 +75,10 @@ fn restart( }; }; - // Update command if applicable. - if let Some(new_command) = to_restart.command.clone() { - task.original_command = new_command.clone(); - task.command = insert_alias(settings, new_command); - } - - // Update path if applicable. - if let Some(path) = to_restart.path.clone() { - task.path = path; - } - - // Update path if applicable. - if to_restart.label.is_some() { - task.label = to_restart.label.clone(); - } else if to_restart.delete_label { - task.label = None - } - - // Update priority if applicable. - if let Some(priority) = to_restart.priority { - task.priority = priority; - } + // Update task properties in case they've been edited. + task.original_command = to_restart.command.clone(); + task.command = insert_alias(settings, to_restart.command); + task.path = to_restart.path; + task.label = to_restart.label.clone(); + task.priority = to_restart.priority; } diff --git a/pueue/src/daemon/network/message_handler/stash.rs b/pueue/src/daemon/network/message_handler/stash.rs index dcc3356e..461e3d04 100644 --- a/pueue/src/daemon/network/message_handler/stash.rs +++ b/pueue/src/daemon/network/message_handler/stash.rs @@ -102,10 +102,10 @@ pub fn stash(settings: &Settings, state: &SharedState, message: StashMessage) -> &state, ), TaskSelection::Group(group) => { - success_msg!("All stashed tasks of group \"{group}\" have been enqueued.") + success_msg!("All queued tasks of group \"{group}\" have been stashd.") } TaskSelection::All => { - success_msg!("All stashed tasks have been enqueued.") + success_msg!("All queued tasks have been stashed.") } } } diff --git a/pueue/tests/client/integration/edit.rs b/pueue/tests/client/integration/edit.rs index 76e98daf..875da42f 100644 --- a/pueue/tests/client/integration/edit.rs +++ b/pueue/tests/client/integration/edit.rs @@ -20,7 +20,10 @@ async fn edit_task_default() -> Result<()> { // Update the task's command by piping a string to the temporary file. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo 'expected command string' > "); + envs.insert( + "EDITOR", + "echo 'expected command string' > ${PUEUE_EDIT_PATH}/0/command ||", + ); run_client_command_with_env(shared, &["edit", "0"], envs)?; // Make sure that both the command has been updated. @@ -51,19 +54,22 @@ async fn edit_all_task_properties() -> Result<()> { // Update all task properties by piping a string to the respective temporary file. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo 'expected string' > "); - run_client_command_with_env( - shared, - &["edit", "--command", "--path", "--label", "0"], - envs, - )?; + envs.insert( + "EDITOR", + "echo 'command' > ${PUEUE_EDIT_PATH}/0/command && \ +echo '/tmp' > ${PUEUE_EDIT_PATH}/0/path && \ +echo 'label' > ${PUEUE_EDIT_PATH}/0/label && \ +echo '5' > ${PUEUE_EDIT_PATH}/0/priority || ", + ); + run_client_command_with_env(shared, &["edit", "0"], envs)?; // Make sure that all properties have been updated. let state = get_state(shared).await?; let task = state.tasks.get(&0).unwrap(); - assert_eq!(task.command, "expected string"); - assert_eq!(task.path.to_string_lossy(), "expected string"); - assert_eq!(task.label, Some("expected string".to_string())); + assert_eq!(task.command, "command"); + assert_eq!(task.path.to_string_lossy(), "/tmp"); + assert_eq!(task.label, Some("label".to_string())); + assert_eq!(task.priority, 5); Ok(()) } @@ -84,8 +90,8 @@ async fn edit_delete_label() -> Result<()> { // Echo an empty string into the file. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo '' > "); - run_client_command_with_env(shared, &["edit", "--label", "0"], envs)?; + envs.insert("EDITOR", "echo '' > ${PUEUE_EDIT_PATH}/0/label ||"); + run_client_command_with_env(shared, &["edit", "0"], envs)?; // Make sure that the label has indeed be deleted let state = get_state(shared).await?; @@ -111,8 +117,8 @@ async fn edit_change_priority() -> Result<()> { // Echo a new priority into the file. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo '99' > "); - run_client_command_with_env(shared, &["edit", "--priority", "0"], envs)?; + envs.insert("EDITOR", "echo '99' > ${PUEUE_EDIT_PATH}/0/priority ||"); + run_client_command_with_env(shared, &["edit", "0"], envs)?; // Make sure that the priority has indeed been updated. let state = get_state(shared).await?; diff --git a/pueue/tests/client/integration/restart.rs b/pueue/tests/client/integration/restart.rs index e60f6928..0454d014 100644 --- a/pueue/tests/client/integration/restart.rs +++ b/pueue/tests/client/integration/restart.rs @@ -19,7 +19,10 @@ async fn restart_and_edit_task_command() -> Result<()> { // Set the editor to a command which replaces the temporary file's content. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo 'sleep 60' > "); + envs.insert( + "EDITOR", + "echo 'sleep 60' > ${PUEUE_EDIT_PATH}/0/command ||", + ); // Restart the command, edit its command and wait for it to start. run_client_command_with_env(shared, &["restart", "--in-place", "--edit", "0"], envs)?; @@ -50,10 +53,10 @@ async fn restart_and_edit_task_path() -> Result<()> { // Set the editor to a command which replaces the temporary file's content. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo '/tmp' > "); + envs.insert("EDITOR", "echo '/tmp' > ${PUEUE_EDIT_PATH}/0/path ||"); // Restart the command, edit its command and wait for it to start. - run_client_command_with_env(shared, &["restart", "--in-place", "--edit-path", "0"], envs)?; + run_client_command_with_env(shared, &["restart", "--in-place", "--edit", "0"], envs)?; wait_for_task_condition(shared, 0, |task| task.is_done()).await?; // Make sure that both the path has been updated. @@ -78,36 +81,32 @@ async fn restart_and_edit_task_path_and_command() -> Result<()> { // Set the editor to a command which replaces the temporary file's content. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo 'replaced string' > "); + envs.insert( + "EDITOR", + "echo 'doesnotexist' > ${PUEUE_EDIT_PATH}/0/command && \ +echo '/tmp' > ${PUEUE_EDIT_PATH}/0/path && \ +echo 'label' > ${PUEUE_EDIT_PATH}/0/label && \ +echo '5' > ${PUEUE_EDIT_PATH}/0/priority || ", + ); // Restart the command, edit its command and path and wait for it to start. // The task will fail afterwards, but it should still be edited. - run_client_command_with_env( - shared, - &[ - "restart", - "--in-place", - "--edit", - "--edit-path", - "--edit-label", - "0", - ], - envs, - )?; + run_client_command_with_env(shared, &["restart", "--in-place", "--edit", "0"], envs)?; wait_for_task_condition(shared, 0, |task| task.is_done()).await?; // Make sure that both the path has been updated. let state = get_state(shared).await?; let task = state.tasks.get(&0).unwrap(); - assert_eq!(task.command, "replaced string"); - assert_eq!(task.path.to_string_lossy(), "replaced string"); - assert_eq!(task.label, Some("replaced string".to_owned())); + assert_eq!(task.command, "doesnotexist"); + assert_eq!(task.path.to_string_lossy(), "/tmp"); + assert_eq!(task.label, Some("label".to_string())); + assert_eq!(task.priority, 5); // Also the task should have been restarted and failed. assert_matches!( task.status, TaskStatus::Done { - result: TaskResult::FailedToSpawn(_), + result: TaskResult::Failed(127), .. }, "The task should have failed" @@ -128,14 +127,10 @@ async fn restart_and_edit_task_priority() -> Result<()> { // Set the editor to a command which replaces the temporary file's content. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo '99' > "); + envs.insert("EDITOR", "echo '99' > ${PUEUE_EDIT_PATH}/0/priority ||"); // Restart the command, edit its priority and wait for it to start. - run_client_command_with_env( - shared, - &["restart", "--in-place", "--edit-priority", "0"], - envs, - )?; + run_client_command_with_env(shared, &["restart", "--in-place", "--edit", "0"], envs)?; wait_for_task_condition(shared, 0, |task| task.is_done()).await?; // Make sure that the priority has been updated. @@ -158,7 +153,10 @@ async fn normal_restart_with_edit() -> Result<()> { // Set the editor to a command which replaces the temporary file's content. let mut envs = HashMap::new(); - envs.insert("EDITOR", "echo 'sleep 60' > "); + envs.insert( + "EDITOR", + "echo 'sleep 60' > ${PUEUE_EDIT_PATH}/0/command ||", + ); // Restart the command, edit its command and wait for it to start. run_client_command_with_env(shared, &["restart", "--edit", "0"], envs)?; diff --git a/pueue/tests/daemon/integration/aliases.rs b/pueue/tests/daemon/integration/aliases.rs index 97a6b2ad..c36b6ed6 100644 --- a/pueue/tests/daemon/integration/aliases.rs +++ b/pueue/tests/daemon/integration/aliases.rs @@ -78,11 +78,10 @@ async fn test_restart_with_alias() -> Result<()> { let message = RestartMessage { tasks: vec![TaskToRestart { task_id: 0, - command: Some("replaced_cmd test".to_string()), - path: None, - label: None, - delete_label: false, - priority: Some(0), + command: "replaced_cmd test".to_string(), + path: task.path, + label: task.label, + priority: task.priority, }], start_immediately: true, stashed: false, diff --git a/pueue/tests/daemon/integration/edit.rs b/pueue/tests/daemon/integration/edit.rs index 186333fc..ec8aba8a 100644 --- a/pueue/tests/daemon/integration/edit.rs +++ b/pueue/tests/daemon/integration/edit.rs @@ -11,7 +11,7 @@ use pueue_lib::task::*; use crate::helper::*; -async fn create_edited_task(shared: &Shared) -> Result { +async fn create_edited_task(shared: &Shared) -> Result> { // Add a task assert_success(add_task(shared, "ls").await?); @@ -23,7 +23,7 @@ async fn create_edited_task(shared: &Shared) -> Result { ); // Send a request to edit that task - let response = send_message(shared, Message::EditRequest(0)).await?; + let response = send_message(shared, Message::EditRequest(vec![0])).await?; if let Message::EditResponse(payload) = response { Ok(payload) } else { @@ -41,11 +41,12 @@ async fn test_edit_flow() -> Result<()> { pause_tasks(shared, TaskSelection::All).await?; wait_for_group_status(shared, PUEUE_DEFAULT_GROUP, GroupStatus::Paused).await?; - let response = create_edited_task(shared).await?; - assert_eq!(response.task_id, 0); - assert_eq!(response.command, "ls"); - assert_eq!(response.path, daemon.tempdir.path()); - assert_eq!(response.priority, 0); + let mut response = create_edited_task(shared).await?; + let mut editable_task = response.remove(0); + assert_eq!(editable_task.id, 0); + assert_eq!(editable_task.command, "ls"); + assert_eq!(editable_task.path, daemon.tempdir.path()); + assert_eq!(editable_task.priority, 0); // Task should be locked, after the request for editing succeeded. assert_matches!( @@ -62,19 +63,13 @@ async fn test_edit_flow() -> Result<()> { "Expected the task to still be locked." ); + editable_task.command = "ls -ahl".to_string(); + editable_task.path = PathBuf::from("/tmp"); + editable_task.label = Some("test".to_string()); + editable_task.priority = 99; + // Send the final message of the protocol and actually change the task. - let response = send_message( - shared, - EditMessage { - task_id: 0, - command: Some("ls -ahl".into()), - path: Some("/tmp".into()), - label: Some("test".to_string()), - delete_label: false, - priority: Some(99), - }, - ) - .await?; + let response = send_message(shared, Message::Edit(vec![editable_task])).await?; assert_success(response); // Make sure the task has been changed and enqueued. diff --git a/pueue/tests/daemon/integration/restart.rs b/pueue/tests/daemon/integration/restart.rs index c08743cd..667db82d 100644 --- a/pueue/tests/daemon/integration/restart.rs +++ b/pueue/tests/daemon/integration/restart.rs @@ -22,11 +22,10 @@ async fn test_restart_in_place() -> Result<()> { let restart_message = RestartMessage { tasks: vec![TaskToRestart { task_id: 0, - command: Some("sleep 60".to_string()), - path: Some(PathBuf::from("/tmp")), - label: Some("test".to_owned()), - delete_label: false, - priority: Some(0), + command: "sleep 60".to_string(), + path: PathBuf::from("/tmp"), + label: Some("test".to_string()), + priority: 0, }], start_immediately: false, stashed: false, @@ -65,17 +64,16 @@ async fn test_cannot_restart_running() -> Result<()> { assert_success(add_task(shared, "sleep 60").await?); // Wait for task 0 to finish. - wait_for_task_condition(shared, 0, |task| task.is_running()).await?; + let task = wait_for_task_condition(shared, 0, |task| task.is_running()).await?; // Restart task 0 with an extended sleep command. let restart_message = RestartMessage { tasks: vec![TaskToRestart { task_id: 0, - command: None, - path: None, - label: None, - delete_label: false, - priority: Some(0), + command: task.command, + path: task.path, + label: task.label, + priority: task.priority, }], start_immediately: false, stashed: false, diff --git a/pueue_lib/src/network/message.rs b/pueue_lib/src/network/message.rs index ffa5a281..9da08da1 100644 --- a/pueue_lib/src/network/message.rs +++ b/pueue_lib/src/network/message.rs @@ -57,14 +57,14 @@ pub enum Message { /// The first part of the three-step protocol to edit a task. /// This one requests an edit from the daemon. - EditRequest(usize), + EditRequest(Vec), + /// The daemon locked the tasks and responds with the tasks' details. + EditResponse(Vec), /// This is send by the client if something went wrong during the editing process. /// The daemon will go ahead and restore the task's old state. - EditRestore(usize), - /// The daemon locked the task and responds with the task's details. - EditResponse(EditResponseMessage), + EditRestore(Vec), /// The client sends the edited details to the daemon. - Edit(EditMessage), + Edit(Vec), Group(GroupMessage), GroupResponse(GroupResponseMessage), @@ -187,16 +187,13 @@ impl_into_message!(RestartMessage, Message::Restart); pub struct TaskToRestart { pub task_id: usize, /// Restart the task with an updated command. - pub command: Option, + pub command: String, /// Restart the task with an updated path. - pub path: Option, + pub path: PathBuf, /// Restart the task with an updated label. pub label: Option, - /// Cbor cannot represent `Option>` yet, which is why we have to utilize a - /// boolean to indicate that the label should be released, rather than an `Some(None)`. - pub delete_label: bool, /// Restart the task with an updated priority. - pub priority: Option, + pub priority: i32, } #[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize)] @@ -244,29 +241,37 @@ pub struct SendMessage { impl_into_message!(SendMessage, Message::Send); #[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize)] -pub struct EditResponseMessage { - pub task_id: usize, +pub struct EditableTask { + pub id: usize, pub command: String, pub path: PathBuf, pub label: Option, pub priority: i32, } -impl_into_message!(EditResponseMessage, Message::EditResponse); - -#[derive(PartialEq, Eq, Clone, Debug, Default, Deserialize, Serialize)] -pub struct EditMessage { - pub task_id: usize, - pub command: Option, - pub path: Option, - pub label: Option, - /// Cbor cannot represent `Option>` yet, which is why we have to utilize a - /// boolean to indicate that the label should be released, rather than an `Some(None)`. - pub delete_label: bool, - pub priority: Option, +impl From<&Task> for EditableTask { + /// Create an editable tasks from any [Task]] + fn from(value: &Task) -> Self { + EditableTask { + id: value.id, + command: value.command.clone(), + path: value.path.clone(), + label: value.label.clone(), + priority: value.priority, + } + } } -impl_into_message!(EditMessage, Message::Edit); +impl EditableTask { + /// Merge a [EditableTask] back into a [Task]. + pub fn into_task(self, task: &mut Task) { + task.id = self.id; + task.command = self.command; + task.path = self.path; + task.label = self.label; + task.priority = self.priority; + } +} #[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize)] pub enum GroupMessage {