[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#41163] [PATCH] gnu: grub: Allow a PNG image and replace (aspect-rat
From: |
Stefan |
Subject: |
[bug#41163] [PATCH] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution). |
Date: |
Sun, 10 May 2020 17:43:47 +0200 |
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