[PATCH] hidemenu fix

[PATCH] hidemenu fix

[Bean]

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

Hi,

This patch moves the screen init code from reader to menu, so that
hidemenu would work as before. A minor side effect is that if grub.cfg
is not found, the "GNU GRUB" header would not be printed, but I guess
this is not an issue.

-- 
Bean

[-- Attachment #2: hidemenu.diff --]
[-- Type: text/x-patch, Size: 1546 bytes --]

diff --git a/kern/rescue_reader.c b/kern/rescue_reader.c
index 2a06f3f..0b738d1 100644
--- a/kern/rescue_reader.c
+++ b/kern/rescue_reader.c
@@ -26,13 +26,6 @@
 
 static char linebuf[GRUB_RESCUE_BUF_SIZE];
 
-static grub_err_t
-grub_rescue_init (void)
-{
-  grub_printf ("Entering rescue mode...\n");
-  return 0;
-}
-
 /* Prompt to input a command and read the line.  */
 static grub_err_t
 grub_rescue_read_line (char **line, int cont)
@@ -77,7 +70,6 @@ grub_rescue_read_line (char **line, int cont)
 static struct grub_reader grub_rescue_reader =
   {
     .name = "rescue",
-    .init = grub_rescue_init,
     .read_line = grub_rescue_read_line
   };
 
diff --git a/normal/main.c b/normal/main.c
index 7f6336e..84e25f9 100644
--- a/normal/main.c
+++ b/normal/main.c
@@ -474,14 +474,7 @@ grub_cmdline_run (int nested)
   grub_reader_t reader = grub_reader_get_current ();
 
   reader_nested = nested;
-  if (reader->init)
-    reader->init ();
-  grub_reader_loop (0);
-}
 
-static grub_err_t
-grub_normal_reader_init (void)
-{
   grub_normal_init_page ();
   grub_setcursor (1);
 
@@ -491,7 +484,7 @@ grub_normal_reader_init (void)
    device/file completions.%s ]\n\n",
 	       reader_nested ? " ESC at any time exits." : "");
 
-  return 0;
+  grub_reader_loop (0);
 }
 
 static char cmdline[GRUB_MAX_CMDLINE];
@@ -524,7 +517,6 @@ grub_normal_read_line (char **line, int cont)
 static struct grub_reader grub_normal_reader =
   {
     .name = "normal",
-    .init = grub_normal_reader_init,
     .read_line = grub_normal_read_line
   };
 

Re: [PATCH] hidemenu fix

[Robert Millan]

On Sat, Jul 11, 2009 at 01:26:50PM +0800, Bean wrote:
> Hi,
> 
> This patch moves the screen init code from reader to menu, so that
> hidemenu would work as before. A minor side effect is that if grub.cfg
> is not found, the "GNU GRUB" header would not be printed, but I guess
> this is not an issue.

Hi Bean,

I don't understand this very well.  First of all, which part is supposed
to be responsible for printing the header?  Are you changing that?  Right
now it is being printed twice.

> -static grub_err_t
> -grub_rescue_init (void)
> -{
> -  grub_printf ("Entering rescue mode...\n");
> -  return 0;
> -}
> -
>  /* Prompt to input a command and read the line.  */
>  static grub_err_t
>  grub_rescue_read_line (char **line, int cont)
> @@ -77,7 +70,6 @@ grub_rescue_read_line (char **line, int cont)
>  static struct grub_reader grub_rescue_reader =
>    {
>      .name = "rescue",
> -    .init = grub_rescue_init,
>      .read_line = grub_rescue_read_line
>    };

Why is grub_rescue_init being removed?

> @@ -524,7 +517,6 @@ grub_normal_read_line (char **line, int cont)
>  static struct grub_reader grub_normal_reader =
>    {
>      .name = "normal",
> -    .init = grub_normal_reader_init,
>      .read_line = grub_normal_read_line
>    };

And this one, too.  If readers are no longer responsible for printing a
header, shouldn't we adjust whatever code that was calling their
init functions, and the grub_reader struct as well?

This patch looks like a kludge to me.  Can we discuss what's wrong with the
current design, and what options do we have to solve this?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: [PATCH] hidemenu fix

[Bean]

On Sun, Jul 12, 2009 at 2:53 AM, Robert Millan<rmh@aybabtu.com> wrote:
> On Sat, Jul 11, 2009 at 01:26:50PM +0800, Bean wrote:
>> Hi,
>>
>> This patch moves the screen init code from reader to menu, so that
>> hidemenu would work as before. A minor side effect is that if grub.cfg
>> is not found, the "GNU GRUB" header would not be printed, but I guess
>> this is not an issue.
>
> Hi Bean,
>
> I don't understand this very well.  First of all, which part is supposed
> to be responsible for printing the header?  Are you changing that?  Right
> now it is being printed twice.

The menu would init screen and print a header, when you click "c", it
clear screen again and print another header. This method should be the
right way. Previously the later code is in normal reader, which cause
screen to be cleared prematurely.

>
>> -static grub_err_t
>> -grub_rescue_init (void)
>> -{
>> -  grub_printf ("Entering rescue mode...\n");
>> -  return 0;
>> -}
>> -
>>  /* Prompt to input a command and read the line.  */
>>  static grub_err_t
>>  grub_rescue_read_line (char **line, int cont)
>> @@ -77,7 +70,6 @@ grub_rescue_read_line (char **line, int cont)
>>  static struct grub_reader grub_rescue_reader =
>>    {
>>      .name = "rescue",
>> -    .init = grub_rescue_init,
>>      .read_line = grub_rescue_read_line
>>    };
>
> Why is grub_rescue_init being removed?

If it's not removed, it would print "Entering rescue mode..." before
the timeout, as rescue reader is started quite early. This is not a
big issue, but to get a silent effect, we can remove this as well. In
effect, we can always tell the current reader from command prompt,
there is no need for the "welcome" message. (It also saves a few bytes
in kernel)

>
>> @@ -524,7 +517,6 @@ grub_normal_read_line (char **line, int cont)
>>  static struct grub_reader grub_normal_reader =
>>    {
>>      .name = "normal",
>> -    .init = grub_normal_reader_init,
>>      .read_line = grub_normal_read_line
>>    };
>
> And this one, too.  If readers are no longer responsible for printing a
> header, shouldn't we adjust whatever code that was calling their
> init functions, and the grub_reader struct as well?
>
> This patch looks like a kludge to me.  Can we discuss what's wrong with the
> current design, and what options do we have to solve this?

I think the new handling is correct. The printing of header is the job
of menu viewer, not reader. Suppose we switch to a graphic viewer,
then the current design would have problem, as it assume control over
the whole screen, but we might want to draw it in a window. Only menu
viewer knows how to draw it properly.

-- 
Bean