GNU bug report logs - #51550
29.0.50; Customize Group INS buttons sometimes don't have a left box line

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 51550 in the body.
You can then email your comments to 51550 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: Po Lu <luangruo <at> yahoo.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; Customize Group INS buttons sometimes don't have a left
 box line
Date: Mon, 01 Nov 2021 21:52:17 +0800

Run emacs -Q.  Do M-x customize-group RET files RET, expand "After Save
Hook", then click "INS" twice.  The second INS button will not have a
left box line, which is visually inconsistent with the first, and with
how buttons work in general across all X-Windows applications.

The repository branch used is not relevant here, it can also be
reproduced in master and emacs-28.

Thanks.

In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30, cairo version 1.17.4)
 of 2021-11-01 built on limity
Repository branch: x-window-xwidget
Windowing system distributor 'The X.Org Foundation', version 11.0.12101002
System Description: Fedora 34 (Workstation Edition)

Configured using:
 'configure --with-xwidgets'

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

Important settings:
  value of $LANG: en_GB.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Custom

Minor modes in effect:
  shell-dirtrack-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny rfc822 mml mml-sec epa
derived gnus-util rmail rmail-loaddefs text-property-search mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
shadowfile tramp tramp-loaddefs trampver tramp-integration files-x
tramp-compat shell pcomplete parse-time iso8601 time-date ls-lisp
format-spec auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map recentf tree-widget plstore epg rfc6068
epg-config latexenc filecache dired dired-loaddefs autorevert filenotify
autoinsert seq gv subr-x byte-opt bytecomp byte-compile cconv ange-ftp
comint ansi-color ring cus-edit pp cus-start cus-load wid-edit help-mode
cl-loaddefs cl-lib iso-transl tooltip eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads xwidget-internal dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
cairo move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 100745 13815)
 (symbols 48 10589 1)
 (strings 32 32135 2089)
 (string-bytes 1 1038099)
 (vectors 16 19747)
 (vector-slots 8 238756 13660)
 (floats 8 41 129)
 (intervals 56 635 172)
 (buffers 992 13))

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50;
 Customize Group INS buttons sometimes don't have a left box line
Date: Mon, 01 Nov 2021 16:29:42 +0200

> Date: Mon, 01 Nov 2021 21:52:17 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> 
> Run emacs -Q.  Do M-x customize-group RET files RET, expand "After Save
> Hook", then click "INS" twice.  The second INS button will not have a
> left box line, which is visually inconsistent with the first, and with
> how buttons work in general across all X-Windows applications.

I cannot reproduce this.  Maybe this is specific to GTK?

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Po Lu <luangruo <at> yahoo.com>, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Mon, 01 Nov 2021 20:28:40 +0100

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

On Mon, 01 Nov 2021 16:29:42 +0200 Eli Zaretskii <eliz <at> gnu.org> wrote:

>> Date: Mon, 01 Nov 2021 21:52:17 +0800
>> From:  Po Lu via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>>
>> Run emacs -Q.  Do M-x customize-group RET files RET, expand "After Save
>> Hook", then click "INS" twice.  The second INS button will not have a
>> left box line, which is visually inconsistent with the first, and with
>> how buttons work in general across all X-Windows applications.
>
> I cannot reproduce this.  Maybe this is specific to GTK?

I see this on a no-toolkit build as well as a Gtk build.  For me, it
suffices to click "INS" once, see attached screenshot.

Steve

[bug#51550.png (image/png, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: luangruo <at> yahoo.com, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Mon, 01 Nov 2021 21:38:51 +0200

> From: Stephen Berman <stephen.berman <at> gmx.net>
> Cc: Po Lu <luangruo <at> yahoo.com>,  51550 <at> debbugs.gnu.org
> Date: Mon, 01 Nov 2021 20:28:40 +0100
> 
> >> Run emacs -Q.  Do M-x customize-group RET files RET, expand "After Save
> >> Hook", then click "INS" twice.  The second INS button will not have a
> >> left box line, which is visually inconsistent with the first, and with
> >> how buttons work in general across all X-Windows applications.
> >
> > I cannot reproduce this.  Maybe this is specific to GTK?
> 
> I see this on a no-toolkit build as well as a Gtk build.  For me, it
> suffices to click "INS" once, see attached screenshot.

According to this screenshot, the problematic button is the first one,
because on my system the first one doesn't have the left and the top
border lines, and neither do all the rest.

I see that xterm does something special in x_draw_relief_rect when the
line width is exactly 1 pixel, and w32term.c doesn't do that.  maybe
that's the problem.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Stephen Berman <stephen.berman <at> gmx.net>, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 02 Nov 2021 09:58:31 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> I see that xterm does something special in x_draw_relief_rect when the
> line width is exactly 1 pixel, and w32term.c doesn't do that.  maybe
> that's the problem.

That isn't related, it only serves to make buttons look nicer and have
more contrast by drawing a black line outside a relief rect.

FWIW, this bug doesn't manifest in 27.2. 

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Po Lu <luangruo <at> yahoo.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 02 Nov 2021 11:58:16 +0100

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

On Tue, 02 Nov 2021 09:58:31 +0800 Po Lu <luangruo <at> yahoo.com> wrote:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>> I see that xterm does something special in x_draw_relief_rect when the
>> line width is exactly 1 pixel, and w32term.c doesn't do that.  maybe
>> that's the problem.
>
> That isn't related, it only serves to make buttons look nicer and have
> more contrast by drawing a black line outside a relief rect.
>
> FWIW, this bug doesn't manifest in 27.2.

With the following patch I no longer see the display glitch:

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

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index fb06a95f51..ba6f26fccc 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -373,7 +373,7 @@ widget-specify-field
 	   (forward-char 1))
 	  (widget-field-add-space
 	   (insert-and-inherit " ")))
-    (setq to (point)))
+    (setq to (1- (point))))
   (let ((keymap (widget-get widget :keymap))
 	(face (or (widget-get widget :value-face) 'widget-field))
 	(help-echo (widget-get widget :help-echo))

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

However, this cannot be the right fix: with this patch the editable
field widget is no longer editable (it also doesnt't extended to the
right window margin, and it's one space shorter than when the
widget-field face is edited to remove the :extend property (without the
patch)).  Moreover, in emacs-27 the editable field is also extended yet
there's no display glitch.  (There's one difference in
widget-specify-field between emacs-27 and 28/master, but undoing that
change in the latter does not eliminate the display glitch.)

There's also something strange about customizing the widget-field face:
when I uncheck the box for "Extend" and click the "State" button to "Set
for Current Session", nothing happens with -Q and without -Q I get the
message "End of file during parsing".  If (without -Q) I click "State"
to "Save for Future Sessions", then it says "SAVED and set" but the
"Extend" checkbox remains checked and the editable fields are extended.
If I keep "Extend" checked and click the Value Menu to "Off", then I can
set the change for the current session, and then the editable field
widget is not extended, but the display glitch remains (and the editable
field is one space longer than with the above patch).

Steve Berman

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 02 Nov 2021 16:13:35 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: Stephen Berman <stephen.berman <at> gmx.net>,  51550 <at> debbugs.gnu.org
> Date: Tue, 02 Nov 2021 09:58:31 +0800
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > I see that xterm does something special in x_draw_relief_rect when the
> > line width is exactly 1 pixel, and w32term.c doesn't do that.  maybe
> > that's the problem.
> 
> That isn't related, it only serves to make buttons look nicer and have
> more contrast by drawing a black line outside a relief rect.

So I guess someone will have to step through the relevant drawing code
and see where do those two border lines come from.  I cannot do this
because the problem doesn't exist on my system.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 03 Nov 2021 07:59:44 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> So I guess someone will have to step through the relevant drawing code
> and see where do those two border lines come from.  I cannot do this
> because the problem doesn't exist on my system.

Thanks, I'll bisect when I get the time.  But you have got one minor
detail wrong: on 27.2, all buttons have a left box line, so the bug is
in the buttons that don't.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 03 Nov 2021 13:35:00 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> So I guess someone will have to step through the relevant drawing code
> and see where do those two border lines come from.  I cannot do this
> because the problem doesn't exist on my system.

It turns out that it's not a bug in the display code.  Bisect says:

commit 8b024a6ff10f7907445ea60c4db8355638616ed1
Author: Stefan Kangas <stefan <at> marxist.se>
Date:   Mon Mar 15 00:27:20 2021 +0100

    * lisp/wid-edit.el (widget-field): Add subtle border to face.

 lisp/wid-edit.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Thanks.

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Po Lu <luangruo <at> yahoo.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 3 Nov 2021 00:03:00 -0700

Po Lu <luangruo <at> yahoo.com> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>> So I guess someone will have to step through the relevant drawing code
>> and see where do those two border lines come from.  I cannot do this
>> because the problem doesn't exist on my system.
>
> It turns out that it's not a bug in the display code.  Bisect says:
>
> commit 8b024a6ff10f7907445ea60c4db8355638616ed1
> Author: Stefan Kangas <stefan <at> marxist.se>
> Date:   Mon Mar 15 00:27:20 2021 +0100
>
>     * lisp/wid-edit.el (widget-field): Add subtle border to face.
>
>  lisp/wid-edit.el | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

It seems like the entire line is shifted a pixel to the left when using
:box, but only on the second consecutive line.  I tried experimenting
with :box 1, :box -1, and I get the same result.

Are we sure that's not a bug in the display code?

In any case, we could revert the commit for emacs-28 and continue investigating
on master.

The diff is:

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index de2b5d4a7c..35e7b9ce7e 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -131,16 +131,21 @@ widget-field
                        (((class grayscale color)
                          (background light))
                         :background "gray85"
+                         ;; We use negative thickness of the
horizontal box border line to
+                         ;; avoid making lines taller when fields
become visible.
+                         :box (:line-width (1 . -1) :color "gray80")
                         :extend t)
                        (((class grayscale color)
                          (background dark))
                         :background "dim gray"
+                         :box (:line-width (1 . -1) :color "gray46")
                         :extend t)
                        (t
                         :slant italic
                         :extend t))
   "Face used for editable fields."
-  :group 'widget-faces)
+  :group 'widget-faces
+  :version "28.1")

 (defface widget-single-line-field '((((type tty))
                                     :background "green3"

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: luangruo <at> yahoo.com, stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 03 Nov 2021 14:53:46 +0200

> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 3 Nov 2021 00:03:00 -0700
> Cc: stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
> 
> It seems like the entire line is shifted a pixel to the left when using
> :box, but only on the second consecutive line.  I tried experimenting
> with :box 1, :box -1, and I get the same result.
> 
> Are we sure that's not a bug in the display code?

I don't know, the arrangement of faces on that button is complicated,
to say the least.  I don't think I even understand which parts of the
display is the widget-field face applied to.  Can you show the
complete definition of the button face at the first line and the
non-first line?

> In any case, we could revert the commit for emacs-28 and continue investigating
> on master.

Yes, I think we should do that.

Thanks.

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

From: Stefan Kangas <stefan <at> marxist.se>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Fri, 5 Nov 2021 00:27:10 -0700

Eli Zaretskii <eliz <at> gnu.org> writes:

>> In any case, we could revert the commit for emacs-28 and continue investigating
>> on master.
>
> Yes, I think we should do that.

Done in commit d8c9a9dc23.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Mon, 27 Dec 2021 18:50:22 +0800

Stefan Kangas <stefan <at> marxist.se> writes:

> Eli Zaretskii <eliz <at> gnu.org> writes:
>
>>> In any case, we could revert the commit for emacs-28 and continue investigating
>>> on master.
>>
>> Yes, I think we should do that.
>
> Done in commit d8c9a9dc23.

I think I found the bug, it's in display_line.

When the label at_end_of_line is reached, it->start_of_box_run_p can be
false.

Afterwards, set_iterator_to_next is called, which reseats the iterator
to the next line, but doesn't set it->start_of_box_run_p if the face is
now different from the previous face and also has a box.

I think the solution is to save the face ID of the iterator after the
call to extend_face_to_end_of_line, then compare it to the face after
the iterator is reseated to the next line, and set
it->start_of_box_run_p to true if that face is different and also has a
box.

WDYT?

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

From: Po Lu <luangruo <at> yahoo.com>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Mon, 27 Dec 2021 19:19:47 +0800

Po Lu <luangruo <at> yahoo.com> writes:

> I think the solution is to save the face ID of the iterator after the
> call to extend_face_to_end_of_line, then compare it to the face after
> the iterator is reseated to the next line, and set
> it->start_of_box_run_p to true if that face is different and also has a
> box.

I have this code mostly working now, but I don't understand why
set_iterator_to_next doesn't seem to be updating it->face_id.

AFAIU, set_iterator_to_next called with RESEAT set to true eventually
calls reseat, which in turn calls handle_face_prop through handle_stop
if the iterator was reseated past the stop position, which should be set
to the beginning of the next line when the face property at that next
line differs from the line whose face is being extended.

However, handle_face_prop is never called, so I have to call it manually
like this:

	  /* Consume the line end.  This skips over invisible lines.  */
	  set_iterator_to_next (it, true);

-->	  handle_face_prop (it);
	  if (it->face_id != DEFAULT_FACE_ID
	      && it->face_id != saved_face_id)
	    {
	      face = FACE_FROM_ID_OR_NULL (it->f, it->face_id);
	      if (face && face->box != FACE_NO_BOX)
		{
		  it->face_box_p = true;
		  it->start_of_box_run_p = true;
		}
	    }

Any ideas?  Thanks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Mon, 27 Dec 2021 16:55:49 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  stephen.berman <at> gmx.net,
>   51550 <at> debbugs.gnu.org
> Date: Mon, 27 Dec 2021 18:50:22 +0800
> 
> I think I found the bug, it's in display_line.
> 
> When the label at_end_of_line is reached, it->start_of_box_run_p can be
> false.
> 
> Afterwards, set_iterator_to_next is called, which reseats the iterator
> to the next line, but doesn't set it->start_of_box_run_p if the face is
> now different from the previous face and also has a box.
> 
> I think the solution is to save the face ID of the iterator after the
> call to extend_face_to_end_of_line, then compare it to the face after
> the iterator is reseated to the next line, and set
> it->start_of_box_run_p to true if that face is different and also has a
> box.

Can you show me a simple Lisp that would reproduce such a problem?  I
don't think I follow your description.  You are actually saying that a
screen line cannot begin with text that has a face with the box
attribute set?

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 28 Dec 2021 08:37:28 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Po Lu <luangruo <at> yahoo.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>,  stephen.berman <at> gmx.net,
>>   51550 <at> debbugs.gnu.org
>> Date: Mon, 27 Dec 2021 18:50:22 +0800
>> 
>> I think I found the bug, it's in display_line.
>> 
>> When the label at_end_of_line is reached, it->start_of_box_run_p can be
>> false.
>> 
>> Afterwards, set_iterator_to_next is called, which reseats the iterator
>> to the next line, but doesn't set it->start_of_box_run_p if the face is
>> now different from the previous face and also has a box.
>> 
>> I think the solution is to save the face ID of the iterator after the
>> call to extend_face_to_end_of_line, then compare it to the face after
>> the iterator is reseated to the next line, and set
>> it->start_of_box_run_p to true if that face is different and also has a
>> box.

> Can you show me a simple Lisp that would reproduce such a problem?  I
> don't think I follow your description.  You are actually saying that a
> screen line cannot begin with text that has a face with the box
> attribute set?

Run this:

(insert #("foo\nfoo " 0 4 (face widget-field) 4 7 (face custom-button)))

The second foo will be missing a start box line, which is the bug here.

Thanks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 28 Dec 2021 14:40:49 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: stefan <at> marxist.se,  stephen.berman <at> gmx.net,  51550 <at> debbugs.gnu.org
> Date: Tue, 28 Dec 2021 08:37:28 +0800
> 
> (insert #("foo\nfoo " 0 4 (face widget-field) 4 7 (face custom-button)))
> 
> The second foo will be missing a start box line, which is the bug here.

I didn't yet look seriously enough at that, but: I can only reproduce
this problem on the master branch; Emacs 28 works as expected.  Is
that what you see as well?  If so, then it is strange that you think
the problem happens in the code that wasn't touched at all between
Emacs 28 and Emacs 29, AFAICT.  Most probably, the root cause is
somewhere else.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 28 Dec 2021 20:45:22 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> I didn't yet look seriously enough at that, but: I can only reproduce
> this problem on the master branch; Emacs 28 works as expected.  Is
> that what you see as well?

Yes, widget-field has a box on master, while it doesn't on emacs-28.  In
fact, it was you who suggested disabling the buggy code on Emacs 28 :)

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 28 Dec 2021 15:17:48 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: stefan <at> marxist.se,  stephen.berman <at> gmx.net,  51550 <at> debbugs.gnu.org
> Date: Tue, 28 Dec 2021 20:45:22 +0800
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > I didn't yet look seriously enough at that, but: I can only reproduce
> > this problem on the master branch; Emacs 28 works as expected.  Is
> > that what you see as well?
> 
> Yes, widget-field has a box on master, while it doesn't on emacs-28.

The other way around, I guess?

Or maybe I don't understand what you mean by "box"?

> In fact, it was you who suggested disabling the buggy code on Emacs
> 28 :)

Which buggy code was that, and where did I suggest disabling it?

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 28 Dec 2021 21:24:41 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Yes, widget-field has a box on master, while it doesn't on emacs-28.

> The other way around, I guess?

Causing widget-field to have a box (which gets extended to the end of
the line) makes the custom button on the next line lose its start box
line, so it's not the other way around.

> Which buggy code was that, and where did I suggest disabling it?

The code that added a box to widget-field.  But it seems that Stefan
suggested to disable it, and you only happened to concur.

Maybe this will help jog your memory (but you really ought to look at
the whole bug report.)

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Stefan Kangas <stefan <at> marxist.se>
>> Date: Wed, 3 Nov 2021 00:03:00 -0700
>> Cc: stephen.berman <at> gmx.net, 51550 <at> debbugs.gnu.org
>> 
>> It seems like the entire line is shifted a pixel to the left when using
>> :box, but only on the second consecutive line.  I tried experimenting
>> with :box 1, :box -1, and I get the same result.
>> 
>> Are we sure that's not a bug in the display code?
>
> I don't know, the arrangement of faces on that button is complicated,
> to say the least.  I don't think I even understand which parts of the
> display is the widget-field face applied to.  Can you show the
> complete definition of the button face at the first line and the
> non-first line?
>
>> In any case, we could revert the commit for emacs-28 and continue investigating
>> on master.
>
> Yes, I think we should do that.

Thanks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Tue, 28 Dec 2021 19:35:32 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  stephen.berman <at> gmx.net,
>   51550 <at> debbugs.gnu.org
> Date: Mon, 27 Dec 2021 18:50:22 +0800
> 
> I think I found the bug, it's in display_line.
> 
> When the label at_end_of_line is reached, it->start_of_box_run_p can be
> false.
> 
> Afterwards, set_iterator_to_next is called, which reseats the iterator
> to the next line, but doesn't set it->start_of_box_run_p if the face is
> now different from the previous face and also has a box.
> 
> I think the solution is to save the face ID of the iterator after the
> call to extend_face_to_end_of_line, then compare it to the face after
> the iterator is reseated to the next line, and set
> it->start_of_box_run_p to true if that face is different and also has a
> box.

I don't think this is right.  At the very least, it will change a
long-standing behavior: we don't set the start_of_box_run flag just
because we found a different face.  Instead, the entire run of
characters whose face(s) have the :box attribute set will appear as a
single box, with a single start at the beginning and a single end at
the end of the run.

Drawing a beginning of a new box each time we find a new face is
certainly not what's expected: the most striking example of that is
the mode line: it could have different strings with different faces,
but we want only one start_of_box_run and only one end_of_box_run_p:
at the very left and the very right, respectively.

Even if you suggest to limit this to a new face that happens to begin
at the start of a new line, I still don't think this is TRT: why is a
new line special for this purpose?  If the newline before it had the
:box attribute set in its face, the character after the newline is
just the continuation of the same run of characters that started on
the previous line.  We shouldn't handle this specially when the :box
attribute crosses a newline.

So I see no bug in how the display code works in this case.

> I have this code mostly working now, but I don't understand why
> set_iterator_to_next doesn't seem to be updating it->face_id.

It does update it->face_id, just not where you expected it to.  See
below.

> AFAIU, set_iterator_to_next called with RESEAT set to true eventually
> calls reseat, which in turn calls handle_face_prop through handle_stop
> if the iterator was reseated past the stop position, which should be set
> to the beginning of the next line when the face property at that next
> line differs from the line whose face is being extended.

No, set_iterator_to_next called with RESEAT non-zero only calls reseat
if it skips some text, e.g., due to invisibility.  If it just moves
from a newline to the first character of the next line, it doesn't
call reseat, because there's no need to do so: reseat is for when we
need to move the iterator non-linearly, or jump over several character
positions.

> However, handle_face_prop is never called, so I have to call it manually
> like this:
> 
> 	  /* Consume the line end.  This skips over invisible lines.  */
> 	  set_iterator_to_next (it, true);
> 
> -->	  handle_face_prop (it);

No, this is not needed.  handle_face_prop will be called once
display_line continues walking the buffer and processes the first
character of the new line.  At that time, it will notice that it
overstepped the stop position, and will call handle_stop, which will
call handle_face_prop, as expected.

You expected it to be called when processing the newline, but that is
too early.

But once again, I don't see anything that needs to be fixed in the
display engine.  Perhaps we need to rethink how the Lisp code sets up
the buttons as regards to the various faces or something.  IOW, let's
have a look at how the faces are set on the text in that case, and see
why we end up with two consecutive faces each one with a :box
attribute, because this has got to trigger this behavior, which you
find surprising and possibly incorrect.  The question is: can we
achieve whatever effect we wanted with the :box attributes without
losing the button appearance in the process by changing the Lisp code
there.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 29 Dec 2021 08:33:22 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> But once again, I don't see anything that needs to be fixed in the
> display engine.  Perhaps we need to rethink how the Lisp code sets up
> the buttons as regards to the various faces or something.  IOW, let's
> have a look at how the faces are set on the text in that case, and see
> why we end up with two consecutive faces each one with a :box
> attribute, because this has got to trigger this behavior, which you
> find surprising and possibly incorrect.  The question is: can we
> achieve whatever effect we wanted with the :box attributes without
> losing the button appearance in the process by changing the Lisp code
> there.

Thanks for the explanation, it was very helpful.  Maybe it would work to
put some kind of zero-width character with no face between the start of
the line and the INS buttons?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 29 Dec 2021 14:56:55 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: stefan <at> marxist.se,  stephen.berman <at> gmx.net,  51550 <at> debbugs.gnu.org
> Date: Wed, 29 Dec 2021 08:33:22 +0800
> 
> Thanks for the explanation, it was very helpful.  Maybe it would work to
> put some kind of zero-width character with no face between the start of
> the line and the INS buttons?

I think it might be easier to use invisible text, assuming it does the
job.  For example, try this slightly modified variant of your recipe,
to see what I mean:

 (insert #("foo\n foo " 0 4 (face widget-field) 4 5 (invisible t) 5 8 (face custom-button)))

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 29 Dec 2021 21:23:54 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> I think it might be easier to use invisible text, assuming it does the
> job.  For example, try this slightly modified variant of your recipe,
> to see what I mean:

>  (insert #("foo\n foo " 0 4 (face widget-field) 4 5 (invisible t) 5 8 (face custom-button)))

I found another potential problem: determining the start of a box run
doesn't take the before-string of an overlay into account.

wid-edit.el displays the button face through an overlay spanning from
the start to the end of the button, and I wanted to solve this problem
by adding a before-string containing a single invisible space, but that
doesn't seem to be affecting the non-display of the button's left box
line.

Any ideas?  Thanks in advance.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 29 Dec 2021 15:37:12 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: stefan <at> marxist.se,  stephen.berman <at> gmx.net,  51550 <at> debbugs.gnu.org
> Date: Wed, 29 Dec 2021 21:23:54 +0800
> 
> >  (insert #("foo\n foo " 0 4 (face widget-field) 4 5 (invisible t) 5 8 (face custom-button)))
> 
> I found another potential problem: determining the start of a box run
> doesn't take the before-string of an overlay into account.
> 
> wid-edit.el displays the button face through an overlay spanning from
> the start to the end of the button, and I wanted to solve this problem
> by adding a before-string containing a single invisible space, but that
> doesn't seem to be affecting the non-display of the button's left box
> line.

Why did you want to use a before-string? why not simply a single
invisible character before the button?  It sounds like an unnecessary
complication to me, since before-strings and after-strings are
inherently tricky, even when they aren't invisible.

But if you want me to look into this, you know what to do: show me a
short Lisp recipe which I could investigate.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 29 Dec 2021 21:54:38 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> Why did you want to use a before-string? why not simply a single
> invisible character before the button?

It has to be an overlay, because wid-insert might not be able to insert
such a character before the button.  (In general, the display of
everything in a button except for text seems to be done entirely with
overlays -- I don't think the buffer is supposed to contain anything
other than the text of the button itself.)

> It sounds like an unnecessary complication to me, since before-strings
> and after-strings are inherently tricky, even when they aren't
> invisible.

> But if you want me to look into this, you know what to do: show me a
> short Lisp recipe which I could investigate.

I hope this helps:

  (with-current-buffer (get-buffer-create "*test*")
    (insert #("foo\nfoo" 0 4 (face widget-field)))
    (let ((overlay (make-overlay 5 8 nil t nil)))
      (overlay-put overlay 'before-string (propertize " " 'invisible t))
      (overlay-put overlay 'face custom-button)))

After this, I would have expected the second "foo" in *test* to begin
with a box line, but it doesn't.

Thanks in advance.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Wed, 29 Dec 2021 16:56:52 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: stefan <at> marxist.se,  stephen.berman <at> gmx.net,  51550 <at> debbugs.gnu.org
> Date: Wed, 29 Dec 2021 21:54:38 +0800
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > Why did you want to use a before-string? why not simply a single
> > invisible character before the button?
> 
> It has to be an overlay, because wid-insert might not be able to insert
> such a character before the button.  (In general, the display of
> everything in a button except for text seems to be done entirely with
> overlays -- I don't think the buffer is supposed to contain anything
> other than the text of the button itself.)

So make an overlay whose property is 'invisible' -- doesn't that work?

What do you mean by "wid-insert might not be able to insert" -- isn't
the text of the button inserted as well? then why not make the
invisible character part of the inserted button text?

>   (with-current-buffer (get-buffer-create "*test*")
>     (insert #("foo\nfoo" 0 4 (face widget-field)))
>     (let ((overlay (make-overlay 5 8 nil t nil)))
>       (overlay-put overlay 'before-string (propertize " " 'invisible t))
>       (overlay-put overlay 'face custom-button)))
> 
> After this, I would have expected the second "foo" in *test* to begin
> with a box line, but it doesn't.

Will look into this soon.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Thu, 30 Dec 2021 09:11:58 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

> So make an overlay whose property is 'invisible' -- doesn't that work?

I will try, thanks.

> What do you mean by "wid-insert might not be able to insert" -- isn't
> the text of the button inserted as well? then why not make the
> invisible character part of the inserted button text?

It shouldn't insert anything except for the text of the button itself --
for one, the invisible character won't end up in the kill ring when you
try to save the button text.

There are also various assumptions made all over the place about the
position of a button and the length of the text in the buffer, and I'm
not confident I can fix them all.

>>   (with-current-buffer (get-buffer-create "*test*")
>>     (insert #("foo\nfoo" 0 4 (face widget-field)))
>>     (let ((overlay (make-overlay 5 8 nil t nil)))
>>       (overlay-put overlay 'before-string (propertize " " 'invisible t))
>>       (overlay-put overlay 'face custom-button)))
>> 
>> After this, I would have expected the second "foo" in *test* to begin
>> with a box line, but it doesn't.

> Will look into this soon.

Thanks.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: stephen.berman <at> gmx.net, stefan <at> marxist.se, 51550 <at> debbugs.gnu.org
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Thu, 30 Dec 2021 13:37:08 +0200

> From: Po Lu <luangruo <at> yahoo.com>
> Cc: stefan <at> marxist.se,  stephen.berman <at> gmx.net,  51550 <at> debbugs.gnu.org
> Date: Wed, 29 Dec 2021 21:54:38 +0800
> 
>   (with-current-buffer (get-buffer-create "*test*")
>     (insert #("foo\nfoo" 0 4 (face widget-field)))
>     (let ((overlay (make-overlay 5 8 nil t nil)))
>       (overlay-put overlay 'before-string (propertize " " 'invisible t))
>       (overlay-put overlay 'face custom-button)))
> 
> After this, I would have expected the second "foo" in *test* to begin
> with a box line, but it doesn't.

It was a subtle bug in how we handle face's box attribute when the
display engine changes the object on which it iterates.  Compare the
effect of this:

  (with-current-buffer (get-buffer-create "*test*")
    (load "wid-edit")
    (load "cus-edit")
    (insert #("foo\nfoo" 0 4 (face widget-field)))
    (let ((overlay (make-overlay 5 8 nil t nil)))
      (overlay-put overlay 'before-string "X")
      (overlay-put overlay 'face custom-button)))

with this:

  (with-current-buffer (get-buffer-create "*test*")
    (load "wid-edit")
    (load "cus-edit")
    (insert #("foo\nXfoo" 0 4 (face widget-field)))
    (let ((overlay (make-overlay 6 9 nil t nil)))
      (overlay-put overlay 'face custom-button)))

The change of face between the "X" and the following button should be
displayed the same, but wasn't.

I installed a fix on master.  Please see if it solves the real-life
use case, and close the bug if it does.

Thanks.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 51550-done <at> debbugs.gnu.org, stephen.berman <at> gmx.net, stefan <at> marxist.se
Subject: Re: bug#51550: 29.0.50; Customize Group INS buttons sometimes don't
 have a left box line
Date: Thu, 30 Dec 2021 19:43:44 +0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Po Lu <luangruo <at> yahoo.com>
>> Cc: stefan <at> marxist.se,  stephen.berman <at> gmx.net,  51550 <at> debbugs.gnu.org
>> Date: Wed, 29 Dec 2021 21:54:38 +0800
>> 
>>   (with-current-buffer (get-buffer-create "*test*")
>>     (insert #("foo\nfoo" 0 4 (face widget-field)))
>>     (let ((overlay (make-overlay 5 8 nil t nil)))
>>       (overlay-put overlay 'before-string (propertize " " 'invisible t))
>>       (overlay-put overlay 'face custom-button)))
>> 
>> After this, I would have expected the second "foo" in *test* to begin
>> with a box line, but it doesn't.
>
> It was a subtle bug in how we handle face's box attribute when the
> display engine changes the object on which it iterates.  Compare the
> effect of this:
>
>   (with-current-buffer (get-buffer-create "*test*")
>     (load "wid-edit")
>     (load "cus-edit")
>     (insert #("foo\nfoo" 0 4 (face widget-field)))
>     (let ((overlay (make-overlay 5 8 nil t nil)))
>       (overlay-put overlay 'before-string "X")
>       (overlay-put overlay 'face custom-button)))
>
> with this:
>
>   (with-current-buffer (get-buffer-create "*test*")
>     (load "wid-edit")
>     (load "cus-edit")
>     (insert #("foo\nXfoo" 0 4 (face widget-field)))
>     (let ((overlay (make-overlay 6 9 nil t nil)))
>       (overlay-put overlay 'face custom-button)))
>
> The change of face between the "X" and the following button should be
> displayed the same, but wasn't.
>
> I installed a fix on master.  Please see if it solves the real-life
> use case, and close the bug if it does.

Yes, that fixes the problem.  I'll push the corresponding fix to the
custom widget code, and I'm closing this bug.  Thanks.

Previous Next

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