bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 258 bytes --]

Quite easy to see with:

(setq display-line-numbers-major-tick 5)
(set-face-attribute 'line-number-major-tick nil :foreground "white"
:background "black")

and then just enable line numbers in *Scratch* and insert a few lines at
the end.

[image: image.png]

[-- Attachment #1.2: Type: text/html, Size: 452 bytes --]

[-- Attachment #2: image.png --]
[-- Type: image/png, Size: 5658 bytes --]

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Juanma Barranquero]

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

This patch seems to fix the problem.

diff --git i/src/xdisp.c w/src/xdisp.c
index 563cf473cf..a2d605f3ea 100644
--- i/src/xdisp.c
+++ w/src/xdisp.c
@@ -22679,9 +22679,11 @@ maybe_produce_line_number (struct it *it)
   && it->what != IT_EOB)
  tem_it.face_id = current_lnum_face_id;
-      else if (display_line_numbers_major_tick > 0
+      else if (!beyond_zv
+       && display_line_numbers_major_tick > 0
        && (lnum_to_display % display_line_numbers_major_tick == 0))
  tem_it.face_id = merge_faces (it->w, Qline_number_major_tick,
       0, DEFAULT_FACE_ID);
-      else if (display_line_numbers_minor_tick > 0
+      else if (!beyond_zv
+       && display_line_numbers_minor_tick > 0
        && (lnum_to_display % display_line_numbers_minor_tick == 0))
  tem_it.face_id = merge_faces (it->w, Qline_number_minor_tick,

[-- Attachment #2: Type: text/html, Size: 1039 bytes --]

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Mon, 7 Oct 2019 05:42:46 +0200
> 
> This patch seems to fix the problem.

Looks good, but please simplify by having 2-level if, the outer one
checking when to display a number, the inner which face to use for
that.  There's no need to test beyond_zv more than once, and AFAIR
beyond_zv and it->what == IT_EOB are equivalent.

Thanks.

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 2975 bytes --]

On Mon, Oct 7, 2019 at 6:17 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Looks good, but please simplify by having 2-level if, the outer one
> checking when to display a number, the inner which face to use for
> that.  There's no need to test beyond_zv more than once, and AFAIR
> beyond_zv and it->what == IT_EOB are equivalent.

beyond_zv and it->what == IT_EOB are similar, but not equivalent. I see the
difference when deleting empty lines at the end of the buffer
(specifically, when deleting from the last line, and not past the last
line).

With the `what' check, the face still bleed info the first post-EOB line.
Checking beyond_zv it works (see attached images).

As for simplifying the check, it is possible to check beyond_zv only once,
with the minor downside of having two paths to set lnum_face (which works
as the default face, and the face post-EOB).

if (check for current line)  // checks also it->what != IT_EOB
   set face to current_lnum_face;
else if (beyond_zv)
   set face to lnum_face;  // 1
else if (check for major tick)
   set face to major_tick;
else if (check for minor tick)
   set face to minor_tick;
else
   set face to lnum_face;  // 2

BTW, if the "it->what != IT_EOB" comparison in the current line check can
indeed be changed to !beyond_zv (which seems to work, at least on my
tests), then you can go to

if (beyond_zv)
   set face to lnum_face;  // 1
else if (check for current line)  // does not check it->what != IT_EOB
   set face to current_lnum_face;
else if (check for major tick)
   set face to major_tick;
else if (check for minor tick)
   set face to minor_tick;
else
   set face to lnum_face; // 2

which is equally clean and saves another comparison.

And the nicest thing is that the patch is very clean (attached also because
Gmail as usual is screwing with it).

diff --git a/src/xdisp.c b/src/xdisp.c
index 29d49d57df..ad73981c1d 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -22662,13 +22662,14 @@ maybe_produce_line_number (struct it *it)
     {
       /* For continuation lines and lines after ZV, instead of a line
         number, produce a blank prefix of the same width.  */
-      if (lnum_face_id != current_lnum_face_id
-         && (EQ (Vdisplay_line_numbers, Qvisual)
-             ? this_line == 0
-             : this_line == it->pt_lnum)
-         /* Avoid displaying the line-number-current-line face on
-            empty lines beyond EOB.  */
-         && it->what != IT_EOB)
+      if (beyond_zv)
+       /* Avoid displaying any face other than line-number on
+          empty lines beyond EOB.  */
+       tem_it.face_id = lnum_face_id;
+      else if (lnum_face_id != current_lnum_face_id
+              && (EQ (Vdisplay_line_numbers, Qvisual)
+                  ? this_line == 0
+                  : this_line == it->pt_lnum))
        tem_it.face_id = current_lnum_face_id;
       else if (display_line_numbers_major_tick > 0
               && (lnum_to_display % display_line_numbers_major_tick == 0))

[-- Attachment #1.2: Type: text/html, Size: 3644 bytes --]

[-- Attachment #2: it_eob.png --]
[-- Type: image/png, Size: 4796 bytes --]

[-- Attachment #3: beyond_zv.png --]
[-- Type: image/png, Size: 5327 bytes --]

[-- Attachment #4: beyond.patch --]
[-- Type: application/octet-stream, Size: 1058 bytes --]

diff --git a/src/xdisp.c b/src/xdisp.c
index 29d49d57df..ad73981c1d 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -22662,13 +22662,14 @@ maybe_produce_line_number (struct it *it)
     {
       /* For continuation lines and lines after ZV, instead of a line
 	 number, produce a blank prefix of the same width.  */
-      if (lnum_face_id != current_lnum_face_id
-	  && (EQ (Vdisplay_line_numbers, Qvisual)
-	      ? this_line == 0
-	      : this_line == it->pt_lnum)
-	  /* Avoid displaying the line-number-current-line face on
-	     empty lines beyond EOB.  */
-	  && it->what != IT_EOB)
+      if (beyond_zv)
+	/* Avoid displaying any face other than line-number on
+	   empty lines beyond EOB.  */
+	tem_it.face_id = lnum_face_id;
+      else if (lnum_face_id != current_lnum_face_id
+	       && (EQ (Vdisplay_line_numbers, Qvisual)
+		   ? this_line == 0
+		   : this_line == it->pt_lnum))
 	tem_it.face_id = current_lnum_face_id;
       else if (display_line_numbers_major_tick > 0
 	       && (lnum_to_display % display_line_numbers_major_tick == 0))

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Juanma Barranquero]

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

As an aside, if it is safe to assume that the faces are not going to change
between digits of the line number, perhaps this change would be a gain.

It moves merging the tick faces outside the lnum_buf loop, and it is done
only for the right line numbers and when not beyond_zv.

diff --git i/src/xdisp.c w/src/xdisp.c
index ad73981c1d..2d79a42270 100644
--- i/src/xdisp.c
+++ w/src/xdisp.c
@@ -22657,6 +22657,21 @@ maybe_produce_line_number (struct it *it)
   int width_limit =
     tem_it.last_visible_x - tem_it.first_visible_x
     - 3 * FRAME_COLUMN_WIDTH (it->f);
+
+  /* Select face for tick line numbers, if needed */
+  int tick_face_id = -1;
+  if (!beyond_zv)
+    {
+      if (display_line_numbers_major_tick > 0
+     && (lnum_to_display % display_line_numbers_major_tick == 0))
+   tick_face_id = merge_faces (it->w, Qline_number_major_tick,
+                   0, DEFAULT_FACE_ID);
+      else if (display_line_numbers_minor_tick > 0
+          && (lnum_to_display % display_line_numbers_minor_tick == 0))
+   tick_face_id = merge_faces (it->w, Qline_number_minor_tick,
+                   0, DEFAULT_FACE_ID);
+    }
+
   /* Produce glyphs for the line number in a scratch glyph_row.  */
   for (const char *p = lnum_buf; *p; p++)
     {
@@ -22671,14 +22686,8 @@ maybe_produce_line_number (struct it *it)
           ? this_line == 0
           : this_line == it->pt_lnum))
    tem_it.face_id = current_lnum_face_id;
-      else if (display_line_numbers_major_tick > 0
-          && (lnum_to_display % display_line_numbers_major_tick == 0))
-   tem_it.face_id = merge_faces (it->w, Qline_number_major_tick,
-                     0, DEFAULT_FACE_ID);
-      else if (display_line_numbers_minor_tick > 0
-          && (lnum_to_display % display_line_numbers_minor_tick == 0))
-   tem_it.face_id = merge_faces (it->w, Qline_number_minor_tick,
-                     0, DEFAULT_FACE_ID);
+      else if (tick_face_id >= 0)
+   tem_it.face_id = tick_face_id;
       else
    tem_it.face_id = lnum_face_id;
       if (beyond_zv

[-- Attachment #2: Type: text/html, Size: 2424 bytes --]

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 8 Oct 2019 06:23:36 +0200
> Cc: 37641@debbugs.gnu.org
> 
> As an aside, if it is safe to assume that the faces are not going to change between digits of the line number,
> perhaps this change would be a gain.

What does "between digits" mean?  While producing the glyphs for the
digits of a single line number, no faces can change, because no Lisp
is invoked.  Is that what you meant?

> It moves merging the tick faces outside the lnum_buf loop, and it is done only for the right line numbers and
> when not beyond_zv.

Yes, moving code out of the loop is a good idea.

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 8 Oct 2019 04:38:18 +0200
> Cc: 37641@debbugs.gnu.org
> 
> > Looks good, but please simplify by having 2-level if, the outer one
> > checking when to display a number, the inner which face to use for
> > that.  There's no need to test beyond_zv more than once, and AFAIR
> > beyond_zv and it->what == IT_EOB are equivalent.
> 
> beyond_zv and it->what == IT_EOB are similar, but not equivalent. I see the difference when deleting empty
> lines at the end of the buffer (specifically, when deleting from the last line, and not past the last line).
> 
> With the `what' check, the face still bleed info the first post-EOB line. Checking beyond_zv it works (see
> attached images).

Sorry, I don't think I understand what the images show me.  They seem
identical.  Which face bleeds and where?  Please point out what should
I be looking at to understand the difference.

Did you try to arrange for the last line to be a multiple of one of
the ticks as well?  Also, what happens if you use the beyond_zv test
in all the conditions, or use the it->what test in all the conditions?

IOW, I don't understand why we need two different conditions regarding
EOB for displaying a number with different faces.  What am I missing?

> As for simplifying the check, it is possible to check beyond_zv only once, with the minor downside of having
> two paths to set lnum_face (which works as the default face, and the face post-EOB).

You could simply start with

  tem_it.face_id = lnum_face_id;

and then have a series of tests for replacing that with another face
ID; it would at least save you the 'else' clause.

Thanks.

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Juanma Barranquero]

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

  On Tue, Oct 8, 2019 at 10:50 AM Eli Zaretskii <eliz@gnu.org> wrote:

> Sorry, I don't think I understand what the images show me.  They seem
> identical.  Which face bleeds and where?  Please point out what should
> I be looking at to understand the difference.

In both images, the line numbered 19 is the last line in the buffer.

In the it_eob image, the "line" past the end of the buffer has an empty
number,
drawn with the major-tick face (but obviously without number). The rest of
non-lines,
until the end of the buffer, have their empty number drawn in the
line-number face.

In the beyond_zv image, the empty line after the end of the buffer is in the
line-number face, as all the other past that point. I think that's the
right behavior.

There's no reason for the line after the end of the buffer to be drawn with
the
major-tick face, even if it would be a major-tick line if it existed. It's
ugly.

> Did you try to arrange for the last line to be a multiple of one of
> the ticks as well?

In my examples? Yes, that's the whole point of the test: knowing what
happens when
the line after EOB would match a tick line number.

>  Also, what happens if you use the beyond_zv test
> in all the conditions

That's what I've done in the second patch I sent (applied after the first
one, not
alone). In my simple tests, everything works as expected.

> or use the it->what test in all the conditions?

I didn't try that, but as the first check (that uses it->what) is trying to
decide
whether to draw with the current-line-number, I don't think it is relevant
to
the problem I was trying to fix. It is relevant for consistency, of course.

> IOW, I don't understand why we need two different conditions regarding
> EOB for displaying a number with different faces.  What am I missing?

I don't know why (or if) the it->what check is necessary, instead of
beyond_zv.
I *know* that the other conditions (the ones affecting the choosing of tick
faces)
need beyond_zv, at least to get what I think is the right behavior (the one
in the
beyond_zv image). As said, I think that in fact it the first check can be
safely
replaced by !beyond_zv.

> You could simply start with
>
>   tem_it.face_id = lnum_face_id;
>
> and then have a series of tests for replacing that with another face
> ID; it would at least save you the 'else' clause.

Yes, good idea. Thanks.

> What does "between digits" mean?  While producing the glyphs for the
> digits of a single line number, no faces can change, because no Lisp
> is invoked.  Is that what you meant?

Yes, thanks. I expected as much, but the way the original code was written,
and
redisplay being notoriously tricky and an arcane art best left to wizards, I
though I was missing something (I'm not joking).

[-- Attachment #2: Type: text/html, Size: 3159 bytes --]

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 789 bytes --]

Turns out there's a difference after all. See the attached images. If you
move the
cursor past the EOB, with the beyond_zv check the line is drawn with the
line-number
face, not the line-number-current-line face. If we check with it->what !=
IT_EOB,
then the line-number-current-line face is used.

I think the right thing to do, when dealing with the "phantom line" just
past the EOB,
is to:
- Use line-number-current-line, if the cursor is there.
- Use the line-number face, and not the tick faces, if the cursor is not,
regardless
  of whether this phantom line would be a tick line or not if it were used.

So there's a patch that checks for tick faces with beyond_zv, for the
current line
face with it->what, and that moves all face setting for line numbers
entirely out of
the loop.

[-- Attachment #1.2: Type: text/html, Size: 924 bytes --]

[-- Attachment #2: bug-37641.patch --]
[-- Type: application/x-patch, Size: 2277 bytes --]

[-- Attachment #3: it_eob.png --]
[-- Type: image/png, Size: 4796 bytes --]

[-- Attachment #4: beyond_zv.png --]
[-- Type: image/png, Size: 5327 bytes --]

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Tue, 8 Oct 2019 12:47:22 +0200
> Cc: 37641@debbugs.gnu.org
> 
> Turns out there's a difference after all. See the attached images. If you move the
> cursor past the EOB, with the beyond_zv check the line is drawn with the line-number
> face, not the line-number-current-line face. If we check with it->what != IT_EOB,
> then the line-number-current-line face is used.
> 
> I think the right thing to do, when dealing with the "phantom line" just past the EOB,
> is to:
> - Use line-number-current-line, if the cursor is there.
> - Use the line-number face, and not the tick faces, if the cursor is not, regardless
>   of whether this phantom line would be a tick line or not if it were used.

Agreed.

> So there's a patch that checks for tick faces with beyond_zv, for the current line
> face with it->what, and that moves all face setting for line numbers entirely out of
> the loop.

LGTM, thanks.

bug#37641: major/minor tick faces bleed into empty lines at the end of buffer

[Juanma Barranquero]

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

Closing. Fixed by this commit:

commit 4b06250ef1fe98a766938862912383d2ee051dfb
 Author: Juanma Barranquero <lekktu@gmail.com>
 Date:   2019-10-09 12:36:57 +0200

     Do not use tick faces beyond ZV (bug#37641)

     * src/xdisp.c (maybe_produce_line_number): Check beyond_zv
     before using a tick face for the line number.  Move all face
     selection code outside the loop that draws the line number.

[-- Attachment #2: Type: text/html, Size: 546 bytes --]