bug#45829: 28.0.50; Some tweaks to the color widget, from wid-edit+.el

["Basil L. Contovounesios" <contovob@tcd.ie>]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Opinions?

Just some minor nits from me.

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

> +(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)?

> +(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.

> +    (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,

-- 
Basil