Refactor Menu System with Composition of Menu Functions

[stormasm]

columnar_menu.rs

    pub fn with_selected_text_style(mut self, selected_text_style: Style) -> Self {
        self.color.selected_text_style = selected_text_style;
        self
    }

list_menu.rs

   pub fn with_selected_text_style(mut self, selected_text_style: Style) -> Self {
        self.color.selected_text_style = selected_text_style;
        self
    }

ide_menu.rs

    pub fn with_selected_text_style(mut self, selected_text_style: Style) -> Self {
        self.color.selected_text_style = selected_text_style;
        self
    }

All three of our menu types have the exact same code in their implementations...

And this is true across all of our menu_functions lending itself to massive code duplication in our menu system.

As we continue to add new types of menus its only going to get worse, meaning in the future when we want to
refactor the menu system its going to be a lot more work...

Ideally we would compose this functionality so we only have one copy of the above code instead of three copies
and the composition style would lead to just adding new code for the functions that are unique to a particular menu
type.

[ysthakur]

To add on to this, only_buffer_difference is also handled pretty much the same way in all the menus, including description_menu, I think

[stormasm]

@ysthakur I am really happy you are going to start taking a look at this issue.

Here are some other things to keep in mind as you go down this path.

These might be longer term goals of where we want to possibly go than
what is possible in your first PR to address this issue but keeping
it in mind might be helpful in the design decisions you make.

Longer term we should probably consolidate the location of all of our menus into one place.

Meaning the description menu that is now located in the nushell repo and
the reedline menus should probably come together...

Not sure the best location of this code but options would include...

• Moving the description menu over to reedline from nushell
• Having a separate menu crate that lives in the reedline repo ?
• Having a separate menu crate that lives inside the nushell organization
  so that both reedline and nushell have access to them along with other
  projects that want a terminal based menu system.

Which brings me to my next point 😄

It might be nice to think of this effort longer term as having a nice API
where completers and menus work together and is available to the broader
rust community... Just an idea but keeping this in mind in your design
might be helpful.

The reason I bring this up is because @maxomatic458 is considering broadening
the scope of how completers work and maybe some brainstorming is in order
for thinking about what the menu system needs from a completion point of
view and what the API should look like ?

#711

In the nushell community we always like to crawl, walk, run so I am not
saying this work needs to be done. But its always nice to have bigger
ideas of where we are possibly going with this work.

Of course we are all having fun and all of our efforts are greatly appreciated.

[ysthakur]

I don't know if there's already a discussion going somewhere, but in case there isn't, I made a HackMD for brainstorming, would love to get more of your opinions @stormasm (cc: @maxomatic458, since you're working on improving completers) (also, just to clarify, I wasn't planning on making a PR all by myself anytime soon, just wanted to be involved in the discussion 🙂)

Moving the description menu into the same crate as the other menus makes sense to me too. Is there any reason to make a separate crate just for the Menu trait and its implementations, though? (I figured such a crate wouldn't be used by anyone other Reedline and Reedline users)

[stormasm]

@ysthakur Thanks for you comments and the HackMd write up as well...

As far as the crate idea is concerned thats probably further down the road than the work proposed in this issue and the Hackmd document...

The concept of having a crate is that if the Menu System API was well designed and generic enough it could be used by other Line Editor and CLI Applications that wanted an off the shelf ready built Menu and Completer System that was not tied to Reedline...

By having a Menu and Completer crate then Reedline could be built and distributed without the baggage of the Menu System and Completer System... Clearly this would require a dramatic Engine refactorization which probably is not going to happen.

But in any case its always good to keep modularity in mind --- @sholderbach would tell me in a polite way that there is a tradeoff here as well ---- and part of me tends to agree with him.

But for now --- its clearly not necessary in any way 😄

[stormasm]

I added your HackMd document here as well so it can be found easier in the next period of time...

https://hackmd.io/gDNAvI-8TDyubogworDyCA

[ysthakur]

Ah, a line-editor-agnostic menu system sounds cool

[maxomatic458]

cc: @maxomatic458, since you're working on improving completers

im probably not making more changes to the completer anytime soon. I just added a function for convenience that menus can make use of.

All the menus also have a lot of fields in common (which could be compacted in a struct).

maybe we could also just get rid of the all the builder functions and use a proc macro for that?

[stormasm]

cc: @maxomatic458, since you're working on improving completers

im probably not making more changes to the completer anytime soon. I just added a function for convenience that menus can make use of.

All the menus also have a lot of fields in common (which could be compacted in a struct).

maybe we could also just get rid of the all the builder functions and use a proc macro for that?

I would probably 👎 the proc macro concept simply because longer term they are more difficult to read, understand, and maintain.

In my view, larger open source projects like nushell etc has lots of different people over time maintaining and reading the code and we should make it easy as possible for them to understand what is going on (including myself) 😄

[maxomatic458]

hmm i see,
so i guess we could do something like this then

struct MenuCommon {
    name: String,
    active: bool,
    // a bunch of fields that the menus share
}

// and then include this in the menus
struct Menu {
    common: MenuCommon,
    some_specific_thing_for_this_menu: ...
}

this would remove a good bit of code duplication, but then we would need two builders (one for the MenuCommon, and one for the menu itself) to build a menu