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

Previous Next

Full log

Message #19 received at 45829 <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 45829 <at> debbugs.gnu.org
Subject: Re: bug#45829: 28.0.50; Some tweaks to the color widget, from
 wid-edit+.el
Date: Sat, 16 Jan 2021 09:10:35 -0300

[Message part 1 (text/plain, inline)]

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

> Mauro Aranda <maurooaranda <at> 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.

[0001-Tweaks-to-the-color-widget-Bug-45829.patch (text/x-patch, inline)]

From be6b966173a9d0d455df884d28efcd6aaa0d54f7 Mon Sep 17 00:00:00 2001
From: Drew Adams <drew.adams <at> oracle.com>
Date: Sat, 16 Jan 2021 08:56:55 -0300
Subject: [PATCH] Tweaks to the color widget (Bug#45829)

* lisp/wid-edit.el (widget-color-match, widget-color-validate): New
functions.
(color): Use the new functions.  Base size on longest defined color
name, defaulting to the longest RGB hex string.
---
 lisp/wid-edit.el | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 7dda04eda2..68a0d3d235 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -4026,17 +4026,19 @@ widget-boolean-prompt-value
 
 ;;; The `color' Widget.
 
-;; 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 13 ; Longest RGB hex string.
+                   (mapcar #'length (defined-colors))))
   :tag "Color"
   :value "black"
   :completions (or facemenu-color-alist (defined-colors))
   :sample-face-get 'widget-color-sample-face-get
   :notify 'widget-color-notify
+  :match #'widget-color-match
+  :validate #'widget-color-validate
   :action 'widget-color-action)
 
 (defun widget-color-value-create (widget)
@@ -4085,6 +4087,19 @@ widget-color-notify
   (overlay-put (widget-get widget :sample-overlay)
 	       'face (widget-apply widget :sample-face-get))
   (widget-default-notify widget child event))
+
+(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))))
+
+(defun widget-color-validate (widget)
+  "Check that WIDGET's value is a valid color."
+  (let ((value (widget-value widget)))
+    (unless (widget-color-match widget value)
+      (widget-put widget :error (format "Invalid color: %S" value))
+      widget)))
 
 ;;; The Help Echo
 
-- 
2.29.2

[0001-Add-test-for-the-widget-color-match-function-Bug-458.patch (text/x-patch, inline)]

From 19efd50021fd5e415305bd367d7fbf6a001db987 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda <at> gmail.com>
Date: Tue, 12 Jan 2021 19:19:21 -0300
Subject: [PATCH] Add test for the widget-color-match function (Bug#45829)

* test/lisp/wid-edit-tests.el (widget-test-color-match): New test.
---
 test/lisp/wid-edit-tests.el | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/test/lisp/wid-edit-tests.el b/test/lisp/wid-edit-tests.el
index 17fdfefce8..f843649784 100644
--- a/test/lisp/wid-edit-tests.el
+++ b/test/lisp/wid-edit-tests.el
@@ -322,4 +322,15 @@ widget-test-widget-move
     (widget-backward 1)
     (should (string= "Second" (widget-value (widget-at))))))
 
+(ert-deftest widget-test-color-match ()
+  "Test that the :match function for the color widget works."
+  (let ((widget (widget-convert 'color)))
+    (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"))))
+
 ;;; wid-edit-tests.el ends here
-- 
2.29.2

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.