Subject: [bug#57963] [PATCH v5 2/2] home: services: Support user's fontconfig configuration.

From: Taiju HIGASHI <[email-masked]>
Date: Tue, 11 Oct 2022 12:54:19 +0900

Hi Andrew,

Thank you for your review!

>> +(define (string-list? value)
>> +  (and (pair? value) (every string? value)))
>
> Better to use list? here and in the other places, at least for the
> consistency, but also for semantic meaning.

OK. I'll rewrite it to below.

--8<---------------cut here---------------start------------->8---
(define (string-list? value)
  (and (list? value) (every string? value)))
--8<---------------cut here---------------end--------------->8---

>> +
>> +(define (serialize-string-list field-name value)
>> +  (sxml->xml-string
>> +   (map
>> +    (lambda (path) `(dir ,path))
>> +    (if (member guix-home-font-dir value)
>> +        value
>> +        (append (list guix-home-font-dir) value)))))
>> +
>> +(define (serialize-string field-name value)
>> +  (define (serialize type value)
>> +    (sxml->xml-string
>> +     `(alias
>> +       (family ,type)
>> +       (prefer
>> +        (family ,value)))))
>> +  (match (list field-name value)
>> +    (('default-font-serif-family family)
>> +     (serialize 'serif family))
>> +    (('default-font-sans-serif-family family)
>> +     (serialize 'sans-serif family))
>> +    (('default-font-monospace-family family)
>> +     (serialize 'monospace family))))
>> +
>> +(define-maybe string)
>> +
>> +(define extra-config-list? list?)
>> +
>> +(define-maybe extra-config-list)
>> +
>> +(define (serialize-extra-config-list field-name value)
>> +  (sxml->xml-string
>> +   (map (match-lambda
>> +          ((? pair? sxml) sxml)
>
> Other branches would never be visited because it will fail earlier by
> define-configuration predicate check for extra-config-list? (which is
> basically list?).

We can specify invalid value such as (list "foo" '(foo bar) 123).

> Also, making multi-type fields is debatable, but isn't great IMO.

I see. If we had to choose one or the other, I would prefer the
string-type field.

> If serialization would support G-exps, we could write
>
> (list #~"RAW_XML_HERE")
>
> or even something like this:
>
> (list #~(READ-THE-WHOLE-FILE #$(local-file "our-old.xml")))

Does it mean that the specification does not allow it now? Or does it
mean that it is not possible with my implementation?

>> +          ((? string? xml) (xml->sxml xml))
>> +          (else
>> +           (raise (formatted-message
>> + (G_ "'extra-config' type must be xml string or sxml list, was
>> given: ~a")
>> +                   value))))
>> +        value)))
>> +
>> +(define-configuration home-fontconfig-configuration
>> +  (font-directories
>> +   (string-list (list guix-home-font-dir))
>
> It's not a generic string-list, but a specific font-directories-list
> with extra logic inside.
>
> Also, because guix-home-font-dir always added to the list, the default
> value should '() and field should be called additional-font-directories
> instead.  Otherwise it will be confusing, why guix-home-font-dir is not
> removed from the final configuration, when this field is set to a
> different value.
>
> I skimmed previous messages, but sorry, if I missed any already
> mentioned points.

Sure, It is more appropriate that the field type is to
font-directories-list field name is to additional-font-directories.

>> +   "The directory list that provides fonts.")
>> +  (default-font-serif-family
>> +    maybe-string
>> +    "The preffered default fonts of serif.")
>> +  (default-font-sans-serif-family
>> +    maybe-string
>> +    "The preffered default fonts of sans-serif.")
>> +  (default-font-monospace-family
>> +    maybe-string
>> +    "The preffered default fonts of monospace.")
>> +  (extra-config
>> +   maybe-extra-config-list
>> +   "Extra configuration values to append to the fonts.conf."))
>> +
>> +(define (add-fontconfig-config-file user-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>"))))
>> +<fontconfig>"
>> +       (serialize-configuration user-config home-fontconfig-configuration-fields)
>
> Just a thought for the future and a point for configuration module
> improvements: It would be cool if serialize-configuration and all other
> serialize- functions returned a G-exps, this way we could write
> something like that:
>
> (home-fontconfig-configuration
>  (font-directories (list (file-append font-iosevka "/share/fonts"))))

Nice.

Thanks,
--
Taiju