feat: migrate and extend history search #60

ahkrr · 2021-06-17T10:07:19Z

This PR addresses this Issue Unify functionality for history search #55
BasicSearch was removed, and its behavior was migrated to the history module.
Additional features are an interactively displayed list, that is scrollable with CTRL-n/CTRL-r or CTRL-p
Maybe in the future make the displayed list optional?
The logic that makes this behavior possible is contained inside the history module.
The result is, that the main loop in readline_helper no longer needs to handle history_state and clean up in the right places

reedline_history.mp4

I wrote this PR against an earlier version of reedline. Now, pressing CTRL-r doesn't bring up the history_search anymore?

ahkrr · 2021-06-17T10:09:13Z

The list length is hardcoded to 8.
This should probably be migrated to 1/5 of the terminal-rows in the future, or something.

sholderbach · 2021-06-17T12:51:02Z

Having this preview list is awesome! From a maintenance perspective I am somewhat reserved towards pulling I/O into the History implementation. This makes dealing with different keybindings already tricky. If we could move the preview list as a separate modal widget running in the engine we could also use it for other purposes like completions or path jumplists.

sholderbach · 2021-06-17T12:54:33Z

I wrote this PR against an earlier version of reedline. Now, pressing CTRL-r doesn't bring up the history_search anymore?

Currently the example binary starts in EditMode::ViNormal for which the bindings are not yet complete. I was able to test it with EditMode::Emacs

diff --git a/src/main.rs b/src/main.rs
index 473d702..7f6173a 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -12,7 +12,7 @@ fn main() -> Result<()> {
 
     let mut line_editor = Reedline::new()
         .with_history("history.txt", 5)?
-        .with_edit_mode(reedline::EditMode::ViNormal)
+        .with_edit_mode(reedline::EditMode::Emacs)
         .with_keybindings(keybindings);
 
     let prompt = Box::new(DefaultPrompt::new(1));

ahkrr · 2021-06-17T12:57:47Z

Putting the logic back into the engine is fine.
I was thinking about alternative history implementations.
If a user wants to do some funky things with displaying the history (or in the future, completions)
they would probably want to write their own logic and not be married to the engine.
One could also put the default into the engine and check for a configuration-setting for a custum history_search.

ahkrr · 2021-06-17T13:00:08Z

What is the polling behavior I am seing when using reedline?

// src/engine.rs +866
        loop {
            if poll(Duration::from_secs(1))? {
                match read()? {
                    Event::Key(KeyEvent { code, modifiers }) => {
                        match (modifiers, code, self.edit_mode) {

When doing nothing reedline always prints at the end?

sholderbach · 2021-06-17T13:03:28Z

What is the polling behavior I am seing when using reedline?

This was added to update the time displayed in the prompt.

ahkrr · 2021-06-17T13:08:30Z

When printing stuff crossterm can set the cursor to Hide.
Before starting to reprint things, setting the cursor to Hide and after printing setting it to Show would probably be a benefit, because I am using alacritty, and I am seeing a flickering of the cursor because of the redrawings. This doesn't happen in all terminals.

ahkrr · 2021-06-17T13:13:38Z

If the cursor is positioned in the last line, this leads to scrolling of the screen because of some erroneous reprinting-logic?

sholderbach · 2021-06-17T13:24:26Z

I was thinking about alternative history implementations.

My primary concern when I factored the history into the History struct was to abstract away the handling of state that might be relative to the indices of the underlying data structure (I changed that around when dealing with the file). The BasicSearch was implemented beforehand and had interactions the exact indices and has both a separate EditCommand dispatch and draw call.

The history itself will need some work to deal with the problem of having possibly multiple sessions, thus separation of data representation and output wouldn't hurt if we change the data structure again.

If a user wants to do some funky things with displaying the history (or in the future, completions)
they would probably want to write their own logic and not be married to the engine.

Currently Engine kind of centralizes all provided ways to perform output and contains the input entry point dispatching to the keybindings/vi engine and then executes the resulting commands with what ever is provided as implementation. What would not fit into this framework (callback or trait based call into a plugin that then executes engine actions to output)?

One could also put the default into the engine and check for a configuration-setting for a custum history_search.

Having a seperate history search struct/trait object is not generally flawed and would definitely allow for customization.

sholderbach · 2021-06-17T13:26:19Z

That's a good call how we could improve the repaints! Yes as soon as the cursor reaches the bottom additional chaos ensues. Especially when resizing the current solution is unsatisfying.

ahkrr · 2021-06-17T13:27:18Z

I will start thinking how I can make the list-logic a reusable component.

* This PR addresses this Issue nushell#55 * BasicSearch was removed * Additional features are an interactively displayed list, that is scrollable with CTRL-n/CTRL-r or CTRL-p * Maybe in the future make the displayed list optional? * The interactive_history_search has its own loop, handling input events, to make more complicated behavior easier maintainable and removing history_search_state from the main loop in `read_line_helper`

ahkrr · 2021-06-18T11:56:17Z

I migrated the behavior into the engine and factored out the list-building.
I also fixed some issues with the scrolling behavior at the end of the screen.
Zooming into the terminal now works, i hope.
Zooming out of the terminal still eats lines.
I will think about how to fix that one.

by reusing local variable

ahkrr · 2021-06-18T12:53:41Z

With these changes, the terminal_size, prompt_offset and prompt_origin now need to be passed to run_edit_commands and adjusted appropriatly.
By making them part of Reedline, the need to pass them around would be removed?

sholderbach

This is certainly a complex PR, kudos for your efforts. I feel like the interactions between the input handling, the business logic of handling the search with suggestions and the size aware output are stronger than they need to be.
With the changes @jonathandturner and @sherubthakur are discussing in #61 and #62 on the horizon, how can we get this merged without introducing too many sharp edges for the the refactor?

Can we break this up into:

necessary state kept in the appropriate struct(s)
that gets updated upon receiving EditCommands or a resize requirement with simple methods
a special paint function that is active when searching and provides simply the visualization with the list.

sholderbach · 2021-06-18T13:09:11Z

src/engine.rs

+        prompt_origin: &mut (u16, u16),
+        prompt_offset: (u16, u16),
+        terminal_size: &mut (u16, u16),


If we need to keep track of those in a number of places, why not make them members of the Reedline engine? This would keep the API cleaner.

Additionally the EditCommands shouldn't modify the terminal size as only the resize event in the main event loop should need to set that and then maybe trigger components to provide a shrunk/expanded version of their content and then handle the relayout in the appropriate repaint.

sholderbach · 2021-06-18T13:18:44Z

src/engine.rs

+            match read()? {
+                Event::Key(KeyEvent { code, modifiers }) => match (modifiers, code) {
+                    (KeyModifiers::NONE | KeyModifiers::SHIFT, KeyCode::Char(c)) => {
+                        search_string.push(c);
+                        #[rustfmt::skip]
+                        let rows = self.paint_interactive_search( &search_string, search_index, &mut history_index, &mut prompt_offset_rows, )?;
+                        (rows < 8 && search_index >= rows)
+                            .then(|| search_index = rows.saturating_sub(1));
+                    }
+                    (KeyModifiers::NONE, KeyCode::Backspace) => {
+                        search_string.pop();
+                        #[rustfmt::skip]
+                        let rows = self.paint_interactive_search( &search_string, search_index, &mut history_index, &mut prompt_offset_rows, )?;
+                        (rows < 8 && search_index >= rows)
+                            .then(|| search_index = rows.saturating_sub(1));
+                    }
+                    (KeyModifiers::CONTROL, KeyCode::Char('n') | KeyCode::Char('r')) => {
+                        search_index += 1;
+                        #[rustfmt::skip]
+                        let rows = self.paint_interactive_search( &search_string, search_index, &mut history_index, &mut prompt_offset_rows, )?;
+                        (rows < 8 && search_index >= rows).then(|| search_index -= 1);
+                    }
+                    (KeyModifiers::CONTROL, KeyCode::Char('p')) => {
+                        search_index = search_index.saturating_sub(1);
+                        #[rustfmt::skip]
+                        self.paint_interactive_search( &search_string, search_index, &mut history_index, &mut prompt_offset_rows, )?;
+                    }
+                    (_, KeyCode::Enter) => {
+                        if let Some(idx) = history_index {
+                            self.line_buffer
+                                .set_buffer(self.history.get_nth_newest(idx).unwrap().clone());
+                            self.line_buffer.move_to_end();
+                        }
+                        // adjust the prompt_offset.1 from the main loop if needed
+                        *prompt_origin_row = prompt_origin_row.saturating_sub(prompt_offset_rows);
+                        return Ok(());
+                    }
+                    (_, KeyCode::Esc) => {
+                        // adjust the prompt_offset.1 from the main loop if needed
+                        *prompt_origin_row = prompt_origin_row.saturating_sub(prompt_offset_rows);
+                        return Ok(());
+                    }
+                    _ => {}


Can we break this apart into the handlers of the appropriate EditCommands?

In this setup the arrow keys or abort with Ctrl+c are not yet added but would be trivially mapped with EditCommand to the respective keybindings.

sholderbach · 2021-06-18T13:27:50Z

src/engine.rs

+    let mut search_results = items
+        .enumerate()
+        .skip(list_index.saturating_sub(7))
+        .filter(|(_, entry)| entry.as_ref().starts_with(&*search_string))
+        .enumerate()
+        .take(8)
+        .peekable();


I wouldn't expect that the actual search takes place here. There rest of the function seems to be concerned with creating the string for the list visualization.

Maybe I will call it run_search_and_build_list, because the filtering needs to be done repeatedly (on changes to the search_string) and the implementation depends on the different indices that are created through enumerate.
If I factor out even more functionality, only the pushing of the different entries to the list would be left.

sholderbach · 2021-06-18T13:38:27Z

src/engine.rs

+        let mut prompt_offset_rows = 0;
+        let mut history_index = None;
+
+        #[rustfmt::skip]


As soon as you have iterated on your signature, I don't think there is a reason to deviate from standard formatting.

ahkrr · 2021-06-18T14:28:16Z

I just looked at #62.
At this point I think I will leave this PR open and revisit it and the suggestions after the refactor and integrate it into the new design.

sholderbach · 2021-06-18T14:34:50Z

Sure no problem!
Would you mind filing your fix for the scrolling issue as a separate PR?

ahkrr · 2021-06-18T14:42:00Z

Thanks for the quick replies.
I will get to the scrolling fixes in a couple hours from now.

ahkrr · 2021-06-18T14:46:20Z

I will integrate it after #62.
Otherwise they need to change things in their current draft.

nixypanda · 2021-06-23T12:20:24Z

I will integrate it after #62.
Otherwise, they need to change things in their current draft.

#62 is mostly to discuss ideas, I would not recommend waiting on that PR. I would like to revisit that PR in the future, One of the things that I wanted to do was merging history and history search, and obviously will like it even more if someone else does it :P. So please go forward with this PR :)

sholderbach self-requested a review June 17, 2021 12:25

ahkrr force-pushed the main branch from ea0cc22 to f9a60bf Compare June 18, 2021 11:52

ahkrr added 4 commits June 18, 2021 14:06

reduce code duplication

aa0c077

fix: adding terminal_size adjustment back

4cccdec

reduce number of function_calls

da5152c

by reusing local variable

added TODO

45d63e1

ahkrr marked this pull request as ready for review June 18, 2021 12:37

adjust terminal_size after interactive_history_search

74cd304

sholderbach requested changes Jun 18, 2021

View reviewed changes

sholderbach mentioned this pull request Jun 18, 2021

Code Organisation/Design Discussion: Making Editing Mode do more work #61

Closed

nixypanda mentioned this pull request Jul 1, 2021

[Design][Pluggable Components] History and searching #86

Closed

ahkrr closed this Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate and extend history search #60

feat: migrate and extend history search #60

ahkrr commented Jun 17, 2021

ahkrr commented Jun 17, 2021

sholderbach commented Jun 17, 2021

sholderbach commented Jun 17, 2021

ahkrr commented Jun 17, 2021

ahkrr commented Jun 17, 2021 •

edited

Loading

sholderbach commented Jun 17, 2021

ahkrr commented Jun 17, 2021 •

edited

Loading

ahkrr commented Jun 17, 2021

sholderbach commented Jun 17, 2021

sholderbach commented Jun 17, 2021

ahkrr commented Jun 17, 2021

ahkrr commented Jun 18, 2021

ahkrr commented Jun 18, 2021

sholderbach left a comment

sholderbach Jun 18, 2021

sholderbach Jun 18, 2021

sholderbach Jun 18, 2021

ahkrr Jun 18, 2021

sholderbach Jun 18, 2021

ahkrr commented Jun 18, 2021

sholderbach commented Jun 18, 2021

ahkrr commented Jun 18, 2021

ahkrr commented Jun 18, 2021

nixypanda commented Jun 23, 2021

feat: migrate and extend history search #60

feat: migrate and extend history search #60

Conversation

ahkrr commented Jun 17, 2021

ahkrr commented Jun 17, 2021

sholderbach commented Jun 17, 2021

sholderbach commented Jun 17, 2021

ahkrr commented Jun 17, 2021

ahkrr commented Jun 17, 2021 • edited Loading

sholderbach commented Jun 17, 2021

ahkrr commented Jun 17, 2021 • edited Loading

ahkrr commented Jun 17, 2021

sholderbach commented Jun 17, 2021

sholderbach commented Jun 17, 2021

ahkrr commented Jun 17, 2021

ahkrr commented Jun 18, 2021

ahkrr commented Jun 18, 2021

sholderbach left a comment

Choose a reason for hiding this comment

sholderbach Jun 18, 2021

Choose a reason for hiding this comment

sholderbach Jun 18, 2021

Choose a reason for hiding this comment

sholderbach Jun 18, 2021

Choose a reason for hiding this comment

ahkrr Jun 18, 2021

Choose a reason for hiding this comment

sholderbach Jun 18, 2021

Choose a reason for hiding this comment

ahkrr commented Jun 18, 2021

sholderbach commented Jun 18, 2021

ahkrr commented Jun 18, 2021

ahkrr commented Jun 18, 2021

nixypanda commented Jun 23, 2021

ahkrr commented Jun 17, 2021 •

edited

Loading

ahkrr commented Jun 17, 2021 •

edited

Loading