On 2022-10-12 20:38, Taiju HIGASHI wrote: > Andrew Tropin 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" >>>>> " >>>>> >>>>> - >>>>> - ~/.guix-home/profile/share/fonts >>>>> -")))) >>>>> +" >>>>> + (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