Fill column indicator functionality

Fill column indicator functionality

[Ergus]

Hi guys:

I am not sure if this is the right mailing list to ask for this.

I can't find any fill column indication functionality in emacs. For 80
(or 120) sacred column rule.

Looking around there is the fill-column-indicator package in melpa that
I have been using for a while, but recently it has been abandoned, but
also it is incompatible with native functionalities like
show-trailing-whitespace, so it enforces to use whitespace-mode instead.

Is it too difficult to add such a functionality native in emacs as you
did with the line numbers in the last release? So it will be much better
and it is not a so fancy functionality?

Thanks in advance.

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Tue, 5 Feb 2019 11:53:40 +0100
> From: Ergus <spacibba@aol.com>
> 
> I can't find any fill column indication functionality in emacs. For 80
> (or 120) sacred column rule.

ruler-mode does it, but maybe you are looking for a different kind of
indicator.  If so, would you like to describe how you'd like it to
look?

> Is it too difficult to add such a functionality native in emacs as you
> did with the line numbers in the last release?

Hard to answer this without knowing what functionality is being
sought.

Re: Fill column indicator functionality

[Ergus]

Hi Eli:

Yes, I tried rule mode first, but it is a little bit too much
distracting.

I would like to have something simple like fill-column-indicator mode
package https://github.com/alpaker/Fill-Column-Indicator.

(but without it's conflicts and many issues) even if it only brings the
most basic functionalities. This is a very basic code editing
functionality that shouldn't depend of external packages (like line
numbers for example) in my opinion.

It could be a line | or a background color change up to the X column. This
is very useful, when coding old Fortran or Cobol, but also in C.

The alternative package now is hl-fill-column but I personally don't
like it because I only see the line when is crossed already.

I know you should be very busy, but maybe it is not that hard to
implement, so I had to ask at least.

Regards,

On Tue, Feb 05, 2019 at 06:41:44PM +0200, Eli Zaretskii wrote:
>> Date: Tue, 5 Feb 2019 11:53:40 +0100
>> From: Ergus <spacibba@aol.com>
>>
>> I can't find any fill column indication functionality in emacs. For 80
>> (or 120) sacred column rule.
>
>ruler-mode does it, but maybe you are looking for a different kind of
>indicator.  If so, would you like to describe how you'd like it to
>look?
>
>> Is it too difficult to add such a functionality native in emacs as you
>> did with the line numbers in the last release?
>
>Hard to answer this without knowing what functionality is being
>sought.

RE: Fill column indicator functionality

[Drew Adams]

> I would like to have something simple like fill-column-indicator mode
> (but without it's conflicts and many issues) even if it only brings the
> most basic functionalities. This is a very basic code editing
> functionality that shouldn't depend of external packages (like line
> numbers for example) in my opinion.
> 
> It could be a line | or a background color change up to the X column.

Haven't been following this thread; sorry.

Have you looked at this Emacs Wiki page? Perhaps it will help - it mentions a few ways to indicate a particular column.

https://www.emacswiki.org/emacs/EightyColumnRule

Re: Fill column indicator functionality

[Ergus]

Hi Drew:

Thanks for replying. I saw the page and I tried almost all the options.

My feature request comes from the fact that almost all the options there
are unmaintained and some look like patches in new clothes.

- Column-marker creates a mark only in the crossed lines. But it is more
  intuitive to have something indicating where is the limit before
  reaching it. Like a rule or a background color change.

- ModeLinePosition like ruler-mode shows the indicators in the borders
  and conflicts with spaceline and sml. But it is also far from the
  center of the screen. When editing there.

- FillColumnIndicator is the one I have been using until now, but it is
  abandoned since a long time ago (the last update was in December but
  it was to add in the readme that it is not longer maintained), it has
  many issues and incompatibilities, enforces to use whitespace-mode
  because is incompatible with show-trailing-whitespace.

- Column-Enforce-Mode is like Column-marker, but changes the rest of the
  line, at least for me it becomes annoying after a while. And also
  highlights the line only after crossed. (SO same issue than Column-maker)

There are other options in melpa: hl-fill-column-mode which is more or
less like column-marker.

I think this is a very basic text editing functionality (like line
numbers). And Emacs shouldn't need a plugin for this very basic text
editing functionalities.

On the other hand, making it part of emacs can be guaranteed to have a
similar clean appearance as you did with the line numbers in the last
release. So everything will be more standard by default.

But again, it is my opinion, I don't want to bother with this too much
because you are very busy people. Any design choice that can make it
better, more efficient or simpler will be fine even if it is not what I
had in mind.

Best,

On Tue, Feb 05, 2019 at 07:56:02PM +0000, Drew Adams wrote:
>> I would like to have something simple like fill-column-indicator mode
>> (but without it's conflicts and many issues) even if it only brings the
>> most basic functionalities. This is a very basic code editing
>> functionality that shouldn't depend of external packages (like line
>> numbers for example) in my opinion.
>>
>> It could be a line | or a background color change up to the X column.
>
>Haven't been following this thread; sorry.
>
>Have you looked at this Emacs Wiki page? Perhaps it will help - it mentions a few ways to indicate a particular column.
>
>https://www.emacswiki.org/emacs/EightyColumnRule
>
>

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Tue, 5 Feb 2019 19:47:20 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> I would like to have something simple like fill-column-indicator mode
> package https://github.com/alpaker/Fill-Column-Indicator.
> 
> (but without it's conflicts and many issues) even if it only brings the
> most basic functionalities. This is a very basic code editing
> functionality that shouldn't depend of external packages (like line
> numbers for example) in my opinion.
> 
> It could be a line | or a background color change up to the X column. This
> is very useful, when coding old Fortran or Cobol, but also in C.

How popular are such extensions?  IMO it only makes sense to implement
something like that in core if there's enough demand, like with line
numbers.

Perhaps you should start a poll on Reddit about this, and we can then
see how many people would like to use such a feature.  Do many people
share you dissatisfaction with solutions that show fill-column only
when it is crossed, for example?

There are also some questions about what exactly to do with
continuation lines, and with situations where fill-column is larger
than the window-width.  fci-mode forces you to turn on truncate-lines,
and basically ignores fill-column values larger than the window-width,
but for a core feature we need to figure out what to do in those
cases.  E.g., your proposal of indicating fill-column with background
color will then cause unpleasant display with alternating background
around continued lines.  These aspects could/should also be discussed
on Reddit before implementing.

Re: Fill column indicator functionality

[John Yates]

On Wed, Feb 6, 2019 at 11:09 AM Eli Zaretskii <eliz@gnu.org> wrote:
> Perhaps you should start a poll on Reddit about this, and we can then
> see how many people would like to use such a feature.  Do many people
> share you dissatisfaction with solutions that show fill-column only
> when it is crossed, for example?

I do not follow (proper verb?) Reddit.  That said I have desired
such a feature for years.

My use case (assumed very common) is actively entering / editing
mono-spaced text within a window that is at least somewhat wider
than the column about which I want some visual forewarning.  If I
am merely reading I most likely do not care to have such a visual
clue.

I would be entirely happy with an ability to draw a single
vertical line at a nominal "physical" column (not necessarily the
fill-column).  That line would be drawn irrespective of wrapped
continuations or other edge cases.  If the column is beyond the
window edge then the line simply would not get drawn.  If the
effect is ill-defined / unpalatable I would be happy to do
without as I would be no worse off than I am today.

(This seems reminiscent of recent discussion of "layers".)

/john

Re: Fill column indicator functionality

[Ergus]

That's exactly the same I would like to have. With a small plus of being
compatible with show-trailing-whitespace.

I will start the post in reddit once I understand how to use it.


On Wed, Feb 06, 2019 at 03:48:42PM -0500, John Yates wrote:
>On Wed, Feb 6, 2019 at 11:09 AM Eli Zaretskii <eliz@gnu.org> wrote:
>> Perhaps you should start a poll on Reddit about this, and we can then
>> see how many people would like to use such a feature.  Do many people
>> share you dissatisfaction with solutions that show fill-column only
>> when it is crossed, for example?
>
>I do not follow (proper verb?) Reddit.  That said I have desired
>such a feature for years.
>
>My use case (assumed very common) is actively entering / editing
>mono-spaced text within a window that is at least somewhat wider
>than the column about which I want some visual forewarning.  If I
>am merely reading I most likely do not care to have such a visual
>clue.
>
>I would be entirely happy with an ability to draw a single
>vertical line at a nominal "physical" column (not necessarily the
>fill-column).  That line would be drawn irrespective of wrapped
>continuations or other edge cases.  If the column is beyond the
>window edge then the line simply would not get drawn.  If the
>effect is ill-defined / unpalatable I would be happy to do
>without as I would be no worse off than I am today.
>
>(This seems reminiscent of recent discussion of "layers".)
>
>/john
>

Re: Fill column indicator functionality

[Basil L. Contovounesios]

Ergus <spacibba@aol.com> writes:

> That's exactly the same I would like to have. With a small plus of being
> compatible with show-trailing-whitespace.
>
> I will start the post in reddit once I understand how to use it.
>
>
> On Wed, Feb 06, 2019 at 03:48:42PM -0500, John Yates wrote:
>>On Wed, Feb 6, 2019 at 11:09 AM Eli Zaretskii <eliz@gnu.org> wrote:
>>> Perhaps you should start a poll on Reddit about this, and we can then
>>> see how many people would like to use such a feature.  Do many people
>>> share you dissatisfaction with solutions that show fill-column only
>>> when it is crossed, for example?
>>
>>I do not follow (proper verb?) Reddit.  That said I have desired
>>such a feature for years.
>>
>>My use case (assumed very common) is actively entering / editing
>>mono-spaced text within a window that is at least somewhat wider
>>than the column about which I want some visual forewarning.  If I
>>am merely reading I most likely do not care to have such a visual
>>clue.
>>
>>I would be entirely happy with an ability to draw a single
>>vertical line at a nominal "physical" column (not necessarily the
>>fill-column).  That line would be drawn irrespective of wrapped
>>continuations or other edge cases.  If the column is beyond the
>>window edge then the line simply would not get drawn.  If the
>>effect is ill-defined / unpalatable I would be happy to do
>>without as I would be no worse off than I am today.
>>
>>(This seems reminiscent of recent discussion of "layers".)

Just chiming in because I also do not read Reddit (readit?).

As a beginner Emacsite (and programmer) in 2014, I felt I could not work
without line numbers or a rule at 80 columns.  I, too, eventually
settled on fci-mode after some quick and inexperienced testing of other
packages/solutions.  It was nice to be able to preemptively format/break
lines as they grew closer to the rule, without having to wait for 80
columns to be exceeded before this was visually indicated, for example.

Since then, I have dispensed with both features because of performance
and compatibility issues like the ones already mentioned, and I have
become accustomed to their absence and a more minimalistic (less
maximalistic?) setup.

Nevertheless, I was very happy to see the addition of
display-line-numbers (thanks Eli!), and I would also welcome some sort
of native column indication like that of fci-mode, because these have
been popular editor features in my own past experience and that of my
peers.

Having said that, I appreciate that the design and implementation of
this will be non-trivial, as there are many decisions to be made and
circumstances/options to be supported.

Thanks,

-- 
Basil

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Cc: John Yates <john@yates-sheets.org>,  Eli Zaretskii <eliz@gnu.org>,  Emacs developers <emacs-devel@gnu.org>
> Date: Thu, 07 Feb 2019 01:41:04 +0000
> 
> Having said that, I appreciate that the design and implementation of
> this will be non-trivial, as there are many decisions to be made and
> circumstances/options to be supported.

This doesn't have to be non-trivial, if we are smart enough not to
over-specify the feature.

Re: Fill column indicator functionality

[Ergus]

Hi again:

I have been checking the display engine and the commits to add the
display-line-number functionality. With this, it really looks much
simpler to implement the version of the vertical line (character at
column X).

Do you still think not worth it to try a simpler implementation where the
only specification is the column number and the character to use? (And
probably the face). Not shown when the line is longer or when the window
is narrower.

If someone can advise or start with this I will be very happy to help. I
think that the there is a lot to gain and nothing to lose. This is a
basic text editing functionality. Many text/code editors have it and
there is not any reason we can't replicate it.

And we should improve firstly the editor functionalities, because that's
the reason people starts using emacs, later they discover the rest; but
the first it is, is a text editor.



On Thu, Feb 07, 2019 at 04:31:00PM +0200, Eli Zaretskii wrote:
>> From: "Basil L. Contovounesios" <contovob@tcd.ie>
>> Cc: John Yates <john@yates-sheets.org>,  Eli Zaretskii <eliz@gnu.org>,  Emacs developers <emacs-devel@gnu.org>
>> Date: Thu, 07 Feb 2019 01:41:04 +0000
>>
>> Having said that, I appreciate that the design and implementation of
>> this will be non-trivial, as there are many decisions to be made and
>> circumstances/options to be supported.
>
>This doesn't have to be non-trivial, if we are smart enough not to
>over-specify the feature.
>

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Sun, 10 Feb 2019 23:04:54 +0100
> From: Ergus <spacibba@aol.com>
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>, emacs-devel@gnu.org,
> 	john@yates-sheets.org
> 
> I have been checking the display engine and the commits to add the
> display-line-number functionality. With this, it really looks much
> simpler to implement the version of the vertical line (character at
> column X).

I'm not sure I understand: much simpler than what alternative?

> Do you still think not worth it to try a simpler implementation where the
> only specification is the column number and the character to use?

Also here: simpler than what alternative(s)?

> If someone can advise or start with this I will be very happy to
> help.

I can describe my idea of implementing this, if that's what you meant,
and point to the places in the code where it should be done.  If you
then decide to work on that, I will gladly help with advice and any
questions you might have.

Thanks.

Re: Fill column indicator functionality

[Jimmy Aguilar Mena]

On Mon, Feb 11, 2019 at 05:55:04PM +0200, Eli Zaretskii wrote:
>> Date: Sun, 10 Feb 2019 23:04:54 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>, emacs-devel@gnu.org,
>> 	john@yates-sheets.org
>>
>> I have been checking the display engine and the commits to add the
>> display-line-number functionality. With this, it really looks much
>> simpler to implement the version of the vertical line (character at
>> column X).
>
>I'm not sure I understand: much simpler than what alternative?
>
The color change one.

>> Do you still think not worth it to try a simpler implementation where the
>> only specification is the column number and the character to use?
>
>Also here: simpler than what alternative(s)?
>
>> If someone can advise or start with this I will be very happy to
>> help.
>
>I can describe my idea of implementing this, if that's what you meant,
>and point to the places in the code where it should be done.  If you
>then decide to work on that, I will gladly help with advice and any
>questions you might have.
>
>Thanks.
>
Exactly that's what I mean. BTW, is it there any legal paperwork before
contributing?

http://bsc.es/disclaimer

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Mon, 11 Feb 2019 17:56:36 +0100
> From: Jimmy Aguilar Mena <jimmy.aguilar@bsc.es>
> Cc: Ergus <spacibba@aol.com>, contovob@tcd.ie, john@yates-sheets.org,
>         emacs-devel@gnu.org
> 
> >> I have been checking the display engine and the commits to add the
> >> display-line-number functionality. With this, it really looks much
> >> simpler to implement the version of the vertical line (character at
> >> column X).
> >
> >I'm not sure I understand: much simpler than what alternative?
> >
> The color change one.

I don't think it's easier, I think both could be implemented with
similar efforts, and in the same place in the code.  (Caveat: I only
thought a little bit about the implementation, so I might be missing
some important details.)

> BTW, is it there any legal paperwork before contributing?

If you don't have a copyright assignment for contributing to Emacs,
you should start the legal paperwork rolling, so that it's in place by
the time you will have code to contribute.

Thanks.

Re: Fill column indicator functionality

[Ergus]

Hi all:

I have been playing with the display engine in order to try to implement
this functionality (at least a proof of concept). But after reading the
comments and the ~33E3 lines in xdisp.c there are more questions now
than answers.

The simplest approach I see is to add a '|' (or similar) in the desired
column in the "at_end_of_line:" label. But maybe there is a better
approach?

My approach is somehow based on the display_line_numbers, but taking
into account that in this case the characters are always the same, maybe
there is a way to set them in the background and it will be simpler?

Is it there any documentation about this? specially the relations
between the data structures? because the it and the glyphs are very
confusing.

Any help please?



On Mon, Feb 11, 2019 at 07:13:25PM +0200, Eli Zaretskii wrote:
>> Date: Mon, 11 Feb 2019 17:56:36 +0100
>> From: Jimmy Aguilar Mena <jimmy.aguilar@bsc.es>
>> Cc: Ergus <spacibba@aol.com>, contovob@tcd.ie, john@yates-sheets.org,
>>         emacs-devel@gnu.org
>>
>> >> I have been checking the display engine and the commits to add the
>> >> display-line-number functionality. With this, it really looks much
>> >> simpler to implement the version of the vertical line (character at
>> >> column X).
>> >
>> >I'm not sure I understand: much simpler than what alternative?
>> >
>> The color change one.
>
>I don't think it's easier, I think both could be implemented with
>similar efforts, and in the same place in the code.  (Caveat: I only
>thought a little bit about the implementation, so I might be missing
>some important details.)
>
>> BTW, is it there any legal paperwork before contributing?
>
>If you don't have a copyright assignment for contributing to Emacs,
>you should start the legal paperwork rolling, so that it's in place by
>the time you will have code to contribute.
>
>Thanks.
>

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Fri, 8 Mar 2019 19:57:46 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> I have been playing with the display engine in order to try to implement
> this functionality (at least a proof of concept). But after reading the
> comments and the ~33E3 lines in xdisp.c there are more questions now
> than answers.

Feel free to ask the questions here.

> The simplest approach I see is to add a '|' (or similar) in the desired
> column in the "at_end_of_line:" label. But maybe there is a better
> approach?

You are on the right track.  But I'd suggest to do this inside the
function extend_face_to_end_of_line.  It is called near that label,
but you will see that it's called in several more places; I expect at
least some of them to need the same changes.

> My approach is somehow based on the display_line_numbers, but taking
> into account that in this case the characters are always the same, maybe
> there is a way to set them in the background and it will be simpler?

Simpler than what?  Can you tell what kind of complexity did you want
to avoid?

Display of line numbers needs to calculate the number of each screen
line, and that of course is not needed in this case.  You just need
to calculate the width of a stretch glyph that will display as
whitespace between the end of the line and the fill column.  You will
see that extend_face_to_end_of_line already does something similar in
the case of R2L lines.

> Is it there any documentation about this?

Maybe, but to point you to it I need to understand what "this" means
in this context.

> specially the relations between the data structures?

Which data structures? there are quite a few of them, so please be
more specific.

> because the it and the glyphs are very confusing.

Feel free to ask any questions about these two, especially about what
confuses you.

> Any help please?

I'm here to help.

And thanks for working on this.

Re: Fill column indicator functionality

[Ergus]

On Fri, Mar 08, 2019 at 10:06:41PM +0200, Eli Zaretskii wrote:
>> Date: Fri, 8 Mar 2019 19:57:46 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> I have been playing with the display engine in order to try to implement
>> this functionality (at least a proof of concept). But after reading the
>> comments and the ~33E3 lines in xdisp.c there are more questions now
>> than answers.
>
>Feel free to ask the questions here.
>
>> The simplest approach I see is to add a '|' (or similar) in the desired
>> column in the "at_end_of_line:" label. But maybe there is a better
>> approach?
>
>You are on the right track.  But I'd suggest to do this inside the
>function extend_face_to_end_of_line.  It is called near that label,
>but you will see that it's called in several more places; I expect at
>least some of them to need the same changes.
>
Hi Eli, I just finished a working version (pretty simple as a proof of
concept). That works in terminal mode. But in graphical mode we should
look for another approach because extend_face_to_end_of_line returns in:

if (FRAME_WINDOW_P (f)
   && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
   && face->box == FACE_NO_BOX
   && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
#ifdef HAVE_WINDOW_SYSTEM
   && !face->stipple
#endif
   && !it->glyph_row->reversed_p)
   return;

So it don't write until the line end.

In graphical mode maybe it is better to do this change writing directly
a vertical line in the windows' frame background. That way it will be maybe
more efficient because we only do it once. But I don't know if this is
possible-recommended.

Else, we can follow a similar approach than in terminal version and
generate a glygh for that cases. What do you think?

BTW: Is it possible to generate a single wider glyph instead of doing a
loop?.

The glyph concept is actually not  clear for me yet, specially in
graphical mode.

>> My approach is somehow based on the display_line_numbers, but taking
>> into account that in this case the characters are always the same, maybe
>> there is a way to set them in the background and it will be simpler?
>
>Simpler than what?  Can you tell what kind of complexity did you want
>to avoid?
>
>Display of line numbers needs to calculate the number of each screen
>line, and that of course is not needed in this case.  You just need
>to calculate the width of a stretch glyph that will display as
>whitespace between the end of the line and the fill column.  You will
>see that extend_face_to_end_of_line already does something similar in
>the case of R2L lines.
>
>> Is it there any documentation about this?
>
>Maybe, but to point you to it I need to understand what "this" means
>in this context.
>
>> specially the relations between the data structures?
>
>Which data structures? there are quite a few of them, so please be
>more specific.
>
glyph <-> it: why this looks so complex? Because "it" has too many
properties I can't understand their functionalities and sometimes I
think I could reinvent the wheel.

>> because the it and the glyphs are very confusing.
>
>Feel free to ask any questions about these two, especially about what
>confuses you.
>
>> Any help please?
>
>I'm here to help.
>
>And thanks for working on this.
>
Thanks to you.

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Sat, 9 Mar 2019 14:22:09 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> >You are on the right track.  But I'd suggest to do this inside the
> >function extend_face_to_end_of_line.  It is called near that label,
> >but you will see that it's called in several more places; I expect at
> >least some of them to need the same changes.
> >
> Hi Eli, I just finished a working version (pretty simple as a proof of
> concept). That works in terminal mode. But in graphical mode we should
> look for another approach because extend_face_to_end_of_line returns in:
> 
> if (FRAME_WINDOW_P (f)
>    && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
>    && face->box == FACE_NO_BOX
>    && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
> #ifdef HAVE_WINDOW_SYSTEM
>    && !face->stipple
> #endif
>    && !it->glyph_row->reversed_p)
>    return;
> 
> So it don't write until the line end.

Yes, you need to add one more condition to that 'if', so that it
doesn't return when this feature is in effect.

> In graphical mode maybe it is better to do this change writing directly
> a vertical line in the windows' frame background.

I don't think we can do this in Emacs, the background is just a color
for us.

> That way it will be maybe more efficient because we only do it
> once.

If it were possible, it still could not be done once, because every
redisplay would have to redraw it.

> But I don't know if this is possible-recommended.

It's not possible with what we have in Emacs today.

> BTW: Is it possible to generate a single wider glyph instead of doing a
> loop?.

Yes, of course: you produce a single stretch glyph of a suitable
width.  Look at what the code does in the "row->reversed_p" case.

> >Which data structures? there are quite a few of them, so please be
> >more specific.
> >
> glyph <-> it: why this looks so complex?

For 2 main reasons:

  . the 'it' structure needs to keep track of several state variables,
    which affect more than one glyph, and it needs to do that without
    relying on the glyphs, because sometimes these functions are
    called with it->glyph_row set to NULL (when we need to simulate
    display without displaying anything, see move_it_to

  . the 'it' structure instance is discarded once the glyphs are
    produced, so if we need to record information about the glyphs, we
    need to keep it with the glyphs themselves

> Because "it" has too many properties I can't understand their
> functionalities and sometimes I think I could reinvent the wheel.

The comments in dispextern.h should help.  If they are not enough, we
can add more comments, but you need to point out the missing
information.

Re: Fill column indicator functionality

[Ergus]

Hi again:

I have a simple version for the column indicator line. There are still
missing functionalities like R2L or multiple columns indicator
implementations, but I don't want to go very complex on this in the
first try.

I have 2 questions:

1) In the graphical implementation I generated the column following the
Eli suggestions with something like: 

    struct font *font = face->font ? face->font : FRAME_FONT (f);

    it->char_to_display = '|';
    int stretch_ascent = (((it->ascent + it->descent)
              * FONT_BASE (font)) / FONT_HEIGHT (font));

    memset (&it->position, 0, sizeof it->position);
    it->avoid_cursor_p = true;
    it->face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID);
    it->start_of_box_run_p = false;
    append_stretch_glyph (it, Qnil, 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;

 The problem is that in this case if (setq show-trailing-whitespace t)
the whitespaces aren't highlighted if they are not longer that the line;
in term mode (with the loop) it works fine.

The function highlight_trailing_whitespace checks for glyph->type ==
STRETCH_GLYPH so I am missing something. 

2) What name do you suggest for this mode? I propose
display-column-indicator.

Thanks in advance.

Ergus

On Sat, Mar 09, 2019 at 04:10:11PM +0200, Eli Zaretskii wrote:
>> Date: Sat, 9 Mar 2019 14:22:09 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> >You are on the right track.  But I'd suggest to do this inside the
>> >function extend_face_to_end_of_line.  It is called near that label,
>> >but you will see that it's called in several more places; I expect at
>> >least some of them to need the same changes.
>> >
>> Hi Eli, I just finished a working version (pretty simple as a proof of
>> concept). That works in terminal mode. But in graphical mode we should
>> look for another approach because extend_face_to_end_of_line returns in:
>>
>> if (FRAME_WINDOW_P (f)
>>    && MATRIX_ROW_DISPLAYS_TEXT_P (it->glyph_row)
>>    && face->box == FACE_NO_BOX
>>    && FACE_COLOR_TO_PIXEL (face->background, f) == FRAME_BACKGROUND_PIXEL (f)
>> #ifdef HAVE_WINDOW_SYSTEM
>>    && !face->stipple
>> #endif
>>    && !it->glyph_row->reversed_p)
>>    return;
>>
>> So it don't write until the line end.
>
>Yes, you need to add one more condition to that 'if', so that it
>doesn't return when this feature is in effect.
>
>> In graphical mode maybe it is better to do this change writing directly
>> a vertical line in the windows' frame background.
>
>I don't think we can do this in Emacs, the background is just a color
>for us.
>
>> That way it will be maybe more efficient because we only do it
>> once.
>
>If it were possible, it still could not be done once, because every
>redisplay would have to redraw it.
>
>> But I don't know if this is possible-recommended.
>
>It's not possible with what we have in Emacs today.
>
>> BTW: Is it possible to generate a single wider glyph instead of doing a
>> loop?.
>
>Yes, of course: you produce a single stretch glyph of a suitable
>width.  Look at what the code does in the "row->reversed_p" case.
>
>> >Which data structures? there are quite a few of them, so please be
>> >more specific.
>> >
>> glyph <-> it: why this looks so complex?
>
>For 2 main reasons:
>
>  . the 'it' structure needs to keep track of several state variables,
>    which affect more than one glyph, and it needs to do that without
>    relying on the glyphs, because sometimes these functions are
>    called with it->glyph_row set to NULL (when we need to simulate
>    display without displaying anything, see move_it_to
>
>  . the 'it' structure instance is discarded once the glyphs are
>    produced, so if we need to record information about the glyphs, we
>    need to keep it with the glyphs themselves
>
>> Because "it" has too many properties I can't understand their
>> functionalities and sometimes I think I could reinvent the wheel.
>
>The comments in dispextern.h should help.  If they are not enough, we
>can add more comments, but you need to point out the missing
>information.
>

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Mon, 11 Mar 2019 11:48:14 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> 1) In the graphical implementation I generated the column following the
> Eli suggestions with something like: 
> 
>     struct font *font = face->font ? face->font : FRAME_FONT (f);
> 
>     it->char_to_display = '|';
>     int stretch_ascent = (((it->ascent + it->descent)
>               * FONT_BASE (font)) / FONT_HEIGHT (font));
> 
>     memset (&it->position, 0, sizeof it->position);
>     it->avoid_cursor_p = true;
>     it->face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID);
>     it->start_of_box_run_p = false;
>     append_stretch_glyph (it, Qnil, 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;
> 
>  The problem is that in this case if (setq show-trailing-whitespace t)
> the whitespaces aren't highlighted if they are not longer that the line;
> in term mode (with the loop) it works fine.
> 
> The function highlight_trailing_whitespace checks for glyph->type ==
> STRETCH_GLYPH so I am missing something. 

I think you are missing this:

      /* If last glyph is a space or stretch, and it's trailing
	 whitespace, set the face of all trailing whitespace glyphs in
	 IT->glyph_row to `trailing-whitespace'.  */
      if ((row->reversed_p ? glyph <= start : glyph >= start)
	  && BUFFERP (glyph->object)  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
	  && (glyph->type == STRETCH_GLYPH
	      || (glyph->type == CHAR_GLYPH
		  && glyph->u.ch == ' '))
	  && trailing_whitespace_p (glyph->charpos))

This test is for those stretch glyphs that came from a buffer,
i.e. those generated by TABs and 'space' display properties.  But in
your case, the object of the stretch glyph is nil, as specified by the
2nd argument of append_stretch_glyph:

    append_stretch_glyph (it, Qnil, width,
                          it->ascent + it->descent, stretch_ascent);

Moreover, I believe you are looking at the wrong code fragment to
explain why trailing-whitespace stopped working.  I think the problem
happens a bit earlier, here:

      /* Skip over glyphs inserted to display the cursor at the
	 end of a line, for extending the face of the last glyph
	 to the end of the line on terminals, and for truncation
	 and continuation glyphs.  */
      if (!row->reversed_p)
	{
	  while (glyph >= start
		 && glyph->type == CHAR_GLYPH
		 && NILP (glyph->object))
	    --glyph;
	}

This skips only glyphs of type CHAR_GLYPH, but your change inserts a
STRETCH_GLYPH, which you also want to skip.  So the above condition
should be augmented to include STRETCH_GLYPH as well.

> 2) What name do you suggest for this mode? I propose
> display-column-indicator.

display-fill-column-indicator-mode?

Thanks again for working on this.

P.S. Thanks to you, I just uncovered an old bug, whereby
show-trailing-whitespace didn't work in R2L screen lines, and for the
same reason: the code didn't account for the stretch glyphs inserted
by the display engine at the left edge of such lines.

Re: Fill column indicator functionality

[Andy Moreton]

On Mon 11 Mar 2019, Eli Zaretskii wrote:

>> Date: Mon, 11 Mar 2019 11:48:14 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>
>> 2) What name do you suggest for this mode? I propose
>> display-column-indicator.
>
> display-fill-column-indicator-mode?
>
> Thanks again for working on this.
>
> P.S. Thanks to you, I just uncovered an old bug, whereby
> show-trailing-whitespace didn't work in R2L screen lines, and for the
> same reason: the code didn't account for the stretch glyphs inserted
> by the display engine at the left edge of such lines.

Isn't there some overlap with what Keith Bershatsky is doing ? It may be
worth discussing if there is any anything in common. His posts usually
start with:

  I am working on optimization of feature requests #22873 (multiple fake
  cursors) and #17684 (crosshairs that track the cursor position, and a
  visible fill column indicator).

His work on bug #17684 may be relevant.

    AndyM

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Mon, 11 Mar 2019 19:58:38 +0000
> 
> Isn't there some overlap with what Keith Bershatsky is doing ? It may be
> worth discussing if there is any anything in common. His posts usually
> start with:
> 
>   I am working on optimization of feature requests #22873 (multiple fake
>   cursors) and #17684 (crosshairs that track the cursor position, and a
>   visible fill column indicator).

The approach there is very different, and personally I think it is not
the best route for the fill-column indication.

The main goal of Keith's feature is not the fill-column indication,
it's the "fake cursors", and the fill-column indicator is implemented
as one more "fake cursor" of sorts.  But if we want only the
fill-column indicator, implementing it as a cursor makes much less
sense, and is also significantly more complicated for no good reason.
Just look at the magnitude of the patch.  It's no coincidence that
this present implementation took so much less time and efforts than
the work on fake cursors.

Re: Fill column indicator functionality

[Ergus]

Hi Eli:

Sorry for being so annoying. But this is my first contribution and I am
still understanding the details on the way.

On Mon, Mar 11, 2019 at 05:30:47PM +0200, Eli Zaretskii wrote:
>
>I think you are missing this:
>
>      /* If last glyph is a space or stretch, and it's trailing
>	 whitespace, set the face of all trailing whitespace glyphs in
>	 IT->glyph_row to `trailing-whitespace'.  */
>      if ((row->reversed_p ? glyph <= start : glyph >= start)
>	  && BUFFERP (glyph->object)  <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>	  && (glyph->type == STRETCH_GLYPH
>	      || (glyph->type == CHAR_GLYPH
>		  && glyph->u.ch == ' '))
>	  && trailing_whitespace_p (glyph->charpos))
>
>This test is for those stretch glyphs that came from a buffer,
>i.e. those generated by TABs and 'space' display properties.  But in
>your case, the object of the stretch glyph is nil, as specified by the
>2nd argument of append_stretch_glyph:
>
>    append_stretch_glyph (it, Qnil, width,
>                          it->ascent + it->descent, stretch_ascent);
>
>Moreover, I believe you are looking at the wrong code fragment to
>explain why trailing-whitespace stopped working.  I think the problem
>happens a bit earlier, here:
>
>      /* Skip over glyphs inserted to display the cursor at the
>	 end of a line, for extending the face of the last glyph
>	 to the end of the line on terminals, and for truncation
>	 and continuation glyphs.  */
>      if (!row->reversed_p)
>	{
>	  while (glyph >= start
>		 && glyph->type == CHAR_GLYPH
>		 && NILP (glyph->object))
>	    --glyph;
>	}
>
I tried:
    if (!row->reversed_p)
	{
	  while (glyph >= start
	         && (glyph->type == CHAR_GLYPH
	             || glyph->type == STRETCH_GLYPH)
		 && NILP (glyph->object))
	    --glyph;
	}

But it didn't work. The behavior is the same: the whitespace is
highlighted only when the line is crossed.

The other corner case I have is because in graphical mode the space for
the dot is always reserved, so when the last character in the line is
just before the line, the line is not drawn for that line.

I can live with that, but maybe there is a simple fix?

In text mode I fixed this changing a while for a do while, but in
graphical mode the approach is different.

>> 2) What name do you suggest for this mode? I propose
>> display-column-indicator.
>
>display-fill-column-indicator-mode?

done.
>
>Thanks again for working on this.
>
>P.S. Thanks to you, I just uncovered an old bug, whereby
>show-trailing-whitespace didn't work in R2L screen lines, and for the
>same reason: the code didn't account for the stretch glyphs inserted
>by the display engine at the left edge of such lines.
>
Best,

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Tue, 12 Mar 2019 16:29:30 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> Sorry for being so annoying.

Nothing to be sorry about.

> I tried:
>     if (!row->reversed_p)
> 	{
> 	  while (glyph >= start
> 	         && (glyph->type == CHAR_GLYPH
> 	             || glyph->type == STRETCH_GLYPH)
> 		 && NILP (glyph->object))
> 	    --glyph;
> 	}
> 
> But it didn't work. The behavior is the same: the whitespace is
> highlighted only when the line is crossed.

I guess at this point I'd need to see the code to help you more.

Alternatively, you could step with a debugger through the code of
highlight_trailing_whitespace and see why it isn't working.  It should
be something simple, I think.

> The other corner case I have is because in graphical mode the space for
> the dot is always reserved, so when the last character in the line is
> just before the line, the line is not drawn for that line.

Sorry, I don't understand: what is "the dot" in this context, and what
do you mean by "just before the line"?  There are too many "lines" in
this sentence, so I'm confused.

> In text mode I fixed this changing a while for a do while, but in
> graphical mode the approach is different.

Well, it sounds like again seeing the code should explain what I don't
understand.

Re: Fill column indicator functionality

[Ergus]

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

Hi Eli:

Here I add a WIP patch for the current implementation so far.

Please tell me any problem you see related or not with my questions to
fix them too. 

Best,
Ergus



On Tue, Mar 12, 2019 at 06:19:34PM +0200, Eli Zaretskii wrote:
>> Date: Tue, 12 Mar 2019 16:29:30 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> Sorry for being so annoying.
>
>Nothing to be sorry about.
>
>> I tried:
>>     if (!row->reversed_p)
>> 	{
>> 	  while (glyph >= start
>> 	         && (glyph->type == CHAR_GLYPH
>> 	             || glyph->type == STRETCH_GLYPH)
>> 		 && NILP (glyph->object))
>> 	    --glyph;
>> 	}
>>
>> But it didn't work. The behavior is the same: the whitespace is
>> highlighted only when the line is crossed.
>
>I guess at this point I'd need to see the code to help you more.
>
>Alternatively, you could step with a debugger through the code of
>highlight_trailing_whitespace and see why it isn't working.  It should
>be something simple, I think.
>
>> The other corner case I have is because in graphical mode the space for
>> the dot is always reserved, so when the last character in the line is
>> just before the line, the line is not drawn for that line.
>
>Sorry, I don't understand: what is "the dot" in this context, and what
>do you mean by "just before the line"?  There are too many "lines" in
>this sentence, so I'm confused.
>
>> In text mode I fixed this changing a while for a do while, but in
>> graphical mode the approach is different.
>
>Well, it sounds like again seeing the code should explain what I don't
>understand.
>

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

diff --git a/lisp/cus-start.el b/lisp/cus-start.el
index baa05d0a89..441d3d5a9b 100644
--- a/lisp/cus-start.el
+++ b/lisp/cus-start.el
@@ -648,6 +648,15 @@ 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
+                                 (choice
+                                  (const :tag "Dynamically computed"
+                                         :value fill-column)
+                                  (integer :menu-tag "Fixed number of columns"
+                                           :value 70
+                                           :format "%v"))
+                                 "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..d8f73193af
--- /dev/null
+++ b/lisp/display-fill-column-indicator.el
@@ -0,0 +1,71 @@
+;;; 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'
+;; 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'."
+  :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)))
+    (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/frame.el b/lisp/frame.el
index dd1d5b030f..c0a0c3a903 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -2663,6 +2663,8 @@ 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
         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..928949f031 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -20305,7 +20305,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 +20360,47 @@ extend_face_to_end_of_line (struct it *it)
 	    }
 	}
 #ifdef HAVE_WINDOW_SYSTEM
+
+      if (!NILP (Vdisplay_fill_column_indicator))
+        {
+	  const int fill_column_indicator_column =
+		  XFIXNAT (Vdisplay_fill_column_indicator_column) - 1;
+
+	  const int current_x = it->current_x;
+	  const int column_x = it->pixel_width * fill_column_indicator_column
+	                       + it->lnum_pixel_width;
+
+	  if ((current_x <= column_x)
+	      && (column_x < it->last_visible_x))
+	    {
+	      struct font *font = face->font ? face->font : FRAME_FONT (f);
+	      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;
+	      const int width = column_x - current_x;
+
+	      it->char_to_display = '|';
+	      int stretch_ascent = (((it->ascent + it->descent)
+		    * FONT_BASE (font)) / FONT_HEIGHT (font));
+
+	      memset (&it->position, 0, sizeof it->position);
+	      it->avoid_cursor_p = true;
+	      it->face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID);
+	      it->start_of_box_run_p = false;
+	      append_stretch_glyph (it, Qnil, 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;
+	    }
+        }
       if (it->glyph_row->reversed_p)
 	{
 	  /* Prepend a stretch glyph to the row, such that the
@@ -20478,10 +20520,34 @@ 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, Qline_number, 0, DEFAULT_FACE_ID);
+		  it->c = it->char_to_display = '|';
+		  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 +20637,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;
 	}
@@ -33213,6 +33281,19 @@ 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 line.  */);
+  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 where to draw the column indicator line when display-column-indicator
+is non-nil .  */);
+  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_BOOL ("inhibit-eval-during-redisplay", inhibit_eval_during_redisplay,
     doc: /* Non-nil means don't eval Lisp during redisplay.  */);
   inhibit_eval_during_redisplay = false;

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Tue, 12 Mar 2019 20:20:17 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> Please tell me any problem you see related or not with my questions to
> fix them too. 

Thanks, see below.

> >> I tried:
> >>     if (!row->reversed_p)
> >> 	{
> >> 	  while (glyph >= start
> >> 	         && (glyph->type == CHAR_GLYPH
> >> 	             || glyph->type == STRETCH_GLYPH)
> >> 		 && NILP (glyph->object))
> >> 	    --glyph;
> >> 	}
> >>
> >> But it didn't work. The behavior is the same: the whitespace is
> >> highlighted only when the line is crossed.
> >
> >I guess at this point I'd need to see the code to help you more.

The problem here is that the above loop also looks at the object from
which the glyph came, and will only skip glyphs whose object is nil.
But in your code, you didn't set up the object of the '|' character,
which comes from it->object, to be nil, you left it at its previous
value, which usually will be either a buffer or a string, depending
from where the last character of the text line came.

To fix this, add this:

  Lisp_Object save_object = it->object;
  it->object = Qnil;

before the call to PRODUCE_GLYPHS, and then restore it->object to its
previous value after PRODUCE_GLYPHS.

In the TTY case, this was already done for you by the code which
extends the face, that's why it worked on TTY frames.

> >> The other corner case I have is because in graphical mode the space for
> >> the dot is always reserved, so when the last character in the line is
> >> just before the line, the line is not drawn for that line.
> >
> >Sorry, I don't understand: what is "the dot" in this context, and what
> >do you mean by "just before the line"?  There are too many "lines" in
> >this sentence, so I'm confused.

I guess you mean that when the place for the cursor is at fill-column,
the indicator is not visible?  I think to fix this, you will need to
modify the code in the function append_space_for_newline, which is
responsible for adding the space glyph for displaying the cursor at
the end of the line.  That function currently adds a ' ' space
character there; you want to replace that with the indicator character
'|', when this mode is in effect.

A few more comments regarding the code:

> +      if (!NILP (Vdisplay_fill_column_indicator))
> +        {
> +	  const int fill_column_indicator_column =
> +		  XFIXNAT (Vdisplay_fill_column_indicator_column) - 1;

Why "- 1"?  It's confusing, and should at least be explained in a
comment, if not changed to not subtract 1.

Also, it is unsafe to call XFIXNAT without verifying the value is a
fixnum, because if it isn't, Emacs will crash.

> +	  const int column_x = it->pixel_width * fill_column_indicator_column
> +	                       + it->lnum_pixel_width;

Using it->pixel_width here will not work in general, as this is just
the pixel width of the last character or image of the screen line.  I
think you should use the font->average_width, and if that is zero,
then font->space_width.  Here 'font' is the default face's font.  This
is because I believe we want to display the indicator at the X
coordinate that corresponds to fill-column in units of the default
font width, right?  I mean, did you try to see what happens when the
text has faces which make some of the characters appear smaller or
larger than the default face?

> +	      struct font *font = face->font ? face->font : FRAME_FONT (f);

I think this uses the wrong face: you should be using default_face
instead.  The above is the face of the last character of the line.

> +	      it->char_to_display = '|';

This character should be customizable.  Perhaps make
display-fill-column-indicator serve double duty: if non-nil, it should
be the character to use as the indicator.

> +	      it->face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID);

You didn't really mean to use the line-number face here, did you?
This should probably be a separate distinct face.

> +  DEFVAR_LISP ("display-fill-column-indicator", Vdisplay_fill_column_indicator,
> +    doc: /* Non-nil means display the fill column indicator line.  */);

The "line" part of the doc string doesn't belong there, I think.  Just
"indicator" is enough.

> +  DEFVAR_LISP ("display-fill-column-indicator-column", Vdisplay_fill_column_indicator_column,
> +    doc: /* Column where to draw the column indicator line when display-column-indicator
> +is non-nil .  */);

The first line of a doc string should be a complete sentence, and it
should fit the 80column display line.  In this case, I'd shorten it
like this:

  Column to draw the indicator when `display-fill-column-indicator' is non-nil.

Please also say in the doc string what are the possible values and in
what column units they are measured.

Thanks for working on this.

Re: Fill column indicator functionality

[Ergus]

Hi Eli:

On Wed, Mar 13, 2019 at 06:19:40PM +0200, Eli Zaretskii wrote:
>> Date: Tue, 12 Mar 2019 20:20:17 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> Please tell me any problem you see related or not with my questions to
>> fix them too.
>
>Thanks, see below.
>
>> >> I tried:
>> >>     if (!row->reversed_p)
>> >> 	{
>> >> 	  while (glyph >= start
>> >> 	         && (glyph->type == CHAR_GLYPH
>> >> 	             || glyph->type == STRETCH_GLYPH)
>> >> 		 && NILP (glyph->object))
>> >> 	    --glyph;
>> >> 	}
>> >>
>> >> But it didn't work. The behavior is the same: the whitespace is
>> >> highlighted only when the line is crossed.
>> >
>> >I guess at this point I'd need to see the code to help you more.
>
>The problem here is that the above loop also looks at the object from
>which the glyph came, and will only skip glyphs whose object is nil.
>But in your code, you didn't set up the object of the '|' character,
>which comes from it->object, to be nil, you left it at its previous
>value, which usually will be either a buffer or a string, depending
>from where the last character of the text line came.
>
>To fix this, add this:
>
>  Lisp_Object save_object = it->object;
>  it->object = Qnil;
>
>before the call to PRODUCE_GLYPHS, and then restore it->object to its
>previous value after PRODUCE_GLYPHS.
>
I thought that append_stretch_glyph was putting the it->object to Now I
see it didn't.

>In the TTY case, this was already done for you by the code which
>extends the face, that's why it worked on TTY frames.
>
>> >> The other corner case I have is because in graphical mode the space for
>> >> the dot is always reserved, so when the last character in the line is
>> >> just before the line, the line is not drawn for that line.
>> >
>> >Sorry, I don't understand: what is "the dot" in this context, and what
>> >do you mean by "just before the line"?  There are too many "lines" in
>> >this sentence, so I'm confused.
>
>I guess you mean that when the place for the cursor is at fill-column,
>the indicator is not visible?  I think to fix this, you will need to
>modify the code in the function append_space_for_newline, which is
>responsible for adding the space glyph for displaying the cursor at
>the end of the line.  That function currently adds a ' ' space
>character there; you want to replace that with the indicator character
>'|', when this mode is in effect.
>
Yes I was referring to cursor as point. My bad.

>A few more comments regarding the code:
>
>> +      if (!NILP (Vdisplay_fill_column_indicator))
>> +        {
>> +	  const int fill_column_indicator_column =
>> +		  XFIXNAT (Vdisplay_fill_column_indicator_column) - 1;
>
>Why "- 1"?  It's confusing, and should at least be explained in a
>comment, if not changed to not subtract 1.
>
The -1 is related with the extra ' ' added by the extend_face function,
I will try to avoid it, in the final patch, but now this is just a prove
of concept.

>Also, it is unsafe to call XFIXNAT without verifying the value is a
>fixnum, because if it isn't, Emacs will crash.
>
>> +	  const int column_x = it->pixel_width * fill_column_indicator_column
>> +	                       + it->lnum_pixel_width;
>

Ok, I'll prevent this.

>Using it->pixel_width here will not work in general, as this is just
>the pixel width of the last character or image of the screen line.  I
>think you should use the font->average_width, and if that is zero,
>then font->space_width.  Here 'font' is the default face's font.  This
>is because I believe we want to display the indicator at the X
>coordinate that corresponds to fill-column in units of the default
>font width, right?  I mean, did you try to see what happens when the
>text has faces which make some of the characters appear smaller or
>larger than the default face?
>
>> +	      struct font *font = face->font ? face->font : FRAME_FONT (f);
>
You just saved me a lot of time of reading the font and face structs
definitions (I will but not now).

>I think this uses the wrong face: you should be using default_face
>instead.  The above is the face of the last character of the line.
>
>> +	      it->char_to_display = '|';
>
>This character should be customizable.  Perhaps make
>display-fill-column-indicator serve double duty: if non-nil, it should
>be the character to use as the indicator.
>

It is customizable actually in my current version, I just sent you the
basic changes to make it work that exposed the issues with minimal code
changes. In fact the default value will be ?\u2402 (and not ?| as in the
example) if the font supports it because it will look like a contiguous
line.

>> +	      it->face_id = merge_faces (it->w, Qline_number, 0, DEFAULT_FACE_ID);
>
>You didn't really mean to use the line-number face here, did you?
>This should probably be a separate distinct face.
>

Yes, I did, this was a prove of concept. I created a fill_column face.


>> +  DEFVAR_LISP ("display-fill-column-indicator", Vdisplay_fill_column_indicator,
>> +    doc: /* Non-nil means display the fill column indicator line.  */);
>
>The "line" part of the doc string doesn't belong there, I think.  Just
>"indicator" is enough.
>
>> +  DEFVAR_LISP ("display-fill-column-indicator-column", Vdisplay_fill_column_indicator_column,
>> +    doc: /* Column where to draw the column indicator line when display-column-indicator
>> +is non-nil .  */);
>
>The first line of a doc string should be a complete sentence, and it
>should fit the 80column display line.  In this case, I'd shorten it
>like this:
>
>  Column to draw the indicator when `display-fill-column-indicator' is non-nil.
>
>Please also say in the doc string what are the possible values and in
>what column units they are measured.
>
Ok, perfect

>Thanks for working on this.

1E6 Thanks.
Ergus

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Wed, 13 Mar 2019 21:02:25 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> >  Lisp_Object save_object = it->object;
> >  it->object = Qnil;
> >
> >before the call to PRODUCE_GLYPHS, and then restore it->object to its
> >previous value after PRODUCE_GLYPHS.
> >
> I thought that append_stretch_glyph was putting the it->object to Now I
> see it didn't.

append_stretch_glyph only does that for the stretch glyph it creates,
it doesn't change it->object.

> 1E6 Thanks.

My pleasure.

Re: Fill column indicator functionality

[Ergus]

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

Here is a patch with Eli's fixes and corrections:

Actually I have 3 more questions:

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. What's the
proper way to change the font size or the character itself to fit the
entire row height in the graphical mode?

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?

3) Actually there are 3 possible configuration options the column
number, the character, and the font. Should we provide some other
alternative?

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.

On Wed, Mar 13, 2019 at 10:09:42PM +0200, Eli Zaretskii wrote:
>> Date: Wed, 13 Mar 2019 21:02:25 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> >  Lisp_Object save_object = it->object;
>> >  it->object = Qnil;
>> >
>> >before the call to PRODUCE_GLYPHS, and then restore it->object to its
>> >previous value after PRODUCE_GLYPHS.
>> >
>> I thought that append_stretch_glyph was putting the it->object to Now I
>> see it didn't.
>
>append_stretch_glyph only does that for the stretch glyph it creates,
>it doesn't change it->object.
>
>> 1E6 Thanks.
>
>My pleasure.
>

[-- Attachment #2: display-fill-column-indicator_wip.patch --]
[-- Type: text/plain, Size: 16093 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..846466a2c3
--- /dev/null
+++ b/lisp/display-fill-column-indicator.el
@@ -0,0 +1,74 @@
+;;; 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'
+;; 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
+          (setq display-fill-column-indicator-character ?\u2502)))
+    (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..153e6a208f 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 linum.el.
+(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..8ac4be8dc7 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -20135,15 +20135,41 @@ 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 +20178,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;
+		  it->end_of_box_run_p = false;
 
+	produce_glyphs:
 	  PRODUCE_GLYPHS (it);
 
 #ifdef HAVE_WINDOW_SYSTEM
@@ -20305,7 +20332,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 +20387,55 @@ 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 +20555,34 @@ 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 +20672,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 +32748,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 faces 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 +33319,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 ?\u2502 the but a good alternative is (ascii 124) if
+yours font doesn't support 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;

Re: Fill column indicator functionality

[Eli Zaretskii]

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

Re: Fill column indicator functionality

[Ergus]

[-- 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;

Re: Fill column indicator functionality

[Andreas Schwab]

On Mär 14 2019, Ergus <spacibba@aol.com> wrote:

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

gitignore is irrelevant for tracked files.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: Fill column indicator functionality

[Eli Zaretskii]

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

Re: Fill column indicator functionality

[Ergus]

On Fri, Mar 15, 2019 at 09:52:53AM +0200, Eli Zaretskii wrote:
>> Date: Thu, 14 Mar 2019 22:13:15 +0100
>> From: Ergus <spacibba@aol.com>
>>
>Can you solve this problem by using the :height attribute whose value
>is 1.0 (a floating-point number) for the face of the character?  If
>that doesn't work, we might introduce a special feature for this use
>case.
>

I'll try this in a while to see how the experience changes. If we set
that this way the font will autoresize depending of the height of the
current column?

Anyway I think the discussion in progress can give a different answer. I
will wait a bit more to get a recommendation. Or a solution that promise
to be better.

>In any case, I think this use case is rare enough to leave it for
>subsequent improvements.  It doesn't sound as show-stopper to me.
>
>> Or use a different type of glyph in graphical mode... maybe an image as
>> the fci package does? Is that possible?
>
>It should be possible to use an image, yes.  Does the fci package
>solve the use case of lines that are taller than default, as in Info
>or EWW?  If so, what do they do -- use a larger slice of an image
>according to the line height or something?  Because using an image
>still needs to solve the problem of displaying a taller image when the
>line's height is larger than the default.
>

The XPM image seems to be a good enough approach according to Alp's
comment/experience, but it only solves a part of the problem. But I
haven't go in the details of what they do and I don't want to over
specify the functionality or produce too much overhead.

>One more potential issue that I think you should test is when the
>fringes are disabled on GUI frames, and we usurp the last column of
>the text area to display continuation and truncation glyphs there, as
>we do on TTY frames.  If the fill-column is equal to the last column
>of the window, or to one before the last, I think you will need to
>augment your comparison with it->last_visible_x in this case.
>
The code that adds the information looks to run later, so the line is
hidden in that case, which for me is the expected behavior. Otherwise I
didn't understand the issue.

Apart from that there, are some corrections I made in order to fix some
background issues I observed in graphical mode.

In graphical interfaces the space after the line is always filled with
the background color of the face of the last produced glyph, even if I
reset it to the saved value after the generation. The only solution I
found so far was to add an extra glyph after reset the face to the
default (saved) value, but hopefully there is a better way?

Re: Fill column indicator functionality

[Óscar Fuentes]

Ergus <spacibba@aol.com> writes:

> Apart from that there, are some corrections I made in order to fix some
> background issues I observed in graphical mode.

One thing I just noticed:

emacs -Q

A graphical frame is shown with the toolbar. In *scratch*

M-x display-fill-column-indicator-mode

Now the part of the toolbar that is not covered by icons has white
background.

Switch to *Messages*. The toolbar is shown normally. Switch back to
*scratch*. The toolbar is partially white again.

Re: Fill column indicator functionality

[Ergus]

Hi Oscar:

I'm trying to reproduce the issue you describe, but I don't see it in my
system.

I will send the corrected patch soon (any way my changes can't be merged
yet); but I am not sure that my work is related with toolbars
issues. Please if you can give some extra information about your system
it will be usefull. Specially the graphical interface and/or OS, because I
only tested GNU/Linux.

Best,
Ergus

On Fri, Mar 15, 2019 at 11:52:52PM +0100, �scar Fuentes wrote:
>Ergus <spacibba@aol.com> writes:
>
>> Apart from that there, are some corrections I made in order to fix some
>> background issues I observed in graphical mode.
>
>One thing I just noticed:
>
>emacs -Q
>
>A graphical frame is shown with the toolbar. In *scratch*
>
>M-x display-fill-column-indicator-mode
>
>Now the part of the toolbar that is not covered by icons has white
>background.
>
>Switch to *Messages*. The toolbar is shown normally. Switch back to
>*scratch*. The toolbar is partially white again.
>
>

Re: Fill column indicator functionality

[Óscar Fuentes]

Ergus <spacibba@aol.com> writes:

> Hi Oscar:
>
> I'm trying to reproduce the issue you describe, but I don't see it in my
> system.
>
> I will send the corrected patch soon (any way my changes can't be merged
> yet); but I am not sure that my work is related with toolbars
> issues. Please if you can give some extra information about your system
> it will be usefull. Specially the graphical interface and/or OS, because I
> only tested GNU/Linux.
>
> Best,
> Ergus

Following is the information as reported by `report-emacs-bug'. I'll send
you an screenshot off-list.

In GNU Emacs 27.0.50 (build 3, x86_64-pc-linux-gnu, X toolkit)
 of 2019-03-14 built on sky
Repository revision: 020e69d992c98fd852e835c9bd707a8d137090f2
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: Debian GNU/Linux buster/sid

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Display-Fill-Column-Indicator mode enabled in current buffer
You can run the command ‘display-fill-column-indicator-mode’ with M-x di-i RET
Display-Fill-Column-Indicator mode enabled in current buffer

Configured using:
 'configure --without-toolkit-scroll-bars --with-x-toolkit=lucid
 --with-modules'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG SOUND GSETTINGS GLIB NOTIFY INOTIFY
LIBSELINUX GNUTLS LIBXML2 FREETYPE LIBOTF XFT ZLIB LUCID X11 XDBE XIM
MODULES THREADS PDUMPER GMP

Important settings:
  value of $LANG: es_ES.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  display-fill-column-indicator-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv dired dired-loaddefs format-spec rfc822 mml
easymenu mml-sec password-cache epa derived epg epg-config gnus-util
rmail rmail-loaddefs time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs cl-lib sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
display-fill-column-indicator elec-pair mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote threads inotify dynamic-setting system-font-setting
font-render-setting x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 45167 5012)
 (symbols 48 5846 1)
 (strings 32 15616 1682)
 (string-bytes 1 501739)
 (vectors 16 9468)
 (vector-slots 8 117448 9386)
 (floats 8 19 27)
 (intervals 56 212 0)
 (buffers 992 11))

Re: Fill column indicator functionality

[Ergus]

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

Hi Oscar:

I think that I found it. So I will try to correct it in the patch update.



On 16 March 2019 00:47:16 CET, "Óscar Fuentes" <ofv@wanadoo.es> wrote:
>Ergus <spacibba@aol.com> writes:
>
>> Hi Oscar:
>>
>> I'm trying to reproduce the issue you describe, but I don't see it in
>my
>> system.
>>
>> I will send the corrected patch soon (any way my changes can't be
>merged
>> yet); but I am not sure that my work is related with toolbars
>> issues. Please if you can give some extra information about your
>system
>> it will be usefull. Specially the graphical interface and/or OS,
>because I
>> only tested GNU/Linux.
>>
>> Best,
>> Ergus
>
>Following is the information as reported by `report-emacs-bug'. I'll
>send
>you an screenshot off-list.
>
>In GNU Emacs 27.0.50 (build 3, x86_64-pc-linux-gnu, X toolkit)
> of 2019-03-14 built on sky
>Repository revision: 020e69d992c98fd852e835c9bd707a8d137090f2
>Repository branch: master
>Windowing system distributor 'The X.Org Foundation', version
>11.0.12003000
>System Description: Debian GNU/Linux buster/sid
>
>Recent messages:
>For information about GNU Emacs and the GNU system, type C-h C-a.
>Display-Fill-Column-Indicator mode enabled in current buffer
>You can run the command ‘display-fill-column-indicator-mode’ with M-x
>di-i RET
>Display-Fill-Column-Indicator mode enabled in current buffer
>
>Configured using:
> 'configure --without-toolkit-scroll-bars --with-x-toolkit=lucid
> --with-modules'
>
>Configured features:
>XAW3D XPM JPEG TIFF GIF PNG RSVG SOUND GSETTINGS GLIB NOTIFY INOTIFY
>LIBSELINUX GNUTLS LIBXML2 FREETYPE LIBOTF XFT ZLIB LUCID X11 XDBE XIM
>MODULES THREADS PDUMPER GMP
>
>Important settings:
>  value of $LANG: es_ES.UTF-8
>  locale-coding-system: utf-8-unix
>
>Major mode: Lisp Interaction
>
>Minor modes in effect:
>  display-fill-column-indicator-mode: t
>  tooltip-mode: t
>  global-eldoc-mode: t
>  eldoc-mode: t
>  electric-indent-mode: t
>  mouse-wheel-mode: t
>  tool-bar-mode: t
>  menu-bar-mode: t
>  file-name-shadow-mode: t
>  global-font-lock-mode: t
>  font-lock-mode: t
>  blink-cursor-mode: t
>  auto-composition-mode: t
>  auto-encryption-mode: t
>  auto-compression-mode: t
>  line-number-mode: t
>  transient-mark-mode: t
>
>Load-path shadows:
>None found.
>
>Features:
>(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
>bytecomp byte-compile cconv dired dired-loaddefs format-spec rfc822 mml
>easymenu mml-sec password-cache epa derived epg epg-config gnus-util
>rmail rmail-loaddefs time-date mm-decode mm-bodies mm-encode mail-parse
>rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs cl-lib sendmail
>rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
>display-fill-column-indicator elec-pair mule-util tooltip eldoc
>electric
>uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
>term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
>tabulated-list replace newcomment text-mode elisp-mode lisp-mode
>prog-mode register page menu-bar rfn-eshadow isearch timer select
>scroll-bar mouse jit-lock font-lock syntax facemenu font-core
>term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
>vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
>hebrew greek romanian slovak czech european ethiopic indian cyrillic
>chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
>help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
>button faces cus-face macroexp files text-properties overlay sha1 md5
>base64 format env code-pages mule custom widget
>hashtable-print-readable
>backquote threads inotify dynamic-setting system-font-setting
>font-render-setting x-toolkit x multi-tty make-network-process emacs)
>
>Memory information:
>((conses 16 45167 5012)
> (symbols 48 5846 1)
> (strings 32 15616 1682)
> (string-bytes 1 501739)
> (vectors 16 9468)
> (vector-slots 8 117448 9386)
> (floats 8 19 27)
> (intervals 56 212 0)
> (buffers 992 11))

[-- Attachment #2: Type: text/html, Size: 4555 bytes --]

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Sat, 16 Mar 2019 00:22:50 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> I am not sure that my work is related with toolbars issues.

In some configuration, where Emacs itself draws the tool bar (as
opposed to the toolkit doing that), the tool bar is a special kind of
window, and redisplay handles that window as well.  Your code should
refrain from producing the indicator in such windows.  I wrote in my
previous message how I propose to do that.

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Óscar Fuentes <ofv@wanadoo.es>
> Date: Fri, 15 Mar 2019 23:52:52 +0100
> 
> emacs -Q
> 
> A graphical frame is shown with the toolbar. In *scratch*
> 
> M-x display-fill-column-indicator-mode
> 
> Now the part of the toolbar that is not covered by icons has white
> background.
> 
> Switch to *Messages*. The toolbar is shown normally. Switch back to
> *scratch*. The toolbar is partially white again.

I think the code which displays the indicator should not do that if
it->w->pseudo_window_p is non-zero.  IOW, if it->w->pseudo_window_p is
non-zero, extend_face_to_end_of_line should returns without producing
the glyphs for the stretch and the indicator character.

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Fri, 15 Mar 2019 21:56:26 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> Anyway I think the discussion in progress can give a different answer. I
> will wait a bit more to get a recommendation. Or a solution that promise
> to be better.

OK, we are waiting for your legal paperwork anyway.

> >One more potential issue that I think you should test is when the
> >fringes are disabled on GUI frames, and we usurp the last column of
> >the text area to display continuation and truncation glyphs there, as
> >we do on TTY frames.  If the fill-column is equal to the last column
> >of the window, or to one before the last, I think you will need to
> >augment your comparison with it->last_visible_x in this case.
> >
> The code that adds the information looks to run later, so the line is
> hidden in that case, which for me is the expected behavior. Otherwise I
> didn't understand the issue.

My concern was that it->last_visible_x is not the last pixel you can
use for the indicator, because there must be space left for the
continuation and truncation glyphs.  But if you tested that use case
and the code works, I'm happy.

> In graphical interfaces the space after the line is always filled with
> the background color of the face of the last produced glyph, even if I
> reset it to the saved value after the generation. The only solution I
> found so far was to add an extra glyph after reset the face to the
> default (saved) value, but hopefully there is a better way?

I don't think I understand what face is "the face of the last produced
glyph".  is that the face of the indicator character, is that the
default face, or is that something else?  Can you show a screenshot?

Re: Fill column indicator functionality

[Alp Aker]

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

>> In graphical interfaces the space after the line is always filled with
>> the background color of the face of the last produced glyph, even if I
>> reset it to the saved value after the generation. The only solution I
>> found so far was to add an extra glyph after reset the face to the
>> default (saved) value, but hopefully there is a better way?
> I don't think I understand what face is "the face of the last produced
> glyph".  is that the face of the indicator character, is that the
> default face, or is that something else?  Can you show a screenshot?
I noticed an issue with non-default backgrounds that span newlines, such 
as happens with region highlighting. I believe I ran into what Ergus is 
describing while working on a fix (see (3) below).  The version of the 
code I looked at was 9dcaa15e5a, from Ergus's Github repo.

You can see the issue with non-default backgrounds in the attached 
screenshots mode-off.png and mode-on.png; activating 
display-fill-column-indicator-mode truncates the highlighting on each 
line.  To fix:

1. The stretch glyph needs to be drawn in the current face, not the 
fill_column face.

2. The default fill-column face should have an unspecified background 
and it should be merged into the current face during display, not into 
the default face.

3. If the fill-column face specifies a background, we need to reset the 
face to the saved face after producing the indicator glyph. Here I found 
it necessary to insert another display element in order for the face 
change to take effect before the background is extended to the end of 
the line. (I used a 0-width stretch glyph.)  Without that, the 
fill-column face is used (see the attached c.png for a screenshot).  I 
believe this need to add another display element at the end of the line 
is what Ergus was asking about.


diff --git a/lisp/faces.el b/lisp/faces.el
index 153e6a208f..6b9980a77f 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2504,7 +2504,7 @@ line-number-current-line

  ;; Definition stolen from linum.el.
  (defface fill-column
-  '((t :inherit (shadow default)))
+  '((t :inherit (shadow)))
    "Face for displaying fill column indicator line.
  This face is used when `display-fill-column-indicator-mode' is
  non-nil.

diff --git a/src/xdisp.c b/src/xdisp.c
index 8ac4be8dc7..7c4f9889eb 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -20416,21 +20416,23 @@ extend_face_to_end_of_line (struct it *it)
            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);

+          it->char_to_display = 
XFIXNAT(Vdisplay_fill_column_indicator_character);
+          it->face_id = merge_faces (it->w, Qfill_column, 0, 
saved_face_id);
            PRODUCE_GLYPHS (it);

+          it->face_id = saved_face_id;
+          append_stretch_glyph (it, Qnil, 0, it->ascent + it->descent,
+                             stretch_ascent);
            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;
@@ -20566,7 +20568,7 @@ extend_face_to_end_of_line (struct it *it)
            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->face_id = merge_faces (it->w, Qfill_column, 0, saved_face);
            it->c = it->char_to_display = 
XFIXNAT(Vdisplay_fill_column_indicator_character);
            PRODUCE_GLYPHS (it);
            it->face_id = saved_face;


[-- Attachment #2: c.png --]
[-- Type: image/png, Size: 34786 bytes --]

[-- Attachment #3: mode-on.png --]
[-- Type: image/png, Size: 48715 bytes --]

[-- Attachment #4: mode-off.png --]
[-- Type: image/png, Size: 47731 bytes --]

Re: Fill column indicator functionality

[Ergus]

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

Hi

The version on github is outdated. I have a workaround for that backgrownd issue in a patch I sent to Eli (in this mailing list) on yesterday. Please try that one and check if you experience the same problems.
I will update the github code latter because i am doing some changes in the lisp interface now.

On 17 March 2019 18:28:22 CET, Alp Aker <alp.tekin.aker@gmail.com> wrote:
>>> In graphical interfaces the space after the line is always filled
>with
>>> the background color of the face of the last produced glyph, even if
>I
>>> reset it to the saved value after the generation. The only solution
>I
>>> found so far was to add an extra glyph after reset the face to the
>>> default (saved) value, but hopefully there is a better way?
>> I don't think I understand what face is "the face of the last
>produced
>> glyph".  is that the face of the indicator character, is that the
>> default face, or is that something else?  Can you show a screenshot?
>I noticed an issue with non-default backgrounds that span newlines,
>such 
>as happens with region highlighting. I believe I ran into what Ergus is
>
>describing while working on a fix (see (3) below).  The version of the 
>code I looked at was 9dcaa15e5a, from Ergus's Github repo.
>
>You can see the issue with non-default backgrounds in the attached 
>screenshots mode-off.png and mode-on.png; activating 
>display-fill-column-indicator-mode truncates the highlighting on each 
>line.  To fix:
>
>1. The stretch glyph needs to be drawn in the current face, not the 
>fill_column face.
>
>2. The default fill-column face should have an unspecified background 
>and it should be merged into the current face during display, not into 
>the default face.
>
>3. If the fill-column face specifies a background, we need to reset the
>
>face to the saved face after producing the indicator glyph. Here I
>found 
>it necessary to insert another display element in order for the face 
>change to take effect before the background is extended to the end of 
>the line. (I used a 0-width stretch glyph.)  Without that, the 
>fill-column face is used (see the attached c.png for a screenshot).  I 
>believe this need to add another display element at the end of the line
>
>is what Ergus was asking about.
>
>
>diff --git a/lisp/faces.el b/lisp/faces.el
>index 153e6a208f..6b9980a77f 100644
>--- a/lisp/faces.el
>+++ b/lisp/faces.el
>@@ -2504,7 +2504,7 @@ line-number-current-line
>
>  ;; Definition stolen from linum.el.
>  (defface fill-column
>-  '((t :inherit (shadow default)))
>+  '((t :inherit (shadow)))
>    "Face for displaying fill column indicator line.
>  This face is used when `display-fill-column-indicator-mode' is
>  non-nil.
>
>diff --git a/src/xdisp.c b/src/xdisp.c
>index 8ac4be8dc7..7c4f9889eb 100644
>--- a/src/xdisp.c
>+++ b/src/xdisp.c
>@@ -20416,21 +20416,23 @@ extend_face_to_end_of_line (struct it *it)
>            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);
>
>+          it->char_to_display = 
>XFIXNAT(Vdisplay_fill_column_indicator_character);
>+          it->face_id = merge_faces (it->w, Qfill_column, 0, 
>saved_face_id);
>            PRODUCE_GLYPHS (it);
>
>+          it->face_id = saved_face_id;
>+          append_stretch_glyph (it, Qnil, 0, it->ascent + it->descent,
>+                             stretch_ascent);
>            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;
>@@ -20566,7 +20568,7 @@ extend_face_to_end_of_line (struct it *it)
>            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->face_id = merge_faces (it->w, Qfill_column, 0,
>saved_face);
>            it->c = it->char_to_display = 
>XFIXNAT(Vdisplay_fill_column_indicator_character);
>            PRODUCE_GLYPHS (it);
>            it->face_id = saved_face;

[-- Attachment #2: Type: text/html, Size: 7007 bytes --]

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Alp Aker <alp.tekin.aker@gmail.com>
> Date: Sun, 17 Mar 2019 13:28:22 -0400
> 
> I noticed an issue with non-default backgrounds that span newlines, such 
> as happens with region highlighting.

That's known, but in the case in point the non-default background
doesn't span newlines: the first character on the next line doesn't
have the same background as the indicator character.

Re: Fill column indicator functionality

[Ergus]

On Fri, Mar 15, 2019 at 09:52:53AM +0200, Eli Zaretskii wrote:

> Can you solve this problem by using the :height attribute whose value
> is 1.0 (a floating-point number) for the face of the character?  If
> that doesn't work, we might introduce a special feature for this use
> case.
> 

Hi:

I tried that solution (setting the :height attribute to 1.0 in the face
declaration) and it didn't work.

```
(defface fill-column-face
 '((t :inherit (shadow default) :height 1.0))
 "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)
```

There is still a small space and for rows with different size fonts it
keeps always the same size.

I see that there is a calc_line_height_property; can we use that to
change the glyph. Maybe it is possible to implement a setter (equivalent
to get_it_property?

We can compare the line height with the it height and change it
temporarily to print the indicator?

Does it makes sense?

Question about documented functions

[Ergus]

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

Hi. I have a question. The functions/macros to work with lisp variables from C in lisp.h header file are documented somewhere?

[-- Attachment #2: Type: text/html, Size: 126 bytes --]

Re: Question about documented functions

[Eli Zaretskii]

> Date: Sat, 16 Mar 2019 11:18:47 +0100
> From: Ergus <spacibba@aol.com>
> 
> Hi. I have a question. The functions/macros to work with lisp variables from C in lisp.h header file are
> documented somewhere?

Only in lisp.h itself.  If something there is not clear enough, we
could improve the commentary.

Re: Question about documented functions

[Ergus]

On Sat, Mar 16, 2019 at 02:21:37PM +0200, Eli Zaretskii wrote:
>> Date: Sat, 16 Mar 2019 11:18:47 +0100
>> From: Ergus <spacibba@aol.com>
>>
>> Hi. I have a question. The functions/macros to work with lisp variables from C in lisp.h header file are
>> documented somewhere?
>
>Only in lisp.h itself.  If something there is not clear enough, we
>could improve the commentary.

The problem is that I was trying to figure out the canonical method to
check if a variable was: t from C code (or assign t to a "DEFVAR_LISP"
defined variable). I was looking for something similar to NILP but I
couldn't get it.

For example:

#if DEFINE_KEY_OPS_AS_MACROS
# define XLI(o) lisp_h_XLI (o)
# define XIL(i) lisp_h_XIL (i)
# define XLP(o) lisp_h_XLP (o)
# define XPL(p) lisp_h_XPL (p)
# define CHECK_FIXNUM(x) lisp_h_CHECK_FIXNUM (x)
# define CHECK_SYMBOL(x) lisp_h_CHECK_SYMBOL (x)
# define CHECK_TYPE(ok, predicate, x) lisp_h_CHECK_TYPE (ok, predicate, x)
# define CONSP(x) lisp_h_CONSP (x)
# define EQ(x, y) lisp_h_EQ (x, y)
# define FLOATP(x) lisp_h_FLOATP (x)
# define FIXNUMP(x) lisp_h_FIXNUMP (x)
# define NILP(x) lisp_h_NILP (x)
# define SET_SYMBOL_VAL(sym, v) lisp_h_SET_SYMBOL_VAL (sym, v)
# define SYMBOL_CONSTANT_P(sym) lisp_h_SYMBOL_CONSTANT_P (sym)
# define SYMBOL_TRAPPED_WRITE_P(sym) lisp_h_SYMBOL_TRAPPED_WRITE_P (sym)
# define SYMBOL_VAL(sym) lisp_h_SYMBOL_VAL (sym)
# define SYMBOLP(x) lisp_h_SYMBOLP (x)
# define TAGGEDP(a, tag) lisp_h_TAGGEDP (a, tag)
# define VECTORLIKEP(x) lisp_h_VECTORLIKEP (x)
# define XCAR(c) lisp_h_XCAR (c)
# define XCDR(c) lisp_h_XCDR (c)
# define XCONS(a) lisp_h_XCONS (a)
# define XHASH(a) lisp_h_XHASH (a)
# ifndef GC_CHECK_CONS_LIST
#  define check_cons_list() lisp_h_check_cons_list ()
# endif
# if USE_LSB_TAG
#  define make_fixnum(n) lisp_h_make_fixnum (n)
#  define XFIXNAT(a) lisp_h_XFIXNAT (a)
#  define XFIXNUM(a) lisp_h_XFIXNUM (a)
#  define XSYMBOL(a) lisp_h_XSYMBOL (a)
#  define XTYPE(a) lisp_h_XTYPE (a)
# endif
#endif

It will be helpful for new people (like me), some explanations about
each of them. Expected arguments, and or objectives. And comments like
XFIXNUM can crash so it is needed to protect the code with an if
FIXNUMP. And then what CHECK_FIXNUM is useful for.

I know I need to read more, but it was a bit confusing to figure out the
difference between FIXNUM and FIXNAT.

And functions that are lisp equivalents (CAR CRD) could say that so
newbies can refer to the elisp manual to look for it.

Re: Question about documented functions

[Eli Zaretskii]

> Date: Sat, 16 Mar 2019 14:53:34 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> The problem is that I was trying to figure out the canonical method to
> check if a variable was: t from C code (or assign t to a "DEFVAR_LISP"
> defined variable). I was looking for something similar to NILP but I
> couldn't get it.

Use Qt in C to refer to the symbol t.  Like this:

  Lisp_Object foo = Qt;      /* assignment */
  ...
  if (EQ (some_object, Qt))  /* test for equality to t */
    ...

> It will be helpful for new people (like me), some explanations about
> each of them. Expected arguments, and or objectives. And comments like
> XFIXNUM can crash so it is needed to protect the code with an if
> FIXNUMP. And then what CHECK_FIXNUM is useful for.
> 
> I know I need to read more, but it was a bit confusing to figure out the
> difference between FIXNUM and FIXNAT.
> 
> And functions that are lisp equivalents (CAR CRD) could say that so
> newbies can refer to the elisp manual to look for it.

OK, I will try to add some more commentary when I have time.

Re: Fill column indicator functionality

[Eli Zaretskii]

> Date: Sat, 16 Mar 2019 10:36:10 +0100
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> I tried that solution (setting the :height attribute to 1.0 in the face
> declaration) and it didn't work.

Well, I'm not really surprised, it was a long shot.

> I see that there is a calc_line_height_property; can we use that to
> change the glyph. Maybe it is possible to implement a setter (equivalent
> to get_it_property?

I don't think I understand the proposal.  calc_line_height_property is
used to produce line-spacing, but I don't see how it could be used for
enlarging the font, since AFAIK that can only be done via a face
definition.

> We can compare the line height with the it height and change it
> temporarily to print the indicator?

That won't help, because the code we are talking about doesn't draw
anything, it just prepares the data structures for drawing.  the
actual drawing happens later, and it is terminal-dependent (for X, see
xterm.c, the function x_draw_glyph_string_foreground, for example).

I think, unless some clever idea comes up which will allow us to use
an existing feature, we should for now leave this issue alone, because
avoiding the gap between indicators when line height varies will
probably require adding some display features, and the indicator
display shouldn't be held off till we do.

Re: Fill column indicator functionality

[Óscar Fuentes]

Ergus <spacibba@aol.com> writes:

> +;;;###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'."

It seems that you forgot to delete half of that docstring.

> @@ -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;
>  	}

This hunk is rejected on `master'. 

> @@ -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.  */

s/Names/Name

> +  DEFSYM (Qfill_column, "fill-column");
> +

Hmmm... is this right? Copy & pasto?

> +  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 but a good"

> +the font in fill-column-face supports Unicode characters.  */);

supports -> does not support ???


This feature is highly appreciated. Thank you.

Re: Fill column indicator functionality

[Ergus]

On Thu, Mar 14, 2019 at 10:28:24PM +0100, �scar Fuentes wrote:
>Ergus <spacibba@aol.com> writes:
>
>
>This hunk is rejected on `master'.
>
Ohh right, Eli fixed it cause it was related with another bug.

I already rebased and fixed all your correction.

Very thanks.

Re: Fill column indicator functionality

[Clément Pit-Claudel]

On 14/03/2019 02.40, Eli Zaretskii wrote:
>> Date: Thu, 14 Mar 2019 04:02:24 +0100
>> From: Ergus <spacibba@aol.com>
>> Cc: emacs-devel@gnu.org
>>
>> 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.

Sorry for jumping in late without having followed the whole discussion.  Here's a naive question: is there any reason to use an actual glyph from a face to draw that line, rather than just a continuous line?

A good analogy might be the way window separators ("vertical borders") are drawn: when I use a non-graphical Emacs it uses a series of '|' characters to draw separations between windows, but when I use a graphical Emacs (with scroll-bars-mode off) it draws a solid black line, or at least that's what I understand from e.g. https://lists.gnu.org/archive/html/help-gnu-emacs/2013-01/msg00257.html.

Sorry if I missed something obvious.
Clément.

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Thu, 14 Mar 2019 14:58:07 -0400
> 
> Sorry for jumping in late without having followed the whole discussion.  Here's a naive question: is there any reason to use an actual glyph from a face to draw that line, rather than just a continuous line?

That's a good question.  Indeed, it would be possible to implement
this with a single thin line being drawn, but I think such an
implementation would have several disadvantages:

 . It will need to be implemented separately for each window-system
   (X, w32, NS), and for TTY frames.  By contrast, the implementation
   discussed here is in a single place, is independent of the display
   kind, and is very simple and localized.

 . Using a glyph makes it easier to support some fancy customizations.
   For example, suppose that someone would like to have the indicator
   be a dotted line, or to have a more prominent appearance -- all
   they'd need to do is find a suitable character, such as ▍ or ▓, and
   be done.  With the continuous line implementation, we'd need to
   implement the corresponding textures in the code, before it could
   be supported.

 . Using the continuous line would cause complications wrt redisplay
   optimizations.  Redisplay of text is highly optimized in Emacs, and
   frequently needs to update only a small portion of the window.  The
   problem is what to do about the indicator line when only some
   portions are redrawn.  A naïve idea of redrawing the entire line
   would cause it to flicker on each keypress.  (The divider avoids
   this problem because it is drawn where no characters can ever
   overwrite it, so it only needs to be redrawn when the window
   dimensions change, something that is rare and causes a complete
   redrawing of the entire window anyway.)  We could instead arrange
   for redrawing only portions of the line that overlap the glyphs
   which are actually redrawn, but then we'd need to write code for
   detecting those portions.  By contrast, using the glyph-based
   implementation we get this for free, because the redisplay
   optimizations already know how to avoid redrawing glyphs that
   didn't change.  We also get for free support for R2L text,
   something that would again require separate code with the
   continuous line idea.

The continuous line idea does have one advantage: it seamlessly
supports lines of variable height.  But that use case is rare, and I
think we will be able to support them with the glyph-based
implementation as well.

Does this answer your question?

Re: Fill column indicator functionality

[Clément Pit-Claudel]

On 15/03/2019 03.30, Eli Zaretskii wrote:
> Does this answer your question?

It does, thanks. It's a very thorough answer. Thanks for taking the time, Eli!

> The continuous line idea does have one advantage: it seamlessly
> supports lines of variable height.  But that use case is rare, and I
> think we will be able to support them with the glyph-based
> implementation as well.

This was actually going to be my follow-up question :) I see lines of variable height in many of most of my Emacs mode, through a combination of factors:

- Font switches due to symbols picked by prettify-symbols-mode not being available in my main programming font
- Variable-pitch fonts being used by AucTeX for section and paragraph titles 
- Varying font sizes to indicate nesting depth in Org-mode
- Changing line spacing to separate top-level items in org-mode

Is there actually a good way to support all this?  Maybe using a very thin space and a face with a :background on it would do.  Or maybe it will be possible to put a 'display (space …) property on the line character so that it displays as a 1-px wide line?

Also, as a second follow-up question: we already have code to draw boxes around characters; would it be hard to extend that code to allow customization of which sides to draw? Alternatively, would it be hard to add a vertical strike-through face property? (I've used horizontal strike-through as a way to display horizontal lines in the past)

Cheers,
Clément.

Re: Fill column indicator functionality

[Óscar Fuentes]

Clément Pit-Claudel <cpitclaudel@gmail.com> writes:

> This was actually going to be my follow-up question :) I see lines of
> variable height in many of most of my Emacs mode, through a
> combination of factors:
>
> - Font switches due to symbols picked by prettify-symbols-mode not
> being available in my main programming font
> - Variable-pitch fonts being used by AucTeX for section and paragraph titles 
> - Varying font sizes to indicate nesting depth in Org-mode
> - Changing line spacing to separate top-level items in org-mode

I don't understand how this feature can make sense for lines with
non-monospaced fonts. The purpose is to know two things:

1. How far we are from the limit column.
2. If the limit was surpassed.

For 2 there are some handy packages (column-overflow-mode, for instance)
that should work for non-monospaced fonts. For 1 the system has no way
of knowing where to put the line, because it depends on the text that we
are going to write.

What I'm missing?

Re: Fill column indicator functionality

[Clément Pit-Claudel]

On 15/03/2019 10.07, Óscar Fuentes wrote:
> What I'm missing?

I this I see what caused the confusion.  There are two answers:

- First, at least one of the examples I mentioned involves changes in line height, not in character width; that's the part I worry most about.
- Second, I believe the 80-character limits is usually intended to serve as a bound on the width of the text on display (the assumption being that short lines are easier to read than long ones, and that keeping a limited width makes it easier to show multiple text fragments side by side).  When the text is monospace, it's natural to express the limit in terms of a number of characters.  When the text is displayed in a variable-pitch font, the natural equivalent is to express the limit in pixels or em units.  IOW, a long lines indicator still makes sense even for variable-pitch fonts; you just don't want it to count characters.

Clément.

Re: Fill column indicator functionality

[Óscar Fuentes]

Clément Pit-Claudel <cpitclaudel@gmail.com> writes:

> On 15/03/2019 10.07, Óscar Fuentes wrote:
>> What I'm missing?
>
> I this I see what caused the confusion.  There are two answers:
>
> - First, at least one of the examples I mentioned involves changes in
> line height, not in character width; that's the part I worry most
> about.

Yes, I've seen this quite often: Emacs chooses a font for some unicode
char and the line height is altered.

> - Second, I believe the 80-character limits is usually intended to
> serve as a bound on the width of the text on display (the assumption
> being that short lines are easier to read than long ones, and that
> keeping a limited width makes it easier to show multiple text
> fragments side by side). When the text is monospace, it's natural to
> express the limit in terms of a number of characters. When the text is
> displayed in a variable-pitch font, the natural equivalent is to
> express the limit in pixels or em units. IOW, a long lines indicator
> still makes sense even for variable-pitch fonts; you just don't want
> it to count characters.

This is more about typesetting than anything else. As long as Emacs has
no real WYSIWYG word-processing capabilities, IMHO the lack of this
feature on that context is not important.

Re: Fill column indicator functionality

[Clément Pit-Claudel]

On 15/03/2019 11.15, Óscar Fuentes wrote:
>> - First, at least one of the examples I mentioned involves changes in
>> line height, not in character width; that's the part I worry most
>> about.

> Yes, I've seen this quite often: Emacs chooses a font for some unicode
> char and the line height is altered.

Right, exactly. It would be great if the new fci implementation handled this smoothly.

Re: Fill column indicator functionality

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 15 Mar 2019 08:44:20 -0400
> 
> On 15/03/2019 03.30, Eli Zaretskii wrote:
> > Does this answer your question?
> 
> It does, thanks. It's a very thorough answer. Thanks for taking the time, Eli!
> 
> > The continuous line idea does have one advantage: it seamlessly
> > supports lines of variable height.  But that use case is rare, and I
> > think we will be able to support them with the glyph-based
> > implementation as well.
> 
> This was actually going to be my follow-up question :) I see lines of variable height in many of most of my Emacs mode, through a combination of factors:
> 
> - Font switches due to symbols picked by prettify-symbols-mode not being available in my main programming font
> - Variable-pitch fonts being used by AucTeX for section and paragraph titles 
> - Varying font sizes to indicate nesting depth in Org-mode
> - Changing line spacing to separate top-level items in org-mode

Does the fci mode support those use cases?

> Is there actually a good way to support all this?  Maybe using a very thin space and a face with a :background on it would do.  Or maybe it will be possible to put a 'display (space …) property on the line character so that it displays as a 1-px wide line?

You are thinking in terms of a Lisp program, but we are talking about
something the display engine itself does.  E.g., setting display
properties from inside the display engine is a non-starter, because
users don't expect such properties to appear without their say-so.

But yes, we have several methods of scaling display elements which
could be used to solve these use cases.  I just don't think we should
block the feature until they are fixed.  However important are the use
cases you enumerated, they are not the majority in Emacs, right?

> Also, as a second follow-up question: we already have code to draw boxes around characters; would it be hard to extend that code to allow customization of which sides to draw? Alternatively, would it be hard to add a vertical strike-through face property? (I've used horizontal strike-through as a way to display horizontal lines in the past)

I'm not sure where are you going with this.  Yes, Emacs is capable of
drawing horizontal and vertical lines of specified pixel-width, but
the issue here is not whether we can do it, the issue is how to do
that in a way that fits as much as possible the current basic design
and implementation of the display engine.  Otherwise, the job becomes
so large that we probably won't have this feature any time soon.

Specifically, drawing a "vertical strike-through" still doesn't solve
the problem with the height of the line, because the glyph we will
"strike through" will still need to have a suitable height, right?

You need to realize that on its highest level, the display engine
manipulates glyphs (and there are several kinds of them).  All the
decorations that are not glyphs can be drawn only in places where
glyphs can never appear.  In a nutshell (and this is admittedly an
over-simplification), the display engine can display and/or lay out
anything that we can present/represent as a glyph, but that's all it
can do.

Re: Fill column indicator functionality

[Clément Pit-Claudel]

On 15/03/2019 10.12, Eli Zaretskii wrote:
>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>> This was actually going to be my follow-up question :) I see lines of variable height in many of most of my Emacs mode, through a combination of factors:
>>
>> - Font switches due to symbols picked by prettify-symbols-mode not being available in my main programming font
>> - Variable-pitch fonts being used by AucTeX for section and paragraph titles 
>> - Varying font sizes to indicate nesting depth in Org-mode
>> - Changing line spacing to separate top-level items in org-mode
> 
> Does the fci mode support those use cases?

Not well, no.  There are discontinuities, and if I use wide characters (like CJK spaces, e.g. '　'), portions of the line are misaligned.

> But yes, we have several methods of scaling display elements which
> could be used to solve these use cases.  I just don't think we should
> block the feature until they are fixed.  However important are the use
> cases you enumerated, they are not the majority in Emacs, right?

No, I don't think they are the majority of Emacs :) 

>> Also, as a second follow-up question: we already have code to draw boxes around characters; would it be hard to extend that code to allow customization of which sides to draw? Alternatively, would it be hard to add a vertical strike-through face property? (I've used horizontal strike-through as a way to display horizontal lines in the past)
> 
> I'm not sure where are you going with this.  Yes, Emacs is capable of
> drawing horizontal and vertical lines of specified pixel-width, but
> the issue here is not whether we can do it, the issue is how to do
> that in a way that fits as much as possible the current basic design
> and implementation of the display engine.  Otherwise, the job becomes
> so large that we probably won't have this feature any time soon.
> 
> Specifically, drawing a "vertical strike-through" still doesn't solve
> the problem with the height of the line, because the glyph we will
> "strike through" will still need to have a suitable height, right?

Maybe? At the moment boxes and backgrounds already span the full line height, so I'm assuming strike-throughs would as well.
All that I was pointing out is that the box display code already draws lines that reliably span the entire line height.

Clément.

Re: Fill column indicator functionality

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 15 Mar 2019 10:35:02 -0400
> 
> > Does the fci mode support those use cases?
> 
> Not well, no.  There are discontinuities, and if I use wide characters (like CJK spaces, e.g. '　'), portions of the line are misaligned.

The misalignment part should disappear with this new implementation,
because it works in pixels.

> > Specifically, drawing a "vertical strike-through" still doesn't solve
> > the problem with the height of the line, because the glyph we will
> > "strike through" will still need to have a suitable height, right?
> 
> Maybe? At the moment boxes and backgrounds already span the full line height, so I'm assuming strike-throughs would as well.

That's again not on the glyph level, it's drawing code specific to
each terminal type.

Re: Fill column indicator functionality

[Clément Pit-Claudel]

On 15/03/2019 12.13, Eli Zaretskii wrote:
> 
>>> Specifically, drawing a "vertical strike-through" still doesn't solve
>>> the problem with the height of the line, because the glyph we will
>>> "strike through" will still need to have a suitable height, right?

>> Maybe? At the moment boxes and backgrounds already span the full line height, so I'm assuming strike-throughs would as well.

> That's again not on the glyph level, it's drawing code specific to
> each terminal type.

Understood. I was asking how hard it would be to use that code for drawing lines (since it already exists ^^)

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 15 Mar 2019 14:26:11 -0400
> Cc: emacs-devel@gnu.org
> 
> I was asking how hard it would be to use that code for drawing lines (since it already exists ^^)

Drawing a line is not hard at all, regardless of whether the code to
do that exists or not.  What is missing is a way to tell the
terminal-specific back-end to draw that line.  This would probably
require a new glyph attribute.

Re: Fill column indicator functionality

[Stefan Monnier]

> This was actually going to be my follow-up question :) I see lines of
> variable height in many of most of my Emacs mode, through a combination of
> factors:
>
> - Font switches due to symbols picked by prettify-symbols-mode not being
> available in my main programming font
> - Variable-pitch fonts being used by AucTeX for section and paragraph titles 
> - Varying font sizes to indicate nesting depth in Org-mode
> - Changing line spacing to separate top-level items in org-mode

I also see such variations due to my use of fonts instead of colors
for font-lock highlighting: I use different fonts for the keyword-face
and the doc-face.  I try to choose them such that they take up exactly
the same number of pixels as my default font, but it's a pretty
delicate/brittle business.

So I agree it's a case we should try and handle "correctly".


        Stefan

Re: Fill column indicator functionality

[Alp Aker]

> That's a good question.  Indeed, it would be possible to implement
> this with a single thin line being drawn, but I think such an
> implementation would have several disadvantages:
>
>   . It will need to be implemented separately for each window-system
>     (X, w32, NS), and for TTY frames.  By contrast, the implementation
>     discussed here is in a single place, is independent of the display
>     kind, and is very simple and localized.

I believe a simpler implementation is possible:  Where Ergus's 
implementation uses a char glyph, it could also use a line-height image 
glyph.  That is, instead of a single window-length element, analogous to 
the window divider, the indicator would consist of many independently 
displayed line segments, one per line.  If we use an image type like XPM 
that's universally supported, this would allow a single implementation 
shared by all window systems, and the implementation for graphical and 
text frames would be the same except for the choice of glyph.  (This is 
the method used by the unfortunate fill-column-indicator package whose 
deficiencies have necessitated the current work.)

>   . Using a glyph makes it easier to support some fancy customizations.
>     For example, suppose that someone would like to have the indicator
>     be a dotted line, or to have a more prominent appearance -- all
>     they'd need to do is find a suitable character, such as ▍ or ▓, and
>     be done.  With the continuous line implementation, we'd need to
>     implement the corresponding textures in the code, before it could
>     be supported.

If we use an image glyph on each line, the customizations would apply 
only to the construction of that XPM image; the display routine that 
positions the image segments doesn't need to know how to construct the 
image.  Users could even supply arbitrary images to use, so long as 
they're of the appropriate height (or they could just be cropped to the 
correct height if necessary).  It's straightforward to define XPM 
bitmaps in code, so this would allow even more flexibility than using 
font characters.

>   . Using the continuous line would cause complications wrt redisplay
>     optimizations.  Redisplay of text is highly optimized in Emacs, and
>     frequently needs to update only a small portion of the window.  The
>     problem is what to do about the indicator line when only some
>     portions are redrawn.

Unless I'm mistaken, the method I'm describing would allow the same 
optimizations that Ergus's current implementation does (since it would, 
as far as display is concerned, be the same except for the choice of 
glyph to use).

Re: Fill column indicator functionality

[Mattias Engdegård]

15 mars 2019 kl. 14.00 skrev Alp Aker <alp.tekin.aker@gmail.com>:
> 
>> That's a good question.  Indeed, it would be possible to implement
>> this with a single thin line being drawn, but I think such an
>> implementation would have several disadvantages:
>> 
>>  . It will need to be implemented separately for each window-system
>>    (X, w32, NS), and for TTY frames.  By contrast, the implementation
>>    discussed here is in a single place, is independent of the display
>>    kind, and is very simple and localized.
> 
> I believe a simpler implementation is possible:  Where Ergus's implementation uses a char glyph, it could also use a line-height image glyph.

Terminal emulators often treat box-drawing characters specially, either drawing the lines themselves or extending parts of the glyphs in case there would be gaps. Even XTerm does this, mainly to supplement missing glyphs.

Being its own terminal emulator, Emacs could do the same to avoid gaps in horizontal and vertical lines that should connect. Box-drawing characters are attractive for use in tables, diagrams and user interfaces, and work in both TTYs and window systems. They are sometimes seen in ASCII+ art in source code comments.

The details are fairly window-system-specific but do not have to be implemented everywhere at once.

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 15 Mar 2019 14:30:55 +0100
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> Terminal emulators often treat box-drawing characters specially, either drawing the lines themselves or extending parts of the glyphs in case there would be gaps. Even XTerm does this, mainly to supplement missing glyphs.
> 
> Being its own terminal emulator, Emacs could do the same to avoid gaps in horizontal and vertical lines that should connect.

Unfortunately, this is not currently possible, not without significant
changes in how the Emacs display engine works internally.  The display
routines are currently called in a way that doesn't allow them to have
a window-global view of the display, not even a way of knowing what's
on the previous and the next screen lines.  In fact, the display
routines need to do their job even if the information about the layout
of adjacent screen lines is not available at all, and redisplay
optimizations frequently make use of this property, by starting a new
redisplay cycle on a specific line, doing part of the layout, then
throwing out the results.  About the only thing the display routines
can rely on is the window-start point and the window pixel
dimensions.  Even the vertical coordinate of a line on which the
display routine was called is frequently unknown. 

> The details are fairly window-system-specific but do not have to be implemented everywhere at once.

I don't think I understand what you are saying here.  My point was
that we have xfns.c/xterm.c for X, w32fns.c/w32term.c for w32, and
nsfns.m/nsterm.m for NS.  Any line drawing that doesn't end up as part
f some glyph must be implemented in each of these back-ends, because
the APIs used for such drawing are different.

Re: Fill column indicator functionality

[Mattias Engdegård]

fre 2019-03-15 klockan 16:24 +0200 skrev Eli Zaretskii:

> Unfortunately, this is not currently possible, not without
> significant changes in how the Emacs display engine works
> internally.  The display routines are currently called in a way that
> doesn't allow them to have a window-global view of the display, not
> even a way of knowing what's on the previous and the next screen
> lines.

Sorry, I was being unclear. What I meant to say was that if some box-
drawing glyph does not go all the way to the edge of its cell, then
Emacs could attempt to improve on the rendering of that glyph instead
of being bound by the font. It would not need to know anything about
what is drawn elsewhere. It's just a way of fixing the font.

I don't even know how common the problem is with modern fonts, or what
the underlying causes are. I've mostly observed it with scaled pixel
fonts.

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Cc: alp.tekin.aker@gmail.com, emacs-devel@gnu.org
> Date: Fri, 15 Mar 2019 16:05:33 +0100
> 
> Sorry, I was being unclear. What I meant to say was that if some box-
> drawing glyph does not go all the way to the edge of its cell, then
> Emacs could attempt to improve on the rendering of that glyph instead
> of being bound by the font. It would not need to know anything about
> what is drawn elsewhere. It's just a way of fixing the font.

Emacs never examines the glyphs that the font produces, it just calls
an API that displays text using glyph numbers it receives from the
font back-end.

Re: Fill column indicator functionality

[Stefan Monnier]

>> Being its own terminal emulator, Emacs could do the same to avoid gaps in
>> horizontal and vertical lines that should connect.
>
> Unfortunately, this is not currently possible, not without significant
> changes in how the Emacs display engine works internally.  The display
> routines are currently called in a way that doesn't allow them to have
> a window-global view of the display, not even a way of knowing what's
> on the previous and the next screen lines.  In fact, the display
> routines need to do their job even if the information about the layout
> of adjacent screen lines is not available at all, and redisplay
> optimizations frequently make use of this property, by starting a new
> redisplay cycle on a specific line, doing part of the layout, then
> throwing out the results.  About the only thing the display routines
> can rely on is the window-start point and the window pixel
> dimensions.  Even the vertical coordinate of a line on which the
> display routine was called is frequently unknown. 

I think what Mattias is referring to is to change the part of the code
where we draw the glyph matrix onto the screen (i.e. the part that's
window-system specific), to add ad-hoc hacks that try to display
box-drawing characters differently to try and fill small gaps
between them.  I don't know how workable that would be, especially when
considering proportional fonts (i.e. the problems with box-drawing
characters aren't limited to gaps between them but also include
misalignment).


        Stefan

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 15 Mar 2019 11:09:25 -0400
> 
> I think what Mattias is referring to is to change the part of the code
> where we draw the glyph matrix onto the screen (i.e. the part that's
> window-system specific), to add ad-hoc hacks that try to display
> box-drawing characters differently to try and fill small gaps
> between them.

Emacs has no idea how a glyph will look on the screen, it just tells
the window-system to display it.

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: Alp Aker <alp.tekin.aker@gmail.com>
> Date: Fri, 15 Mar 2019 09:00:59 -0400
> 
> > That's a good question.  Indeed, it would be possible to implement
> > this with a single thin line being drawn, but I think such an
> > implementation would have several disadvantages:
> >
> >   . It will need to be implemented separately for each window-system
> >     (X, w32, NS), and for TTY frames.  By contrast, the implementation
> >     discussed here is in a single place, is independent of the display
> >     kind, and is very simple and localized.
> 
> I believe a simpler implementation is possible:  Where Ergus's 
> implementation uses a char glyph, it could also use a line-height image 
> glyph.

How do you produce a line-height tall image glyph?  Maybe I'm missing
something, but I think we can only display image glyphs whose height
is fixed and determined by the image, or by its slice whose dimensions
are determined by a suitable display property, i.e. computed in
advance of redisplay.

But in any case, if such an image with dynamically computed height can
be produced reasonably easily by the display engine itself, what you
propose is a trivial extension of the code presented here: we just
need to replace the character glyph by an image glyph.

> the implementation for graphical and 
> text frames would be the same except for the choice of glyph.

No, the implementation for the text-mode frame will always need to be
(slightly) different , because it doesn't support images and it
implements the stretch glyph as a series of space-character glyphs.

> >   . Using a glyph makes it easier to support some fancy customizations.
> >     For example, suppose that someone would like to have the indicator
> >     be a dotted line, or to have a more prominent appearance -- all
> >     they'd need to do is find a suitable character, such as ▍ or ▓, and
> >     be done.  With the continuous line implementation, we'd need to
> >     implement the corresponding textures in the code, before it could
> >     be supported.
> 
> If we use an image glyph on each line, the customizations would apply 
> only to the construction of that XPM image

Maybe I'm again missing something, but how can a Lisp program
(i.e. user customizations) produce an XPM image of unlimited height?
If the height is fixed, the image will be unable to produce vertical
lines taller than its height, in order to support tall lines.

Btw, I don't think we can assume universal XPM support: Emacs can
support platforms that only provide monochrome bitmapped images, no
XPM.  E.g., if I remove or rename the XPM DLL, Emacs on Windows still
shows the tool-bar icons, albeit ugly monochrome versions of them.

> It's straightforward to define XPM bitmaps in code, so this would
> allow even more flexibility than using font characters.

I'm sure users will find customizing a character much easier than
customizing an image.  In any case, I wouldn't give up on allowing a
character, even if we do support images, because there's no reason to
restrict users to images.  For example, a suitable selection of a
character could produce identical displays on GUI and TTY frames,
something that an image could never do.

> >   . Using the continuous line would cause complications wrt redisplay
> >     optimizations.  Redisplay of text is highly optimized in Emacs, and
> >     frequently needs to update only a small portion of the window.  The
> >     problem is what to do about the indicator line when only some
> >     portions are redrawn.
> 
> Unless I'm mistaken, the method I'm describing would allow the same 
> optimizations

Yes, but I was replying to a completely different idea.  What you
propose is just a trivial extension of the code posted here, so all of
the effort that went into that will still be valid, and most of my
message is not really relevant.

Re: Fill column indicator functionality

[Alp Aker]

> How do you produce a line-height tall image glyph?  Maybe I'm missing
> something, but I think we can only display image glyphs whose height
> is fixed and determined by the image, or by its slice whose dimensions
> are determined by a suitable display property, i.e. computed in
> advance of redisplay.
>
> But in any case, if such an image with dynamically computed height can
> be produced reasonably easily by the display engine itself, what you
> propose is a trivial extension of the code presented here: we just
> need to replace the character glyph by an image glyph.

I did not have a dynamically sized images in mind, only fixed-height 
images.  They offer a way of achieving the graphical effect that some 
people in this thread have requested that would involve, as you point 
out, only a trivial extension of what Ergus has already done.

Would lack of support for variable line heights be an issue?  I can only 
offer my experience here as the author of fill-column-indicator.  Among 
the many issues that package has, lack of support for variable line 
heights in a window hasn't been one of them.  It just isn't something 
users have requested.

> I'm sure users will find customizing a character much easier than
> customizing an image.  In any case, I wouldn't give up on allowing a
> character, even if we do support images, because there's no reason to
> restrict users to images.  For example, a suitable selection of a
> character could produce identical displays on GUI and TTY frames,
> something that an image could never do.

A choice between image and character was what I had in mind.  (If a 
buffer is displayed on both GUI and TTY frames, you can even use both at 
the same time, which is what fci-mode does.)

I wouldn't expect end users to create their own images.  Again, I can 
only offer my own experience, but the only customization requests I've 
received have been for (a) color, (b) thickness, and (c) dashed vs. 
solid.  Those can be offered as simple customization options.  Arbitrary 
user defined images are possible on this scheme, but wouldn't be the 
basic mode of customization.

Re: Fill column indicator functionality

[Clément Pit-Claudel]

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

On 15/03/2019 10.19, Alp Aker wrote:
> Would lack of support for variable line heights be an issue?  I can
> only offer my experience here as the author of fill-column-indicator.
> Among the many issues that package has, lack of support for variable
> line heights in a window hasn't been one of them.  It just isn't
> something users have requested.

It's the main reason why I don't use fci-mode :) I've attached a screenshot.


[-- Attachment #2: fci.png --]
[-- Type: image/png, Size: 58510 bytes --]

Re: Fill column indicator functionality

[Johan Bockgård]

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

Clément Pit-Claudel <cpitclaudel@gmail.com> writes:

> On 15/03/2019 10.19, Alp Aker wrote:
>> Would lack of support for variable line heights be an issue?  I can
>> only offer my experience here as the author of fill-column-indicator.
>> Among the many issues that package has, lack of support for variable
>> line heights in a window hasn't been one of them.  It just isn't
>> something users have requested.
>
> It's the main reason why I don't use fci-mode :) I've attached a screenshot.

You can use the "stipple" face feature (on platforms that support it) to
draw a 1 pixel tall repeating bitmap (with some limitations).

##

(require 'fill-column-indicator)

(defface fci-face
  `((t (:foreground
	,(or fci-rule-character-color fci-rule-color)
	:stipple
	(,(frame-char-width) 1
	 ,(string (lsh (1- (expt 2 fci-rule-width))
		       (/ (frame-char-width) 2))
		  0 0 0 0)))))
  "")

(setq fci-always-use-textual-rule t
      fci-rule-character ?\s)

(defadvice fci-make-rule-string (around frob activate)
  (setq ad-return-value
	(propertize (char-to-string fci-rule-character) 'face 'fci-face)))

(fci-mode)

##

Example with big line spacing:


[-- Attachment #2: fci-with-stipple.png --]
[-- Type: image/png, Size: 20960 bytes --]

Re: Fill column indicator functionality

[Clément Pit-Claudel]

On 16/03/2019 11.07, Johan Bockgård wrote:
> Clément Pit-Claudel <cpitclaudel@gmail.com> writes:
> 
>> On 15/03/2019 10.19, Alp Aker wrote:
>>> Would lack of support for variable line heights be an issue?  I can
>>> only offer my experience here as the author of fill-column-indicator.
>>> Among the many issues that package has, lack of support for variable
>>> line heights in a window hasn't been one of them.  It just isn't
>>> something users have requested.
>>
>> It's the main reason why I don't use fci-mode :) I've attached a screenshot.
> 
> You can use the "stipple" face feature (on platforms that support it) to
> draw a 1 pixel tall repeating bitmap (with some limitations).

Very nice :)

Re: Fill column indicator functionality

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Alp Aker <alp.tekin.aker@gmail.com>
> Date: Fri, 15 Mar 2019 10:19:04 -0400
> 
> I did not have a dynamically sized images in mind, only fixed-height 
> images.  They offer a way of achieving the graphical effect that some 
> people in this thread have requested that would involve, as you point 
> out, only a trivial extension of what Ergus has already done.

Right.

> Would lack of support for variable line heights be an issue?

I hope not, at least not initially.  But Clément seems to think
otherwise.

> I can only offer my experience here as the author of
> fill-column-indicator.  Among the many issues that package has, lack
> of support for variable line heights in a window hasn't been one of
> them.  It just isn't something users have requested.

This is Emacs: as soon as you offer a feature that solves N problems
users always wanted solved, someone will come up and argue that
solution for the N+1th problem is an absolute must to have ;-)

> A choice between image and character was what I had in mind.  (If a 
> buffer is displayed on both GUI and TTY frames, you can even use both at 
> the same time, which is what fci-mode does.)
> 
> I wouldn't expect end users to create their own images.  Again, I can 
> only offer my own experience, but the only customization requests I've 
> received have been for (a) color, (b) thickness, and (c) dashed vs. 
> solid.  Those can be offered as simple customization options.  Arbitrary 
> user defined images are possible on this scheme, but wouldn't be the 
> basic mode of customization.

Sounds like we are in violent agreement, then.  Thanks for your
comments, the experience of fci-mode is, of course, something we
should learn from when we design the customization facilities of this
new implementation.

Re: Fill column indicator functionality

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

> This is Emacs: as soon as you offer a feature that solves N problems
> users always wanted solved, someone will come up and argue that
> solution for the N+1th problem is an absolute must to have ;-)

+1

This feature, as implemented, is useful. Furthermore, its implementation
is not problematic on the sense of making it difficult to replace it
with somethin more comprehensive on the future.

Although the improvements suggested are reasonable and well-founded, It
would be a shame if it is delayed as a consequence of the social
dynamics you mention.

Re: Fill column indicator functionality

[Clément Pit-Claudel]

On 15/03/2019 13.24, Óscar Fuentes wrote:
> Although the improvements suggested are reasonable and well-founded, It
> would be a shame if it is delayed as a consequence of the social
> dynamics you mention.

Oh, yes, of course. I don't intend to delay anything ("now is better than never"), just to make sure that we have all use cases in mind so that we don't design something that is needlessly restrictive (or hard to extend in the future).

I am thankful for the cool work that Ergus is doing, and for Eli's dedication and patient mentorship.  

Re: Fill column indicator functionality

[Alp Aker]

> I'm sure users will find customizing a character much easier than
> customizing an image.  In any case, I wouldn't give up on allowing a
> character, even if we do support images, because there's no reason to
> restrict users to images.  For example, a suitable selection of a
> character could produce identical displays on GUI and TTY frames,
> something that an image could never do.

I just wanted to expand on this point and clarify my motivations for 
chiming in on this discussion.  I'm very grateful Ergus is doing this 
work, as I've long felt that the functionality of fill-column-indicator 
can only be robustly implemented in the display engine.  I merely want 
to offer my experience as the author of what, until now, has been the 
main package providing this kind of functionality in order to make 
Ergus's work more successful.

When I first implemented fill-column-indicator I used a character-based 
approach and, overwhelmingly, the feedback I received from users 
concerned difficulties in finding and successfully configuring use of a 
font that would provide a seamless visual effect.  When I added the 
option to use XPM images (generated not by the user, but by the package, 
with customization options concerning color, thickness, etc.), users 
only seemed to make use of that.  I've never received a complaint from 
someone wanting to use the package on a system that didn't support XPM 
images.





>
>>>    . Using the continuous line would cause complications wrt redisplay
>>>      optimizations.  Redisplay of text is highly optimized in Emacs, and
>>>      frequently needs to update only a small portion of the window.  The
>>>      problem is what to do about the indicator line when only some
>>>      portions are redrawn.
>> Unless I'm mistaken, the method I'm describing would allow the same
>> optimizations
> Yes, but I was replying to a completely different idea.  What you
> propose is just a trivial extension of the code posted here, so all of
> the effort that went into that will still be valid, and most of my
> message is not really relevant.

Re: Fill column indicator functionality

[John Yates]

On Sat, Mar 9, 2019 at 8:24 AM Ergus <spacibba@aol.com> wrote:
>
> In graphical mode maybe it is better to do this change writing directly
> a vertical line in the windows' frame background.

That would be much closer to the experience I desire.  I advocate
one small change: draw the vertical line in the foreground.

/john

Re: Fill column indicator functionality

[Eli Zaretskii]

> From: John Yates <john@yates-sheets.org>
> Date: Sat, 9 Mar 2019 13:02:04 -0500
> Cc: Eli Zaretskii <eliz@gnu.org>, Emacs developers <emacs-devel@gnu.org>
> 
> On Sat, Mar 9, 2019 at 8:24 AM Ergus <spacibba@aol.com> wrote:
> >
> > In graphical mode maybe it is better to do this change writing directly
> > a vertical line in the windows' frame background.
> 
> That would be much closer to the experience I desire.  I advocate
> one small change: draw the vertical line in the foreground.

I'll welcome patches to implement such a capability.  Right now, I
don't think we can do that in Emacs.