[PATCH 03/24] bootmenu: Add a few comments

[Simon Glass]

Hi Heinrich,

On Mon, 17 Oct 2022 at 15:46, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 10/17/22 22:29, Simon Glass wrote:
> > The behaviour of these two functions is completely undocumented. Add some
> > notes so the poor, suffering dev can figure out what is going on.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >   include/menu.h | 42 ++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> >
> > diff --git a/include/menu.h b/include/menu.h
> > index 702aacb170c..0b4d9734149 100644
> > --- a/include/menu.h
> > +++ b/include/menu.h
> > @@ -42,6 +42,7 @@ struct bootmenu_data {
> >       struct bootmenu_entry *first;   /* first menu entry */
> >   };
> >
> > +/** enum bootmenu_key - keys that can be returned by the bootmenu */
> >   enum bootmenu_key {
> >       KEY_NONE = 0,
> >       KEY_UP,
> > @@ -53,8 +54,49 @@ enum bootmenu_key {
> >       KEY_SPACE,
> >   };
> >
> > +/**
> > + * bootmenu_autoboot_loop() - handle autobooting if no key is pressed
> > + *
> > + * This shows a prompt to allow the user to press a key to interrupt auto boot
> > + * of the first menu option.
> > + *
> > + * It then waits for the required time (menu->delay in seconds) for a key to be
> > + * pressed. If nothing is pressed in that time, @key returns KEY_SELECT
> > + * indicating that the current option should be chosen.
> > + *
> > + * @menu: Menu being processed
> > + * @key: Returns the code for the key the user pressed:
> > + *   enter: KEY_SELECT
> > + *   Ctrl-C: KEY_QUIT
> > + *   anything else: KEY_NONE
> > + * @esc: Set to 1 if the escape key is pressed, otherwise not updated
> > + */
> >   void bootmenu_autoboot_loop(struct bootmenu_data *menu,
> >                           enum bootmenu_key *key, int *esc);
> > +
> > +/**
> > + * bootmenu_loop() - handle waiting for a keypress when autoboot is disabled
> > + *
> > + * This is used when the menu delay is negative, indicating that the delay has
> > + * elapsed, or there was no delay to begin with.
>
> Unfortunately the description does not match the code.
>
> This function is entered if some key was pressed, so autoboot was
> stopped. When the delay elapses the default action is taken by the caller.

This one doesn't do default action, right? That functionality is in
bootmenu_autoboot_loop() above.

>
> > + *
> > + * It reads a character and processes it, returning a menu-key code if a
> > + * character is recognised
> > + *
> > + * @menu: Menu being processed
> > + * @key: Returns the code for the key the user pressed:
> > + *   enter: KEY_SELECT
> > + *   Ctrl-C: KEY_QUIT
> > + *   Up arrow: KEY_UP
> > + *   Down arrow: KEY_DOWN
> > + *   Escape (by itself): KEY_QUIT
> > + *   Plus: KEY_PLUS
> > + *   Minus: KEY_MINUS
> > + *   Space: KEY_SPACE
>
> We already discussed that this list is to change. We should support
> accelerator keys in menus.

OK, but not related to my change here.

>
> > + * @esc: On input, a non-zero value indicates that an escape sequence has
>
> 1 indicates that the ESC key has been pressed.
> All other values indicate that the ESC key has not been pressed.
>
> --
>
> The whole design is broken in that the concept of a menu is not properly
> encapsulated.
>
> A function like bootmenu_autoboot_loop() should not be exported.
>
> The view side of a menu should be in a function that takes the following
> arguments:
>
> - the location (x,y) on the screen
> - the size (dx, dy) of the displayed menu
>    (further items should be viewed by vertical scrolling,
>    more characters by horizontal scrolling)
> - a list of menu items to display
> - a event function called whenever the selected menu item changes (or NULL)
>
> Such an event function will allow to display extra information for a
> menu item.
>
> The model would be an array of utf-8 strings.
>
> The controller will invoke the view function and receive the index of
> the selected item as the return value (or -1 if ESC is pressed).

I think the existing menu system is not useful as a basis for what we
want, which I why I designed expo. We should focus on getting that
right. Just to be clear, I will not be taking on any effort involved
in making things work with EFI. But you may wish to.

>
> Best regards
>
> Heinrich
>
> > + *   resulted in that many characters so far. On exit this is updated to the
> > + *   new number of characters
> > + */
> >   void bootmenu_loop(struct bootmenu_data *menu,
> >                  enum bootmenu_key *key, int *esc);
> >
>

Regards,
Simon