From e7b703b9221e895f660953c34b30e929657368af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20R=C3=BC=C3=9Fler?= Date: Sat, 24 Apr 2021 09:08:25 +0200 Subject: [PATCH] Improve blame view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Set default shortcut to `B` instead of `b` because the latter would shadow `[b]ranches`. - Add scrollbar. - Show resolved commit id in title instead of `HEAD`. - Make commit id bold if it is the commit id the file is blamed at. - Don’t run blame on a binary file. - Add shortcut for inspecting a commit in blame view. --- asyncgit/src/error.rs | 3 + asyncgit/src/sync/blame.rs | 47 +++++++++++---- asyncgit/src/sync/mod.rs | 2 +- src/app.rs | 1 + src/components/blame_file.rs | 114 ++++++++++++++++++++++++++++++----- src/keys.rs | 2 +- src/ui/style.rs | 13 ++++ vim_style_key_config.ron | 2 +- 8 files changed, 155 insertions(+), 29 deletions(-) diff --git a/asyncgit/src/error.rs b/asyncgit/src/error.rs index ff1580f8..1ba229fe 100644 --- a/asyncgit/src/error.rs +++ b/asyncgit/src/error.rs @@ -21,6 +21,9 @@ pub enum Error { #[error("git: uncommitted changes")] UncommittedChanges, + #[error("git: can\u{2019}t run blame on a binary file")] + NoBlameOnBinaryFile, + #[error("io error:{0}")] Io(#[from] std::io::Error), diff --git a/asyncgit/src/sync/blame.rs b/asyncgit/src/sync/blame.rs index 4daec799..ba0b8b92 100644 --- a/asyncgit/src/sync/blame.rs +++ b/asyncgit/src/sync/blame.rs @@ -1,7 +1,10 @@ //! Sync git API for fetching a file blame use super::{utils, CommitId}; -use crate::{error::Result, sync::get_commit_info}; +use crate::{ + error::{Error, Result}, + sync::get_commit_info, +}; use std::io::{BufRead, BufReader}; use std::path::Path; @@ -22,28 +25,47 @@ pub struct BlameHunk { pub end_line: usize, } -/// A `BlameFile` represents as a collection of hunks. This resembles `git2`’s -/// API. -#[derive(Default, Clone, Debug)] +/// A `BlameFile` represents a collection of lines. This is targeted at how the +/// data will be used by the UI. +#[derive(Clone, Debug)] pub struct FileBlame { + /// + pub commit_id: CommitId, /// pub path: String, /// pub lines: Vec<(Option, String)>, } +/// +pub enum BlameAt { + /// + Head, + /// + Commit(CommitId), +} + /// pub fn blame_file( repo_path: &str, file_path: &str, - commit_id: &str, + blame_at: &BlameAt, ) -> Result { let repo = utils::repo(repo_path)?; + let commit_id = match blame_at { + BlameAt::Head => utils::get_head_repo(&repo)?, + BlameAt::Commit(commit_id) => *commit_id, + }; - let spec = format!("{}:{}", commit_id, file_path); + let spec = format!("{}:{}", commit_id.to_string(), file_path); let blame = repo.blame_file(Path::new(file_path), None)?; let object = repo.revparse_single(&spec)?; let blob = repo.find_blob(object.id())?; + + if blob.is_binary() { + return Err(Error::NoBlameOnBinaryFile); + } + let reader = BufReader::new(blob.content()); let lines: Vec<(Option, String)> = reader @@ -84,6 +106,7 @@ pub fn blame_file( .collect(); let file_blame = FileBlame { + commit_id, path: file_path.into(), lines, }; @@ -96,7 +119,7 @@ mod tests { use crate::error::Result; use crate::sync::{ blame_file, commit, stage_add_file, tests::repo_init_empty, - BlameHunk, + BlameAt, BlameHunk, }; use std::{ fs::{File, OpenOptions}, @@ -112,7 +135,7 @@ mod tests { let repo_path = root.as_os_str().to_str().unwrap(); assert!(matches!( - blame_file(&repo_path, "foo", "HEAD"), + blame_file(&repo_path, "foo", &BlameAt::Head), Err(_) )); @@ -122,7 +145,7 @@ mod tests { stage_add_file(repo_path, file_path)?; commit(repo_path, "first commit")?; - let blame = blame_file(&repo_path, "foo", "HEAD")?; + let blame = blame_file(&repo_path, "foo", &BlameAt::Head)?; assert!(matches!( blame.lines.as_slice(), @@ -146,7 +169,7 @@ mod tests { stage_add_file(repo_path, file_path)?; commit(repo_path, "second commit")?; - let blame = blame_file(&repo_path, "foo", "HEAD")?; + let blame = blame_file(&repo_path, "foo", &BlameAt::Head)?; assert!(matches!( blame.lines.as_slice(), @@ -173,14 +196,14 @@ mod tests { file.write(b"line 3\n")?; - let blame = blame_file(&repo_path, "foo", "HEAD")?; + let blame = blame_file(&repo_path, "foo", &BlameAt::Head)?; assert_eq!(blame.lines.len(), 2); stage_add_file(repo_path, file_path)?; commit(repo_path, "third commit")?; - let blame = blame_file(&repo_path, "foo", "HEAD")?; + let blame = blame_file(&repo_path, "foo", &BlameAt::Head)?; assert_eq!(blame.lines.len(), 3); diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 155ea38f..3589fb34 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -25,7 +25,7 @@ pub mod status; mod tags; pub mod utils; -pub use blame::{blame_file, BlameHunk, FileBlame}; +pub use blame::{blame_file, BlameAt, BlameHunk, FileBlame}; pub use branch::{ branch_compare_upstream, checkout_branch, config_is_pull_rebase, create_branch, delete_branch, get_branch_remote, diff --git a/src/app.rs b/src/app.rs index a7b91ccf..354013e8 100644 --- a/src/app.rs +++ b/src/app.rs @@ -95,6 +95,7 @@ impl App { key_config.clone(), ), blame_file_popup: BlameFileComponent::new( + &queue, &strings::blame_title(&key_config), theme.clone(), key_config.clone(), diff --git a/src/components/blame_file.rs b/src/components/blame_file.rs index 2185c4c5..cba2458e 100644 --- a/src/components/blame_file.rs +++ b/src/components/blame_file.rs @@ -5,12 +5,13 @@ use super::{ use crate::{ components::{utils::string_width_align, ScrollType}, keys::SharedKeyConfig, + queue::{InternalEvent, Queue}, strings, - ui::style::SharedTheme, + ui::{self, style::SharedTheme}, }; use anyhow::Result; use asyncgit::{ - sync::{blame_file, BlameHunk, FileBlame}, + sync::{blame_file, BlameAt, BlameHunk, CommitId, FileBlame}, CWD, }; use crossterm::event::Event; @@ -27,6 +28,7 @@ use tui::{ pub struct BlameFileComponent { title: String, theme: SharedTheme, + queue: Queue, visible: bool, path: Option, file_blame: Option, @@ -35,7 +37,6 @@ pub struct BlameFileComponent { current_height: std::cell::Cell, } -static COMMIT_ID: &str = "HEAD"; static NO_COMMIT_ID: &str = "0000000"; static NO_AUTHOR: &str = ""; static MIN_AUTHOR_WIDTH: usize = 3; @@ -70,14 +71,22 @@ impl DrawableComponent for BlameFileComponent { .as_deref() .unwrap_or(""); - let title = if self.file_blame.is_some() { - format!("{} -- {} -- {}", self.title, path, COMMIT_ID) - } else { - format!( - "{} -- {} -- ", - self.title, path - ) - }; + let title = self.file_blame.as_ref().map_or_else( + || { + format!( + "{} -- {} -- ", + self.title, path + ) + }, + |file_blame| { + format!( + "{} -- {} -- {}", + self.title, + path, + file_blame.commit_id.get_short_string() + ) + }, + ); let rows = self.get_rows(area.width.into()); let author_width = get_author_width(area.width.into()); @@ -97,6 +106,8 @@ impl DrawableComponent for BlameFileComponent { Constraint::Min(0), ]; + let number_of_rows = rows.len(); + let table = Table::new(rows) .widths(&constraints) .column_spacing(1) @@ -116,6 +127,26 @@ impl DrawableComponent for BlameFileComponent { f.render_widget(Clear, area); f.render_stateful_widget(table, area, &mut table_state); + ui::draw_scrollbar( + f, + area, + &self.theme, + number_of_rows, + // April 2021: we don’t have access to `table_state.offset` + // (it’s private), so we use `table_state.selected()` as a + // replacement. + // + // Other widgets, for example `BranchListComponent`, manage + // scroll state themselves and use `self.scroll_top` in this + // situation. + // + // There are plans to change `render_stateful_widgets`, so this + // might be acceptable as an interim solution. + // + // https://github.com/fdehau/tui-rs/issues/448 + table_state.selected().unwrap_or(0), + ); + self.table_state.set(table_state); self.current_height.set(area.height.into()); } @@ -143,7 +174,17 @@ impl Component for BlameFileComponent { CommandInfo::new( strings::commands::scroll(&self.key_config), true, + self.file_blame.is_some(), + ) + .order(1), + ); + out.push( + CommandInfo::new( + strings::commands::log_details_open( + &self.key_config, + ), true, + self.file_blame.is_some(), ) .order(1), ); @@ -176,6 +217,20 @@ impl Component for BlameFileComponent { self.move_selection(ScrollType::PageDown); } else if key == self.key_config.page_up { self.move_selection(ScrollType::PageUp); + } else if key == self.key_config.focus_right { + self.hide(); + + return self.selected_commit().map_or( + Ok(false), + |id| { + self.queue.borrow_mut().push_back( + InternalEvent::InspectCommit( + id, None, + ), + ); + Ok(true) + }, + ); } return Ok(true); @@ -203,6 +258,7 @@ impl Component for BlameFileComponent { impl BlameFileComponent { /// pub fn new( + queue: &Queue, title: &str, theme: SharedTheme, key_config: SharedKeyConfig, @@ -210,6 +266,7 @@ impl BlameFileComponent { Self { title: String::from(title), theme, + queue: queue.clone(), visible: false, path: None, file_blame: None, @@ -222,7 +279,7 @@ impl BlameFileComponent { /// pub fn open(&mut self, path: &str) -> Result<()> { self.path = Some(path.into()); - self.file_blame = blame_file(CWD, path, COMMIT_ID).ok(); + self.file_blame = blame_file(CWD, path, &BlameAt::Head).ok(); self.table_state.get_mut().select(Some(0)); self.show()?; @@ -322,9 +379,20 @@ impl BlameFileComponent { |hunk| utils::time_to_string(hunk.time, true), ); + let is_blamed_commit = self + .file_blame + .as_ref() + .and_then(|file_blame| { + blame_hunk.map(|hunk| { + file_blame.commit_id == hunk.commit_id + }) + }) + .unwrap_or(false); + vec![ - Cell::from(commit_hash) - .style(self.theme.commit_hash(false)), + Cell::from(commit_hash).style( + self.theme.commit_hash_in_blame(is_blamed_commit), + ), Cell::from(time).style(self.theme.commit_time(false)), Cell::from(author).style(self.theme.commit_author(false)), ] @@ -372,4 +440,22 @@ impl BlameFileComponent { needs_update } + + fn selected_commit(&self) -> Option { + self.file_blame.as_ref().and_then(|file_blame| { + let table_state = self.table_state.take(); + + let commit_id = + table_state.selected().and_then(|selected| { + file_blame.lines[selected] + .0 + .as_ref() + .map(|hunk| hunk.commit_id) + }); + + self.table_state.set(table_state); + + commit_id + }) + } } diff --git a/src/keys.rs b/src/keys.rs index 1938a4e0..168ed72c 100644 --- a/src/keys.rs +++ b/src/keys.rs @@ -104,7 +104,7 @@ impl Default for KeyConfig { shift_up: KeyEvent { code: KeyCode::Up, modifiers: KeyModifiers::SHIFT}, shift_down: KeyEvent { code: KeyCode::Down, modifiers: KeyModifiers::SHIFT}, enter: KeyEvent { code: KeyCode::Enter, modifiers: KeyModifiers::empty()}, - blame: KeyEvent { code: KeyCode::Char('b'), modifiers: KeyModifiers::empty()}, + blame: KeyEvent { code: KeyCode::Char('B'), modifiers: KeyModifiers::SHIFT}, edit_file: KeyEvent { code: KeyCode::Char('e'), modifiers: KeyModifiers::empty()}, status_stage_all: KeyEvent { code: KeyCode::Char('a'), modifiers: KeyModifiers::empty()}, status_reset_item: KeyEvent { code: KeyCode::Char('D'), modifiers: KeyModifiers::SHIFT}, diff --git a/src/ui/style.rs b/src/ui/style.rs index b06d2750..2a19e1f2 100644 --- a/src/ui/style.rs +++ b/src/ui/style.rs @@ -229,6 +229,19 @@ impl Theme { ) } + pub fn commit_hash_in_blame( + &self, + is_blamed_commit: bool, + ) -> Style { + if is_blamed_commit { + Style::default() + .fg(self.commit_hash) + .add_modifier(Modifier::BOLD) + } else { + Style::default().fg(self.commit_hash) + } + } + pub fn push_gauge(&self) -> Style { Style::default() .fg(self.push_gauge_fg) diff --git a/vim_style_key_config.ron b/vim_style_key_config.ron index a8487a1d..abdf4986 100644 --- a/vim_style_key_config.ron +++ b/vim_style_key_config.ron @@ -45,7 +45,7 @@ shift_down: ( code: Char('J'), modifiers: ( bits: 1,),), enter: ( code: Enter, modifiers: ( bits: 0,),), - blame: ( code: Char('b'), modifiers: ( bits: 0,),), + blame: ( code: Char('B'), modifiers: ( bits: 1,),), edit_file: ( code: Char('I'), modifiers: ( bits: 1,),),