holm/nushell
1
1
Fork 0
forked from extern/nushell
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Don't redraw prompt when transient prompt disabled (#10788)

Browse Source 
# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->

moonlander pointed out in Discord that the transient prompt feature
added in release 0.86 (implemented in #10391) is causing the normal
prompt to be redrawn when the transient prompt variables are unset or
set to null. This PR is for fixing that, although it's more of a bandaid
fix. Maybe the transient prompt feature should be taken out entirely for
now so more thought can be given to its implementation.

Previously, I'd thought that when reedline redraws the prompt after a
command is entered, it's a whole new prompt, but apparently it's
actually the same prompt as the current line (?). So now, `nu_prompt` in
`repl.rs` is an `Arc<RwLock<NushellPrompt>>` (rather than just a
`NushellPrompt`), and this `Arc` is shared with the `TransientPrompt`
object so that if it can't find one of the `TRANSIENT_PROMPT_*`
variables, it uses a segment from `NushellPrompt` rather than
re-evaluate `PROMPT_COMMAND`, `PROMPT_COMMAND_RIGHT`, etc. Using an
`RwLock` means that there's a bunch of `.expect()`s all over the place,
which is not nice. It could perhaps be avoided with some changes on the
reedline side.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

`$env.PROMPT_COMMAND` (and other such variables) should no longer be
executed twice if the corresponding `$env.TRANSIENT_PROMPT_*` variable
is not set.

<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

# Steps to reproduce

Described by moonlander in Discord
[here](https://discord.com/channels/601130461678272522/614593951969574961/1164928022126792844).

Adding this to `env.nu` will result in `11` being added to
`/tmp/run_count` every time any command is run. The expected behavior is
a single `1` being added to `/tmp/run_count` instead of two. The prompt
command should not be executed again when the prompt is redrawn after a
command is executed.

```nu
$env.PROMPT_COMMAND = {||
  touch /tmp/run_count
  '1' | save /tmp/run_count --append
  '>'
}

# $env.TRANSIENT_PROMPT_COMMAND not set
```

If the following is added to `env.nu`, then `12` will be added to
`/tmp/run_count` every time any command is run, which is expected
behavior because the normal prompt command must be displayed the first
time the prompt is shown, then the transient prompt command is run when
the prompt is redrawn.
```nu
$env.TRANSIENT_PROMPT_COMMAND = {||
  touch /tmp/run_count
  '2' | save /tmp/run_count --append
  '>'
}
```

Here's a screenshot of what adding that first snippet looks like (`cargo
run` in the `main` branch):

![image](https://github.com/nushell/nushell/assets/45539777/b27a5c07-55b4-43c7-8a2c-0deba2d9d53a)


Here's a screenshot of what it looks like with this PR (only one `1` is
added to `/tmp/run_count` each time):

![image](https://github.com/nushell/nushell/assets/45539777/2b5c0a3a-8566-4428-9fda-1ffcc1dd6ae3)
This commit is contained in:
Yash Thakur 2023-12-15 14:41:44 -05:00 committed by GitHub
parent 3c6fac059e
commit 87717b9ddd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 74 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

117
crates/nu-cli/src/prompt_update.rs
View File

@ -8,7 +8,7 @@ use nu_protocol::{
};
use reedline::Prompt;
use std::borrow::Cow;
use std::sync::Arc;
use std::sync::{Arc, RwLock};
// Name of environment variable where the prompt could be stored
pub(crate) const PROMPT_COMMAND: &str = "PROMPT_COMMAND";
@ -98,12 +98,12 @@ fn get_prompt_string(
})
}
pub(crate) fn update_prompt<'prompt>(
pub(crate) fn update_prompt(
config: &Config,
engine_state: &EngineState,
stack: &Stack,
nu_prompt: &'prompt mut NushellPrompt,
) -> &'prompt dyn Prompt {
nu_prompt: Arc<RwLock<NushellPrompt>>,
) {
let mut stack = stack.clone();
let left_prompt_string = get_prompt_string(PROMPT_COMMAND, config, engine_state, &mut stack);
@ -138,95 +138,96 @@ pub(crate) fn update_prompt<'prompt>(
get_prompt_string(PROMPT_INDICATOR_VI_NORMAL, config, engine_state, &mut stack);
// apply the other indicators
nu_prompt.update_all_prompt_strings(
left_prompt_string,
right_prompt_string,
prompt_indicator_string,
prompt_multiline_string,
(prompt_vi_insert_string, prompt_vi_normal_string),
config.render_right_prompt_on_last_line,
);
nu_prompt
.write()
.expect("Could not lock on nu_prompt to update")
.update_all_prompt_strings(
left_prompt_string,
right_prompt_string,
prompt_indicator_string,
prompt_multiline_string,
(prompt_vi_insert_string, prompt_vi_normal_string),
config.render_right_prompt_on_last_line,
);
let ret_val = nu_prompt as &dyn Prompt;
trace!("update_prompt {}:{}:{}", file!(), line!(), column!());
ret_val
}
struct TransientPrompt {
prompt_lock: Arc<RwLock<NushellPrompt>>,
engine_state: Arc<EngineState>,
stack: Stack,
}
/// Try getting `$env.TRANSIENT_PROMPT_<X>`, and get `$env.PROMPT_<X>` if that fails
fn get_transient_prompt_string(
transient_prompt: &str,
prompt: &str,
config: &Config,
engine_state: &EngineState,
stack: &mut Stack,
) -> Option<String> {
get_prompt_string(transient_prompt, config, engine_state, stack)
.or_else(|| get_prompt_string(prompt, config, engine_state, stack))
impl TransientPrompt {
fn new_prompt(&self) -> NushellPrompt {
if let Ok(prompt) = self.prompt_lock.read() {
prompt.clone()
} else {
NushellPrompt::new()
}
}
}
impl Prompt for TransientPrompt {
fn render_prompt_left(&self) -> Cow<str> {
let mut nu_prompt = NushellPrompt::new();
let mut nu_prompt = self.new_prompt();
let config = &self.engine_state.get_config().clone();
let mut stack = self.stack.clone();
nu_prompt.update_prompt_left(get_transient_prompt_string(
if let Some(s) = get_prompt_string(
TRANSIENT_PROMPT_COMMAND,
PROMPT_COMMAND,
config,
&self.engine_state,
&mut stack,
));
) {
nu_prompt.update_prompt_left(Some(s))
}
nu_prompt.render_prompt_left().to_string().into()
}
fn render_prompt_right(&self) -> Cow<str> {
let mut nu_prompt = NushellPrompt::new();
let mut nu_prompt = self.new_prompt();
let config = &self.engine_state.get_config().clone();
let mut stack = self.stack.clone();
nu_prompt.update_prompt_right(
get_transient_prompt_string(
TRANSIENT_PROMPT_COMMAND_RIGHT,
PROMPT_COMMAND_RIGHT,
config,
&self.engine_state,
&mut stack,
),
config.render_right_prompt_on_last_line,
);
if let Some(s) = get_prompt_string(
TRANSIENT_PROMPT_COMMAND_RIGHT,
config,
&self.engine_state,
&mut stack,
) {
nu_prompt.update_prompt_right(Some(s), config.render_right_prompt_on_last_line)
}
nu_prompt.render_prompt_right().to_string().into()
}
fn render_prompt_indicator(&self, prompt_mode: reedline::PromptEditMode) -> Cow<str> {
let mut nu_prompt = NushellPrompt::new();
let mut nu_prompt = self.new_prompt();
let config = &self.engine_state.get_config().clone();
let mut stack = self.stack.clone();
nu_prompt.update_prompt_indicator(get_transient_prompt_string(
if let Some(s) = get_prompt_string(
TRANSIENT_PROMPT_INDICATOR,
PROMPT_INDICATOR,
config,
&self.engine_state,
&mut stack,
));
nu_prompt.update_prompt_vi_insert(get_transient_prompt_string(
) {
nu_prompt.update_prompt_indicator(Some(s))
}
if let Some(s) = get_prompt_string(
TRANSIENT_PROMPT_INDICATOR_VI_INSERT,
PROMPT_INDICATOR_VI_INSERT,
config,
&self.engine_state,
&mut stack,
));
nu_prompt.update_prompt_vi_normal(get_transient_prompt_string(
) {
nu_prompt.update_prompt_vi_insert(Some(s))
}
if let Some(s) = get_prompt_string(
TRANSIENT_PROMPT_INDICATOR_VI_NORMAL,
PROMPT_INDICATOR_VI_NORMAL,
config,
&self.engine_state,
&mut stack,
));
) {
nu_prompt.update_prompt_vi_normal(Some(s))
}
nu_prompt
.render_prompt_indicator(prompt_mode)
.to_string()
@ -234,16 +235,17 @@ impl Prompt for TransientPrompt {
}
fn render_prompt_multiline_indicator(&self) -> Cow<str> {
let mut nu_prompt = NushellPrompt::new();
let mut nu_prompt = self.new_prompt();
let config = &self.engine_state.get_config().clone();
let mut stack = self.stack.clone();
nu_prompt.update_prompt_multiline(get_transient_prompt_string(
if let Some(s) = get_prompt_string(
TRANSIENT_PROMPT_MULTILINE_INDICATOR,
PROMPT_MULTILINE_INDICATOR,
config,
&self.engine_state,
&mut stack,
));
) {
nu_prompt.update_prompt_multiline(Some(s))
}
nu_prompt
.render_prompt_multiline_indicator()
.to_string()
@ -262,8 +264,13 @@ impl Prompt for TransientPrompt {
}
/// Construct the transient prompt
pub(crate) fn transient_prompt(engine_state: Arc<EngineState>, stack: &Stack) -> Box<dyn Prompt> {
pub(crate) fn transient_prompt(
prompt_lock: Arc<RwLock<NushellPrompt>>,
engine_state: Arc<EngineState>,
stack: &Stack,
) -> Box<dyn Prompt> {
Box::new(TransientPrompt {
prompt_lock,
engine_state,
stack: stack.clone(),
})

16
crates/nu-cli/src/repl.rs
View File

@ -29,7 +29,7 @@ use std::{
env::temp_dir,
io::{self, IsTerminal, Write},
path::Path,
sync::atomic::Ordering,
sync::{atomic::Ordering, Arc, RwLock},
time::Instant,
};
use sysinfo::SystemExt;
@ -68,7 +68,7 @@ pub fn evaluate_repl(
let mut entry_num = 0;
let mut nu_prompt = NushellPrompt::new();
let nu_prompt = Arc::new(RwLock::new(NushellPrompt::new()));
let start_time = std::time::Instant::now();
// Translate environment variables from Strings to Values
@ -269,6 +269,7 @@ pub fn evaluate_repl(
.with_ansi_colors(config.use_ansi_coloring)
.with_cursor_config(cursor_config)
.with_transient_prompt(prompt_update::transient_prompt(
Arc::clone(&nu_prompt),
engine_reference.clone(),
stack,
));
@ -424,7 +425,7 @@ pub fn evaluate_repl(
start_time = std::time::Instant::now();
let config = &engine_state.get_config().clone();
let prompt = prompt_update::update_prompt(config, engine_state, stack, &mut nu_prompt);
prompt_update::update_prompt(config, engine_state, stack, Arc::clone(&nu_prompt));
perf(
"update_prompt",
start_time,
@ -437,7 +438,14 @@ pub fn evaluate_repl(
entry_num += 1;
start_time = std::time::Instant::now();
let input = line_editor.read_line(prompt);
let input = {
line_editor.read_line(
&nu_prompt
.read()
.expect("Could not lock on prompt to pass to read_line")
.to_owned(),
)
};
let shell_integration = config.shell_integration;
match input {