From 83081f9852dfc6f743fb569c084690d325526502 Mon Sep 17 00:00:00 2001 From: Reilly Wood <26268125+rgwood@users.noreply.github.com> Date: Sun, 7 Jul 2024 06:09:59 -0700 Subject: [PATCH] `explore`: pass config to views at creation time (#13312) cc: @zhiburt This is an internal refactoring for `explore`. Previously, views inside `explore` were created with default/incorrect configuration and then the correct configuration was passed to them using a function called `setup()`. I believe this was because configuration was dynamic and could change while `explore` was running. After https://github.com/nushell/nushell/pull/10259, configuration can no longer be changed on the fly. So we can clean this up by removing `setup()` and passing configuration to views when they are created. --- crates/nu-explore/src/commands/expand.rs | 3 +- crates/nu-explore/src/commands/help.rs | 10 ++- crates/nu-explore/src/commands/mod.rs | 3 + crates/nu-explore/src/commands/nu.rs | 10 +-- crates/nu-explore/src/commands/table.rs | 5 +- crates/nu-explore/src/commands/try.rs | 5 +- crates/nu-explore/src/lib.rs | 9 +- crates/nu-explore/src/pager/mod.rs | 105 +++++++++------------- crates/nu-explore/src/registry/command.rs | 5 +- crates/nu-explore/src/views/binary/mod.rs | 19 ++-- crates/nu-explore/src/views/mod.rs | 6 -- crates/nu-explore/src/views/record/mod.rs | 28 +----- crates/nu-explore/src/views/try.rs | 35 ++++---- 13 files changed, 93 insertions(+), 150 deletions(-) diff --git a/crates/nu-explore/src/commands/expand.rs b/crates/nu-explore/src/commands/expand.rs index d7d75af6c5..cbb86336af 100644 --- a/crates/nu-explore/src/commands/expand.rs +++ b/crates/nu-explore/src/commands/expand.rs @@ -1,7 +1,7 @@ use super::ViewCommand; use crate::{ nu_common::{self, collect_input}, - views::Preview, + views::{Preview, ViewConfig}, }; use anyhow::Result; use nu_color_config::StyleComputer; @@ -43,6 +43,7 @@ impl ViewCommand for ExpandCmd { engine_state: &EngineState, stack: &mut Stack, value: Option, + _: &ViewConfig, ) -> Result { if let Some(value) = value { let value_as_string = convert_value_to_string(value, engine_state, stack)?; diff --git a/crates/nu-explore/src/commands/help.rs b/crates/nu-explore/src/commands/help.rs index 8be468383e..684e713939 100644 --- a/crates/nu-explore/src/commands/help.rs +++ b/crates/nu-explore/src/commands/help.rs @@ -1,5 +1,5 @@ use super::ViewCommand; -use crate::views::Preview; +use crate::views::{Preview, ViewConfig}; use anyhow::Result; use nu_ansi_term::Color; use nu_protocol::{ @@ -99,7 +99,13 @@ impl ViewCommand for HelpCmd { Ok(()) } - fn spawn(&mut self, _: &EngineState, _: &mut Stack, _: Option) -> Result { + fn spawn( + &mut self, + _: &EngineState, + _: &mut Stack, + _: Option, + _: &ViewConfig, + ) -> Result { Ok(HelpCmd::view()) } } diff --git a/crates/nu-explore/src/commands/mod.rs b/crates/nu-explore/src/commands/mod.rs index 141dc25f56..a748ddaaa3 100644 --- a/crates/nu-explore/src/commands/mod.rs +++ b/crates/nu-explore/src/commands/mod.rs @@ -1,3 +1,5 @@ +use crate::views::ViewConfig; + use super::pager::{Pager, Transition}; use anyhow::Result; use nu_protocol::{ @@ -49,5 +51,6 @@ pub trait ViewCommand { engine_state: &EngineState, stack: &mut Stack, value: Option, + config: &ViewConfig, ) -> Result; } diff --git a/crates/nu-explore/src/commands/nu.rs b/crates/nu-explore/src/commands/nu.rs index a0d479fd92..84e1fb9706 100644 --- a/crates/nu-explore/src/commands/nu.rs +++ b/crates/nu-explore/src/commands/nu.rs @@ -48,6 +48,7 @@ impl ViewCommand for NuCmd { engine_state: &EngineState, stack: &mut Stack, value: Option, + config: &ViewConfig, ) -> Result { let value = value.unwrap_or_default(); @@ -62,7 +63,7 @@ impl ViewCommand for NuCmd { return Ok(NuView::Preview(Preview::new(&text))); } - let mut view = RecordView::new(columns, values); + let mut view = RecordView::new(columns, values, config.explore_config.clone()); if is_record { view.set_top_layer_orientation(Orientation::Left); @@ -119,11 +120,4 @@ impl View for NuView { NuView::Preview(v) => v.exit(), } } - - fn setup(&mut self, config: ViewConfig<'_>) { - match self { - NuView::Records(v) => v.setup(config), - NuView::Preview(v) => v.setup(config), - } - } } diff --git a/crates/nu-explore/src/commands/table.rs b/crates/nu-explore/src/commands/table.rs index 1079cf6cea..9ab1e39b98 100644 --- a/crates/nu-explore/src/commands/table.rs +++ b/crates/nu-explore/src/commands/table.rs @@ -1,7 +1,7 @@ use super::ViewCommand; use crate::{ nu_common::collect_input, - views::{Orientation, RecordView}, + views::{Orientation, RecordView, ViewConfig}, }; use anyhow::Result; use nu_protocol::{ @@ -49,13 +49,14 @@ impl ViewCommand for TableCmd { _: &EngineState, _: &mut Stack, value: Option, + config: &ViewConfig, ) -> Result { let value = value.unwrap_or_default(); let is_record = matches!(value, Value::Record { .. }); let (columns, data) = collect_input(value)?; - let mut view = RecordView::new(columns, data); + let mut view = RecordView::new(columns, data, config.explore_config.clone()); if is_record { view.set_top_layer_orientation(Orientation::Left); diff --git a/crates/nu-explore/src/commands/try.rs b/crates/nu-explore/src/commands/try.rs index e70b4237d9..e1290651c8 100644 --- a/crates/nu-explore/src/commands/try.rs +++ b/crates/nu-explore/src/commands/try.rs @@ -1,5 +1,5 @@ use super::ViewCommand; -use crate::views::TryView; +use crate::views::{TryView, ViewConfig}; use anyhow::Result; use nu_protocol::{ engine::{EngineState, Stack}, @@ -43,9 +43,10 @@ impl ViewCommand for TryCmd { engine_state: &EngineState, stack: &mut Stack, value: Option, + config: &ViewConfig, ) -> Result { let value = value.unwrap_or_default(); - let mut view = TryView::new(value); + let mut view = TryView::new(value, config.explore_config.clone()); view.init(self.command.clone()); view.try_run(engine_state, stack)?; diff --git a/crates/nu-explore/src/lib.rs b/crates/nu-explore/src/lib.rs index d347321354..30b3e5cf87 100644 --- a/crates/nu-explore/src/lib.rs +++ b/crates/nu-explore/src/lib.rs @@ -10,6 +10,7 @@ use anyhow::Result; use commands::{ExpandCmd, HelpCmd, NuCmd, QuitCmd, TableCmd, TryCmd}; pub use default_context::add_explore_context; pub use explore::Explore; +use explore::ExploreConfig; use nu_common::{collect_pipeline, has_simple_value, CtrlC}; use nu_protocol::{ engine::{EngineState, Stack}, @@ -43,7 +44,7 @@ fn run_pager( if is_binary { p.show_message("For help type :help"); - let view = binary_view(input)?; + let view = binary_view(input, config.explore_config)?; return p.run(engine_state, stack, ctrlc, Some(view), commands); } @@ -73,7 +74,7 @@ fn create_record_view( is_record: bool, config: PagerConfig, ) -> Option { - let mut view = RecordView::new(columns, data); + let mut view = RecordView::new(columns, data, config.explore_config.clone()); if is_record { view.set_top_layer_orientation(Orientation::Left); } @@ -91,14 +92,14 @@ fn help_view() -> Option { Some(Page::new(HelpCmd::view(), false)) } -fn binary_view(input: PipelineData) -> Result { +fn binary_view(input: PipelineData, config: &ExploreConfig) -> Result { let data = match input { PipelineData::Value(Value::Binary { val, .. }, _) => val, PipelineData::ByteStream(bs, _) => bs.into_bytes()?, _ => unreachable!("checked beforehand"), }; - let view = BinaryView::new(data); + let view = BinaryView::new(data, config); Ok(Page::new(view, true)) } diff --git a/crates/nu-explore/src/pager/mod.rs b/crates/nu-explore/src/pager/mod.rs index a56df95bc7..3114543f7c 100644 --- a/crates/nu-explore/src/pager/mod.rs +++ b/crates/nu-explore/src/pager/mod.rs @@ -90,19 +90,42 @@ impl<'a> Pager<'a> { engine_state: &EngineState, stack: &mut Stack, ctrlc: CtrlC, - mut view: Option, + view: Option, commands: CommandRegistry, ) -> Result> { - if let Some(page) = &mut view { - page.view.setup(ViewConfig::new( - self.config.nu_config, - self.config.explore_config, - self.config.style_computer, - self.config.lscolors, - )) + // setup terminal + enable_raw_mode()?; + let mut stdout = io::stdout(); + execute!(stdout, EnterAlternateScreen, Clear(ClearType::All))?; + + let backend = CrosstermBackend::new(stdout); + let mut terminal = Terminal::new(backend)?; + + let mut info = ViewInfo { + status: Some(Report::default()), + ..Default::default() + }; + + if let Some(text) = self.message.take() { + info.status = Some(Report::message(text, Severity::Info)); } - run_pager(engine_state, stack, ctrlc, self, view, commands) + let result = render_ui( + &mut terminal, + engine_state, + stack, + ctrlc, + self, + &mut info, + view, + commands, + )?; + + // restore terminal + disable_raw_mode()?; + execute!(io::stdout(), LeaveAlternateScreen)?; + + Ok(result) } } @@ -145,49 +168,6 @@ impl<'a> PagerConfig<'a> { } } -fn run_pager( - engine_state: &EngineState, - stack: &mut Stack, - ctrlc: CtrlC, - pager: &mut Pager, - view: Option, - commands: CommandRegistry, -) -> Result> { - // setup terminal - enable_raw_mode()?; - let mut stdout = io::stdout(); - execute!(stdout, EnterAlternateScreen, Clear(ClearType::All))?; - - let backend = CrosstermBackend::new(stdout); - let mut terminal = Terminal::new(backend)?; - - let mut info = ViewInfo { - status: Some(Report::default()), - ..Default::default() - }; - - if let Some(text) = pager.message.take() { - info.status = Some(Report::message(text, Severity::Info)); - } - - let result = render_ui( - &mut terminal, - engine_state, - stack, - ctrlc, - pager, - &mut info, - view, - commands, - )?; - - // restore terminal - disable_raw_mode()?; - execute!(io::stdout(), LeaveAlternateScreen)?; - - Ok(result) -} - #[allow(clippy::too_many_arguments)] fn render_ui( term: &mut Terminal, @@ -440,15 +420,20 @@ fn run_command( Command::View { mut cmd, stackable } => { // what we do we just replace the view. let value = view_stack.curr_view.as_mut().and_then(|p| p.view.exit()); - let mut new_view = cmd.spawn(engine_state, stack, value)?; + let view_cfg = ViewConfig::new( + pager.config.nu_config, + pager.config.explore_config, + pager.config.style_computer, + pager.config.lscolors, + ); + + let new_view = cmd.spawn(engine_state, stack, value, &view_cfg)?; if let Some(view) = view_stack.curr_view.take() { if !view.stackable { view_stack.stack.push(view); } } - setup_view(&mut new_view, &pager.config); - view_stack.curr_view = Some(Page::raw(new_view, stackable)); Ok(CmdResult::new(false, true, cmd.name().to_owned())) @@ -456,16 +441,6 @@ fn run_command( } } -fn setup_view(view: &mut Box, cfg: &PagerConfig<'_>) { - let cfg = ViewConfig::new( - cfg.nu_config, - cfg.explore_config, - cfg.style_computer, - cfg.lscolors, - ); - view.setup(cfg); -} - fn set_cursor_cmd_bar(f: &mut Frame, area: Rect, pager: &Pager) { if pager.cmd_buf.is_cmd_input { // todo: deal with a situation where we exceed the bar width diff --git a/crates/nu-explore/src/registry/command.rs b/crates/nu-explore/src/registry/command.rs index 7e0bc131b5..6a3e9a128f 100644 --- a/crates/nu-explore/src/registry/command.rs +++ b/crates/nu-explore/src/registry/command.rs @@ -1,6 +1,6 @@ use crate::{ commands::{SimpleCommand, ViewCommand}, - views::View, + views::{View, ViewConfig}, }; use anyhow::Result; @@ -78,8 +78,9 @@ where engine_state: &nu_protocol::engine::EngineState, stack: &mut nu_protocol::engine::Stack, value: Option, + cfg: &ViewConfig, ) -> Result { - let view = self.0.spawn(engine_state, stack, value)?; + let view = self.0.spawn(engine_state, stack, value, cfg)?; Ok(Box::new(view) as Box) } } diff --git a/crates/nu-explore/src/views/binary/mod.rs b/crates/nu-explore/src/views/binary/mod.rs index f288861be8..2a35b7c180 100644 --- a/crates/nu-explore/src/views/binary/mod.rs +++ b/crates/nu-explore/src/views/binary/mod.rs @@ -40,11 +40,15 @@ struct Settings { } impl BinaryView { - pub fn new(data: Vec) -> Self { + pub fn new(data: Vec, cfg: &ExploreConfig) -> Self { + let settings = settings_from_config(cfg); + // There's gotta be a nicer way of doing this than creating a widget just to count lines + let count_rows = BinaryWidget::new(&data, settings.opts, Default::default()).count_lines(); + Self { data, - cursor: WindowCursor2D::default(), - settings: Settings::default(), + cursor: WindowCursor2D::new(count_rows, 1).expect("Failed to create XYCursor"), + settings, } } } @@ -87,15 +91,6 @@ impl View for BinaryView { // todo: impl Cursor + peek of a value None } - - fn setup(&mut self, cfg: ViewConfig<'_>) { - self.settings = settings_from_config(cfg.explore_config); - - let count_rows = - BinaryWidget::new(&self.data, self.settings.opts, Default::default()).count_lines(); - // TODO: refactor View so setup() is fallible and we don't have to panic here - self.cursor = WindowCursor2D::new(count_rows, 1).expect("Failed to create XYCursor"); - } } fn create_binary_widget(v: &BinaryView) -> BinaryWidget<'_> { diff --git a/crates/nu-explore/src/views/mod.rs b/crates/nu-explore/src/views/mod.rs index 7aab197eb5..bdbf3129db 100644 --- a/crates/nu-explore/src/views/mod.rs +++ b/crates/nu-explore/src/views/mod.rs @@ -99,8 +99,6 @@ pub trait View { fn exit(&mut self) -> Option { None } - - fn setup(&mut self, _: ViewConfig<'_>) {} } impl View for Box { @@ -131,8 +129,4 @@ impl View for Box { fn show_data(&mut self, i: usize) -> bool { self.as_mut().show_data(i) } - - fn setup(&mut self, cfg: ViewConfig<'_>) { - self.as_mut().setup(cfg) - } } diff --git a/crates/nu-explore/src/views/record/mod.rs b/crates/nu-explore/src/views/record/mod.rs index bd3c7e8e0a..4f617686f1 100644 --- a/crates/nu-explore/src/views/record/mod.rs +++ b/crates/nu-explore/src/views/record/mod.rs @@ -36,14 +36,12 @@ pub struct RecordView { } impl RecordView { - pub fn new(columns: Vec, records: Vec>) -> Self { + pub fn new(columns: Vec, records: Vec>, cfg: ExploreConfig) -> Self { Self { layer_stack: vec![RecordLayer::new(columns, records)], mode: UIMode::View, orientation: Orientation::Top, - // TODO: It's kind of gross how this temporarily has an incorrect/default config. - // See if we can pass correct config in through the constructor - cfg: ExploreConfig::default(), + cfg, } } @@ -72,23 +70,6 @@ impl RecordView { .expect("we guarantee that 1 entry is always in a list") } - pub fn get_top_layer_orientation(&mut self) -> Orientation { - self.get_top_layer().orientation - } - - pub fn set_orientation(&mut self, orientation: Orientation) { - self.orientation = orientation; - - // we need to reset all indexes as we can't no more use them. - self.reset_cursors(); - } - - fn reset_cursors(&mut self) { - for layer in &mut self.layer_stack { - layer.reset_cursor(); - } - } - pub fn set_top_layer_orientation(&mut self, orientation: Orientation) { let layer = self.get_top_layer_mut(); layer.orientation = orientation; @@ -299,11 +280,6 @@ impl View for RecordView { fn exit(&mut self) -> Option { Some(build_last_value(self)) } - - // todo: move the method to Command? - fn setup(&mut self, cfg: ViewConfig<'_>) { - self.cfg = cfg.explore_config.clone(); - } } fn get_element_info( diff --git a/crates/nu-explore/src/views/try.rs b/crates/nu-explore/src/views/try.rs index b23ca3e9f4..099e792e38 100644 --- a/crates/nu-explore/src/views/try.rs +++ b/crates/nu-explore/src/views/try.rs @@ -1,5 +1,6 @@ use super::{record::RecordView, util::nu_style_to_tui, Layout, Orientation, View, ViewConfig}; use crate::{ + explore::ExploreConfig, nu_common::{collect_pipeline, run_command_with_value}, pager::{report::Report, Frame, Transition, ViewInfo}, }; @@ -23,17 +24,19 @@ pub struct TryView { table: Option, view_mode: bool, border_color: Style, + config: ExploreConfig, } impl TryView { - pub fn new(input: Value) -> Self { + pub fn new(input: Value, config: ExploreConfig) -> Self { Self { input, table: None, - immediate: false, - border_color: Style::default(), + immediate: config.try_reactive, + border_color: nu_style_to_tui(config.table.separator_style), view_mode: false, command: String::new(), + config, } } @@ -42,7 +45,13 @@ impl TryView { } pub fn try_run(&mut self, engine_state: &EngineState, stack: &mut Stack) -> Result<()> { - let view = run_command(&self.command, &self.input, engine_state, stack)?; + let view = run_command( + &self.command, + &self.input, + engine_state, + stack, + &self.config, + )?; self.table = Some(view); Ok(()) } @@ -122,7 +131,6 @@ impl View for TryView { f.render_widget(table_block, table_area); if let Some(table) = &mut self.table { - table.setup(cfg); let area = Rect::new( area.x + 2, area.y + 4, @@ -232,20 +240,6 @@ impl View for TryView { fn show_data(&mut self, i: usize) -> bool { self.table.as_mut().map_or(false, |v| v.show_data(i)) } - - fn setup(&mut self, config: ViewConfig<'_>) { - self.border_color = nu_style_to_tui(config.explore_config.table.separator_style); - self.immediate = config.explore_config.try_reactive; - - let mut r = RecordView::new(vec![], vec![]); - r.setup(config); - - if let Some(view) = &mut self.table { - view.setup(config); - view.set_orientation(r.get_top_layer_orientation()); - view.set_top_layer_orientation(r.get_top_layer_orientation()); - } - } } fn run_command( @@ -253,6 +247,7 @@ fn run_command( input: &Value, engine_state: &EngineState, stack: &mut Stack, + config: &ExploreConfig, ) -> Result { let pipeline = run_command_with_value(command, input, engine_state, stack)?; @@ -260,7 +255,7 @@ fn run_command( let (columns, values) = collect_pipeline(pipeline)?; - let mut view = RecordView::new(columns, values); + let mut view = RecordView::new(columns, values, config.clone()); if is_record { view.set_top_layer_orientation(Orientation::Left); }