bug#69942: 30.0.50; Fontification of radio-button widget labels

[Stephen Berman via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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

On Sun, 24 Mar 2024 19:47:16 +0100 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Sat, 23 Mar 2024 18:05:30 -0300 Mauro Aranda <maurooaranda@gmail.com> wrote:
>
>> Stephen Berman <stephen.berman@gmx.net> writes:
>>
>>> In bug#69941 I reported a faulty fontification of radio-button widgets
>>> and noted in passing that the labels associated with the radio buttons
>>> also have unexpected faces, namely, the widget-inactive face regardless
>>> of whether the associated radio buttons are inactive or active (except
>>> for the label of a radio button that has been pressed, which has the
>>> default face).  While the faulty fontification discussed in bug#69941
>>> appears to be a real bug, the widget-inactive face assigned to
>>> radio-button labels is apparently by design -- it was present in the
>>> initial commit of the widget library.  But this seems to me to have been
>>> a UX mistake, since it effectively ignores the semantics implied by the
>>> name widget-inactive.  I think a less surprising UI would be for the
>>> labels to be fontified according to the widget's activation state:
>>> default face when the widget is active and widget-inactive face when
>>> it's inactive.  The attached patches provide two possible
>>> implementations of this UI.
>>>
>>> The first patch makes the change unconditionally, treating the current
>>> fontification as a UI/UX bug.  But it may be argued that this aspect of
>>> the widget UI should not be unconditionally changed, since it was
>>> apparently a deliberate design choice and there have been (AFAIK) no
>>> complaints about the semantic discrepancy till now.  The lack of
>>> complaint could be because the widget-inactive face inherits the shadow
>>> face, so it is not sharply different from the default face. But if one
>>> uses a very different face (as I did for illustrative purposes in
>>> bug#69941), the inconsistency is very obvious and (IMO) jarring.
>>> Nevertheless, to allow keeping the current fontification, the second
>>> patch conditionalizes the change from the current fontification by means
>>> of a user option (with the default being the current fontification).
>>>
>>> Is either of these changes acceptable?
>>
>> Thanks for working on this.  What about adding a widget-unselected face?
>> I think that might be the intention with using the widget-inactive face
>> for unselected radio items.
>
> Yes, I agree that was likely the intention.  But I think it's
> superfluous: after all, the distinction between selected (or chosen) and
> unselected items is already clear from the appearance of the radio
> buttons or, with checklist widgets, the check boxes (my patch neglected
> checklists, but it's straightforward to account for them: in
> widget-checklist-add-item the (widget-apply child :deactivate) sexp
> should be wrapped in an (unless widget-radio-face-from-state ...)).
>
> On the other hand, with an unselected face for the labels of the radio
> button or check boxes, if it defaults to inheriting the shadow face for
> unselected items, that corresponds to the current appearance with the
> widget-inactive face, and by setting the widget-unselected face to the
> default face, all labels would appear the same, which is what I want.
> So for me that's an acceptable alternative to my proposed defcustom.  I
> tried to implement it, but I'm not very conversant with the workings of
> widget properties and how to apply faces depending on the widget's
> state, and I haven't managed to come up with a working implementation
> yet.  I'll keep trying, but you or someone else might be able to do it
> sooner.
>
> (There is another argument, besides superfluousness, against using a
> separate face for unselected items: using multiple check boxes instead
> of a checklist, as e.g. recentf-edit-list does.  With these the label of
> each check box is supplied by the :tag property, so it is not touched by
> the current handling in terms of the child widget's activation state.
> I'm not sure if using an unselected face here would be unproblematic or
> not.)

Ok, I've gotten further with implementing disinguishing by faces
selected (chosen) and unselected radio buttons in radio-button-choice
widgets and check boxes in checklist widgets, see the attached patch.
Initial tests seem ok, but it definitely needs more testing.

Steve Berman


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: selected and unselected widgets --]
[-- Type: text/x-patch, Size: 5205 bytes --]

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 172da3db1e0..005aa918087 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -555,6 +555,27 @@ widget-specify-active
       (delete-overlay inactive)
       (widget-put widget :inactive nil))))

+(defface widget-unselected
+  '((t :inherit shadow))
+  "Face used for unselected widgets."
+  :group 'widget-faces
+  :version "30.1")
+
+(defun widget-specify-unselected (widget from to)
+  "Fontify WIDGET as unselected (not chosen)."
+  (let ((overlay (make-overlay from to nil t nil)))
+    (overlay-put overlay 'face 'widget-unselected)
+    (overlay-put overlay 'evaporate t)
+    (overlay-put overlay 'priority 100)
+    (widget-put widget :unselected overlay)))
+
+(defun widget-specify-selected (widget)
+  "Remove fontification of WIDGET as unselected (not chosen)."
+  (let ((unselected (widget-get widget :unselected)))
+    (when unselected
+      (delete-overlay unselected)
+      (widget-put widget :unselected nil))))
+
 ;;; Widget Properties.

 (defsubst widget-type (widget)
@@ -2415,10 +2436,16 @@ 'checkbox
 (defun widget-checkbox-action (widget &optional event)
   "Toggle checkbox, notify parent, and set active state of sibling."
   (widget-toggle-action widget event)
-  (let ((sibling (widget-get-sibling widget)))
+  (let* ((sibling (widget-get-sibling widget))
+         (from (widget-get sibling :from))
+	 (to (widget-get sibling :to)))
     (when sibling
-      (widget-apply sibling
-	            (if (widget-value widget) :activate :deactivate))
+      (if (widget-value widget)
+          (progn
+            (widget-apply sibling :activate)
+            (widget-specify-selected sibling))
+        :deactivate
+        (widget-specify-unselected sibling from to))
       (widget-clear-undo))))

 ;;; The `checklist' Widget.
@@ -2474,15 +2501,19 @@ widget-checklist-add-item
 	       ((eq escape ?v)
 		(setq child
 		      (cond ((not chosen)
-			     (let ((child (widget-create-child widget type)))
-			       (widget-apply child :deactivate)
+			     (let* ((child (widget-create-child widget type))
+                                    (from (widget-get child :from))
+			            (to (widget-get child :to)))
+                               (widget-specify-unselected child from to)
 			       child))
                             ((widget-inline-p type t)
 			     (widget-create-child-value
-			      widget type (cdr chosen)))
+			      widget type (cdr chosen))
+                             (widget-specify-selected child))
 			    (t
 			     (widget-create-child-value
-			      widget type (car (cdr chosen)))))))
+			      widget type (car (cdr chosen)))
+                             (widget-specify-selected child)))))
 	       (t
 		(error "Unknown escape `%c'" escape)))))
      ;; Update properties.
@@ -2653,8 +2684,11 @@ widget-radio-add-item
 				(widget-create-child-value
 				 widget type value)
 			      (widget-create-child widget type)))
-		(unless chosen
-		  (widget-apply child :deactivate)))
+                (if chosen
+                    (widget-specify-selected child)
+                  (let ((from (widget-get child :from))
+			(to (widget-get child :to)))
+                    (widget-specify-unselected child from to))))
 	       (t
 		(error "Unknown escape `%c'" escape)))))
      ;; Update properties.
@@ -2704,14 +2738,17 @@ widget-radio-value-set
     (dolist (current (widget-get widget :children))
       (let* ((button (widget-get current :button))
 	     (match (and (not found)
-			 (widget-apply current :match value))))
+			 (widget-apply current :match value)))
+             (from (widget-get current :from))
+	     (to (widget-get current :to)))
 	(widget-value-set button match)
 	(if match
-	    (progn
-	      (widget-value-set current value)
-	      (widget-apply current :activate))
-	  (widget-apply current :deactivate))
-	(setq found (or found match))))))
+            (progn
+              (widget-value-set current value)
+              (widget-apply current :activate)
+              (widget-specify-selected current))
+          (widget-specify-unselected current from to))
+        (setq found (or found match))))))

 (defun widget-radio-validate (widget)
   ;; Valid if we have made a valid choice.
@@ -2731,13 +2768,16 @@ widget-radio-action
   (let ((buttons (widget-get widget :buttons)))
     (when (memq child buttons)
       (dolist (current (widget-get widget :children))
-	(let* ((button (widget-get current :button)))
+	(let* ((button (widget-get current :button))
+               (from (widget-get current :from))
+	       (to (widget-get current :to)))
 	  (cond ((eq child button)
 		 (widget-value-set button t)
-		 (widget-apply current :activate))
+		 (widget-apply current :activate)
+                 (widget-specify-selected current))
 		((widget-value button)
 		 (widget-value-set button nil)
-		 (widget-apply current :deactivate)))))))
+                 (widget-specify-unselected current from to)))))))
   ;; Pass notification to parent.
   (widget-apply widget :notify child event))