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

[Stefan]

Hi Mathieu!

Many thanks for your reply! :-)

> Am 10.05.2020 um 09:07 schrieb Mathieu Othacehe <address@hidden>:
> 
> 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