menu refactor

[maxomatic458]

remove code duplication in the menus using the MenuCommon struct and a MenuBuilder trait.
related #706.

[maxomatic458]

the api for the menus is pretty much the same. the only breaking change is that the common builder functions (with_name, with_marker...) are now in a trait.

[fdncred]

Thanks @maxomatic458. We'll take a look at this soon.

/cc @ysthakur I think you were thinking about going in this direction too. Your review would be helpful.

[ysthakur]

I'll take a look too. I was actually working on that issue too, just hadn't made a PR yet, lol. I only made some helpers, though, so this PR and my branch aren't doing the same thing

[fdncred]

It might be good for @maxomatic458 and @ysthakur to join forces on this PR somehow because, like we said in the meeting on Wednesday, we want to avoid breaking changes as much as possible.

[maxomatic458]

i mostly got rid of code duplication by moving the builder methods in a trait (which i guess is somewhat a breaking change)
If you are ok with that, then i can move this PR over to @ysthakur's fork.

[ysthakur]

That'd be cool. I'd made a completer_input helper function to help with update_values (gets the input that needs to be passed to the completer), a replace_in_buffer helper, and a can_partially_complete helper (for the columnar and IDE menus). They're all after this line in menu_functions.rs. completer_input and can_partially_complete could now become methods on MenuCommon. Caveat: the completer_input helper I made doesn't replace newlines, because I think that might screw up the history completer. Instead, I changed the DefaultCompleter to replace \r and \n with spaces. Other completers would also have to be updated to handle newlines.

Also, to avoid breaking changes, would it be possible to keep Menu the same as before, but have a MenuWithCommon trait for Menus that have a MenuCommon? And then there could be an implementation of Menu for any menu that implements MenuWithCommon. Not sure if that's doable in Rust. It would still mean that anyone building the columnar, list, or IDE menus would need to use the new MenuBuilder, but it'd be less of a breaking change, at least

[ysthakur]

If you are ok with that, then i can move this PR over to @ysthakur's fork.

@maxomatic458 No need, you can take the stuff from my fork instead since this PR is already up

[stormasm]

@maxomatic458 @ysthakur once you all are in a good state with this PR I will review and test as well...
thanks for taking this one for a spin 😄
it will certainly help clean up the code a bit...

[stormasm]

@maxomatic458 I reviewed your code so far and it looks really good ! Great job...

[stormasm]

@ysthakur all great suggestions --- thanks for those and I am sure @maxomatic458 and you will get the code to a place you are both happy with...

We are also not in any hurry to land this code --- its much more important that you feel comfortable with the changes you both are making --- so take as much time as you both need to figure out the best path forward...

I merely did the nushell branch just so we can see so far what compiler errors are showing up on the nushell side of things.

Thanks to both of you for pursuing the refactor --- its looking good so far...

[maxomatic458]

Ok, so i made a MenuSettings struct that looks like this

pub struct MenuSettings {
    /// Menu name
    name: String,
    /// Menu coloring
    color: MenuTextStyle,
    /// Menu marker when active
    marker: String,
    /// Calls the completer using only the line buffer difference difference
    /// after the menu was activated
    only_buffer_difference: bool,
}

which gets rid of all the duplicate builder functions

and i made these changes here to the Menu trait

pub trait Menu: Send {
    /// Get MenuSettings
    fn settings(&self) -> &MenuSettings;

    /// Menu name
    fn name(&self) -> &str {
        &self.settings().name
    }

    /// Menu indicator
    fn indicator(&self) -> &str {
        &self.settings().marker
    }
    ...

to also get rid of those two functions, but this is a breaking change, because now each menu needs a implementation for settings (which could be made optional via the panic approach)

and i also added all the changes from @ysthakur

and for nushell, you basically just need to bring the MenuBuilder trait into scope and thats pretty much it

[stormasm]

@maxomatic458 @ysthakur Super fantastic job !!

I got everything compiling on my nushell branch --- and ran through all of the menu types on the Nushell side of the world...

https://github.com/stormasm/nushell/tree/menu_refactor_01

Everything appears to be working fine... Although it is preliminary testing, I haven't found any issues so far...

The nushell side of the world could probably use a bit more testing....
before we land this PR on the Reedline side!

Would you all mind running nushell off my branch which is up and running and working...

And see if you see any oddities / weirdness on the nushell side of the world...

Once you both give me the thumbs up (that you have tested my nushell branch) I will land the Reedline PR...

Then we will finalize the nushell side of the world to make sure that is good...

And then I will land the nushell side after that....

Again --- wow ! really cool 😄

@fdncred if you feel like doing some testing as well that would be cool...

[maxomatic458]

found an unrelated bug #727 (but thats on that also happens on the current reedline version)
tested all menu types with different configs and didnt find any bugs (apart from that)

[fdncred]

Were we able to avoid breaking changes?

[maxomatic458]

Were we able to avoid breaking changes?

currently the only breaking changes are:

• MenuBuilder trait needs to be imported where the builtin reedline menus are built
• Menu trait has a new function that needs to be implemented (that could be fixed with a base implementation see above, e.g by panicing)

[ysthakur]

@maxomatic458 Everything looks good to me too! I guess we'll have to go with panicking since there's no other obvious ways to avoid a breaking change.

Were we able to avoid breaking changes?

@fdncred The completer_input helper that was added to help with update_values doesn't replace newlines with spaces anymore. Either the columnar, IDE, and description menus could do the newline-replacing before calling that helper, or we could leave it to be handled by completers downstream (DefaultCompleter in Reedline has already been changed to handle newlines)

[stormasm]

Were we able to avoid breaking changes?

Breaking changes were pretty much non-existent except for some very minor issues which I already dealt with in my Nushell branch....

[maxomatic458]

@stormasm i added a panic, so existing menu implementations (outside of nushell) will not break.
should i leave it in or should i remove it?

[stormasm]

@stormasm i added a panic, so existing menu implementations (outside of nushell) will not break. should i leave it in or should i remove it?

Lets leave it in for now....
So that existing menu implementations will not break....
If we come across other implementations that don't like the idea then we can re-address at that time...

And see if anyone complains later 😄
Then we can worry about it...
And deal with it...
Unless others feel strongly the other way...
@maxomatic458 What do you think we should do ?

[stormasm]

@maxomatic458 @ysthakur
I will plan on landing your PR sometime tomorrow morning (Sunday) my time --- I live in Oregon, USA

I will test your branch more this evening and in the morning...
And then land your Reedline PR

Then I will play around with the Reedline main branch in nushell tomorrow post landing...

And plan on updating Nushell with the Reedline main branch Sunday evening or Monday morning...
If no one who is testing finds any issues...

Unless someone disagrees --- or for some reason you all don't feel its ready...

This has been an incredible job ! I am impressed 😄

Thanks kindly for both of your efforts...
I think the menu code base will be a lot more solid moving forward...
Meaning it will be a lot easier for us to maintain / others to use and understand better
and just overall a much cleaner menu design for the entire Reedline community...

[ysthakur]

I'd like to add a couple test cases for the can_partially_complete helper before then. It's not a priority, since the columnar menu already has tests for that and we'd just be moving them from there into menu_functions.rs, but it might make more sense for tests for that helper to live with the other tests for helpers in menu_functions.rs rather than with the tests specific to the columnar menu.

[ysthakur]

You know what, never mind, it's fine if the can_partially_complete tests live in columnar_menu.rs. I don't want to delay this PR further.

[fdncred]

we don't have to do it in this PR but it would be great to have more tests. it seems that tests are the only thing these days that guarantees other changes don't affect previous changes. i know reedline is kind of a special case since it's a line editor but we need to figure out how to do more thorough testing and have more coverage in our tests. i don't want to hold this PR up either but it would be nice to get back to tests at some point.

[ysthakur]

@fdncred Absolutely, it just so happens that the columnar menu is already thoroughly testing can_partially_complete, and I think the other two helpers we added have all their edge cases covered too. There is the edge case of calling replace_in_buffer where the insertion point isn't the same as the end of the span that's being replaced, but I'm not sure how that'd happen or what the expected behavior should be in that situation (currently, it moves the insertion point around according to the offset but doesn't mess anything up)

[fdncred]

That's good to hear. Thanks.

[stormasm]

@maxomatic458 @ysthakur We are up and running in nushell with your newest amazing menu code !! 😄