From dffbc15067621ae4e4a613ed11edf2e9398da8fa Mon Sep 17 00:00:00 2001 From: wongjiahau Date: Sat, 25 Feb 2023 12:34:33 +0800 Subject: [PATCH] refactor(explorer,tree): remove unwrap to avoid panics --- changes | 4 +- helix-term/src/ui/explorer.rs | 87 ++++++++--------- helix-term/src/ui/tree.rs | 173 ++++++++++++++++++---------------- 3 files changed, 136 insertions(+), 128 deletions(-) diff --git a/changes b/changes index ff97ff45..c5d82a77 100644 --- a/changes +++ b/changes @@ -47,14 +47,12 @@ New: - [x] bind "C-n/C-p" to up/down - [x] bind "="/"_" to zoom-in/zoom-out - [x] Sticky ancestors +- [x] remove unwrap and expect - [] Toggle preview - [] search highlight matching word - [] Error didn't clear - [] should preview be there by default? - [] Fix panic bugs (see github comments) -- [] explorer(preview): use popup instead of custom components - [] explorer(preview): overflow where bufferline is there -- [] explorer(preview): implement scrolling C-j/C-k - [] symlink not showing -- [] remove unwrap and expect - [] bug(tree): zb does not work, because clash with explorer 'b' diff --git a/helix-term/src/ui/explorer.rs b/helix-term/src/ui/explorer.rs index d0d9aac1..6fcde7b5 100644 --- a/helix-term/src/ui/explorer.rs +++ b/helix-term/src/ui/explorer.rs @@ -279,34 +279,36 @@ impl Explorer { } fn render_preview(&mut self, area: Rect, surface: &mut Surface, editor: &Editor) { - let item = self.tree.current().item(); - let head_area = render_block( - area.clip_bottom(area.height.saturating_sub(2)), - surface, - Borders::BOTTOM, - ); - let path_str = format!("{}", item.path.display()); - surface.set_stringn( - head_area.x, - head_area.y, - path_str, - head_area.width as usize, - get_theme!(editor.theme, "ui.explorer.dir", "ui.text"), - ); - - let body_area = area.clip_top(2); - let style = editor.theme.get("ui.text"); - let content = get_preview(&item.path, body_area.height as usize) - .unwrap_or_else(|err| vec![err.to_string()]); - content.into_iter().enumerate().for_each(|(row, line)| { + if let Ok(current) = self.tree.current() { + let item = current.item(); + let head_area = render_block( + area.clip_bottom(area.height.saturating_sub(2)), + surface, + Borders::BOTTOM, + ); + let path_str = format!("{}", item.path.display()); surface.set_stringn( - body_area.x, - body_area.y + row as u16, - line, - body_area.width as usize, - style, + head_area.x, + head_area.y, + path_str, + head_area.width as usize, + get_theme!(editor.theme, "ui.explorer.dir", "ui.text"), ); - }) + + let body_area = area.clip_top(2); + let style = editor.theme.get("ui.text"); + let content = get_preview(&item.path, body_area.height as usize) + .unwrap_or_else(|err| vec![err.to_string()]); + content.into_iter().enumerate().for_each(|(row, line)| { + surface.set_stringn( + body_area.x, + body_area.y + row as u16, + line, + body_area.width as usize, + style, + ); + }) + } } fn new_create_folder_prompt(&mut self) -> Result<()> { @@ -338,14 +340,14 @@ impl Explorer { } fn nearest_folder(&self) -> Result { - let current = self.tree.current(); - if current.item().is_parent() { - Ok(current.item().path.to_path_buf()) + let current = self.tree.current()?.item(); + if current.is_parent() { + Ok(current.path.to_path_buf()) } else { - let parent_path = current.item().path.parent().ok_or_else(|| { + let parent_path = current.path.parent().ok_or_else(|| { anyhow::anyhow!(format!( "Unable to get parent path of '{}'", - current.item().path.to_string_lossy() + current.path.to_string_lossy() )) })?; Ok(parent_path.to_path_buf()) @@ -353,7 +355,7 @@ impl Explorer { } fn new_remove_prompt(&mut self) -> Result<()> { - let item = self.tree.current().item(); + let item = self.tree.current()?.item(); match item.file_type { FileType::Folder => self.new_remove_folder_prompt(), FileType::File => self.new_remove_file_prompt(), @@ -361,8 +363,8 @@ impl Explorer { } } - fn new_rename_prompt(&mut self, cx: &mut Context) { - let path = self.tree.current_item().path.clone(); + fn new_rename_prompt(&mut self, cx: &mut Context) -> Result<()> { + let path = self.tree.current_item()?.path.clone(); self.prompt = Some(( PromptAction::RenameFile, Prompt::new( @@ -373,10 +375,11 @@ impl Explorer { ) .with_line(path.to_string_lossy().to_string(), cx.editor), )); + Ok(()) } fn new_remove_file_prompt(&mut self) -> Result<()> { - let item = self.tree.current_item(); + let item = self.tree.current_item()?; ensure!( item.path.is_file(), "The path '{}' is not a file", @@ -395,7 +398,7 @@ impl Explorer { } fn new_remove_folder_prompt(&mut self) -> Result<()> { - let item = self.tree.current_item(); + let item = self.tree.current_item()?; ensure!( item.path.is_dir(), "The path '{}' is not a folder", @@ -616,7 +619,7 @@ impl Explorer { }; let line = prompt.line(); - let current_item_path = explorer.tree.current_item().path.clone(); + let current_item_path = explorer.tree.current_item()?.path.clone(); match (&action, event) { (PromptAction::CreateFolder, key!(Enter)) => explorer.new_folder(line)?, (PromptAction::CreateFile, key!(Enter)) => explorer.new_file(line)?, @@ -682,7 +685,7 @@ impl Explorer { } fn change_root_to_current_folder(&mut self) -> Result<()> { - self.change_root(self.tree.current_item().path.clone()) + self.change_root(self.tree.current_item()?.path.clone()) } fn change_root_parent_folder(&mut self) -> Result<()> { @@ -715,7 +718,7 @@ impl Explorer { } fn rename_current(&mut self, line: &String) -> Result<()> { - let item = self.tree.current_item(); + let item = self.tree.current_item()?; let path = PathBuf::from(line); if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; @@ -726,13 +729,13 @@ impl Explorer { } fn remove_folder(&mut self) -> Result<()> { - let item = self.tree.current_item(); + let item = self.tree.current_item()?; std::fs::remove_dir_all(&item.path)?; self.tree.refresh() } fn remove_file(&mut self) -> Result<()> { - let item = self.tree.current_item(); + let item = self.tree.current_item()?; std::fs::remove_file(&item.path)?; self.tree.refresh() } @@ -796,7 +799,7 @@ impl Component for Explorer { key!(']') => self.change_root_to_current_folder()?, key!('[') => self.go_to_previous_root(), key!('d') => self.new_remove_prompt()?, - key!('r') => self.new_rename_prompt(cx), + key!('r') => self.new_rename_prompt(cx)?, key!('-') | key!('_') => self.decrease_size(), key!('+') | key!('=') => self.increase_size(), _ => { diff --git a/helix-term/src/ui/tree.rs b/helix-term/src/ui/tree.rs index 656efa33..2997e5b3 100644 --- a/helix-term/src/ui/tree.rs +++ b/helix-term/src/ui/tree.rs @@ -439,15 +439,16 @@ impl TreeView { self.tree.regenerate_index(); } - fn move_to_parent(&mut self) { - if let Some(parent) = self.current_parent() { + fn move_to_parent(&mut self) -> Result<()> { + if let Some(parent) = self.current_parent()? { let index = parent.index; self.set_selected(index) } + Ok(()) } fn move_to_children(&mut self, filter: &String) -> Result<()> { - let current = self.current_mut(); + let current = self.current_mut()?; if current.is_opened { self.set_selected(self.selected + 1); Ok(()) @@ -526,17 +527,17 @@ impl TreeView { params: &mut T::Params, selected_index: usize, filter: &String, - ) { - let selected_item = self.get_mut(selected_index); + ) -> Result<()> { + let selected_item = self.get_mut(selected_index)?; if selected_item.is_opened { selected_item.close(); self.regenerate_index(); - return; + return Ok(()); } if let Some(mut on_open_fn) = self.on_opened_fn.take() { - let mut f = || { - let current = self.current_mut(); + let mut f = || -> Result<()> { + let current = self.current_mut()?; match on_open_fn(&mut current.item, cx, params) { TreeOp::GetChildsAndInsert => { if let Err(err) = current.open(filter) { @@ -545,11 +546,13 @@ impl TreeView { } TreeOp::Noop => {} }; + Ok(()) }; - f(); + f()?; self.regenerate_index(); - self.on_opened_fn = Some(on_open_fn) - } + self.on_opened_fn = Some(on_open_fn); + }; + Ok(()) } fn set_search_str(&mut self, s: String) { @@ -675,36 +678,36 @@ impl TreeView { }) } - fn get(&self, index: usize) -> &Tree { - self.tree - .get(index) - .expect(format!("Tree: index {index} is out of bound").as_str()) + fn get(&self, index: usize) -> Result<&Tree> { + self.tree.get(index).ok_or_else(|| { + anyhow::anyhow!("Programming error: TreeView.get: index {index} is out of bound") + }) } - fn get_mut(&mut self, index: usize) -> &mut Tree { - self.tree - .get_mut(index) - .expect(format!("Tree: index {index} is out of bound").as_str()) + fn get_mut(&mut self, index: usize) -> Result<&mut Tree> { + self.tree.get_mut(index).ok_or_else(|| { + anyhow::anyhow!("Programming error: TreeView.get_mut: index {index} is out of bound") + }) } - pub fn current(&self) -> &Tree { + pub fn current(&self) -> Result<&Tree> { self.get(self.selected) } - pub fn current_mut(&mut self) -> &mut Tree { + pub fn current_mut(&mut self) -> Result<&mut Tree> { self.get_mut(self.selected) } - fn current_parent(&self) -> Option<&Tree> { - if let Some(parent_index) = self.current().parent_index { - Some(self.get(parent_index)) + fn current_parent(&self) -> Result>> { + if let Some(parent_index) = self.current()?.parent_index { + Ok(Some(self.get(parent_index)?)) } else { - None + Ok(None) } } - pub fn current_item(&self) -> &T { - &self.current().item + pub fn current_item(&self) -> Result<&T> { + Ok(&self.current()?.item) } pub fn winline(&self) -> usize { @@ -1013,56 +1016,60 @@ impl TreeView { } let count = std::mem::replace(&mut self.count, 0); - match key_event { - key!(i @ '0'..='9') => self.count = i.to_digit(10).unwrap() as usize + count * 10, - key!('k') | key!(Up) | ctrl!('p') => self.move_up(1.max(count)), - key!('j') | key!(Down) | ctrl!('n') => self.move_down(1.max(count)), - key!('z') => { - self.on_next_key = Some(Box::new(|_, tree, event| match event { - key!('z') => tree.align_view_center(), - key!('t') => tree.align_view_top(), - key!('b') => tree.align_view_bottom(), - _ => {} - })); - } - key!('h') | key!(Left) => self.move_to_parent(), - key!('l') | key!(Right) => match self.move_to_children(filter) { - Ok(_) => {} - Err(err) => cx.editor.set_error(err.to_string()), - }, - shift!('H') => self.move_left(1), - shift!('L') => self.move_right(1), - key!(Enter) | key!('o') => self.on_enter(cx, params, self.selected, filter), - ctrl!('d') => self.move_down_half_page(), - ctrl!('u') => self.move_up_half_page(), - key!('g') => { - self.on_next_key = Some(Box::new(|_, tree, event| match event { - key!('g') => tree.move_to_first_line(), - key!('e') => tree.move_to_last_line(), - key!('h') => tree.move_leftmost(), - key!('l') => tree.move_rightmost(), - _ => {} - })); - } - key!('/') => self.new_search_prompt(Direction::Forward), - key!('n') => self.move_to_next_search_match(), - shift!('N') => self.move_to_previous_next_match(), - key!('f') => self.new_filter_prompt(cx), - key!(PageDown) => self.move_down_page(), - key!(PageUp) => self.move_up_page(), - shift!('R') => { - let filter = self.filter.clone(); - if let Err(error) = self.refresh_with_filter(&filter) { - cx.editor.set_error(error.to_string()) + (|| -> Result { + match key_event { + key!(i @ '0'..='9') => { + self.count = i.to_digit(10).unwrap_or(0) as usize + count * 10 } - } - key!(Home) => self.move_leftmost(), - key!(End) => self.move_rightmost(), - ctrl!('o') => self.jump_backward(), - _ => return EventResult::Ignored(None), - } - - EventResult::Consumed(None) + key!('k') | key!(Up) | ctrl!('p') => self.move_up(1.max(count)), + key!('j') | key!(Down) | ctrl!('n') => self.move_down(1.max(count)), + key!('z') => { + self.on_next_key = Some(Box::new(|_, tree, event| match event { + key!('z') => tree.align_view_center(), + key!('t') => tree.align_view_top(), + key!('b') => tree.align_view_bottom(), + _ => {} + })); + } + key!('h') | key!(Left) => self.move_to_parent()?, + key!('l') | key!(Right) => self.move_to_children(filter)?, + shift!('H') => self.move_left(1), + shift!('L') => self.move_right(1), + key!(Enter) | key!('o') => self.on_enter(cx, params, self.selected, filter)?, + ctrl!('d') => self.move_down_half_page(), + ctrl!('u') => self.move_up_half_page(), + key!('g') => { + self.on_next_key = Some(Box::new(|_, tree, event| match event { + key!('g') => tree.move_to_first_line(), + key!('e') => tree.move_to_last_line(), + key!('h') => tree.move_leftmost(), + key!('l') => tree.move_rightmost(), + _ => {} + })); + } + key!('/') => self.new_search_prompt(Direction::Forward), + key!('n') => self.move_to_next_search_match(), + shift!('N') => self.move_to_previous_next_match(), + key!('f') => self.new_filter_prompt(cx), + key!(PageDown) => self.move_down_page(), + key!(PageUp) => self.move_up_page(), + shift!('R') => { + let filter = self.filter.clone(); + if let Err(error) = self.refresh_with_filter(&filter) { + cx.editor.set_error(error.to_string()) + } + } + key!(Home) => self.move_leftmost(), + key!(End) => self.move_rightmost(), + ctrl!('o') => self.jump_backward(), + _ => return Ok(EventResult::Ignored(None)), + }; + Ok(EventResult::Consumed(None)) + })() + .unwrap_or_else(|err| { + cx.editor.set_error(format!("{err}")); + EventResult::Consumed(None) + }) } fn handle_filter_event(&mut self, event: &KeyEvent, cx: &mut Context) -> EventResult { @@ -1577,7 +1584,7 @@ mod test_tree_view { .trim() ); - view.move_to_parent(); + view.move_to_parent().unwrap(); assert_eq!( render(&mut view), " @@ -1591,7 +1598,7 @@ mod test_tree_view { ); view.move_to_last_line(); - view.move_to_parent(); + view.move_to_parent().unwrap(); assert_eq!( render(&mut view), " @@ -1751,7 +1758,7 @@ krabby_patty .trim() ); - view.move_to_parent(); + view.move_to_parent().unwrap(); assert_eq!( render(&mut view), " @@ -1764,7 +1771,7 @@ krabby_patty .trim() ); - view.move_to_parent(); + view.move_to_parent().unwrap(); assert_eq!( render(&mut view), " @@ -1777,7 +1784,7 @@ krabby_patty .trim() ); - view.move_to_parent(); + view.move_to_parent().unwrap(); assert_eq!( render(&mut view), " @@ -1988,7 +1995,7 @@ krabby_patty view.refresh_with_filter(&"ar".to_string()).unwrap(); // 3. Get the current item - let item = view.current_item(); + let item = view.current_item().unwrap(); // 3a. Expects no failure assert_eq!(item.name, "who_lives_in_a_pine") @@ -2214,7 +2221,7 @@ krabby_patty // 5.3 Move up view.move_up(1); - assert_eq!(view.current_item().name, "baaa"); + assert_eq!(view.current_item().unwrap().name, "baaa"); assert_eq!( render(&mut view), "