fix: columnar_menu create_string with quoted suggestions

[blindFS]

This is a stopgap for nushell/nushell#12680, nushell/nushell#13951, nushell/nushell#13630, nushell/nushell#15302.

Fuzzy matching will need more care, and I think @ysthakur knows how to fix that.

nushell/nushell#13951 also exposes another bug in directory_completion, i.e. not doing quoting, which is handled properly in file_completion.

[ysthakur]

Thanks, it'd be good to have at least a temporary fix for this bug. Could you also modify ide_completions? It has the same problem with split_at.

For fuzzy completions, you can iterate over the graphemes in every suggestion and highlight every grapheme that contains a character whose index is less than shortest_base.len(). I was working on a function to do that here. That one's more complicated than what you need to do because it takes a list of match indices rather than just highlight a prefix (the function's from #798).

[blindFS]

@ysthakur Thanks for the reminder! That's interesting, because I was looking at #839 and thought only columnar_menu had the problem, and I also greped create_string in the repo.

It turns out that ide_menu already got the width -> len fix somewhere else and the function name is create_value_string, LoL.

[blindFS]

For fuzzy completions, you can iterate over the graphemes in every suggestion and highlight every grapheme that contains a character whose index is less than shortest_base.len(). I was working on a function to do that here. That one's more complicated than what you need to do because it takes a list of match indices rather than just highlight a prefix (the function's from #798).

I was hoping for a systematic solution for both issues, I'm not sure if nucleo_matcher can directly return the matching indices while calculating the score so that we don't need some extra overhead here in reedline.

[ysthakur]

LGTM! At some point, I'd like to get rid of the code duplication, but that's for another PR.

---

Nucleo can indeed return the matched indices, it's just going to take a bit of redesigning to include that information in Suggestions. To clarify, I wasn't suggesting Reedline do the matching all over again.

[blindFS]

Nucleo can indeed return the matched indices, it's just going to take a bit of redesigning to include that information in Suggestions. To clarify, I wasn't suggesting Reedline do the matching all over again.

That was what in my mind, e.g. 1 more field matched_indices: Option<Vec<usize>>.

If specified, do the ansi decoration accordingly.
Fallback to the original split_at when it's None. In this way, we only need to provide that when 1. Fuzzy algo enabled, 2. Extra quote added. And the indices should be easier to compute where the suggestion item is created. What do you think?

[ysthakur]

@blindFS Yeah, that's what #798 was going to do. However, maxomatic suggested allowing more customizability and suggested a Styler trait. I sort of abandoned that PR, but last time I was working on it, I found that the Styler trait was too complicated, and that it was easier to add a field to Suggestion allowing the completer more control over styling (more control than just match_indices). I'll update that linked PR with my findings as soon as I can get back to it.

[blindFS]

@blindFS Yeah, that's what #798 was going to do. However, maxomatic suggested allowing more customizability and suggested a Styler trait. I sort of abandoned that PR, but last time I was working on it, I found that the Styler trait was too complicated, and that it was easier to add a field to Suggestion allowing the completer more control over styling (more control than just match_indices). I'll update that linked PR with my findings as soon as I can get back to it.

I see, I think we are on the exact same page here. Sorry I misunderstood what you're going to do because I just had a glance at the linked function code you gave me earlier and didn't follow the entire stream of #798. IMHO, extra styling options are nice, but if things get complicated, we shouldn't let that subtlety block this important bug fix, right?

[ysthakur]

Yup, I have a PR up (#887) for finishing this bug fix for fuzzy matching too. After that panic's fixed, we can work on figuring out adding match indices to Suggestion somehow.

[reubeno]

Just came here to say a huge thanks to @blindFS and @ysthakur for the change 👏 ! My project just got a report yesterday of a panic in the .split_at() call in ColumnarMenu, but unrelated to quotes. I see this change has .min() calls in just the right places to avoid walking past the end of the string -- and sure enough, that takes care of the issue we were seeing with completion suggestions shorter than the text-being-completed.

Can't wait for the fix to show up in the next official release! We're heavy (and happy!) users of reedline completion, so don't hestitate to let me know if y'all could ever use help with testing or fixes in this area.