bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Ihor Radchenko]

Hi,

We recently got the following bug report in Org
https://list.orgmode.org/871qqdxrjz.fsf@gmail.com/T/#u

0. Run `emacs -Q`

1. Create a buffer with this content and ensure `org-mode` is on
------------------------------------------------------------
* One
#+begin_src emacs-lisp
#+end_src
* Two
#+begin_src emacs-lisp
#+end_src
------------------------------------------------------------

2. Change the face `org-block-end-line` to have an extended background on the end-line of the
org-block

(set-face-attribute
   'org-block-end-line nil
   :extend t
   :background "#4e5079")

4. Go to "One" and press tab

5. Notice how the background colour now appears next to "One".

I do not see anything wrong on the Org side.
Maybe Emacs should not apply :extent t attribute to the newline when the
text in fontified line is hidden?


In GNU Emacs 28.1.90 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.34, cairo version 1.16.0)
 of 2022-07-17 built on localhost
Repository revision: f5218385c064aa959650bfe49ca32795270851eb
Repository branch: emacs-28
Windowing system distributor 'The X.Org Foundation', version 11.0.12101004
System Description: Gentoo Linux


-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Kévin Le Gouguec]

Hey Ihor,

Ihor Radchenko <yantar92@posteo.net> writes:

> I do not see anything wrong on the Org side.
> Maybe Emacs should not apply :extent t attribute to the newline when the
> text in fontified line is hidden?

IIUC, this is bug#52587?

The conclusion there, AFAIR, was "it's unfortunate, but that's the
design for now; let's rediscuss when we see some patches for outline.el
(or the specific modes that use extended backgrounds)".

I don't think the solution you suggest here was proposed in that report.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> Cc: 59141@debbugs.gnu.org
> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Date: Wed, 09 Nov 2022 08:49:07 +0100
> 
> Ihor Radchenko <yantar92@posteo.net> writes:
> 
> > I do not see anything wrong on the Org side.
> > Maybe Emacs should not apply :extent t attribute to the newline when the
> > text in fontified line is hidden?
> 
> IIUC, this is bug#52587?

Yes.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

>> I do not see anything wrong on the Org side.
>> Maybe Emacs should not apply :extent t attribute to the newline when the
>> text in fontified line is hidden?
>
> IIUC, this is bug#52587?

And bug#53981.

> The conclusion there, AFAIR, was "it's unfortunate, but that's the
> design for now; let's rediscuss when we see some patches for outline.el
> (or the specific modes that use extended backgrounds)".

Here is an experimental patch for outline.el
that demonstrates a possible way to fix this:

diff --git a/lisp/outline.el b/lisp/outline.el
index a646f71db8..73ae707821 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -930,7 +930,8 @@ outline-flag-region
     ;; We use `front-advance' here because the invisible text begins at the
     ;; very end of the heading, before the newline, so text inserted at FROM
     ;; belongs to the heading rather than to the entry.
-    (let ((o (make-overlay from to nil 'front-advance)))
+    (let ((o (make-overlay from (1+ to))))
+      (overlay-put o 'display "\n")
       (overlay-put o 'evaporate t)
       (overlay-put o 'invisible 'outline)
       (overlay-put o 'isearch-open-invisible

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Ihor Radchenko]

Juri Linkov <juri@linkov.net> writes:

> Here is an experimental patch for outline.el
> that demonstrates a possible way to fix this:
>
> diff --git a/lisp/outline.el b/lisp/outline.el
> index a646f71db8..73ae707821 100644
> --- a/lisp/outline.el
> +++ b/lisp/outline.el
> @@ -930,7 +930,8 @@ outline-flag-region
>      ;; We use `front-advance' here because the invisible text begins at the
>      ;; very end of the heading, before the newline, so text inserted at FROM
>      ;; belongs to the heading rather than to the entry.
> -    (let ((o (make-overlay from to nil 'front-advance)))
> +    (let ((o (make-overlay from (1+ to))))
> +      (overlay-put o 'display "\n")

Note that 'font-advance is there for a reason.

Also, what will happen with ellipsis when there is 'display property?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

>> Here is an experimental patch for outline.el
>> that demonstrates a possible way to fix this:
>>
>> diff --git a/lisp/outline.el b/lisp/outline.el
>> index a646f71db8..73ae707821 100644
>> --- a/lisp/outline.el
>> +++ b/lisp/outline.el
>> @@ -930,7 +930,8 @@ outline-flag-region
>>      ;; We use `front-advance' here because the invisible text begins at the
>>      ;; very end of the heading, before the newline, so text inserted at FROM
>>      ;; belongs to the heading rather than to the entry.
>> -    (let ((o (make-overlay from to nil 'front-advance)))
>> +    (let ((o (make-overlay from (1+ to))))
>> +      (overlay-put o 'display "\n")
>
> Note that 'font-advance is there for a reason.
>
> Also, what will happen with ellipsis when there is 'display property?

Then ellipsis could be added to (overlay-put o 'display "...\n").

Alternatively, maybe it would be sufficient to put the 'display "\n" property
only on the final newline, thus hiding its face properties.
The drawback is that it requires more overlays.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Ihor Radchenko]

Juri Linkov <juri@linkov.net> writes:

>> Also, what will happen with ellipsis when there is 'display property?
>
> Then ellipsis could be added to (overlay-put o 'display "...\n").

Then, changes to `buffer-invisibility-spec' will not affect the overlay.

> Alternatively, maybe it would be sufficient to put the 'display "\n" property
> only on the final newline, thus hiding its face properties.
> The drawback is that it requires more overlays.

But don't display property inherit face? I tried to play with
`put-text-property' 'display "\n" and even with (propertize "\n" 'face
'default) but :extend still appears to affect the display.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> Cc: 59141@debbugs.gnu.org,
>  Kévin Le Gouguec <kevin.legouguec@gmail.com>
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Fri, 11 Nov 2022 01:58:10 +0000
> 
> > Alternatively, maybe it would be sufficient to put the 'display "\n" property
> > only on the final newline, thus hiding its face properties.
> > The drawback is that it requires more overlays.
> 
> But don't display property inherit face? I tried to play with
> `put-text-property' 'display "\n" and even with (propertize "\n" 'face
> 'default) but :extend still appears to affect the display.

Did you read the mode "Displaying Faces" in the ELisp manual?  It
explains why this happens.

Emacs 29 has a new face attribute value 'reset'; did you try setting
the :extend attribute to the value 'reset?

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> But don't display property inherit face? I tried to play with
>> `put-text-property' 'display "\n" and even with (propertize "\n" 'face
>> 'default) but :extend still appears to affect the display.
>
> Did you read the mode "Displaying Faces" in the ELisp manual?  It
> explains why this happens.

I did. But I apparently screwed something up during testing. Trying
again now yielded expected results.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

>>> Also, what will happen with ellipsis when there is 'display property?
>>
>> Then ellipsis could be added to (overlay-put o 'display "...\n").
>
> Then, changes to `buffer-invisibility-spec' will not affect the overlay.

Frankly speaking, I don't understand why the problem can't be solved
simply by adding a newline without any faces or text properties
before the next outline heading.

So for example, the problem with shortdoc discussed in bug#53981,
can be solved just by adding a newline:

diff --git a/lisp/emacs-lisp/shortdoc.el b/lisp/emacs-lisp/shortdoc.el
index 18b758a9ca3..6000f1f5375 100644
--- a/lisp/emacs-lisp/shortdoc.el
+++ b/lisp/emacs-lisp/shortdoc.el
@@ -1385,7 +1385,7 @@ shortdoc-display-group
         ;; There may be functions not yet defined in the data.
         ((fboundp (car data))
          (when prev
-           (insert (make-separator-line)))
+           (insert (make-separator-line) "\n"))
          (setq prev t)
          (shortdoc--display-function data))))
      (cdr (assq group shortdoc--groups))))

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Ihor Radchenko]

Juri Linkov <juri@linkov.net> writes:

> Frankly speaking, I don't understand why the problem can't be solved
> simply by adding a newline without any faces or text properties
> before the next outline heading.

Because we cannot demand users to use extra newline before headings.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Al Haji-Ali]

I've tried Juri's solution of adding an overlay 'display property with "...\n" and at least the final result is what I would expect (the entry is fully hidden including the extended background). One difference in behaviour is that point cannot be after "..." on the same line (only before) but I am not sure that's a serious drawback. Adding the 'display property could be done conditionally if looking-at "\n" (or if the line's face has an :extend property).

Note that when the 'display is "\n" only, the ellipses are not shown. Nevertheless, both behaviours go against the documentation of `invisible-p` where it says that if t, "the text would be replaced by an ellipsis", so I am not sure if showing the 'display text is unintended.

One solution could be a modification to overlay rendering where an invisibility-spec that has (atom . t) could be replaced by ellipses but (atom . "text") would be replaced by "text" and then "text" could be "...\n" in the situation under consideration. I seem to recall that org-fold had this behaviour, but I am not sure.

-- Al

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> Cc: 59141@debbugs.gnu.org, kevin.legouguec@gmail.com, juri@linkov.net
> From: Al Haji-Ali <abdo.haji.ali@gmail.com>
> Date: Fri, 11 Nov 2022 12:30:13 +0000
> 
> Note that when the 'display is "\n" only, the ellipses are not shown. Nevertheless, both behaviours go against the documentation of `invisible-p` where it says that if t, "the text would be replaced by an ellipsis", so I am not sure if showing the 'display text is unintended.

If 'display' and 'invisible' properties start at the same buffer
position, the 'display' property "wins", because the display engine
handles it before it handles the invisible property.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Al Haji-Ali]

On 11/11/2022, Eli Zaretskii wrote:
> If 'display' and 'invisible' properties start at the same buffer
> position, the 'display' property "wins", because the display engine
> handles it before it handles the invisible property.
I see, thanks for explaining.

What do you think about the suggestion solution of changing the overlay rendering to include arbitrary text?

More to the point, what do people think about the making the extent of an outline of an entry include a new line? Is that more or less logical than the current behaviour?

For example,

,----
| * A
| TextA
| * B
| TextB
`----

Is the content of the "A" entry "\nTextA\n", or is it "\nTextA"?
I would argue it's the former if we consider the whole entry to "A\nTextA\n" which is separated into header "A" and content "\nTextA\n".

Best regards,
-- Al

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Al Haji-Ali <abdo.haji.ali@gmail.com>
> Cc: yantar92@posteo.net, 59141@debbugs.gnu.org, kevin.legouguec@gmail.com,
>  juri@linkov.net
> Date: Fri, 11 Nov 2022 16:00:51 +0000
> 
> What do you think about the suggestion solution of changing the overlay rendering to include arbitrary text?

I don't think I understand the question.  What do you mean by "overlay
rendering"?  An overlay can produce text only if it has a
before-string or after-string property, so what do you mean by
"arbitrary text"?

> More to the point, what do people think about the making the extent of an outline of an entry include a new line? Is that more or less logical than the current behaviour?

How would that help?  The display engine is blissfully unaware of Org
headings and entries, it just sees text in a buffer.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Abdul-Lateef Haji-Ali]

On 11/11/2022, Eli Zaretskii wrote:
> I don't think I understand the question.  What do you mean by "overlay
> rendering"?  An overlay can produce text only if it has a
> before-string or after-string property, so what do you mean by
> "arbitrary text"?

I might be using the wrong terms here, but I think the idea is straightforward.
According to `buffer-invisibility-spec` docs

,----
| If the (ATOM . ELLIPSIS) form is used, and ELLIPSIS is non-nil, an
| ellipsis will be displayed after the invisible characters.
`----

so I am wondering if a new form like `(ATOM . STRING)` can be supported so that STRING can be displayed after the invisible characters instead of ellipses.

>> More to the point, what do people think about the making the extent of an outline of an entry include a new line? Is that more or less logical than the current behaviour?
>
> How would that help?  The display engine is blissfully unaware of Org
> headings and entries, it just sees text in a buffer.
The question was indeed directed to org users rather than to display engine developers.

The suggested solution is have an outline entry include the new line character (so that the whole extended lines is hidden). The displayed text after the invisible text would then be "...\n".
The final output is behaviour to the current one except for two things:
1. The entry would contain "\n'", logically speaking.
2. The extended line would be hidden as part of the entry.

I hope this clarifies my point.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Abdul-Lateef Haji-Ali <abdo.haji.ali@gmail.com>
> Cc: yantar92@posteo.net, 59141@debbugs.gnu.org, kevin.legouguec@gmail.com,
>  juri@linkov.net
> Date: Fri, 11 Nov 2022 19:47:45 +0000
> 
> 
> On 11/11/2022, Eli Zaretskii wrote:
> > I don't think I understand the question.  What do you mean by "overlay
> > rendering"?  An overlay can produce text only if it has a
> > before-string or after-string property, so what do you mean by
> > "arbitrary text"?
> 
> I might be using the wrong terms here, but I think the idea is straightforward.
> According to `buffer-invisibility-spec` docs
> 
> ,----
> | If the (ATOM . ELLIPSIS) form is used, and ELLIPSIS is non-nil, an
> | ellipsis will be displayed after the invisible characters.
> `----
> 
> so I am wondering if a new form like `(ATOM . STRING)` can be supported so that STRING can be displayed after the invisible characters instead of ellipses.

This is what the 'display' property already does, so I don't see why
we'd need yet another way of replacing some buffer text with a
different text on display.

> The suggested solution is have an outline entry include the new line character (so that the whole extended lines is hidden). The displayed text after the invisible text would then be "...\n".

AFAIU, your proposal will make this:

,----
| * A
| TextA
| * B
| TextB
`----

be displayed like this after hiding the bodies:

,----
| * A* B
`----

because the newline will be hidden

AFAIU, this is not what Org wants to display when the bodies of the
headings are hidden.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Abdul-Lateef Haji-Ali]

On 11/11/2022, Eli Zaretskii wrote:
>> so I am wondering if a new form like `(ATOM . STRING)` can be supported so that STRING can be displayed after the invisible characters instead of ellipses.
>
> This is what the 'display' property already does, so I don't see why
> we'd need yet another way of replacing some buffer text with a
> different text on display.

Yes indeed, but I assumed that controlling visibility in the buffer by modifying the visibility-spec would be desirable, which is difficult to achieve with `display`.

>> The suggested solution is have an outline entry include the new line character (so that the whole extended lines is hidden). The displayed text after the invisible text would then be "...\n".
> ,----
> | * A* B
> `----
>
> because the newline will be hidden

At the moment, if the new line is included in the entry then the output would be
,----
| * A...* B
`----

However, if the displayed text after the invisible text is changed to "...\n" instead of "..." (through `display` or better yet through a change to invisibility-specs as I suggested) then the output is correct.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Abdul-Lateef Haji-Ali <abdo.haji.ali@gmail.com>
> Cc: yantar92@posteo.net, 59141@debbugs.gnu.org, kevin.legouguec@gmail.com,
>  juri@linkov.net
> Date: Fri, 11 Nov 2022 20:17:26 +0000
> 
> >> The suggested solution is have an outline entry include the new line character (so that the whole extended lines is hidden). The displayed text after the invisible text would then be "...\n".
> > ,----
> > | * A* B
> > `----
> >
> > because the newline will be hidden
> 
> At the moment, if the new line is included in the entry then the output would be
> ,----
> | * A...* B
> `----
> 
> However, if the displayed text after the invisible text is changed to "...\n" instead of "..." (through `display` or better yet through a change to invisibility-specs as I suggested) then the output is correct.

You are basically suggesting to hide the entire buffer text and
instead display something else via display properties.  That's not
what outline modes do.

The solution to this problem is simple: don't use the :extend
attribute for these faces.  That's all.  This attribute is not
intended for what you want to achieve here.  A whole bunch of problems
automatically gets resolved if you don't use :extend.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

> You are basically suggesting to hide the entire buffer text and
> instead display something else via display properties.  That's not
> what outline modes do.
>
> The solution to this problem is simple: don't use the :extend
> attribute for these faces.  That's all.  This attribute is not
> intended for what you want to achieve here.  A whole bunch of problems
> automatically gets resolved if you don't use :extend.

Can't speak to what :extend is intended for, but I think it's worth
noting that magit-section successfully combines overlays with :extend to
do exactly what outline.el does (a hierarchy of collapsable buffer
sections), without the problem under discussion.

If someone came up with a patch to allow outline.el to delimit and hide
sections using a logic closer to magit-section's[1] (via an opt-in
variable, to be set e.g. by major modes, so that users happy with the
status quo are not affected), would that patch be given any
consideration?

Asking because OT1H, it seems to me that this kind of change would be on
par with Juri's proposed outline-search-function in bug#53981, i.e. an
opt-in change to a core part of outline.el's design; OTOH the above
sounds quite final, so I don't want to waste anyone's time.


[1] Here are the relevant parts AFAIU, based on a cursory exploration.

Obviously nothing can be ported verbatim to outline.el: e.g. the eieio
idioms are not suited, neither are the parts that manipulate buffer
content directly (like magit-diff-wash-hunk deleting then re-inserting
the hunk heading).

Still, I'm hopeful we could teach outline.el the logic of where headings
end and content starts, and how to set overlays to hide the latter.

* magit-section.el:

(defclass magit-section ()
  ([…]
   (start    :initform nil :initarg :start)
   (content  :initform nil)
   (end      :initform nil)
   […]))

(defun magit-section-hide (section)
  "Hide the body of the current section."
  (interactive (list (magit-current-section)))
  (if (eq section magit-root-section)
      (user-error "Cannot hide root section")
    (oset section hidden t)
    (when-let ((beg (oref section content)))
      (let ((end (oref section end)))
        (when (< beg (point) end)
          (goto-char (oref section start)))
        (remove-overlays beg end 'invisible t)
        (let ((o (make-overlay beg end)))
          (overlay-put o 'evaporate t)
          (overlay-put o 'invisible t))))
    […]))

(defmacro magit-insert-section (&rest args)
  "Insert a section at point.
[…]"
  […]
                         :start (point-marker)
  […])

* magit-diff.el:

(defun magit-diff-wash-hunk ()
  (when (looking-at "^@\\{2,\\} \\(.+?\\) @\\{2,\\}\\(?: \\(.*\\)\\)?")
    (let* ((heading  (match-string 0))
           (ranges   (mapcar
                      (lambda (str)
                        (let ((range
                               (mapcar #'string-to-number
                                       (split-string (substring str 1) ","))))
                          ;; A single line is +1 rather than +1,1.
                          (if (length= range 1)
                              (nconc range (list 1))
                            range)))
                      (split-string (match-string 1))))
           (about    (match-string 2))
           (combined (length= ranges 3))
           (value    (cons about ranges)))
      (magit-delete-line)
      (magit-insert-section section (hunk value)
        (insert (propertize (concat heading "\n")
                            'font-lock-face 'magit-diff-hunk-heading))
        (magit-insert-heading)
        (while (not (or (eobp) (looking-at "^[^-+\s\\]")))
          (forward-line))
        (oset section end (point))
        (oset section washer #'magit-diff-paint-hunk)
        (oset section combined combined)
        […]))
    t))

JS-less, text/plain references:

https://raw.githubusercontent.com/magit/magit/master/lisp/magit-section.el
https://raw.githubusercontent.com/magit/magit/master/lisp/magit-diff.el

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

> If someone came up with a patch to allow outline.el to delimit and hide
> sections using a logic closer to magit-section's[1] (via an opt-in
> variable, to be set e.g. by major modes, so that users happy with the
> status quo are not affected), would that patch be given any
> consideration?

I suspect magit just inserts newlines without text properties
between outlines, thus avoiding the problem.  Could you send
a screenshot that shows a newline with the :extend attribute
as the final character of the outline in magit?

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Kévin Le Gouguec]

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

Juri Linkov <juri@linkov.net> writes:

>> If someone came up with a patch to allow outline.el to delimit and hide
>> sections using a logic closer to magit-section's[1] (via an opt-in
>> variable, to be set e.g. by major modes, so that users happy with the
>> status quo are not affected), would that patch be given any
>> consideration?
>
> I suspect magit just inserts newlines without text properties
> between outlines, thus avoiding the problem.

I don't believe that to be the case?  Diff sections, for example, are
shown as an outline with 3 levels:

(1) filename heading,
(2) hunk headings,
(3) the hunk content (context lines + removed and added lines).

(1), (2) and (3) are painted with :extended faces, applied through text
properties (the magit-diff-file-heading, magit-diff-hunk-heading and
magit-diff-{context,removed,added}, respectively).

When under point, they also get painted with another set of :extended
faces (e.g. magit-diff-file-heading-highlight), applied either through
overlays ((1), (2)) or text properties (3).

AFAICT there is no single unpainted newline beween these sections.

>                                               Could you send
> a screenshot that shows a newline with the :extend attribute
> as the final character of the outline in magit?

See attached, showing a recent revision from emacs.git master, with the
diff completely folded (1.png) with hunks folded (2.png) and completely
unfolded (3.png).


I hope I understood your suspicion correctly and my answer was relevant?
Let me know if I misunderstood.  FWIW "insert a supplementary newline
before headings" is indeed a well-known workaround in the Org community.


[-- Attachment #2: 1.png --]
[-- Type: image/png, Size: 78804 bytes --]

[-- Attachment #3: 2.png --]
[-- Type: image/png, Size: 91712 bytes --]

[-- Attachment #4: 3.png --]
[-- Type: image/png, Size: 192074 bytes --]

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

>> Could you send a screenshot that shows a newline with the :extend
>> attribute as the final character of the outline in magit?
>
> See attached, showing a recent revision from emacs.git master, with the
> diff completely folded (1.png) with hunks folded (2.png) and completely
> unfolded (3.png).
>
> I hope I understood your suspicion correctly and my answer was relevant?
> Let me know if I misunderstood.  FWIW "insert a supplementary newline
> before headings" is indeed a well-known workaround in the Org community.

Thanks for the screenshots.  What I still don't see is an example
where a newline at the end of the last line of the outline (diff hunk)
has some non-default face.  I.e. the same as in your test case in
https://debbugs.gnu.org/52587#13

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Kévin Le Gouguec]

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

Juri Linkov <juri@linkov.net> writes:

>>> Could you send a screenshot that shows a newline with the :extend
>>> attribute as the final character of the outline in magit?
>>
>> See attached, showing a recent revision from emacs.git master, with the
>> diff completely folded (1.png) with hunks folded (2.png) and completely
>> unfolded (3.png).
>>
>> I hope I understood your suspicion correctly and my answer was relevant?
>> Let me know if I misunderstood.  FWIW "insert a supplementary newline
>> before headings" is indeed a well-known workaround in the Org community.
>
> Thanks for the screenshots.  What I still don't see is an example
> where a newline at the end of the last line of the outline (diff hunk)
> has some non-default face.  I.e. the same as in your test case in
> https://debbugs.gnu.org/52587#13

All of them do, unless I misunderstand something?  I.e. all hunks are
painted in #100f10, the background of magit-diff-context-highlight,
while the default face has background #000000.

Attaching an… "annotated" version of the previous screenshots.  To
recap:

* default background is #000000,

* magit-diff-file-heading-highlight background is #203448,

* magit-diff-hunk-heading-highlight background is #323232,

* magit-diff-context-highlight background is #100f10,

* when a hunk is folded, the background for the final newline of the
  hunk content (#100f10) does not "bleed" into the hunk header's
  extension,

* instead, this hunk header's extension keeps the color it has when the
  hunk is unfolded, i.e. the same color as the rest of the header
  (#323232).

Let me know if that clarifies things, or if I've misunderstood what you
are asking for.


[-- Attachment #2: annotated.png --]
[-- Type: image/png, Size: 266039 bytes --]

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

> * when a hunk is folded, the background for the final newline of the
>   hunk content (#100f10) does not "bleed" into the hunk header's
>   extension,
>
> * instead, this hunk header's extension keeps the color it has when the
>   hunk is unfolded, i.e. the same color as the rest of the header
>   (#323232).
>
> Let me know if that clarifies things, or if I've misunderstood what you
> are asking for.

Thanks for the clarification.  One thing I still don't understand is
does magit use outline-mode?  I see the outline indicators in the left margin
that are available from outline-minor-mode.  But the mode-line shows that
outline-mode is not enabled.  And indeed, there are two things that confirm
that outline-mode is not used: 1. there is no ellipsis at the end of the
outline heading lines; 2. currently outline-mode is unable to use the
face attribute ':extended' to extend the face of the outline headings
to the end of the window as it's shown on your screenshot, because the
outline faces are not added to the newline on the outline heading line.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Kévin Le Gouguec]

Juri Linkov <juri@linkov.net> writes:

>> * when a hunk is folded, the background for the final newline of the
>>   hunk content (#100f10) does not "bleed" into the hunk header's
>>   extension,
>>
>> * instead, this hunk header's extension keeps the color it has when the
>>   hunk is unfolded, i.e. the same color as the rest of the header
>>   (#323232).
>>
>> Let me know if that clarifies things, or if I've misunderstood what you
>> are asking for.
>
> Thanks for the clarification.  One thing I still don't understand is
> does magit use outline-mode?

It does not; it relies on magit-section (see references a couple of
messages prior), which does more or less the exact same job outline.el
does: let the user navigate and un/fold a hierarchy of headings.

Some recent features of outline.el have indeed been in magit-section for
a long time: visibility cycling with TAB/S-TAB, bitmap fringe indicators
(with fallback to outline-style ellipses e.g. on TTYs).

My point with this comparison is to show that an outline-like UI with
:extended backgrounds is obviously possible; in my previous messages, I
tried to highlight the relevant code in magit-section that handles
delimiting section headings vs content and setting the overlays.

I did that mainly FTR, so that Someone™ with motivation and time can see
if outline.el could grow a user option to support a similar way to
display outlines, thus solving the problem of :extended backgrounds.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

>> I still don't understand is does magit use outline-mode?
>
> It does not; it relies on magit-section (see references a couple of
> messages prior), which does more or less the exact same job outline.el
> does: let the user navigate and un/fold a hierarchy of headings.
>
> Some recent features of outline.el have indeed been in magit-section for
> a long time: visibility cycling with TAB/S-TAB, bitmap fringe indicators
> (with fallback to outline-style ellipses e.g. on TTYs).
>
> My point with this comparison is to show that an outline-like UI with
> :extended backgrounds is obviously possible; in my previous messages, I
> tried to highlight the relevant code in magit-section that handles
> delimiting section headings vs content and setting the overlays.
>
> I did that mainly FTR, so that Someone™ with motivation and time can see
> if outline.el could grow a user option to support a similar way to
> display outlines, thus solving the problem of :extended backgrounds.

I haven't looked at the magit-section source code.  I once tried
to copy the syntax highlighting code from diff-mode to magit-diff,
but magit code is such a mess that I abandoned the attempt.

But from your screenshots it's clear what is needed to do
to achieve the same in outline(-minor)-mode:

1. to support the :extended face attribute on the outline heading lines,
newlines should be included in the match.  This is not a patch,
but only shows possible changes:

@@ -242,7 +242,7 @@ outline-font-lock-keywords
   '(
     ;; Highlight headings according to the level.
     (eval . (list (or outline-search-function
-                      (concat "^\\(?:" outline-regexp "\\).*"))
+                      (concat "^\\(?:" outline-regexp "\\).*\n"))
                   0 '(if outline-minor-mode
                          (if outline-minor-mode-highlight
                              (list 'face (outline-font-lock-face)))
@@ -486,7 +486,7 @@ outline-minor-mode-highlight-buffer
   (save-excursion
     (goto-char (point-min))
     (let ((regexp (unless outline-search-function
-                    (concat "^\\(?:" outline-regexp "\\).*$"))))
+                    (concat "^\\(?:" outline-regexp "\\).*\n"))))
       (while (if outline-search-function
                  (funcall outline-search-function)
                (re-search-forward regexp nil t))

Maybe such changes are not needed when a function in
outline-search-function could include newlines in the match.

2. not to hide the newline of the outline heading line,
overlay boundaries could be shifted forward by 1:

@@ -960,7 +960,7 @@ outline-flag-region
     ;; We use `front-advance' here because the invisible text begins at the
     ;; very end of the heading, before the newline, so text inserted at FROM
     ;; belongs to the heading rather than to the entry.
-    (let ((o (make-overlay from to nil 'front-advance)))
+    (let ((o (make-overlay (1+ from) (1+ to) nil 'front-advance)))
       (overlay-put o 'evaporate t)
       (overlay-put o 'invisible 'outline)
       (overlay-put o 'isearch-open-invisible

Maybe this could be conditional via a new option that you proposed.

3. Your screenshot shows that magit doesn't use an ellipsis.
And indeed, ellipses get in the way.  But we need to find a way
to disable them without breaking this feature.  The line

  (overlay-put o 'invisible 'outline)

either should be replaced with

  (overlay-put o 'invisible t)

or the ellipsis glyph to be disabled with something like:

  (or standard-display-table (setq standard-display-table (make-display-table)))
  (set-char-table-extra-slot standard-display-table 4 (vector))

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 59141@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,  Abdul-Lateef
>  Haji-Ali <abdo.haji.ali@gmail.com>,  yantar92@posteo.net
> Date: Mon, 14 Nov 2022 19:32:04 +0200
> 
> or the ellipsis glyph to be disabled with something like:
> 
>   (or standard-display-table (setq standard-display-table (make-display-table)))
>   (set-char-table-extra-slot standard-display-table 4 (vector))

Why not just set selective-display-ellipses to nil?

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

>> or the ellipsis glyph to be disabled with something like:
>>
>>   (or standard-display-table (setq standard-display-table (make-display-table)))
>>   (set-char-table-extra-slot standard-display-table 4 (vector))
>
> Why not just set selective-display-ellipses to nil?

(setq-default selective-display-ellipses nil) doesn't remove ellipses.
What am I missing?

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: kevin.legouguec@gmail.com,  59141@debbugs.gnu.org,
>   abdo.haji.ali@gmail.com,  yantar92@posteo.net
> Date: Tue, 15 Nov 2022 10:02:38 +0200
> 
> >> or the ellipsis glyph to be disabled with something like:
> >>
> >>   (or standard-display-table (setq standard-display-table (make-display-table)))
> >>   (set-char-table-extra-slot standard-display-table 4 (vector))
> >
> > Why not just set selective-display-ellipses to nil?
> 
> (setq-default selective-display-ellipses nil) doesn't remove ellipses.
> What am I missing?

Ah, it only works when selective-display is turned on.

We could perhaps extend it to affect ellipsis after text hidden by
properties and overlays.  Would that be helpful?

Alternatively, we could fix the documentation to clarify that this
variable doesn't affect text hidden by invisible properties.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> >> or the ellipsis glyph to be disabled with something like:
>> >>
>> >>   (or standard-display-table (setq standard-display-table (make-display-table)))
>> >>   (set-char-table-extra-slot standard-display-table 4 (vector))
>> >
>> > Why not just set selective-display-ellipses to nil?
>> 
>> (setq-default selective-display-ellipses nil) doesn't remove ellipses.
>> What am I missing?
>
> Ah, it only works when selective-display is turned on.
>
> We could perhaps extend it to affect ellipsis after text hidden by
> properties and overlays.  Would that be helpful?
>
> Alternatively, we could fix the documentation to clarify that this
> variable doesn't affect text hidden by invisible properties.

What about simply changing the buffer-invisibility-spec?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Juri Linkov <juri@linkov.net>,  kevin.legouguec@gmail.com,
>   59141@debbugs.gnu.org,  abdo.haji.ali@gmail.com
> Date: Tue, 15 Nov 2022 15:01:38 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> >> or the ellipsis glyph to be disabled with something like:
> >> >>
> >> >>   (or standard-display-table (setq standard-display-table (make-display-table)))
> >> >>   (set-char-table-extra-slot standard-display-table 4 (vector))
> >> >
> >> > Why not just set selective-display-ellipses to nil?
> >> 
> >> (setq-default selective-display-ellipses nil) doesn't remove ellipses.
> >> What am I missing?
> >
> > Ah, it only works when selective-display is turned on.
> >
> > We could perhaps extend it to affect ellipsis after text hidden by
> > properties and overlays.  Would that be helpful?
> >
> > Alternatively, we could fix the documentation to clarify that this
> > variable doesn't affect text hidden by invisible properties.
> 
> What about simply changing the buffer-invisibility-spec?

I'm not following: how is that question related to what
selective-display-ellipses does (or does not do)?

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> What about simply changing the buffer-invisibility-spec?
>
> I'm not following: how is that question related to what
> selective-display-ellipses does (or does not do)?

It is related to "ellipsis glyph to be disabled" (parent message in the
thread).

If a text is hidden using invisible property, the appearance of such
text can be controlled by `buffer-invisibility-spec'. One can display
default ellipsis or custom ellipsis or disable ellipsis completely.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: juri@linkov.net,  kevin.legouguec@gmail.com,  59141@debbugs.gnu.org,
>   abdo.haji.ali@gmail.com
> Date: Wed, 16 Nov 2022 01:38:17 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> What about simply changing the buffer-invisibility-spec?
> >
> > I'm not following: how is that question related to what
> > selective-display-ellipses does (or does not do)?
> 
> It is related to "ellipsis glyph to be disabled" (parent message in the
> thread).
> 
> If a text is hidden using invisible property, the appearance of such
> text can be controlled by `buffer-invisibility-spec'. One can display
> default ellipsis or custom ellipsis or disable ellipsis completely.

AFAIU, that's what Juri suggested, to which I replied that using
selective-display-ellipses could be easier (except that it doesn't
work in this situation).

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

>>> What about simply changing the buffer-invisibility-spec?
>>
>> I'm not following: how is that question related to what
>> selective-display-ellipses does (or does not do)?
>
> It is related to "ellipsis glyph to be disabled" (parent message in the
> thread).
>
> If a text is hidden using invisible property, the appearance of such
> text can be controlled by `buffer-invisibility-spec'. One can display
> default ellipsis or custom ellipsis or disable ellipsis completely.

You are right, the ellipsis could be disabled by `buffer-invisibility-spec'
when buttons are used to indicate the hidden outlines.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Kévin Le Gouguec]

Juri Linkov <juri@linkov.net> writes:

>> My point with this comparison is to show that an outline-like UI with
>> :extended backgrounds is obviously possible; in my previous messages, I
>> tried to highlight the relevant code in magit-section that handles
>> delimiting section headings vs content and setting the overlays.
>>
>> I did that mainly FTR, so that Someone™ with motivation and time can see
>> if outline.el could grow a user option to support a similar way to
>> display outlines, thus solving the problem of :extended backgrounds.
>
> I haven't looked at the magit-section source code.  I once tried
> to copy the syntax highlighting code from diff-mode to magit-diff,
> but magit code is such a mess that I abandoned the attempt.
>
> But from your screenshots it's clear what is needed to do
> to achieve the same in outline(-minor)-mode:

I don't think I found anything particularly messy about magit-section,
but piecing together the logic responsible for delimiting section
headings and content certainly took me enough time that I felt it was
worth summarizing.

Glad the screenshots helped.

> 3. Your screenshot shows that magit doesn't use an ellipsis.
> And indeed, ellipses get in the way.  But we need to find a way
> to disable them without breaking this feature.  The line
>
>   (overlay-put o 'invisible 'outline)
>
> either should be replaced with
>
>   (overlay-put o 'invisible t)
>
> or the ellipsis glyph to be disabled with something like:
>
>   (or standard-display-table (setq standard-display-table (make-display-table)))
>   (set-char-table-extra-slot standard-display-table 4 (vector))

As I mentioned, magit supports replacing its fringe bitmaps with
ellipses.  See the magit-section-visibility-indicator user option, and
the functions that act on it[1].  tl;dr the option can be set to

* (SYMBOL1 . SYMBOL2): each symbol describes a fringe bitmap to be used
  for expandable (1) or collapsible (2) sections,

* (STRING . BOOLEAN): STRING is appended at the end of collapsed
  sections.

IIUC magit-section sets these ellipses via 'after-string overlays on the
last character before a heading's newline, rather than adjusting display
tables.


Thanks for weighing in on how outline.el could be adapted, Juri.  Can't
promise I'll be able to act on your advice and turn it into a patch
anytime soon, but finally hashing out the implementation details between
these two packages has made me more hopeful outline.el can be taught
more tricks without resorting to ugly hacks.


[1]

  (defcustom magit-section-visibility-indicator
    (if (window-system)
        '(magit-fringe-bitmap> . magit-fringe-bitmapv)
      (cons (if (char-displayable-p ?…) "…" "...")
            t))
    "Whether and how to indicate that a section can be expanded/collapsed.

  If nil, then don't show any indicators.
  Otherwise the value has to have one of these two forms:

  \(EXPANDABLE-BITMAP . COLLAPSIBLE-BITMAP)

    Both values have to be variables whose values are fringe
    bitmaps.  In this case every section that can be expanded or
    collapsed gets an indicator in the left fringe.

    To provide extra padding around the indicator, set
    `left-fringe-width' in `magit-mode-hook'.

  \(STRING . BOOLEAN)

    In this case STRING (usually an ellipsis) is shown at the end
    of the heading of every collapsed section.  Expanded sections
    get no indicator.  The cdr controls whether the appearance of
    these ellipsis take section highlighting into account.  Doing
    so might potentially have an impact on performance, while not
    doing so is kinda ugly."
    :package-version '(magit-section . "3.0.0")
    :group 'magit-section
    :type '(choice (const :tag "No indicators" nil)
                   (cons  :tag "Use +- fringe indicators"
                          (const magit-fringe-bitmap+)
                          (const magit-fringe-bitmap-))
                   (cons  :tag "Use >v fringe indicators"
                          (const magit-fringe-bitmap>)
                          (const magit-fringe-bitmapv))
                   (cons  :tag "Use bold >v fringe indicators)"
                          (const magit-fringe-bitmap-bold>)
                          (const magit-fringe-bitmap-boldv))
                   (cons  :tag "Use custom fringe indicators"
                          (variable :tag "Expandable bitmap variable")
                          (variable :tag "Collapsible bitmap variable"))
                   (cons  :tag "Use ellipses at end of headings"
                          (string :tag "Ellipsis" "…")
                          (choice :tag "Use face kludge"
                                  (const :tag "Yes (potentially slow)" t)
                                  (const :tag "No (kinda ugly)" nil)))))

  (defun magit-section-maybe-update-visibility-indicator (section)
    (when (and magit-section-visibility-indicator
               (magit-section-content-p section))
      (let* ((beg (oref section start))
             (eoh (save-excursion
                    (goto-char beg)
                    (line-end-position))))
        (cond
         ((symbolp (car-safe magit-section-visibility-indicator))
          (let ((ov (magit--overlay-at beg 'magit-vis-indicator 'fringe)))
            (unless ov
              (setq ov (make-overlay beg eoh nil t))
              (overlay-put ov 'evaporate t)
              (overlay-put ov 'magit-vis-indicator 'fringe))
            (overlay-put
             ov 'before-string
             (propertize "fringe" 'display
                         (list 'left-fringe
                               (if (oref section hidden)
                                   (car magit-section-visibility-indicator)
                                 (cdr magit-section-visibility-indicator))
                               'fringe)))))
         ((stringp (car-safe magit-section-visibility-indicator))
          (let ((ov (magit--overlay-at (1- eoh) 'magit-vis-indicator 'eoh)))
            (cond ((oref section hidden)
                   (unless ov
                     (setq ov (make-overlay (1- eoh) eoh))
                     (overlay-put ov 'evaporate t)
                     (overlay-put ov 'magit-vis-indicator 'eoh))
                   (overlay-put ov 'after-string
                                (car magit-section-visibility-indicator)))
                  (ov
                   (delete-overlay ov)))))))))

  (defvar-local magit--ellipses-sections nil)

  (defun magit-section-maybe-paint-visibility-ellipses ()
    ;; This is needed because we hide the body instead of "the body
    ;; except the final newline and additionally the newline before
    ;; the body"; otherwise we could use `buffer-invisibility-spec'.
    (when (stringp (car-safe magit-section-visibility-indicator))
      (let* ((sections (append magit--ellipses-sections
                               (setq magit--ellipses-sections
                                     (or (magit-region-sections)
                                         (list (magit-current-section))))))
             (beg (--map (oref it start) sections))
             (end (--map (oref it end)   sections)))
        (when (region-active-p)
          ;; This ensures that the region face is removed from ellipses
          ;; when the region becomes inactive, but fails to ensure that
          ;; all ellipses within the active region use the region face,
          ;; because the respective overlay has not yet been updated at
          ;; this time.  The magit-selection face is always applied.
          (push (region-beginning) beg)
          (push (region-end)       end))
        (setq beg (apply #'min beg))
        (setq end (apply #'max end))
        (dolist (ov (overlays-in beg end))
          (when (eq (overlay-get ov 'magit-vis-indicator) 'eoh)
            (overlay-put
             ov 'after-string
             (propertize
              (car magit-section-visibility-indicator) 'font-lock-face
              (let ((pos (overlay-start ov)))
                (delq nil (nconc (--map (overlay-get it 'font-lock-face)
                                        (overlays-at pos))
                                 (list (get-char-property
                                        pos 'font-lock-face))))))))))))

  (defun magit-section-maybe-remove-visibility-indicator (section)
    (when (and magit-section-visibility-indicator
               (= (oref section content)
                  (oref section end)))
      (dolist (o (overlays-in (oref section start)
                              (save-excursion
                                (goto-char (oref section start))
                                (1+ (line-end-position)))))
        (when (overlay-get o 'magit-vis-indicator)
          (delete-overlay o)))))

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

> As I mentioned, magit supports replacing its fringe bitmaps with
> ellipses.  See the magit-section-visibility-indicator user option, and
> the functions that act on it[1].  tl;dr the option can be set to
>
> * (SYMBOL1 . SYMBOL2): each symbol describes a fringe bitmap to be used
>   for expandable (1) or collapsible (2) sections,
>
> * (STRING . BOOLEAN): STRING is appended at the end of collapsed
>   sections.
>
> IIUC magit-section sets these ellipses via 'after-string overlays on the
> last character before a heading's newline, rather than adjusting display
> tables.
>
> Thanks for weighing in on how outline.el could be adapted, Juri.  Can't
> promise I'll be able to act on your advice and turn it into a patch
> anytime soon, but finally hashing out the implementation details between
> these two packages has made me more hopeful outline.el can be taught
> more tricks without resorting to ugly hacks.

Thanks for summarizing the relevant details of magit-section.
Now everything is clear.

>   (defun magit-section-maybe-paint-visibility-ellipses ()
>     ;; This is needed because we hide the body instead of "the body
>     ;; except the final newline and additionally the newline before
>     ;; the body"; otherwise we could use `buffer-invisibility-spec'.

This comment explains why this is needed.  I believe we could avoid
ugly hacks in outline.el because unlike magit-section, outline.el can
display arrows on the left side even on TTYs.  But since the default
ellipses should be disabled in any case, even when users don't want to use
the left-side arrows, we could add ellipses in a more clean way: as a new
value of the user option 'outline-minor-mode-use-buttons'.  So when it's
customized to 'ellipsis', the ellipsis overlay will be created in a new
pcase branch in outline--insert-button.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

> we could add ellipses in a more clean way: as a new value of the user
> option 'outline-minor-mode-use-buttons'.  So when it's customized to
> 'ellipsis', the ellipsis overlay will be created in a new pcase branch
> in outline--insert-button.

Unfortunately, this is not possible to do because of a bug
in overlay handling.  Here is a short test case:

(progn
  (insert "ab")
  (let ((o (make-overlay 1 2)))
    (overlay-put o 'after-string "!"))
  (let ((o (make-overlay 2 3)))
    (overlay-put o 'invisible t)))

The problem is that "!" from 'after-string' is not displayed,
even when it's on the text that is not invisible.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 59141@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,  Abdul-Lateef
>  Haji-Ali <abdo.haji.ali@gmail.com>,  yantar92@posteo.net
> Date: Tue, 22 Nov 2022 09:52:40 +0200
> 
> > we could add ellipses in a more clean way: as a new value of the user
> > option 'outline-minor-mode-use-buttons'.  So when it's customized to
> > 'ellipsis', the ellipsis overlay will be created in a new pcase branch
> > in outline--insert-button.
> 
> Unfortunately, this is not possible to do because of a bug
> in overlay handling.  Here is a short test case:
> 
> (progn
>   (insert "ab")
>   (let ((o (make-overlay 1 2)))
>     (overlay-put o 'after-string "!"))
>   (let ((o (make-overlay 2 3)))
>     (overlay-put o 'invisible t)))
> 
> The problem is that "!" from 'after-string' is not displayed,
> even when it's on the text that is not invisible.

There's no bug here.  after-string is displayed when the end position of the
overlay is reached, and in this case the end position is invisible, so it is
skipped.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

>> (progn
>>   (insert "ab")
>>   (let ((o (make-overlay 1 2)))
>>     (overlay-put o 'after-string "!"))
>>   (let ((o (make-overlay 2 3)))
>>     (overlay-put o 'invisible t)))
>>
>> The problem is that "!" from 'after-string' is not displayed,
>> even when it's on the text that is not invisible.
>
> There's no bug here.  after-string is displayed when the end position of the
> overlay is reached, and in this case the end position is invisible, so it is
> skipped.

If really there is no way to display a string overlay before the invisible overlay,
this is fine because when a new option will be added, the ellipsis overlay
will be added to the end of the line, and the invisible overlay will start
at the beginning of the next line.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: kevin.legouguec@gmail.com,  59141@debbugs.gnu.org,
>   abdo.haji.ali@gmail.com,  yantar92@posteo.net
> Date: Tue, 22 Nov 2022 19:35:34 +0200
> 
> >> (progn
> >>   (insert "ab")
> >>   (let ((o (make-overlay 1 2)))
> >>     (overlay-put o 'after-string "!"))
> >>   (let ((o (make-overlay 2 3)))
> >>     (overlay-put o 'invisible t)))
> >>
> >> The problem is that "!" from 'after-string' is not displayed,
> >> even when it's on the text that is not invisible.
> >
> > There's no bug here.  after-string is displayed when the end position of the
> > overlay is reached, and in this case the end position is invisible, so it is
> > skipped.
> 
> If really there is no way to display a string overlay before the invisible overlay,
> this is fine because when a new option will be added, the ellipsis overlay
> will be added to the end of the line, and the invisible overlay will start
> at the beginning of the next line.

Does the below produce what you want?

  (progn
    (insert "ab")
    (let ((o (make-overlay 2 3)))
      (overlay-put o 'after-string "!"))
    (let ((o (make-overlay 2 3)))
      (overlay-put o 'invisible t)))

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

> Does the below produce what you want?
>
>   (progn
>     (insert "ab")
>     (let ((o (make-overlay 2 3)))
>       (overlay-put o 'after-string "!"))
>     (let ((o (make-overlay 2 3)))
>       (overlay-put o 'invisible t)))

Thanks, this is a possibility.  Maybe not easy to use
since for an ellipsis overlay will need to get the boundaries
of the invisible outline overlay.  Or maybe simpler to put
'after-string' on the same invisible overlay like this:

@@ -973,6 +974,8 @@ outline-flag-region
     ;; belongs to the heading rather than to the entry.
     (let ((o (make-overlay from to nil 'front-advance)))
       (overlay-put o 'evaporate t)
+      (when (eq outline-minor-mode-use-buttons 'ellipsis)
+        (overlay-put o 'after-string "…"))
       (overlay-put o 'invisible 'outline)
       (overlay-put o 'isearch-open-invisible
 		   (or outline-isearch-open-invisible-function

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: kevin.legouguec@gmail.com,  59141@debbugs.gnu.org,
>   abdo.haji.ali@gmail.com,  yantar92@posteo.net
> Date: Tue, 22 Nov 2022 21:16:27 +0200
> 
> > Does the below produce what you want?
> >
> >   (progn
> >     (insert "ab")
> >     (let ((o (make-overlay 2 3)))
> >       (overlay-put o 'after-string "!"))
> >     (let ((o (make-overlay 2 3)))
> >       (overlay-put o 'invisible t)))
> 
> Thanks, this is a possibility.  Maybe not easy to use
> since for an ellipsis overlay will need to get the boundaries
> of the invisible outline overlay.

I don't think you need the exact boundaries.  All you need is to make sure
the START of the after-string overlay is in invisible text.  It doesn't
matter where its END is.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Juri Linkov]

> The solution to this problem is simple: don't use the :extend
> attribute for these faces.  That's all.  This attribute is not
> intended for what you want to achieve here.  A whole bunch of problems
> automatically gets resolved if you don't use :extend.

This problem exists even when the :extend attribute is not used as
Kévin demonstrated in https://debbugs.gnu.org/52587#13

So I see the only solution to modify the display engine
somewhere in `setup_for_ellipsis', so that after displaying
`default_invis_vector' or `DISP_INVIS_VECTOR', it could
remove all face text attributes from the next newline
that follows the ellipsis.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Abdul-Lateef Haji-Ali <abdo.haji.ali@gmail.com>,  yantar92@posteo.net,
>   59141@debbugs.gnu.org,  kevin.legouguec@gmail.com
> Date: Sat, 12 Nov 2022 19:52:31 +0200
> 
> > The solution to this problem is simple: don't use the :extend
> > attribute for these faces.  That's all.  This attribute is not
> > intended for what you want to achieve here.  A whole bunch of problems
> > automatically gets resolved if you don't use :extend.
> 
> This problem exists even when the :extend attribute is not used as
> Kévin demonstrated in https://debbugs.gnu.org/52587#13

That's a different problem: the face is not extended to the window's
edge.

I really don't understand why you expect Emacs not to display a
character using the face that a Lisp program puts on that character.

> So I see the only solution to modify the display engine
> somewhere in `setup_for_ellipsis', so that after displaying
> `default_invis_vector' or `DISP_INVIS_VECTOR', it could
> remove all face text attributes from the next newline
> that follows the ellipsis.

Not going to happen, not on my watch.  This kind of artificial changes
make no sense at all, they will only cause bugs.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Ihor Radchenko]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

>> I do not see anything wrong on the Org side.
>> Maybe Emacs should not apply :extent t attribute to the newline when the
>> text in fontified line is hidden?
>
> IIUC, this is bug#52587?
>
> The conclusion there, AFAIR, was "it's unfortunate, but that's the
> design for now; let's rediscuss when we see some patches for outline.el
> (or the specific modes that use extended backgrounds)".

I fixed the problem on Org side in bug#65896.
I think that this bug report and maybe even bug#52587 can be closed.
Unless it is still of interest to develop some solution on Emacs side.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: 59141@debbugs.gnu.org
> Date: Thu, 25 Jan 2024 22:53:45 +0000
> 
> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
> 
> >> I do not see anything wrong on the Org side.
> >> Maybe Emacs should not apply :extent t attribute to the newline when the
> >> text in fontified line is hidden?
> >
> > IIUC, this is bug#52587?
> >
> > The conclusion there, AFAIR, was "it's unfortunate, but that's the
> > design for now; let's rediscuss when we see some patches for outline.el
> > (or the specific modes that use extended backgrounds)".
> 
> I fixed the problem on Org side in bug#65896.
> I think that this bug report and maybe even bug#52587 can be closed.
> Unless it is still of interest to develop some solution on Emacs side.

Thanks, I'm closing both of those bugs.

bug#52587: bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: 59141@debbugs.gnu.org
> Date: Thu, 25 Jan 2024 22:53:45 +0000
> 
> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
> 
> >> I do not see anything wrong on the Org side.
> >> Maybe Emacs should not apply :extent t attribute to the newline when the
> >> text in fontified line is hidden?
> >
> > IIUC, this is bug#52587?
> >
> > The conclusion there, AFAIR, was "it's unfortunate, but that's the
> > design for now; let's rediscuss when we see some patches for outline.el
> > (or the specific modes that use extended backgrounds)".
> 
> I fixed the problem on Org side in bug#65896.
> I think that this bug report and maybe even bug#52587 can be closed.
> Unless it is still of interest to develop some solution on Emacs side.

Thanks, I'm closing both of those bugs.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Wed, 09 Nov 2022 02:24:24 +0000
> 
> I do not see anything wrong on the Org side.

Nothing wrong, no.  But there's also nothing wrong with the Emacs
behavior in this case: the last glyph on the "One" line has buffer
position inside the region that has the org-block-end-line face.

> Maybe Emacs should not apply :extent t attribute to the newline when the
> text in fontified line is hidden?

That'd be quite a complication.  The code which implements the :extend
attribute runs long after the invisible text was processed and
skipped, leaving no glyphs on display.  That code only knows what is
the face at the buffer position that is the last on the screen line.
So we'd need to go back and analyze the text which precedes the
newline, and see whether any characters there are nowhere on display,
which could be quite tricky, given the bidi reordering.

Also, your proposal would disallow having the :extend face on the
newline alone, I think.

So I'd rather we didn't.

I said many times that people shouldn't willy-nilly use the :extent
attribute, just because it looks cooler in some situation.  The intent
was and still is for faces not to have this attribute set, with very
few very special exceptions, like the 'region' face.  But people don't
listen...

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

> I said many times that people shouldn't willy-nilly use the :extent
> attribute, just because it looks cooler in some situation.  The intent
> was and still is for faces not to have this attribute set, with very
> few very special exceptions, like the 'region' face.  But people don't
> listen...

FWIW, my motivation for setting :extend on outline headings doesn't stem
from aesthetics.

Clearly delineating "sections" with colored backgrounds that span the
whole window width is a neat visual aid, IME; that's just one more guide
for the eye to exploit when e.g. locating the next same-level heading
(for those who like to look before they leap), getting a sense of how
"long" a section is (in terms of lines if expanded, or subsections if
folded), etc.

(Other people's sensibilities will necessarily differ, e.g. they might
find a document's structure easier to scan visually without distracting
colored backgrounds.  I certainly don't mean to imply that :extending
heading backgrounds would be "objectively" better, only that it would be
genuinely helpful for _some_ folks)

So this "visual aid" aspect is why I suspect reporters think being able
to :extend outline heading backgrounds is about as desirable as
:extending the hl-line or region faces, and it's a shame it breaks down
when folding.

It's not a critical feature to be missing, and that motivation does not
invalidate the implementation challenges you highlight.  I just wanted
to spell this point out FTR, as I didn't feel "it looks cooler" conveyed
it quite as convincingly 🙂

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: Ihor Radchenko <yantar92@posteo.net>,  59141@debbugs.gnu.org
> Date: Wed, 09 Nov 2022 23:19:42 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I said many times that people shouldn't willy-nilly use the :extent
> > attribute, just because it looks cooler in some situation.  The intent
> > was and still is for faces not to have this attribute set, with very
> > few very special exceptions, like the 'region' face.  But people don't
> > listen...
> 
> FWIW, my motivation for setting :extend on outline headings doesn't stem
> from aesthetics.
> 
> Clearly delineating "sections" with colored backgrounds that span the
> whole window width is a neat visual aid, IME;

Do you see a contradiction between the last two sentences?  Aren't
"neat visual aid" and "aesthetics" synonyms?

> So this "visual aid" aspect is why I suspect reporters think being able
> to :extend outline heading backgrounds is about as desirable as
> :extending the hl-line or region faces, and it's a shame it breaks down
> when folding.

My suggestion is to use "other means" for such cues.  Especially if
you want to have that in modes that hide text, such as Org.  The
:extend attribute wasn't intended to support this usage.

> It's not a critical feature to be missing, and that motivation does not
> invalidate the implementation challenges you highlight.  I just wanted
> to spell this point out FTR, as I didn't feel "it looks cooler" conveyed
> it quite as convincingly 🙂

I don't see the difference, really.  "Cool" isn't a derogatory
adjective, or at least I didn't mean it to be so.

bug#59141: 28.1.90; Face :extend when all the line but trailing \n is invisible

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Cc: Ihor Radchenko <yantar92@posteo.net>,  59141@debbugs.gnu.org
>> Date: Wed, 09 Nov 2022 23:19:42 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I said many times that people shouldn't willy-nilly use the :extent
>> > attribute, just because it looks cooler in some situation.  The intent
>> > was and still is for faces not to have this attribute set, with very
>> > few very special exceptions, like the 'region' face.  But people don't
>> > listen...
>> 
>> FWIW, my motivation for setting :extend on outline headings doesn't stem
>> from aesthetics.
>> 
>> Clearly delineating "sections" with colored backgrounds that span the
>> whole window width is a neat visual aid, IME;
>
> Do you see a contradiction between the last two sentences?  Aren't
> "neat visual aid" and "aesthetics" synonyms?

"Visual aid" evokes accessibility and ergonomics; "aesthetics" are only
tangentially related to those concepts, in my understanding at least.  I
would not consider them synonyms, FWIW.

AFAIU, accessibility and aesthetics are both connected to individual
preferences, and they're not completely independent; I still believe
that "making sections easier to spot" and "making sections prettier"
should not be conflated.

>> So this "visual aid" aspect is why I suspect reporters think being able
>> to :extend outline heading backgrounds is about as desirable as
>> :extending the hl-line or region faces, and it's a shame it breaks down
>> when folding.
>
> My suggestion is to use "other means" for such cues.  Especially if
> you want to have that in modes that hide text, such as Org.  The
> :extend attribute wasn't intended to support this usage.

Sure, and I'll take this opportunity to thank everyone who's worked on
outline.el and icons.el these past months - margin indicators and
icon-backed buttons are very welcome additions!

>> It's not a critical feature to be missing, and that motivation does not
>> invalidate the implementation challenges you highlight.  I just wanted
>> to spell this point out FTR, as I didn't feel "it looks cooler" conveyed
>> it quite as convincingly 🙂
>
> I don't see the difference, really.  "Cool" isn't a derogatory
> adjective, or at least I didn't mean it to be so.

My point wasn't to denounce derogatoryness but to try to reframe the
motivation for the feature.  "Cool" does not suggest forethought,
"willy-nilly" does not suggest purpose; I think people who want
:extended backgrounds lack for neither forethought nor purpose.

(And I felt like it was worth making that point because it was not
discussed the last time around, AFAIR.  In bug#52587, I mostly rambled
about the “abstract sense of what is and is not "part" of the subsection
to hide”; I don't recall us going into aesthetics nor accessibility)

Wouldn't want to belabor the point though; if I'm the only one who
thinks this distinction is any use, that's fair; apologies for the
noise.