* [bug#41163] [PATCH] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution).
@ 2020-05-09 22:44 Stefan
2020-05-10 7:07 ` Mathieu Othacehe
0 siblings, 1 reply; 6+ messages in thread
From: Stefan @ 2020-05-09 22:44 UTC (permalink / raw)
To: 41163
* 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(-)
diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 67736724a7..c70c3e260d 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -40,12 +40,12 @@
#:use-module (srfi srfi-2)
#:export (grub-image
grub-image?
- grub-image-aspect-ratio
+ grub-image-resolution
grub-image-file
grub-theme
grub-theme?
- grub-theme-images
+ grub-theme-image
grub-theme-color-normal
grub-theme-color-highlight
@@ -82,34 +82,28 @@ denoting a file name."
(define-record-type* <grub-image>
grub-image make-grub-image
grub-image?
- (aspect-ratio grub-image-aspect-ratio ;rational number
- (default 4/3))
- (file grub-image-file)) ;file-valued gexp (SVG)
+ (resolution grub-image-resolution
+ (default '(1024 . 768)))
+ (file grub-image-file
+ (default (file-append %artwork-repository
+ "/grub/GuixSD-fully-black-4-3.svg"))))
(define-record-type* <grub-theme>
+ ;; Default theme contributed by Felipe López.
grub-theme make-grub-theme
grub-theme?
- (images grub-theme-images
- (default '())) ;list of <grub-image>
+ (image grub-theme-image
+ (default (grub-image)))
(color-normal grub-theme-color-normal
- (default '((fg . cyan) (bg . blue))))
+ (default '((fg . light-gray) (bg . black))))
(color-highlight grub-theme-color-highlight
- (default '((fg . white) (bg . blue))))
+ (default '((fg . yellow) (bg . black))))
(gfxmode grub-gfxmode
(default '("auto")))) ;list of string
-(define %background-image
- (grub-image
- (aspect-ratio 4/3)
- (file (file-append %artwork-repository
- "/grub/GuixSD-fully-black-4-3.svg"))))
+(define %background-image (grub-image))
-(define %default-theme
- ;; Default theme contributed by Felipe López.
- (grub-theme
- (images (list %background-image))
- (color-highlight '((fg . yellow) (bg . black)))
- (color-normal '((fg . light-gray) (bg . black))))) ;XXX: #x303030
+(define %default-theme (grub-theme))
^L
;;;
@@ -117,32 +111,34 @@ denoting a file name."
;;;
(define (bootloader-theme config)
- "Return user defined theme in CONFIG if defined or %default-theme
+ "Return user defined theme in CONFIG if defined or a default theme
otherwise."
- (or (bootloader-configuration-theme config) %default-theme))
+ (or (bootloader-configuration-theme config) (grub-theme)))
(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\".
+Otherwise the picture in SVG is just copied."
(computed-file "grub-image.png"
(with-imported-modules '((gnu build svg))
(with-extensions (list guile-rsvg guile-cairo)
- #~(begin
- (use-modules (gnu build svg))
- (svg->png #+svg #$output
- #:width #$width
- #:height #$height))))))
-
-(define* (grub-background-image config #:key (width 1024) (height 768))
- "Return the GRUB background image defined in CONFIG with a ratio of
-WIDTH/HEIGHT, or #f if none was found."
- (let* ((ratio (/ width height))
- (image (find (lambda (image)
- (= (grub-image-aspect-ratio image) ratio))
- (grub-theme-images
- (bootloader-theme config)))))
+ #~(if (string-suffix? ".svg" #+svg)
+ (begin
+ (use-modules (gnu build svg))
+ (svg->png #+svg #$output
+ #:width #$width
+ #:height #$height))
+ (copy-file #+svg #$output))))))
+
+(define* (grub-background-image config)
+ "Return the GRUB background image defined in CONFIG or #f if none was found.
+If the suffix of the image file is \".svg\", then it is converted into a PNG
+file with the resolution provided in CONFIG."
+ (let* ((image (grub-theme-image (bootloader-theme config))))
(and image
- (svg->png (grub-image-file image)
- #:width width #:height height))))
+ (let ((resolution (grub-image-resolution image)))
+ (svg->png (grub-image-file image)
+ #:width (car resolution)
+ #:height (cdr resolution))))))
(define* (eye-candy config store-device store-mount-point
#:key port)
--
2.26.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [bug#41163] [PATCH] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution).
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
0 siblings, 1 reply; 6+ messages in thread
From: Mathieu Othacehe @ 2020-05-10 7:07 UTC (permalink / raw)
To: Stefan; +Cc: 41163
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug#41163] [PATCH] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution).
2020-05-10 7:07 ` Mathieu Othacehe
@ 2020-05-10 15:43 ` Stefan
2020-05-17 21:53 ` Stefan
0 siblings, 1 reply; 6+ messages in thread
From: Stefan @ 2020-05-10 15:43 UTC (permalink / raw)
To: Mathieu Othacehe; +Cc: 41163
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug#41163] [PATCH] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution).
2020-05-10 15:43 ` Stefan
@ 2020-05-17 21:53 ` Stefan
2020-05-17 22:03 ` Stefan
2020-05-19 7:36 ` bug#41163: " Mathieu Othacehe
0 siblings, 2 replies; 6+ messages in thread
From: Stefan @ 2020-05-17 21:53 UTC (permalink / raw)
To: Mathieu Othacehe; +Cc: 41163
* gnu/bootloaders/grub.scm (<grub-image>): Remove this record and replace it
by ...
(<grub-theme>)[image]: ... this field with the default from %background-image,
(<grub-theme>)[resolution]: ... this field with the defaults from 'width' and
'height' of 'grub-background-image'.
(<grub-theme>)[images]: Remove this field.
(svg->png): Use 'copy-file' instead of 'svg->png', if the suffix of the image
file is not ".svg".
(grub-background-image): Remove the arguments 'width' and 'height'.
(grub-theme-image): Add function.
(grub-theme-resolution): Add function.
(grub-theme-gfxmode): Add function.
(grub-image): Remove function.
(grub-image?): Remove function.
(grub-image-aspect-ratio): Remove function.
(grub-image-file): Remove function.
(grub-theme-images): Remove function.
(%default-theme): Remove variable.
(%background-image): Remove variable.
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 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. This also
makes the use of a special <grub-image> record unnecessary.
Moving the default values from '%background-image' and '%default-theme' into
<grub-theme> makes a customisation easier without (inherit) and allows to remove
the undocumented variables %background-image' and '%default-theme'.
---
gnu/bootloader/grub.scm | 93 ++++++++++++++++-------------------------
1 file changed, 37 insertions(+), 56 deletions(-)
diff --git a/gnu/bootloader/grub.scm b/gnu/bootloader/grub.scm
index 3f61b4a963..fb2e4cc00d 100644
--- a/gnu/bootloader/grub.scm
+++ b/gnu/bootloader/grub.scm
@@ -37,19 +37,13 @@
#:use-module (ice-9 regex)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-2)
- #:export (grub-image
- grub-image?
- grub-image-aspect-ratio
- grub-image-file
-
- grub-theme
+ #:export (grub-theme
grub-theme?
- grub-theme-images
+ grub-theme-image
+ grub-theme-resolution
grub-theme-color-normal
grub-theme-color-highlight
-
- %background-image
- %default-theme
+ grub-theme-gfxmode
grub-bootloader
grub-efi-bootloader
@@ -77,70 +71,57 @@ denoting a file name."
file))))
(#f file)))
-(define-record-type* <grub-image>
- grub-image make-grub-image
- grub-image?
- (aspect-ratio grub-image-aspect-ratio ;rational number
- (default 4/3))
- (file grub-image-file)) ;file-valued gexp (SVG)
-
(define-record-type* <grub-theme>
+ ;; Default theme contributed by Felipe López.
grub-theme make-grub-theme
grub-theme?
- (images grub-theme-images
- (default '())) ;list of <grub-image>
+ (image grub-theme-image
+ (default (file-append %artwork-repository
+ "/grub/GuixSD-fully-black-4-3.svg")))
+ (resolution grub-theme-resolution
+ (default '(1024 . 768)))
(color-normal grub-theme-color-normal
- (default '((fg . cyan) (bg . blue))))
+ (default '((fg . light-gray) (bg . black))))
(color-highlight grub-theme-color-highlight
- (default '((fg . white) (bg . blue))))
- (gfxmode grub-gfxmode
+ (default '((fg . yellow) (bg . black))))
+ (gfxmode grub-theme-gfxmode
(default '("auto")))) ;list of string
-(define %background-image
- (grub-image
- (aspect-ratio 4/3)
- (file (file-append %artwork-repository
- "/grub/GuixSD-fully-black-4-3.svg"))))
-
-(define %default-theme
- ;; Default theme contributed by Felipe López.
- (grub-theme
- (images (list %background-image))
- (color-highlight '((fg . yellow) (bg . black)))
- (color-normal '((fg . light-gray) (bg . black))))) ;XXX: #x303030
-
^L
;;;
;;; Background image & themes.
;;;
(define (bootloader-theme config)
- "Return user defined theme in CONFIG if defined or %default-theme
+ "Return user defined theme in CONFIG if defined or a default theme
otherwise."
- (or (bootloader-configuration-theme config) %default-theme))
+ (or (bootloader-configuration-theme config) (grub-theme)))
(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\".
+Otherwise the picture in SVG is just copied."
(computed-file "grub-image.png"
(with-imported-modules '((gnu build svg))
(with-extensions (list guile-rsvg guile-cairo)
- #~(begin
- (use-modules (gnu build svg))
- (svg->png #+svg #$output
- #:width #$width
- #:height #$height))))))
-
-(define* (grub-background-image config #:key (width 1024) (height 768))
- "Return the GRUB background image defined in CONFIG with a ratio of
-WIDTH/HEIGHT, or #f if none was found."
- (let* ((ratio (/ width height))
- (image (find (lambda (image)
- (= (grub-image-aspect-ratio image) ratio))
- (grub-theme-images
- (bootloader-theme config)))))
- (and image
- (svg->png (grub-image-file image)
- #:width width #:height height))))
+ #~(if (string-suffix? ".svg" #+svg)
+ (begin
+ (use-modules (gnu build svg))
+ (svg->png #+svg #$output
+ #:width #$width
+ #:height #$height))
+ (copy-file #+svg #$output))))))
+
+(define* (grub-background-image config)
+ "Return the GRUB background image defined in CONFIG or #f if none was found.
+If the suffix of the image file is \".svg\", then it is converted into a PNG
+file with the resolution provided in CONFIG."
+ (let* ((theme (bootloader-theme config))
+ (image (grub-theme-image theme)))
+ (and image
+ (match (grub-theme-resolution theme)
+ (((? number? width) . (? number? height))
+ (svg->png image #:width width #:height height))
+ (_ #f)))))
(define* (eye-candy config store-device store-mount-point
#:key system port)
@@ -153,7 +134,7 @@ system string---e.g., \"x86_64-linux\"."
(define setup-gfxterm-body
(let ((gfxmode
(or (and-let* ((theme (bootloader-configuration-theme config))
- (gfxmode (grub-gfxmode theme)))
+ (gfxmode (grub-theme-gfxmode theme)))
(string-join gfxmode ";"))
"auto")))
--
2.26.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [bug#41163] [PATCH] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution).
2020-05-17 21:53 ` Stefan
@ 2020-05-17 22:03 ` Stefan
2020-05-19 7:36 ` bug#41163: " Mathieu Othacehe
1 sibling, 0 replies; 6+ messages in thread
From: Stefan @ 2020-05-17 22:03 UTC (permalink / raw)
To: Mathieu Othacehe; +Cc: 41163
Hi Mathieu!
> Am 10.05.2020 um 09:07 schrieb Mathieu Othacehe <othacehe@gnu.org>:
>
> You could move this check to "grub-background-image" procedure. So that
> "svg->png" only deals with ".svg" files.
I wasn’t able to do this. To check the suffix I need a gexp inside ‘grub-background-image’. But calling ‘svg->png’ then is a nested gexp. It seems to me as if nesting gexps is not possible – at least I had a lot of trouble trying this.
Bye
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#41163: [PATCH] gnu: grub: Allow a PNG image and replace (aspect-ratio) with (resolution).
2020-05-17 21:53 ` Stefan
2020-05-17 22:03 ` Stefan
@ 2020-05-19 7:36 ` Mathieu Othacehe
1 sibling, 0 replies; 6+ messages in thread
From: Mathieu Othacehe @ 2020-05-19 7:36 UTC (permalink / raw)
To: Stefan; +Cc: 41163-done
Hello Stefan,
> (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 renamed this one "image->png" which is closer to what's happening I
think.
Tested your patch with both the default configuration and a custom PNG
image, it works fine.
Pushed as 9cdb10d5.
Bonus points if you come up with a documentation patch describing how to
setup a custom svg/png image :)
Thanks,
Mathieu
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-19 7:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2020-05-17 21:53 ` Stefan
2020-05-17 22:03 ` Stefan
2020-05-19 7:36 ` bug#41163: " Mathieu Othacehe
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).