Re: [PATCH v4 3/4] eficonfig: add vertical scroll support

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Fri, Jan 20, 2023 at 05:43:57PM +0900, Masahisa Kojima wrote:
> The current eficonfig menu does not support vertical scroll,
> so it can not display the menu entries greater than
> the console row size.
>
> This commit add the vertial scroll support.
> The console size is retrieved by
> SIMPLE_TEXT_OUTPUT_PROTOCOL.QueryMode() service, then
> calculates the row size for menu entry by subtracting
> menu header and description row size from the console row size.
> "start" and "end" are added in the efimenu structure.
> "start" keeps the menu entry index at the top, "end" keeps
> the bottom menu entry index. item_data_print() menu function
> only draws the menu entry between "start" and "end".
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> No update since v3
>
> Changes in v3:
> - modify "reverse" local variable type to bool
>
> Changes in v2:
> - add comment when the user key press is not valid
> - add const qualifier to eficonfig_change_boot_order_desc
>
>  cmd/eficonfig.c      | 79 ++++++++++++++++++++++++++++++++++++--------
>  include/efi_config.h |  4 +++
>  include/efi_loader.h |  1 +
>  3 files changed, 70 insertions(+), 14 deletions(-)
>
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 01bd1b05bc..5692b332c7 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -30,8 +30,13 @@ static const char *eficonfig_change_boot_order_desc =
>  	"  Press SPACE to activate or deactivate the entry\n"
>  	"  Select [Save] to complete, ESC/CTRL+C to quit";
>
> +static struct efi_simple_text_output_protocol *cout;
> +static int avail_row;
> +
>  #define EFICONFIG_DESCRIPTION_MAX 32
>  #define EFICONFIG_OPTIONAL_DATA_MAX 64
> +#define EFICONFIG_MENU_HEADER_ROW_NUM 3
> +#define EFICONFIG_MENU_DESC_ROW_NUM 5
>
>  /**
>   * struct eficonfig_filepath_info - structure to be used to store file path
> @@ -157,18 +162,16 @@ void eficonfig_print_entry(void *data)
>  	struct eficonfig_entry *entry = data;
>  	bool reverse = (entry->efi_menu->active == entry->num);
>
> -	/* TODO: support scroll or page for many entries */
> +	if (entry->efi_menu->start > entry->num || entry->efi_menu->end < entry->num)
> +		return;
>
> -	/*
> -	 * Move cursor to line where the entry will be drawn (entry->num)
> -	 * First 3 lines(menu header) + 1 empty line
> -	 */
> -	printf(ANSI_CURSOR_POSITION, entry->num + 4, 7);
> +	printf(ANSI_CURSOR_POSITION, (entry->num - entry->efi_menu->start) +
> +	       EFICONFIG_MENU_HEADER_ROW_NUM + 1, 7);
>
>  	if (reverse)
>  		puts(ANSI_COLOR_REVERSE);
>
> -	printf("%s", entry->title);
> +	printf(ANSI_CLEAR_LINE "%s", entry->title);
>
>  	if (reverse)
>  		puts(ANSI_COLOR_RESET);
> @@ -191,8 +194,8 @@ void eficonfig_display_statusline(struct menu *m)
>  	       ANSI_CURSOR_POSITION ANSI_CLEAR_LINE ANSI_CURSOR_POSITION
>  	       "%s"
>  	       ANSI_CLEAR_LINE_TO_END,
> -	       1, 1, entry->efi_menu->menu_header, entry->efi_menu->count + 5, 1,
> -	       entry->efi_menu->count + 6, 1, entry->efi_menu->menu_desc);
> +	       1, 1, entry->efi_menu->menu_header, avail_row + 4, 1,
> +	       avail_row + 5, 1, entry->efi_menu->menu_desc);
>  }
>
>  /**
> @@ -216,13 +219,23 @@ char *eficonfig_choice_entry(void *data)
>
>  		switch (key) {
>  		case BKEY_UP:
> -			if (efi_menu->active > 0)
> +			if (efi_menu->active > 0) {
>  				--efi_menu->active;
> +				if (efi_menu->start > efi_menu->active) {
> +					efi_menu->start--;
> +					efi_menu->end--;
> +				}

For all the sequences of this do:
void efi_menu_adjust(struct efimenu *menu, bool add)  {
    if (add && efi_menu->end < efi_menu->active) {
        menu->start++;
	menu->end++;
    } else if (!add && efi_menu->start > efi_menu->active)
        menu->start--;
	menu->end--;
    }
}
#define efi_menu_up(_a) efi_menu_adjust(_a, true)
#define efi_menu_down(_a) efi_menu_adjust(_a, false)

I think this would look a bit more readable

> +			}
>  			/* no menu key selected, regenerate menu */
>  			return NULL;
>  		case BKEY_DOWN:
> -			if (efi_menu->active < efi_menu->count - 1)
> +			if (efi_menu->active < efi_menu->count - 1) {
>  				++efi_menu->active;
> +				if (efi_menu->end < efi_menu->active) {
> +					efi_menu->start++;
> +					efi_menu->end++;

[...]

Regards
/Ilias