Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Thu, 14 Mar 2019 04:02:24 +0100
> From: Ergus <address@hidden>
> Cc: address@hidden
> 
> 1) In the graphical mode the character I use now (?\u2502) does not look
> like a vertical line (there is a small vertical space between them so
> the line now looks like with ?|); in terminal it looks fine.

It looks fine with the default font I use here in GUI frames: no
vertical space between lines.  So this obviously depends on the font
used for the indicator character.

> What's the proper way to change the font size or the character
> itself to fit the entire row height in the graphical mode?

I don't recommend changing the size, because that will change the
height of the entire screen line, and will have the unpleasant side
effect of changing the vertical spacing of those lines that don't
exceed the fill-column, while lines that do exceed it will have the
original vertical spacing.  Plus, it won't make the vertical space
between the indicator characters disappear.

Instead, I suggest to change the font used for that character, if your
default font has this problem.

The way to change both the size and the font of the character is by
customizing the face used for the indicator.  And since you already
let users customize that face, you can leave this issue alone, and
rely on users to find a suitable font.

> 2) Are there any extra considerations needed to use the character
> ?\u2502 ? Is it needed to check (for example) if the actual fonts can
> display it properly?

If the user customized the face, you don't need to make sure the font
can display it: it's up to the user to do so.  For the default, you
could perhaps check in the function which turns on this mode that
U+2502 is displayable, and if not, fall back on the ASCII '|'.  See
char-displayable-p.  There's also font-get-glyphs, if you need a more
accurate test (but I don't think you do).

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

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

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

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

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

> -      while (it->current_x <= it->last_visible_x)
> -     PRODUCE_GLYPHS (it);
> +      /* Display fill-column-line if mode is active */
> +      if (!NILP (Vdisplay_fill_column_indicator))
> +     {
> +       const int fill_column_indicator_line =
> +         XFIXNAT (Vdisplay_fill_column_indicator_column) + 
> it->lnum_pixel_width;
> +       do
> +         {
> +           if (it->current_x == fill_column_indicator_line)
> +             {
> +               const int saved_face = it->face_id;
> +               it->face_id = merge_faces (it->w, Qfill_column, 0, 
> DEFAULT_FACE_ID);
> +               it->c = it->char_to_display = 
> XFIXNAT(Vdisplay_fill_column_indicator_character);
> +               PRODUCE_GLYPHS (it);
> +               it->face_id = saved_face;
> +               it->c = it->char_to_display = ' ';
> +             }
> +           else
> +             PRODUCE_GLYPHS (it);
> +         } while (it->current_x < it->last_visible_x);
> +     }
> +      else
> +     {
> +          do
> +         {
> +           PRODUCE_GLYPHS (it);
> +            } while (it->current_x < it->last_visible_x);
> +        }

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?

> +  /* 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.  */
                                                                       ^^^

> +  DEFVAR_LISP ("display-fill-column-indicator-character", 
> Vdisplay_fill_column_indicator_character,
> +    doc: /* Character to draw the indicator when 
> `display-fill-column-indicator' is non-nil.
> +The default is ?\u2502 the but a good alternative is (ascii 124) if
> +yours font doesn't support unicode characters.  */);

We use the U+NNNN notation in documentation and comments, not ?\uNNNN.
Also, please capitalize "Unicode".  And "yours font" should be "your
font" (I'd actually reword to refer to the font specified by the
fill-column face).

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.