On 2022-10-11 12:54, Taiju HIGASHI wrote:

> 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?).

Oh, I missed the map over the list elements and slightly missread the
code.  I thought (according to my incorrect perception of
implementation) extra-config have to be either sxml or string, that's is
why I said that it will fail earlier because plan string value won't
satisfy list predicate attached to extra-config field, but in a fact
extra-config is always a list, but can be a list of sxml's and strings
mixed together.

Thus, some of my comments are invalid.  Sorry for the confusion.  I'll
rephrase and elaborate in the later message.

>
> 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?
>

It's not possible with the current 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.
>

As Liliana mentioned in the other message, it's better not to set
anything implicitly.  It's better to keep the name, but change the
implementation and remove implicitly and unconditionally added
directory.

>>> +   "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

-- 
Best regards,
Andrew Tropin