Re: [PATCH] menu: add GRUB_RIGHT_TO_SELECT to toggle select-by-right-arrow-key

[jeffbai]

Hi Avnish,

在 2024-11-18 14:54，Avnish Chouhan 写道：
> > Message: 1
> > Date: Fri, 15 Nov 2024 18:04:03 +0800
> > From: Mingcong Bai <jeffbai@aosc.io>
> > To: grub-devel@gnu.org
> > Cc: sakiiily@aosc.io, cyan@cyano.uk, chenx97@aosc.io, jeffbai@aosc.io,
> >         fsf@live.com
> > Subject: [PATCH] menu: add GRUB_RIGHT_TO_SELECT to toggle
> >         select-by-right-arrow-key
> > Message-ID: <20241115100403.6305-1-jeffbai@aosc.io>
> >
> > Normally, GRUB allows using a combination of the Return/Enter (`\n' 
> > and
> > `\r'), Ctrl-F, and the right arrow key to select a menu item. However, 
> > on
> > some keyboards (especially those half-height arrow keys found on 
> > laptop
> > computers), it is very easy to accidentally select a menu item when 
> > the
> > user meant to press the up/down arrow key.
> >
> > Implement an GRUB_RIGHT_TO_SELECT option (boolean) to enable/disable 
> > right
> > arrow key for selecting menu items.
> >
> > Co-developed-by: Mag Mell <sakiiily@aosc.io>
> > Signed-off-by: Mingcong Bai <jeffbai@aosc.io>
> > ---
> >  docs/grub.texi           |  4 ++++
> >  grub-core/normal/menu.c  | 20 +++++++++++++++++---
> >  util/grub-mkconfig.in    |  3 ++-
> >  util/grub.d/00_header.in |  6 ++++++
> >  4 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/grub.texi b/docs/grub.texi
> > index acf6f4428..7ae4f4886 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -1593,6 +1593,10 @@ This option may be set to a list of GRUB module
> > names separated by spaces.
> >  Each module will be loaded as early as possible, at the start of
> >  @file{grub.cfg}.
> >
> > +@item GRUB_RIGHT_TO_SELECT
> > +This option may be set to specify whether the right arrow key may be 
> > used
> > +to make a selection. This option defaults to @samp{true}.
> > +
> >  @end table
> >
> >  The following options are still accepted for compatibility with 
> > existing
> > diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
> > index 6a90e091f..78566b264 100644
> > --- a/grub-core/normal/menu.c
> > +++ b/grub-core/normal/menu.c
> > @@ -32,6 +32,7 @@
> >  #include <grub/script_sh.h>
> >  #include <grub/gfxterm.h>
> >  #include <grub/dl.h>
> > +#include <grub/env.h>
> >
> >  /* Time to delay after displaying an error message about a 
> > default/fallback
> >     entry failing to boot.  */
> > @@ -579,11 +580,16 @@ run_menu (grub_menu_t menu, int nested, int
> > *auto_boot, int *notify_boot)
> >    int default_entry, current_entry;
> >    int timeout;
> >    enum timeout_style timeout_style;
> > +  bool right_to_select;
> >
> >    *notify_boot = 1;
> >
> >    default_entry = get_entry_number (menu, "default");
> >
> > +  /* Read if the right arrow key is enabled for selection. Default to 
> > `true'
> > +     if unset. */
>
> Hi Mingcong,
>
> Please make this multiline comment as per GNU GRUB coding standards. 
> Something like
>
> /*
>  * xxxxxxxx
>  * xxxxxxxx
>  */

Noted.

> > +  right_to_select = grub_env_get_bool ("right_to_select", true);
> > +
> >    /* If DEFAULT_ENTRY is not within the menu entries, fall back to
> >       the first entry.  */
> >    if (default_entry < 0 || default_entry >= menu->size)
> > @@ -762,9 +768,17 @@ run_menu (grub_menu_t menu, int nested, int
> > *auto_boot, int *notify_boot)
> >             case '\r':
> >             case GRUB_TERM_KEY_RIGHT:
> >             case GRUB_TERM_CTRL | 'f':
> > -             menu_fini ();
> > -              *auto_boot = 0;
> > -             return current_entry;
> > +             /* Right arrow key to select only when boolean value
> > +                `right_to_select' is set to `true' or not specified
> > +                (defaults to `true'). */
>
> Same as commented earlier!
>
> > +             if ((c != GRUB_TERM_KEY_RIGHT) ||
> > +                 right_to_select)
> > +               {
> > +                 menu_fini ();
> > +                 *auto_boot = 0;
> > +                 return current_entry;
> > +               }
> > +             break;
>
> This also seems little off on coding style.
> Thank you!

Got it. I will send a v2 patch in a bit, thanks for your review!

Best Regards,
Mingcong Bai
> Regards,
> Avnish Chouhan
>
>
> >             case GRUB_TERM_ESC:
> >               if (nested)
> > diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
> > index 73a973b25..b15bfc13a 100644
> > --- a/util/grub-mkconfig.in
> > +++ b/util/grub-mkconfig.in
> > @@ -254,7 +254,8 @@ export GRUB_DEFAULT \
> >    GRUB_ENABLE_CRYPTODISK \
> >    GRUB_BADRAM \
> >    GRUB_OS_PROBER_SKIP_LIST \
> > -  GRUB_DISABLE_SUBMENU
> > +  GRUB_DISABLE_SUBMENU \
> > +  GRUB_RIGHT_TO_SELECT
> >
> >  if test "x${grub_cfg}" != "x"; then
> >    rm -f "${grub_cfg}.new"
> > diff --git a/util/grub.d/00_header.in b/util/grub.d/00_header.in
> > index 5e17a9db4..0adf84019 100644
> > --- a/util/grub.d/00_header.in
> > +++ b/util/grub.d/00_header.in
> > @@ -37,6 +37,7 @@ if [ "x${GRUB_DEFAULT}" = "x" ] ; then 
> > GRUB_DEFAULT=0 ; fi
> >  if [ "x${GRUB_DEFAULT}" = "xsaved" ] ; then 
> > GRUB_DEFAULT='${saved_entry}' ; fi
> >  if [ "x${GRUB_TIMEOUT}" = "x" ] ; then GRUB_TIMEOUT=5 ; fi
> >  if [ "x${GRUB_GFXMODE}" = "x" ] ; then GRUB_GFXMODE=auto ; fi
> > +if [ "x${GRUB_RIGHT_TO_SELECT}" = "x" ] ; then 
> > GRUB_RIGHT_TO_SELECT=true ; fi
> >
> >  if [ "x${GRUB_DEFAULT_BUTTON}" = "x" ] ; then
> > GRUB_DEFAULT_BUTTON="$GRUB_DEFAULT" ; fi
> >  if [ "x${GRUB_DEFAULT_BUTTON}" = "xsaved" ] ; then
> > GRUB_DEFAULT_BUTTON='${saved_entry}' ; fi
> > @@ -373,3 +374,8 @@ fi
> >  if [ "x${GRUB_BADRAM}" != "x" ] ; then
> >    echo "badram ${GRUB_BADRAM}"
> >  fi
> > +
> > +# Whether to allow selecting with the right arrow key.
> > +cat << EOF
> > +set right_to_select=${GRUB_RIGHT_TO_SELECT}
> > +EOF
> > --
> > 2.47.0
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel