Improvement discussions

[juergenpabel]

I came across your library for a small project of mine that's build around circuitpython on an M5stack DinMeter. Your library allows me to implement the entire application UI as a menu, which is about as much as one can do with such a small display.

I stumbled upon a few things that I will add/change in my fork - but would like to discuss these beforehand in order to adapt so that you'll accept future pull requests:

1. library file structure

• create a menu subdir, move *.py into menu/ and add an __init__.py
• file reorganisation: split menu.py into menu.py, menuitem.py and action.py

2. CircuitPython API changes

• displayio: use a displayio.Group instance as the rendering "target" instead of displayio.Display (which doesn't exist anymore); app is responsible for activating the group onto the active display

3. Menu

• add (keyword-based) configuration parameters to Menu constructor
  • font: pass in a font (default=terminalio.FONT)
  • header: a header to (always) render in the first line when this menu instance is shown (default=None)
• Menu.run()
  • add a callable parameter (keyword=display_handler) that takes a displayio.Group argument (which is the generated group of the menu)...maybe DisplayHandler should also be a new abstract class for (mostly) typing purposes (seems like a reasonable approach)
  • add a callable parameter (keyword=exit_condition) for application controlled exit conditioning (default=None, which would than effectively result in while True again)
• Menu.debounce()
  • remove and adapt loop in Menu.run() to check for timestamp if debouncing complete (instead of busy waiting)...also important for #5 on this list

4. Menu items

• rename FinalMenuItem to ExitMenuItem (as it merely emits a sub-menu exit, but it doesn't need to be the final/last item in a list)
• rename TitleMenuItem to NonselectableMenuItem

5. async compatibility

• I don't want to add specific async features, but want async applications to be able to use this library in an async runtime context
• removing any locking / waiting code like time.sleep() is essential (see debouncing point, above)
• async compatibility could be implemented by the application by providing an exit_condition callable that also does async event loop switching - but I am open to other approaches

I am sure there is more, but that's what I could think of just now. Let me know, I would like to finish my project within the next couple of weeks (including this sub-task).

[zerario]

Oh, finally someone other than me seemingly interested in this project! 😅 Truth be told, last time I looked at this code or used the library myself was pretty much exactly two years ago...

While I can't commit to a lot time-wise, all the changes you describe sound reasonable. They also all sound relatively small and self-contained, so as long as you take care of opening separate isolated PRs for them, I should be able to merge those.

Two things come to mind:

• Regarding the exit_condition: A different API that might make sense is making it possible to subclass Menu and document what kind of API is intended for subclasses to override. Seems more natural at the point you want to customize behavior code-wise. However, might take a bit more care since the class was not written with explicit subclassing in mind.
• I really don't like NonselectableMenuItem over the way more simple TitleMenuItem. What's the reasoning for this? Maybe something like TextMenuItem would work?

[juergenpabel]

Oh, finally someone other than me seemingly interested in this project! 😅 Truth be told, last time I looked at this code or used the library myself was pretty much exactly two years ago...

And that's just how it is...most of my projects are like that (and that's ok, eventually someone stumbles upon it and finds it useful).

While I can't commit to a lot time-wise, all the changes you describe sound reasonable. They also all sound relatively small and self-contained, so as long as you take care of opening separate isolated PRs for them, I should be able to merge those.

Cool, do you want me to create an issue per "changeset" or just add to this one (I'm ok with either approach)?

Two things come to mind:

* Regarding the `exit_condition`: A different API that might make sense is making it possible to subclass `Menu` and document what kind of API is intended for subclasses to override. Seems more natural at the point you want to customize behavior code-wise. However, might take a bit more care since the class was not written with explicit subclassing in mind.

Yeah...the exit_condition was my minimum-intrusive (as to code changes) idea, but after working out a first PoC, I've come upon another issue that in the end leads to the subclassing approach: The current implementation generates "self.drawables" for all menu instances upon instantiation - on "smaller" systems (like <1MB RAM) this limits the total menu items to some unreasonably low number (<50 for an ESP32 with 512kb). If those drawables are generated only when the corresponding menu instance is actually in use (actively shown or "up the stack"), this makes that memory limitation a lot less relevant. So....some Menu class code changes are necessary anyhow, my approach would be: move out all menu execution related data (self.drawables, self.layout, ...) into a separate class that than handles the "execution" of that menu instance - and this class can than be subclassed (and passed in as a parameter into Menu.run()).

* I really don't like `NonselectableMenuItem` over the way more simple `TitleMenuItem`. What's the reasoning for this? Maybe something like `TextMenuItem` would work?

My idea was to have an optional menu header that's always rendered in the very first line (like the contextual name of the currently shown menu, something like "Settings > General". During my PoC I've implemented this not as a separate menu-item class but rather "hardcoded" it into the menu class as a header element (which might be None and is than not rendered). My current idea is to carry this over into the default implementation of the menu-execution class from above. (On a side note there: I would actually split out menu item rendering (vs. menu "context", like the menu name) into a separate displayio.Group instance, so that for example paging would only need to replace that displayio.Group reference with the displayio.Group reference of the next page worth of menu items. This would also open up (for larger displays than mine with 240x135) to have these menu items rendered as part of some "surrounding" UI (displayio.Group element-tree).