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

[Avnish Chouhan <avnish@linux.ibm.com>]

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


> +  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!

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