From: Stefan <stefan-guix@vodafonemail.de>
To: Mathieu Othacehe <othacehe@gnu.org>
Cc: 41163@debbugs.gnu.org
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 [thread overview]
Message-ID: <9A0DB516-B428-40D9-8E00-2C189457F425@vodafonemail.de> (raw)
In-Reply-To: <87y2q0i7xx.fsf@gnu.org>
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
next prev parent reply other threads:[~2020-05-10 15:43 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-09 22:44 [bug#41163] [PATCH] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution) Stefan
2020-05-10 7:07 ` Mathieu Othacehe
2020-05-10 15:43 ` Stefan [this message]
2020-05-17 21:53 ` Stefan
2020-05-17 22:03 ` Stefan
2020-05-19 7:36 ` bug#41163: " Mathieu Othacehe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9A0DB516-B428-40D9-8E00-2C189457F425@vodafonemail.de \
--to=stefan-guix@vodafonemail.de \
--cc=41163@debbugs.gnu.org \
--cc=othacehe@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).