Improve blame view

- 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

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

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
+/// A `BlameFile` represents a collection of lines. This is targeted at how the
-/// API.
+/// data will be used by the UI.
-#[derive(Default, Clone, Debug)]
+#[derive(Clone, Debug)]
 pub struct FileBlame {
+    ///
+    pub commit_id: CommitId,
     ///
     pub path: String,
     ///
     pub lines: Vec<(Option<BlameHunk>, String)>,
 }
 
+///
+pub enum BlameAt {
+    ///
+    Head,
+    ///
+    Commit(CommitId),
+}
+
 ///
 pub fn blame_file(
     repo_path: &str,
     file_path: &str,
-    commit_id: &str,
+    blame_at: &BlameAt,
 ) -> Result<FileBlame> {
     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<BlameHunk>, 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);
 

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,

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

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<String>,
     file_blame: Option<FileBlame>,
@@ -35,7 +37,6 @@ pub struct BlameFileComponent {
     current_height: std::cell::Cell<usize>,
 }
 
-static COMMIT_ID: &str = "HEAD";
 static NO_COMMIT_ID: &str = "0000000";
 static NO_AUTHOR: &str = "<no author>";
 static MIN_AUTHOR_WIDTH: usize = 3;
@@ -70,14 +71,22 @@ impl DrawableComponent for BlameFileComponent {
                 .as_deref()
                 .unwrap_or("<no path for blame available>");
 
-            let title = if self.file_blame.is_some() {
+            let title = self.file_blame.as_ref().map_or_else(
-                format!("{} -- {} -- {}", self.title, path, COMMIT_ID)
+                || {
-            } else {
                     format!(
                         "{} -- {} -- <no blame available>",
                         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)
+            Cell::from(commit_hash).style(
-                .style(self.theme.commit_hash(false)),
+                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<CommitId> {
+        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
+        })
+    }
 }

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},

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)

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,),),