From ae0b54fa2716eda963b2d23316a3f10dc971083a Mon Sep 17 00:00:00 2001 From: samlich <1349989+samlich@users.noreply.github.com> Date: Thu, 6 Apr 2023 09:26:32 +0000 Subject: [PATCH 1/6] enable deleting history items with keybinding --- src/edit_mode/keybindings.rs | 1 + src/engine.rs | 31 +++++++++++++++++++++++++++++++ src/enums.rs | 4 ++++ src/history/cursor.rs | 28 ++++++++++++++++++++++++++++ src/history/file_backed.rs | 27 ++++++++++++++++++++------- src/history/item.rs | 2 +- 6 files changed, 85 insertions(+), 8 deletions(-) diff --git a/src/edit_mode/keybindings.rs b/src/edit_mode/keybindings.rs index 772e4199..f2e5b6c3 100644 --- a/src/edit_mode/keybindings.rs +++ b/src/edit_mode/keybindings.rs @@ -101,6 +101,7 @@ pub fn add_common_control_bindings(kb: &mut Keybindings) { kb.add_binding(KM::CONTROL, KC::Char('d'), ReedlineEvent::CtrlD); kb.add_binding(KM::CONTROL, KC::Char('l'), ReedlineEvent::ClearScreen); kb.add_binding(KM::CONTROL, KC::Char('r'), ReedlineEvent::SearchHistory); + kb.add_binding(KM::SHIFT, KC::Delete, ReedlineEvent::DeleteHistoryItem); kb.add_binding(KM::CONTROL, KC::Char('o'), ReedlineEvent::OpenEditor); } /// Add the arrow navigation and its `Ctrl` variants diff --git a/src/engine.rs b/src/engine.rs index 29d7097b..78263840 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -764,6 +764,16 @@ impl Reedline { // A handled Event causes a repaint Ok(EventStatus::Handled) } + ReedlineEvent::DeleteHistoryItem => { + match self + .history_cursor + .delete_item_at_cursor(self.history.as_mut()) + { + Some(Ok(())) => Ok(EventStatus::Handled), + None => Ok(EventStatus::Inapplicable), + Some(Err(_)) => Ok(EventStatus::Inapplicable), + } + } ReedlineEvent::PreviousHistory | ReedlineEvent::Up | ReedlineEvent::SearchHistory => { self.history_cursor .back(self.history.as_ref()) @@ -1097,6 +1107,27 @@ impl Reedline { self.enter_history_search(); Ok(EventStatus::Handled) } + ReedlineEvent::DeleteHistoryItem => { + if self.input_mode == InputMode::HistoryTraversal { + match self + .history_cursor + .delete_item_at_cursor(self.history.as_mut()) + { + Some(Ok(())) => { + self.update_buffer_from_history(); + // are these needed/correct? + self.editor.move_to_start(UndoBehavior::HistoryNavigation); + self.editor + .move_to_line_end(UndoBehavior::HistoryNavigation); + Ok(EventStatus::Handled) + } + None => Ok(EventStatus::Inapplicable), + Some(Err(_)) => Ok(EventStatus::Inapplicable), + } + } else { + Ok(EventStatus::Inapplicable) + } + } ReedlineEvent::Multiple(events) => { let mut latest_signal = EventStatus::Inapplicable; for event in events { diff --git a/src/enums.rs b/src/enums.rs index 009d6ed2..eb21c343 100644 --- a/src/enums.rs +++ b/src/enums.rs @@ -470,6 +470,9 @@ pub enum ReedlineEvent { /// Search the history for a string SearchHistory, + /// Delete currently selected history item + DeleteHistoryItem, + /// In vi mode multiple reedline events can be chained while parsing the /// command or movement characters Multiple(Vec), @@ -539,6 +542,7 @@ impl Display for ReedlineEvent { ReedlineEvent::Left => write!(f, "Left"), ReedlineEvent::NextHistory => write!(f, "NextHistory"), ReedlineEvent::SearchHistory => write!(f, "SearchHistory"), + ReedlineEvent::DeleteHistoryItem => write!(f, "DeleteHistoryItem"), ReedlineEvent::Multiple(_) => write!(f, "Multiple[ {{ ReedLineEvents, }} ]"), ReedlineEvent::UntilFound(_) => write!(f, "UntilFound [ {{ ReedLineEvents, }} ]"), ReedlineEvent::Menu(_) => write!(f, "Menu Name: "), diff --git a/src/history/cursor.rs b/src/history/cursor.rs index ab41ba4f..532bc1a2 100644 --- a/src/history/cursor.rs +++ b/src/history/cursor.rs @@ -91,6 +91,34 @@ impl HistoryCursor { self.current.as_ref().map(|e| e.command_line.to_string()) } + /// Delete the item (if present) at the cursor from the history, and go back or forward one item + pub fn delete_item_at_cursor(&mut self, history: &mut dyn History) -> Option> { + let current = self.current.as_ref()?; + if let Some(id) = current.id { + match history.delete(id) { + Ok(()) => { + // Cursor must be moved *after* deletion, as deleting may + // alter which entry a `HistoryItemId` points to. + if self.back(history).is_err() + || self.current.is_none() + || self.current.as_ref().unwrap().id == Some(id) + { + if self.forward(history).is_err() { + self.current = None; + } + } + Some(Ok(())) + } + Err(e) => Some(Err(e)), + } + } else { + if self.back(history).is_err() { + self.current = None; + } + Some(Ok(())) + } + } + /// Poll the current [`HistoryNavigationQuery`] mode pub fn get_navigation(&self) -> HistoryNavigationQuery { self.query.clone() diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs index d048fe11..5048521b 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -192,13 +192,26 @@ impl History for FileBackedHistory { Ok(()) } - fn delete(&mut self, _h: super::HistoryItemId) -> Result<()> { - Err(ReedlineError( - ReedlineErrorVariants::HistoryFeatureUnsupported { - history: "FileBackedHistory", - feature: "removing entries", - }, - )) + fn delete(&mut self, h: super::HistoryItemId) -> Result<()> { + let id = usize::try_from(h.0).map_err(|_| { + ReedlineError(ReedlineErrorVariants::OtherHistoryError( + "Invalid ID (not usize)", + )) + })?; + if self.len_on_disk <= id { + self.entries.remove(id); + Ok(()) + } else { + // Since no ID is written to disk, it's not possible to delete them. + // E.g. consider another instance having deleted entries, after this instance + // loaded the file. + Err(ReedlineError( + ReedlineErrorVariants::HistoryFeatureUnsupported { + history: "FileBackedHistory", + feature: "removing entries", + }, + )) + } } /// Writes unwritten history contents to disk. diff --git a/src/history/item.rs b/src/history/item.rs index 79cd4b52..fbcdc08e 100644 --- a/src/history/item.rs +++ b/src/history/item.rs @@ -52,7 +52,7 @@ impl From for i64 { /// This trait represents additional arbitrary context to be added to a history (optional, see [`HistoryItem`]) pub trait HistoryItemExtraInfo: Serialize + DeserializeOwned + Default + Send {} -#[derive(Default, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Default, Debug, PartialEq, Eq)] /// something that is serialized as null and deserialized by ignoring everything pub struct IgnoreAllExtraInfo; From 4a4af763cee5139ac85d186d2de82a674b19033a Mon Sep 17 00:00:00 2001 From: samlich <1349989+samlich@users.noreply.github.com> Date: Wed, 12 Apr 2023 21:50:08 +0000 Subject: [PATCH 2/6] review --- src/engine.rs | 37 +++++++++++++++++-------------------- src/history/cursor.rs | 9 ++++----- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 78263840..70fb372b 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -770,8 +770,7 @@ impl Reedline { .delete_item_at_cursor(self.history.as_mut()) { Some(Ok(())) => Ok(EventStatus::Handled), - None => Ok(EventStatus::Inapplicable), - Some(Err(_)) => Ok(EventStatus::Inapplicable), + Some(Err(_)) | None => Ok(EventStatus::Inapplicable), } } ReedlineEvent::PreviousHistory | ReedlineEvent::Up | ReedlineEvent::SearchHistory => { @@ -1108,24 +1107,22 @@ impl Reedline { Ok(EventStatus::Handled) } ReedlineEvent::DeleteHistoryItem => { - if self.input_mode == InputMode::HistoryTraversal { - match self - .history_cursor - .delete_item_at_cursor(self.history.as_mut()) - { - Some(Ok(())) => { - self.update_buffer_from_history(); - // are these needed/correct? - self.editor.move_to_start(UndoBehavior::HistoryNavigation); - self.editor - .move_to_line_end(UndoBehavior::HistoryNavigation); - Ok(EventStatus::Handled) - } - None => Ok(EventStatus::Inapplicable), - Some(Err(_)) => Ok(EventStatus::Inapplicable), + if self.input_mode != InputMode::HistoryTraversal { + return Ok(EventStatus::Inapplicable); + } + match self + .history_cursor + .delete_item_at_cursor(self.history.as_mut()) + { + Some(Ok(())) => { + self.update_buffer_from_history(); + // TODO: are these needed/correct? + self.editor.move_to_start(UndoBehavior::HistoryNavigation); + self.editor + .move_to_line_end(UndoBehavior::HistoryNavigation); + Ok(EventStatus::Handled) } - } else { - Ok(EventStatus::Inapplicable) + Some(Err(_)) | None => Ok(EventStatus::Inapplicable), } } ReedlineEvent::Multiple(events) => { @@ -1314,7 +1311,7 @@ impl Reedline { .set_buffer(prefix, UndoBehavior::HistoryNavigation); } } - HistoryNavigationQuery::SubstringSearch(_) => todo!(), + HistoryNavigationQuery::SubstringSearch(x) => todo!("{:?}", x), } } diff --git a/src/history/cursor.rs b/src/history/cursor.rs index 532bc1a2..0f9905fd 100644 --- a/src/history/cursor.rs +++ b/src/history/cursor.rs @@ -99,13 +99,12 @@ impl HistoryCursor { Ok(()) => { // Cursor must be moved *after* deletion, as deleting may // alter which entry a `HistoryItemId` points to. - if self.back(history).is_err() + if (self.back(history).is_err() || self.current.is_none() - || self.current.as_ref().unwrap().id == Some(id) + || self.current.as_ref().unwrap().id == Some(id)) + && self.forward(history).is_err() { - if self.forward(history).is_err() { - self.current = None; - } + self.current = None; } Some(Ok(())) } From e6e84c8f6bb3c7afb8cfa8a7196b68f130dc1297 Mon Sep 17 00:00:00 2001 From: samlich <1349989+samlich@users.noreply.github.com> Date: Wed, 12 Apr 2023 21:59:29 +0000 Subject: [PATCH 3/6] clear buffer when not in history mode --- src/engine.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/engine.rs b/src/engine.rs index 70fb372b..f77296ba 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1108,7 +1108,8 @@ impl Reedline { } ReedlineEvent::DeleteHistoryItem => { if self.input_mode != InputMode::HistoryTraversal { - return Ok(EventStatus::Inapplicable); + self.run_edit_commands(&[EditCommand::Clear]); + return Ok(EventStatus::Handled); } match self .history_cursor From 883b4c8b5b29a8989830b16f832b265b79898d6c Mon Sep 17 00:00:00 2001 From: samlich <1349989+samlich@users.noreply.github.com> Date: Wed, 12 Apr 2023 22:16:49 +0000 Subject: [PATCH 4/6] add comment about move_to_line_end --- src/engine.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/engine.rs b/src/engine.rs index f77296ba..989f3971 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1117,7 +1117,7 @@ impl Reedline { { Some(Ok(())) => { self.update_buffer_from_history(); - // TODO: are these needed/correct? + // Move to end of first line, see `Self::previous_history()`. self.editor.move_to_start(UndoBehavior::HistoryNavigation); self.editor .move_to_line_end(UndoBehavior::HistoryNavigation); @@ -1188,6 +1188,7 @@ impl Reedline { .back(self.history.as_ref()) .expect("todo: error handling"); self.update_buffer_from_history(); + // Move to end of *first* line, so that pressing up again goes directly to previous item. self.editor.move_to_start(UndoBehavior::HistoryNavigation); self.editor .move_to_line_end(UndoBehavior::HistoryNavigation); From baeecc289e40646d5b2f6362b9665b35dc6b6c58 Mon Sep 17 00:00:00 2001 From: samlich <1349989+samlich@users.noreply.github.com> Date: Wed, 12 Apr 2023 22:30:13 +0000 Subject: [PATCH 5/6] oops --- src/engine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine.rs b/src/engine.rs index 989f3971..541a51f4 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1313,7 +1313,7 @@ impl Reedline { .set_buffer(prefix, UndoBehavior::HistoryNavigation); } } - HistoryNavigationQuery::SubstringSearch(x) => todo!("{:?}", x), + HistoryNavigationQuery::SubstringSearch(_) => todo!(), } } From 55af00e7a32e9e72e8c67cdbd5953cf41c942bca Mon Sep 17 00:00:00 2001 From: samlich <1349989+samlich@users.noreply.github.com> Date: Sat, 29 Apr 2023 17:38:30 +0000 Subject: [PATCH 6/6] delete from file on file-backed history --- src/history/file_backed.rs | 161 +++++++++++++++++++++---------------- 1 file changed, 90 insertions(+), 71 deletions(-) diff --git a/src/history/file_backed.rs b/src/history/file_backed.rs index 5048521b..8a598c24 100644 --- a/src/history/file_backed.rs +++ b/src/history/file_backed.rs @@ -198,26 +198,93 @@ impl History for FileBackedHistory { "Invalid ID (not usize)", )) })?; - if self.len_on_disk <= id { - self.entries.remove(id); - Ok(()) - } else { - // Since no ID is written to disk, it's not possible to delete them. - // E.g. consider another instance having deleted entries, after this instance - // loaded the file. - Err(ReedlineError( - ReedlineErrorVariants::HistoryFeatureUnsupported { - history: "FileBackedHistory", - feature: "removing entries", - }, - )) + + let delete = self.entries[id].clone(); + let mut i = 0; + while i < self.entries.len() { + if self.entries[i] == delete { + self.entries.remove(i); + if i < self.len_on_disk { + self.len_on_disk -= 1; + } + } else { + i += 1; + } } + + self.sync_and_delete_item(Some(&delete)) + .map_err(|e| ReedlineError(ReedlineErrorVariants::IOError(e))) } /// Writes unwritten history contents to disk. /// /// If file would exceed `capacity` truncates the oldest entries. fn sync(&mut self) -> std::io::Result<()> { + self.sync_and_delete_item(None) + } + + fn session(&self) -> Option { + self.session + } +} + +impl FileBackedHistory { + /// Creates a new in-memory history that remembers `n <= capacity` elements + /// + /// # Panics + /// + /// If `capacity == usize::MAX` + pub fn new(capacity: usize) -> Self { + if capacity == usize::MAX { + panic!("History capacity too large to be addressed safely"); + } + FileBackedHistory { + capacity, + entries: VecDeque::new(), + file: None, + len_on_disk: 0, + session: None, + } + } + + /// Creates a new history with an associated history file. + /// + /// History file format: commands separated by new lines. + /// If file exists file will be read otherwise empty file will be created. + /// + /// + /// **Side effects:** creates all nested directories to the file + /// + pub fn with_file(capacity: usize, file: PathBuf) -> std::io::Result { + let mut hist = Self::new(capacity); + if let Some(base_dir) = file.parent() { + std::fs::create_dir_all(base_dir)?; + } + hist.file = Some(file); + hist.sync()?; + Ok(hist) + } + + // this history doesn't store any info except command line + fn construct_entry(id: Option, command_line: String) -> HistoryItem { + HistoryItem { + id, + start_timestamp: None, + command_line, + session_id: None, + hostname: None, + cwd: None, + duration: None, + exit_status: None, + more_info: None, + } + } + + /// Writes unwritten history contents to disk, and optionally deletes + /// all occurences of the provided item. + /// + /// If file would exceed `capacity` truncates the oldest entries. + fn sync_and_delete_item(&mut self, delete: Option<&str>) -> std::io::Result<()> { if let Some(fname) = &self.file { // The unwritten entries let own_entries = self.entries.range(self.len_on_disk..); @@ -236,9 +303,18 @@ impl History for FileBackedHistory { let mut writer_guard = f_lock.write()?; let (mut foreign_entries, truncate) = { let reader = BufReader::new(writer_guard.deref()); + let mut deletions = false; let mut from_file = reader .lines() .map(|o| o.map(|i| decode_entry(&i))) + .filter(|x| { + if x.as_deref().ok() == delete { + deletions = true; + false + } else { + true + } + }) .collect::>>()?; if from_file.len() + own_entries.len() > self.capacity { ( @@ -246,7 +322,7 @@ impl History for FileBackedHistory { true, ) } else { - (from_file, false) + (from_file, deletions) } }; @@ -282,63 +358,6 @@ impl History for FileBackedHistory { } Ok(()) } - - fn session(&self) -> Option { - self.session - } -} - -impl FileBackedHistory { - /// Creates a new in-memory history that remembers `n <= capacity` elements - /// - /// # Panics - /// - /// If `capacity == usize::MAX` - pub fn new(capacity: usize) -> Self { - if capacity == usize::MAX { - panic!("History capacity too large to be addressed safely"); - } - FileBackedHistory { - capacity, - entries: VecDeque::new(), - file: None, - len_on_disk: 0, - session: None, - } - } - - /// Creates a new history with an associated history file. - /// - /// History file format: commands separated by new lines. - /// If file exists file will be read otherwise empty file will be created. - /// - /// - /// **Side effects:** creates all nested directories to the file - /// - pub fn with_file(capacity: usize, file: PathBuf) -> std::io::Result { - let mut hist = Self::new(capacity); - if let Some(base_dir) = file.parent() { - std::fs::create_dir_all(base_dir)?; - } - hist.file = Some(file); - hist.sync()?; - Ok(hist) - } - - // this history doesn't store any info except command line - fn construct_entry(id: Option, command_line: String) -> HistoryItem { - HistoryItem { - id, - start_timestamp: None, - command_line, - session_id: None, - hostname: None, - cwd: None, - duration: None, - exit_status: None, - more_info: None, - } - } } impl Drop for FileBackedHistory {