Hi Heinrich,
On Mon, 17 Oct 2022 at 15:46, Heinrich Schuchardt <xypron.g...@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 <s...@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
