Re: Fill column indicator functionality

[Ergus <spacibba@aol.com>]

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 <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> 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