Re: [PATCH] hidemenu fix

[Bean]

On Sun, Jul 12, 2009 at 2:53 AM, Robert Millan<address@hidden> 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