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

From: Elijah Gabe Pérez <[email-masked]>
Date: Sat, 05 Apr 2025 11:57:42 -0600

Eli Zaretskii <[email-masked]> writes:

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

I'll take it into account.  Thanks.

>> +		    {
>> +		      PRODUCE_GLYPHS (it);
>> +		    }
>
> I think it's not a good idea to call PRODUCE_GLYPHS inside
> get_next_display_element.

I originally wanted to put this in display_line, however,
get_next_display_element was overriding the character face so i thought
that adding it to get_next_display_element (where ^L is displayed)
would be a better solution.

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

The dashes are temporal, I'm planning to add a variable that store
the character to display (like display-fill-column-indicator-character).

I don't think it would be better display a line instead a character, in
that case, I would have preferred to use a face with a strike-through.

This should work for both Terminal and GUI frames.

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

Yeah, this will be buffer-local and have an user option (not minor mode)
for enable/disable it (disabled by default), i didn't added the options
yet because first i was testing if this works well, the same for the
face used.

-- 
                                          - E.G via GNU Emacs and Org.