all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#69941: 30.0.50; Faulty fontification of radio button widgets
@ 2024-03-22 14:45 Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-03-22 15:31 ` Eli Zaretskii
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-22 14:45 UTC (permalink / raw)
  To: 69941

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

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


[-- Attachment #2: widget-example.el --]
[-- Type: application/emacs-lisp, Size: 949 bytes --]

[-- Attachment #3: Type: text/plain, Size: 4446 bytes --]


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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: widget-default-create.diff --]
[-- Type: text/x-patch, Size: 608 bytes --]

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

[-- Attachment #5: Type: text/plain, Size: 744 bytes --]


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-03-22 14:45 bug#69941: 30.0.50; Faulty fontification of radio button widgets Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-03-22 15:31 ` Eli Zaretskii
  2024-03-23 20:49   ` Mauro Aranda
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2024-03-22 15:31 UTC (permalink / raw)
  To: Stephen Berman, Mauro Aranda, Stefan Monnier; +Cc: 69941

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

Mauro and Stefan, any comments or suggestions?





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-03-22 15:31 ` Eli Zaretskii
@ 2024-03-23 20:49   ` Mauro Aranda
  2024-03-24 18:45     ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 10+ messages in thread
From: Mauro Aranda @ 2024-03-23 20:49 UTC (permalink / raw)
  To: Eli Zaretskii, Stephen Berman; +Cc: 69941, Stefan Monnier

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

 > 5. Tab back to "Activate" and press RET, again restoring the initial
 > state.  Now tab to radio button "Two" and press RET.
 > => The fontification is the same as in step 4: radio button "Two" has
 > the widget-inactive face but radio button "One" has the default (active)
 > face, though it is again inactive.  Repeatedly pressing either of the
 > radio buttons (after activating them), does not change the fontification
 > of "One" again.
 >
 >
 > The faulty fontification of radio button "One" also obtains if there is
 > just one radio button instead of two, and if there are more than two
 > radio buttons, it is only the first one that displays the odd
 > fontification (admittedly, I've only test up to three radio buttons).
 >
 > I've tried to debug this and found that the problem seems to be due to
 > the sexp (set-marker-insertion-type from t) near the end of
 > widget-default-create, which advances the marker specified by the
 > widget's :from property.  Changing t to nil fixes the faulty
 > fontification of the first radio button.
 >
 > I investigated the history of this code, and while the value t for the
 > marker insertion type was used in the initial commit, it was changed to
 > nil in commit e0f956935, with the message "Insert new text at the :from
 > marker _after_ the marker, not before it."  But 18 days later it was
 > changed back to t in commit 3bff434b8, that also added "Document need to
 > put some text before the %v escape in :format string" of editable-field
 > widgets.  (I looked at the bug-gnu-emacs and emacs-devel mailing list
 > archives but found nothing relevant at the time just prior to these
 > commits.)

I'm pretty sure it makes sense for user-editable widgets that the
value for insertion-type be t.

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






^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-03-23 20:49   ` Mauro Aranda
@ 2024-03-24 18:45     ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-01 15:20       ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-03-24 18:45 UTC (permalink / raw)
  To: Mauro Aranda; +Cc: Eli Zaretskii, 69941, Stefan Monnier

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

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

> Stephen Berman <stephen.berman@gmx.net> writes:
>
>> 5. Tab back to "Activate" and press RET, again restoring the initial
>> state.  Now tab to radio button "Two" and press RET.
>> => The fontification is the same as in step 4: radio button "Two" has
>> the widget-inactive face but radio button "One" has the default (active)
>> face, though it is again inactive.  Repeatedly pressing either of the
>> radio buttons (after activating them), does not change the fontification
>> of "One" again.
>>
>>
>> The faulty fontification of radio button "One" also obtains if there is
>> just one radio button instead of two, and if there are more than two
>> radio buttons, it is only the first one that displays the odd
>> fontification (admittedly, I've only test up to three radio buttons).
>>
>> I've tried to debug this and found that the problem seems to be due to
>> the sexp (set-marker-insertion-type from t) near the end of
>> widget-default-create, which advances the marker specified by the
>> widget's :from property.  Changing t to nil fixes the faulty
>> fontification of the first radio button.
>>
>> I investigated the history of this code, and while the value t for the
>> marker insertion type was used in the initial commit, it was changed to
>> nil in commit e0f956935, with the message "Insert new text at the :from
>> marker _after_ the marker, not before it."  But 18 days later it was
>> changed back to t in commit 3bff434b8, that also added "Document need to
>> put some text before the %v escape in :format string" of editable-field
>> widgets.  (I looked at the bug-gnu-emacs and emacs-devel mailing list
>> archives but found nothing relevant at the time just prior to these
>> commits.)
>
> I'm pretty sure it makes sense for user-editable widgets that the
> value for insertion-type be t.

Yes, if my understanding is correct, it's just radio-button-choice
widgets that need (the effect of) insertion type nil (at least for
setting the widget-inactive face), see below.

>> So evidently the advancing marker insertion type is needed for at least
>> some widgets, though it seems to be problematic for radio buttons.  So I
>> tried to conditionalize the choice of t or nil on the type of the
>> widget.  I used (not (eq 'radio-button (widget-type widget))), since the
>> argument `widget' of widget-default-create is, according to Edebug,
>> indeed radio-button, so negating the eq sexp returns nil, which I had
>> found to be the value of the marker insertion type that fixes the
>> fontification (however, I couldn't think of a way of limiting the
>> conditioning to only the first radio button, but in my testing so far
>> that lack doesn't appear to make a difference).
>
> I'm not sure if the right target is the radio-button widget.  It could
> be the radio-button-choice widget.  Did you try to conditionalize the code
> against the radio-button-choice widget?

I didn't, because I got hung up on the radio-button widget, since in
Edebug that is what I saw and (mistakenly) took to be the current widget
when widget-inactive face is set.  But the resulting marker insertion
type discrepancy is really proof that I was looking at the wrong widget
type (as I already realized in my comments cited below, but I didn't
think to simply try it with radio-button-choice until now, so thanks for
pointing me in the right direction!).  And indeed, with
radio-button-choice, negating the eq test DTRT, i.e., using (not (eq
'radio-button-choice (widget-type widget))) as the condtion results in
the correct fontification.  Since this sexp gives the
radio-button-choice widget's :from property the marker insertion type
nil, there is no discrepancy between using that sexp and directly using
nil, so changing my patch to use that condition would be in improvement.
Alternatively, ...

>> But in fact, using the negation of the value of the eq sexp results in
>> the same faulty fontification, while omitting the negation (as in the
>> attached patch), which yields the advancing insertion type t, gives the
>> correct fontification, just like using nil does.  This makes no sense to
>> me, yet it is reliably reproducible.  The only possible explanation that
>> occurs to me is that the bug is triggered elsewhere in the Emacs code
>> and somehow using the sexp that evaluates to t as the marker insertion
>> type affects that code, while using t itself does not (or rather, has
>> the opposite effect); but how that could be and where the culpable code
>> is, I don't know (as a guess, perhaps in the C code that adds faces, but
>> I don't know how to debug that).  If anyone knows or has an idea what's
>> going on here, please communicate it.  In the meantime I will continue
>> to use the widget library with the patch to see whether it has unwanted
>> consequences.
>
> I don't know much about that code in Emacs.  If we find some hack that
> works maybe we can use that until someone figures it out.  But again,
> given your analysis, I'd like to find out if using the condition on the
> radio-button-choice widget works as expected.  And of course, the hack
> shouldn't be added to the widget-default-create, which should remain
> type agnostic.

... since the issue is fontification with the widget-inactive face,
perhaps a better location for the condition is widget-specify-inactive,
as in the attached patch.  It's still a hack though, since
widget-specify-inactive is also type-agnostic by design.  But if the
issue really is confined to radio-button-choice widget's, I guess any
solution will have to refer to that type.  However, between adding the
condition to widget-specify-inactive or to widget-default-create, I'm
not sure which is less hacky: since the patch to widget-default-create
effectively undoes the result of setting the marker insertion type to t,
perhaps it is cleaner just to set it to nil for radio-button-choice
widgets in widget-default-create.  Or maybe someone will come up with a
better fix...

Steve Berman


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: widget-specify-inactive.diff --]
[-- Type: text/x-patch, Size: 1135 bytes --]

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)

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-03-24 18:45     ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-01 15:20       ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-06 10:18         ` Eli Zaretskii
  2024-04-18 10:18         ` Mauro Aranda
  0 siblings, 2 replies; 10+ messages in thread
From: Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-01 15:20 UTC (permalink / raw)
  To: Mauro Aranda; +Cc: Eli Zaretskii, 69941, Stefan Monnier

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

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

> On Sat, 23 Mar 2024 17:49:53 -0300 Mauro Aranda <maurooaranda@gmail.com> wrote:
>
>> Stephen Berman <stephen.berman@gmx.net> writes:
>>
>>> 5. Tab back to "Activate" and press RET, again restoring the initial
>>> state.  Now tab to radio button "Two" and press RET.
>>> => The fontification is the same as in step 4: radio button "Two" has
>>> the widget-inactive face but radio button "One" has the default (active)
>>> face, though it is again inactive.  Repeatedly pressing either of the
>>> radio buttons (after activating them), does not change the fontification
>>> of "One" again.
>>>
>>>
>>> The faulty fontification of radio button "One" also obtains if there is
>>> just one radio button instead of two, and if there are more than two
>>> radio buttons, it is only the first one that displays the odd
>>> fontification (admittedly, I've only test up to three radio buttons).
>>>
>>> I've tried to debug this and found that the problem seems to be due to
>>> the sexp (set-marker-insertion-type from t) near the end of
>>> widget-default-create, which advances the marker specified by the
>>> widget's :from property.  Changing t to nil fixes the faulty
>>> fontification of the first radio button.
>>>
>>> I investigated the history of this code, and while the value t for the
>>> marker insertion type was used in the initial commit, it was changed to
>>> nil in commit e0f956935, with the message "Insert new text at the :from
>>> marker _after_ the marker, not before it."  But 18 days later it was
>>> changed back to t in commit 3bff434b8, that also added "Document need to
>>> put some text before the %v escape in :format string" of editable-field
>>> widgets.  (I looked at the bug-gnu-emacs and emacs-devel mailing list
>>> archives but found nothing relevant at the time just prior to these
>>> commits.)
>>
>> I'm pretty sure it makes sense for user-editable widgets that the
>> value for insertion-type be t.
>
> Yes, if my understanding is correct, it's just radio-button-choice
> widgets that need (the effect of) insertion type nil (at least for
> setting the widget-inactive face), see below.
>
>>> So evidently the advancing marker insertion type is needed for at least
>>> some widgets, though it seems to be problematic for radio buttons.  So I
>>> tried to conditionalize the choice of t or nil on the type of the
>>> widget.  I used (not (eq 'radio-button (widget-type widget))), since the
>>> argument `widget' of widget-default-create is, according to Edebug,
>>> indeed radio-button, so negating the eq sexp returns nil, which I had
>>> found to be the value of the marker insertion type that fixes the
>>> fontification (however, I couldn't think of a way of limiting the
>>> conditioning to only the first radio button, but in my testing so far
>>> that lack doesn't appear to make a difference).
>>
>> I'm not sure if the right target is the radio-button widget.  It could
>> be the radio-button-choice widget.  Did you try to conditionalize the code
>> against the radio-button-choice widget?
>
> I didn't, because I got hung up on the radio-button widget, since in
> Edebug that is what I saw and (mistakenly) took to be the current widget
> when widget-inactive face is set.  But the resulting marker insertion
> type discrepancy is really proof that I was looking at the wrong widget
> type (as I already realized in my comments cited below, but I didn't
> think to simply try it with radio-button-choice until now, so thanks for
> pointing me in the right direction!).  And indeed, with
> radio-button-choice, negating the eq test DTRT, i.e., using (not (eq
> 'radio-button-choice (widget-type widget))) as the condtion results in
> the correct fontification.  Since this sexp gives the
> radio-button-choice widget's :from property the marker insertion type
> nil, there is no discrepancy between using that sexp and directly using
> nil, so changing my patch to use that condition would be in improvement.
> Alternatively, ...
>
>>> But in fact, using the negation of the value of the eq sexp results in
>>> the same faulty fontification, while omitting the negation (as in the
>>> attached patch), which yields the advancing insertion type t, gives the
>>> correct fontification, just like using nil does.  This makes no sense to
>>> me, yet it is reliably reproducible.  The only possible explanation that
>>> occurs to me is that the bug is triggered elsewhere in the Emacs code
>>> and somehow using the sexp that evaluates to t as the marker insertion
>>> type affects that code, while using t itself does not (or rather, has
>>> the opposite effect); but how that could be and where the culpable code
>>> is, I don't know (as a guess, perhaps in the C code that adds faces, but
>>> I don't know how to debug that).  If anyone knows or has an idea what's
>>> going on here, please communicate it.  In the meantime I will continue
>>> to use the widget library with the patch to see whether it has unwanted
>>> consequences.
>>
>> I don't know much about that code in Emacs.  If we find some hack that
>> works maybe we can use that until someone figures it out.  But again,
>> given your analysis, I'd like to find out if using the condition on the
>> radio-button-choice widget works as expected.  And of course, the hack
>> shouldn't be added to the widget-default-create, which should remain
>> type agnostic.
>
> ... since the issue is fontification with the widget-inactive face,
> perhaps a better location for the condition is widget-specify-inactive,
> as in the attached patch.  It's still a hack though, since
> widget-specify-inactive is also type-agnostic by design.  But if the
> issue really is confined to radio-button-choice widget's, I guess any
> solution will have to refer to that type.  However, between adding the
> condition to widget-specify-inactive or to widget-default-create, I'm
> not sure which is less hacky: since the patch to widget-default-create
> effectively undoes the result of setting the marker insertion type to t,
> perhaps it is cleaner just to set it to nil for radio-button-choice
> widgets in widget-default-create.  Or maybe someone will come up with a
> better fix...
>
> Steve Berman
>
> diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
> index 172da3db1e0..01319853edc 100644
> --- a/lisp/wid-edit.el
> +++ b/lisp/wid-edit.el
> @@ -532,6 +532,17 @@ widget-inactive
>  
>  (defun widget-specify-inactive (widget from to)
>    "Make WIDGET inactive for user modifications."
> +  ;; When WIDGET is a radio-button-choice widget and its first child
> +  ;; radio-button widget is inserted, the marker FROM, which has
> +  ;; insertion type t, advances to the position after the radio button,
> +  ;; and since the overlay setting the widget-inactive face begins at
> +  ;; the position of FROM, this results in the first radio button
> +  ;; incorrectly not being fontified with the widget-inactive face.  To
> +  ;; ensure it is correctly fontified, we move FROM backward by 3,
> +  ;; i.e. the length of the radio-button widget (from its string
> +  ;; representation "( )" or "(x)") (bug#69941).
> +  (when (eq (widget-type widget) 'radio-button-choice)
> +    (set-marker from (- from 3)))
>    (unless (widget-get widget :inactive)
>      (let ((overlay (make-overlay from to nil t nil)))
>        (overlay-put overlay 'face 'widget-inactive)

To fix this bug, do you have a preference between this patch for
widget-specify-inactive and the attached patch for
widget-default-create?  Or do you have a better fix?

Steve Berman


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: widget-default-create patch --]
[-- Type: text/x-patch, Size: 2097 bytes --]

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 172da3db1e0..7fc9ac59b0a 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -1733,8 +1733,17 @@ 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))
+         ;; When WIDGET is a radio-button-choice widget and its first
+         ;; child radio-button widget is inserted, advancing the marker
+         ;; FROM would make the overlay setting the widget-inactive face
+         ;; begin right after the first radio button, which would hence
+         ;; incorrectly not be fontified with the widget-inactive face.
+         ;; To ensure it is correctly fontified, we set the marker
+         ;; insertion type of FROM to nil only when WIDGET is
+         ;; radio-button-choice, otherwise to t (bug#69941).
+         (from-mit (not (eq 'radio-button-choice (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)))
diff --git a/test/lisp/wid-edit-tests.el b/test/lisp/wid-edit-tests.el
index 4b049478b29..d416eb99022 100644
--- a/test/lisp/wid-edit-tests.el
+++ b/test/lisp/wid-edit-tests.el
@@ -336,7 +336,13 @@ widget-test-widget-move
     (widget-forward 2)
     (forward-char)
     (widget-backward 1)
-    (should (string= "Second" (widget-value (widget-at))))))
+    (should (string= "Second" (widget-value (widget-at))))
+    ;; Check that moving to a widget at beginning of buffer does not
+    ;; signal a beginning-of-buffer error (bug#69943).
+    (widget-backward 1)   ; Should not signal beginning-of-buffer error.
+    (widget-forward 2)
+    (should (string= "Third" (widget-value (widget-at))))
+    (widget-forward 1)))  ; Should not signal beginning-of-buffer error.

 (ert-deftest widget-test-color-match ()
   "Test that the :match function for the color widget works."

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-04-01 15:20       ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-06 10:18         ` Eli Zaretskii
  2024-04-18 10:18         ` Mauro Aranda
  1 sibling, 0 replies; 10+ messages in thread
From: Eli Zaretskii @ 2024-04-06 10:18 UTC (permalink / raw)
  To: maurooaranda, Stephen Berman; +Cc: 69941, monnier

Ping!

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  69941@debbugs.gnu.org,  Stefan Monnier
>  <monnier@iro.umontreal.ca>
> Date: Mon, 01 Apr 2024 17:20:40 +0200
> 
> On Sun, 24 Mar 2024 19:45:20 +0100 Stephen Berman <stephen.berman@gmx.net> wrote:
> 
> > On Sat, 23 Mar 2024 17:49:53 -0300 Mauro Aranda <maurooaranda@gmail.com> wrote:
> >
> >> Stephen Berman <stephen.berman@gmx.net> writes:
> >>
> >>> 5. Tab back to "Activate" and press RET, again restoring the initial
> >>> state.  Now tab to radio button "Two" and press RET.
> >>> => The fontification is the same as in step 4: radio button "Two" has
> >>> the widget-inactive face but radio button "One" has the default (active)
> >>> face, though it is again inactive.  Repeatedly pressing either of the
> >>> radio buttons (after activating them), does not change the fontification
> >>> of "One" again.
> >>>
> >>>
> >>> The faulty fontification of radio button "One" also obtains if there is
> >>> just one radio button instead of two, and if there are more than two
> >>> radio buttons, it is only the first one that displays the odd
> >>> fontification (admittedly, I've only test up to three radio buttons).
> >>>
> >>> I've tried to debug this and found that the problem seems to be due to
> >>> the sexp (set-marker-insertion-type from t) near the end of
> >>> widget-default-create, which advances the marker specified by the
> >>> widget's :from property.  Changing t to nil fixes the faulty
> >>> fontification of the first radio button.
> >>>
> >>> I investigated the history of this code, and while the value t for the
> >>> marker insertion type was used in the initial commit, it was changed to
> >>> nil in commit e0f956935, with the message "Insert new text at the :from
> >>> marker _after_ the marker, not before it."  But 18 days later it was
> >>> changed back to t in commit 3bff434b8, that also added "Document need to
> >>> put some text before the %v escape in :format string" of editable-field
> >>> widgets.  (I looked at the bug-gnu-emacs and emacs-devel mailing list
> >>> archives but found nothing relevant at the time just prior to these
> >>> commits.)
> >>
> >> I'm pretty sure it makes sense for user-editable widgets that the
> >> value for insertion-type be t.
> >
> > Yes, if my understanding is correct, it's just radio-button-choice
> > widgets that need (the effect of) insertion type nil (at least for
> > setting the widget-inactive face), see below.
> >
> >>> So evidently the advancing marker insertion type is needed for at least
> >>> some widgets, though it seems to be problematic for radio buttons.  So I
> >>> tried to conditionalize the choice of t or nil on the type of the
> >>> widget.  I used (not (eq 'radio-button (widget-type widget))), since the
> >>> argument `widget' of widget-default-create is, according to Edebug,
> >>> indeed radio-button, so negating the eq sexp returns nil, which I had
> >>> found to be the value of the marker insertion type that fixes the
> >>> fontification (however, I couldn't think of a way of limiting the
> >>> conditioning to only the first radio button, but in my testing so far
> >>> that lack doesn't appear to make a difference).
> >>
> >> I'm not sure if the right target is the radio-button widget.  It could
> >> be the radio-button-choice widget.  Did you try to conditionalize the code
> >> against the radio-button-choice widget?
> >
> > I didn't, because I got hung up on the radio-button widget, since in
> > Edebug that is what I saw and (mistakenly) took to be the current widget
> > when widget-inactive face is set.  But the resulting marker insertion
> > type discrepancy is really proof that I was looking at the wrong widget
> > type (as I already realized in my comments cited below, but I didn't
> > think to simply try it with radio-button-choice until now, so thanks for
> > pointing me in the right direction!).  And indeed, with
> > radio-button-choice, negating the eq test DTRT, i.e., using (not (eq
> > 'radio-button-choice (widget-type widget))) as the condtion results in
> > the correct fontification.  Since this sexp gives the
> > radio-button-choice widget's :from property the marker insertion type
> > nil, there is no discrepancy between using that sexp and directly using
> > nil, so changing my patch to use that condition would be in improvement.
> > Alternatively, ...
> >
> >>> But in fact, using the negation of the value of the eq sexp results in
> >>> the same faulty fontification, while omitting the negation (as in the
> >>> attached patch), which yields the advancing insertion type t, gives the
> >>> correct fontification, just like using nil does.  This makes no sense to
> >>> me, yet it is reliably reproducible.  The only possible explanation that
> >>> occurs to me is that the bug is triggered elsewhere in the Emacs code
> >>> and somehow using the sexp that evaluates to t as the marker insertion
> >>> type affects that code, while using t itself does not (or rather, has
> >>> the opposite effect); but how that could be and where the culpable code
> >>> is, I don't know (as a guess, perhaps in the C code that adds faces, but
> >>> I don't know how to debug that).  If anyone knows or has an idea what's
> >>> going on here, please communicate it.  In the meantime I will continue
> >>> to use the widget library with the patch to see whether it has unwanted
> >>> consequences.
> >>
> >> I don't know much about that code in Emacs.  If we find some hack that
> >> works maybe we can use that until someone figures it out.  But again,
> >> given your analysis, I'd like to find out if using the condition on the
> >> radio-button-choice widget works as expected.  And of course, the hack
> >> shouldn't be added to the widget-default-create, which should remain
> >> type agnostic.
> >
> > ... since the issue is fontification with the widget-inactive face,
> > perhaps a better location for the condition is widget-specify-inactive,
> > as in the attached patch.  It's still a hack though, since
> > widget-specify-inactive is also type-agnostic by design.  But if the
> > issue really is confined to radio-button-choice widget's, I guess any
> > solution will have to refer to that type.  However, between adding the
> > condition to widget-specify-inactive or to widget-default-create, I'm
> > not sure which is less hacky: since the patch to widget-default-create
> > effectively undoes the result of setting the marker insertion type to t,
> > perhaps it is cleaner just to set it to nil for radio-button-choice
> > widgets in widget-default-create.  Or maybe someone will come up with a
> > better fix...
> >
> > Steve Berman
> >
> > diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
> > index 172da3db1e0..01319853edc 100644
> > --- a/lisp/wid-edit.el
> > +++ b/lisp/wid-edit.el
> > @@ -532,6 +532,17 @@ widget-inactive
> >  
> >  (defun widget-specify-inactive (widget from to)
> >    "Make WIDGET inactive for user modifications."
> > +  ;; When WIDGET is a radio-button-choice widget and its first child
> > +  ;; radio-button widget is inserted, the marker FROM, which has
> > +  ;; insertion type t, advances to the position after the radio button,
> > +  ;; and since the overlay setting the widget-inactive face begins at
> > +  ;; the position of FROM, this results in the first radio button
> > +  ;; incorrectly not being fontified with the widget-inactive face.  To
> > +  ;; ensure it is correctly fontified, we move FROM backward by 3,
> > +  ;; i.e. the length of the radio-button widget (from its string
> > +  ;; representation "( )" or "(x)") (bug#69941).
> > +  (when (eq (widget-type widget) 'radio-button-choice)
> > +    (set-marker from (- from 3)))
> >    (unless (widget-get widget :inactive)
> >      (let ((overlay (make-overlay from to nil t nil)))
> >        (overlay-put overlay 'face 'widget-inactive)
> 
> To fix this bug, do you have a preference between this patch for
> widget-specify-inactive and the attached patch for
> widget-default-create?  Or do you have a better fix?
> 
> Steve Berman





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-04-01 15:20       ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-06 10:18         ` Eli Zaretskii
@ 2024-04-18 10:18         ` Mauro Aranda
  2024-04-18 11:33           ` Eli Zaretskii
  1 sibling, 1 reply; 10+ messages in thread
From: Mauro Aranda @ 2024-04-18 10:18 UTC (permalink / raw)
  To: Stephen Berman; +Cc: Eli Zaretskii, 69941, Stefan Monnier

 > On Sun, 24 Mar 2024 19:45:20 +0100 Stephen Berman 
<stephen.berman@gmx.net> wrote:
 >
 >> On Sat, 23 Mar 2024 17:49:53 -0300 Mauro Aranda 
<maurooaranda@gmail.com> wrote:
 >>
 >>> Stephen Berman <stephen.berman@gmx.net> writes:
 >>>
 >>>> 5. Tab back to "Activate" and press RET, again restoring the initial
 >>>> state.  Now tab to radio button "Two" and press RET.
 >>>> => The fontification is the same as in step 4: radio button "Two" has
 >>>> the widget-inactive face but radio button "One" has the default 
(active)
 >>>> face, though it is again inactive.  Repeatedly pressing either of the
 >>>> radio buttons (after activating them), does not change the 
fontification
 >>>> of "One" again.
 >>>>
 >>>>
 >>>> The faulty fontification of radio button "One" also obtains if 
there is
 >>>> just one radio button instead of two, and if there are more than two
 >>>> radio buttons, it is only the first one that displays the odd
 >>>> fontification (admittedly, I've only test up to three radio buttons).
 >>>>
 >>>> I've tried to debug this and found that the problem seems to be due to
 >>>> the sexp (set-marker-insertion-type from t) near the end of
 >>>> widget-default-create, which advances the marker specified by the
 >>>> widget's :from property.  Changing t to nil fixes the faulty
 >>>> fontification of the first radio button.
 >>>>
 >>>> I investigated the history of this code, and while the value t for the
 >>>> marker insertion type was used in the initial commit, it was 
changed to
 >>>> nil in commit e0f956935, with the message "Insert new text at the 
:from
 >>>> marker _after_ the marker, not before it."  But 18 days later it was
 >>>> changed back to t in commit 3bff434b8, that also added "Document 
need to
 >>>> put some text before the %v escape in :format string" of 
editable-field
 >>>> widgets.  (I looked at the bug-gnu-emacs and emacs-devel mailing list
 >>>> archives but found nothing relevant at the time just prior to these
 >>>> commits.)
 >>>
 >>> I'm pretty sure it makes sense for user-editable widgets that the
 >>> value for insertion-type be t.
 >>
 >> Yes, if my understanding is correct, it's just radio-button-choice
 >> widgets that need (the effect of) insertion type nil (at least for
 >> setting the widget-inactive face), see below.
 >>
 >>>> So evidently the advancing marker insertion type is needed for at 
least
 >>>> some widgets, though it seems to be problematic for radio 
buttons.  So I
 >>>> tried to conditionalize the choice of t or nil on the type of the
 >>>> widget.  I used (not (eq 'radio-button (widget-type widget))), 
since the
 >>>> argument `widget' of widget-default-create is, according to Edebug,
 >>>> indeed radio-button, so negating the eq sexp returns nil, which I had
 >>>> found to be the value of the marker insertion type that fixes the
 >>>> fontification (however, I couldn't think of a way of limiting the
 >>>> conditioning to only the first radio button, but in my testing so far
 >>>> that lack doesn't appear to make a difference).
 >>>
 >>> I'm not sure if the right target is the radio-button widget.  It could
 >>> be the radio-button-choice widget.  Did you try to conditionalize 
the code
 >>> against the radio-button-choice widget?
 >>
 >> I didn't, because I got hung up on the radio-button widget, since in
 >> Edebug that is what I saw and (mistakenly) took to be the current widget
 >> when widget-inactive face is set.  But the resulting marker insertion
 >> type discrepancy is really proof that I was looking at the wrong widget
 >> type (as I already realized in my comments cited below, but I didn't
 >> think to simply try it with radio-button-choice until now, so thanks for
 >> pointing me in the right direction!).  And indeed, with
 >> radio-button-choice, negating the eq test DTRT, i.e., using (not (eq
 >> 'radio-button-choice (widget-type widget))) as the condtion results in
 >> the correct fontification.  Since this sexp gives the
 >> radio-button-choice widget's :from property the marker insertion type
 >> nil, there is no discrepancy between using that sexp and directly using
 >> nil, so changing my patch to use that condition would be in improvement.
 >> Alternatively, ...
 >>
 >>>> But in fact, using the negation of the value of the eq sexp results in
 >>>> the same faulty fontification, while omitting the negation (as in the
 >>>> attached patch), which yields the advancing insertion type t, 
gives the
 >>>> correct fontification, just like using nil does. This makes no 
sense to
 >>>> me, yet it is reliably reproducible.  The only possible 
explanation that
 >>>> occurs to me is that the bug is triggered elsewhere in the Emacs code
 >>>> and somehow using the sexp that evaluates to t as the marker insertion
 >>>> type affects that code, while using t itself does not (or rather, has
 >>>> the opposite effect); but how that could be and where the culpable 
code
 >>>> is, I don't know (as a guess, perhaps in the C code that adds 
faces, but
 >>>> I don't know how to debug that).  If anyone knows or has an idea 
what's
 >>>> going on here, please communicate it.  In the meantime I will continue
 >>>> to use the widget library with the patch to see whether it has 
unwanted
 >>>> consequences.
 >>>
 >>> I don't know much about that code in Emacs.  If we find some hack that
 >>> works maybe we can use that until someone figures it out.  But again,
 >>> given your analysis, I'd like to find out if using the condition on the
 >>> radio-button-choice widget works as expected.  And of course, the hack
 >>> shouldn't be added to the widget-default-create, which should remain
 >>> type agnostic.
 >>
 >> ... since the issue is fontification with the widget-inactive face,
 >> perhaps a better location for the condition is widget-specify-inactive,
 >> as in the attached patch.  It's still a hack though, since
 >> widget-specify-inactive is also type-agnostic by design. But if the
 >> issue really is confined to radio-button-choice widget's, I guess any
 >> solution will have to refer to that type.  However, between adding the
 >> condition to widget-specify-inactive or to widget-default-create, I'm
 >> not sure which is less hacky: since the patch to widget-default-create
 >> effectively undoes the result of setting the marker insertion type to t,
 >> perhaps it is cleaner just to set it to nil for radio-button-choice
 >> widgets in widget-default-create.  Or maybe someone will come up with a
 >> better fix...
 >>
 >> Steve Berman
 >>
 >> diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
 >> index 172da3db1e0..01319853edc 100644
 >> --- a/lisp/wid-edit.el
 >> +++ b/lisp/wid-edit.el
 >> @@ -532,6 +532,17 @@ widget-inactive
 >>
 >>  (defun widget-specify-inactive (widget from to)
 >>    "Make WIDGET inactive for user modifications."
 >> +  ;; When WIDGET is a radio-button-choice widget and its first child
 >> +  ;; radio-button widget is inserted, the marker FROM, which has
 >> +  ;; insertion type t, advances to the position after the radio button,
 >> +  ;; and since the overlay setting the widget-inactive face begins at
 >> +  ;; the position of FROM, this results in the first radio button
 >> +  ;; incorrectly not being fontified with the widget-inactive face.  To
 >> +  ;; ensure it is correctly fontified, we move FROM backward by 3,
 >> +  ;; i.e. the length of the radio-button widget (from its string
 >> +  ;; representation "( )" or "(x)") (bug#69941).
 >> +  (when (eq (widget-type widget) 'radio-button-choice)
 >> +    (set-marker from (- from 3)))
 >>    (unless (widget-get widget :inactive)
 >>      (let ((overlay (make-overlay from to nil t nil)))
 >>        (overlay-put overlay 'face 'widget-inactive)
 >
 > To fix this bug, do you have a preference between this patch for
 > widget-specify-inactive and the attached patch for
 > widget-default-create?  Or do you have a better fix?
 >
 > Steve Berman

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

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






^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-04-18 10:18         ` Mauro Aranda
@ 2024-04-18 11:33           ` Eli Zaretskii
  2024-04-18 13:38             ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 10+ messages in thread
From: Eli Zaretskii @ 2024-04-18 11:33 UTC (permalink / raw)
  To: Mauro Aranda; +Cc: 69941, stephen.berman, monnier

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





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-04-18 11:33           ` Eli Zaretskii
@ 2024-04-18 13:38             ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-04-21 19:45               ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 10+ messages in thread
From: Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-18 13:38 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 69941, Mauro Aranda, monnier

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

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





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#69941: 30.0.50; Faulty fontification of radio button widgets
  2024-04-18 13:38             ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-04-21 19:45               ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 10+ messages in thread
From: Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-04-21 19:45 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 69941, Mauro Aranda, monnier

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

> On Thu, 18 Apr 2024 14:33:57 +0300 Eli Zaretskii <eliz@gnu.org> wrote:
>
>>> Date: Thu, 18 Apr 2024 07:18:29 -0300
>>> Cc: Eli Zaretskii <eliz@gnu.org>, 69941@debbugs.gnu.org,
>>>  Stefan Monnier <monnier@iro.umontreal.ca>
>>> From: Mauro Aranda <maurooaranda@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





^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-04-21 19:45 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-22 14:45 bug#69941: 30.0.50; Faulty fontification of radio button widgets Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-22 15:31 ` Eli Zaretskii
2024-03-23 20:49   ` Mauro Aranda
2024-03-24 18:45     ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-01 15:20       ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-06 10:18         ` Eli Zaretskii
2024-04-18 10:18         ` Mauro Aranda
2024-04-18 11:33           ` Eli Zaretskii
2024-04-18 13:38             ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-04-21 19:45               ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.