Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add a way to spawn populated LineEditor #6007

Closed
wants to merge 2 commits into from

Conversation

ekorchmar
Copy link
Contributor

Closes #6002

Adds a new constructor for LineEditor, that allows to specify text to pre-populate the input.
Also removes a call to line.clear() in a method to initiate editing, as it is currently only ever called after editor struct had been newly spawned.

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Please rebase this PR as there is a partial conflict with #6054

I have a couple of suggestions, but this otherwise looks good!

config/src/keyassignment.rs Outdated Show resolved Hide resolved
docs/config/lua/keyassignment/PromptInputLine.md Outdated Show resolved Hide resolved
termwiz/src/lineedit/mod.rs Outdated Show resolved Hide resolved
wezterm-gui/src/overlay/prompt.rs Outdated Show resolved Hide resolved
ekorchmar and others added 2 commits September 22, 2024 21:14
feat: support pre-filled content in prompts

docs: document new PromptInputLine parameter

fix: remove reset of line contents
@ekorchmar
Copy link
Contributor Author

Rebased onto new master and implemented requested changes.

@@ -20,6 +20,9 @@ upon the input.
anything, or CTRL-C to cancel the input.
* `prompt` - the text to show as the prompt. You may embed escape sequences
and/or use [wezterm.format](../wezterm/format.md). Defaults to: `"> "`. {{since('nightly', inline=True)}}
* `initial_value` - optional. If provided, the initial content of the input
field will be set to this value. The user may edit it prior to submitting
the input.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's tag this with the version it was introduced with:

Suggested change
the input.
the input. {{since('nightly', inline=True)}}

Comment on lines 813 to -814
fn read_line_impl(&mut self, host: &mut dyn LineEditorHost) -> Result<Option<String>> {
self.line.clear();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I didn't spot this initially. I think this is a big semantic change that breaks behavior. You can probably see this in the debug overlay (ctrl-shift-L) if you type in foo and hit enter. Does it cause foo to show up in the next line?

What I think should be done here is to adjust this method:

fn read_line_impl(&mut self,
    host: &mut dyn LineEditorHost,
    initial_value: Option<&str>
) -> Result<Option<String>> {
    self.line.clear();
    if let Some(value) = initial_value {
        self.line.set_line_and_cursor(value, value.len());
    }

Then, refactor:

pub fn read_line(&mut self, host: &mut dyn LineEditorHost) -> Result<Option<String>>

into

pub fn read_line_with_optional_initial_value(&mut self, host: &mut dyn LineEditorHost, initial_value: Option<&str>) -> Result<Option<String>>

that calls the above function:

...
let res = self.read_line_impl(host, initial_value);
...

Then finally add back the original method, but make it call the refactored one:

pub fn read_line(&mut self, host: &mut dyn LineEditorHost) -> Result<Option<String>> {
   self.read_line_with_optional_initial_value(host, None)
}

Note that the public methods on this struct need to have doc comments because they are part of the public API that is published to https://docs.rs/termwiz/latest/termwiz/lineedit/struct.LineEditor.html

Copy link
Contributor Author

@ekorchmar ekorchmar Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a big semantic change that breaks behavior. You can probably see this in the debug overlay (ctrl-shift-L) if you type in foo and hit enter. Does it cause foo to show up in the next line?

From what I can test, it does not appear as if removal of this line causes old contents to appear anywhere; I tested various combinations of launching REPL, custom editor, pressing enter, cancelling input with Esc and Ctrl+C and other things.

If I read the code correctly, in every instance where a LineEditor struct is spawned, it's read_line method is only ever called once, with no loops. As such, the "clear" function does not actually do anything: it only ever sees an already empty, freshly created line.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The termwiz crate is a public export and used by others, so just looking at the usage within wezterm is not sufficient to determine that nothing is impacted by this change.

I went ahead and merged this with my suggested changes.

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. Thank you for reviewing this

Comment on lines +71 to 74
if let Some(value) = &args.initial_value {
editor.set_line_and_cursor(value, value.len());
}
let line = editor.read_line(&mut host)?;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the above changes, this would become:

Suggested change
if let Some(value) = &args.initial_value {
editor.set_line_and_cursor(value, value.len());
}
let line = editor.read_line(&mut host)?;
let line = editor.read_line_with_optional_initial_value(&mut host, args.initial_value.as_deref())?;

@wez wez closed this in 67603e7 Sep 22, 2024
wez added a commit that referenced this pull request Sep 22, 2024
@ekorchmar ekorchmar deleted the prompt branch September 23, 2024 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to pre-populate user prompts with custom text
2 participants