[bug#41163] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution).

* gnu/bootloaders/grub.scm (<grub-image>)[resolution]: Replacement of the
'aspect-ratio' field.
(<grub-theme>)[image]: Replacement of the 'images' field.
(svg->png): Remove default-values for 'width' and 'height' and only do the
conversion with them, if the suffix of the file is actually ".svg".

Using image formats different to SVG was not possible.

For a <grub-image> to be chosen, the 'aspect-ratio' of it had to be 4/3, as the
resolution of any image was defaulting to 1024 x 768.

There was no code yet to determine the proper boot-resolution, to make any use
of a list of images with different aspect-ratios.

It seems to be a better solution to only define a single image with any format,
and use a given resolution only for the conversion from a SVG file.

Moving the default values from '%background-image' and '%default-theme' into
<grub-image> and <grub-theme> makes a customisation easier without (inherit)
and allows to deprecate %background-image' and '%default-theme'.
---
 gnu/bootloader/grub.scm | 74 +++++++++++++++++++----------------------
 gnu/system.scm          |  1 +
 2 files changed, 36 insertions(+), 39 deletions(-)

Hello Stefan,

For some reason I cannot apply this patch. Could you send an updated
revision based on master?

A few comments below:

> * gnu/bootloaders/grub.scm (<grub-image>)[resolution]: Replacement of the
> 'aspect-ratio' field.

You can write something like:

* gnu/bootloaders/grub.scm (<grub-image>)[aspect-ratio]: Remove this
field and replace it by ...
[resolution]: ... this field.

> +  (resolution grub-image-resolution
> +              (default '(1024 . 768)))
> +  (file       grub-image-file
> +              (default (file-append %artwork-repository
> +                                    "/grub/GuixSD-fully-black-4-3.svg"))))

I'm not sure about defaulting to this file. This record is meant to
describe a generic image. Could you keep this empty?

>  (define* (svg->png svg #:key width height)
> -  "Build a PNG of HEIGHT x WIDTH from SVG."
> +  "Build a PNG of HEIGHT x WIDTH from SVG if its file suffix is \".svg\".

I'm not sure having "svg->png" handle other file types than ".svg" is
very clear. 

> +                     #~(if (string-suffix? ".svg" #+svg)
> +                           (begin
> +                             (use-modules (gnu build svg))
> +                             (svg->png #+svg #$output
> +                                       #:width #$width
> +                                       #:height #$height))
> +                           (copy-file #+svg #$output))))))

You could move this check to "grub-background-image" procedure. So that
"svg->png" only deals with ".svg" files.

> +         (let ((resolution (grub-image-resolution image)))
> +           (svg->png (grub-image-file image)
> +                     #:width (car resolution)
> +                     #:height (cdr resolution))))))

"car" and "cdr" should be avoided. You can write something like that
instead:

--8<---------------cut here---------------start------------->8---
(match resolution
       ((width . height)
        (svg->png (grub-image-file image)
                  #:width width
                  #:height height)))
--8<---------------cut here---------------end--------------->8---

Thanks for this patch,

Mathieu

Hi Mathieu!

Many thanks for your reply! :-)

> Am 10.05.2020 um 09:07 schrieb Mathieu Othacehe <othacehe@gnu.org>:
> 
> For some reason I cannot apply this patch. Could you send an updated
> revision based on master?

I’ll try.

> A few comments below:
> 
>> * gnu/bootloaders/grub.scm (<grub-image>)[resolution]: Replacement of the
>> 'aspect-ratio' field.
> 
> You can write something like:
> 
> * gnu/bootloaders/grub.scm (<grub-image>)[aspect-ratio]: Remove this
> field and replace it by ...
> [resolution]: ... this field.

OK.

>> +  (resolution grub-image-resolution
>> +              (default '(1024 . 768)))
>> +  (file       grub-image-file
>> +              (default (file-append %artwork-repository
>> +                                    "/grub/GuixSD-fully-black-4-3.svg"))))
> 
> I'm not sure about defaulting to this file. This record is meant to
> describe a generic image. Could you keep this empty?

The point is that currently the grub-theme used by default is %default-theme, even if you set the theme field inside your bootloader-configuration to #f (this is documented). And %default-theme is using the very same file. 

If you now just want to modify the size of this image file to your screen resolution (I'd guess 1920 x 1080 is common), then you need to inherit form %default-theme and somehow change the resolution, probably making use of %background-image as well. This gets complicated and is not obvious – moreover as both variables are not documented.

With my proposed change, you can create a new grub-theme which has all the current guix defaults and only modify the parts you want to change:

(grub-theme (image (grub-image (resolution '(1920 . 1080)))))

Compare this with an alternative code to achieve the same, if the file field would default to #f:

(grub-theme (inherit %default-theme) 
           (image (grub-image (inherit %background-image) 
                              (resolution '(1920 . 1080))))))

Actually I question that it makes much sense to have a separate grub-image record at all, now that I'm about to remove the list of it from grub-theme. But I'm hesitating to change too much – that’s also the reason that I didn't delete %default-theme and %background-image.

So despite your comment, I’d go even further: For me something like this would look much easier without having any loss:

(grub-theme (resolution '(1920 . 1080)
           (file %my-neat-backround-svg)
           (color-highlight '((fg . green) (bg . black))))

Or to stay with the simpler examples above:

(grub-theme (resolution '(1920 . 1080)))

And any missing field would stay the same as the current default.

>> (define* (svg->png svg #:key width height)
>> -  "Build a PNG of HEIGHT x WIDTH from SVG."
>> +  "Build a PNG of HEIGHT x WIDTH from SVG if its file suffix is \".svg\".
> 
> I'm not sure having "svg->png" handle other file types than ".svg" is
> very clear. 
> 
>> +                     #~(if (string-suffix? ".svg" #+svg)
>> +                           (begin
>> +                             (use-modules (gnu build svg))
>> +                             (svg->png #+svg #$output
>> +                                       #:width #$width
>> +                                       #:height #$height))
>> +                           (copy-file #+svg #$output))))))
> 
> You could move this check to "grub-background-image" procedure. So that
> "svg->png" only deals with ".svg" files.

Yes, that makes sense.

>> +         (let ((resolution (grub-image-resolution image)))
>> +           (svg->png (grub-image-file image)
>> +                     #:width (car resolution)
>> +                     #:height (cdr resolution))))))
> 
> "car" and "cdr" should be avoided. You can write something like that
> instead:
> 
> --8<---------------cut here---------------start------------->8---
> (match resolution
>      ((width . height)
>       (svg->png (grub-image-file image)
>                 #:width width
>                 #:height height)))
> --8<---------------cut here---------------end--------------->8—

Looks nice.


Bye

Stefan