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

[Andrew Tropin <andrew@trop.in>]

[-- Attachment #1: Type: text/plain, Size: 6947 bytes --]

On 2022-10-12 20:38, Taiju HIGASHI wrote:

> Andrew Tropin <andrew@trop.in> writes:
>
>> 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.
>
> I was worried that I was the only one who did not understand the code I
> wrote, but I've relieved to hear that it was a misunderstanding :)
>
> Is it OK to have multiple data types (XML string and SXML list) in a
> list?
>

I think it's not a great practice, I'll describe an alternative approach
in the other 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.
>
> I'll try to modify it so that it can support G-exps.
>
>>>>> + ((? 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.
>
> OK.  I'll modify the default value to an empty list and include
> ~/.guix-home/profile/share/fonts in the sample code in the
> documentation.
>

The default value is good, but the code, which always adds
~/.guix-home/profile/share/fonts to fontdirs is not.

--8<---------------cut here---------------start------------->8---
+    (if (member guix-home-font-dir value)
+        value
+        (append (list guix-home-font-dir) value))
--8<---------------cut here---------------end--------------->8---


>>>>> +   "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
>
> Thanks,
> --
> Taiju

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]