Re: Fill column indicator functionality

[Ergus]

Hi Eli:

On Wed, Mar 13, 2019 at 06:19:40PM +0200, Eli Zaretskii wrote:
> > 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.
I thought that append_stretch_glyph was putting the it->object to Now I
see it didn't.

> 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.
Yes I was referring to cursor as point. My bad.

> 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.
The -1 is related with the extra ' ' added by the extend_face function,
I will try to avoid it, in the final patch, but now this is just a prove
of concept.

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

Ok, I'll prevent this.

> 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);
You just saved me a lot of time of reading the font and face structs
definitions (I will but not now).

> 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 is customizable actually in my current version, I just sent you the
basic changes to make it work that exposed the issues with minimal code
changes. In fact the default value will be ?\u2402 (and not ?| as in the
example) if the font supports it because it will look like a contiguous
line.

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

Yes, I did, this was a prove of concept. I created a fill_column 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.
Ok, perfect

> Thanks for working on this.

1E6 Thanks.
Ergus