don't close branchlist every time (#550)

Stephan Dilly · Stephan Dilly · commit 73504847a752 · 2021-05-27T21:29:24.000+02:00
* do not close branchlist after branch rename * do not close branchlist after deleting a branch * closes #543
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ![charcount](assets/char_count.gif)
 
 ### Fixed
+- don't close branchlist every time ([#550](https://github.com/extrawurst/gitui/issues/550))
 - fixed key binding for *external exitor* in vim key bindings [[@yanganto](https://github.com/yanganto)] ([#549](https://github.com/extrawurst/gitui/issues/549))
 - fix some potential errors when deleting files while they are being diffed ([#490](https://github.com/extrawurst/gitui/issues/490))
 - push defaults to 'origin' remote if it exists ([#494](https://github.com/extrawurst/gitui/issues/494))
diff --git a/asyncgit/src/sync/branch.rs b/asyncgit/src/sync/branch.rs
@@ -33,7 +33,7 @@ pub(crate) fn get_branch_name(repo_path: &str) -> Result<String> {
 }
 
 ///
-pub struct BranchForDisplay {
+pub struct BranchInfo {
     ///
     pub name: String,
     ///
@@ -48,12 +48,9 @@ pub struct BranchForDisplay {
     pub has_upstream: bool,
 }
 
-/// Used to return only the nessessary information for displaying a branch
-/// rather than an iterator over the actual branches
-pub fn get_branches_to_display(
-    repo_path: &str,
-) -> Result<Vec<BranchForDisplay>> {
-    scope_time!("get_branches_to_display");
+/// returns a list of `BranchInfo` with a simple summary of info about a single branch
+pub fn get_branches_info(repo_path: &str) -> Result<Vec<BranchInfo>> {
+    scope_time!("get_branches_info");
 
     let cur_repo = utils::repo(repo_path)?;
     let branches_for_display = cur_repo
@@ -62,7 +59,7 @@ pub fn get_branches_to_display(
             let branch = b?.0;
             let top_commit = branch.get().peel_to_commit()?;
 
-            Ok(BranchForDisplay {
+            Ok(BranchInfo {
                 name: bytes2string(branch.name_bytes()?)?,
                 reference: bytes2string(branch.get().name_bytes())?,
                 top_commit_message: bytes2string(
@@ -299,7 +296,7 @@ mod tests_branches {
         let repo_path = root.as_os_str().to_str().unwrap();
 
         assert_eq!(
-            get_branches_to_display(repo_path)
+            get_branches_info(repo_path)
                 .unwrap()
                 .iter()
                 .map(|b| b.name.clone())
@@ -317,7 +314,7 @@ mod tests_branches {
         create_branch(repo_path, "test").unwrap();
 
         assert_eq!(
-            get_branches_to_display(repo_path)
+            get_branches_info(repo_path)
                 .unwrap()
                 .iter()
                 .map(|b| b.name.clone())
diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs
@@ -23,8 +23,8 @@ pub mod utils;
 
 pub use branch::{
     branch_compare_upstream, checkout_branch, create_branch,
-    delete_branch, get_branches_to_display, rename_branch,
-    BranchCompare, BranchForDisplay,
+    delete_branch, get_branches_info, rename_branch, BranchCompare,
+    BranchInfo,
 };
 pub use commit::{amend, commit, tag};
 pub use commit_details::{
diff --git a/src/app.rs b/src/app.rs
@@ -2,12 +2,13 @@ use crate::{
     accessors, any_popup_visible,
     cmdbar::CommandBar,
     components::{
-        event_pump, CommandBlocking, CommandInfo, CommitComponent,
-        Component, CreateBranchComponent, DrawableComponent,
+        event_pump, BranchListComponent, CommandBlocking,
+        CommandInfo, CommitComponent, Component,
+        CreateBranchComponent, DrawableComponent,
         ExternalEditorComponent, HelpComponent,
         InspectCommitComponent, MsgComponent, PushComponent,
-        RenameBranchComponent, ResetComponent, SelectBranchComponent,
-        StashMsgComponent, TagCommitComponent,
+        RenameBranchComponent, ResetComponent, StashMsgComponent,
+        TagCommitComponent,
     },
     draw_popups,
     input::{Input, InputEvent, InputState},
@@ -49,7 +50,7 @@ pub struct App {
     tag_commit_popup: TagCommitComponent,
     create_branch_popup: CreateBranchComponent,
     rename_branch_popup: RenameBranchComponent,
-    select_branch_popup: SelectBranchComponent,
+    select_branch_popup: BranchListComponent,
     cmdbar: RefCell<CommandBar>,
     tab: usize,
     revlog: Revlog,
@@ -127,7 +128,7 @@ impl App {
                 theme.clone(),
                 key_config.clone(),
             ),
-            select_branch_popup: SelectBranchComponent::new(
+            select_branch_popup: BranchListComponent::new(
                 queue.clone(),
                 theme.clone(),
                 key_config.clone(),
@@ -510,7 +511,7 @@ impl App {
                         )
                     } else {
                         flags.insert(NeedsUpdate::ALL);
-                        self.select_branch_popup.hide();
+                        self.select_branch_popup.update_branches()?;
                     }
                 }
                 Action::ForcePush(branch, force) => self
diff --git a/src/components/branchlist.rs b/src/components/branchlist.rs
@@ -6,13 +6,11 @@ use crate::{
     components::ScrollType,
     keys::SharedKeyConfig,
     queue::{Action, InternalEvent, NeedsUpdate, Queue},
-    strings,
+    strings, try_or_popup,
     ui::{self, calc_scroll_top},
 };
 use asyncgit::{
-    sync::{
-        checkout_branch, get_branches_to_display, BranchForDisplay,
-    },
+    sync::{checkout_branch, get_branches_info, BranchInfo},
     CWD,
 };
 use crossterm::event::Event;
@@ -33,8 +31,8 @@ use anyhow::Result;
 use ui::style::SharedTheme;
 
 ///
-pub struct SelectBranchComponent {
-    branch_names: Vec<BranchForDisplay>,
+pub struct BranchListComponent {
+    branch_names: Vec<BranchInfo>,
     visible: bool,
     selection: u16,
     scroll_top: Cell<usize>,
@@ -44,7 +42,7 @@ pub struct SelectBranchComponent {
     key_config: SharedKeyConfig,
 }
 
-impl DrawableComponent for SelectBranchComponent {
+impl DrawableComponent for BranchListComponent {
     fn draw<B: Backend>(
         &self,
         f: &mut Frame<B>,
@@ -104,7 +102,7 @@ impl DrawableComponent for SelectBranchComponent {
     }
 }
 
-impl Component for SelectBranchComponent {
+impl Component for BranchListComponent {
     fn commands(
         &self,
         out: &mut Vec<CommandInfo>,
@@ -166,15 +164,12 @@ impl Component for SelectBranchComponent {
                 } else if e == self.key_config.page_up {
                     return self.move_selection(ScrollType::PageUp);
                 } else if e == self.key_config.enter {
-                    if let Err(e) = self.switch_to_selected_branch() {
-                        log::error!("switch branch error: {}", e);
-                        self.queue.borrow_mut().push_back(
-                            InternalEvent::ShowErrorMsg(format!(
-                                "switch branch error:\n{}",
-                                e
-                            )),
-                        );
-                    }
+                    try_or_popup!(
+                        self,
+                        "switch branch error:",
+                        self.switch_to_selected_branch()
+                    );
+
                     self.hide()
                 } else if e == self.key_config.create_branch {
                     self.queue
@@ -190,7 +185,8 @@ impl Component for SelectBranchComponent {
                             cur_branch.name.clone(),
                         ),
                     );
-                    self.hide();
+
+                    self.update_branches()?;
                 } else if e == self.key_config.delete_branch
                     && !self.selection_is_cur_branch()
                 {
@@ -228,7 +224,7 @@ impl Component for SelectBranchComponent {
     }
 }
 
-impl SelectBranchComponent {
+impl BranchListComponent {
     pub fn new(
         queue: Queue,
         theme: SharedTheme,
@@ -245,10 +241,6 @@ impl SelectBranchComponent {
             current_height: Cell::new(0),
         }
     }
-    /// Get all the names of the branches in the repo
-    pub fn get_branch_names() -> Result<Vec<BranchForDisplay>> {
-        Ok(get_branches_to_display(CWD)?)
-    }
 
     ///
     pub fn open(&mut self) -> Result<()> {
@@ -258,14 +250,14 @@ impl SelectBranchComponent {
         Ok(())
     }
 
-    ////
+    /// fetch list of branches
     pub fn update_branches(&mut self) -> Result<()> {
-        self.branch_names = Self::get_branch_names()?;
+        self.branch_names = get_branches_info(CWD)?;
+        self.set_selection(self.selection)?;
         Ok(())
     }
 
-    ///
-    pub fn selection_is_cur_branch(&self) -> bool {
+    fn selection_is_cur_branch(&self) -> bool {
         self.branch_names
             .iter()
             .enumerate()
@@ -278,10 +270,7 @@ impl SelectBranchComponent {
 
     ///
     fn move_selection(&mut self, scroll: ScrollType) -> Result<bool> {
-        let num_branches: u16 = self.branch_names.len().try_into()?;
-        let num_branches = num_branches.saturating_sub(1);
-
-        let mut new_selection = match scroll {
+        let new_selection = match scroll {
             ScrollType::Up => self.selection.saturating_add(1),
             ScrollType::Down => self.selection.saturating_sub(1),
             ScrollType::PageDown => self
@@ -293,15 +282,26 @@ impl SelectBranchComponent {
             _ => self.selection,
         };
 
-        if new_selection > num_branches {
-            new_selection = num_branches;
-        }
-
-        self.selection = new_selection;
+        self.set_selection(new_selection)?;
 
         Ok(true)
     }
 
+    fn set_selection(&mut self, selection: u16) -> Result<()> {
+        let num_branches: u16 = self.branch_names.len().try_into()?;
+        let num_branches = num_branches.saturating_sub(1);
+
+        let selection = if selection > num_branches {
+            num_branches
+        } else {
+            selection
+        };
+
+        self.selection = selection;
+
+        Ok(())
+    }
+
     /// Get branches to display
     fn get_text(
         &self,
diff --git a/src/components/mod.rs b/src/components/mod.rs
@@ -1,3 +1,4 @@
+mod branchlist;
 mod changes;
 mod command;
 mod commit;
@@ -14,12 +15,12 @@ mod msg;
 mod push;
 mod rename_branch;
 mod reset;
-mod select_branch;
 mod stashmsg;
 mod tag_commit;
 mod textinput;
 mod utils;
 
+pub use branchlist::BranchListComponent;
 pub use changes::ChangesComponent;
 pub use command::{CommandInfo, CommandText};
 pub use commit::CommitComponent;
@@ -35,7 +36,6 @@ pub use msg::MsgComponent;
 pub use push::PushComponent;
 pub use rename_branch::RenameBranchComponent;
 pub use reset::ResetComponent;
-pub use select_branch::SelectBranchComponent;
 pub use stashmsg::StashMsgComponent;
 pub use tag_commit::TagCommitComponent;
 pub use textinput::{InputType, TextInputComponent};
diff --git a/src/components/utils/mod.rs b/src/components/utils/mod.rs
@@ -10,6 +10,7 @@ pub mod statustree;
 macro_rules! try_or_popup {
     ($self:ident, $msg:literal, $e:expr) => {
         if let Err(err) = $e {
+            ::log::error!("{} {}", $msg, err);
             $self.queue.borrow_mut().push_back(
                 InternalEvent::ShowErrorMsg(format!(
                     "{}\n{}",
