From: Stephen Berman via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
To: Mauro Aranda <maurooaranda@gmail.com>
Cc: Eli Zaretskii <eliz@gnu.org>,
69941@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
Subject: bug#69941: 30.0.50; Faulty fontification of radio button widgets
Date: Mon, 01 Apr 2024 17:20:40 +0200 [thread overview]
Message-ID: <8734s5w1mf.fsf@gmx.net> (raw)
In-Reply-To: <87wmprqxj3.fsf@gmx.net> (Stephen Berman's message of "Sun, 24 Mar 2024 19:45:20 +0100")
[-- 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."
next prev parent reply other threads:[~2024-04-01 15:20 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
2024-04-26 12:45 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-09 7:21 ` Eli Zaretskii
2024-05-09 14:15 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-11 13:59 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-13 2:22 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-13 13:15 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-13 13:26 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-13 13:28 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-13 13:53 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-13 14:19 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-05-25 7:51 ` Eli Zaretskii
2024-05-25 9:30 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-06-08 11:50 ` Eli Zaretskii
2024-06-08 12:12 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-06-08 13:32 ` Eli Zaretskii
2024-06-08 14:14 ` Stephen Berman via Bug reports for GNU Emacs, the Swiss army knife of text editors
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8734s5w1mf.fsf@gmx.net \
--to=bug-gnu-emacs@gnu.org \
--cc=69941@debbugs.gnu.org \
--cc=eliz@gnu.org \
--cc=maurooaranda@gmail.com \
--cc=monnier@iro.umontreal.ca \
--cc=stephen.berman@gmx.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).