Re: Fill column indicator functionality

[Ergus]

On Thu, Mar 14, 2019 at 08:40:27AM +0200, Eli Zaretskii wrote:

> > 3) Actually there are 3 possible configuration options the column
> > number, the character, and the font. Should we provide some other
> > alternative?
>
> See above: since you already provide a special face, the font
> customization is already covered and doesn't need a separate knob.
Sorry, I wanted to put face not fonts in fact

> > The next step is to implement the R2L version of this, but I don't have
> > used R2L editing ever, so it may take a while.
>
> Are you sure you need to do anything at all for supporting R2L?  Both
> PRODUCE_GLYPHS and append_stretch_glyph DTRT transparently with R2L
> screen lines, so I think you don't need anything else.  You can try
> using TUTORIAL.he as your playing ground; I don't expect you to see
> any problems with this feature in R2L lines.
Now I tried and it works, and passed my tests. 

> A couple of comments to the patch you posted:
>
> > +         int local_default_face_id = lookup_basic_face (it->w, it->f, 
> > DEFAULT_FACE_ID);
>
> Please keep the source lines shorter than 78 columns; longer lines
> should be broken into several shorter lines (here and elsewhere in
> your patch).

That's why I needed to implement this functionality ;).

> > -                the end of the row, there will be no stretch glyph,
> > -                so leave the box flag set.  */
> > +                the end of the row, there will be no stretch glyph,
> > +                so leave the box flag set.  */
> >               && saved_x + FRAME_COLUMN_WIDTH (it->f) < it->last_visible_x)
> > -           it->end_of_box_run_p = false;
> > +                 it->end_of_box_run_p = false;
>
> This part seems to change only the whitespace, and I don't see any
> reason for doing that here.
Fixed
> > +             it->char_to_display = 
> > XFIXNAT(Vdisplay_fill_column_indicator_character);
>
> Please leave one space between the macro name and the opening
> parenthesis that follows it.
>
>
> Here you replaced a while loop with a do-while loop, which means you
> always produce at least one glyph, where the original code might not
> have produced any glyphs.  Did you verify this cannot give us any trouble?
> Can this code be entered with it->current_x == it->last_visible_x?
In the original code there was a PRODUCE_GLYPHS (it); just before the
while loop, so at least one was always produced, that's why I switched
to a do loop. Checking the patch, a potential issue may come from the
fact that I changes the <= for a < and actually there was a missing
GLYPH at the very end. I fixed that now.

> > +  /* Names of the faces used to display fill column indicator character. */
> > +  DEFSYM (Qfill_column, "fill-column");
>
> "Name of the face", in singular, right?
>
> Also, please keep 2 spaces after the last sentence of a comment, like
> this:
>
>  /* Names of the faces used to display fill column indicator character.  */
>                                                                       ^^^
>
> Thanks.
>
> P.S. Btw, there's no need to post diffs for ldefs-boot.el, that file
> is regenerated and committed automatically from time to time, courtesy
> of Glenn's scripts.

Good to know, then why it is not in the gitignore as loaddefs.el?

More questions:

There is DEFVAR_INT and DEFVAR_BOOL. Does it makes sense to use those to
define display-fill-column-indicator-column and
display-fill-column-indicator; instead of DEFVAR_LISP??

Should we add some code in order to assert that if the user changes
fill-column and it is equal to display-fill-column-indicator-column;
then the second one should be updated too? Because
display-fill-column-indicator-column must be == fill-column except if the
users explicitly sets it to a different value.

Finally, if you think that there is something else missing to consider
this as ready, please tell me, else, tell me how to do a pull request.

Maybe in the future I will consider the alternative to change the
background color, but I don't want to make this too complex or add more
overhead to the display engine.

Best
Ergus

display-fill-column-indicator.patch
Description: Text document