Re: [PATCH] Enable pager by default

[Daniel Kiper]

On Wed, Oct 23, 2019 at 01:26:54PM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
>
> On 10/22/19 4:04 PM, Daniel Kiper wrote:
> > On Tue, Oct 22, 2019 at 10:30:20AM +0200, Javier Martinez Canillas wrote:
> >> Hello Daniel,
> >>
> >> On 10/21/19 4:56 PM, Daniel Kiper wrote:
> >>> On Fri, Oct 18, 2019 at 02:43:18PM +0200, Javier Martinez Canillas wrote:
> >>>> From: Peter Jones <address@hidden>
> >>>>
> >>>> When user enters into the GRUB shell and tries to use help command, lot 
> >>>> of
> >>>> information is scrolled out of screen and the user doesn't have chance to
> >>>> read it. Also, there isn't any information about 'set pager=1' at the end
> >>>> of the help output, to tell the user how scrolling could be enabled.
> >>>>
> >>>> So just enable pager by default which leads to a much better experience.
> >>>
> >>> Hmmm... What will happen if a command produce tons of output during boot
> >>> process? I am afraid that it will hang indefinitely waiting for an user
> >>> input. This should not happen. So, I tend to agree that current help
> >>> command behavior is annoying but I do not like the solution.
> >>
> >> Ok. I'll then explore having a paginated output only for the help command
> >> instead of globally enabling it by default.
> >
> > Great! Though I would think about something which can be used also in
> > other commands producing a lot of output. Maybe we should introduce "-p"
> > (pause) command line option for such commands. And I am not against
> > using existing code to do a pause. We just have to do it carefully.
> >
>
> So I've tried different approaches to solves this. I'm not that sure about
> introducing a -p option, since I think that we could have the same issue
> of users not knowing that have to use this option to scroll the output.
>
> Unless we print for each command a message after its output, telling users
> that have to use -p if they want to have paginated output for that command.
>
> What do you think about the following patch? That way we can specify if the
> output for each command must be printed with the pager enabled or not.
>
> Probably should be broken in 2 patches, but just want to know your opinion.
>
> From 7c4da6295ebd3a034d1f7e32099eab33efa465d4 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <address@hidden>
> Date: Tue, 22 Oct 2019 15:35:12 +0200
> Subject: [PATCH v2] Add a GRUB_COMMAND_FLAG_PAGINATED to request paginated
>  output for commands
>
> When user enters into the GRUB shell and tries to use help command, lot of
> information is scrolled out of screen and the user doesn't have chance to
> read it. Also, there isn't any information about 'set pager=1' at the end
> of the help output, to tell the user how scrolling could be enabled.
>
> Since the out for some commands may not fit into a screen, add a new flag
> GRUB_COMMAND_FLAG_PAGINATED that can be used when registering commands that
> can be used to request the pager to be enabled while executing the handler.
>
> Signed-off-by: Javier Martinez Canillas <address@hidden>

In general I like the idea but I think patch requires some polishing...

Hmmm... Still thinking about "-p" flag which allows user to choose
between pager on/off. Or something which I proposed in the email to
Michael...

> ---
>
> Changes in v2:
> - Only enable pager temporarily while printing output for commands that asked
>   for paginated output instead of doing it globally using the pager variable.
>
>  grub-core/commands/help.c    |  3 ++-
>  grub-core/commands/minicmd.c | 13 ++++++++-----
>  grub-core/normal/dyncmd.c    |  7 +++++++
>  grub-core/normal/term.c      | 17 +++++++++++++++++
>  grub-core/script/execute.c   |  7 +++++++
>  include/grub/command.h       |  4 +++-
>  include/grub/normal.h        |  3 +++
>  7 files changed, 47 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/commands/help.c b/grub-core/commands/help.c
> index f0be89baa19..6e20e1447a8 100644
> --- a/grub-core/commands/help.c
> +++ b/grub-core/commands/help.c
> @@ -142,7 +142,8 @@ static grub_extcmd_t cmd;
>  
>  GRUB_MOD_INIT(help)
>  {
> -  cmd = grub_register_extcmd ("help", grub_cmd_help, 0,
> +  cmd = grub_register_extcmd ("help", grub_cmd_help,
> +                           GRUB_COMMAND_FLAG_PAGINATED,
>                             N_("[PATTERN ...]"),
>                             N_("Show a help message."), 0);
>  }
> diff --git a/grub-core/commands/minicmd.c b/grub-core/commands/minicmd.c
> index 6bbce3128cf..e6a9242bfa6 100644
> --- a/grub-core/commands/minicmd.c
> +++ b/grub-core/commands/minicmd.c
> @@ -21,6 +21,7 @@
>  #include <grub/mm.h>
>  #include <grub/err.h>
>  #include <grub/env.h>
> +#include <grub/extcmd.h>
>  #include <grub/misc.h>
>  #include <grub/file.h>
>  #include <grub/disk.h>
> @@ -75,7 +76,7 @@ grub_mini_cmd_cat (struct grub_command *cmd __attribute__ 
> ((unused)),
>
>  /* help */
>  static grub_err_t
> -grub_mini_cmd_help (struct grub_command *cmd __attribute__ ((unused)),
> +grub_mini_cmd_help (grub_extcmd_context_t ctxt __attribute__ ((unused)),
>                   int argc __attribute__ ((unused)),
>                   char *argv[] __attribute__ ((unused)))
>  {
> @@ -188,7 +189,8 @@ grub_mini_cmd_exit (struct grub_command *cmd 
> __attribute__ ((unused)),
>    /* Not reached.  */
>  }
>
> -static grub_command_t cmd_cat, cmd_help;
> +static grub_command_t cmd_cat;
> +static grub_extcmd_t cmd_help;
>  static grub_command_t cmd_dump, cmd_rmmod, cmd_lsmod, cmd_exit;
>
>  GRUB_MOD_INIT(minicmd)
> @@ -197,8 +199,9 @@ GRUB_MOD_INIT(minicmd)
>      grub_register_command ("cat", grub_mini_cmd_cat,
>                          N_("FILE"), N_("Show the contents of a file."));
>    cmd_help =
> -    grub_register_command ("help", grub_mini_cmd_help,
> -                        0, N_("Show this message."));
> +    grub_register_extcmd ("help", grub_mini_cmd_help,
> +                          GRUB_COMMAND_FLAG_PAGINATED,
> +                          0, N_("Show this message."), 0);
>    cmd_dump =
>      grub_register_command ("dump", grub_mini_cmd_dump,
>                          N_("ADDR [SIZE]"), N_("Show memory contents."));
> @@ -216,7 +219,7 @@ GRUB_MOD_INIT(minicmd)
>  GRUB_MOD_FINI(minicmd)
>  {
>    grub_unregister_command (cmd_cat);
> -  grub_unregister_command (cmd_help);
> +  grub_unregister_extcmd (cmd_help);
>    grub_unregister_command (cmd_dump);
>    grub_unregister_command (cmd_rmmod);
>    grub_unregister_command (cmd_lsmod);
> diff --git a/grub-core/normal/dyncmd.c b/grub-core/normal/dyncmd.c
> index 719ebf477f2..1ef52cbe43b 100644
> --- a/grub-core/normal/dyncmd.c
> +++ b/grub-core/normal/dyncmd.c
> @@ -78,11 +78,18 @@ grub_dyncmd_dispatcher (struct grub_extcmd_context *ctxt,
>    cmd = grub_command_find (name);
>    if (cmd)
>      {
> +      /* Temporarily enable paginated output for commands that asked for it 
> */
> +      if (cmd->flags & GRUB_COMMAND_FLAG_PAGINATED)
> +        grub_enable_temp_more ();
> +
>        if (cmd->flags & GRUB_COMMAND_FLAG_BLOCKS &&
>         cmd->flags & GRUB_COMMAND_FLAG_EXTCMD)
>       ret = grub_extcmd_dispatcher (cmd, argc, args, ctxt->script);
>        else
>       ret = (cmd->func) (cmd, argc, args);
> +
> +      if (cmd->flags & GRUB_COMMAND_FLAG_PAGINATED)
> +        grub_disable_temp_more ();
>      }
>    else
>      ret = grub_errno;
> diff --git a/grub-core/normal/term.c b/grub-core/normal/term.c
> index a1e5c5a0daf..7d4021ff8be 100644
> --- a/grub-core/normal/term.c
> +++ b/grub-core/normal/term.c
> @@ -57,6 +57,7 @@ static struct term_state *term_states = NULL;
>
>  /* If the more pager is active.  */
>  static int grub_more;
> +static int temp_more;
>
>  static void
>  putcode_real (grub_uint32_t code, struct grub_term_output *term, int 
> fixed_tab);
> @@ -128,6 +129,22 @@ grub_set_more (int onoff)
>    grub_normal_reset_more ();
>  }
>
> +void
> +grub_enable_temp_more (void)
> +{
> +  temp_more = grub_more;
> +
> +  if (!temp_more)
> +    grub_set_more (1);

Does it change pager variable value? If yes I think
that you should store its original value here and
restore below.

> +}
> +
> +void
> +grub_disable_temp_more (void)
> +{
> +  if (!temp_more)
> +    grub_set_more (0);

Daniel