bug#70637: :box vertical bar artifacts at 'display boundaries

bug#70637: :box vertical bar artifacts at 'display boundaries

[JD Smith]

I have identified a display bug related to placing :box face parameters within 'display strings.  Disable font-lock mode in *scratch* and evaluate:

(insert "\n"
 (propertize " " 'display (propertize "   " 'face '(:box t)))
 (propertize "middle" 'face '(:box t))
 (propertize " " 'display (propertize "   " 'face '(:box t))))

So far so good: the box is correctly merged across all three elements and wraps around them.  But now move point within and across the displayed boxed text.  Internal vertical divisions bars separating the 'display and normal :box regions appear.  

If you add a face box property to the blank flanking strings as well, this prevents the internal boundary from appearing. 

bug#70637: :box vertical bar artifacts at 'display boundaries

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sun, 28 Apr 2024 20:50:31 -0400
> 
> 
> I have identified a display bug related to placing :box face parameters within 'display strings.  Disable font-lock mode in *scratch* and evaluate:
> 
> (insert "\n"
>  (propertize " " 'display (propertize "   " 'face '(:box t)))
>  (propertize "middle" 'face '(:box t))
>  (propertize " " 'display (propertize "   " 'face '(:box t))))
> 
> So far so good: the box is correctly merged across all three elements and wraps around them.  But now move point within and across the displayed boxed text.  Internal vertical divisions bars separating the 'display and normal :box regions appear.  
> 
> If you add a face box property to the blank flanking strings as well, this prevents the internal boundary from appearing. 

How important is it to fix this use case (as opposed to using the
workaround you describe in the last sentence)?  The price for fixing
it would be that we will need to redraw more than the single glyph
below the cursor when showing the cursor (which with the default
blink-cursor-mode happens twice a second), which will cause flickering
around the cursor, and I wonder whether it's justified?

bug#70637: :box vertical bar artifacts at 'display boundaries

[JD Smith]

> On Apr 29, 2024, at 7:40 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Sun, 28 Apr 2024 20:50:31 -0400
>> 
>> 
>> I have identified a display bug related to placing :box face parameters within 'display strings.  Disable font-lock mode in *scratch* and evaluate:
>> 
>> (insert "\n"
>> (propertize " " 'display (propertize "   " 'face '(:box t)))
>> (propertize "middle" 'face '(:box t))
>> (propertize " " 'display (propertize "   " 'face '(:box t))))
>> 
>> So far so good: the box is correctly merged across all three elements and wraps around them.  But now move point within and across the displayed boxed text.  Internal vertical divisions bars separating the 'display and normal :box regions appear.  
>> 
>> If you add a face box property to the blank flanking strings as well, this prevents the internal boundary from appearing.
> 
> How important is it to fix this use case (as opposed to using the
> workaround you describe in the last sentence)?  The price for fixing
> it would be that we will need to redraw more than the single glyph
> below the cursor when showing the cursor (which with the default
> blink-cursor-mode happens twice a second), which will cause flickering
> around the cursor, and I wonder whether it's justified?

The workaround may be enough for most cases, unless you can't apply an overall 'face for some reason.  Any sense why it leaves the vertical bars behind?

bug#70637: :box vertical bar artifacts at 'display boundaries

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Mon, 29 Apr 2024 08:01:09 -0400
> Cc: 70637@debbugs.gnu.org
> 
> 
> 
> > On Apr 29, 2024, at 7:40 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> >> From: JD Smith <jdtsmith@gmail.com>
> >> Date: Sun, 28 Apr 2024 20:50:31 -0400
> >> 
> >> 
> >> I have identified a display bug related to placing :box face parameters within 'display strings.  Disable font-lock mode in *scratch* and evaluate:
> >> 
> >> (insert "\n"
> >> (propertize " " 'display (propertize "   " 'face '(:box t)))
> >> (propertize "middle" 'face '(:box t))
> >> (propertize " " 'display (propertize "   " 'face '(:box t))))
> >> 
> >> So far so good: the box is correctly merged across all three elements and wraps around them.  But now move point within and across the displayed boxed text.  Internal vertical divisions bars separating the 'display and normal :box regions appear.  
> >> 
> >> If you add a face box property to the blank flanking strings as well, this prevents the internal boundary from appearing.
> > 
> > How important is it to fix this use case (as opposed to using the
> > workaround you describe in the last sentence)?  The price for fixing
> > it would be that we will need to redraw more than the single glyph
> > below the cursor when showing the cursor (which with the default
> > blink-cursor-mode happens twice a second), which will cause flickering
> > around the cursor, and I wonder whether it's justified?
> 
> The workaround may be enough for most cases, unless you can't apply an overall 'face for some reason.

Why would we be unable to apply the box face on characters that are
not actually shown?

> Any sense why it leaves the vertical bars behind?

This happens when the glyph under cursor has the beginning-of-box or
end-of-box flag set.  When we display the entire stretch of characters
on that line, we (correctly) don't pay attention to these flags in the
middle of the glyph sequence, but redrawing the cursor draws just one
glyph, and knows nothing about those before or after it.  So it draws
the unnecessary border, because the glyph under cursor has the flag
set.

Those box flags are set on the glyphs produced from the display
strings because when we process the beginning or end of the string, we
don't have any idea whether the characters of the underlying buffer
text before/after the string have the same value of the :box face, so
we cannot avoid setting these flags at the first and the last
character of the display string.

bug#70637: :box vertical bar artifacts at 'display boundaries

[JD Smith]

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



> On Apr 29, 2024, at 9:12 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> 
>> The workaround may be enough for most cases, unless you can't apply an overall 'face for some reason.
> 
> Why would we be unable to apply the box face on characters that are
> not actually shown?

I guess I should rephrase to "if it's inconvenient to apply an overall face with :box", e.g. if you're adding on the output of another package.  There's also of course the need to discover the workaround.

>> Any sense why it leaves the vertical bars behind?
> 
> This happens when the glyph under cursor has the beginning-of-box or
> end-of-box flag set.  When we display the entire stretch of characters
> on that line, we (correctly) don't pay attention to these flags in the
> middle of the glyph sequence, but redrawing the cursor draws just one
> glyph, and knows nothing about those before or after it.  So it draws
> the unnecessary border, because the glyph under cursor has the flag
> set.
> 
> Those box flags are set on the glyphs produced from the display
> strings because when we process the beginning or end of the string, we
> don't have any idea whether the characters of the underlying buffer
> text before/after the string have the same value of the :box face, so
> we cannot avoid setting these flags at the first and the last
> character of the display string.
 
I see, makes sense.  So the cursor blink code would also have to "look ahead/behind" the underlying glyph to know whether to ignore the flag.  Probably this is such a rare case that unless there are other related artifacts, it's worth documenting but not fixing.

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

bug#70637: :box vertical bar artifacts at 'display boundaries

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Mon, 29 Apr 2024 11:19:52 -0400
> Cc: 70637@debbugs.gnu.org
> 
>  This happens when the glyph under cursor has the beginning-of-box or
>  end-of-box flag set.  When we display the entire stretch of characters
>  on that line, we (correctly) don't pay attention to these flags in the
>  middle of the glyph sequence, but redrawing the cursor draws just one
>  glyph, and knows nothing about those before or after it.  So it draws
>  the unnecessary border, because the glyph under cursor has the flag
>  set.
> 
>  Those box flags are set on the glyphs produced from the display
>  strings because when we process the beginning or end of the string, we
>  don't have any idea whether the characters of the underlying buffer
>  text before/after the string have the same value of the :box face, so
>  we cannot avoid setting these flags at the first and the last
>  character of the display string.
> 
>  
> I see, makes sense.  So the cursor blink code would also have to "look ahead/behind" the underlying glyph to
> know whether to ignore the flag.

It's not just to "look", it's actually to redraw.  because the logic
which determines whether we draw the borders lives in the code that
draws the glyphs on the glass, and to DTRT it needs to be presented
with a sequence of glyphs that begins before the one under cursor and
ends after it.

> Probably this is such a rare case that unless there are other related
> artifacts, it's worth documenting but not fixing.

Suggestions for how to document this are welcome.

bug#70637: :box vertical bar artifacts at 'display boundaries

[Eli Zaretskii]

> Cc: 70637@debbugs.gnu.org
> Date: Mon, 29 Apr 2024 18:34:40 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: JD Smith <jdtsmith@gmail.com>
> > Date: Mon, 29 Apr 2024 11:19:52 -0400
> > Cc: 70637@debbugs.gnu.org
> > 
> >  This happens when the glyph under cursor has the beginning-of-box or
> >  end-of-box flag set.  When we display the entire stretch of characters
> >  on that line, we (correctly) don't pay attention to these flags in the
> >  middle of the glyph sequence, but redrawing the cursor draws just one
> >  glyph, and knows nothing about those before or after it.  So it draws
> >  the unnecessary border, because the glyph under cursor has the flag
> >  set.
> > 
> >  Those box flags are set on the glyphs produced from the display
> >  strings because when we process the beginning or end of the string, we
> >  don't have any idea whether the characters of the underlying buffer
> >  text before/after the string have the same value of the :box face, so
> >  we cannot avoid setting these flags at the first and the last
> >  character of the display string.
> > 
> >  
> > I see, makes sense.  So the cursor blink code would also have to "look ahead/behind" the underlying glyph to
> > know whether to ignore the flag.
> 
> It's not just to "look", it's actually to redraw.  because the logic
> which determines whether we draw the borders lives in the code that
> draws the glyphs on the glass, and to DTRT it needs to be presented
> with a sequence of glyphs that begins before the one under cursor and
> ends after it.
> 
> > Probably this is such a rare case that unless there are other related
> > artifacts, it's worth documenting but not fixing.
> 
> Suggestions for how to document this are welcome.

Ping!

bug#70637: :box vertical bar artifacts at 'display boundaries

[JD Smith]

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

I presume this is a more general issue than just :box.  One idea is to add a warning to the Elisp section "Display Specs That Replace The Text", perhaps at the end:

Note: certain `face' attributes such as `:box' can lead to display artifacts when applied to the replacing text in a `display' specification.  These attributes may be incorrectly merged with adjacent non-`display' `face' properties.  This can be mitigated by applying the `face' attributes directly to the text being replaced, rather than (or in addition to) the `display' replacement text itself.

Maybe a bit too wordy.

> On May 9, 2024, at 3:36 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> Cc: 70637@debbugs.gnu.org
>> Date: Mon, 29 Apr 2024 18:34:40 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>>> From: JD Smith <jdtsmith@gmail.com>
>>> Date: Mon, 29 Apr 2024 11:19:52 -0400
>>> Cc: 70637@debbugs.gnu.org
>>> 
>>> This happens when the glyph under cursor has the beginning-of-box or
>>> end-of-box flag set.  When we display the entire stretch of characters
>>> on that line, we (correctly) don't pay attention to these flags in the
>>> middle of the glyph sequence, but redrawing the cursor draws just one
>>> glyph, and knows nothing about those before or after it.  So it draws
>>> the unnecessary border, because the glyph under cursor has the flag
>>> set.
>>> 
>>> Those box flags are set on the glyphs produced from the display
>>> strings because when we process the beginning or end of the string, we
>>> don't have any idea whether the characters of the underlying buffer
>>> text before/after the string have the same value of the :box face, so
>>> we cannot avoid setting these flags at the first and the last
>>> character of the display string.
>>> 
>>> 
>>> I see, makes sense.  So the cursor blink code would also have to "look ahead/behind" the underlying glyph to
>>> know whether to ignore the flag.
>> 
>> It's not just to "look", it's actually to redraw.  because the logic
>> which determines whether we draw the borders lives in the code that
>> draws the glyphs on the glass, and to DTRT it needs to be presented
>> with a sequence of glyphs that begins before the one under cursor and
>> ends after it.
>> 
>>> Probably this is such a rare case that unless there are other related
>>> artifacts, it's worth documenting but not fixing.
>> 
>> Suggestions for how to document this are welcome.
> 
> Ping!


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

bug#70637: :box vertical bar artifacts at 'display boundaries

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Thu, 9 May 2024 09:31:28 -0400
> Cc: 70637@debbugs.gnu.org
> 
> I presume this is a more general issue than just :box.  One idea is to add a warning to the Elisp section "Display
> Specs That Replace The Text", perhaps at the end:
> 
>  Note: certain `face' attributes such as `:box' can lead to display artifacts when applied to the replacing
>  text in a `display' specification.  These attributes may be incorrectly merged with adjacent non-`display'
>  `face' properties.  This can be mitigated by applying the `face' attributes directly to the text being
>  replaced, rather than (or in addition to) the `display' replacement text itself.

Actually, this is specific to :box, since only for that attribute we
need to determine the beginning and the end of the box.

> Maybe a bit too wordy.

Yes, I agree.  If you can reword it to be specific to :box and to
include an example, I think it would be good.

Thanks.

bug#70637: :box vertical bar artifacts at 'display boundaries

[JD Smith]

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



> On May 11, 2024, at 5:41 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: JD Smith <jdtsmith@gmail.com>
>> Date: Thu, 9 May 2024 09:31:28 -0400
>> Cc: 70637@debbugs.gnu.org
>> 
>> I presume this is a more general issue than just :box.  One idea is to add a warning to the Elisp section "Display
>> Specs That Replace The Text", perhaps at the end:
>> 
>> Note: certain `face' attributes such as `:box' can lead to display artifacts when applied to the replacing
>> text in a `display' specification.  These attributes may be incorrectly merged with adjacent non-`display'
>> `face' properties.  This can be mitigated by applying the `face' attributes directly to the text being
>> replaced, rather than (or in addition to) the `display' replacement text itself.
> 
> Actually, this is specific to :box, since only for that attribute we
> need to determine the beginning and the end of the box.
> 
>> Maybe a bit too wordy.
> 
> Yes, I agree.  If you can reword it to be specific to :box and to
> include an example, I think it would be good.


Feel free to edit:

Note: Using `:box' attributes in replacing `display' text adjacent to normal text with the same `:box' styling can lead to display artifacts.  These can be avoided by applying the `:box' attribute directly to the text being replaced, rather than (or in addition to) the `display' replacement text itself.

;; Causes :box artifacts from the cursor
(progn
  (put-text-property 1 2 'display (propertize "  [" 'face '(:box t)))
  (put-text-property 2 3 'face '(:box t))
  (put-text-property 3 4 'display (propertize "]  " 'face '(:box t))))

;; No :box artifacts
(progn
  (add-text-properties 1 2 '(face (:box t) display "  ["))
  (put-text-property 2 3 'face '(:box t))
  (add-text-properties 3 4 '(face (:box t) display "]  ")))


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

bug#70637: :box vertical bar artifacts at 'display boundaries

[Eli Zaretskii]

> From: JD Smith <jdtsmith@gmail.com>
> Date: Sat, 11 May 2024 13:16:54 -0400
> Cc: 70637@debbugs.gnu.org
> 
> > On May 11, 2024, at 5:41 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > 
> >> From: JD Smith <jdtsmith@gmail.com>
> >> Date: Thu, 9 May 2024 09:31:28 -0400
> >> Cc: 70637@debbugs.gnu.org
> >> 
> >> I presume this is a more general issue than just :box.  One idea is to add a warning to the Elisp section "Display
> >> Specs That Replace The Text", perhaps at the end:
> >> 
> >> Note: certain `face' attributes such as `:box' can lead to display artifacts when applied to the replacing
> >> text in a `display' specification.  These attributes may be incorrectly merged with adjacent non-`display'
> >> `face' properties.  This can be mitigated by applying the `face' attributes directly to the text being
> >> replaced, rather than (or in addition to) the `display' replacement text itself.
> > 
> > Actually, this is specific to :box, since only for that attribute we
> > need to determine the beginning and the end of the box.
> > 
> >> Maybe a bit too wordy.
> > 
> > Yes, I agree.  If you can reword it to be specific to :box and to
> > include an example, I think it would be good.
> 
> 
> Feel free to edit:

Thanks, I installed a variant of this, and I'm therefore closing this
bug.