Re: Fill column indicator functionality

[Eli Zaretskii <eliz@gnu.org>]

> Date: Thu, 14 Mar 2019 17:51:49 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> >> 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. 

As expected.

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

Because unlike loaddefs.el, ldefs-boot.el is a versioned file.  It
must be in the repository, otherwise people would be unable to build a
fresh checkout.  It just isn't maintained by hand, that's all.

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

Using DEFVAR_INT and DEFVAR_BOOL makes it easier to reference the
variable in C, since you have a normal C integer or boolean.  So, for
example, you can say

  if (emacs_scroll_step > 0)

instead of

  if (XFIXNAT (Vemacs_scroll_step) > 0)

and

  if (!inhibit_message)

instead of

  if (!NILP (Vinhibit_message))

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

We could use a special value of t for
display-fill-column-indicator-column to mean the current value of
fill-column.

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

I think the main part that is missing is your copyright assignment.
Did you do your legal paperwork?  If not, now is the time to start it.
This patch is large enough to require a copyright assignment.

The only other missing parts are documentation: NEWS and the user
manual.

Btw, did you test the code when there are variable-size characters in
the buffer?  Good test cases are Info files and also Web pages with
images visited via EWW.

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

Indicating the fill-column with background can use almost the same
code as the one you wrote, but we will need to decide what to do if
the text that exceeds fill-column itself has non-default background.
Also, I think the visual effect will be less pleasant when there are
continued lines, because background color is more prominent than an
indicator by a single character.

Thanks.