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

get correct cursor pos when menu indicator contains newline #708

Merged
merged 7 commits into from
Jan 20, 2024

Conversation

maxomatic458
Copy link
Contributor

fixes a bug from #696

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 654 lines in your changes are missing coverage. Please review.

Comparison is base (2f3eb3e) 49.69% compared to head (75bdae3) 47.59%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #708      +/-   ##
==========================================
- Coverage   49.69%   47.59%   -2.11%     
==========================================
  Files          46       47       +1     
  Lines        8433     9312     +879     
==========================================
+ Hits         4191     4432     +241     
- Misses       4242     4880     +638     
Files Coverage Δ
src/painting/prompt_lines.rs 19.62% <100.00%> (+19.62%) ⬆️
src/menu/columnar_menu.rs 33.02% <0.00%> (-0.19%) ⬇️
src/menu/list_menu.rs 8.69% <0.00%> (-0.06%) ⬇️
src/menu/mod.rs 4.60% <0.00%> (-0.10%) ⬇️
src/painting/painter.rs 18.68% <0.00%> (+0.19%) ⬆️
src/engine.rs 5.05% <0.00%> (-0.03%) ⬇️
src/menu/ide_menu.rs 25.11% <25.11%> (ø)

... and 1 file with indirect coverage changes

@fdncred
Copy link
Collaborator

fdncred commented Jan 19, 2024

it would be great if we had a test for this to ensure no breakage in the future.

/cc @stormasm

@stormasm
Copy link
Contributor

stormasm commented Jan 19, 2024

I am not sure this is working correctly as I tested it by modifying your ide_completions example with the following code

fn add_newline_keybinding(keybindings: &mut Keybindings) {
    keybindings.add_binding(
        KeyModifiers::ALT,
        KeyCode::Enter,
        ReedlineEvent::Edit(vec![EditCommand::InsertNewline]),
    );
}

and then this code as well...

   let mut keybindings = default_emacs_keybindings();
    add_menu_keybindings(&mut keybindings);
    add_newline_keybinding(&mut keybindings);
    let edit_mode = Box::new(Emacs::new(keybindings));

similar to the way it works in the demo example...

and then tried typing one letter basically just the "t"

and then insert one new line and then hit tab...
and then insert one new line and then hit tab...
and keep doing that and see what you are seeing...

See if it works properly on your machine / I am on a Mac

After the third new line is entered I am not seeing test anymore

I just see "this is the reedline crate"

@maxomatic458
Copy link
Contributor Author

should be fixed now

i found another problem (not specific to my menu)
if you enter a bunch of newlines (so many until the prompt is not visible anymore), then the cursor will enter the menu

here is a screenshot of the columnar menu, basically using the same code as above
grafik

@maxomatic458
Copy link
Contributor Author

After the third new line is entered I am not seeing test anymore

im not to sure what you mean by that?
can you replicate this in other menus?

@stormasm
Copy link
Contributor

stormasm commented Jan 19, 2024

So check out the demo example

Type an "h" at the prompt and you will see another "h" show up on the screen many lines down probably where the menu ends...

This is a separate issue than the one I mentioned earlier...
But lets address this one first. Which seems more serious (possibly)...

@maxomatic458
Copy link
Contributor Author

@stormasm so i tried that
asciicast

but i dont think i was able to replicate it. Did i do something wrong? (did you also try the latest commit?)

if you referred to the cut off at the bottom, then thats intended (you can scroll throught the options that dont fit on the screen)

@maxomatic458
Copy link
Contributor Author

Type an "h" at the prompt and you will see another "h" show up on the screen many lines down probably where the menu ends...

this might be somewhat intentional, i think thats just a multiline history suggestion. but you cant hit tab to complete it, so i guess thats bugged, and it probably shouldnt even show up if you are not in multiline mode

@maxomatic458
Copy link
Contributor Author

asciicast

@stormasm
Copy link
Contributor

This is your example -> ide_completions.rs modified to show whats going on...

https://github.com/stormasm/nutmp/blob/main/reedline/ide_completions.rs

Note that after the third input line the first line "test" goes away --- this does not seem correct to me...

pr708-0119.mov

@stormasm
Copy link
Contributor

stormasm commented Jan 20, 2024

Here is a slightly longer video --- you will note that after the third InsertNewLine that "test" for some reason disappears...

pr708-01-19-take2.mov

@maxomatic458
Copy link
Contributor Author

maxomatic458 commented Jan 20, 2024

as soon as i enter a newline i get "NO RECORDS FOUND" until i enter text in the current line,

i start the example
and i press these buttons
t, tab, ALT+ENTER

i also tried this on the columnar menu and it does the same

can you check if this happens for other menus as well?
is it even intentional that the completion continues over multiple lines?

@maxomatic458
Copy link
Contributor Author

ok i was able to replicate it, by using the unix linebreaks
the same thing happens on the columnar menu as well

@maxomatic458
Copy link
Contributor Author

@stormasm
ok so it looks like this line here

let trimmed_buffer = editor.get_buffer().replace('\n', " ");

causes this to only happen on unix (only replace "\n", not "\r\n")

changing the line to

let trimmed_buffer = editor.get_buffer()
    .replace("\r\n", "  ")
    .replace("\n", " ");

will lead to it to work on windows as well. but there is still the issue that some of the suggestion dissapear after a few lines (like your clip above)

i think this might be something for a seperate PR (as this isn't caused by the ide menu, and happens in the other menus as well)

should i open up a seperate issue for this?

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

Ah, line endings. we've seen that time and time again in reedline and nushell, since we're cross platform. nice catch.

Let's see what @stormasm says but imo, I think I'd put this change

let trimmed_buffer = editor.get_buffer()
    .replace("\r\n", "  ")
    .replace("\n", " ");

in a separate PR in case we have to revert it (or this current PR) for some odd reason.

@maxomatic458
Copy link
Contributor Author

just squeezed this in here as well in preperation for full support to nushell

@stormasm
Copy link
Contributor

Ah, line endings. we've seen that time and time again in reedline and nushell, since we're cross platform. nice catch.

Let's see what @stormasm says but imo, I think I'd put this change

let trimmed_buffer = editor.get_buffer()
    .replace("\r\n", "  ")
    .replace("\n", " ");

in a separate PR in case we have to revert it (or this current PR) for some odd reason.

I will defer to @fdncred for the call on what to do here as I don't know what the best way to go is...

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

@maxomatic458 can you open another pr for that line ending change? We'll land that real quick.

@maxomatic458
Copy link
Contributor Author

@fdncred i just made #709

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

does this need to be rebased to accommodate the line ending pr?

@maxomatic458
Copy link
Contributor Author

@fdncred i don't think so

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

Ok. Let's give it a try.

@fdncred fdncred merged commit 9ca229d into nushell:main Jan 20, 2024
8 checks passed
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.

3 participants