"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> Opinions?
>
> Just some minor nits from me.

Hi Basil, thanks for taking a look.

>> -;; Fixme: match
>>  (define-widget 'color 'editable-field
>>    "Choose a color name (with sample)."
>>    :format "%{%t%}: %v (%{sample%})\n"
>>    :value-create 'widget-color-value-create
>> -  :size 10
>> +  :size (1+ (apply #'max (mapcar #'length (defined-colors))))
>
> Is defined-colors guaranteed to return non-nil?
> If not, you need (apply #'max 0 ...).

Thanks for catching this one.  I modified it so as to default to 13,
which would be the longest hex string.

>> +(defun widget-color-match (_widget value)
>> +  "Non-nil if VALUE is a defined color or a RGB hex string."
>> +  (and (stringp value)
>> +       (or (color-defined-p value)
>> +           (string-match-p "^#\\([[:xdigit:]]\\{3\\}\\)\\{1,4\\}$" value))))
>
> Shouldn't that be "\\`#[[:xdigit:]]\\{3\\}\\{1,4\\}\\'"
> or at least "\\`#\\(?:[[:xdigit:]]\\{3\\}\\)\\{1,4\\}\\'"
> (if you want to be explicit)?

I prefer the latter.  I kept the ^ and $, though.

>> +(ert-deftest widget-test-color-match ()
>> +  "Test that the :match function for the color widget works."
>> +  (let* ((widget (widget-convert 'color)))
>
> Nit: could also be let.

Ah yes, the let* was just a leftover.

>> +    (should (widget-apply widget :match "red"))
>> +    (should (widget-apply widget :match "#fa3"))
>> +    (should (widget-apply widget :match "#ff0000"))
>> +    (should (widget-apply widget :match "#111222333"))
>> +    (should (widget-apply widget :match "#111122223333"))
>> +    (should-not (widget-apply widget :match "someundefinedcolorihope"))
>> +    (should-not (widget-apply widget :match "#11223"))))
>
> Thanks,

New patch attached, thanks.