Subject: Re: bug#77544: (WIP) [PATCH] Prettify page separators.

From: Eli Zaretskii <[email-masked]>
Date: Sat, 05 Apr 2025 10:40:25 +0300

> From: Elijah Gabe Pérez <[email-masked]>
> Date: Fri, 04 Apr 2025 23:01:02 -0600
> 
> This feature prettify the page separator character (^L) with
> a horizontal line (to end of buffer) only if the character
> is at beginning of line.
> 
> Unlike popular packages like page-break-lines and form-feed(-st), this
> one is written in C and does not present performance problems (I hope).
> 
> This package tries to be close to page-break-lines and be an alternative
> to make-separator-line.
> 
> This partially works, however there are a few bugs that I can't find a
> solution for:
> 
> - It won't display the characters inserted in front.
>   I want to make it similar to display-fill-column, that only
>   insert the horizontal line character if the row is empty.
> - For some reason it inserts newlines after inserting the character.
> - The highlighting stops when inserting or deleting newlines from
>   previous or next line.
> 
> I would like to hear your suggestions.

Thanks, please see a fewe comments below.

> --- a/src/xdisp.c
> +++ b/src/xdisp.c
> @@ -8430,6 +8430,34 @@ get_next_display_element (struct it *it)
>  	      int lface_id = 0;
>  	      int escape_glyph;
>  
> +	      if (c == '\f' && it->current_x == it->first_visible_x)

IMO, this condition is incorrect.  You should test for the previous
character being a newline.  The it->first_visible_x condition will be
wrong in a continuation line, when the window is hscrolled, and when
display-line-numbers-mode is turned on, as well as in some other
situations (line/wrap-prefix etc.).

> +		{
> +		  const int save_face_id = it->face_id;
> +		  const int save_char = it->char_to_display;
> +		  const int save_current_x = it->current_x;
> +		  const int save_ascent = it->ascent;
> +		  const int save_descent = it->descent;
> +
> +		  face_id =
> +		    merge_faces (it->w, Qtrailing_whitespace, 0, it->face_id);
> +		  XSETINT (it->ctl_chars[0], 0);
> +		  ctl_len = 0;
> +
> +		  it->char_to_display = '-';
> +		  it->face_id = face_id;
> +		  for ( ; it->current_x - it->end_charpos <= it->last_visible_x; it->current_x++)

This condition is also incorrect, IMO: it->current_x is the pixel
coordinate, whereas it->end_charpos is a character position in the
buffer, so you cannot meaningfully subtract them.  Also, I think this
will be incorrect when the window has display margins.

The right way to figure out the usable width is to call the function
window_box_width or window_box_right_offset.

These wrong conditions are probably the reasons for at least part of
the problems you see with this patch.

> +		    {
> +		      PRODUCE_GLYPHS (it);
> +		    }

I think it's not a good idea to call PRODUCE_GLYPHS inside
get_next_display_element.  And in any case, this implementation is
unnecessarily complicated, because we should probably use the display
vector instead.  The only complication here is that the number of
glyphs in the display vector cannot be the one you need, since we need
to produce the number of glyphs according to the window width.  So we
will need some special flag in the iterator object to keep producing
glyphs from the display vector until we hit the right edge of the
window's text area.

Alternatively, we could implement this entirely in the
terminal-specific back-end of the display engine (xterm.c, w32term.c,
etc.), where we could detect the "^L" glyph sequence and replace it
with a horizontal line of a suitable width and thickness.  That would
allow us to produce a separator that is much better-looking than just
showing a long line of dashes.

In any case, this must be an optional feature, so we will need a user
option to control it, by default off.

Adding Gerd in case he has ideas, suggestions, or further comments.

Thanks for working on this.