GNU bug report logs - #69941
30.0.50; Faulty fontification of radio button widgets

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 69941 in the body.
You can then email your comments to 69941 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; Faulty fontification of radio button widgets
Date: Fri, 22 Mar 2024 15:45:42 +0100

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

0. Save the following code (attached here to circumvent line breaks
added by the mail program) as widget-example.el:

[widget-example.el (application/emacs-lisp, attachment)]

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

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.

[widget-default-create.diff (text/x-patch, attachment)]

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

In GNU Emacs 30.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.18.0) of 2024-03-22 built on strobelfs2
Repository revision: c1530a2e4973005633ebe00d447f1f3aa1200301
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101009
System Description: Linux From Scratch r12.0-112

Configured using:
 'configure -C --with-xwidgets 'CFLAGS=-Og -g3'
 PKG_CONFIG_PATH=/opt/qt5/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER
PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM XWIDGETS GTK3 ZLIB

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>,
 Mauro Aranda <maurooaranda <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Fri, 22 Mar 2024 17:31:46 +0200

> 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 <at> 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?

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 23 Mar 2024 17:49:53 -0300

Stephen Berman <stephen.berman <at> 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.

> 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?

> 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.

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 69941 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sun, 24 Mar 2024 19:45:20 +0100

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

On Sat, 23 Mar 2024 17:49:53 -0300 Mauro Aranda <maurooaranda <at> gmail.com> wrote:

> Stephen Berman <stephen.berman <at> 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

[widget-specify-inactive.diff (text/x-patch, attachment)]

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 69941 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 01 Apr 2024 17:20:40 +0200

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

On Sun, 24 Mar 2024 19:45:20 +0100 Stephen Berman <stephen.berman <at> gmx.net> wrote:

> On Sat, 23 Mar 2024 17:49:53 -0300 Mauro Aranda <maurooaranda <at> gmail.com> wrote:
>
>> Stephen Berman <stephen.berman <at> 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

[Message part 2 (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: maurooaranda <at> gmail.com, Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 06 Apr 2024 13:18:31 +0300

Ping!

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  69941 <at> debbugs.gnu.org,  Stefan Monnier
>  <monnier <at> 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 <at> gmx.net> wrote:
> 
> > On Sat, 23 Mar 2024 17:49:53 -0300 Mauro Aranda <maurooaranda <at> gmail.com> wrote:
> >
> >> Stephen Berman <stephen.berman <at> 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

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 69941 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 18 Apr 2024 07:18:29 -0300

> On Sun, 24 Mar 2024 19:45:20 +0100 Stephen Berman 
<stephen.berman <at> gmx.net> wrote:
>
>> On Sat, 23 Mar 2024 17:49:53 -0300 Mauro Aranda 
<maurooaranda <at> gmail.com> wrote:
>>
>>> Stephen Berman <stephen.berman <at> 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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941 <at> debbugs.gnu.org, stephen.berman <at> gmx.net, monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 18 Apr 2024 14:33:57 +0300

> Date: Thu, 18 Apr 2024 07:18:29 -0300
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 69941 <at> debbugs.gnu.org,
>  Stefan Monnier <monnier <at> iro.umontreal.ca>
> From: Mauro Aranda <maurooaranda <at> gmail.com>
> 
>  > 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.

My opinion doesn't matter much in this case.  If you two agree on a
solution, feel free to install it, even if it is not 110% clean.

Thanks.

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69941 <at> debbugs.gnu.org, Mauro Aranda <maurooaranda <at> gmail.com>,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 18 Apr 2024 15:38:33 +0200

On Thu, 18 Apr 2024 14:33:57 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> Date: Thu, 18 Apr 2024 07:18:29 -0300
>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 69941 <at> debbugs.gnu.org,
>>  Stefan Monnier <monnier <at> iro.umontreal.ca>
>> From: Mauro Aranda <maurooaranda <at> gmail.com>
>> 
>>  > 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.
>
> My opinion doesn't matter much in this case.  If you two agree on a
> solution, feel free to install it, even if it is not 110% clean.

I've been using the patch for for widget-specify-inactive in an
application I'm developing that exercises radio-button-choice widgets,
but I'll switch to using the patch for widget-default-create instead.
I've been encountering inconsistent behavior in combination with the use
of widget-unselected face that I haven't tracked down the cause of yet.
I don't expect using the patch for widget-default-create will improve
this issue, but I'll find out.  I also plan to test that patch in
combination with widget-unselected face with checklist widgets, which my
application currently does not use.  I'll report back here before
committing the patch for widget-default-create (or something else,
depending on the outcome of further testing).

Steve Berman

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69941 <at> debbugs.gnu.org, Mauro Aranda <maurooaranda <at> gmail.com>,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sun, 21 Apr 2024 21:45:59 +0200

On Thu, 18 Apr 2024 15:38:33 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:

> On Thu, 18 Apr 2024 14:33:57 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>
>>> Date: Thu, 18 Apr 2024 07:18:29 -0300
>>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 69941 <at> debbugs.gnu.org,
>>>  Stefan Monnier <monnier <at> iro.umontreal.ca>
>>> From: Mauro Aranda <maurooaranda <at> gmail.com>
>>> 
>>>  > 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.
>>
>> My opinion doesn't matter much in this case.  If you two agree on a
>> solution, feel free to install it, even if it is not 110% clean.
>
> I've been using the patch for for widget-specify-inactive in an
> application I'm developing that exercises radio-button-choice widgets,
> but I'll switch to using the patch for widget-default-create instead.
> I've been encountering inconsistent behavior in combination with the use
> of widget-unselected face that I haven't tracked down the cause of yet.
> I don't expect using the patch for widget-default-create will improve
> this issue, but I'll find out.  I also plan to test that patch in
> combination with widget-unselected face with checklist widgets, which my
> application currently does not use.  I'll report back here before
> committing the patch for widget-default-create (or something else,
> depending on the outcome of further testing).

Just a brief status report: My testing does indeed indicate that the
fontification problem with radio-button-choice also occurs with
checklist widgets, though the pattern appears not to be identical; I
need to do more testing and debugging.

Steve Berman

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69941 <at> debbugs.gnu.org, Mauro Aranda <maurooaranda <at> gmail.com>,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Fri, 26 Apr 2024 14:45:54 +0200

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

On Sun, 21 Apr 2024 21:45:59 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:

> On Thu, 18 Apr 2024 15:38:33 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
>
>> On Thu, 18 Apr 2024 14:33:57 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>
>>>> Date: Thu, 18 Apr 2024 07:18:29 -0300
>>>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 69941 <at> debbugs.gnu.org,
>>>>  Stefan Monnier <monnier <at> iro.umontreal.ca>
>>>> From: Mauro Aranda <maurooaranda <at> gmail.com>
>>>> 
>>>>  > 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.
>>>
>>> My opinion doesn't matter much in this case.  If you two agree on a
>>> solution, feel free to install it, even if it is not 110% clean.
>>
>> I've been using the patch for for widget-specify-inactive in an
>> application I'm developing that exercises radio-button-choice widgets,
>> but I'll switch to using the patch for widget-default-create instead.
>> I've been encountering inconsistent behavior in combination with the use
>> of widget-unselected face that I haven't tracked down the cause of yet.
>> I don't expect using the patch for widget-default-create will improve
>> this issue, but I'll find out.  I also plan to test that patch in
>> combination with widget-unselected face with checklist widgets, which my
>> application currently does not use.  I'll report back here before
>> committing the patch for widget-default-create (or something else,
>> depending on the outcome of further testing).
>
> Just a brief status report: My testing does indeed indicate that the
> fontification problem with radio-button-choice also occurs with
> checklist widgets, though the pattern appears not to be identical; I
> need to do more testing and debugging.

Further testing confirms that checklists are subject to this problem, so
I've added them to the attached patch.  The rest of this post reports
results from and speculations based on my debugging efforts, which
remain somewhat inconclusive.

According to my tests, checklists and radio-button-choice widgets do
indeed display the same problem with the first checkbox or radio-button,
respectively: if it's selected and then the parent widget is
deactivated, then the button/checkbox incorrectly does not have
widget-inactive face.  I think the reason for this is that selecting
inserts "[X]" for the checkbox and "(*)" for the radio-button, and since
the parent widget's :from property has marker insertion type `t', its
position advances to after the insertion (I guess this is because the
starting position of the first checkbox/button coincides with the parent
widget's :from), so the overlay with the widget-inactive face beginning
at :from does not cover the checkbox/button.

But checklists and radio-button-choice widgets differ when a non-initial
checkbox/button is selected.  With checklists, multiple checkboxes can
be selected, and selecting the second checkbox does not advance the
parent widget's :from position, unlike with radio-button-choice widget's
when selecting the second radio-button, as I reported in my OP.  I think
this is because in radio-button-choice widgets only one radio-button can
be selected, so selecting any one triggers the :from marker's advancing.
I could not verify this hypothesis through debugging because I was
unable to find out exactly when this happens.  The marker advance is
done in the C code, I think at adjust_markers_for_insert in insdel.c; I
set a gdb breakpoint there and this triggers when I select a radio
button, but it's too early: a lot happens in wid-edit.el between
selecting a button and the selection becoming visible, and the
breakpoint triggered so often that I gave up.  Is there a way to make a
breakpoint in the C code trigger only when a specific part of
wid-edit.el is evaluated?

Nevertheless, by assigning the :from marker the insertion type nil in
widget-default-create when the widget is either a checklist or
radio-button-choice, does result in the correct fontification of the
first checkbox/radio-button in all tests I've conducted with varying the
selection.  And conceptually it seems to me correct that :from should
not advance with these widgets: selecting a checkbox or button is
operationally quite different from inserting text (e.g. in an
editable-field widget), even though the implementation technically
involves insertion.  So I think the attached patch is at least a viable
stopgap, until a better (or at least less ad hoc) fix is found.

Steve Berman

[Message part 2 (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org, maurooaranda <at> gmail.com, monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 09 May 2024 10:21:44 +0300

Ping!  Any comments?  Or should we install the proposed patch?

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  69941 <at> debbugs.gnu.org,
>   monnier <at> iro.umontreal.ca
> Date: Fri, 26 Apr 2024 14:45:54 +0200
> 
> On Sun, 21 Apr 2024 21:45:59 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
> 
> > On Thu, 18 Apr 2024 15:38:33 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
> >
> >> On Thu, 18 Apr 2024 14:33:57 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
> >>
> >>>> Date: Thu, 18 Apr 2024 07:18:29 -0300
> >>>> Cc: Eli Zaretskii <eliz <at> gnu.org>, 69941 <at> debbugs.gnu.org,
> >>>>  Stefan Monnier <monnier <at> iro.umontreal.ca>
> >>>> From: Mauro Aranda <maurooaranda <at> gmail.com>
> >>>> 
> >>>>  > 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.
> >>>
> >>> My opinion doesn't matter much in this case.  If you two agree on a
> >>> solution, feel free to install it, even if it is not 110% clean.
> >>
> >> I've been using the patch for for widget-specify-inactive in an
> >> application I'm developing that exercises radio-button-choice widgets,
> >> but I'll switch to using the patch for widget-default-create instead.
> >> I've been encountering inconsistent behavior in combination with the use
> >> of widget-unselected face that I haven't tracked down the cause of yet.
> >> I don't expect using the patch for widget-default-create will improve
> >> this issue, but I'll find out.  I also plan to test that patch in
> >> combination with widget-unselected face with checklist widgets, which my
> >> application currently does not use.  I'll report back here before
> >> committing the patch for widget-default-create (or something else,
> >> depending on the outcome of further testing).
> >
> > Just a brief status report: My testing does indeed indicate that the
> > fontification problem with radio-button-choice also occurs with
> > checklist widgets, though the pattern appears not to be identical; I
> > need to do more testing and debugging.
> 
> Further testing confirms that checklists are subject to this problem, so
> I've added them to the attached patch.  The rest of this post reports
> results from and speculations based on my debugging efforts, which
> remain somewhat inconclusive.
> 
> According to my tests, checklists and radio-button-choice widgets do
> indeed display the same problem with the first checkbox or radio-button,
> respectively: if it's selected and then the parent widget is
> deactivated, then the button/checkbox incorrectly does not have
> widget-inactive face.  I think the reason for this is that selecting
> inserts "[X]" for the checkbox and "(*)" for the radio-button, and since
> the parent widget's :from property has marker insertion type `t', its
> position advances to after the insertion (I guess this is because the
> starting position of the first checkbox/button coincides with the parent
> widget's :from), so the overlay with the widget-inactive face beginning
> at :from does not cover the checkbox/button.
> 
> But checklists and radio-button-choice widgets differ when a non-initial
> checkbox/button is selected.  With checklists, multiple checkboxes can
> be selected, and selecting the second checkbox does not advance the
> parent widget's :from position, unlike with radio-button-choice widget's
> when selecting the second radio-button, as I reported in my OP.  I think
> this is because in radio-button-choice widgets only one radio-button can
> be selected, so selecting any one triggers the :from marker's advancing.
> I could not verify this hypothesis through debugging because I was
> unable to find out exactly when this happens.  The marker advance is
> done in the C code, I think at adjust_markers_for_insert in insdel.c; I
> set a gdb breakpoint there and this triggers when I select a radio
> button, but it's too early: a lot happens in wid-edit.el between
> selecting a button and the selection becoming visible, and the
> breakpoint triggered so often that I gave up.  Is there a way to make a
> breakpoint in the C code trigger only when a specific part of
> wid-edit.el is evaluated?
> 
> Nevertheless, by assigning the :from marker the insertion type nil in
> widget-default-create when the widget is either a checklist or
> radio-button-choice, does result in the correct fontification of the
> first checkbox/radio-button in all tests I've conducted with varying the
> selection.  And conceptually it seems to me correct that :from should
> not advance with these widgets: selecting a checkbox or button is
> operationally quite different from inserting text (e.g. in an
> editable-field widget), even though the implementation technically
> involves insertion.  So I think the attached patch is at least a viable
> stopgap, until a better (or at least less ad hoc) fix is found.
> 
> Steve Berman

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 09 May 2024 10:15:29 -0400

> 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 really not familiar with the widget code, but looking around that
code I see that we have

       (set-marker-insertion-type BLAfromBLA t)
       (set-marker-insertion-type BLAtoBLA nil)

at various places, and I think that makes a lot of sense when you
consider that we don't want text inserted right before or right after
the widget to suddenly become part of the widget.
But OTOH while "printing" the widget itself, we'd want the exact
opposite (i.e. nil for from and t for to).

Could it be that part of the problem is that the insertion of
a radio-button widget into a radio-button-choice widget is done "too
late", i.e. after the radio-button-choice widget has been printed?


        Stefan

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 11 May 2024 15:59:19 +0200

On Thu, 09 May 2024 10:15:29 -0400 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> 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 really not familiar with the widget code, but looking around that
> code I see that we have
>
>        (set-marker-insertion-type BLAfromBLA t)
>        (set-marker-insertion-type BLAtoBLA nil)
>
> at various places, and I think that makes a lot of sense when you
> consider that we don't want text inserted right before or right after
> the widget to suddenly become part of the widget.

Yes, Mauro Aranda's response to my OP also made this point, though not
so explicitly, so I think it sort of went by me, but after reading your
comment, I did some further testing and, indeed, my latest patch (I've
only tested that but the other versions I've posted should not differ in
this respect) does result in text inserted directly before a checklist
or radio-button-choice widget being after the widget's :from marker,
which makes the text get covered by the widget-inactive overlay, which
it should not be.

So it seems that either my patch is not the right fix for the
fontification problem, or it has to be accompanied by further
adjustments.  I've briefly looked at the latter approach, thinking that
using insert-before-markers might help, but my attempts have failed so
far.

> But OTOH while "printing" the widget itself, we'd want the exact
> opposite (i.e. nil for from and t for to).
>
> Could it be that part of the problem is that the insertion of
> a radio-button widget into a radio-button-choice widget is done "too
> late", i.e. after the radio-button-choice widget has been printed?

To avoid this I suspect that the radio-button-choice widget would have
to be redesigned, and also the checklist widget, which has a similar
issue.  Although, it seems that a viable alternative to using checklist
widgets may be simply to use checkbox widgets, as e.g. recentf-edit-list
does.  But radio buttons have to be grouped, since only one per group
can be selected.  How to do this without using an enclosing widget like
radio-button-choice, which by definition has it's own :from and :to
properties, I don't know.

Steve Berman

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sun, 12 May 2024 22:22:38 -0400

>> Could it be that part of the problem is that the insertion of
>> a radio-button widget into a radio-button-choice widget is done "too
>> late", i.e. after the radio-button-choice widget has been printed?
> To avoid this I suspect that the radio-button-choice widget would have
> to be redesigned, and also the checklist widget, which has a similar
> issue.

What does "this" refer to?"


        Stefan

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 13 May 2024 15:15:50 +0200

On Sun, 12 May 2024 22:22:38 -0400 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>>> Could it be that part of the problem is that the insertion of
>>> a radio-button widget into a radio-button-choice widget is done "too
>>> late", i.e. after the radio-button-choice widget has been printed?
>> To avoid this I suspect that the radio-button-choice widget would have
>> to be redesigned, and also the checklist widget, which has a similar
>> issue.
>
> What does "this" refer to?"

I meant the "too late" insertion of radio buttons into
radio-button-choice widgets (and likewise of checkboxes into checklist
widgets), because IIUC the way this is currently done (by
widget-radio-add-item and widget-checklist-add-item, respectively)
requires that the container widget is already in the buffer.

Steve Berman

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 13 May 2024 09:26:31 -0400

>>>> Could it be that part of the problem is that the insertion of
>>>> a radio-button widget into a radio-button-choice widget is done "too
>>>> late", i.e. after the radio-button-choice widget has been printed?
>>> To avoid this I suspect that the radio-button-choice widget would have
>>> to be redesigned, and also the checklist widget, which has a similar
>>> issue.
>> What does "this" refer to?"
> I meant the "too late" insertion of radio buttons into
> radio-button-choice widgets (and likewise of checkboxes into checklist
> widgets), because IIUC the way this is currently done (by
> widget-radio-add-item and widget-checklist-add-item, respectively)
> requires that the container widget is already in the buffer.

Then maybe widgets which expect to be filled after they're created
should make sure they have an additional character at the beginning and
another at the end so insertions "inside" don't get confused from
insertions "right before" or "right after".


        Stefan

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 13 May 2024 09:28:54 -0400

>> I meant the "too late" insertion of radio buttons into
>> radio-button-choice widgets (and likewise of checkboxes into checklist
>> widgets), because IIUC the way this is currently done (by
>> widget-radio-add-item and widget-checklist-add-item, respectively)
>> requires that the container widget is already in the buffer.
>
> Then maybe widgets which expect to be filled after they're created
> should make sure they have an additional character at the beginning and
> another at the end so insertions "inside" don't get confused from
> insertions "right before" or "right after".

Or maybe `widget-*-add-item` should temporarily change the insertion
type of the from/to markers?


        Stefan

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 13 May 2024 15:53:54 +0200

On Mon, 13 May 2024 09:28:54 -0400 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>>> I meant the "too late" insertion of radio buttons into
>>> radio-button-choice widgets (and likewise of checkboxes into checklist
>>> widgets), because IIUC the way this is currently done (by
>>> widget-radio-add-item and widget-checklist-add-item, respectively)
>>> requires that the container widget is already in the buffer.
>>
>> Then maybe widgets which expect to be filled after they're created
>> should make sure they have an additional character at the beginning and
>> another at the end so insertions "inside" don't get confused from
>> insertions "right before" or "right after".

Something like with editable-field widgets (cf. commit 3bff434b8:
"Document need to put some text before the %v escape in :format
string")?  Wouldn't that complicate the display these widgets,
e.g. forcing a corresponding offset of all radio buttons or checkboxes
in order to align with the first one?

> Or maybe `widget-*-add-item` should temporarily change the insertion
> type of the from/to markers?

What do you mean by "temporarily"?  Recall the problem that prompted my
OP in this bug is the misfontification of the first radio button in a
deactivated radio-button-choice widget, and the deactivation can happen
any time, long after the widget and its children haven been created.

Maybe it could be left up to the deactivation code to ensure that the
radio-button-choice widget's :from and the :from of its first child
coincide when deactivation (and accompanying fontification) occurs, and
then reset the previous values afterwards.  Though that seems pretty ad
hoc... (and would resetting the :from affect the fontification?)

Steve Berman

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 13 May 2024 10:19:43 -0400

>> Or maybe `widget-*-add-item` should temporarily change the insertion
>> type of the from/to markers?
> What do you mean by "temporarily"?

I think you underestimated the meaning of:

    I'm really not familiar with the widget code

I put "really" before the "not", because I know very little about
that code.

> Maybe it could be left up to the deactivation code to ensure that the
> radio-button-choice widget's :from and the :from of its first child
> coincide when deactivation (and accompanying fontification) occurs, and
> then reset the previous values afterwards.

I suspect thinking in terms of "activation/deactivation" will not
be helpful.  We should look at the code which does the insertion of text
(which presumably happens, among other things, upon deactivation).

> Though that seems pretty ad hoc...

If needed, we might be able to make it less ad-hoc by defining
a function for the purpose of (re)inserting text inside an
existing widget.

> (and would resetting the :from affect the fontification?)

It'd be up to that new function to make sure things work as they should.


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941 <at> debbugs.gnu.org, stephen.berman <at> gmx.net
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 25 May 2024 10:51:08 +0300

Ping!  Any further comments about this, or changes to install?

> Cc: 69941 <at> debbugs.gnu.org
> Date: Mon, 13 May 2024 10:19:43 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> >> Or maybe `widget-*-add-item` should temporarily change the insertion
> >> type of the from/to markers?
> > What do you mean by "temporarily"?
> 
> I think you underestimated the meaning of:
> 
>     I'm really not familiar with the widget code
> 
> I put "really" before the "not", because I know very little about
> that code.
> 
> > Maybe it could be left up to the deactivation code to ensure that the
> > radio-button-choice widget's :from and the :from of its first child
> > coincide when deactivation (and accompanying fontification) occurs, and
> > then reset the previous values afterwards.
> 
> I suspect thinking in terms of "activation/deactivation" will not
> be helpful.  We should look at the code which does the insertion of text
> (which presumably happens, among other things, upon deactivation).
> 
> > Though that seems pretty ad hoc...
> 
> If needed, we might be able to make it less ad-hoc by defining
> a function for the purpose of (re)inserting text inside an
> existing widget.
> 
> > (and would resetting the :from affect the fontification?)
> 
> It'd be up to that new function to make sure things work as they should.
> 
> 
>         Stefan
> 
> 
> 
> 
> 

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69941 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 25 May 2024 11:30:23 +0200

On Sat, 25 May 2024 10:51:08 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:

> Ping!  Any further comments about this, or changes to install?

I haven't been able to pursue Stefan's suggestions yet.  For the use
case for which I made the patch, I find the results overall better than
without it.  Nevertheless, text inserted in front of the first radio
button unintentionally getting fontified with widget-inactive face is a
clear bug, so I think the patch should not be installed; I hope I (or
someone else) can improve it.

Steve Berman

>> Cc: 69941 <at> debbugs.gnu.org
>> Date: Mon, 13 May 2024 10:19:43 -0400
>> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>> >> Or maybe `widget-*-add-item` should temporarily change the insertion
>> >> type of the from/to markers?
>> > What do you mean by "temporarily"?
>>
>> I think you underestimated the meaning of:
>>
>>     I'm really not familiar with the widget code
>>
>> I put "really" before the "not", because I know very little about
>> that code.
>>
>> > Maybe it could be left up to the deactivation code to ensure that the
>> > radio-button-choice widget's :from and the :from of its first child
>> > coincide when deactivation (and accompanying fontification) occurs, and
>> > then reset the previous values afterwards.
>>
>> I suspect thinking in terms of "activation/deactivation" will not
>> be helpful.  We should look at the code which does the insertion of text
>> (which presumably happens, among other things, upon deactivation).
>>
>> > Though that seems pretty ad hoc...
>>
>> If needed, we might be able to make it less ad-hoc by defining
>> a function for the purpose of (re)inserting text inside an
>> existing widget.
>>
>> > (and would resetting the :from affect the fontification?)
>>
>> It'd be up to that new function to make sure things work as they should.
>>
>>
>>         Stefan
>>
>>
>>
>>
>>

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: monnier <at> iro.umontreal.ca, Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 08 Jun 2024 14:50:57 +0300

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,  69941 <at> debbugs.gnu.org
> Date: Sat, 25 May 2024 11:30:23 +0200
> 
> On Sat, 25 May 2024 10:51:08 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> > Ping!  Any further comments about this, or changes to install?
> 
> I haven't been able to pursue Stefan's suggestions yet.  For the use
> case for which I made the patch, I find the results overall better than
> without it.  Nevertheless, text inserted in front of the first radio
> button unintentionally getting fontified with widget-inactive face is a
> clear bug, so I think the patch should not be installed; I hope I (or
> someone else) can improve it.

Stefan, any other comments?  Should I close the bug?

> >> Cc: 69941 <at> debbugs.gnu.org
> >> Date: Mon, 13 May 2024 10:19:43 -0400
> >> From:  Stefan Monnier via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> >>
> >> >> Or maybe `widget-*-add-item` should temporarily change the insertion
> >> >> type of the from/to markers?
> >> > What do you mean by "temporarily"?
> >>
> >> I think you underestimated the meaning of:
> >>
> >>     I'm really not familiar with the widget code
> >>
> >> I put "really" before the "not", because I know very little about
> >> that code.
> >>
> >> > Maybe it could be left up to the deactivation code to ensure that the
> >> > radio-button-choice widget's :from and the :from of its first child
> >> > coincide when deactivation (and accompanying fontification) occurs, and
> >> > then reset the previous values afterwards.
> >>
> >> I suspect thinking in terms of "activation/deactivation" will not
> >> be helpful.  We should look at the code which does the insertion of text
> >> (which presumably happens, among other things, upon deactivation).
> >>
> >> > Though that seems pretty ad hoc...
> >>
> >> If needed, we might be able to make it less ad-hoc by defining
> >> a function for the purpose of (re)inserting text inside an
> >> existing widget.
> >>
> >> > (and would resetting the :from affect the fontification?)
> >>
> >> It'd be up to that new function to make sure things work as they should.
> >>
> >>
> >>         Stefan
> >>
> >>
> >>
> >>
> >>
> 

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69941 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 08 Jun 2024 14:12:42 +0200

On Sat, 08 Jun 2024 14:50:57 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,  69941 <at> debbugs.gnu.org
>> Date: Sat, 25 May 2024 11:30:23 +0200
>>
>> On Sat, 25 May 2024 10:51:08 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>
>> > Ping!  Any further comments about this, or changes to install?
>>
>> I haven't been able to pursue Stefan's suggestions yet.  For the use
>> case for which I made the patch, I find the results overall better than
>> without it.  Nevertheless, text inserted in front of the first radio
>> button unintentionally getting fontified with widget-inactive face is a
>> clear bug, so I think the patch should not be installed; I hope I (or
>> someone else) can improve it.
>
> Stefan, any other comments?  Should I close the bug?

While my patch prevents the misfontification I reported in my OP, it
adds another source of misfontification and is therefore not suitable
for installing.  So the original bug remains and I think this bug report
should not yet be closed.

Steve Berman

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 08 Jun 2024 16:32:06 +0300

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Cc: monnier <at> iro.umontreal.ca,  69941 <at> debbugs.gnu.org
> Date: Sat, 08 Jun 2024 14:12:42 +0200
> 
> On Sat, 08 Jun 2024 14:50:57 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
> >> From: Stephen Berman <stephen.berman <at> gmx.net>
> >> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,  69941 <at> debbugs.gnu.org
> >> Date: Sat, 25 May 2024 11:30:23 +0200
> >>
> >> On Sat, 25 May 2024 10:51:08 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
> >>
> >> > Ping!  Any further comments about this, or changes to install?
> >>
> >> I haven't been able to pursue Stefan's suggestions yet.  For the use
> >> case for which I made the patch, I find the results overall better than
> >> without it.  Nevertheless, text inserted in front of the first radio
> >> button unintentionally getting fontified with widget-inactive face is a
> >> clear bug, so I think the patch should not be installed; I hope I (or
> >> someone else) can improve it.
> >
> > Stefan, any other comments?  Should I close the bug?
> 
> While my patch prevents the misfontification I reported in my OP, it
> adds another source of misfontification and is therefore not suitable
> for installing.  So the original bug remains and I think this bug report
> should not yet be closed.

OK, but what shall we do instead?  Is anyone working on this?

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69941 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 08 Jun 2024 16:14:20 +0200

On Sat, 08 Jun 2024 16:32:06 +0300 nobody wrote:

>> From: Stephen Berman <stephen.berman <at> gmx.net>
>> Cc: monnier <at> iro.umontreal.ca,  69941 <at> debbugs.gnu.org
>> Date: Sat, 08 Jun 2024 14:12:42 +0200
>>
>> On Sat, 08 Jun 2024 14:50:57 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>
>> >> From: Stephen Berman <stephen.berman <at> gmx.net>
>> >> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>,  69941 <at> debbugs.gnu.org
>> >> Date: Sat, 25 May 2024 11:30:23 +0200
>> >>
>> >> On Sat, 25 May 2024 10:51:08 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>> >>
>> >> > Ping!  Any further comments about this, or changes to install?
>> >>
>> >> I haven't been able to pursue Stefan's suggestions yet.  For the use
>> >> case for which I made the patch, I find the results overall better than
>> >> without it.  Nevertheless, text inserted in front of the first radio
>> >> button unintentionally getting fontified with widget-inactive face is a
>> >> clear bug, so I think the patch should not be installed; I hope I (or
>> >> someone else) can improve it.
>> >
>> > Stefan, any other comments?  Should I close the bug?
>>
>> While my patch prevents the misfontification I reported in my OP, it
>> adds another source of misfontification and is therefore not suitable
>> for installing.  So the original bug remains and I think this bug report
>> should not yet be closed.
>
> OK, but what shall we do instead?  Is anyone working on this?

I'm really unclear how to try and implement what Stefan suggested.
Perhaps Mauro or someone else more familiar with the widget code might
be able to do it or give me some guidance.

Steve Berman

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: 69941 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 9 Jan 2025 09:53:58 -0300

Stephen Berman <stephen.berman <at> gmx.net> writes:

> On Sat, 08 Jun 2024 16:32:06 +0300 nobody wrote:
>
>>> From: Stephen Berman <stephen.berman <at> gmx.net>
>>> Cc: monnier <at> iro.umontreal.ca,  69941 <at> debbugs.gnu.org
>>> Date: Sat, 08 Jun 2024 14:12:42 +0200
>>>
>>> On Sat, 08 Jun 2024 14:50:57 +0300 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>>
>>> >> From: Stephen Berman <stephen.berman <at> gmx.net>
>>> >> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 
69941 <at> debbugs.gnu.org
>>> >> Date: Sat, 25 May 2024 11:30:23 +0200
>>> >>
>>> >> On Sat, 25 May 2024 10:51:08 +0300 Eli Zaretskii <eliz <at> gnu.org> 
wrote:
>>> >>
>>> >> > Ping!  Any further comments about this, or changes to install?
>>> >>
>>> >> I haven't been able to pursue Stefan's suggestions yet.  For the use
>>> >> case for which I made the patch, I find the results overall 
better than
>>> >> without it.  Nevertheless, text inserted in front of the first radio
>>> >> button unintentionally getting fontified with widget-inactive 
face is a
>>> >> clear bug, so I think the patch should not be installed; I hope 
I (or
>>> >> someone else) can improve it.
>>> >
>>> > Stefan, any other comments?  Should I close the bug?
>>>
>>> While my patch prevents the misfontification I reported in my OP, it
>>> adds another source of misfontification and is therefore not suitable
>>> for installing.  So the original bug remains and I think this bug 
report
>>> should not yet be closed.
>>
>> OK, but what shall we do instead?  Is anyone working on this?
>
> I'm really unclear how to try and implement what Stefan suggested.
> Perhaps Mauro or someone else more familiar with the widget code might
> be able to do it or give me some guidance.
>
> Steve Berman

I'll work on this bug.

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stephen Berman <stephen.berman <at> gmx.net>, 69941 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, monnier <at> iro.umontreal.ca
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 13 Jan 2025 11:35:54 -0300

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

It turns out the problem is general to all composite widgets.  Luckily,
it seems that it only surfaces in corner cases, and mostly as a
fontification problem.

Stefan detected the problem: widgets whose children get recreated (but
they themselves don't) might end up with their markers not containing
the entire child anymore.  But since it might happen with any composite
widget, we can't just fix this for radio or checklist widgets.

I attach a patch with a first idea (based on Stefan's suggestions),
where we "protect" the parent's markers, inside widget-default-create.
This is so we can try to catch almost all situations where this bug
might arise.  It doesn't catch _all_, in theory, since it might happen
that a grandparent widget doesn't adjust its markers, but maybe that's
unusual enough that we can get away with it.

The patch includes a test, that hopefully shows how easy is to make this
bug appear.

[0001-Protect-parent-markers-when-recreating-a-child-widge.patch (text/x-patch, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 16 Jan 2025 23:28:02 -0500

> I attach a patch with a first idea (based on Stefan's suggestions),
> where we "protect" the parent's markers, inside widget-default-create.
> This is so we can try to catch almost all situations where this bug
> might arise.

I can't judge whether `widget-default-create` covers all the cases, but
other than that, it's looking good (well, I find this "set and later reset"
to be rather ugly/brittle, but that's probably as good as it gets given
what we have).

> +(defun widget--protect-parent-markers (widget)
> +  "Protect the :from and :to markers of the WIDGET's parent, if necessary.
> +
> +Usually, the :from marker has type t, while the :to marker has type nil.
> +When creating a child or a button inside a composite widget, they have to be
> +changed to nil and t respectively, so that the WIDGET's parent (if any),
> +properly contains all of its children and buttons."
> +  (let* ((parent (widget-get widget :parent))
> +         (parent-from-marker (and parent (widget-get parent :from)))
> +         (parent-to-marker (and parent (widget-get parent :to)))
> +         (pos (point)))
> +    (when (and parent-from-marker
> +               (eq pos (marker-position parent-from-marker)))
> +      (set-marker-insertion-type parent-from-marker nil))
> +    (when (and parent-to-marker
> +               (eq pos (marker-position parent-to-marker)))
> +      (set-marker-insertion-type parent-to-marker t))))
> +
> +(defun widget--unprotect-parent-markers (widget)
> +  "Unprotect the :from and :to markers of the WIDGET's parent, if necessary.
> +
> +Changes the :from marker type back to t and the :to marker type back to nil."
> +  (let* ((parent (widget-get widget :parent))
> +         (parent-from-marker (and parent (widget-get parent :from)))
> +         (parent-to-marker (and parent (widget-get parent :to))))
> +    (when parent-from-marker
> +      (set-marker-insertion-type parent-from-marker t))
> +    (when parent-to-marker
> +      (set-marker-insertion-type parent-to-marker nil))))

I'm not a big fan of the name "protect" here, to be honest.
Maybe `widget--prepare-parent-for-insertion`?

> It doesn't catch _all_, in theory, since it might happen
> that a grandparent widget doesn't adjust its markers, but maybe that's
> unusual enough that we can get away with it.

We can probably handle that case easily by just adding to
`widget--protect-parent-markers` a recursive call if either the :from or
the :to marker had to be adjusted.  The only difficulty I see is making
sure we adjust all the makers in the "unprotect" case that we
protected earlier.  Maybe we should make
`widget--protect-parent-markers` return a list of (MARKER
. PREVIOUS-TYPE) so the `widget--unprotect-parent-markers`
would turn into something like (mapcar (apply-partially #'apply
#'set-marker-insertion-type) ...).
IIUC that list would be empty in the vast majority of cases.


        Stefan

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Fri, 17 Jan 2025 08:52:29 -0300

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

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> I attach a patch with a first idea (based on Stefan's suggestions),
>> where we "protect" the parent's markers, inside widget-default-create.
>> This is so we can try to catch almost all situations where this bug
>> might arise.
>
> I can't judge whether `widget-default-create` covers all the cases, but
> other than that, it's looking good (well, I find this "set and later 
reset"
> to be rather ugly/brittle, but that's probably as good as it gets given
> what we have).

Thanks for taking a look.

>> +(defun widget--protect-parent-markers (widget)
>> +  "Protect the :from and :to markers of the WIDGET's parent, if 
necessary.
>> +
>> +Usually, the :from marker has type t, while the :to marker has type 
nil.
>> +When creating a child or a button inside a composite widget, they 
have to be
>> +changed to nil and t respectively, so that the WIDGET's parent (if 
any),
>> +properly contains all of its children and buttons."
>> +  (let* ((parent (widget-get widget :parent))
>> +         (parent-from-marker (and parent (widget-get parent :from)))
>> +         (parent-to-marker (and parent (widget-get parent :to)))
>> +         (pos (point)))
>> +    (when (and parent-from-marker
>> +               (eq pos (marker-position parent-from-marker)))
>> +      (set-marker-insertion-type parent-from-marker nil))
>> +    (when (and parent-to-marker
>> +               (eq pos (marker-position parent-to-marker)))
>> +      (set-marker-insertion-type parent-to-marker t))))
>> +
>> +(defun widget--unprotect-parent-markers (widget)
>> +  "Unprotect the :from and :to markers of the WIDGET's parent, if 
necessary.
>> +
>> +Changes the :from marker type back to t and the :to marker type 
back to nil."
>> +  (let* ((parent (widget-get widget :parent))
>> +         (parent-from-marker (and parent (widget-get parent :from)))
>> +         (parent-to-marker (and parent (widget-get parent :to))))
>> +    (when parent-from-marker
>> +      (set-marker-insertion-type parent-from-marker t))
>> +    (when parent-to-marker
>> +      (set-marker-insertion-type parent-to-marker nil))))
>
> I'm not a big fan of the name "protect" here, to be honest.
> Maybe `widget--prepare-parent-for-insertion`?

Sounds a little bit better, thanks.  I went for something similar.

>> It doesn't catch _all_, in theory, since it might happen
>> that a grandparent widget doesn't adjust its markers, but maybe that's
>> unusual enough that we can get away with it.
>
> We can probably handle that case easily by just adding to
> `widget--protect-parent-markers` a recursive call if either the :from or
> the :to marker had to be adjusted.  The only difficulty I see is making
> sure we adjust all the makers in the "unprotect" case that we
> protected earlier.  Maybe we should make
> `widget--protect-parent-markers` return a list of (MARKER
> . PREVIOUS-TYPE) so the `widget--unprotect-parent-markers`
> would turn into something like (mapcar (apply-partially #'apply
> #'set-marker-insertion-type) ...).
> IIUC that list would be empty in the vast majority of cases.

Thanks for this idea.

I attach a patch that implements something like that, along with a new
test to check the recursive case.

[0001-Prepare-markers-for-insertions-inside-of-a-widget.patch (text/x-patch, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Fri, 17 Jan 2025 09:12:46 -0500

> I attach a patch that implements something like that, along with a new
> test to check the recursive case.

Great.  A few more comments:

> +  (let* ((parent (widget-get widget :parent))
> +         (parent-from-marker (and parent (widget-get parent :from)))
> +         (parent-to-marker (and parent (widget-get parent :to)))
> +         (continue nil)
> +         (lst nil)
> +         (pos (point)))
> +    (when (and parent-from-marker
> +               (eq pos (marker-position parent-from-marker)))
> +      (set-marker-insertion-type parent-from-marker nil)
> +      (push (cons parent-from-marker t) lst)
> +      (setq continue t))
> +    (when (and parent-to-marker
> +               (eq pos (marker-position parent-to-marker)))
> +      (set-marker-insertion-type parent-to-marker t)
> +      (push (cons parent-to-marker nil) lst)
> +      (setq continue t))
> +    (when continue
> +      (setq lst
> +            (append lst (widget--prepare-markers-for-inside-insertion parent))))
> +    lst))

It just occurred to me that if widget updates happen recursively
(meaning that another widget update happens between the two calls to
`widget--prepare-markers-for-*-insertion`), then the
`widget--prepare-markers-for-outside-insertion` done at the end
of the inner call will mean that any insertion that happens afterwards
can end up inserting "outside" instead of "inside".  So, I think to
protect against that, we need the above code to push only those markers
which we do change (i.e. we need to test the current value of
`marker-insertion-type` before pushing it to `lst`).
Does that make sense, or am I imagining things?


        Stefan


PS: While I'm here, I might as well mention the irrelevant tweaks
  I'd be tempted to do:

- Maybe instead of "prepare" I'd use "revert" in the name of the
  second function.
- `continue` is non-nil exactly when `lst` is non-nil, so we can drop
  `continue`.
- Since `lst` is a freshly consructed list, we can use `nconc`.
- The `setq` at the end is right before returning `lst`
  so we don't need it, instead we can finish with:

    (when lst
      (nconc lst (widget--prepare-markers-for-inside-insertion parent)))))

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Fri, 17 Jan 2025 17:13:07 -0300

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

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> I attach a patch that implements something like that, along with a new
>> test to check the recursive case.
>
> Great.  A few more comments:
>
>> +  (let* ((parent (widget-get widget :parent))
>> +         (parent-from-marker (and parent (widget-get parent :from)))
>> +         (parent-to-marker (and parent (widget-get parent :to)))
>> +         (continue nil)
>> +         (lst nil)
>> +         (pos (point)))
>> +    (when (and parent-from-marker
>> +               (eq pos (marker-position parent-from-marker)))
>> +      (set-marker-insertion-type parent-from-marker nil)
>> +      (push (cons parent-from-marker t) lst)
>> +      (setq continue t))
>> +    (when (and parent-to-marker
>> +               (eq pos (marker-position parent-to-marker)))
>> +      (set-marker-insertion-type parent-to-marker t)
>> +      (push (cons parent-to-marker nil) lst)
>> +      (setq continue t))
>> +    (when continue
>> +      (setq lst
>> + (append lst (widget--prepare-markers-for-inside-insertion
>> parent))))
>> +    lst))
>
> It just occurred to me that if widget updates happen recursively
> (meaning that another widget update happens between the two calls to
> `widget--prepare-markers-for-*-insertion`), then the
> `widget--prepare-markers-for-outside-insertion` done at the end
> of the inner call will mean that any insertion that happens afterwards
> can end up inserting "outside" instead of "inside".  So, I think to
> protect against that, we need the above code to push only those markers
> which we do change (i.e. we need to test the current value of
> `marker-insertion-type` before pushing it to `lst`).
> Does that make sense, or am I imagining things?
>

I think I get what you mean.  It makes sense, although I couldn't figure
out a precise scenario where this might actually happen.  I'll try to
explain:

Recursive updates happen anytime we are creating a composite
widget, and I think the situation you describe could potentially happen
when we are recreating a composite widget that's inside another
composite widget and that should be common enough to worry about it.

So we have: composite-widget > composite-widget2 > children.

If we're recreating the 'composite-widget2', then it will have markers
pointing nowhere (because of the deletion that happens first in
`widget-default-value-set`).  So the code will prepare the markers of
'composite-widget', but when `widget-default-create` gets called for
creating 'children', `widget--prepare-markers-for-inside-insertion` will
return an empty list, effectively avoiding the scenario you though
about, IIUC.

Nevertheless, since my imagination can only go so far, and because it's
quite easy to add those checks, I implemented that in the attached
patch.

> PS: While I'm here, I might as well mention the irrelevant tweaks
>   I'd be tempted to do:
>
> - Maybe instead of "prepare" I'd use "revert" in the name of the
>   second function.
> - `continue` is non-nil exactly when `lst` is non-nil, so we can drop
>   `continue`.
> - Since `lst` is a freshly consructed list, we can use `nconc`.
> - The `setq` at the end is right before returning `lst`
>   so we don't need it, instead we can finish with:
>
>     (when lst
>       (nconc lst (widget--prepare-markers-for-inside-insertion 
parent)))))

These are most welcome, thanks.  New patch attached.

[0001-Prepare-markers-for-insertions-inside-of-a-widget.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: monnier <at> iro.umontreal.ca, Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941 <at> debbugs.gnu.org, stephen.berman <at> gmx.net
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 01 Feb 2025 13:58:12 +0200

> Date: Fri, 17 Jan 2025 17:13:07 -0300
> Cc: Stephen Berman <stephen.berman <at> gmx.net>, 69941 <at> debbugs.gnu.org,
>  Eli Zaretskii <eliz <at> gnu.org>
> From: Mauro Aranda <maurooaranda <at> gmail.com>
> 
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> 
>  >> I attach a patch that implements something like that, along with a new
>  >> test to check the recursive case.
>  >
>  > Great.  A few more comments:
>  >
>  >> +  (let* ((parent (widget-get widget :parent))
>  >> +         (parent-from-marker (and parent (widget-get parent :from)))
>  >> +         (parent-to-marker (and parent (widget-get parent :to)))
>  >> +         (continue nil)
>  >> +         (lst nil)
>  >> +         (pos (point)))
>  >> +    (when (and parent-from-marker
>  >> +               (eq pos (marker-position parent-from-marker)))
>  >> +      (set-marker-insertion-type parent-from-marker nil)
>  >> +      (push (cons parent-from-marker t) lst)
>  >> +      (setq continue t))
>  >> +    (when (and parent-to-marker
>  >> +               (eq pos (marker-position parent-to-marker)))
>  >> +      (set-marker-insertion-type parent-to-marker t)
>  >> +      (push (cons parent-to-marker nil) lst)
>  >> +      (setq continue t))
>  >> +    (when continue
>  >> +      (setq lst
>  >> + (append lst (widget--prepare-markers-for-inside-insertion
>  >> parent))))
>  >> +    lst))
>  >
>  > It just occurred to me that if widget updates happen recursively
>  > (meaning that another widget update happens between the two calls to
>  > `widget--prepare-markers-for-*-insertion`), then the
>  > `widget--prepare-markers-for-outside-insertion` done at the end
>  > of the inner call will mean that any insertion that happens afterwards
>  > can end up inserting "outside" instead of "inside".  So, I think to
>  > protect against that, we need the above code to push only those markers
>  > which we do change (i.e. we need to test the current value of
>  > `marker-insertion-type` before pushing it to `lst`).
>  > Does that make sense, or am I imagining things?
>  >
> 
> I think I get what you mean.  It makes sense, although I couldn't figure
> out a precise scenario where this might actually happen.  I'll try to
> explain:
> 
> Recursive updates happen anytime we are creating a composite
> widget, and I think the situation you describe could potentially happen
> when we are recreating a composite widget that's inside another
> composite widget and that should be common enough to worry about it.
> 
> So we have: composite-widget > composite-widget2 > children.
> 
> If we're recreating the 'composite-widget2', then it will have markers
> pointing nowhere (because of the deletion that happens first in
> `widget-default-value-set`).  So the code will prepare the markers of
> 'composite-widget', but when `widget-default-create` gets called for
> creating 'children', `widget--prepare-markers-for-inside-insertion` will
> return an empty list, effectively avoiding the scenario you though
> about, IIUC.
> 
> Nevertheless, since my imagination can only go so far, and because it's
> quite easy to add those checks, I implemented that in the attached
> patch.
> 
>  > PS: While I'm here, I might as well mention the irrelevant tweaks
>  >   I'd be tempted to do:
>  >
>  > - Maybe instead of "prepare" I'd use "revert" in the name of the
>  >   second function.
>  > - `continue` is non-nil exactly when `lst` is non-nil, so we can drop
>  >   `continue`.
>  > - Since `lst` is a freshly consructed list, we can use `nconc`.
>  > - The `setq` at the end is right before returning `lst`
>  >   so we don't need it, instead we can finish with:
>  >
>  >     (when lst
>  >       (nconc lst (widget--prepare-markers-for-inside-insertion 
> parent)))))
> 
> These are most welcome, thanks.  New patch attached.

Stefan, any further comments, or should we install this?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69941 <at> debbugs.gnu.org, stephen.berman <at> gmx.net,
 Mauro Aranda <maurooaranda <at> gmail.com>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sun, 02 Feb 2025 18:28:39 -0500

> Stefan, any further comments, or should we install this?

We can install this, AFAIC.


        Stefan

Message #112 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941-done <at> debbugs.gnu.org, stephen.berman <at> gmx.net, maurooaranda <at> gmail.com
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Wed, 05 Feb 2025 15:10:16 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  stephen.berman <at> gmx.net,
>   69941 <at> debbugs.gnu.org
> Date: Sun, 02 Feb 2025 18:28:39 -0500
> 
> > Stefan, any further comments, or should we install this?
> 
> We can install this, AFAIC.

Thanks, installed on the master branch, and closing the bug.

Message #115 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69941-done <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 maurooaranda <at> gmail.com
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 06 Feb 2025 15:15:29 +0100

On Wed, 05 Feb 2025 15:10:16 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  stephen.berman <at> gmx.net,
>>   69941 <at> debbugs.gnu.org
>> Date: Sun, 02 Feb 2025 18:28:39 -0500
>>
>> > Stefan, any further comments, or should we install this?
>>
>> We can install this, AFAIC.
>
> Thanks, installed on the master branch, and closing the bug.

I've only now found time to try Mauro's patch, and while I can confirm
that it fixes the example of misfontification I reported in my OP,
unfortunately I still see the same kind of misfontification with the
program of mine that prompted my OP.  I haven't had time to investigate
further and I'm not sure when I will, but if and when I can provide more
information, I'll either reopen this bug or file a new one.

Steve Berman

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

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 6 Feb 2025 11:19:53 -0300

Stephen Berman <stephen.berman <at> gmx.net> writes:

> On Wed, 05 Feb 2025 15:10:16 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:
>
>>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>>> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  stephen.berman <at> gmx.net,
>>>   69941 <at> debbugs.gnu.org
>>> Date: Sun, 02 Feb 2025 18:28:39 -0500
>>>
>>> > Stefan, any further comments, or should we install this?
>>>
>>> We can install this, AFAIC.
>>
>> Thanks, installed on the master branch, and closing the bug.
>
> I've only now found time to try Mauro's patch, and while I can confirm
> that it fixes the example of misfontification I reported in my OP,
> unfortunately I still see the same kind of misfontification with the
> program of mine that prompted my OP.  I haven't had time to investigate
> further and I'm not sure when I will, but if and when I can provide more
> information, I'll either reopen this bug or file a new one.
>
> Steve Berman

Can you share some code? Either here or by private email?

Message #124 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 06 Feb 2025 16:29:14 +0100

On Thu, 6 Feb 2025 11:19:53 -0300 Mauro Aranda <maurooaranda <at> gmail.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>
>> On Wed, 05 Feb 2025 15:10:16 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>
>>>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>>>> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  stephen.berman <at> gmx.net,
>>>>   69941 <at> debbugs.gnu.org
>>>> Date: Sun, 02 Feb 2025 18:28:39 -0500
>>>>
>>>> > Stefan, any further comments, or should we install this?
>>>>
>>>> We can install this, AFAIC.
>>>
>>> Thanks, installed on the master branch, and closing the bug.
>>
>> I've only now found time to try Mauro's patch, and while I can confirm
>> that it fixes the example of misfontification I reported in my OP,
>> unfortunately I still see the same kind of misfontification with the
>> program of mine that prompted my OP.  I haven't had time to investigate
>> further and I'm not sure when I will, but if and when I can provide more
>> information, I'll either reopen this bug or file a new one.
>>
>> Steve Berman
>
> Can you share some code? Either here or by private email?

Rather than posting or sending you the whole program as well as a data
file needed to run it and explaining how to use it, I'd prefer to try
and isolate the problem and provide a small and self-contained code
snippet to reproduce it.  I'll try to do that in the next few days.

Steve Berman

Message #127 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Thu, 6 Feb 2025 19:54:02 -0300

Stephen Berman <stephen.berman <at> gmx.net> writes:

> On Thu, 6 Feb 2025 11:19:53 -0300 Mauro Aranda 
<maurooaranda <at> gmail.com> wrote:
>
>> Stephen Berman <stephen.berman <at> gmx.net> writes:
>>
>>> On Wed, 05 Feb 2025 15:10:16 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>>
>>>>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>>>>> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  stephen.berman <at> gmx.net,
>>>>>   69941 <at> debbugs.gnu.org
>>>>> Date: Sun, 02 Feb 2025 18:28:39 -0500
>>>>>
>>>>> > Stefan, any further comments, or should we install this?
>>>>>
>>>>> We can install this, AFAIC.
>>>>
>>>> Thanks, installed on the master branch, and closing the bug.
>>>
>>> I've only now found time to try Mauro's patch, and while I can confirm
>>> that it fixes the example of misfontification I reported in my OP,
>>> unfortunately I still see the same kind of misfontification with the
>>> program of mine that prompted my OP.  I haven't had time to investigate
>>> further and I'm not sure when I will, but if and when I can provide 
more
>>> information, I'll either reopen this bug or file a new one.
>>>
>>> Steve Berman
>>
>> Can you share some code? Either here or by private email?
>
> Rather than posting or sending you the whole program as well as a data
> file needed to run it and explaining how to use it, I'd prefer to try
> and isolate the problem and provide a small and self-contained code
> snippet to reproduce it.  I'll try to do that in the next few days.
>
> Steve Berman

Ok then, I'll wait.

Message #130 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 08 Feb 2025 14:20:48 +0100

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

On Thu, 6 Feb 2025 19:54:02 -0300 Mauro Aranda <maurooaranda <at> gmail.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>
>> On Thu, 6 Feb 2025 11:19:53 -0300 Mauro Aranda <maurooaranda <at> gmail.com>
>  wrote:
>>
>>> Stephen Berman <stephen.berman <at> gmx.net> writes:
>>>
>>>> On Wed, 05 Feb 2025 15:10:16 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:
>>>>
>>>>>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>>>>>> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  stephen.berman <at> gmx.net,
>>>>>>   69941 <at> debbugs.gnu.org
>>>>>> Date: Sun, 02 Feb 2025 18:28:39 -0500
>>>>>>
>>>>>> > Stefan, any further comments, or should we install this?
>>>>>>
>>>>>> We can install this, AFAIC.
>>>>>
>>>>> Thanks, installed on the master branch, and closing the bug.
>>>>
>>>> I've only now found time to try Mauro's patch, and while I can confirm
>>>> that it fixes the example of misfontification I reported in my OP,
>>>> unfortunately I still see the same kind of misfontification with the
>>>> program of mine that prompted my OP.  I haven't had time to investigate
>>>> further and I'm not sure when I will, but if and when I can provide more
>>>> information, I'll either reopen this bug or file a new one.
>>>>
>>>> Steve Berman
>>>
>>> Can you share some code? Either here or by private email?
>>
>> Rather than posting or sending you the whole program as well as a data
>> file needed to run it and explaining how to use it, I'd prefer to try
>> and isolate the problem and provide a small and self-contained code
>> snippet to reproduce it.  I'll try to do that in the next few days.
>>
>> Steve Berman
>
> Ok then, I'll wait.

With the attached file I can reproduce the problem.  Save the file and
then invoke Emacs from master (i.e., with your patch) as follows:

emacs -Q -l srb-widget-test.el -f srb-widget-test

In the displayed buffer "*Widget Test*" the three radio buttons "One",
"Two" and "Confirm" are active, which the default face visually
confirms.  (Their labels are fontified with face `widget-unselected'.)
Now do the following steps:

1. Hit TAB to move point to radio button "One".
2. Type RET to activate the "Reset" button and move point to it.  Also,
   the radio buttons are now deactivated, which is indicated by their
   now being fontified with face `widget-inactive' (as are their
   labels).
3. Type RET on the "Reset" button to reactivate the radio buttons.
4. Hit TAB twice to move point to radio button "Two".  Hitting RET
   results in the same state as in step 2.
5. Repeat step 3 and then hit TAB three times (or S-TAB once) to move
   point to radio button "Confirm".  This pops up two indented radio
   buttons "Right" and "Wrong" under "Confirm", and point is now on
   "Right".  Note that the radio buttons "One", "Two" and "Confirm" are
   deactivated, while "Right" and "Wrong" are active.
6. Type RET on radio button "Right".  This deletes radio buttons "Right"
   and "Wrong" and moves point to "Reset".  Radio buttons "One", "Two"
   and "Confirm" remain deactivated (confirmed by tabbing to each and
   typing RET, which dings and displays the message "Attempt to perform
   action on inactive widget"); however:
=> Now radio button "One" is misfontified with the default face instead
   of face `widget-inactive'.

If you repeat step 5 and then tab to radio button "Wrong" and hit RET,
the resulting state is the same as in step 6, i.e., radio button "One"
(and only it) is again misfontified.

Note that if you load the file and execute the recipe in emacs-30, the
misfontification of radio button "One" also happens on steps 2, 4 and 5.
So the correct fontification on these steps in master is due to your
patch.  But the patch doesn't prevent the misfontification in step 6.
However, I have found two ways to avoid this misfontification: One way
is to uncomment the line `(widget-apply srb--radio-widget :activate)' in
`srb--confirm', the other way is to comment out the line
`(widget-value-set srb--radio-widget "")' in `srb--result'.

The second "workaround" (i.e., not using an emtpy value for the radio
button widget) may be regarded as the "proper" way to use radio buttons,
since it visually distinguishes the selected button from the unselected
ones.  But the empty string is a valid value, and there may be aesthetic
or other reasons for using it (one in this case is that the code to move
point to the "Reset" button doesn't work as intended on hitting the
child radio buttons, though that is probably not hard to fix).

I briefly tried to find the cause of the misfontification by stepping
through `widget-radio-value-set', but this seems to require a more
detailed examination of the widget properties than I currently have time
for, or perhaps just a better understanding of the widget code than I
have.

Steve Berman

[srb-widget-test.el (application/emacs-lisp, attachment)]

Message #133 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 8 Feb 2025 14:06:21 -0300

Stephen Berman <stephen.berman <at> gmx.net> writes:

> On Thu, 6 Feb 2025 19:54:02 -0300 Mauro Aranda 
<maurooaranda <at> gmail.com> wrote:
>
>> Stephen Berman <stephen.berman <at> gmx.net> writes:
>>
>>> On Thu, 6 Feb 2025 11:19:53 -0300 Mauro Aranda <maurooaranda <at> gmail.com>
>>  wrote:
>>>
>>>> Stephen Berman <stephen.berman <at> gmx.net> writes:
>>>>
>>>>> On Wed, 05 Feb 2025 15:10:16 +0200 Eli Zaretskii <eliz <at> gnu.org> 
wrote:
>>>>>
>>>>>>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>>>>>>> Cc: Mauro Aranda <maurooaranda <at> gmail.com>,  stephen.berman <at> gmx.net,
>>>>>>>   69941 <at> debbugs.gnu.org
>>>>>>> Date: Sun, 02 Feb 2025 18:28:39 -0500
>>>>>>>
>>>>>>> > Stefan, any further comments, or should we install this?
>>>>>>>
>>>>>>> We can install this, AFAIC.
>>>>>>
>>>>>> Thanks, installed on the master branch, and closing the bug.
>>>>>
>>>>> I've only now found time to try Mauro's patch, and while I can 
confirm
>>>>> that it fixes the example of misfontification I reported in my OP,
>>>>> unfortunately I still see the same kind of misfontification with the
>>>>> program of mine that prompted my OP.  I haven't had time to 
investigate
>>>>> further and I'm not sure when I will, but if and when I can 
provide more
>>>>> information, I'll either reopen this bug or file a new one.
>>>>>
>>>>> Steve Berman
>>>>
>>>> Can you share some code? Either here or by private email?
>>>
>>> Rather than posting or sending you the whole program as well as a data
>>> file needed to run it and explaining how to use it, I'd prefer to try
>>> and isolate the problem and provide a small and self-contained code
>>> snippet to reproduce it.  I'll try to do that in the next few days.
>>>
>>> Steve Berman
>>
>> Ok then, I'll wait.
>
> With the attached file I can reproduce the problem.  Save the file and
> then invoke Emacs from master (i.e., with your patch) as follows:
>
> emacs -Q -l srb-widget-test.el -f srb-widget-test
>
> In the displayed buffer "*Widget Test*" the three radio buttons "One",
> "Two" and "Confirm" are active, which the default face visually
> confirms.  (Their labels are fontified with face `widget-unselected'.)
> Now do the following steps:
>
> 1. Hit TAB to move point to radio button "One".
> 2. Type RET to activate the "Reset" button and move point to it.  Also,
>    the radio buttons are now deactivated, which is indicated by their
>    now being fontified with face `widget-inactive' (as are their
>    labels).
> 3. Type RET on the "Reset" button to reactivate the radio buttons.
> 4. Hit TAB twice to move point to radio button "Two". Hitting RET
>    results in the same state as in step 2.
> 5. Repeat step 3 and then hit TAB three times (or S-TAB once) to move
>    point to radio button "Confirm".  This pops up two indented radio
>    buttons "Right" and "Wrong" under "Confirm", and point is now on
>    "Right".  Note that the radio buttons "One", "Two" and "Confirm" are
>    deactivated, while "Right" and "Wrong" are active.
> 6. Type RET on radio button "Right".  This deletes radio buttons "Right"
>    and "Wrong" and moves point to "Reset".  Radio buttons "One", "Two"
>    and "Confirm" remain deactivated (confirmed by tabbing to each and
>    typing RET, which dings and displays the message "Attempt to perform
>    action on inactive widget"); however:
> => Now radio button "One" is misfontified with the default face instead
>    of face `widget-inactive'.
>
> If you repeat step 5 and then tab to radio button "Wrong" and hit RET,
> the resulting state is the same as in step 6, i.e., radio button "One"
> (and only it) is again misfontified.

Thank you for taking your time to put together this recipe.  I'll be
debugging this between today and tomorrow when I'll have some free
time.

> Note that if you load the file and execute the recipe in emacs-30, the
> misfontification of radio button "One" also happens on steps 2, 4 and 5.
> So the correct fontification on these steps in master is due to your
> patch.  But the patch doesn't prevent the misfontification in step 6.
> However, I have found two ways to avoid this misfontification: One way
> is to uncomment the line `(widget-apply srb--radio-widget :activate)' in
> `srb--confirm', the other way is to comment out the line
> `(widget-value-set srb--radio-widget "")' in `srb--result'.

Ok, good to know that the patch was a step forward.  I'm still not sure
if it's the same bug or something different.

Message #136 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sat, 8 Feb 2025 20:50:20 -0300

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

Stephen Berman <stephen.berman <at> gmx.net> writes:

> With the attached file I can reproduce the problem.  Save the file and
> then invoke Emacs from master (i.e., with your patch) as follows:
>
> emacs -Q -l srb-widget-test.el -f srb-widget-test
>
> In the displayed buffer "*Widget Test*" the three radio buttons "One",
> "Two" and "Confirm" are active, which the default face visually
> confirms.  (Their labels are fontified with face `widget-unselected'.)
> Now do the following steps:
>
> 1. Hit TAB to move point to radio button "One".
> 2. Type RET to activate the "Reset" button and move point to it.  Also,
>    the radio buttons are now deactivated, which is indicated by their
>    now being fontified with face `widget-inactive' (as are their
>    labels).
> 3. Type RET on the "Reset" button to reactivate the radio buttons.
> 4. Hit TAB twice to move point to radio button "Two". Hitting RET
>    results in the same state as in step 2.
> 5. Repeat step 3 and then hit TAB three times (or S-TAB once) to move
>    point to radio button "Confirm".  This pops up two indented radio
>    buttons "Right" and "Wrong" under "Confirm", and point is now on
>    "Right".  Note that the radio buttons "One", "Two" and "Confirm" are
>    deactivated, while "Right" and "Wrong" are active.
> 6. Type RET on radio button "Right".  This deletes radio buttons "Right"
>    and "Wrong" and moves point to "Reset".  Radio buttons "One", "Two"
>    and "Confirm" remain deactivated (confirmed by tabbing to each and
>    typing RET, which dings and displays the message "Attempt to perform
>    action on inactive widget"); however:
> => Now radio button "One" is misfontified with the default face instead
>    of face `widget-inactive'.
>
> If you repeat step 5 and then tab to radio button "Wrong" and hit RET,
> the resulting state is the same as in step 6, i.e., radio button "One"
> (and only it) is again misfontified.

OK, it's a similar problem, but this time with the :inactive overlay,
which has FRONT-ADVANCE t.

> ;;; -*- lexical-binding: t; -*-
>
> (defvar srb--radio-widget)
> (defvar srb--confirm-widget)
> (defvar srb--reset-button)
>
> (defun srb--result (widget)
>   (pcase (widget-value widget)
>       ("One" (ignore))
>       ("Two" (ignore))
>       ("Confirm"
>        (goto-char (widget-get srb--radio-widget :to))
>        (setq srb--confirm-widget
>          (widget-create
>           'radio-button-choice
>           :indent 4
>           :notify (lambda (child &rest _)
>             (srb--confirm child))
>           '(item "Right")
>           '(item "Wrong")))))
>   (when srb--reset-button
>     (widget-apply srb--reset-button :activate))
>   (if (equal (widget-value srb--radio-widget) "Confirm")
>       (widget-backward 2)
>     (goto-char (widget-get srb--reset-button :from)))
>   (widget-value-set srb--radio-widget "")
>   (widget-apply srb--radio-widget :deactivate))

Here, before the :deactivate call the widget is already inactive, and
because of that the :deactivate call (which calls
widget-specify-inactive) does nothing at all, since it detects there's
already an :inactive overlay.  That :inactive overlay doesn't include
the 1st button anymore, since that one gets inserted right at the
overlay start.

So, modifying an inactive widget right at the start or at the end of the
overlay can produce this effect.

I don't think we can solve it easily like my patch did with the :from
and :to markers, though.  But here's an idea:

When a widget is already inactive (i.e., it has an :inactive property),
instead of doing nothing we make sure to move the overlay so that it
includes again all the widget, from the :from marker to the :to marker.

If we do that, we should document that if some code modifies an
:inactive widget, it has to call again :deactivate.

I attach a patch with that idea, together with documentation changes and
a new test.

[0001-Fix-bad-fontification-of-inactive-widgets.patch (text/x-patch, attachment)]

Message #139 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sun, 09 Feb 2025 11:31:10 +0100

On Sat, 8 Feb 2025 20:50:20 -0300 Mauro Aranda <maurooaranda <at> gmail.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>
>> With the attached file I can reproduce the problem.  Save the file and
>> then invoke Emacs from master (i.e., with your patch) as follows:
>>
>> emacs -Q -l srb-widget-test.el -f srb-widget-test
>>
>> In the displayed buffer "*Widget Test*" the three radio buttons "One",
>> "Two" and "Confirm" are active, which the default face visually
>> confirms.  (Their labels are fontified with face `widget-unselected'.)
>> Now do the following steps:
>>
>> 1. Hit TAB to move point to radio button "One".
>> 2. Type RET to activate the "Reset" button and move point to it.  Also,
>>    the radio buttons are now deactivated, which is indicated by their
>>    now being fontified with face `widget-inactive' (as are their
>>    labels).
>> 3. Type RET on the "Reset" button to reactivate the radio buttons.
>> 4. Hit TAB twice to move point to radio button "Two". Hitting RET
>>    results in the same state as in step 2.
>> 5. Repeat step 3 and then hit TAB three times (or S-TAB once) to move
>>    point to radio button "Confirm".  This pops up two indented radio
>>    buttons "Right" and "Wrong" under "Confirm", and point is now on
>>    "Right".  Note that the radio buttons "One", "Two" and "Confirm" are
>>    deactivated, while "Right" and "Wrong" are active.
>> 6. Type RET on radio button "Right".  This deletes radio buttons "Right"
>>    and "Wrong" and moves point to "Reset".  Radio buttons "One", "Two"
>>    and "Confirm" remain deactivated (confirmed by tabbing to each and
>>    typing RET, which dings and displays the message "Attempt to perform
>>    action on inactive widget"); however:
>> => Now radio button "One" is misfontified with the default face instead
>>    of face `widget-inactive'.
>>
>> If you repeat step 5 and then tab to radio button "Wrong" and hit RET,
>> the resulting state is the same as in step 6, i.e., radio button "One"
>> (and only it) is again misfontified.
>
> OK, it's a similar problem, but this time with the :inactive overlay,
> which has FRONT-ADVANCE t.
>
>> ;;; -*- lexical-binding: t; -*-
>>
>> (defvar srb--radio-widget)
>> (defvar srb--confirm-widget)
>> (defvar srb--reset-button)
>>
>> (defun srb--result (widget)
>>   (pcase (widget-value widget)
>>       ("One" (ignore))
>>       ("Two" (ignore))
>>       ("Confirm"
>>        (goto-char (widget-get srb--radio-widget :to))
>>        (setq srb--confirm-widget
>>          (widget-create
>>           'radio-button-choice
>>           :indent 4
>>           :notify (lambda (child &rest _)
>>             (srb--confirm child))
>>           '(item "Right")
>>           '(item "Wrong")))))
>>   (when srb--reset-button
>>     (widget-apply srb--reset-button :activate))
>>   (if (equal (widget-value srb--radio-widget) "Confirm")
>>       (widget-backward 2)
>>     (goto-char (widget-get srb--reset-button :from)))
>>   (widget-value-set srb--radio-widget "")
>>   (widget-apply srb--radio-widget :deactivate))
>
> Here, before the :deactivate call the widget is already inactive, and
> because of that the :deactivate call (which calls
> widget-specify-inactive) does nothing at all, since it detects there's
> already an :inactive overlay.  That :inactive overlay doesn't include
> the 1st button anymore, since that one gets inserted right at the
> overlay start.
>
> So, modifying an inactive widget right at the start or at the end of the
> overlay can produce this effect.
>
> I don't think we can solve it easily like my patch did with the :from
> and :to markers, though.  But here's an idea:
>
> When a widget is already inactive (i.e., it has an :inactive property),
> instead of doing nothing we make sure to move the overlay so that it
> includes again all the widget, from the :from marker to the :to marker.
>
> If we do that, we should document that if some code modifies an
> :inactive widget, it has to call again :deactivate.
>
> I attach a patch with that idea, together with documentation changes and
> a new test.

Thanks, I confirm this fixes the remaining misfontification.  However,
if I switch the order of the last two lines above -- i.e, first
deactivate the radio widget and then set its value to the empty
string -- then I see the same misfontification of radio button "One"
that still occurs in emacs-30 (i.e. without both this patch and your
previous one).  Is this expected, and if so, can you explain why?

Steve Berman

Message #142 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sun, 9 Feb 2025 07:42:36 -0300

Stephen Berman <stephen.berman <at> gmx.net> writes:

> On Sat, 8 Feb 2025 20:50:20 -0300 Mauro Aranda 
<maurooaranda <at> gmail.com> wrote:
>> So, modifying an inactive widget right at the start or at the end of the
>> overlay can produce this effect.
>>
>> I don't think we can solve it easily like my patch did with the :from
>> and :to markers, though.  But here's an idea:
>>
>> When a widget is already inactive (i.e., it has an :inactive property),
>> instead of doing nothing we make sure to move the overlay so that it
>> includes again all the widget, from the :from marker to the :to marker.
>>
>> If we do that, we should document that if some code modifies an
>> :inactive widget, it has to call again :deactivate.
>>
>> I attach a patch with that idea, together with documentation changes and
>> a new test.
>
> Thanks, I confirm this fixes the remaining misfontification. However,
> if I switch the order of the last two lines above -- i.e, first
> deactivate the radio widget and then set its value to the empty
> string -- then I see the same misfontification of radio button "One"
> that still occurs in emacs-30 (i.e. without both this patch and your
> previous one).  Is this expected, and if so, can you explain why?
>

Isn't that what I described? I mean, if you first deactivate and then
you change the value, you're modifying an already inactive widget, and
now the documentation says that if you do that, you should make sure the
:deactivate function runs again...

Without my patches, it's a 2-headed bug:
1) The :from marker doesn't include the 1st button anymore. That's the
bug my first patch solved.
2) The :inactive overlay doesn't include the 1st button anymore. That's
the bug my second patch tries to solve.

Message #145 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sun, 09 Feb 2025 12:04:01 +0100

On Sun, 9 Feb 2025 07:42:36 -0300 Mauro Aranda <maurooaranda <at> gmail.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>
>> On Sat, 8 Feb 2025 20:50:20 -0300 Mauro Aranda <maurooaranda <at> gmail.com>
>  wrote:
>>> So, modifying an inactive widget right at the start or at the end of the
>>> overlay can produce this effect.
>>>
>>> I don't think we can solve it easily like my patch did with the :from
>>> and :to markers, though.  But here's an idea:
>>>
>>> When a widget is already inactive (i.e., it has an :inactive property),
>>> instead of doing nothing we make sure to move the overlay so that it
>>> includes again all the widget, from the :from marker to the :to marker.
>>>
>>> If we do that, we should document that if some code modifies an
>>> :inactive widget, it has to call again :deactivate.
>>>
>>> I attach a patch with that idea, together with documentation changes and
>>> a new test.
>>
>> Thanks, I confirm this fixes the remaining misfontification. However,
>> if I switch the order of the last two lines above -- i.e, first
>> deactivate the radio widget and then set its value to the empty
>> string -- then I see the same misfontification of radio button "One"
>> that still occurs in emacs-30 (i.e. without both this patch and your
>> previous one).  Is this expected, and if so, can you explain why?
>>
>
> Isn't that what I described? I mean, if you first deactivate and then
> you change the value, you're modifying an already inactive widget, and
> now the documentation says that if you do that, you should make sure the
> :deactivate function runs again...

Right, thanks.  I was too eager to test your patch and just glanced at
your doc changes without really trying to understand them (or your
comments above).  Sorry, mea culpa.

> Without my patches, it's a 2-headed bug:
> 1) The :from marker doesn't include the 1st button anymore. That's the
> bug my first patch solved.
> 2) The :inactive overlay doesn't include the 1st button anymore. That's
> the bug my second patch tries to solve.

Thanks, this helps my understanding further.  (Nevertheless, much of the
Widget library remains a black box to me...)

Steve Berman

Message #148 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Sun, 9 Feb 2025 08:41:52 -0300

Stephen Berman <stephen.berman <at> gmx.net> writes:

> On Sun, 9 Feb 2025 07:42:36 -0300 Mauro Aranda 
<maurooaranda <at> gmail.com> wrote:
>
>> Stephen Berman <stephen.berman <at> gmx.net> writes:
>>
>>> On Sat, 8 Feb 2025 20:50:20 -0300 Mauro Aranda <maurooaranda <at> gmail.com>
>>  wrote:
>>>> So, modifying an inactive widget right at the start or at the end 
of the
>>>> overlay can produce this effect.
>>>>
>>>> I don't think we can solve it easily like my patch did with the :from
>>>> and :to markers, though.  But here's an idea:
>>>>
>>>> When a widget is already inactive (i.e., it has an :inactive 
property),
>>>> instead of doing nothing we make sure to move the overlay so that it
>>>> includes again all the widget, from the :from marker to the :to 
marker.
>>>>
>>>> If we do that, we should document that if some code modifies an
>>>> :inactive widget, it has to call again :deactivate.
>>>>
>>>> I attach a patch with that idea, together with documentation 
changes and
>>>> a new test.
>>>
>>> Thanks, I confirm this fixes the remaining misfontification. However,
>>> if I switch the order of the last two lines above -- i.e, first
>>> deactivate the radio widget and then set its value to the empty
>>> string -- then I see the same misfontification of radio button "One"
>>> that still occurs in emacs-30 (i.e. without both this patch and your
>>> previous one).  Is this expected, and if so, can you explain why?
>>>
>>
>> Isn't that what I described? I mean, if you first deactivate and then
>> you change the value, you're modifying an already inactive widget, and
>> now the documentation says that if you do that, you should make sure the
>> :deactivate function runs again...
>
> Right, thanks.  I was too eager to test your patch and just glanced at
> your doc changes without really trying to understand them (or your
> comments above).  Sorry, mea culpa.

No problem!

Message #151 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Mauro Aranda <maurooaranda <at> gmail.com>
To: 69941-done <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 24 Feb 2025 19:58:16 -0300

No further comments in two weeks, so I installed the fix.

Message #154 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Mauro Aranda <maurooaranda <at> gmail.com>
Cc: 69941-done <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 24 Feb 2025 23:20:05 -0500

> No further comments in two weeks, so I installed the fix.

Thanks, Mauro!


        Stefan

Message #157 received at 69941-done <at> debbugs.gnu.org (full text, mbox):

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69941-done <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Mauro Aranda <maurooaranda <at> gmail.com>
Subject: Re: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Tue, 25 Feb 2025 10:12:38 +0100

On Mon, 24 Feb 2025 23:20:05 -0500 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> No further comments in two weeks, so I installed the fix.
>
> Thanks, Mauro!

Yes, thanks much for the fix(es) and feedback!

Steve Berman

Previous Next

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