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 <pjo...@redhat.com> > >>>> > >>>> 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 <javi...@redhat.com> > 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 <javi...@redhat.com>
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel