Re: Fill column indicator functionality

[Ergus <spacibba@aol.com>]

[-- Attachment #1: Type: text/plain, Size: 3981 bytes --]

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

[-- Attachment #2: display-fill-column-indicator.patch --]
[-- Type: text/plain, Size: 16359 bytes --]

diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index baa05d0a89..3f58eac63b 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -648,6 +648,11 @@ since it could result in memory overflow and make Emacs crash."
                                   (const :tag "Count lines from beginning of narrowed region"
                                          :value nil))
                                  "26.1")
+
+             (display-fill-column-indicator-column display-fill-column-indicator
+                                 integer "27.1")
+             (display-fill-column-indicator-character display-fill-column-indicator
+                                 character "27.1")
 	     ;; xfaces.c
 	     (scalable-fonts-allowed display boolean "22.1")
 	     ;; xfns.c
diff --git a/lisp/display-fill-column-indicator.el b/lisp/display-fill-column-indicator.el
new file mode 100644
index 0000000000..6e0990839e
--- /dev/null
+++ b/lisp/display-fill-column-indicator.el
@@ -0,0 +1,78 @@
+;;; display-fill-column-indicator.el --- interface for display-fill-column-indicator -*- lexical-binding: t -*-
+
+;; Copyright (C) 2017-2019 Free Software Foundation, Inc.
+
+;; Maintainer: emacs-devel@gnu.org
+;; Keywords: convenience
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Provides a minor mode interface for `display-fill-column-indicator'.
+;;
+;; Toggle display of line numbers with M-x
+;; display-fill-column-indicator-mode.  To enable line numbering in
+;; all buffers, use M-x global-display-fill-column-indicator-mode.  To
+;; change the default line column
+
+
+;; NOTE: Customization variables for
+;; `display-fill-column-indicator-column' and
+;; `display-fill-column-indicator-char' itself are defined in
+;; cus-start.el.
+
+;;; Code:
+
+(defgroup display-fill-column-indicator nil
+  "Display line numbers in the buffer."
+  :group 'convenience
+  :group 'display)
+
+
+;;;###autoload
+(define-minor-mode display-fill-column-indicator-mode
+  "Toggle display fill column indicator.
+This uses `display-fill-column-indicator' internally.
+
+To change the position of the line displayed by default,
+customize `display-fill-column-indicator-column' you can change the
+character for the line setting `display-fill-column-indicator-character'."
+  :lighter nil
+  (if display-fill-column-indicator-mode
+      (progn
+        (setq display-fill-column-indicator t)
+        (unless display-fill-column-indicator-column
+          (setq display-fill-column-indicator-column fill-column))
+        (unless display-fill-column-indicator-character
+          (if (char-displayable-p ?\u2502)
+              (setq display-fill-column-indicator-character ?\u2502)
+            (setq display-fill-column-indicator-character ?|))))
+    (setq display-fill-column-indicator nil)))
+
+(defun display-fill-column-indicator--turn-on ()
+  "Turn on `display-fill-column-indicator-mode'."
+  (unless (or (minibufferp)
+              (and (daemonp) (null (frame-parameter nil 'client))))
+    (display-fill-column-indicator-mode)))
+
+;;;###autoload
+(define-globalized-minor-mode global-display-fill-column-indicator-mode
+  display-fill-column-indicator-mode display-fill-column-indicator--turn-on)
+
+(provide 'display-fill-column-indicator)
+
+;;; display-fill-column-indicator.el ends here
diff --git a/lisp/faces.el b/lisp/faces.el
index ab6c384c80..c60f1da6f5 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2502,6 +2502,20 @@ unwanted effects."
   :group 'basic-faces
   :group 'display-line-numbers)
 
+;; Definition stolen from display-line-numbers.
+(defface fill-column
+  '((t :inherit (shadow default)))
+  "Face for displaying fill column indicator line.
+This face is used when `display-fill-column-indicator-mode' is
+non-nil.
+
+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."
+  :version "27.1"
+  :group 'basic-faces
+  :group 'display-fill-column-indicator)
+
 (defface escape-glyph
   '((((background dark)) :foreground "cyan")
     ;; See the comment in minibuffer-prompt for
diff --git a/lisp/frame.el b/lisp/frame.el
index dd1d5b030f..03c4d0761b 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -2663,6 +2663,9 @@ See also `toggle-frame-maximized'."
         display-line-numbers-width
         display-line-numbers-current-absolute
         display-line-numbers-widen
+        display-fill-column-indicator
+        display-fill-column-indicator-column
+        display-fill-column-indicator-character
         bidi-paragraph-direction
         bidi-display-reordering))
 
diff --git a/lisp/ldefs-boot.el b/lisp/ldefs-boot.el
index 0e8e5f699b..1227ceb377 100644
--- a/lisp/ldefs-boot.el
+++ b/lisp/ldefs-boot.el
@@ -7734,7 +7734,44 @@ See `display-line-numbers-mode' for more information on Display-Line-Numbers mod
 
 \(fn &optional ARG)" t nil)
 
-(if (fboundp 'register-definition-prefixes) (register-definition-prefixes "display-line-numbers" '("display-line-numbers-")))
+(if (fboundp 'register-definition-prefixes)
+    (register-definition-prefixes "display-line-numbers" '("display-line-numbers-")))
+
+;;;***
+\f
+;;;### (autoloads nil "display-fill-column-indicator" "display-fill-column-indicator.el"
+;;;;;;  (0 0 0 0))
+;;; Generated autoloads from display-fill-column-indicator.el
+
+(autoload 'display-fill-column-indicator-mode "display-fill-column-indicator" "\
+Toggle display fill column indicator.
+This uses `display-fill-column-indicator' internally.
+
+To change the position of the line displayed by default,
+customize `display-fill-column-indicator-column'.
+
+\(fn &optional ARG)" t nil)
+
+(defvar global-display-fill-column-indicator-mode nil "\
+Non-nil if Global Display-fill-column-indicator mode is enabled.
+See the `global-display-fill-column-indicator-mode' command
+for a description of this minor mode.")
+
+(custom-autoload 'global-display-fill-column-indicator-mode
+                 "display-fill-column-indicator" nil)
+
+(autoload 'global-display-fill-column-indicator-mode
+  "display-fill-column-indicator" "\
+Toggle display fill column indicator.
+This uses `display-fill-column-indicator' internally.
+
+To change the position of the line displayed by default,
+customize `display-fill-column-indicator-column'.
+
+\(fn &optional ARG)" t nil)
+
+(if (fboundp 'register-definition-prefixes)
+    (register-definition-prefixes "display-fill-column-indicator" '("display-fill-column-indicator-")))
 
 ;;;***
 \f
diff --git a/src/xdisp.c b/src/xdisp.c
index 6d30afda6d..1994c9b6a1 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -20135,15 +20135,48 @@ append_space_for_newline (struct it *it, bool default_face_p)
 	  it->what = IT_CHARACTER;
 	  memset (&it->position, 0, sizeof it->position);
 	  it->object = Qnil;
-	  it->c = it->char_to_display = ' ';
 	  it->len = 1;
 
+	  int local_default_face_id =
+	    lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID);
+	  struct face* default_face =
+	    FACE_FROM_ID_OR_NULL (it->f, local_default_face_id);
+
+	  /* 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 */
+	  if (!NILP (Vdisplay_fill_column_indicator)
+	      && FIXNATP (Vdisplay_fill_column_indicator_column)
+	      && FIXNATP (Vdisplay_fill_column_indicator_character))
+	    {
+	       struct font *font =
+	         default_face->font ? default_face->font : FRAME_FONT (it->f);
+	       const int char_width =
+	         font->average_width ? font->average_width : font->space_width;
+	       const int fill_column =
+		 XFIXNAT (Vdisplay_fill_column_indicator_column);
+	       const int column_x =
+		 char_width * fill_column + it->lnum_pixel_width;
+
+	       if (it->current_x == column_x)
+	         {
+	           it->c = it->char_to_display =
+		     XFIXNAT (Vdisplay_fill_column_indicator_character);
+	           it->face_id =
+		     merge_faces (it->w, Qfill_column, 0, DEFAULT_FACE_ID);
+	           face = FACE_FROM_ID(it->f, it->face_id);
+	           goto produce_glyphs;
+	         }
+	    }
+
+	  it->c = it->char_to_display = ' ';
 	  /* If the default face was remapped, be sure to use the
 	     remapped face for the appended newline.  */
 	  if (default_face_p)
-	    it->face_id = lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID);
+	    it->face_id = local_default_face_id;
 	  else if (it->face_before_selective_p)
 	    it->face_id = it->saved_face_id;
+
 	  face = FACE_FROM_ID (it->f, it->face_id);
 	  it->face_id = FACE_FOR_CHAR (it->f, face, 0, -1, Qnil);
 	  /* In R2L rows, we will prepend a stretch glyph that will
@@ -20152,11 +20185,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.  */
 	      && saved_x + FRAME_COLUMN_WIDTH (it->f) < it->last_visible_x)
 	    it->end_of_box_run_p = false;
 
+	produce_glyphs:
 	  PRODUCE_GLYPHS (it);
 
 #ifdef HAVE_WINDOW_SYSTEM
@@ -20305,7 +20339,8 @@ extend_face_to_end_of_line (struct it *it)
 #ifdef HAVE_WINDOW_SYSTEM
       && !face->stipple
 #endif
-      && !it->glyph_row->reversed_p)
+      && !it->glyph_row->reversed_p
+      && NILP (Vdisplay_fill_column_indicator))
     return;
 
   /* Set the glyph row flag indicating that the face of the last glyph
@@ -20359,6 +20394,60 @@ extend_face_to_end_of_line (struct it *it)
 	    }
 	}
 #ifdef HAVE_WINDOW_SYSTEM
+
+      if (!NILP (Vdisplay_fill_column_indicator)
+          && FIXNATP (Vdisplay_fill_column_indicator_column)
+          && FIXNATP (Vdisplay_fill_column_indicator_character))
+        {
+
+	  struct font *font =
+	    default_face->font ? default_face->font : FRAME_FONT (f);
+	  const int char_width =
+	    font->average_width ? font->average_width : font->space_width;
+
+	  const int fill_column =
+	    XFIXNAT (Vdisplay_fill_column_indicator_column);
+
+	  const int column_x = char_width * fill_column + it->lnum_pixel_width;
+
+	  if ((it->current_x < column_x)
+	      && (column_x < it->last_visible_x))
+	    {
+	      const char saved_char = it->char_to_display;
+	      const struct text_pos saved_pos = it->position;
+	      const bool saved_avoid_cursor = it->avoid_cursor_p;
+	      const int saved_face_id = it->face_id;
+	      const bool saved_box_start = it->start_of_box_run_p;
+	      Lisp_Object save_object = it->object;
+
+	      /* The stretch width needs to considet the latter added glyph */
+	      const int stretch_width = column_x - it->current_x - char_width;
+
+	      int stretch_ascent = (((it->ascent + it->descent)
+		    * FONT_BASE (font)) / FONT_HEIGHT (font));
+
+	      it->char_to_display =
+	        XFIXNAT (Vdisplay_fill_column_indicator_character);
+	      memset (&it->position, 0, sizeof it->position);
+	      it->avoid_cursor_p = true;
+	      it->face_id =
+	        merge_faces (it->w, Qfill_column, 0, DEFAULT_FACE_ID);
+	      it->start_of_box_run_p = false;
+	      it->object = Qnil;
+
+	      append_stretch_glyph (it, Qnil, stretch_width,
+	                            it->ascent + it->descent, stretch_ascent);
+
+	      PRODUCE_GLYPHS (it);
+
+	      it->position = saved_pos;
+	      it->avoid_cursor_p = saved_avoid_cursor;
+	      it->face_id = saved_face_id;
+	      it->start_of_box_run_p = saved_box_start;
+	      it->char_to_display = saved_char;
+	      it->object = save_object;
+	    }
+        }
       if (it->glyph_row->reversed_p)
 	{
 	  /* Prepend a stretch glyph to the row, such that the
@@ -20478,10 +20567,37 @@ extend_face_to_end_of_line (struct it *it)
 	it->face_id = default_face->id;
       else
 	it->face_id = face->id;
-      PRODUCE_GLYPHS (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);
+        }
 
       if (WINDOW_RIGHT_MARGIN_WIDTH (it->w) > 0
 	  && (it->glyph_row->used[RIGHT_MARGIN_AREA]
@@ -20571,14 +20687,16 @@ highlight_trailing_whitespace (struct it *it)
       if (!row->reversed_p)
 	{
 	  while (glyph >= start
-		 && glyph->type == CHAR_GLYPH
+	         && (glyph->type == CHAR_GLYPH
+	             || glyph->type == STRETCH_GLYPH)
 		 && NILP (glyph->object))
 	    --glyph;
 	}
       else
 	{
 	  while (glyph <= start
-		 && glyph->type == CHAR_GLYPH
+		 && (glyph->type == CHAR_GLYPH
+	             || glyph->type == STRETCH_GLYPH)
 		 && NILP (glyph->object))
 	    ++glyph;
 	}
@@ -32645,6 +32763,9 @@ be let-bound around code that needs to disable messages temporarily. */);
   /* Name of a text property which disables line-number display.  */
   DEFSYM (Qdisplay_line_numbers_disable, "display-line-numbers-disable");
 
+  /* Names of the face used to display fill column indicator character.  */
+  DEFSYM (Qfill_column, "fill-column");
+
   /* Name and number of the face used to highlight escape glyphs.  */
   DEFSYM (Qescape_glyph, "escape-glyph");
 
@@ -33213,6 +33334,27 @@ either `relative' or `visual'.  */);
   DEFSYM (Qdisplay_line_numbers_widen, "display-line-numbers-widen");
   Fmake_variable_buffer_local (Qdisplay_line_numbers_widen);
 
+  DEFVAR_LISP ("display-fill-column-indicator", Vdisplay_fill_column_indicator,
+    doc: /* Non-nil means display the fill column indicator.  */);
+  Vdisplay_fill_column_indicator = Qnil;
+  DEFSYM (Qdisplay_fill_column_indicator, "display-fill-column-indicator");
+  Fmake_variable_buffer_local (Qdisplay_fill_column_indicator);
+
+  DEFVAR_LISP ("display-fill-column-indicator-column", Vdisplay_fill_column_indicator_column,
+    doc: /* Column to draw the indicator when `display-fill-column-indicator' is non-nil.
+The default value is the variable `fill-column' if not other value is given. */);
+  Vdisplay_fill_column_indicator_column = Qnil;
+  DEFSYM (Qdisplay_fill_column_indicator_column, "display-fill-column-indicator-column");
+  Fmake_variable_buffer_local (Qdisplay_fill_column_indicator_column);
+
+  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 the but a good alternative is (ascii 124) if
+the font in fill-column-face supports Unicode characters.  */);
+  Vdisplay_fill_column_indicator_character = Qnil;
+  DEFSYM (Qdisplay_fill_column_indicator_character, "display-fill-column-indicator-character");
+  Fmake_variable_buffer_local (Qdisplay_fill_column_indicator_character);
+
   DEFVAR_BOOL ("inhibit-eval-during-redisplay", inhibit_eval_during_redisplay,
     doc: /* Non-nil means don't eval Lisp during redisplay.  */);
   inhibit_eval_during_redisplay = false;