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

[Eli Zaretskii]

Ping!

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  69941@debbugs.gnu.org,  Stefan Monnier
>  <monnier@iro.umontreal.ca>
> Date: Mon, 01 Apr 2024 17:20:40 +0200
> 
> On Sun, 24 Mar 2024 19:45:20 +0100 Stephen Berman <stephen.berman@gmx.net> 
> wrote:
> 
> > On Sat, 23 Mar 2024 17:49:53 -0300 Mauro Aranda <maurooaranda@gmail.com> 
> > wrote:
> >
> >> Stephen Berman <stephen.berman@gmx.net> writes:
> >>
> >>> 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.)
> >>
> >> I'm pretty sure it makes sense for user-editable widgets that the
> >> value for insertion-type be t.
> >
> > Yes, if my understanding is correct, it's just radio-button-choice
> > widgets that need (the effect of) insertion type nil (at least for
> > setting the widget-inactive face), see below.
> >
> >>> 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).
> >>
> >> I'm not sure if the right target is the radio-button widget.  It could
> >> be the radio-button-choice widget.  Did you try to conditionalize the code
> >> against the radio-button-choice widget?
> >
> > I didn't, because I got hung up on the radio-button widget, since in
> > Edebug that is what I saw and (mistakenly) took to be the current widget
> > when widget-inactive face is set.  But the resulting marker insertion
> > type discrepancy is really proof that I was looking at the wrong widget
> > type (as I already realized in my comments cited below, but I didn't
> > think to simply try it with radio-button-choice until now, so thanks for
> > pointing me in the right direction!).  And indeed, with
> > radio-button-choice, negating the eq test DTRT, i.e., using (not (eq
> > 'radio-button-choice (widget-type widget))) as the condtion results in
> > the correct fontification.  Since this sexp gives the
> > radio-button-choice widget's :from property the marker insertion type
> > nil, there is no discrepancy between using that sexp and directly using
> > nil, so changing my patch to use that condition would be in improvement.
> > Alternatively, ...
> >
> >>> 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.
> >>
> >> I don't know much about that code in Emacs.  If we find some hack that
> >> works maybe we can use that until someone figures it out.  But again,
> >> given your analysis, I'd like to find out if using the condition on the
> >> radio-button-choice widget works as expected.  And of course, the hack
> >> shouldn't be added to the widget-default-create, which should remain
> >> type agnostic.
> >
> > ... since the issue is fontification with the widget-inactive face,
> > perhaps a better location for the condition is widget-specify-inactive,
> > as in the attached patch.  It's still a hack though, since
> > widget-specify-inactive is also type-agnostic by design.  But if the
> > issue really is confined to radio-button-choice widget's, I guess any
> > solution will have to refer to that type.  However, between adding the
> > condition to widget-specify-inactive or to widget-default-create, I'm
> > not sure which is less hacky: since the patch to widget-default-create
> > effectively undoes the result of setting the marker insertion type to t,
> > perhaps it is cleaner just to set it to nil for radio-button-choice
> > widgets in widget-default-create.  Or maybe someone will come up with a
> > better fix...
> >
> > Steve Berman
> >
> > diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
> > index 172da3db1e0..01319853edc 100644
> > --- a/lisp/wid-edit.el
> > +++ b/lisp/wid-edit.el
> > @@ -532,6 +532,17 @@ widget-inactive
> >  
> >  (defun widget-specify-inactive (widget from to)
> >    "Make WIDGET inactive for user modifications."
> > +  ;; When WIDGET is a radio-button-choice widget and its first child
> > +  ;; radio-button widget is inserted, the marker FROM, which has
> > +  ;; insertion type t, advances to the position after the radio button,
> > +  ;; and since the overlay setting the widget-inactive face begins at
> > +  ;; the position of FROM, this results in the first radio button
> > +  ;; incorrectly not being fontified with the widget-inactive face.  To
> > +  ;; ensure it is correctly fontified, we move FROM backward by 3,
> > +  ;; i.e. the length of the radio-button widget (from its string
> > +  ;; representation "( )" or "(x)") (bug#69941).
> > +  (when (eq (widget-type widget) 'radio-button-choice)
> > +    (set-marker from (- from 3)))
> >    (unless (widget-get widget :inactive)
> >      (let ((overlay (make-overlay from to nil t nil)))
> >        (overlay-put overlay 'face 'widget-inactive)
> 
> To fix this bug, do you have a preference between this patch for
> widget-specify-inactive and the attached patch for
> widget-default-create?  Or do you have a better fix?
> 
> Steve Berman