[bug#38809] bootloader: grub: Add resolution override.

Hi,

With this patch I finally have a readable GRUB menu.  I am not sure
about the name and semantics of `resolution': Better to use `gfxmode',
and/or use a list of strings '("1024x768x32" "auto"), ... WDYT?

Greetings,
janneke

Jan Nieuwenhuizen <janneke@gnu.org> writes:

> Hi,
>
> With this patch I finally have a readable GRUB menu.  I am not sure
> about the name and semantics of `resolution': Better to use `gfxmode',
> and/or use a list of strings '("1024x768x32" "auto"), ... WDYT?
>
> Greetings,
> janneke
>
>>From dfceb5702b0de5787f79e1098b4787e9d3adbe4e Mon Sep 17 00:00:00 2001
> From: Jan Nieuwenhuizen <janneke@gnu.org>
> Date: Mon, 30 Dec 2019 11:25:40 +0100
> Subject: [PATCH] bootloader: grub: Add resolution override.
>
> * gnu/bootloader/grub.scm (<grub-theme>): Add `resolution' entry.
> (eye-candy): Use it.
> * doc/guix.texi (Bootloader Configuration): Document it.
> ---
>  doc/guix.texi           | 26 ++++++++++++++++++++++++--
>  gnu/bootloader/grub.scm | 11 +++++++----
>  2 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index efc59c1aaf..02cecae043 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -25923,9 +25923,20 @@ must @emph{not} be an OS device name such as @file{/dev/sda1}.
>  @end table
>  @end deftp
>  
> +@cindex HDPI
> +@cindex resolution
>  @c FIXME: Write documentation once it's stable.
> -For now only GRUB has theme support. GRUB themes are created using
> -the @code{grub-theme} form, which is not documented yet.
> +For now only GRUB has theme support.  GRUB themes are created using
> +the @code{grub-theme} form, which is not fully documented yet.
> +
> +@deftp {Data Type} grub-theme
> +Data type representing the configuration of the GRUB theme.
> +
> +@table @asis
> +@item @code{resolution} (default: @code{"auto"})
> +The @code{gfxmode} to set (@pxref{gfxmode,,, grub, GNU GRUB manual}).
> +@end table
> +@end deftp
>  
>  @defvr {Scheme Variable} %default-theme
>  This is the default GRUB theme used by the operating system if no
> @@ -25936,6 +25947,17 @@ It comes with a fancy background image displaying the GNU and Guix
>  logos.
>  @end defvr
>  
> +For example, to override the default resolution, you may use something
> +like
> +
> +@lisp
> +(bootloader
> + (grub-configuration
> + ;; @dots{}
> + (theme (grub-theme
> +         (inherit %default-theme)
> +         (resolution "1024x786x32;auto")))))
> +@end lisp
>  
>  @node Invoking guix system
>  @section Invoking @code{guix system}
> diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
> index fd62f627a0..70de3edeb5 100644
> --- a/gnu/bootloader/grub.scm
> +++ b/gnu/bootloader/grub.scm
> @@ -3,6 +3,7 @@
>  ;;; Copyright © 2016 Chris Marusich <cmmarusich@gmail.com>
>  ;;; Copyright © 2017 Leo Famulari <leo@famulari.name>
>  ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
> +;;; Copyright © 2019 Jan (janneke) Nieuwenhuizen <janneke@gnu.org>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -88,7 +89,9 @@ denoting a file name."
>    (color-normal    grub-theme-color-normal
>                     (default '((fg . cyan) (bg . blue))))
>    (color-highlight grub-theme-color-highlight
> -                   (default '((fg . white) (bg . blue)))))
> +                   (default '((fg . white) (bg . blue))))
> +  (resolution      grub-resolution
> +                   (default "auto")))
>  
>  (define %background-image
>    (grub-image
> @@ -149,8 +152,8 @@ system string---e.g., \"x86_64-linux\"."
>      ;; most other modern architectures have no other mode and therefore don't
>      ;; need to be switched.
>      (if (string-match "^(x86_64|i[3-6]86)-" system)
> -        "
> -  # Leave 'gfxmode' to 'auto'.
> +        (string-append "
> +  set gfxmode=" (grub-resolution (bootloader-configuration-theme config)) "
>    insmod video_bochs
>    insmod video_cirrus
>    insmod gfxterm
> @@ -166,7 +169,7 @@ system string---e.g., \"x86_64-linux\"."
>      insmod vbe
>      insmod vga
>    fi
> -"
> +")
>          ""))
>  
>    (define (setup-gfxterm config font-file)
> -- 
> 2.24.0

Hi Jan,

This seems like a sensible change, and is something other distributions
seem to implement in their own manner, as well. I am in favor of this
change, and will test it ASAP.

Jan Nieuwenhuizen 写道：
> With this patch I finally have a readable GRUB menu.

Thanks!  Perhaps it will help solve my EFI framebuffer woes 
(probably not but who knows.

> I am not sure about the name and semantics of `resolution':
> Better to use `gfxmode', and/or use a list of strings
> '("1024x768x32" "auto"), ... WDYT?

Yes please, on both counts.  Grub-theme should expose the GRUB 
variable, not a lossy abstraction.

Kind regards,

T G-R

Hi janneke,

On Mon, 30 Dec 2019 11:33:09 +0100
Jan Nieuwenhuizen <janneke@gnu.org> wrote:

> With this patch I finally have a readable GRUB menu.  I am not sure
> about the name and semantics of `resolution': Better to use `gfxmode',
> and/or use a list of strings '("1024x768x32" "auto"), ... WDYT?

Especially with HiDPI, the word "resolution" is ambiguous.

Also, gfxmode values are not standardized (f.e. u-boot would not know what
to do with them), so using a non-grub-specific key doesn't bring us anything.

I much prefer lists instead of one string.  But grub apparently has "," and
";" as separators--presumably with the same meaning?

It also has the special value "keep" as possible first item.

In view of this complication it's maybe better not to use lists.

Danny Milosavljevic writes:

Hi Danny,

>> With this patch I finally have a readable GRUB menu.  I am not sure
>> about the name and semantics of `resolution': Better to use `gfxmode',
>> and/or use a list of strings '("1024x768x32" "auto"), ... WDYT?
>
> Especially with HiDPI, the word "resolution" is ambiguous.
>
> Also, gfxmode values are not standardized (f.e. u-boot would not know what
> to do with them), so using a non-grub-specific key doesn't bring us anything.

That's another good argument; thank you for chiming in!  I think we all
agree now.  I sent an updated patch using `gfxmode' in another mail.

> I much prefer lists instead of one string.  But grub apparently has "," and
> ";" as separators--presumably with the same meaning?

Yes, that was also Tobias' wish, so we now have it as a list too.

> It also has the special value "keep" as possible first item.
>
> In view of this complication it's maybe better not to use lists.

Hmm, yes that's why I had some doubts too.  After Tobias' comment I now
chose to use a list, and the example now says

   (gfxmode '("1024x786x32" "auto"))

I hope that people can imagine prepending "keep"?

Greetings,
janneke

Hi janneke,

On Sat, 04 Jan 2020 17:07:24 +0100
Jan Nieuwenhuizen <janneke@gnu.org> wrote:

> Hmm, yes that's why I had some doubts too.  After Tobias' comment I now
> chose to use a list, and the example now says
> 
>    (gfxmode '("1024x786x32" "auto"))
> 
> I hope that people can imagine prepending "keep"?

Yes, it should be fine.  Thanks!  LGTM.

Danny Milosavljevic writes:

Hi Danny, Tobias,

> Yes, it should be fine.  Thanks!  LGTM.

After chatting with Tobias on irc, pushed to master as a23091880d4dc6115acbfa3b7ef09d731fc5abb0

Greetings,
janneke

I've been using the updated patch for a couple weeks now without issue. Maybe it's time to commit it again?

Alex Griffin 写道：
> I've been using the updated patch for a couple weeks now without 
> issue. Maybe it's time to commit it again?

Yes.  I think I LGTM'd this on IRC already, but regardless: LGTM!

Kind regards,

T G-R

Tobias Geerinckx-Rice writes:

> Alex Griffin 写道：
>> I've been using the updated patch for a couple weeks now without
>> issue. Maybe it's time to commit it again?
>
> Yes.  I think I LGTM'd this on IRC already, but regardless: LGTM!
>
> Kind regards,

Thank you both, pushed to master as f52fe7c3f29dfd0804c9d1f297b91287eabcdfb2

janneke