From e371153034db126235f342b52e2e6301e0880e70 Mon Sep 17 00:00:00 2001 From: Alexandru Macovei Date: Sat, 5 Nov 2022 17:51:03 +0200 Subject: [PATCH] Report failure to copy to clipboard (#1410) * (refactor) move copy_commit_hash from revlog into commitlist, and make fewer functions public in commitlist * (refactor) reduce duplication in commit copying code; use already-stored commits instead of looking up items * (clipboard) actually check subprocess exit status, and report failure instead of ignoring it * (commitlist) display popup with copy failure instead of exiting the application on error --- src/clipboard.rs | 16 +++- src/components/commitlist.rs | 140 +++++++++-------------------------- src/components/utils/mod.rs | 2 +- src/strings.rs | 6 +- src/tabs/revlog.rs | 15 ++-- 5 files changed, 57 insertions(+), 122 deletions(-) diff --git a/src/clipboard.rs b/src/clipboard.rs index fcb6af12..61170fe2 100644 --- a/src/clipboard.rs +++ b/src/clipboard.rs @@ -17,6 +17,7 @@ fn exec_copy_with_args( .args(args) .stdin(Stdio::piped()) .stdout(Stdio::null()) + .stderr(Stdio::piped()) .spawn() .map_err(|e| anyhow!("`{:?}`: {}", command, e))?; @@ -27,11 +28,20 @@ fn exec_copy_with_args( .write_all(text.as_bytes()) .map_err(|e| anyhow!("`{:?}`: {}", command, e))?; - process - .wait() + let out = process + .wait_with_output() .map_err(|e| anyhow!("`{:?}`: {}", command, e))?; - Ok(()) + if out.status.success() { + Ok(()) + } else { + let msg = if out.stderr.is_empty() { + format!("{}", out.status).into() + } else { + String::from_utf8_lossy(&out.stderr) + }; + Err(anyhow!("`{command:?}`: {msg}")) + } } fn exec_copy(command: &str, text: &str) -> Result<()> { diff --git a/src/components/commitlist.rs b/src/components/commitlist.rs index bb84c168..e58977c2 100644 --- a/src/components/commitlist.rs +++ b/src/components/commitlist.rs @@ -6,7 +6,7 @@ use crate::{ }, keys::{key_match, SharedKeyConfig}, queue::{InternalEvent, Queue}, - strings::{self, copy_fail, copy_success, symbol}, + strings::{self, symbol}, ui::style::{SharedTheme, Theme}, ui::{calc_scroll_top, draw_scrollbar}, }; @@ -143,14 +143,6 @@ impl CommitList { self.marked.clear(); } - /// - pub fn marked_indexes(&self) -> Vec { - let (indexes, _): (Vec, Vec<_>) = - self.marked.iter().copied().unzip(); - - indexes - } - /// pub fn marked_commits(&self) -> Vec { let (_, commits): (Vec<_>, Vec) = @@ -159,105 +151,47 @@ impl CommitList { commits } - fn marked_consecutive(&self) -> bool { - let marked = self.marked_indexes(); - - for i in 1..marked.len() { - if marked[i - 1] + 1 != marked[i] { - return false; - } - } - - true - } - - pub fn copy_marked_hashes(&self) -> Result<()> { - if self.marked_consecutive() { - let m = self.marked_indexes(); - - let first = self.items.iter().nth(m[0]); - - let last = self.items.iter().nth(m[m.len() - 1]); - - if let (Some(f), Some(l)) = (first, last) { - let yank = - format!("{}^..{}", f.hash_short, l.hash_short); - if let Err(e) = crate::clipboard::copy_string(&yank) { - self.queue.push(InternalEvent::ShowErrorMsg( - copy_fail(&e.to_string()), - )); - return Err(e); - } - self.queue.push(InternalEvent::ShowInfoMsg( - copy_success(&yank), - )); - }; - } else { - let separate = self - .marked_indexes() + pub fn copy_commit_hash(&self) -> Result<()> { + let marked = self.marked.as_slice(); + let yank: Option> = match marked { + [] => self + .items .iter() - .map(|e| { - self.items - .iter() - .nth(*e) - .map_or_else(String::new, |le| { - le.hash_short.to_string() - }) - }) - .join(" "); - - if let Err(e) = crate::clipboard::copy_string(&separate) { - self.queue.push(InternalEvent::ShowErrorMsg( - copy_fail(&e.to_string()), - )); - return Err(e); - } - self.queue.push(InternalEvent::ShowInfoMsg( - copy_success(&separate), - )); - } - - Ok(()) - } - - pub fn copy_entry_hash(&self) -> Result<()> { - match self.marked_count() { - 0 => { - if let Some(e) = self.items.iter().nth( + .nth( self.selection .saturating_sub(self.items.index_offset()), - ) { - if let Err(e) = - crate::clipboard::copy_string(&e.hash_short) - { - self.queue.push(InternalEvent::ShowErrorMsg( - copy_fail(&e.to_string()), - )); - return Err(e); - } - self.queue.push(InternalEvent::ShowInfoMsg( - copy_success(&e.hash_short), - )); - } + ) + .map(|e| Cow::Borrowed(e.hash_short.as_ref())), + [(_idx, commit)] => { + Some(commit.get_short_string().into()) } - 1 => { - if let Some(e) = - self.items.iter().nth(self.marked_indexes()[0]) - { - if let Err(e) = - crate::clipboard::copy_string(&e.hash_short) - { - self.queue.push(InternalEvent::ShowErrorMsg( - copy_fail(&e.to_string()), - )); - return Err(e); - } - self.queue.push(InternalEvent::ShowInfoMsg( - copy_success(&e.hash_short), - )); - } + [first, .., last] => { + let marked_consecutive = + marked.windows(2).all(|w| w[0].0 + 1 == w[1].0); + + let yank = if marked_consecutive { + format!( + "{}^..{}", + first.1.get_short_string(), + last.1.get_short_string() + ) + } else { + marked + .iter() + .map(|(_idx, commit)| { + commit.get_short_string() + }) + .join(" ") + }; + Some(yank.into()) } - _ => {} + }; + + if let Some(yank) = yank { + crate::clipboard::copy_string(&yank)?; + self.queue.push(InternalEvent::ShowInfoMsg( + strings::copy_success(&yank), + )); } Ok(()) } diff --git a/src/components/utils/mod.rs b/src/components/utils/mod.rs index 9f6a70a9..ad5cb390 100644 --- a/src/components/utils/mod.rs +++ b/src/components/utils/mod.rs @@ -12,7 +12,7 @@ pub mod statustree; /// It will show a popup in that case #[macro_export] macro_rules! try_or_popup { - ($self:ident, $msg:literal, $e:expr) => { + ($self:ident, $msg:expr, $e:expr) => { if let Err(err) = $e { ::log::error!("{} {}", $msg, err); $self.queue.push(InternalEvent::ShowErrorMsg(format!( diff --git a/src/strings.rs b/src/strings.rs index 47bf594a..45ed4cde 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -30,7 +30,7 @@ pub static PUSH_TAGS_STATES_DONE: &str = "done"; pub static POPUP_TITLE_SUBMODULES: &str = "Submodules"; pub static POPUP_TITLE_FUZZY_FIND: &str = "Fuzzy Finder"; -pub static POPUP_FAIL_COPY: &str = "Failed to copy the Text"; +pub static POPUP_FAIL_COPY: &str = "Failed to copy text"; pub static POPUP_SUCCESS_COPY: &str = "Copied Text"; pub mod symbol { @@ -360,10 +360,6 @@ pub fn copy_success(s: &str) -> String { format!("{POPUP_SUCCESS_COPY} \"{s}\"") } -pub fn copy_fail(e: &str) -> String { - format!("{POPUP_FAIL_COPY}: {e}") -} - pub fn ellipsis_trim_start(s: &str, width: usize) -> Cow { if s.width() <= width { Cow::Borrowed(s) diff --git a/src/tabs/revlog.rs b/src/tabs/revlog.rs index d8c83fa7..1e146592 100644 --- a/src/tabs/revlog.rs +++ b/src/tabs/revlog.rs @@ -163,15 +163,6 @@ impl Revlog { self.list.selected_entry().map(|e| e.id) } - fn copy_commit_hash(&self) -> Result<()> { - if self.list.marked_count() > 1 { - self.list.copy_marked_hashes()?; - } else { - self.list.copy_entry_hash()?; - } - Ok(()) - } - fn selected_commit_tags( &self, commit: &Option, @@ -260,7 +251,11 @@ impl Component for Revlog { self.update()?; return Ok(EventState::Consumed); } else if key_match(k, self.key_config.keys.copy) { - self.copy_commit_hash()?; + try_or_popup!( + self, + strings::POPUP_FAIL_COPY, + self.list.copy_commit_hash() + ); return Ok(EventState::Consumed); } else if key_match(k, self.key_config.keys.push) { self.queue.push(InternalEvent::PushTags);