bug#22320: Overlays with an 'invisible property break stacking of overlay faces

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Clément Pit--Claudel]

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

Hi all,

Adding an 'invisible property to an overlay seems to break stacking of faces.
To reproduce, run the following snippet. The whole buffer should have a red
background, but the invisible section does not inherit that background.

(with-current-buffer (get-buffer-create "bug")
  (erase-buffer)
  (fundamental-mode)
  (add-to-invisibility-spec '(outline . t))
  (insert "line1 line2 line3")
  (let ((ov (make-overlay 7 12)))
    (overlay-put ov 'invisible 'outline))
  (let ((ov (make-overlay 7 8)))
    (overlay-put ov 'face '(:underline t)))
  (let ((ov (make-overlay (point-min) (point-max))))
    (overlay-put ov 'face '(:background "red")))
  (pop-to-buffer (current-buffer)))

I initially thought this was an issue of priorities, but removing the overlay with
the invisible property makes the issue disappear. Similarly, removing the
underline overlay makes the issue disappear.

Here is another (more "real-world") way to reproduce this issue (run the
snippet, then use C-x h in the newly created buffer to mark all text: the
invisible section isn't highlighted)

(with-current-buffer (get-buffer-create "org-bug")
  (erase-buffer)
  (org-mode)
  (insert "* line1\n** line2\n* line3")
  (org-shifttab)
  (pop-to-buffer (current-buffer)))

Yet another way it to reproduce it is given below

(with-current-buffer (get-buffer-create "flyspell-bug")
  (erase-buffer)
  (text-mode)
  (add-to-invisibility-spec '(outline . t))
  (insert "line1\nline2\nline3")
  (flyspell-check-region-doublons (point-min) (point-max))
  (let ((ov (make-overlay 7 12)))
    (overlay-put ov 'invisible 'outline))
  (pop-to-buffer (current-buffer)))

Interestingly, the first example is broken in both 24.4 and emacs-25; but the second one is broken only in emacs-25 (the third one does not run in Emacs 24.4).

I wonder if this is connected to another issue that I've seen, where having a composition (using prettify-symbols-mode) just before an invisible region would cause the background of the invisible region to be incorrect, but setting prettify-symbols-unprettify-at-point and moving the point to the edge of the symbol would cause the background to be fine. Unfortunately I don't have a good repro for that issue.

Cheers,
Clément.

In GNU Emacs 25.0.50.8 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
 of 2015-12-29 built on clem-w50-mint
Repository revision: a21bb238ce7bcc9c13a9cf66db77918304daa2fc
Windowing system distributor 'The X.Org Foundation', version 11.0.11501000
System Description:	Linux Mint 17.2 Rafaela

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  recentf-mode: t
  ido-ubiquitous-mode: t
  keybindings-global-mode: t
  keybindings-minor-mode: t
  ido-everywhere: t
  flycheck-pos-tip-mode: t
  global-company-mode: t
  company-mode: t
  global-page-break-lines-mode: t
  which-key-mode: t
  delete-selection-mode: t
  show-paren-mode: t
  save-place-mode: t
  savehist-mode: t
  xterm-mouse-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-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
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Features:
(shadow sort ws-butler flyspell ispell ruler-mode gnus-util mail-extr
emacsbug message dired format-spec rfc822 mml mml-sec mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util help-fns mail-prsvr
mail-utils smex recentf tree-widget company-math math-symbol-lists
company-files company-oddmuse company-keywords company-etags etags xref
project company-gtags company-dabbrev-code company-dabbrev company-capf
company-cmake company-xcode company-clang company-semantic company-eclim
company-template company-css company-nxml company-bbdb ido-ubiquitous s
ucs-normalize ido-completing-read+ cus-edit cus-start cus-load wid-edit
autoloads ido haskell-prettify flycheck-pos-tip pos-tip flycheck
find-func rx subr-x jka-compr let-alist proof-site proof-autoloads
pg-vars company agda2 smart-mode-line-dark-theme smart-mode-line
rich-minority page-break-lines diminish which-key which-func imenu
elapsed time eml-mode derived demo-mode dash always-make-directory
easy-escape easy-mmode delsel paren saveplace savehist xt-mouse
finder-inf edmacro kmacro advice tex-site cl-seq cl eieio eieio-core
cl-macs info package epg-config compatibility seq byte-opt gv compile
comint ansi-color ring bytecomp byte-compile cl-extra help-mode easymenu
cl-loaddefs pcase cl-lib cconv tangomod-dark-theme time-date mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel x-win term/common-win x-dnd tool-bar dnd fontset image regexp-opt
fringe tabulated-list newcomment elisp-mode lisp-mode prog-mode register
page menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock
font-lock syntax facemenu font-core 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 charscript case-table epa-hook
jka-cmpr-hook help simple abbrev 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 dbusbind inotify dynamic-setting
system-font-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 433672 15194)
 (symbols 48 33265 1)
 (miscs 40 54 211)
 (strings 32 81160 11716)
 (string-bytes 1 2094594)
 (vectors 16 42572)
 (vector-slots 8 774284 7488)
 (floats 8 294 55)
 (intervals 56 2934 164)
 (buffers 976 14)
 (heap 1024 43373 1840))


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Eli Zaretskii]

> From: Clément Pit--Claudel <clement.pitclaudel@live.com>
> Date: Wed, 6 Jan 2016 13:04:17 -0500
> 
> Adding an 'invisible property to an overlay seems to break stacking of faces.
> To reproduce, run the following snippet. The whole buffer should have a red
> background, but the invisible section does not inherit that background.
> 
> (with-current-buffer (get-buffer-create "bug")
>   (erase-buffer)
>   (fundamental-mode)
>   (add-to-invisibility-spec '(outline . t))
>   (insert "line1 line2 line3")
>   (let ((ov (make-overlay 7 12)))
>     (overlay-put ov 'invisible 'outline))
>   (let ((ov (make-overlay 7 8)))
>     (overlay-put ov 'face '(:underline t)))
>   (let ((ov (make-overlay (point-min) (point-max))))
>     (overlay-put ov 'face '(:background "red")))
>   (pop-to-buffer (current-buffer)))
> 
> I initially thought this was an issue of priorities, but removing the overlay 
> with
> the invisible property makes the issue disappear. Similarly, removing the
> underline overlay makes the issue disappear.

It's a feature.  More accurately, it's the best solution found (more
than 10 years ago!) for a difficult problem.  In a nutshell, in a
general use case the invisible text can (and many times does) have
non-trivial face attributes, and using those faces for displaying the
ellipsis would result in each instance of the ellipsis to have some
random face.  This "feature" makes that face predictable: when the
invisible text has a face that is different from the last visible
character before it, the ellipsis is displayed with the 'default'
face.

You can read the discussion which led to the current implementation
here:

  http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-04/msg00338.html

Note that the display engine only looks at the face of the first
invisible character; the rest are skipped without looking at their
faces (or any other text properties or overlays).  So if you change
your test case in a very minor way:

  (let ((ov (make-overlay 8 9)))
    (overlay-put ov 'face '(:underline t)))

The "problem" doesn't happen.

> Here is another (more "real-world") way to reproduce this issue (run the
> snippet, then use C-x h in the newly created buffer to mark all text: the
> invisible section isn't highlighted)
> 
> (with-current-buffer (get-buffer-create "org-bug")
>   (erase-buffer)
>   (org-mode)
>   (insert "* line1\n** line2\n* line3")
>   (org-shifttab)
>   (pop-to-buffer (current-buffer)))

You say that this use case is only broken in Emacs 25, but I see this
in all the previous versions as well (as expected, given when this was
coded).  So if you really see this working correctly in Emacs 24.4,
then some other factors on your system were at work here.  Maybe you
didn't test this exact test case there?

> Yet another way it to reproduce it is given below
> 
> (with-current-buffer (get-buffer-create "flyspell-bug")
>   (erase-buffer)
>   (text-mode)
>   (add-to-invisibility-spec '(outline . t))
>   (insert "line1\nline2\nline3")
>   (flyspell-check-region-doublons (point-min) (point-max))
>   (let ((ov (make-overlay 7 12)))
>     (overlay-put ov 'invisible 'outline))
>   (pop-to-buffer (current-buffer)))

What exactly is the problem here?  I don't see anything that looks
problematic.  In particular, there are no duplicate misspelling in
this buffer, so flyspell-check-region-doublons should do nothing.
What am I missing?

> Interestingly, the first example is broken in both 24.4 and emacs-25; but the 
> second one is broken only in emacs-25 (the third one does not run in Emacs 
> 24.4).

This behavior should be present in any Emacs released after Apr 2005.

> I wonder if this is connected to another issue that I've seen, where having a 
> composition (using prettify-symbols-mode) just before an invisible region would 
> cause the background of the invisible region to be incorrect, but setting 
> prettify-symbols-unprettify-at-point and moving the point to the edge of the 
> symbol would cause the background to be fine. Unfortunately I don't have a good 
> repro for that issue.

Hard to tell without a reproducible test case.

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Clément Pit--Claudel]

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

Hi Eli,

Thanks a lot for your response.

On 01/07/2016 10:54 AM, Eli Zaretskii wrote:
> It's a feature.  More accurately, it's the best solution found (more
> than 10 years ago!) for a difficult problem.  In a nutshell, in a
> general use case the invisible text can (and many times does) have
> non-trivial face attributes, and using those faces for displaying the
> ellipsis would result in each instance of the ellipsis to have some
> random face.  This "feature" makes that face predictable: when the
> invisible text has a face that is different from the last visible
> character before it, the ellipsis is displayed with the 'default'
> face.
>
> You can read the discussion which led to the current implementation
> here:
> 
>   http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-04/msg00338.html

Interesting, thanks for the pointer. I read the discussion, but I see no rationale there for this choice. Was one ever given? Why is this better than, for example, keeping the face of the first invisible character and extending it to the whole ellipsis?

Am I mistaken to think that this choice is also inconsistent with the way faces on composed characters are handled? It seems that composing a sequence of characters into "..." will keep the face of the first compose character. That default is useful in many places (such as Arthur's nameless mode, or in flyspell or flycheck when an error covers a sequence of characters composed into a single one). Is there a good reason to treat ellipses standing for invisible spans differently?

The interactions of the current behaviour with transient mark mode are especially confusing: consider the two examples below:

(with-current-buffer (get-buffer-create "region covers "def", but ellipsis isn't highlighted")
  (erase-buffer)
  (fundamental-mode)
  (add-to-invisibility-spec '(outline . t))
  (insert "abcdefghi")
  (let ((ov (make-overlay 4 7)))
    (overlay-put ov 'invisible 'outline))
  (let ((ov (make-overlay 6 (point-max))))
    (overlay-put ov 'face 'region))
  (pop-to-buffer (current-buffer)))

(with-current-buffer (get-buffer-create "region now covers one more char, and ellispis is highlighed")
  (erase-buffer)
  (fundamental-mode)
  (add-to-invisibility-spec '(outline . t))
  (insert "abcdefghi")
  (let ((ov (make-overlay 4 7)))
    (overlay-put ov 'invisible 'outline))
  (let ((ov (make-overlay 5 (point-max))))
    (overlay-put ov 'face 'region))
  (pop-to-buffer (current-buffer)))

Using transient-mark-mode and selecting characters one by one from the end of the buffer, "i", "h", and "g" are progressively selected, then nothing happens despite "def" being in fact selected (so pressing C-w at that point kills more than highlighted), then "def" and "c" get highlighted at the same time. This means that the highlighted region doesn't actually correspond to the marked region. Is that also expected? (I realize that this can also happen if the point is in the middle of an invisible region, but that's a lot more of a corner case than using Shift+arrow keys to select a buffer section)

> Note that the display engine only looks at the face of the first
> invisible character; the rest are skipped without looking at their
> faces (or any other text properties or overlays).  So if you change
> your test case in a very minor way:
> 
>   (let ((ov (make-overlay 8 9)))
>     (overlay-put ov 'face '(:underline t)))
> 
> The "problem" doesn't happen.

That's true, but I have no control over this; the only thing that I see is that invisible sections of org Buffers do not have the right background (same for folded theorems in Coq buffers, due to the bug with font fallback and overlays).

>> Here is another (more "real-world") way to reproduce this issue (run the
>> snippet, then use C-x h in the newly created buffer to mark all text: the
>> invisible section isn't highlighted)
>>
>> (with-current-buffer (get-buffer-create "org-bug")
>>   (erase-buffer)
>>   (org-mode)
>>   (insert "* line1\n** line2\n* line3")
>>   (org-shifttab)
>>   (pop-to-buffer (current-buffer)))
> 
> You say that this use case is only broken in Emacs 25, but I see this
> in all the previous versions as well (as expected, given when this was
> coded).  So if you really see this working correctly in Emacs 24.4,
> then some other factors on your system were at work here.  Maybe you
> didn't test this exact test case there?

You're right; this example works fine in 24.3, but it is indeed broken in 24.5. I tested both with -Q:

   Works:  GNU Emacs 24.3.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2015-06-17 on clem-w50-mint
   Broken: GNU Emacs 24.5.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2015-06-20 on clem-w50-mint

Is this something that changed in org then?

>> Yet another way it to reproduce it is given below
>>
>> (with-current-buffer (get-buffer-create "flyspell-bug")
>>   (erase-buffer)
>>   (text-mode)
>>   (add-to-invisibility-spec '(outline . t))
>>   (insert "line1\nline2\nline3")
>>   (flyspell-check-region-doublons (point-min) (point-max))
>>   (let ((ov (make-overlay 7 12)))
>>     (overlay-put ov 'invisible 'outline))
>>   (pop-to-buffer (current-buffer)))
> 
> What exactly is the problem here?  I don't see anything that looks
> problematic.  In particular, there are no duplicate misspelling in
> this buffer, so flyspell-check-region-doublons should do nothing.
> What am I missing?

Indeed, I use aspell instead of ispell. It seems to ignore numbers, and thus flags the first two instances of "line" as duplicates.

>> Interestingly, the first example is broken in both 24.4 and emacs-25; but the 
>> second one is broken only in emacs-25 (the third one does not run in Emacs 
>> 24.4).
> 
> This behavior should be present in any Emacs released after Apr 2005.

I wonder what makes 24.3 behave differently in Org mode.

>> I wonder if this is connected to another issue that I've seen, where having a
>> composition (using prettify-symbols-mode) just before an invisible region would 
>> cause the background of the invisible region to be incorrect, but setting 
>> prettify-symbols-unprettify-at-point and moving the point to the edge of the 
>> symbol would cause the background to be fine. Unfortunately I don't have a good 
>> repro for that issue.
> 
> Hard to tell without a reproducible test case.

This is in fact due to font fallback. I opened a separate bug report, as it does not seem obviously related to the current issue.

Thanks,
Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Clément Pit--Claudel]

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

On 01/07/2016 12:03 PM, Clément Pit--Claudel wrote:
>> You can read the discussion which led to the current implementation
>> here:
>>
>>   http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-04/msg00338.html

Interestingly, even after disabling the fix for the issue that led to it, I can't reproduce the issue posted there. Instead, I get the behaviour that I described in my previous mail. Am I doing it wrong?

From 7648ca61042cd6c54bce94ce9f8938e176a8d083 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit--Claudel?= <clement.pitclaudel@live.com>
Date: Thu, 7 Jan 2016 12:30:30 -0500
Subject: [PATCH] Don't set overlay face to default

---
 src/xdisp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index b18bfd0..4ddae50 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -4586,8 +4586,9 @@ setup_for_ellipsis (struct it *it, int len)
   /* Remember the current face id in case glyphs specify faces.
      IT's face is restored in set_iterator_to_next.
      saved_face_id was set to preceding char's face in handle_stop.  */
-  if (it->saved_face_id < 0 || it->saved_face_id != it->face_id)
-    it->saved_face_id = it->face_id = DEFAULT_FACE_ID;
+  /* if (it->saved_face_id < 0 || it->saved_face_id != it->face_id) */
+  /*   it->saved_face_id = it->face_id = DEFAULT_FACE_ID; */
 
   /* If the ellipsis represents buffer text, it means we advanced in
      the buffer, so we should no longer ignore overlay strings.  */
-- 
2.7.0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Eli Zaretskii]

> Cc: 22320@debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel@live.com>
> Date: Thu, 7 Jan 2016 12:03:38 -0500
> 
> >   http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-04/msg00338.html
> 
> Interesting, thanks for the pointer. I read the discussion, but I see no rationale there for this choice. Was one ever given?

I do see a rationale provided here:

  http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-04/msg00339.html

The change causes the code to keep using the same face in cases where
that is not controversial, and fall back on 'default' otherwise.

> Why is this better than, for example, keeping the face of the first invisible character and extending it to the whole ellipsis?

That would go back to the original problem, whereby each ellipsis
could have a different face depending on the faces in the invisible
text.  I think it's confusing to have the faces of the invisible text
show through, don't you agree?  Ellipsis just indicates there's some
hidden text, why should it "inherit" the face of that hidden text?

> Am I mistaken to think that this choice is also inconsistent with the way faces on composed characters are handled? It seems that composing a sequence of characters into "..." will keep the face of the first compose character. That default is useful in many places (such as Arthur's nameless mode, or in flyspell or flycheck when an error covers a sequence of characters composed into a single one). Is there a good reason to treat ellipses standing for invisible spans differently?

I'm not sure I understand what you describe here.  Can you show me
some simple Lisp which demonstrates the situation?

> The interactions of the current behaviour with transient mark mode are especially confusing

I agree, but I learned long ago that one should use invisible text as
little as possible, and in particular expect that text properties and
overlays put on invisible text will have some specific effect.  The
display engine skips the invisible text entirely, looking only at its
first character (and sometimes the last); if you think about this, you
will immediately understand that having properties and overlays on
invisible text is playing with fire -- basically, you bump into
undefined behavior.

> Using transient-mark-mode and selecting characters one by one from the end of the buffer, "i", "h", and "g" are progressively selected, then nothing happens despite "def" being in fact selected (so pressing C-w at that point kills more than highlighted), then "def" and "c" get highlighted at the same time. This means that the highlighted region doesn't actually correspond to the marked region. Is that also expected?

The ellipsis gets highlighted as soon as the last visible character
before the invisible text has the same face as the invisible text.
That is what you see.  So yes, this is expected.

Like I said: it's a hard problem, and the solution only caters to the
simple, no-brainer, use cases.  I'm not surprised that you can come up
with weird situations, but I cannot see a better, more general
solution here.

> >> (with-current-buffer (get-buffer-create "org-bug")
> >>   (erase-buffer)
> >>   (org-mode)
> >>   (insert "* line1\n** line2\n* line3")
> >>   (org-shifttab)
> >>   (pop-to-buffer (current-buffer)))
> > 
> > You say that this use case is only broken in Emacs 25, but I see this
> > in all the previous versions as well (as expected, given when this was
> > coded).  So if you really see this working correctly in Emacs 24.4,
> > then some other factors on your system were at work here.  Maybe you
> > didn't test this exact test case there?
> 
> You're right; this example works fine in 24.3, but it is indeed broken in 24.5. I tested both with -Q:

No, it behaves the same in 24.3 and in 24.5 (and in all versions
before that).  I don't understand how you see something different.

>    Works:  GNU Emacs 24.3.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2015-06-17 on clem-w50-mint
>    Broken: GNU Emacs 24.5.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2015-06-20 on clem-w50-mint
> 
> Is this something that changed in org then?

No, nothing changed.  Which version of Org did you use in each Emacs
release?  (I used the one bundled with that release.)

> >> (with-current-buffer (get-buffer-create "flyspell-bug")
> >>   (erase-buffer)
> >>   (text-mode)
> >>   (add-to-invisibility-spec '(outline . t))
> >>   (insert "line1\nline2\nline3")
> >>   (flyspell-check-region-doublons (point-min) (point-max))
> >>   (let ((ov (make-overlay 7 12)))
> >>     (overlay-put ov 'invisible 'outline))
> >>   (pop-to-buffer (current-buffer)))
> > 
> > What exactly is the problem here?  I don't see anything that looks
> > problematic.  In particular, there are no duplicate misspelling in
> > this buffer, so flyspell-check-region-doublons should do nothing.
> > What am I missing?
> 
> Indeed, I use aspell instead of ispell. It seems to ignore numbers, and thus flags the first two instances of "line" as duplicates.

And what problems do you see then?

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Eli Zaretskii]

> Cc: 22320@debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel@live.com>
> Date: Thu, 7 Jan 2016 12:36:35 -0500
> 
> Interestingly, even after disabling the fix for the issue that led to it, I can't reproduce the issue posted there. Instead, I get the behaviour that I described in my previous mail.

I'm not surprised: a lot has changed in the display engine since
2005.  The original issue was just the tip of an iceberg: the general
problem was that the ellipsis would have an unpredictable face which
depended on the face of the first invisible character.  I think this
is more confusing than what we have now, because that situation is by
far much more common.

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Clément Pit--Claudel]

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

On 01/07/2016 01:52 PM, Eli Zaretskii wrote:
>> Cc: 22320@debbugs.gnu.org
>> From: Clément Pit--Claudel <clement.pitclaudel@live.com>
>> Date: Thu, 7 Jan 2016 12:36:35 -0500
>>
>> Interestingly, even after disabling the fix for the issue that led to it, I can't reproduce the issue posted there. Instead, I get the behaviour that I described in my previous mail.
> 
> I'm not surprised: a lot has changed in the display engine since
> 2005.  The original issue was just the tip of an iceberg: the general
> problem was that the ellipsis would have an unpredictable face which
> depended on the face of the first invisible character.  I think this
> is more confusing than what we have now, because that situation is by
> far much more common.

I find the current situation more confusing, as it introduces many inconsistencies. Inheriting the face of the first hidden character, and applying it to each dot in the ellipsis, seems a lot more consistent to me (and it does feel predictable). Of course there will be gotchas, but no more than with using the default face; on the other hand, the current heuristic is rather unpredictable (at least the region selection example and the face fallback example in my other message both felt confusing to me).

The same problems exist for composition, but keeping the properties of the first character seems to work well there; maybe we could consider harmonizing both behaviors?

(with-current-buffer (get-buffer-create "emulating invisibility with composition works")
(erase-buffer)
(fundamental-mode)
(add-to-invisibility-spec '(outline . t))
(insert "line1 line2 line3")
;; This composition snippet was taken from Arthur's nameless-mode:
(compose-region 7 12 (cdr (apply #'append (mapcar (lambda (x) (list '(Br . Bl) x)) "..."))))
(let ((ov (make-overlay 7 8)))
(overlay-put ov 'face '(:underline t)))
(let ((ov (make-overlay (point-min) (point-max))))
(overlay-put ov 'face '(:background "red")))
(pop-to-buffer (current-buffer)))

compared to

(with-current-buffer (get-buffer-create "using invisibility directly doesn't work")
(erase-buffer)
(fundamental-mode)
(add-to-invisibility-spec '(outline . t))
(insert "line1 line2 line3")
(let ((ov (make-overlay 7 12)))
(overlay-put ov 'invisible 'outline))
(let ((ov (make-overlay 7 8)))
(overlay-put ov 'face '(:underline t)))
(let ((ov (make-overlay (point-min) (point-max))))
(overlay-put ov 'face '(:background "red")))
(pop-to-buffer (current-buffer)))

Thanks for your help with this issue!

Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Clément Pit--Claudel]

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

On 01/07/2016 01:46 PM, Eli Zaretskii wrote:
>> Cc: 22320@debbugs.gnu.org
>> From: Clément Pit--Claudel <clement.pitclaudel@live.com>
>> Date: Thu, 7 Jan 2016 12:03:38 -0500
>>
>>>   http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-04/msg00338.html
>>
>> Interesting, thanks for the pointer. I read the discussion, but I see no rationale there for this choice. Was one ever given?
> 
> I do see a rationale provided here:
> 
>   http://lists.gnu.org/archive/html/emacs-pretest-bug/2005-04/msg00339.html
> 
> The change causes the code to keep using the same face in cases where
> that is not controversial, and fall back on 'default' otherwise.

I understand what the change does, but I don't see that message as justifying it; instead, it asserts that the font-lock face is arbitrary; on the other hand, what I'm seeing with that code currently is a large section of the buffer highlighted in blue, and every time a folded section follows a character that's not in my current font (so about half of all folded sections) the ellipsis has the wrong background, making it stand out.

>> Why is this better than, for example, keeping the face of the first invisible character and extending it to the whole ellipsis?
> 
> That would go back to the original problem, whereby each ellipsis
> could have a different face depending on the faces in the invisible
> text.

No, my proposal would be to make all three dots use the face of the first hidden character.

> I think it's confusing to have the faces of the invisible text
> show through, don't you agree?

Not really; in a sense they already show through, since changing the faces of the hidden text can change the way it's displayed. Applying the face of the first hidden character to all three dots of the ellipsis has multiple advantages:

* If an incorrectly spelled word is invisible, the dots are underlined.
* If the first hidden character in fact does have the same face (from the user perspective) as the preceding one, the ellipsis is never reset to the default face (the font fallback issue means that at the moment I can have perfectly uniform-looking text, and folding still gives the default face to the ellipsis).

Invisible text is often used for folding sections of a buffer. Folding hides information, but we have an opportunity to show some of that information; namely, the information carried by the first invisible character. The current solution does take the information contained in the first character into account, but then makes it stand out or not based on a complex criterion.

In fact, I wonder if this isn't two problems we're looking at: the first one is falling back to default, and the second one is that falling back to default ignores the surrounding overlays. My ideal solution would be to use the first hidden character (or make it configurable), but even without this I feel that it would be a progress for the *face* of the ellipses to be reset to default, and then for that face to be merged with overlays. In other word one would consider that ellipses always have the default face, plus overlays that cover the full ellipsis.

> Ellipsis just indicates there's some hidden text, why should it "inherit" the
> face of that hidden text?

I think of ellipses as a compressed representation of that text; barring a better way to summarize that text, inheriting the faces of its first character seems like a decent approach. If I write red-green-red and color each word with the corresponding color, then make "green" invisible, my spontaneous expectation is that three green dots will appear (mostly because I don't think of invisible as invisible, but rather "compressed"/"folded")

>> Am I mistaken to think that this choice is also inconsistent with the way faces on composed characters are handled? It seems that composing a sequence of characters into "..." will keep the face of the first compose character. That default is useful in many places (such as Arthur's nameless mode, or in flyspell or flycheck when an error covers a sequence of characters composed into a single one). Is there a good reason to treat ellipses standing for invisible spans differently?
> 
> I'm not sure I understand what you describe here.  Can you show me
> some simple Lisp which demonstrates the situation?

Sure (also posted in reply to your other message):

(with-current-buffer (get-buffer-create "emulating invisibility with composition works")
  (erase-buffer)
  (fundamental-mode)
  (add-to-invisibility-spec '(outline . t))
  (insert "line1 line2 line3")
  ;; This composition snippet was taken from Arthur's nameless-mode:
  (compose-region 7 12 (cdr (apply #'append (mapcar (lambda (x) (list '(Br . Bl) x)) "..."))))
  (let ((ov (make-overlay 7 8)))
    (overlay-put ov 'face '(:underline t)))
  (let ((ov (make-overlay (point-min) (point-max))))
    (overlay-put ov 'face '(:background "red")))
  (pop-to-buffer (current-buffer)))

compared to

(with-current-buffer (get-buffer-create "using invisibility directly doesn't work")
  (erase-buffer)
  (fundamental-mode)
  (add-to-invisibility-spec '(outline . t))
  (insert "line1 line2 line3")
  (let ((ov (make-overlay 7 12)))
    (overlay-put ov 'invisible 'outline))
  (let ((ov (make-overlay 7 8)))
    (overlay-put ov 'face '(:underline t)))
  (let ((ov (make-overlay (point-min) (point-max))))
    (overlay-put ov 'face '(:background "red")))
  (pop-to-buffer (current-buffer)))

>> The interactions of the current behaviour with transient mark mode are especially confusing
> 
> I agree, but I learned long ago that one should use invisible text as
> little as possible, and in particular expect that text properties and
> overlays put on invisible text will have some specific effect.  The
> display engine skips the invisible text entirely, looking only at its
> first character (and sometimes the last); if you think about this, you
> will immediately understand that having properties and overlays on
> invisible text is playing with fire -- basically, you bump into
> undefined behavior.
>
>> Using transient-mark-mode and selecting characters one by one from the end of the buffer, "i", "h", and "g" are progressively selected, then nothing happens despite "def" being in fact selected (so pressing C-w at that point kills more than highlighted), then "def" and "c" get highlighted at the same time. This means that the highlighted region doesn't actually correspond to the marked region. Is that also expected?
> 
> The ellipsis gets highlighted as soon as the last visible character
> before the invisible text has the same face as the invisible text.
> That is what you see.  So yes, this is expected.
> 
> Like I said: it's a hard problem, and the solution only caters to the
> simple, no-brainer, use cases.  I'm not surprised that you can come up
> with weird situations, but I cannot see a better, more general
> solution here.

Inheriting the face of the first character and applying it to the whole ellipsis would work here.

>>>> (with-current-buffer (get-buffer-create "org-bug")
>>>>   (erase-buffer)
>>>>   (org-mode)
>>>>   (insert "* line1\n** line2\n* line3")
>>>>   (org-shifttab)
>>>>   (pop-to-buffer (current-buffer)))
>>>
>>> You say that this use case is only broken in Emacs 25, but I see this
>>> in all the previous versions as well (as expected, given when this was
>>> coded).  So if you really see this working correctly in Emacs 24.4,
>>> then some other factors on your system were at work here.  Maybe you
>>> didn't test this exact test case there?
>>
>> You're right; this example works fine in 24.3, but it is indeed broken in 24.5. I tested both with -Q:
> 
> No, it behaves the same in 24.3 and in 24.5 (and in all versions
> before that).  I don't understand how you see something different.
> 
>>    Works:  GNU Emacs 24.3.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2015-06-17 on clem-w50-mint
>>    Broken: GNU Emacs 24.5.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2015-06-20 on clem-w50-mint
>>
>> Is this something that changed in org then?
> 
> No, nothing changed.  Which version of Org did you use in each Emacs
> release?  (I used the one bundled with that release.)

I'm using the bundled Org with emacs -Q in both cases.

>>>> (with-current-buffer (get-buffer-create "flyspell-bug")
>>>>   (erase-buffer)
>>>>   (text-mode)
>>>>   (add-to-invisibility-spec '(outline . t))
>>>>   (insert "line1\nline2\nline3")
>>>>   (flyspell-check-region-doublons (point-min) (point-max))
>>>>   (let ((ov (make-overlay 7 12)))
>>>>     (overlay-put ov 'invisible 'outline))
>>>>   (pop-to-buffer (current-buffer)))
>>>
>>> What exactly is the problem here?  I don't see anything that looks
>>> problematic.  In particular, there are no duplicate misspelling in
>>> this buffer, so flyspell-check-region-doublons should do nothing.
>>> What am I missing?
>>
>> Indeed, I use aspell instead of ispell. It seems to ignore numbers, and thus flags the first two instances of "line" as duplicates.
> 
> And what problems do you see then?

aspell marks line2 as a doublon, places an underline on it, and thus the invisible text doesn't inherit the selection face when I mark the whole buffer.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Eli Zaretskii]

> Cc: 22320@debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel@live.com>
> Date: Thu, 7 Jan 2016 14:07:14 -0500
> 
> I find the current situation more confusing, as it introduces many inconsistencies. Inheriting the face of the first hidden character, and applying it to each dot in the ellipsis, seems a lot more consistent to me (and it does feel predictable).

I don't see how it would be less confusing: the invisible characters
are invisible, so figuring out why some ellipses are in black, others
in blue, still others have some non-default background colors, and
some use a different font, sounds like mission impossible to me: you
don't see the text that gives the ellipsis its looks.

> The same problems exist for composition, but keeping the properties of the first character seems to work well there; maybe we could consider harmonizing both behaviors?

I'm not sure I understand what exactly are you proposing to do.  We
cannot treat invisible text like we treat character compositions, each
one invokes a very different machinery with distinct and very
different features.

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Eli Zaretskii]

> Cc: 22320@debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel@live.com>
> Date: Thu, 7 Jan 2016 14:28:09 -0500
> 
> >> Why is this better than, for example, keeping the face of the first invisible character and extending it to the whole ellipsis?
> > 
> > That would go back to the original problem, whereby each ellipsis
> > could have a different face depending on the faces in the invisible
> > text.
> 
> No, my proposal would be to make all three dots use the face of the first hidden character.

Yes, I understand.  But then each invisible text will pass its
potentially different face to its ellipsis, so each ellipsis will
generally look different.

> > I think it's confusing to have the faces of the invisible text
> > show through, don't you agree?
> 
> Not really; in a sense they already show through, since changing the faces of the hidden text can change the way it's displayed. Applying the face of the first hidden character to all three dots of the ellipsis has multiple advantages:
> 
> * If an incorrectly spelled word is invisible, the dots are underlined.

If seeing an ellipsis highlighted as a misspelled word is not
confusing to you, then I guess we have very different ideas of what's
confusing...

> * If the first hidden character in fact does have the same face (from the user perspective) as the preceding one, the ellipsis is never reset to the default face (the font fallback issue means that at the moment I can have perfectly uniform-looking text, and folding still gives the default face to the ellipsis).

If the invisible text uses a different font, you will have the
ellipsis displayed with that different font, out of the blue.  It
could, for example, be a much larger or a much smaller font.  Maybe
it's not confusing to you, but it definitely is to me.

> Invisible text is often used for folding sections of a buffer. Folding hides information, but we have an opportunity to show some of that information; namely, the information carried by the first invisible character. The current solution does take the information contained in the first character into account, but then makes it stand out or not based on a complex criterion.

My POV is that when you make text invisible, you don't want to see it.
None of it, not even its colors or its font.  Having some of its
attributes show through is just plain wrong, IMO.

> In fact, I wonder if this isn't two problems we're looking at: the first one is falling back to default, and the second one is that falling back to default ignores the surrounding overlays. My ideal solution would be to use the first hidden character (or make it configurable), but even without this I feel that it would be a progress for the *face* of the ellipses to be reset to default, and then for that face to be merged with overlays. In other word one would consider that ellipses always have the default face, plus overlays that cover the full ellipsis.

Any non-default face is always merged with the default, so saying
let's merge it with the default is asking for a no-op.  It will change
nothing.

What the code does is it deliberately ignores the faces specified by
the overlays (or text properties) put on the invisible text, if that
face is different from the preceding visible text.  We can either
ignore that face or not ignore it; there's no 3rd way.  The way you
suggest is go back to what we've been doing before that 2005 change.
I don't like going back in principle (where does that end? can someone
tomorrow go back from our going back, just because they think they are
right and going back was wrong?).  And I don't think it's justified to
go back in this case.  If someone comes up with a clever idea to fix
more use cases that have faces on invisible text, that would be
progress.  But returning to what we did back then doesn't sound right
to me, so I don't think we should do it.

> >> Am I mistaken to think that this choice is also inconsistent with the way faces on composed characters are handled? It seems that composing a sequence of characters into "..." will keep the face of the first compose character. That default is useful in many places (such as Arthur's nameless mode, or in flyspell or flycheck when an error covers a sequence of characters composed into a single one). Is there a good reason to treat ellipses standing for invisible spans differently?
> > 
> > I'm not sure I understand what you describe here.  Can you show me
> > some simple Lisp which demonstrates the situation?
> 
> Sure (also posted in reply to your other message):

Composed characters are processed, while invisible text is simply
skipped.  That's a world of difference.  Each of these 2 features was
introduced for a very different purpose, so it's small wonder they
produce different effects on display.

> Inheriting the face of the first character and applying it to the whole ellipsis would work here.

But will fail elsewhere.

> >>    Works:  GNU Emacs 24.3.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2015-06-17 on clem-w50-mint
> >>    Broken: GNU Emacs 24.5.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8) of 2015-06-20 on clem-w50-mint
> >>
> >> Is this something that changed in org then?
> > 
> > No, nothing changed.  Which version of Org did you use in each Emacs
> > release?  (I used the one bundled with that release.)
> 
> I'm using the bundled Org with emacs -Q in both cases.

Then I don't know what to think.  I see the "gap" in the region face
in all old versions, exactly like I see in 24.5 and in the current
emacs-25 branch.

> >> Indeed, I use aspell instead of ispell. It seems to ignore numbers, and thus flags the first two instances of "line" as duplicates.
> > 
> > And what problems do you see then?
> 
> aspell marks line2 as a doublon, places an underline on it, and thus the invisible text doesn't inherit the selection face when I mark the whole buffer.

Ah, you never said the 3rd use case needs "C-x h" as well.

Yes, I see that here, too.  It's the same situation as in the other
cases.

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Eli Zaretskii]

> Date: Thu, 07 Jan 2016 22:12:42 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 22320@debbugs.gnu.org
> 
> > The same problems exist for composition, but keeping the properties of the first character seems to work well there; maybe we could consider harmonizing both behaviors?
> 
> I'm not sure I understand what exactly are you proposing to do.  We
> cannot treat invisible text like we treat character compositions, each
> one invokes a very different machinery with distinct and very
> different features.

Here's a suggestion: ignore the face of the invisible text altogether,
and instead always use the face of the last visible character.  The
patch to do that is below; it fixes all of your test cases.  But since
you would like to see the face of the invisible text show through, I'm
not sure you will like it...

WDYT?

diff --git a/src/xdisp.c b/src/xdisp.c
index 87a92fc..7e5f7df 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -4583,14 +4583,15 @@ setup_for_ellipsis (struct it *it, int len)
   it->current.dpvec_index = 0;
   it->dpvec_face_id = -1;
 
-  /* Reset the current face ID to default if the last visible
-     character and the first invisible character have different faces.
-     IT->saved_face_id was set in handle_stop to the face of the
-     preceding character, and will be different from IT->face_id only
-     if the invisible text skipped in handle_invisible_prop has some
-     non-default face.  IT's face is restored in set_iterator_to_next.  */
-  if (it->saved_face_id < 0 || it->saved_face_id != it->face_id)
-    it->saved_face_id = it->face_id = DEFAULT_FACE_ID;
+  /* Use IT->saved_face_id for the ellipsis, so that it has the same
+     face as the preceding text.  IT->saved_face_id was set in
+     handle_stop to the face of the preceding character, and will be
+     different from IT->face_id only if the invisible text skipped in
+     handle_invisible_prop has some non-default face.  We thus ignore
+     the face of the invisible text when we display the ellipsis.
+     IT's face is restored in set_iterator_to_next.  */
+  if (it->saved_face_id >= 0)
+    it->face_id = it->saved_face_id;
 
   /* If the ellipsis represents buffer text, it means we advanced in
      the buffer, so we should no longer ignore overlay strings.  */

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Clément Pit--Claudel]

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

On 01/07/2016 03:35 PM, Eli Zaretskii wrote:
>>> In fact, I wonder if this isn't two problems we're looking at:
>>> the first one is falling back to default, and the second one is
>>> that falling back to default ignores the surrounding overlays. My
>>> ideal solution would be to use the first hidden character (or
>>> make it configurable), but even without this I feel that it would
>>> be a progress for the *face* of the ellipses to be reset to
>>> default, and then for that face to be merged with overlays. In
>>> other word one would consider that ellipses always have the
>>> default face, plus overlays that cover the full ellipsis.
>
> Any non-default face is always merged with the default, so saying 
> let's merge it with the default is asking for a no-op.  It will
> change nothing.

Yes, sorry for being unclear. Hopefully the following is more clear. For any overlay/invisible section pair there are five cases, right?

1. The overlay does not intersect with the invisible area. No problems here.
2. The overlay covers a few characters before the invisible area and the first few characters of the invisible area, but not the last ones.
3. The overlay covers a few characters after the invisible area and the last few characters of the invisible area, but not the first ones.
4. The overlay is a subset of the invisible area.
5. The overlay is a superset of the invisible area.

The current solution doesn't think in these terms; instead, it ignores all overlays for the ellipses, unless the first hidden character has the same face as the preceding one.

Always applying the face of the first character to the ellipsis means applying to the ellipsis the faces of overlays in categories 2, 5, and 4 if they cover the first character. You pointed out that the special case of 4 is confusing.

Another option (the one I was trying to explain above) is to apply to the ellipsis only the faces of overlays in category 5. I feel that this should be rather uncontroversial (don't you think?).

The problem with this way of thinking about overlays and invisible text is that it doesn't necessarily map directly to the implementation. IIUC, the current implementation computes faces without taking invisibility into account for the first character before and after the left boundary of the invisible section, and then compares them. At this point it's too late to merge certain faces but not others.

Thanks again for your explanations and your time.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

bug#22320: Overlays with an 'invisible property break stacking of overlay faces

[Eli Zaretskii]

> Cc: 22320@debbugs.gnu.org
> From: Clément Pit--Claudel <clement.pitclaudel@live.com>
> Date: Thu, 7 Jan 2016 19:27:30 -0500
> 
> 1. The overlay does not intersect with the invisible area. No problems here.
> 2. The overlay covers a few characters before the invisible area and the first few characters of the invisible area, but not the last ones.

In this case, Emacs doesn't care where the overlay ends, that buffer
position is skipped without checking overlays.

> 3. The overlay covers a few characters after the invisible area and the last few characters of the invisible area, but not the first ones.

Likewise.

> 4. The overlay is a subset of the invisible area.

If it starts on the first invisible character, Emacs will take note;
otherwise it won't.

> 5. The overlay is a superset of the invisible area.

This is the case with all your use cases.

> The current solution doesn't think in these terms; instead, it ignores all overlays for the ellipses, unless the first hidden character has the same face as the preceding one.
> 
> Always applying the face of the first character to the ellipsis means applying to the ellipsis the faces of overlays in categories 2, 5, and 4 if they cover the first character. You pointed out that the special case of 4 is confusing.
> 
> Another option (the one I was trying to explain above) is to apply to the ellipsis only the faces of overlays in category 5. I feel that this should be rather uncontroversial (don't you think?).

Well, now that I've committed what I believe is a better solution, I
think we can put these issues to rest.

> IIUC, the current implementation computes faces without taking invisibility into account for the first character before and after the left boundary of the invisible section, and then compares them. At this point it's too late to merge certain faces but not others.

Yes.  More accurately, the display engine always considers faces
before it considers invisibility, so it always sees the face of the
first invisible character.