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

[Mauro Aranda <maurooaranda@gmail.com>]

 > 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

I don't really have a better fix.  I mean, ideally, we'd find the reason
why the setting behaves differently for the radio-button-choice widget,
and only for the first one in a radio widget, as it seems to me. But
I'll need more time to be able to look into that.

That said, if Eli is OK with installing a minor hack (with a FIXME,
please), I don't have problems.  And since it's a hack (and hopefully
temporary), it's better if we keep it at widget-default-create then.