Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Tue, 12 Mar 2019 20:20:17 +0100
> From: Ergus <address@hidden>
> Cc: address@hidden
> 
> Please tell me any problem you see related or not with my questions to
> fix them too. 

Thanks, see below.

> >> I tried:
> >>     if (!row->reversed_p)
> >>    {
> >>      while (glyph >= start
> >>             && (glyph->type == CHAR_GLYPH
> >>                 || glyph->type == STRETCH_GLYPH)
> >>             && NILP (glyph->object))
> >>        --glyph;
> >>    }
> >>
> >> But it didn't work. The behavior is the same: the whitespace is
> >> highlighted only when the line is crossed.
> >
> >I guess at this point I'd need to see the code to help you more.

The problem here is that the above loop also looks at the object from
which the glyph came, and will only skip glyphs whose object is nil.
But in your code, you didn't set up the object of the '|' character,
which comes from it->object, to be nil, you left it at its previous
value, which usually will be either a buffer or a string, depending
from where the last character of the text line came.

To fix this, add this:

  Lisp_Object save_object = it->object;
  it->object = Qnil;

before the call to PRODUCE_GLYPHS, and then restore it->object to its
previous value after PRODUCE_GLYPHS.

In the TTY case, this was already done for you by the code which
extends the face, that's why it worked on TTY frames.

> >> The other corner case I have is because in graphical mode the space for
> >> the dot is always reserved, so when the last character in the line is
> >> just before the line, the line is not drawn for that line.
> >
> >Sorry, I don't understand: what is "the dot" in this context, and what
> >do you mean by "just before the line"?  There are too many "lines" in
> >this sentence, so I'm confused.

I guess you mean that when the place for the cursor is at fill-column,
the indicator is not visible?  I think to fix this, you will need to
modify the code in the function append_space_for_newline, which is
responsible for adding the space glyph for displaying the cursor at
the end of the line.  That function currently adds a ' ' space
character there; you want to replace that with the indicator character
'|', when this mode is in effect.

A few more comments regarding the code:

> +      if (!NILP (Vdisplay_fill_column_indicator))
> +        {
> +       const int fill_column_indicator_column =
> +               XFIXNAT (Vdisplay_fill_column_indicator_column) - 1;

Why "- 1"?  It's confusing, and should at least be explained in a
comment, if not changed to not subtract 1.

Also, it is unsafe to call XFIXNAT without verifying the value is a
fixnum, because if it isn't, Emacs will crash.

> +       const int column_x = it->pixel_width * fill_column_indicator_column
> +                            + it->lnum_pixel_width;

Using it->pixel_width here will not work in general, as this is just
the pixel width of the last character or image of the screen line.  I
think you should use the font->average_width, and if that is zero,
then font->space_width.  Here 'font' is the default face's font.  This
is because I believe we want to display the indicator at the X
coordinate that corresponds to fill-column in units of the default
font width, right?  I mean, did you try to see what happens when the
text has faces which make some of the characters appear smaller or
larger than the default face?

> +           struct font *font = face->font ? face->font : FRAME_FONT (f);

I think this uses the wrong face: you should be using default_face
instead.  The above is the face of the last character of the line.

> +           it->char_to_display = '|';

This character should be customizable.  Perhaps make
display-fill-column-indicator serve double duty: if non-nil, it should
be the character to use as the indicator.

> +           it->face_id = merge_faces (it->w, Qline_number, 0, 
> DEFAULT_FACE_ID);

You didn't really mean to use the line-number face here, did you?
This should probably be a separate distinct face.

> +  DEFVAR_LISP ("display-fill-column-indicator", 
> Vdisplay_fill_column_indicator,
> +    doc: /* Non-nil means display the fill column indicator line.  */);

The "line" part of the doc string doesn't belong there, I think.  Just
"indicator" is enough.

> +  DEFVAR_LISP ("display-fill-column-indicator-column", 
> Vdisplay_fill_column_indicator_column,
> +    doc: /* Column where to draw the column indicator line when 
> display-column-indicator
> +is non-nil .  */);

The first line of a doc string should be a complete sentence, and it
should fit the 80column display line.  In this case, I'd shorten it
like this:

  Column to draw the indicator when `display-fill-column-indicator' is non-nil.

Please also say in the doc string what are the possible values and in
what column units they are measured.

Thanks for working on this.