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

ide style completions #696

Merged
merged 15 commits into from
Jan 18, 2024
Merged

ide style completions #696

merged 15 commits into from
Jan 18, 2024

Conversation

maxomatic458
Copy link
Contributor

@maxomatic458 maxomatic458 commented Jan 3, 2024

#660

demo clip: asciicast

@ysthakur
Copy link
Member

ysthakur commented Jan 3, 2024

This is so cool! Would it be possible to also show descriptions for each value when that particular value is highlighted? (like how IDEs show you doc comments when you go to a particular function)

@maxomatic458
Copy link
Contributor Author

yes im planing to implement it like inshellisense
so a description box to the right (or the left depending on how much space is available)

@fdncred
Copy link
Collaborator

fdncred commented Jan 3, 2024

wow, that looks super cool! nice work!

@maxomatic458
Copy link
Contributor Author

is there an easy way to add descriptions to a completer?
i cant find any way to do so. is that not implemented yet?

@fdncred
Copy link
Collaborator

fdncred commented Jan 4, 2024

is there an easy way to add descriptions to a completer? i cant find any way to do so. is that not implemented yet?

Seems like one of the types of completers has value and description.

image

This is how I've enabled this in nushell

menu

  {
    name: commands_menu
    only_buffer_difference: false
    marker: "# "
    type: {
      layout: columnar
      columns: 4
      col_width: 20
      col_padding: 2
    }
    style: {
      text: green
      selected_text: green_reverse
      description_text: yellow
    }
    source: { |buffer, position|
      scope commands
      | where name =~ $buffer
      | each { |it| {value: $it.name description: $it.usage} }
    }
  }

keybinding

  {
    name: commands_menu
    modifier: control
    keycode: char_t
    mode: [emacs, vi_normal, vi_insert]
    event: { send: menu name: commands_menu }
  }

@maxomatic458
Copy link
Contributor Author

that seems to be nushell specific.
reedlines default completer doesnt seem to have that implemented

i guess for testing purposes i can just add the descriptions in the menu

@fdncred
Copy link
Collaborator

fdncred commented Jan 4, 2024

I don't think nushell could have a menu on its own if reedline didn't support it. You can see here that the Suggestion struct supports a value and description.

/// Suggestion returned by the Completer
#[derive(Debug, Default, Clone, PartialEq, Eq)]
pub struct Suggestion {
/// String replacement that will be introduced to the the buffer
pub value: String,
/// Optional description for the replacement
pub description: Option<String>,
/// Optional vector of strings in the suggestion. These can be used to
/// represent examples coming from a suggestion
pub extra: Option<Vec<String>>,
/// Replacement span in the buffer
pub span: Span,
/// Whether to append a space after selecting this suggestion.
/// This helps to avoid that a completer repeats the complete suggestion.
pub append_whitespace: bool,
}

Nushell has this "convert_to_suggestion" function that may help if there's no example of how to do it in reedline itself.
https://github.com/nushell/nushell/blob/42bb42a2e1718469133fe3a71ac76c40389bbb30/crates/nu-cli/src/menus/menu_completions.rs#L76-L162

@maxomatic458
Copy link
Contributor Author

yes i have seen this, but the DefaultCompleter in reedline always returns None as description

Suggestion {
value: format!("{span_line}{ext}"),
description: None,
extra: None,
span,
append_whitespace: false,
}

reedline does support it, but the DefaultCompleter doesnt
so im just inserting some dummy values in the complete function for testing

@maxomatic458
Copy link
Contributor Author

so i implemented descriptions like this
asciicast

i made a enum to control where the description box shows up
https://github.com/maxomatic458/reedline/blob/94b7ebe829249c0ee3699b527ff90308bf7a258d/src/menu/ide_menu.rs#L9-L17

i also added an optional border
asciicast

@maxomatic458
Copy link
Contributor Author

should i add checks to make sure that the description or completion has a valid size? (maybe cut off some stuff if it doesnt fit)
Im asking because the other menus dont have any checks for that

@fdncred
Copy link
Collaborator

fdncred commented Jan 4, 2024

I thought there was a limit or it wrapped or something?

@maxomatic458
Copy link
Contributor Author

i added that for the description box currently
but not for the completions

i guess ill cut of strings that dont fit.
the list & column menus dont seem to check for that

@fdncred
Copy link
Collaborator

fdncred commented Jan 4, 2024

maybe truncate with ... indicating that they were truncated?

@maxomatic458 maxomatic458 marked this pull request as ready for review January 5, 2024 16:44
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

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

Comparison is base (2f3eb3e) 49.69% compared to head (e716386) 47.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #696      +/-   ##
==========================================
- Coverage   49.69%   47.33%   -2.37%     
==========================================
  Files          46       47       +1     
  Lines        8433     9304     +871     
==========================================
+ Hits         4191     4404     +213     
- Misses       4242     4900     +658     
Files Coverage Δ
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/painting/prompt_lines.rs 0.00% <0.00%> (ø)
src/menu/ide_menu.rs 25.11% <25.11%> (ø)

@maxomatic458 maxomatic458 marked this pull request as draft January 10, 2024 21:25
@fdncred
Copy link
Collaborator

fdncred commented Jan 11, 2024

@maxomatic458 Do you have time to clean up these CI errors?

@maxomatic458
Copy link
Contributor Author

@fdncred i will once im done, i marked it as draft again, i decided i want to change some stuff (more user input validation)

@fdncred
Copy link
Collaborator

fdncred commented Jan 11, 2024

Ah, ok. We just made a nushell release and we like to land things like this early after a release to give us time to test it. Not rushing you, just explaining how we usually work.

@maxomatic458 maxomatic458 marked this pull request as ready for review January 11, 2024 18:45
@maxomatic458
Copy link
Contributor Author

@fdncred i think i have fixed it

it works for me now using your config

$env.PROMPT_INDICATOR = {|| if ($env.LAST_EXIT_CODE == 0) {$"(ansi green_bold)\n❯ (ansi reset)"} else {$"(ansi red_bold)\n❯ (ansi reset)"}}
$env.TRANSIENT_PROMPT_COMMAND = ""
$env.TRANSIENT_PROMPT_INDICATOR = {|| if ($env.LAST_EXIT_CODE == 0) {$"(ansi light_yellow_bold)❯ (ansi reset)"} else {$"(ansi light_red_bold)❯ (ansi reset)"}}

could you give it a try now?

@fdncred
Copy link
Collaborator

fdncred commented Jan 18, 2024

@maxomatic458 I'm trying to build your nushell main and am getting this. Is there a different way I should try this?
image

@maxomatic458
Copy link
Contributor Author

@fdncred did you run cargo update?

@fdncred
Copy link
Collaborator

fdncred commented Jan 18, 2024

@maxomatic458 ok i did cargo update -p reedline and it compiles now (with a couple warnings) but doesn't run. I guess it's because the my gstat plugin maybe?

Please join our Discord community at https://discord.gg/NtAbbGn
Our GitHub repository is at https://github.com/nushell/nushell
Our Documentation is located at https://nushell.sh
Tweet us at @nu_shell
Learn how to remove this at: https://nushell.sh/book/configuration.html#remove-welcome-message

It's been this long since Nushell's first commit:
4yrs 8months 7days 22hrs 56mins 52secs 295ms 948µs 900ns

Startup Time: 1sec 251ms 217µs

 .cargo    │  assets  │  devdocs │  src   │  .gitattributes │  Cargo.toml         │  Cross.toml │  rust-toolchain.toml
 .githooks │  benches │  docker  │  tests │  .gitignore     │  CODE_OF_CONDUCT.md │  LICENSE    │  toolkit.nu
 .github   │  crates  │  scripts │  wix   │  Cargo.lock     │  CONTRIBUTING.md    │  README.md  │
toolkit module detected...
activating toolkit module with `use toolkit.nu`
Error:   × Plugin failed to decode
  help: invalid length 3, expected struct CallInfo with 4 elements


Error:   × Plugin failed to decode
  help: invalid length 3, expected struct CallInfo with 4 elements


Error:   × Plugin failed to decode
  help: invalid length 3, expected struct CallInfo with 4 elements

I'm going to try cargo clean/rebuild.

@fdncred
Copy link
Collaborator

fdncred commented Jan 18, 2024

oh, this is because your main may be out of date. we had a plugin change in the last week or so.

@maxomatic458
Copy link
Contributor Author

oh ok, ill sync my fork

@fdncred
Copy link
Collaborator

fdncred commented Jan 18, 2024

I think it's probably good enough to land but it's still weird that it doesn't start where my cursor is on the second line of my prompt. I just don't think that's something you've introduced, but has been there all along. So, I wouldn't expect you to fix that part in this PR.
ide

@maxomatic458
Copy link
Contributor Author

@fdncred
yes, same thing happens using any menu (with your config)
i think that happens because your base indicator contains a \n,
but your completion indicator doesnt

i think you could fix that by just adding a \n to your completion indicator (because the default indicator just gets replaced with the menu indicator)
so i think its not a bug, but just a weird config

@fdncred
Copy link
Collaborator

fdncred commented Jan 18, 2024

LOL. I do have a weird config. I'll try your suggestion as soon as I'm done rebuilding.

@fdncred
Copy link
Collaborator

fdncred commented Jan 18, 2024

That's mostly it but something else is going on too but that's for another PR.
image

Just to document. The odd thing is that this prints like the screenshot
marker: $" \n❯ (char -u '1f4ce') "
but this doesn't show the ❯ at all
marker: $"\n❯ (char -u '1f4ce') "

@fdncred fdncred merged commit 31eaeeb into nushell:main Jan 18, 2024
8 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Jan 18, 2024

Thanks for all your hard work on this PR! Now to enable it in nushell. :)

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

@maxomatic458 Is there a PR in nushell to use set_cursor_pos in nushell so we can build with this PR? (I don't see one) If not can you create one? Apparently, your PR is another PR nushell/nushell#11535 (comment) because it isn't implemented.

@maxomatic458
Copy link
Contributor Author

ill make one right now

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

awe, thank you so much! we really appreciate it.

@maxomatic458
Copy link
Contributor Author

@fdncred just made one nushell/nushell#11589

fdncred pushed a commit to nushell/nushell that referenced this pull request Jan 20, 2024
update to support the latest reedline changes from
nushell/reedline#696
@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

@maxomatic458 one last thing. I was trying to use this ide style menu and i'm getting errors in nushell. What am I getting wrong?

I added this to my menus in config.nu

  {
    name: completion_menu
    only_buffer_difference: false
    marker: $" \n❯ (char -u '1f4ce') "
    type: {
      layout: ide # the new ide style menu
      border: true
    }
    style: {
      text: { fg: "#33ff00" }
      selected_text: { fg: "#0c0c0c" bg: "#33ff00" attr: b}
      description_text: yellow
    }
  }

image
Maybe there are some other changed needed in nushell that I'm not seeing because I'm half asleep still. 😆

@maxomatic458
Copy link
Contributor Author

@fdncred yea there is no support currently for loading it from config.nu, its pretty easy to add (ive already done it in my fork, which i have deleted by accident)
i can make a PR for this later. But i think ill do this after #708 is merged (so multiline edits properly work)

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

ya, i was looking for your fork. LOL. i'm fine with waiting for 708. thanks!

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

Have you run into this? I can't seem to select a word+space, like " word" or "word " on my mac. I'm doing shift+ctrl+left and shift+ctrl+right in the gif. I could've swore i was able to do it on my windows machine.
select

@maxomatic458
Copy link
Contributor Author

on windows im able to do this here:
" word"
cursor at the start, then ctrl+shift+right
will select the whole string (including the space at the start)

same works with "word ", when the cursor is behind the the last space
do you think this might be related to this PR?

but i dont really see whats wrong in your clip,
do you mean that you would require an extra press of ctrl+shift+right to select the space as well?

@fdncred
Copy link
Collaborator

fdncred commented Jan 20, 2024

on mac, i was able to figure out that shift+left was doing the selection character by character. doing ctrl+shift+left is doing the selection word by word.

what i was expecting was that ctrl+shift+left would select character by character vs word by word. maybe that's my mistake.

I'm also wondering if terminal bindings come into play here with selection, like we saw with select_all previously with WT.

dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
update to support the latest reedline changes from
nushell/reedline#696
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.

4 participants