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

Author: alexmaco

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<()> {

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<usize> {
-		let (indexes, _): (Vec<usize>, Vec<_>) =
-			self.marked.iter().copied().unzip();
-
-		indexes
-	}
-
 	///
 	pub fn marked_commits(&self) -> Vec<CommitId> {
 		let (_, commits): (Vec<_>, Vec<CommitId>) =
@@ -159,106 +151,48 @@ 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<Cow<str>> = match marked {
+			[] => self
+				.items
 				.iter()
-				.map(|e| {
-					self.items
+				.nth(
+					self.selection
+						.saturating_sub(self.items.index_offset()),
+				)
+				.map(|e| Cow::Borrowed(e.hash_short.as_ref())),
+			[(_idx, commit)] => {
+				Some(commit.get_short_string().into())
+			}
+			[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()
-						.nth(*e)
-						.map_or_else(String::new, |le| {
-							le.hash_short.to_string()
+						.map(|(_idx, commit)| {
+							commit.get_short_string()
 						})
-				})
-				.join(" ");
-
-			if let Err(e) = crate::clipboard::copy_string(&separate) {
-				self.queue.push(InternalEvent::ShowErrorMsg(
-					copy_fail(&e.to_string()),
-				));
-				return Err(e);
+						.join(" ")
+				};
+				Some(yank.into())
 			}
+		};
+
+		if let Some(yank) = yank {
+			crate::clipboard::copy_string(&yank)?;
 			self.queue.push(InternalEvent::ShowInfoMsg(
-				copy_success(&separate),
+				strings::copy_success(&yank),
 			));
 		}
-
-		Ok(())
-	}
-
-	pub fn copy_entry_hash(&self) -> Result<()> {
-		match self.marked_count() {
-			0 => {
-				if let Some(e) = self.items.iter().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),
-					));
-				}
-			}
-			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),
-					));
-				}
-			}
-			_ => {}
-		}
 		Ok(())
 	}
 

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!(

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<str> {
 	if s.width() <= width {
 		Cow::Borrowed(s)

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<CommitId>,
@@ -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);