| Message ID | 20220921002921.23631-2-higashi@taiju.info |
|---|---|
| State | New |
| Headers | show |
| Series | Support user's fontconfig. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git-branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Am Mittwoch, dem 21.09.2022 um 09:29 +0900 schrieb Taiju HIGASHI: > * gnu/home/services/fontutils.scm (add-fontconfig-config-file): > Support user's fontconfig. > --- > gnu/home/services/fontutils.scm | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/gnu/home/services/fontutils.scm > b/gnu/home/services/fontutils.scm > index 6062eaed6a..3ea8b1db74 100644 > --- a/gnu/home/services/fontutils.scm > +++ b/gnu/home/services/fontutils.scm > @@ -1,6 +1,7 @@ > ;;; GNU Guix --- Functional package management for GNU > ;;; Copyright © 2021 Andrew Tropin <andrew@trop.in> > ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz> > +;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info> > ;;; > ;;; This file is part of GNU Guix. > ;;; > @@ -21,6 +22,7 @@ (define-module (gnu home services fontutils) > #:use-module (gnu home services) > #:use-module (gnu packages fontutils) > #:use-module (guix gexp) > + #:use-module (srfi srfi-1) > > #:export (home-fontconfig-service-type)) > > @@ -33,15 +35,18 @@ (define-module (gnu home services fontutils) > ;;; > ;;; Code: > > -(define (add-fontconfig-config-file he-symlink-path) > +(define (add-fontconfig-config-file font-config) > `(("fontconfig/fonts.conf" > ,(mixed-text-file > "fonts.conf" > "<?xml version='1.0'?> > <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> > <fontconfig> > - <dir>~/.guix-home/profile/share/fonts</dir> > -</fontconfig>")))) > + <dir>~/.guix-home/profile/share/fonts</dir>\n" > + (if (null? font-config) > + "" > + (string-join font-config "\n" 'suffix)) > + "</fontconfig>\n")))) I think it'd be wiser to pretty-print SXML here. The structure could look something like `(fontconfig (dir "~/.guix-home/profile/share/fonts") ,@(extra-user-config ...)) Also, for the particular use case of handling multiple profiles gracefully (rather than the current status quo) I think fontconfig- service-type should be able to construct (dir "#$profile/share/fonts") style entries on its own. However, given that multiple profiles aren't supported yet, this is future work. Cheers
Hi Liliana, Thank you for your review. >> -(define (add-fontconfig-config-file he-symlink-path) >> +(define (add-fontconfig-config-file font-config) >> `(("fontconfig/fonts.conf" >> ,(mixed-text-file >> "fonts.conf" >> "<?xml version='1.0'?> >> <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> >> <fontconfig> >> - <dir>~/.guix-home/profile/share/fonts</dir> >> -</fontconfig>")))) >> + <dir>~/.guix-home/profile/share/fonts</dir>\n" >> + (if (null? font-config) >> + "" >> + (string-join font-config "\n" 'suffix)) >> + "</fontconfig>\n")))) > I think it'd be wiser to pretty-print SXML here. > The structure could look something like > `(fontconfig > (dir "~/.guix-home/profile/share/fonts") > ,@(extra-user-config ...)) That's definitely better! Does this assume that SXML will also accept additional user settings? > Also, for the particular use case of handling multiple profiles > gracefully (rather than the current status quo) I think fontconfig- > service-type should be able to construct (dir "#$profile/share/fonts") > style entries on its own. However, given that multiple profiles aren't > supported yet, this is future work. Noted. I believe that even with the current patch, it is possible to add arbitrary directories, so it will be better than what we have now. Cheers
Am Mittwoch, dem 21.09.2022 um 18:59 +0900 schrieb Taiju HIGASHI: > Hi Liliana, > > Thank you for your review. > > > > -(define (add-fontconfig-config-file he-symlink-path) > > > +(define (add-fontconfig-config-file font-config) > > > `(("fontconfig/fonts.conf" > > > ,(mixed-text-file > > > "fonts.conf" > > > "<?xml version='1.0'?> > > > <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> > > > <fontconfig> > > > - <dir>~/.guix-home/profile/share/fonts</dir> > > > -</fontconfig>")))) > > > + <dir>~/.guix-home/profile/share/fonts</dir>\n" > > > + (if (null? font-config) > > > + "" > > > + (string-join font-config "\n" 'suffix)) > > > + "</fontconfig>\n")))) > > I think it'd be wiser to pretty-print SXML here. > > The structure could look something like > > `(fontconfig > > (dir "~/.guix-home/profile/share/fonts") > > ,@(extra-user-config ...)) > > That's definitely better! > Does this assume that SXML will also accept additional user settings? It assumes that whatever (extra-user-config ...) does, it returns a list of SXML nodes, e.g. ((dir "~/.fonts")). Writing correct SXML should be comparatively simpler to writing correct XML. > > Also, for the particular use case of handling multiple profiles > > gracefully (rather than the current status quo) I think fontconfig- > > service-type should be able to construct (dir > > "#$profile/share/fonts") style entries on its own. However, given > > that multiple profiles aren't supported yet, this is future work. > > Noted. I believe that even with the current patch, it is possible to > add arbitrary directories, so it will be better than what we have > now. That's fine, just know that this use case might at some point become obsolete thanks to a better implementation :)
Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> writes: > Am Mittwoch, dem 21.09.2022 um 18:59 +0900 schrieb Taiju HIGASHI: >> Hi Liliana, >> >> Thank you for your review. >> >> > > -(define (add-fontconfig-config-file he-symlink-path) >> > > +(define (add-fontconfig-config-file font-config) >> > > `(("fontconfig/fonts.conf" >> > > ,(mixed-text-file >> > > "fonts.conf" >> > > "<?xml version='1.0'?> >> > > <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> >> > > <fontconfig> >> > > - <dir>~/.guix-home/profile/share/fonts</dir> >> > > -</fontconfig>")))) >> > > + <dir>~/.guix-home/profile/share/fonts</dir>\n" >> > > + (if (null? font-config) >> > > + "" >> > > + (string-join font-config "\n" 'suffix)) >> > > + "</fontconfig>\n")))) >> > I think it'd be wiser to pretty-print SXML here. >> > The structure could look something like >> > `(fontconfig >> > (dir "~/.guix-home/profile/share/fonts") >> > ,@(extra-user-config ...)) >> >> That's definitely better! >> Does this assume that SXML will also accept additional user settings? > It assumes that whatever (extra-user-config ...) does, it returns a > list of SXML nodes, e.g. ((dir "~/.fonts")). Writing correct SXML > should be comparatively simpler to writing correct XML. I just sent you the v2 patch. It uses SXML to handle the user's extra configs. I also made it so that the user can pass SXML directly. I also wrote a test but did not include it in the patch because I thought it would be a technical debt. I'm attaching that as a reference. ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info> ;;; ;;; This file is part of GNU Guix. ;;; ;;; GNU Guix is free software; you can redistribute it and/or modify it ;;; under the terms of the GNU General Public License as published by ;;; the Free Software Foundation; either version 3 of the License, or (at ;;; your option) any later version. ;;; ;;; GNU Guix is distributed in the hope that it will be useful, but ;;; WITHOUT ANY WARRANTY; without even the implied warranty of ;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the ;;; GNU General Public License for more details. ;;; ;;; You should have received a copy of the GNU General Public License ;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>. (define-module (test-home-services-fontutils) #:use-module (gnu services) #:use-module (gnu home services) #:use-module (gnu home services fontutils) #:use-module (guix tests) #:use-module (sxml simple) #:use-module (srfi srfi-1) #:use-module (srfi srfi-64)) ;; or (@@ (gnu home services fontutils) add-fontconfig-config-file) (define add-fontconfig-config-file (let* ((extensions (service-type-extensions home-fontconfig-service-type)) (extension (find (lambda (ext) (eq? (service-extension-target ext) home-xdg-configuration-files-service-type)) extensions)) (compute (service-extension-compute extension))) compute)) (define (assert-fontconfig-value value expected) (mock ((guix gexp) mixed-text-file (lambda* (name #:key guile #:rest text) (let ((text (string-join text ""))) (unless (string= text expected) (error "assert failed. actual: %s" text))))) (add-fontconfig-config-file value) #t)) (test-begin "home-services-fontutils") (test-assert "fontconfig (default value)" (assert-fontconfig-value '() "\ <?xml version='1.0'?> <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> <fontconfig><dir>~/.guix-home/profile/share/fonts</dir></fontconfig> ")) (test-assert "fontconfig (a text)" (assert-fontconfig-value '("<foo>foo</foo>") "\ <?xml version='1.0'?> <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> <fontconfig><dir>~/.guix-home/profile/share/fonts</dir><foo>foo</foo></fontconfig> ")) (test-assert "fontconfig (multiple texts)" (assert-fontconfig-value '("<foo>foo</foo>" "<bar><baz>baz</baz></bar>") "\ <?xml version='1.0'?> <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> <fontconfig><dir>~/.guix-home/profile/share/fonts</dir><foo>foo</foo><bar><baz>baz</baz></bar></fontconfig> ")) (test-assert "fontconfig (a sxml)" (assert-fontconfig-value '((foo foo)) "\ <?xml version='1.0'?> <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> <fontconfig><dir>~/.guix-home/profile/share/fonts</dir><foo>foo</foo></fontconfig> ")) (test-assert "fontconfig (multiple sxml)" (assert-fontconfig-value '((foo foo) (bar (baz baz))) "\ <?xml version='1.0'?> <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> <fontconfig><dir>~/.guix-home/profile/share/fonts</dir><foo>foo</foo><bar><baz>baz</baz></bar></fontconfig> ")) (test-error "fontconfig (invalid value)" (add-fontconfig-config-file '(123))) (test-end "home-services-fontutils") >> > Also, for the particular use case of handling multiple profiles >> > gracefully (rather than the current status quo) I think fontconfig- >> > service-type should be able to construct (dir >> > "#$profile/share/fonts") style entries on its own. However, given >> > that multiple profiles aren't supported yet, this is future work. >> >> Noted. I believe that even with the current patch, it is possible to >> add arbitrary directories, so it will be better than what we have >> now. > That's fine, just know that this use case might at some point become > obsolete thanks to a better implementation :) No problem. I would like to solve the current problem first. A better implementation is always welcome :) Cheers
Am Donnerstag, dem 22.09.2022 um 10:27 +0900 schrieb Taiju HIGASHI: > I also wrote a test but did not include it in the patch because I > thought it would be a technical debt. > I'm attaching that as a reference. Added tests are always welcome.
diff --git a/gnu/home/services/fontutils.scm b/gnu/home/services/fontutils.scm index 6062eaed6a..3ea8b1db74 100644 --- a/gnu/home/services/fontutils.scm +++ b/gnu/home/services/fontutils.scm @@ -1,6 +1,7 @@ ;;; GNU Guix --- Functional package management for GNU ;;; Copyright © 2021 Andrew Tropin <andrew@trop.in> ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz> +;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info> ;;; ;;; This file is part of GNU Guix. ;;; @@ -21,6 +22,7 @@ (define-module (gnu home services fontutils) #:use-module (gnu home services) #:use-module (gnu packages fontutils) #:use-module (guix gexp) + #:use-module (srfi srfi-1) #:export (home-fontconfig-service-type)) @@ -33,15 +35,18 @@ (define-module (gnu home services fontutils) ;;; ;;; Code: -(define (add-fontconfig-config-file he-symlink-path) +(define (add-fontconfig-config-file font-config) `(("fontconfig/fonts.conf" ,(mixed-text-file "fonts.conf" "<?xml version='1.0'?> <!DOCTYPE fontconfig SYSTEM 'fonts.dtd'> <fontconfig> - <dir>~/.guix-home/profile/share/fonts</dir> -</fontconfig>")))) + <dir>~/.guix-home/profile/share/fonts</dir>\n" + (if (null? font-config) + "" + (string-join font-config "\n" 'suffix)) + "</fontconfig>\n")))) (define (regenerate-font-cache-gexp _) `(("profile/share/fonts" @@ -49,6 +54,8 @@ (define (regenerate-font-cache-gexp _) (define home-fontconfig-service-type (service-type (name 'home-fontconfig) + (compose concatenate) + (extend append) (extensions (list (service-extension home-xdg-configuration-files-service-type @@ -59,7 +66,7 @@ (define home-fontconfig-service-type (service-extension home-profile-service-type (const (list fontconfig))))) - (default-value #f) + (default-value '()) (description "Provides configuration file for fontconfig and make fc-* utilities aware of font packages installed in Guix Home's profile.")))