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

[Heinrich Schuchardt]

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.

> + *
> + * 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.

> + * @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).

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);
>