bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Davor Rotim]

[-- Attachment #1.1: Type: text/plain, Size: 1267 bytes --]

Hello, in the attached images are two cases I noticed where
`display-fill-column-indicator-mode' causes display bugs. First case is
with faces that use the :overline or :underline property, the lines will
extend fully towards the indicator. Second case is with `company-mode' when
there's no text entered and the completion dialog pops up which
display-fill-column-indicator-mode treats like ordinary text.

In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.10,
cairo version 1.16.0)
 of 2019-07-30 built on lambda
Repository revision: 99156a03bfee8304cf2644470dceb668e6262c98
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux bullseye/sid

Configured using:
 'configure 'CFLAGS=-march=native -O2 -pipe -fstack-protector-strong
 -fno-plt' --prefix=/home/drot/.local
 '--program-transform-name=s/^ctags$/ctags.emacs/' --with-cairo
 --with-modules --enable-link-time-optimization --disable-gcc-warnings'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
ZLIB TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD
JSON PDUMPER LCMS2 GMP

[-- Attachment #1.2: Type: text/html, Size: 1453 bytes --]

[-- Attachment #2: company.png --]
[-- Type: image/png, Size: 39993 bytes --]

[-- Attachment #3: org-mode-disabled.png --]
[-- Type: image/png, Size: 56118 bytes --]

[-- Attachment #4: org-mode-enabled.png --]
[-- Type: image/png, Size: 57265 bytes --]

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> From: Davor Rotim <rotim.davor@gmail.com>
> Date: Tue, 30 Jul 2019 20:11:04 +0200
> 
> Hello, in the attached images are two cases I noticed where `display-fill-column-indicator-mode' causes
> display bugs. First case is with faces that use the :overline or :underline property, the lines will extend fully
> towards the indicator. Second case is with `company-mode' when there's no text entered and the completion
> dialog pops up which display-fill-column-indicator-mode treats like ordinary text.

Jimmy, could you please take a look at these two issues?

Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

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

Hi Eli:

I will give a look the next week. 

The org mode issue seems more or less easy to fix. 

But for the company-mode I need to check if there is a condition to distinguish normal text from the company popups. From the display engine. Else we could consider to extend the line always until the end of the screen... If that is possible...

On August 2, 2019 11:16:52 AM GMT+02:00, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Davor Rotim <rotim.davor@gmail.com>
>> Date: Tue, 30 Jul 2019 20:11:04 +0200
>> 
>> Hello, in the attached images are two cases I noticed where
>`display-fill-column-indicator-mode' causes
>> display bugs. First case is with faces that use the :overline or
>:underline property, the lines will extend fully
>> towards the indicator. Second case is with `company-mode' when
>there's no text entered and the completion
>> dialog pops up which display-fill-column-indicator-mode treats like
>ordinary text.
>
>Jimmy, could you please take a look at these two issues?
>
>Thanks.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> Date: Fri, 02 Aug 2019 12:25:15 +0200
> CC: 36858@debbugs.gnu.org,Davor Rotim <rotim.davor@gmail.com>
> From: Ergus <spacibba@aol.com>
> 
> I will give a look the next week. 

Thanks.

> The org mode issue seems more or less easy to fix. 
> 
> But for the company-mode I need to check if there is a condition to distinguish normal text from the company
> popups. From the display engine. Else we could consider to extend the line always until the end of the
> screen... If that is possible...

The glyph rows generated by Company should all have their ends_at_zv_p
flag set in this use case, AFAIR.  Maybe this will help.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

Hi Eli:

I have been looking into this issue and I already fixed both, but I have
a doubt and two comments.

Comment:

1) The condition ends_at_zv_p didn't work as expected, I don't know If
this is an issue somewhere else, but at least in my tests, the condition
was always false. (for all the lines implied before and after ZV, where
there was company window or not)

So the filter condition I am using now is:

IT_CHARPOS (*it) < ZV

which seems to work fine.

2) There is a corner case because the indicator is never generated for
the latest line in the buffer. So a \n is required always at the end of
the buffer if there is text, which for me is fine (unix format), but I
don't know if I should correct that, should I? This issue was there
before this latest changes, so it is unrelated and it is not really so
significant I think.

Doubt:

In terminal emacs, in the original emacs-26 code, in the function:
extend_face_to_end_of_line the code was:

```
 face = FACE_FROM_ID (f, (it->face_before_selective_p
			   ? it->saved_face_id
			   : it->face_id));
 (...)

 if (it->glyph_row->ends_at_zv_p)
     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);
```

So the rest of the line was filled with the last face, (so this issue was
already there since then, because the rest of the line is filled with an
underlined face)

I can change the code to fill the rest of the line with a new merged
face (as I do for graphical emacs), but I think that this fix is
unrelated with dfci, so maybe someone else must give a look before to
prevent me breaking anything.

Which face is the right one to use to fill the rest of the row in the
general case?

For my case I use:
    merge_faces (it->w, Qfill_column_indicator, 0, saved_face_id)

because Qfill_column_indicator face has explicitly set underline and
overline (and some other properties) to false; But maybe we need an
extra face with same properties?

What do you suggest?


On Fri, Aug 02, 2019 at 12:16:52PM +0300, Eli Zaretskii wrote:
>> From: Davor Rotim <rotim.davor@gmail.com>
>> Date: Tue, 30 Jul 2019 20:11:04 +0200
>>
>> Hello, in the attached images are two cases I noticed where `display-fill-column-indicator-mode' causes
>> display bugs. First case is with faces that use the :overline or :underline property, the lines will extend fully
>> towards the indicator. Second case is with `company-mode' when there's no text entered and the completion
>> dialog pops up which display-fill-column-indicator-mode treats like ordinary text.
>
>Jimmy, could you please take a look at these two issues?
>
>Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

Hi Eli:

There are two different behaviors for tui and gui in
extend_face_to_end_of_line.

In gui the face is automatically extended.  It is unclear for me what
face it is using, because this has to do with the issue that in X the
last character is extended automatically, so no loop or stretch_glyph is
needed. But it seems to be using the default face because the underline
is not extend after the text and the space for the cursor.

ON the other hand, for the tui there is

      if (it->glyph_row->ends_at_zv_p)
	it->face_id = default_face->id;
      else
	it->face_id = face->id;

which extends the face using the face of the last glyph. this extends
the underline until the end of the row; but it is different to the gui
behavior, so it is incoherent.

Whats the right behavior in the general case?

Extend the underline the whole line or fit it to the text?

If it is the second whats the right face to fill until the end of the
row

Does the same policy applies to append_space_for_newline? Because now
there is an extra underlined space after the text.

I am just waiting for your recommendation in order to submit a fix for
this issue.

Thanks in advance

Esgus.


On Fri, Aug 02, 2019 at 02:53:30PM +0300, Eli Zaretskii wrote:
>> Date: Fri, 02 Aug 2019 12:25:15 +0200
>> CC: 36858@debbugs.gnu.org,Davor Rotim <rotim.davor@gmail.com>
>> From: Ergus <spacibba@aol.com>
>>
>> I will give a look the next week.
>
>Thanks.
>
>> The org mode issue seems more or less easy to fix.
>>
>> But for the company-mode I need to check if there is a condition to distinguish normal text from the company
>> popups. From the display engine. Else we could consider to extend the line always until the end of the
>> screen... If that is possible...
>
>The glyph rows generated by Company should all have their ends_at_zv_p
>flag set in this use case, AFAIR.  Maybe this will help.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Dmitry Gutov]

On 7/30/19 9:11 PM, Davor Rotim wrote:
> Second case is with `company-mode' when there's no text entered and the 
> completion dialog pops up which display-fill-column-indicator-mode 
> treats like ordinary text.

I'm not sure we could/should change something here.

The company popup renderer makes it seem like the buffer spans longer 
than it does. I think it's okay, and it might not be the place of the 
display engine to second-guess it.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> Date: Mon, 5 Aug 2019 17:27:47 +0200
> From: Ergus <spacibba@aol.com>
> Cc: 36858@debbugs.gnu.org, Davor Rotim <rotim.davor@gmail.com>
> 
> 1) The condition ends_at_zv_p didn't work as expected, I don't know If
> this is an issue somewhere else, but at least in my tests, the condition
> was always false. (for all the lines implied before and after ZV, where
> there was company window or not)
> 
> So the filter condition I am using now is:
> 
> IT_CHARPOS (*it) < ZV
> 
> which seems to work fine.
> 
> 2) There is a corner case because the indicator is never generated for
> the latest line in the buffer. So a \n is required always at the end of
> the buffer if there is text, which for me is fine (unix format), but I
> don't know if I should correct that, should I?

Unix format has nothing to do with this, as in a buffer we always have
only \n characters at end of line.  But notr having the indicator show
in the last line of a buffer that doesn't end in a newline is
unfortunate.  Which is why I suggested to test the ends_at_zv_p flag.
What exactly didn't work with it?  Can you show me a test case where
the glyph rows past ZV don't have this flag set?  Maybe you should
test the enabled_p flag as well?

> In terminal emacs, in the original emacs-26 code, in the function:
> extend_face_to_end_of_line the code was:
> 
> ```
>  face = FACE_FROM_ID (f, (it->face_before_selective_p
> 			   ? it->saved_face_id
> 			   : it->face_id));
>  (...)
> 
>  if (it->glyph_row->ends_at_zv_p)
>      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);
> ```
> 
> So the rest of the line was filled with the last face, (so this issue was
> already there since then, because the rest of the line is filled with an
> underlined face)
> 
> I can change the code to fill the rest of the line with a new merged
> face (as I do for graphical emacs), but I think that this fix is
> unrelated with dfci, so maybe someone else must give a look before to
> prevent me breaking anything.

This is a more general issue, and I will respond to your question on
emacs-devel.

Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

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

On Wed, Aug 07, 2019 at 05:38:30PM +0300, Eli Zaretskii wrote:
>> Date: Mon, 5 Aug 2019 17:27:47 +0200
>> From: Ergus <spacibba@aol.com>
>> Cc: 36858@debbugs.gnu.org, Davor Rotim <rotim.davor@gmail.com>
>>
>> 1) The condition ends_at_zv_p didn't work as expected, I don't know If
>> this is an issue somewhere else, but at least in my tests, the condition
>> was always false. (for all the lines implied before and after ZV, where
>> there was company window or not)
>>
>> So the filter condition I am using now is:
>>
>> IT_CHARPOS (*it) < ZV
>>
>> which seems to work fine.
>>
>> 2) There is a corner case because the indicator is never generated for
>> the latest line in the buffer. So a \n is required always at the end of
>> the buffer if there is text, which for me is fine (unix format), but I
>> don't know if I should correct that, should I?
>
>Unix format has nothing to do with this, as in a buffer we always have
>only \n characters at end of line.  But notr having the indicator show
>in the last line of a buffer that doesn't end in a newline is
>unfortunate.  Which is why I suggested to test the ends_at_zv_p flag.
>What exactly didn't work with it?  Can you show me a test case where
>the glyph rows past ZV don't have this flag set?  Maybe you should
>test the enabled_p flag as well?
>
Hi Eli:

I just made this test:

in this code (in xdisp.c):

if (it->current_x < indicator_column_x)
  {
    it->face_id = merge_faces (it->w, Qextend_to_end_of_line,
                               0, extend_face_merged_id);

    it->char_to_display = XFIXNAT (Vdisplay_fill_column_indicator_character);
    PRODUCE_GLYPHS (it);

    it->face_id = extend_face_merged_id;
}

I changed char_to_display:

it->char_to_display = (it->glyph_row->ends_at_zv_p) ? '1' : '0';

And then I obtained the attached image.

As you can see the condition returns 0 for lines before zv, for the last
text line and for the company extra lines.

>> In terminal emacs, in the original emacs-26 code, in the function:
>> extend_face_to_end_of_line the code was:
>>
>> ```
>>  face = FACE_FROM_ID (f, (it->face_before_selective_p
>> 			   ? it->saved_face_id
>> 			   : it->face_id));
>>  (...)
>>
>>  if (it->glyph_row->ends_at_zv_p)
>>      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);
>> ```
>>
>> So the rest of the line was filled with the last face, (so this issue was
>> already there since then, because the rest of the line is filled with an
>> underlined face)
>>
>> I can change the code to fill the rest of the line with a new merged
>> face (as I do for graphical emacs), but I think that this fix is
>> unrelated with dfci, so maybe someone else must give a look before to
>> prevent me breaking anything.
>
>This is a more general issue, and I will respond to your question on
>emacs-devel.
>
>Thanks.

[-- Attachment #2: Screenshot_2019-08-07_18-10-20.png --]
[-- Type: image/png, Size: 7415 bytes --]

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> Date: Wed, 7 Aug 2019 18:20:33 +0200
> From: Ergus <spacibba@aol.com>
> Cc: 36858@debbugs.gnu.org, rotim.davor@gmail.com
> 
> in this code (in xdisp.c):
> 
> if (it->current_x < indicator_column_x)
>   {
>     it->face_id = merge_faces (it->w, Qextend_to_end_of_line,
>                                0, extend_face_merged_id);
> 
>     it->char_to_display = XFIXNAT (Vdisplay_fill_column_indicator_character);
>     PRODUCE_GLYPHS (it);
> 
>     it->face_id = extend_face_merged_id;
> }
> 
> I changed char_to_display:
> 
> it->char_to_display = (it->glyph_row->ends_at_zv_p) ? '1' : '0';

(There's no need to make any changes for that, you can simply invoke
dump-glyph-row or dump-glyph-matrix.)

> And then I obtained the attached image.

Right, I forgot where in the code we set that flag, and display of an
after-string at EOB indeed happens before that.

But since Dmitry says the case of Company mode doesn't need to be
fixed, I think this is a moot point now.  We should only solve the
issue with attributes being extended all the way towards the indicator
column.

Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

On Wed, Aug 07, 2019 at 07:37:04PM +0300, Eli Zaretskii wrote:
>> Date: Wed, 7 Aug 2019 18:20:33 +0200
>> From: Ergus <spacibba@aol.com>
>> Cc: 36858@debbugs.gnu.org, rotim.davor@gmail.com
>>
>> in this code (in xdisp.c):
>>
>> if (it->current_x < indicator_column_x)
>>   {
>>     it->face_id = merge_faces (it->w, Qextend_to_end_of_line,
>>                                0, extend_face_merged_id);
>>
>>     it->char_to_display = XFIXNAT (Vdisplay_fill_column_indicator_character);
>>     PRODUCE_GLYPHS (it);
>>
>>     it->face_id = extend_face_merged_id;
>> }
>>
>> I changed char_to_display:
>>
>> it->char_to_display = (it->glyph_row->ends_at_zv_p) ? '1' : '0';
>
>(There's no need to make any changes for that, you can simply invoke
>dump-glyph-row or dump-glyph-matrix.)
>
How is it?
>
>> And then I obtained the attached image.
>
>Right, I forgot where in the code we set that flag, and display of an
>after-string at EOB indeed happens before that.
>

This issue is already fixed with the other condition I mentioned:

IT_CHARPOS (*it) < ZV

But ends_at_zv_p this also need to be fixed because there are some tests
inside extend_face_to_end_of_line that compare with ends_at_zv_p. In the
worst case we need to remove these comparisons.

But ideally the flag must be set before right?

I think that there is another condition somewhere else that does not
call extend_face_to_end_of_line for the last line, probably due to the
same issue.

>But since Dmitry says the case of Company mode doesn't need to be
>fixed, I think this is a moot point now.  We should only solve the
>issue with attributes being extended all the way towards the indicator
>column.
>
Yes, I agree that we need to fix this first.

>Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> Date: Wed, 7 Aug 2019 19:06:54 +0200
> From: Ergus <spacibba@aol.com>
> Cc: 36858@debbugs.gnu.org, rotim.davor@gmail.com
> 
> >(There's no need to make any changes for that, you can simply invoke
> >dump-glyph-row or dump-glyph-matrix.)
> >
> How is it?

Just invoke these functions, they dump the glyph row's contents,
including the ends_at_zv_p, to stderr.

> This issue is already fixed with the other condition I mentioned:
> 
> IT_CHARPOS (*it) < ZV
> 
> But ends_at_zv_p this also need to be fixed because there are some tests
> inside extend_face_to_end_of_line that compare with ends_at_zv_p. In the
> worst case we need to remove these comparisons.

Not sure I understand why the comparisons need to be removed.  Can you
elaborate?

> But ideally the flag must be set before right?

In a buffer showing only buffer text (no after-strings at EOB), all
the glyph rows starting from the one showing EOB have their
ends_at_zv_p flag set.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

On Wed, Aug 07, 2019 at 08:29:13PM +0300, Eli Zaretskii wrote:
>> This issue is already fixed with the other condition I mentioned:
>>
>> IT_CHARPOS (*it) < ZV
>>
>> But ends_at_zv_p this also need to be fixed because there are some tests
>> inside extend_face_to_end_of_line that compare with ends_at_zv_p. In the
>> worst case we need to remove these comparisons.
>
>Not sure I understand why the comparisons need to be removed.  Can you
>elaborate?
>

>> But ideally the flag must be set before right?
>
>In a buffer showing only buffer text (no after-strings at EOB), all
>the glyph rows starting from the one showing EOB have their
>ends_at_zv_p flag set.

Hi:

In my tests inside extend_face_to_end_of_line the flag ends_at_zv_p is
always false. And for the last line (where it is supposed to be true)
the function extend_face_to_end_of_line is not called at all. So
actually all the code like:

	      if (it->glyph_row->ends_at_zv_p)
		it->face_id = default_face->id;
	      else
		it->face_id = face->id;

does nothing now.

We should fix this in order to create an indicator also for the last
line.

I think that the problem is in the condition:

if (!get_next_display_element (it)) inside display_line that filters the
call to  extend_face_to_end_of_line with:

if (row->reversed_p
    || lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID) !=
    DEFAULT_FACE_ID)
      extend_face_to_end_of_line (it);

And needs to be extended probably with with:

|| (!row_text_area_empty (row))

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

On Wed, Aug 07, 2019 at 09:46:21PM +0200, Ergus wrote:
>On Wed, Aug 07, 2019 at 08:29:13PM +0300, Eli Zaretskii wrote:
>>>This issue is already fixed with the other condition I mentioned:
>>>
>>>IT_CHARPOS (*it) < ZV
>>>
>>>But ends_at_zv_p this also need to be fixed because there are some tests
>>>inside extend_face_to_end_of_line that compare with ends_at_zv_p. In the
>>>worst case we need to remove these comparisons.
>>
>>Not sure I understand why the comparisons need to be removed.  Can you
>>elaborate?
>>
>
>>>But ideally the flag must be set before right?
>>
>>In a buffer showing only buffer text (no after-strings at EOB), all
>>the glyph rows starting from the one showing EOB have their
>>ends_at_zv_p flag set.
>
>Hi:
>
>In my tests inside extend_face_to_end_of_line the flag ends_at_zv_p is
>always false. And for the last line (where it is supposed to be true)
>the function extend_face_to_end_of_line is not called at all. So
>actually all the code like:
>
>	      if (it->glyph_row->ends_at_zv_p)
>		it->face_id = default_face->id;
>	      else
>		it->face_id = face->id;
>
>does nothing now.
>
>We should fix this in order to create an indicator also for the last
>line.
>
>I think that the problem is in the condition:
>
>if (!get_next_display_element (it)) inside display_line that filters the
>call to  extend_face_to_end_of_line with:
>
>if (row->reversed_p
>   || lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID) !=
>   DEFAULT_FACE_ID)
>     extend_face_to_end_of_line (it);
>
>And needs to be extended probably with with:
>
>|| (!row_text_area_empty (row))

Hi Eli:

Sorry for bothering so much.

After tying this solution I proposed yesterday to add the indicator also
for the latest line (call extend_face_to_end_of_line for a no empty line
without \n too) I get it working perfectly fine only in the tui
interface.

In gui it just doesn't work. But it seems that the issue is not due to
the condition:

if (!get_next_display_element (it));

because when I add a test glyph unconditionally there; it is not printed
in the screen, but when I add a message to stdout it is.

For sure I am missing something here, but what?

Is it possible that some optimization in the gui glue code stops
printing glyphs OR that some later code hides the extra glyphs?

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> Date: Wed, 7 Aug 2019 21:46:21 +0200
> From: Ergus <spacibba@aol.com>
> Cc: 36858@debbugs.gnu.org, rotim.davor@gmail.com
> 
> >In a buffer showing only buffer text (no after-strings at EOB), all
> >the glyph rows starting from the one showing EOB have their
> >ends_at_zv_p flag set.
> 
> Hi:
> 
> In my tests inside extend_face_to_end_of_line the flag ends_at_zv_p is
> always false. And for the last line (where it is supposed to be true)
> the function extend_face_to_end_of_line is not called at all. So
> actually all the code like:
> 
> 	      if (it->glyph_row->ends_at_zv_p)
> 		it->face_id = default_face->id;
> 	      else
> 		it->face_id = face->id;
> 
> does nothing now.

I see 2 calls to extend_face_to_end_of_line that have an explicit
setting of the ends_at_zv_p flag to 'true' right before the call.  The
fact that you don't see that just means that you are not trying the
use cases where those code fragments are executed.  Which doesn't
surprise me, since the Emacs display engine supports dozens of rare
and subtle use cases, some of which are not easy to even reproduce.

> I think that the problem is in the condition:
> 
> if (!get_next_display_element (it)) inside display_line that filters the
> call to  extend_face_to_end_of_line with:
> 
> if (row->reversed_p
>     || lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID) !=
>     DEFAULT_FACE_ID)
>       extend_face_to_end_of_line (it);
> 
> And needs to be extended probably with with:
> 
> || (!row_text_area_empty (row))

Not sure about which place you are talking: there are several places
in xdisp.c where we call get_next_display_element and test that its
value is zero.

In any case, if we are going to call extend_face_to_end_of_line in
some of the places where we currently don't do that at EOB, we should
condition of the display-fill-column-indicator-mode being ON as well.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> Date: Thu, 8 Aug 2019 09:17:58 +0200
> From: Ergus <spacibba@aol.com>
> Cc: 36858@debbugs.gnu.org, rotim.davor@gmail.com
> 
> After tying this solution I proposed yesterday to add the indicator also
> for the latest line (call extend_face_to_end_of_line for a no empty line
> without \n too) I get it working perfectly fine only in the tui
> interface.
> 
> In gui it just doesn't work. But it seems that the issue is not due to
> the condition:
> 
> if (!get_next_display_element (it));
> 
> because when I add a test glyph unconditionally there; it is not printed
> in the screen, but when I add a message to stdout it is.
> 
> For sure I am missing something here, but what?
> 
> Is it possible that some optimization in the gui glue code stops
> printing glyphs OR that some later code hides the extra glyphs?

I don't think I understand what you tried well enough to answer the
question.  Can you show a patch relative to the current master?

Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

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

On Thu, Aug 08, 2019 at 08:33:38PM +0300, Eli Zaretskii wrote:
>> Date: Thu, 8 Aug 2019 09:17:58 +0200
>> From: Ergus <spacibba@aol.com>
>> Cc: 36858@debbugs.gnu.org, rotim.davor@gmail.com
>>
>> After tying this solution I proposed yesterday to add the indicator also
>> for the latest line (call extend_face_to_end_of_line for a no empty line
>> without \n too) I get it working perfectly fine only in the tui
>> interface.
>>
>> In gui it just doesn't work. But it seems that the issue is not due to
>> the condition:
>>
>> if (!get_next_display_element (it));
>>
>> because when I add a test glyph unconditionally there; it is not printed
>> in the screen, but when I add a message to stdout it is.
>>
>> For sure I am missing something here, but what?
>>
>> Is it possible that some optimization in the gui glue code stops
>> printing glyphs OR that some later code hides the extra glyphs?
>
>I don't think I understand what you tried well enough to answer the
>question.  Can you show a patch relative to the current master?
>
See the attachement

>Thanks.

[-- Attachment #2: last_line.patch --]
[-- Type: text/plain, Size: 1388 bytes --]

diff --git a/src/xdisp.c b/src/xdisp.c
index 7338d2b7d4..a39f2e0bd9 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -21837,17 +21837,17 @@ display_line (struct it *it, int cursor_vpos)
 	 buffer reached.  */
       if (!get_next_display_element (it))
 	{
-	  bool row_has_glyphs = false;
+	  const bool row_has_glyphs = row_text_area_empty (row);
 	  /* Maybe add a space at the end of this line that is used to
 	     display the cursor there under X.  Set the charpos of the
 	     first glyph of blank lines not corresponding to any text
 	     to -1.  */
 	  if (IT_OVERFLOW_NEWLINE_INTO_FRINGE (it))
 	    row->exact_window_width_line_p = true;
-	  else if ((append_space_for_newline (it, true)
+	  else if (row_has_glyphs
+	           ||(append_space_for_newline (it, true)
 		    && row->used[TEXT_AREA] == 1)
-		   || row->used[TEXT_AREA] == 0
-		   || (row_has_glyphs = row_text_area_empty (row)))
+		   || row->used[TEXT_AREA] == 0)
 	    {
 	      row->glyphs[TEXT_AREA]->charpos = -1;
 	      /* Don't reset the displays_text_p flag if we are
@@ -21878,7 +21878,8 @@ display_line (struct it *it, int cursor_vpos)
 	     background color.  */
 	  if (row->reversed_p
 	      || lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID)
-              != DEFAULT_FACE_ID)
+              != DEFAULT_FACE_ID
+	      || !row_has_glyphs)
 	    extend_face_to_end_of_line (it);
 	  break;
 	}

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

On Thu, Aug 08, 2019 at 08:31:02PM +0300, Eli Zaretskii wrote:
>> Date: Wed, 7 Aug 2019 21:46:21 +0200
>> From: Ergus <spacibba@aol.com>
>> Cc: 36858@debbugs.gnu.org, rotim.davor@gmail.com
>>
>> >In a buffer showing only buffer text (no after-strings at EOB), all
>> >the glyph rows starting from the one showing EOB have their
>> >ends_at_zv_p flag set.
>>
>> Hi:
>>
>> In my tests inside extend_face_to_end_of_line the flag ends_at_zv_p is
>> always false. And for the last line (where it is supposed to be true)
>> the function extend_face_to_end_of_line is not called at all. So
>> actually all the code like:
>>
>> 	      if (it->glyph_row->ends_at_zv_p)
>> 		it->face_id = default_face->id;
>> 	      else
>> 		it->face_id = face->id;
>>
>> does nothing now.
>
>I see 2 calls to extend_face_to_end_of_line that have an explicit
>setting of the ends_at_zv_p flag to 'true' right before the call.  The
>fact that you don't see that just means that you are not trying the
>use cases where those code fragments are executed.  Which doesn't
>surprise me, since the Emacs display engine supports dozens of rare
>and subtle use cases, some of which are not easy to even reproduce.
>
>> I think that the problem is in the condition:
>>
>> if (!get_next_display_element (it)) inside display_line that filters the
>> call to  extend_face_to_end_of_line with:
>>
>> if (row->reversed_p
>>     || lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID) !=
>>     DEFAULT_FACE_ID)
>>       extend_face_to_end_of_line (it);
>>
>> And needs to be extended probably with with:
>>
>> || (!row_text_area_empty (row))
>
>Not sure about which place you are talking: there are several places
>in xdisp.c where we call get_next_display_element and test that its
>value is zero.
>
>In any case, if we are going to call extend_face_to_end_of_line in
>some of the places where we currently don't do that at EOB, we should
>condition of the display-fill-column-indicator-mode being ON as well.

Yes I know. I was just trying to find the key condition for our case.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> From: Davor Rotim <rotim.davor@gmail.com>
> Date: Tue, 30 Jul 2019 20:11:04 +0200
> 
> Hello, in the attached images are two cases I noticed where `display-fill-column-indicator-mode' causes
> display bugs. First case is with faces that use the :overline or :underline property, the lines will extend fully
> towards the indicator. Second case is with `company-mode' when there's no text entered and the completion
> dialog pops up which display-fill-column-indicator-mode treats like ordinary text.

Can you please show a minimal Org file to reproduce the issue with Org
mode display?

Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Davor Rotim]

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

Of course, this is the test example I used in the screenshot:

Aliases
#+BEGIN_SRC sh
alias cp="cp --interactive"
alias mv="mv --interactive"
alias rm="rm -I"
#+END_SRC

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

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> X-Spam-Status: No, score=0.8 required=5.0 tests=BAYES_50,FREEMAIL_FROM,
> 	HTML_MESSAGE autolearn=disabled version=3.3.2
> Of course, this is the test example I used in the screenshot:
> 
> Aliases
> #+BEGIN_SRC sh
> alias cp="cp --interactive"
> alias mv="mv --interactive"
> alias rm="rm -I"
> #+END_SRC

Thanks, but this doesn't reproduce the problem on my system.  I've put
the above on a file foo.org, visited it from "emacs -Q" (which turns
on the Org mode automatically), then typed "M-x
display-fill-column-indicator-mode RET".  This displayed the
indicators, but didn't show any underlines.  I'm guessing there's
something else missing, something you do in your customizations.
Could you please show the minimal customizations that are necessary to
show the faces as I see them in your original report?

Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Davor Rotim]

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

Sorry, forgot to list the relevant org-mode faces that I have customized:

First one is `org-block-begin-line'

Family: unspecified
Foundry: unspecified
Width: unspecified
Height: unspecified
Weight: unspecified
Slant: italic
Foreground: #969896
DistantForeground: unspecified
Background: #373b41
Underline: t
Overline: unspecified
Strike-through: unspecified
Box: unspecified
Inverse: unspecified
Stipple: unspecified
Font: unspecified
Fontset: unspecified
Inherit: unspecified

Second one is `org-block-end-line'

Family: unspecified
Foundry: unspecified
Width: unspecified
Height: unspecified
Weight: unspecified
Slant: italic
Foreground: #969896
DistantForeground: unspecified
Background: #373b41
Underline: unspecified
Overline: t
Strike-through: unspecified
Box: unspecified
Inverse: unspecified
Stipple: unspecified
Font: unspecified
Fontset: unspecified
Inherit: unspecified

Please note that this is not relevant to only Org mode, this display bug
happens with all faces
where I turn on the overline or underline property. For example
`font-lock-comment-face' will show
the same bug if I turn on the overline or underline property.

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

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> From: Davor Rotim <rotim.davor@gmail.com>
> Date: Sat, 10 Aug 2019 12:12:55 +0200
> 
> Sorry, forgot to list the relevant org-mode faces that I have customized:

Thanks.

> Please note that this is not relevant to only Org mode, this display bug happens with all faces
> where I turn on the overline or underline property. For example `font-lock-comment-face' will show
> the same bug if I turn on the overline or underline property.

I understand, I just need an easy example to work with.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> Date: Sat, 10 Aug 2019 13:45:37 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 36858@debbugs.gnu.org
> 
> > From: Davor Rotim <rotim.davor@gmail.com>
> > Date: Sat, 10 Aug 2019 12:12:55 +0200
> > 
> > Sorry, forgot to list the relevant org-mode faces that I have customized:
> 
> Thanks.
> 
> > Please note that this is not relevant to only Org mode, this display bug happens with all faces
> > where I turn on the overline or underline property. For example `font-lock-comment-face' will show
> > the same bug if I turn on the overline or underline property.
> 
> I understand, I just need an easy example to work with.

OK, I see the reason for the problem now: it's because org.el places
the face on the newline that ends the line, not just on the text of
that line.

Can one of the Org developers (CC'ed) please tell why Org does this?
Why not limit the face to the actual text, and avoid putting the face
on the newline?

Thanks.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Carsten Dominik]

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

On Sat, Aug 10, 2019 at 1:53 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Sat, 10 Aug 2019 13:45:37 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 36858@debbugs.gnu.org
> >
> > > From: Davor Rotim <rotim.davor@gmail.com>
> > > Date: Sat, 10 Aug 2019 12:12:55 +0200
> > >
> > > Sorry, forgot to list the relevant org-mode faces that I have
> customized:
> >
> > Thanks.
> >
> > > Please note that this is not relevant to only Org mode, this display
> bug happens with all faces
> > > where I turn on the overline or underline property. For example
> `font-lock-comment-face' will show
> > > the same bug if I turn on the overline or underline property.
> >
> > I understand, I just need an easy example to work with.
>
> OK, I see the reason for the problem now: it's because org.el places
> the face on the newline that ends the line, not just on the text of
> that line.
>
> Can one of the Org developers (CC'ed) please tell why Org does this?
> Why not limit the face to the actual text, and avoid putting the face
> on the newline?
>

Because it looks good.  The begin/end lines delineate a block, and if you
use a background color, then the color goes all across the window, which I
think looks good and shows the structure better.

Carsten


> Thanks.
>

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

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> From: Carsten Dominik <carsten.dominik@gmail.com>
> Date: Sat, 10 Aug 2019 15:21:14 +0200
> Cc: Nicolas Goaziou <mail@nicolasgoaziou.fr>, rotim.davor@gmail.com, Ergus <spacibba@aol.com>, 
> 	36858@debbugs.gnu.org
> 
>  Can one of the Org developers (CC'ed) please tell why Org does this?
>  Why not limit the face to the actual text, and avoid putting the face
>  on the newline?
> 
> Because it looks good.  The begin/end lines delineate a block, and if you use a background color, then the
> color goes all across the window, which I think looks good and shows the structure better.

But that happens only if the face specifies a background color.  If it
specifies, say, :underline instead, on GUI frames it just extends one
character cell beyond the last character, and on TTY frames it goes to
the end of the window, i.e. behaves inconsistently.  And with
display-fill-column-indicator-mode turned on, on GUI frames it goes
half-way till the fill column: yet another inconsistent behavior.

So if you think the current display looks good, how about making it
optional?  Then this could be turned off to avoid the inconsistent
display in those use cases where it matters.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Carsten Dominik]

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

Hi Eli,

On Sat, Aug 10, 2019 at 3:38 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Carsten Dominik <carsten.dominik@gmail.com>
> > Date: Sat, 10 Aug 2019 15:21:14 +0200
> > Cc: Nicolas Goaziou <mail@nicolasgoaziou.fr>, rotim.davor@gmail.com,
> Ergus <spacibba@aol.com>,
> >       36858@debbugs.gnu.org
> >
> >  Can one of the Org developers (CC'ed) please tell why Org does this?
> >  Why not limit the face to the actual text, and avoid putting the face
> >  on the newline?
> >
> > Because it looks good.  The begin/end lines delineate a block, and if
> you use a background color, then the
> > color goes all across the window, which I think looks good and shows the
> structure better.
>
> But that happens only if the face specifies a background color.  If it
> specifies, say, :underline instead, on GUI frames it just extends one
> character cell beyond the last character, and on TTY frames it goes to
> the end of the window, i.e. behaves inconsistently.  And with
> display-fill-column-indicator-mode turned on, on GUI frames it goes
> half-way till the fill column: yet another inconsistent behavior.
>
> So if you think the current display looks good, how about making it
> optional?  Then this could be turned off to avoid the inconsistent
> display in those use cases where it matters.
>

It is possible to make it optional.  However, it seems to me that this is a
bug in the display engine that should be fixed anyway, don't you agree?

Carsten

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

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> From: Carsten Dominik <carsten.dominik@gmail.com>
> Date: Sat, 10 Aug 2019 16:17:18 +0200
> Cc: Nicolas Goaziou <mail@nicolasgoaziou.fr>, rotim.davor@gmail.com, Ergus <spacibba@aol.com>, 
> 	36858@debbugs.gnu.org
> 
>  So if you think the current display looks good, how about making it
>  optional?  Then this could be turned off to avoid the inconsistent
>  display in those use cases where it matters.
> 
> It is possible to make it optional.  However, it seems to me that this is a bug in the display engine that should
> be fixed anyway, don't you agree?

It's not a bug, it's how the display engine was designed to work.  If
we decide to change the design, and stop extending the face of the
last character to the edge of the window, when the face covers the
newline, then the issue with display-fill-column-indicator-mode in Org
mode will also go away.  But we haven't yet made such a decision, see
the discussion which starts here:

  https://lists.gnu.org/archive/html/emacs-devel/2019-08/msg00132.html

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Ergus]

On Sat, Aug 10, 2019 at 04:38:35PM +0300, Eli Zaretskii wrote:
>> From: Carsten Dominik <carsten.dominik@gmail.com>
>> Date: Sat, 10 Aug 2019 15:21:14 +0200
>> Cc: Nicolas Goaziou <mail@nicolasgoaziou.fr>, rotim.davor@gmail.com, Ergus <spacibba@aol.com>,
>> 	36858@debbugs.gnu.org
>>
>>  Can one of the Org developers (CC'ed) please tell why Org does this?
>>  Why not limit the face to the actual text, and avoid putting the face
>>  on the newline?
>>
>> Because it looks good.  The begin/end lines delineate a block, and if you use a background color, then the
>> color goes all across the window, which I think looks good and shows the structure better.
>
>But that happens only if the face specifies a background color.  If it
>specifies, say, :underline instead, on GUI frames it just extends one
>character cell beyond the last character, and on TTY frames it goes to
>the end of the window, i.e. behaves inconsistently.  And with
>display-fill-column-indicator-mode turned on, on GUI frames it goes
>half-way till the fill column: yet another inconsistent behavior.
>
>So if you think the current display looks good, how about making it
>optional?  Then this could be turned off to avoid the inconsistent
>display in those use cases where it matters.

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Carsten Dominik]

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

Hi Eli,

I have now made this configurable in org-mode, through a variable
`org-fontify-whole-block-delimiter-line'.
The default is t, because this is how Org have been working for a while now.

- Carsten

On Sat, Aug 10, 2019 at 4:42 PM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Carsten Dominik <carsten.dominik@gmail.com>
> > Date: Sat, 10 Aug 2019 16:17:18 +0200
> > Cc: Nicolas Goaziou <mail@nicolasgoaziou.fr>, rotim.davor@gmail.com,
> Ergus <spacibba@aol.com>,
> >       36858@debbugs.gnu.org
> >
> >  So if you think the current display looks good, how about making it
> >  optional?  Then this could be turned off to avoid the inconsistent
> >  display in those use cases where it matters.
> >
> > It is possible to make it optional.  However, it seems to me that this
> is a bug in the display engine that should
> > be fixed anyway, don't you agree?
>
> It's not a bug, it's how the display engine was designed to work.  If
> we decide to change the design, and stop extending the face of the
> last character to the edge of the window, when the face covers the
> newline, then the issue with display-fill-column-indicator-mode in Org
> mode will also go away.  But we haven't yet made such a decision, see
> the discussion which starts here:
>
>   https://lists.gnu.org/archive/html/emacs-devel/2019-08/msg00132.html
>

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

bug#36858: 27.0.50; display bugs with display-fill-column-indicator-mode

[Eli Zaretskii]

> From: Carsten Dominik <carsten.dominik@gmail.com>
> Date: Mon, 12 Aug 2019 09:10:36 +0200
> Cc: Nicolas Goaziou <mail@nicolasgoaziou.fr>, rotim.davor@gmail.com, Ergus <spacibba@aol.com>, 
> 	36858@debbugs.gnu.org
> 
> I have now made this configurable in org-mode, through a variable `org-fontify-whole-block-delimiter-line'.

Thank you.

> The default is t, because this is how Org have been working for a while now.

I agree that the default should be t for reasons of backward
compatibility.

bug#36858: (no subject)

[Ergus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Fixed with the :extend attribute added to faces and merged in master in
commit: 3d6075e3ee8c447f8974b37007a1b1ae1af8917c