Fix panic in menus

[maxomatic458]

closes #769

[maxomatic458]

now also fixes #774

[sholderbach]

Why do we need to trim the string and thus change the length here?

[maxomatic458]

shortest_base_string can include trailing whitespace from the buffer, because the completer will ignore whitespace, when making a completion. "test " -> will complete the same as "test" but will have a different span. so we need to trim it, so 4 chars match and not 5.

[sholderbach]

As the self.working_details.shortest_base_string succeeded to be generated in self.update_values() (the range coming from Completer::complete_with_base_ranges did not violate char boundaries which would panic the SliceIndex)

reedline/src/menu/columnar_menu.rs

Lines 497 to 504 in 46f410b

	let (values, base_ranges) = completer.complete_with_base_ranges(&input, pos);
	self.values = values;
	self.working_details.shortest_base_string = base_ranges
	.iter()
	.map(|range| editor.get_buffer()[range.clone()].to_string())
	.min_by_key(|s| s.len())
	.unwrap_or_default();

having to do char based indexing, indicates to me that shortest_base_string seems to be not a strict prefix of suggestion.value.
Don't know if that is really the intended semantics to then split here based on length.

[maxomatic458]

the ranges from Completer::complete_with_base_ranges (and pretty much all buffer related stuff) are byte based not character based, to support unicode we need to do char indexing here.

shortest_base_string seems to be not a strict prefix of suggestion.value

thats actually somewhat true if you use other completion methods (instead of the default starts_with)
if thats the case if match_len <= suggestion.value.chars().count() will most likely fail and not style the matching string at all.

[sholderbach]

If the byte indxes are wrong it should already fail on the index in L502, if I remember the slice index semantics of str correctly.

So this whole section doing the match highlighting either needs to be aware of which matching mode was used, get the correct coordinates to highlight or perform some alignment to check which characters in the query match to the picked Suggestion.

[maxomatic458]

Im pretty sure the indexing in L502 wont fail because the range is directly coming from the completer (doesnt matter which completion mode was used)

if we wanted to add support for other completion methods then we would need to be aware of that, but currently those cases are just ignored (so if the check failes, the text remains unstyled)

Im not planning to add support for other completion methods in this, i just want to make them not crash the styling system. (and support unicode)

full support for other completion methods would be something for another PR

[sholderbach]

Im pretty sure the indexing in L502 wont fail because the range is directly coming from the completer (doesnt matter which completion mode was used)

The Completer implementer can deliver invalid (i.e. not byte-encoding aware or out-of bounds) ranges in its span (or completely override complete_with_base_ranges).
But this would panic L502 as it tries to index the buffer &str. Indexing outside the encoded chars/bounds of a str is a panic
So the culprit for what you a trying to fix is not a malformed span from the Completer (which should be fixed at that source first and then hardened here in a way that still informs implementers).

if you use other completion methods (instead of the default starts_with)
if thats the case if match_len <= suggestion.value.chars().count() will most likely fail and not style the matching string at all.

We shouldn't accept a highlighting system that only stops to highlight mismatching prompted text and suggestion under the circumstance that something else is fishy.

The highlighting should mark the text that is equal/equivalent between the user query and the suggestions.
And the assumption that the user query is a prefix of all suggestions is not true.

(I don't have the time to do a full archaeology on #730 or before if the shortest_base_string is the right proxy to reflect the matching text itself)

[maxomatic458]

I think ill close this PR for now and make another one where i will implement your suggestions. (so add proper support for different completion methods and figure out a safer way to get the base ranges)

[fdncred]

Based on your input @maxomatic458, I'm going to close this. I look forward to seeing your next one. Thanks!

[RGBCube]

closed as completed, is this fixed? if so, we should close the tracking issues

[fdncred]

@RGBCube I closed it based on this comment #782 (comment)