Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Wed, 1 May 2019 13:08:08 +0200
> From: Ergus <address@hidden>
> Cc: address@hidden
> 
> Should we consider to add this functionality already to the master branch??

It needs a few final touches, and then it can land.  See my comments
below:

> +initialization tries to set it to U+2502 or '|'.

Please use @samp{|} instead of literal quotes.

> address@hidden fill-column-face

This face should be indexed, use @vindex for that.

> +Specifies the face used to display the indicator it inherits its
> +default values from shadow and the default face.

These are two sentences, not one.  Please place a period after
"indicator".

> +  Emacs can display an indicator in the @code{fill-column} position
> +using the Display fill column indicator mode 

The mode should be capitalized: "Display Fill Column Indicator mode".

> +---

This should be "+++", to indicate that the manuals have been updated.
"---" means there's no need to document the described feature in the
manuals.

> +There are 2 new buffer local variables and 1 face to customize this
> +mode:
> +
> +*** 'display-fill-column-indicator-column' is the column where the
> +    indicator should be set. It can take positive numerical values for
> +    the column or the special value t. Any other value disables the
> +    indicator. The default value is t.
> +    
> +    When the value is t it means that the variable 'fill-column' will
> +    be used.
> +
> +*** 'display-fill-column-indicator-char' is the character used for the
> +    indicator. This character can be any valid char including unicode
> +    ones if the user's font supports them.
> +    
> +    When the mode is enabled throw the functions
> +    'display-fill-column-indicator-mode' and
> +    'global-display-fill-column-indicator-mode', they check if there
> +    is a value non-nil for this variable, otherwise the initialization
> +    tries to set it to U+2502 or '|'.
> +    
> +*** 'fill-column-face' is the face used to display the indicator it
> +    inherits it default values from shadow and the default faces.

No need to use "***" here.  In fact, I'm not sure we should describe
these new variables in NEWS, just point to the manual instead.

> +;; Copyright (C) 2017-2019 Free Software Foundation, Inc.

This is copy-paste; it should be just 2019.

> +(define-minor-mode display-fill-column-indicator-mode
> +  "Toggle display fill column indicator.
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"Toggle display of fill-column indicator.".

> +To change the position of the column displayed by default,
> +customize `display-fill-column-indicator-column' you can change the
> +character for the indicator setting 
> `display-fill-column-indicator-character'."

There are 2 sentences here, not one.

> +;; Definition stolen from display-line-numbers.
> +(defface fill-column-face
> +  '((t :inherit (shadow default)))
> +  "Face for displaying fill column indicator line.
                                                ^^^^
I'd drop the "line" part.  You allude to the shape of the indicator
character, but it doesn't have to look like a line, and using "line"
is confusing here.

> +If you customize the font of this face, make sure it is a
> +monospaced font, otherwise the line's characters will not line
> +up horizontally."

Is this true for the indicator?  I think this comes from the
line-number face, but I'm not sure it is applicable to the fill-column
indicator.

> diff --git a/lisp/ldefs-boot.el b/lisp/ldefs-boot.el
> index cb378ce..45c72ff 100644
> --- a/lisp/ldefs-boot.el
> +++ b/lisp/ldefs-boot.el

You don't need to update ldefs-boot.el, it is done automatically by
Glenn's script.

> +       /* Corner case for when display-fill-column-indicator-mode
> +          is active and the extra character should be added in the
> +          same place than the line */

Here and elsewhere, please use our conventions for writing comments:
they should start with a capital letter and end with a period and 2
spaces before the "*/" comment terminator.

> +            if (EQ (Vdisplay_fill_column_indicator_column, Qt)
> +                && FIXNATP (BVAR (current_buffer, fill_column)))
> +              fill_column_indicator_column =
> +                XFIXNAT (BVAR (current_buffer, fill_column));
> +            else if (FIXNATP (Vdisplay_fill_column_indicator_column))
> +              fill_column_indicator_column =
> +                XFIXNAT (Vdisplay_fill_column_indicator_column);

There's no 'else' clause here.  What will happen if neither of these
two conditions holds?  What do we want to happen then?

> @@ -20163,11 +20215,12 @@ append_space_for_newline (struct it *it, bool 
> default_face_p)
>            set.  */
>         if (it->glyph_row->reversed_p
>             /* But if the appended newline glyph goes all the way to
> -              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.  */

Please don't convert TABs to spaces in C sources.

> +            if (EQ (Vdisplay_fill_column_indicator_column, Qt)
> +                && FIXNATP (BVAR (current_buffer, fill_column)))
> +              fill_column_indicator_column =
> +                XFIXNAT (BVAR (current_buffer, fill_column));
> +            else if (FIXNATP (Vdisplay_fill_column_indicator_column))
> +              fill_column_indicator_column =
> +                XFIXNAT (Vdisplay_fill_column_indicator_column);

Same here: what if neither of the 2 conditions holds?

> +               /* Only generate a stretch glysph if there is distance
                                             ^^^^^^
A typo.

> +               /* Generate the glysph indicator only if
                                  ^^^^^^
And here.  Also, I think you mean "indicator glyph", i.e. transpose
the words.

> +               /* If there is space after the indicator generate an
                                                          ^
Comma missing there.

> +                  extra empty glysph to restore the face. */
                                 ^^^^^^
Another typo.

I think this problem was only on X?  If so, I suggest to say that in
the comment.

>  #ifdef HAVE_WINDOW_SYSTEM
> +
>        if (it->glyph_row->reversed_p)

IMO, this extra line actually makes the source less readable, not
more.

> +      /* Display fill-column-line if needed.  */

You mean "Display fill-column-line indicator if needed.", yes?

> +       /* Vdisplay_fill_column_indicator_column accepts the special
> +          value t to use the default fill-column variable.  */
> +       if (EQ (Vdisplay_fill_column_indicator_column, Qt)
> +           && FIXNATP (BVAR (current_buffer, fill_column)))
> +         fill_column_indicator_column =
> +           XFIXNAT (BVAR (current_buffer, fill_column)) + 
> it->lnum_pixel_width;
> +       else if (FIXNATP (Vdisplay_fill_column_indicator_column))
> +         fill_column_indicator_column =
> +           XFIXNAT (Vdisplay_fill_column_indicator_column) + 
> it->lnum_pixel_width;

Once again: no 'else'.

> +  /* Names of the face used to display fill column indicator character.  */
        ^^^^^
"Name", in singular.  And I would lose the "character" part, as it
might not be a character, at least in the future.

> +  DEFVAR_LISP ("display-fill-column-indicator-column", 
> Vdisplay_fill_column_indicator_column,
> +    doc: /* Column for indicator when `display-fill-column-indicator' is 
> non-nil.
> +The default value is t which means that the indicator will use the 
> `fill-column' variable.

The last line is too long, please break it into 2.

> +If a numeric value is set, the indicator will be drawn in that column
   ^^^^^^^^^^^^^^^^^^^^^^^^^^
"If a number, ..."

Btw, "number" is not accurate, as it must be an integer, right?  It
cannot be a float.

> +  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 U+2502 but a good alternative is (ascii 124) if
> +the font in fill-column-face does not support Unicode characters.  */);

Please quote fill-column-face `like this', so it will generate a link
in the *Help* buffer.

Finally, there are several places where you have left trailing
whitespace, please remove that.  Also, sometimes you left only 1 space
between sentences, whereas our convention is to leave 2.

Thanks!