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

[Masahisa Kojima]

Hi Ilias,

On Tue, 24 Jan 2023 at 00:07, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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 at 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

Yes, it is more readable. Thank you.

Regards,
Masahisa Kojima

>
> > +                     }
> >                       /* 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