bug#69941: 30.0.50; Faulty fontification of radio button widgets

[Eli Zaretskii]

> Date: Fri, 22 Mar 2024 15:45:42 +0100
> From:  Stephen Berman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> 0. Save the following code (attached here to circumvent line breaks
> added by the mail program) as widget-example.el:
> 
> (custom-set-faces '(widget-inactive ((t (:foreground "magenta"
>                                                    :background "yellow")))))
> 
> (defvar my-radio-widget)
> (defvar my-activate-button)
> 
> (defun my-widget-example ()
>   (interactive)
>   (switch-to-buffer "*My Widget Example*")
>   (kill-all-local-variables)
>   (let ((inhibit-read-only t))
>     (erase-buffer))
>   (remove-overlays)
>   (setq my-radio-widget
>       (widget-create 'radio-button-choice
>                      :notify (lambda (widget &rest _)
>                                (widget-apply widget :deactivate)
>                                (widget-apply my-activate-button :activate))
>                      '(item "One") '(item "Two")))
>   (setq my-activate-button
>       (widget-create 'push-button
>                      :notify (lambda (widget &rest _)
>                                (widget-value-set my-radio-widget "")
>                                (widget-apply my-radio-widget :activate)
>                                (widget-apply widget :deactivate))
>                      "Activate"))
>   (widget-apply my-activate-button :deactivate)
>   (use-local-map widget-keymap)
>   (widget-setup))
> 
> 1. emacs -Q -l widget-example.el
> 2. M-x my-widget-example
> 
> In the buffer "*My Widget Example*" it easy to see (due to value of the
> widget-inactive face set in widget-example.el) that the push-button
> widget "Activate" is inactive and the radio-button widgets labelled
> "One" and "Two" are active (the buttons have the default face; that the
> labels next to the buttons have the widget-inactive face may seem odd,
> but that's not the bug I'm reporting here; I address that issue in a
> separate bug report).
> 
> 3. Press TAB (or S-TAB) twice to put point on the radio button "Two",
> then press RET.  As the fontification shows, now both radio buttons are
> inactive (so pressing RET on either raises the error "Attempt to perform
> action on inactive widget"), and the "Activate" button is now active.
> After tabbing to the "Activate" button and pressing RET, the initial
> state is restored, with the two radio buttons active and "Activate"
> inactive.
> 
> 4. Now tab up to the radio buttone "One" and press RET.
> => While radio button "Two" agains has the widget-inactive face, radio
> button "One" (just the button, not its label) has the default face used
> for active widgets, though it is in fact inactive (as pressing RET and
> getting the corresponding error verifies).
> 
> 5. Tab back to "Activate" and press RET, again restoring the initial
> state.  Now tab to radio button "Two" and press RET.
> => The fontification is the same as in step 4: radio button "Two" has
> the widget-inactive face but radio button "One" has the default (active)
> face, though it is again inactive.  Repeatedly pressing either of the
> radio buttons (after activating them), does not change the fontification
> of "One" again.
> 
> 
> The faulty fontification of radio button "One" also obtains if there is
> just one radio button instead of two, and if there are more than two
> radio buttons, it is only the first one that displays the odd
> fontification (admittedly, I've only test up to three radio buttons).
> 
> I've tried to debug this and found that the problem seems to be due to
> the sexp (set-marker-insertion-type from t) near the end of
> widget-default-create, which advances the marker specified by the
> widget's :from property.  Changing t to nil fixes the faulty
> fontification of the first radio button.
> 
> I investigated the history of this code, and while the value t for the
> marker insertion type was used in the initial commit, it was changed to
> nil in commit e0f956935, with the message "Insert new text at the :from
> marker _after_ the marker, not before it."  But 18 days later it was
> changed back to t in commit 3bff434b8, that also added "Document need to
> put some text before the %v escape in :format string" of editable-field
> widgets.  (I looked at the bug-gnu-emacs and emacs-devel mailing list
> archives but found nothing relevant at the time just prior to these
> commits.)
> 
> So evidently the advancing marker insertion type is needed for at least
> some widgets, though it seems to be problematic for radio buttons.  So I
> tried to conditionalize the choice of t or nil on the type of the
> widget.  I used (not (eq 'radio-button (widget-type widget))), since the
> argument `widget' of widget-default-create is, according to Edebug,
> indeed radio-button, so negating the eq sexp returns nil, which I had
> found to be the value of the marker insertion type that fixes the
> fontification (however, I couldn't think of a way of limiting the
> conditioning to only the first radio button, but in my testing so far
> that lack doesn't appear to make a difference).
> 
> But in fact, using the negation of the value of the eq sexp results in
> the same faulty fontification, while omitting the negation (as in the
> attached patch), which yields the advancing insertion type t, gives the
> correct fontification, just like using nil does.  This makes no sense to
> me, yet it is reliably reproducible.  The only possible explanation that
> occurs to me is that the bug is triggered elsewhere in the Emacs code
> and somehow using the sexp that evaluates to t as the marker insertion
> type affects that code, while using t itself does not (or rather, has
> the opposite effect); but how that could be and where the culpable code
> is, I don't know (as a guess, perhaps in the C code that adds faces, but
> I don't know how to debug that).  If anyone knows or has an idea what's
> going on here, please communicate it.  In the meantime I will continue
> to use the widget library with the patch to see whether it has unwanted
> consequences.
> 
> diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
> index 172da3db1e0..c2cd48e1551 100644
> --- a/lisp/wid-edit.el
> +++ b/lisp/wid-edit.el
> @@ -1733,8 +1733,9 @@ widget-default-create
>         (goto-char value-pos)
>         (widget-apply widget :value-create)))
>     (let ((from (point-min-marker))
> -      (to (point-max-marker)))
> -     (set-marker-insertion-type from t)
> +      (to (point-max-marker))
> +         (from-mit (eq 'radio-button (widget-type widget))))
> +     (set-marker-insertion-type from from-mit)
>       (set-marker-insertion-type to nil)
>       (widget-put widget :from from)
>       (widget-put widget :to to)))
> 

Mauro and Stefan, any comments or suggestions?