bug#39360: Add :extend attribute to show-paren-mode expression face

bug#39360: Add :extend attribute to show-paren-mode expression face

[Clément Pit-Claudel]

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

Hi all,

I've attached a patch to add :extend the show-paren-mode face applied to expressions.  Is that the right way to return to the previous behavior?
I might have missed discussions about this; is there a reason why :extend t isn't the default?

Clément.

[-- Attachment #2: 0001-Add-extend-face-property-to-show-paren-match-express.patch --]
[-- Type: text/x-patch, Size: 875 bytes --]

From 7f7221e35d48755c814e4ee3b08c298a8717d883 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit-Claudel?= <clement.pitclaudel@live.com>
Date: Thu, 30 Jan 2020 16:02:02 -0500
Subject: [PATCH] Add :extend face property to show-paren-match-expression

* lisp/faces.el (show-paren-match-expression): Add :extend attribute.
---
 lisp/faces.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 9a49ea8104..6a4cf2a2a2 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2841,7 +2841,7 @@ show-paren-match
   :group 'paren-showing-faces)
 
 (defface show-paren-match-expression
-  '((t :inherit show-paren-match))
+  '((t :inherit show-paren-match :extend t))
   "Face used for a matching paren when highlighting the whole expression.
 This face is used by `show-paren-mode'."
   :group 'paren-showing-faces
-- 
2.17.1

bug#39360: Add :extend attribute to show-paren-mode expression face

[Dmitry Gutov]

Hi!

On 31.01.2020 0:08, Clément Pit-Claudel wrote:

> I've attached a patch to add :extend the show-paren-mode face applied to expressions.  Is that the right way to return to the previous behavior?

Yes. And I think it's a good choice for this face, going by analogy with 
the region face.

> I might have missed discussions about this; is there a reason why :extend t isn't the default?

Because we'd have had to disable it on too many faces... I think. Maybe 
Eli has a different answer.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Thu, 30 Jan 2020 16:08:50 -0500
> 
> I've attached a patch to add :extend the show-paren-mode face applied to expressions.  Is that the right way to return to the previous behavior?

Thanks.

Please describe a use case where this problem shows up, I'm not sure I
understand it completely.  More accurately, I don't think I understand
why you think we must have the previous behavior with this face,
i.e. why the new behavior is "wrong".

> I might have missed discussions about this; is there a reason why :extend t isn't the default?

Because the whole idea of introducing :extend was that almost all
faces don't need this attribute.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Clément Pit-Claudel]

On 2020-01-31 02:38, Eli Zaretskii wrote:
>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>> Date: Thu, 30 Jan 2020 16:08:50 -0500
>>
>> I've attached a patch to add :extend the show-paren-mode face applied to expressions.  Is that the right way to return to the previous behavior?
> 
> Thanks.
> 
> Please describe a use case where this problem shows up, I'm not sure I
> understand it completely.  More accurately, I don't think I understand
> why you think we must have the previous behavior with this face,
> i.e. why the new behavior is "wrong".

Unless people were complaining about the old behavior for this particular face, I think that behavior was fine.
More to the point, though, since the newline is inside the parentheses, it makes sense to me to extend the highlight continuously up to the edge of the window.

>> I might have missed discussions about this; is there a reason why :extend t isn't the default?
> 
> Because the whole idea of introducing :extend was that almost all
> faces don't need this attribute.

I see.  The new default looks weird to me, but I'm sure I'll get used to it.  Is there a reason to highlight one blank space at the end of each line, though, instead of only highlighting the text?

Clément.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> Cc: 39360@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 31 Jan 2020 09:52:25 -0500
> 
> > Please describe a use case where this problem shows up, I'm not sure I
> > understand it completely.  More accurately, I don't think I understand
> > why you think we must have the previous behavior with this face,
> > i.e. why the new behavior is "wrong".
> 
> Unless people were complaining about the old behavior for this particular face, I think that behavior was fine.

IMO it was not fine, because Emacs was behaving differently from every
other application out there which supports some form of highlighting
text that spans several lines.  The old behavior was also inconsistent
between different face attributes: background color behaved
differently from underline, for example, and GUI frames behaved
differently from TTY frames.

> More to the point, though, since the newline is inside the parentheses, it makes sense to me to extend the highlight continuously up to the edge of the window.

The newline is inside the parentheses, but the empty space to the
right of the newline is not part of the text.  Why should it be
highlighted?

> >> I might have missed discussions about this; is there a reason why :extend t isn't the default?
> > 
> > Because the whole idea of introducing :extend was that almost all
> > faces don't need this attribute.
> 
> I see.  The new default looks weird to me, but I'm sure I'll get used to it.

The question I'd like to ask is whether you'll get used to it wrt the
show-paren-mode face.

> Is there a reason to highlight one blank space at the end of each line, though, instead of only highlighting the text?

That blank space stands for the newline, so if the newline has that
face, the space glyph at EOL gets highlighted to indicate that fact.

bug#39360: Add :extend attribute to show-paren-mode expression face

[martin rudalics]

 > Is there a reason to highlight one blank space at the end of each line, though, instead of only highlighting the text?

It continues to irritate me as well.  Maybe an option to turn it off?

martin

bug#39360: Add :extend attribute to show-paren-mode expression face

[Clément Pit-Claudel]

On 2020-01-31 10:32, Eli Zaretskii wrote:
>> Unless people were complaining about the old behavior for this
>> particular face, I think that behavior was fine.

> IMO it was not fine, because Emacs was behaving differently from
> every other application out there which supports some form of
> highlighting text that spans several lines.

OK, good point, I never paid attention to that.

> The newline is inside the parentheses, but the empty space to the 
> right of the newline is not part of the text.  Why should it be 
> highlighted?

For the same reason that the region highlight goes to the edge of the window, I think.

>> Is there a reason to highlight one blank space at the end of each
>> line, though, instead of only highlighting the text?
> 
> That blank space stands for the newline, so if the newline has that 
> face, the space glyph at EOL gets highlighted to indicate that fact.

I see; I would expect that space to be visible only in whitespace-mode (in fact, when enabling whitespace-mode, there's now a character to indicate the line ending and a highlighted whitespace next to it)

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> Cc: 39360@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 31 Jan 2020 16:44:27 +0100
> 
>  > Is there a reason to highlight one blank space at the end of each line, though, instead of only highlighting the text?
> 
> It continues to irritate me as well.  Maybe an option to turn it off?

IMO, it's too early to discuss such an option: we don't yet know how
this feature will end up in Emacs 27 and beyond, because we didn't yet
have enough user feedback.  What we have now is similar to how other
programs behave, including highlighting of the space beyond EOL.  Does
it irritate you in those other programs as well?

bug#39360: Add :extend attribute to show-paren-mode expression face

[Stephen Berman]

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

On Fri, 31 Jan 2020 09:52:25 -0500 Clément Pit-Claudel <cpitclaudel@gmail.com> wrote:

> On 2020-01-31 02:38, Eli Zaretskii wrote:
>>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>>> Date: Thu, 30 Jan 2020 16:08:50 -0500
[...]
>>> I might have missed discussions about this; is there a reason why :extend t
>>> isn't the default?
>> 
>> Because the whole idea of introducing :extend was that almost all
>> faces don't need this attribute.
>
> I see.  The new default looks weird to me, but I'm sure I'll get used to it.

What also looks weird and infelicitous to me is that a nonextending
background color face on multiple lines shows up on the empty space at
the beginning of a line, i.e., it appears to extend into this space, see
attached screenshot.  I think this only happens with white space
characters, not with lines indented e.g. with wrap-prefix.  It would be
nice to treat all "empty" space the same in this respect.

Steve Berman


[-- Attachment #2: show-paren.png --]
[-- Type: image/png, Size: 22170 bytes --]

bug#39360: Add :extend attribute to show-paren-mode expression face

[Stephen Berman]

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

On Fri, 31 Jan 2020 09:52:25 -0500 Clément Pit-Claudel <cpitclaudel@gmail.com> wrote:

> On 2020-01-31 02:38, Eli Zaretskii wrote:
>>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>>> Date: Thu, 30 Jan 2020 16:08:50 -0500
[...]
>>> I might have missed discussions about this; is there a reason why :extend t
>>> isn't the default?
>> 
>> Because the whole idea of introducing :extend was that almost all
>> faces don't need this attribute.
>
> I see.  The new default looks weird to me, but I'm sure I'll get used to it.

What also looks weird and infelicitous to me is that a nonextending
background color face on multiple lines shows up on the empty space at
the beginning of a line, i.e., it appears to extend into this space, see
attached screenshot.  I think this only happens with white space
characters, not with lines indented e.g. with wrap-prefix.  It would be
nice to treat all "empty" space the same in this respect.

Steve Berman


[-- Attachment #2: show-paren.png --]
[-- Type: image/png, Size: 22170 bytes --]

bug#39360: Add :extend attribute to show-paren-mode expression face

[Clément Pit-Claudel]

On 2020-01-31 10:55, Eli Zaretskii wrote:
> What we have now is similar to how other
> programs behave, including highlighting of the space beyond EOL.  Does
> it irritate you in those other programs as well?

On my machine at least, none of vim, nano, or thunderbird highlights the EOL space.  

Firefox does highlight the end-of-line space on line wraps, but not when the newline is due to a paragraph change or an explicit <br /> in the source. LibreOffice and gedit highlight past the end of the line, stretching to the end of the window like Emacs does for the region.

If I highlight a region in LibreOffice using the highlighter tool to apply a background color, that color extends over one space to the right of the text when there's a line wrap (because the space over which the line is broken is before the break), but explicit newlines are not highlighted.

I couldn't find a program on my machine that behave like Emacs now does (highlighting explicit newlines).

Clément.

bug#39360: Add :extend attribute to show-paren-mode expression face

[martin rudalics]

 > IMO, it's too early to discuss such an option: we don't yet know how
 > this feature will end up in Emacs 27 and beyond, because we didn't yet
 > have enough user feedback.  What we have now is similar to how other
 > programs behave, including highlighting of the space beyond EOL.  Does
 > it irritate you in those other programs as well?

I've never seen another program highlighting the newline character the
way we do now.

martin

bug#39360: Add :extend attribute to show-paren-mode expression face

[martin rudalics]

 > What also looks weird and infelicitous to me is that a nonextending
 > background color face on multiple lines shows up on the empty space at
 > the beginning of a line, i.e., it appears to extend into this space, see
 > attached screenshot.  I think this only happens with white space
 > characters, not with lines indented e.g. with wrap-prefix.  It would be
 > nice to treat all "empty" space the same in this respect.

It would be nice to have that.  But I doubt that it can be done.

martin

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> Cc: 39360@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 31 Jan 2020 10:51:08 -0500
> 
> > The newline is inside the parentheses, but the empty space to the 
> > right of the newline is not part of the text.  Why should it be 
> > highlighted?
> 
> For the same reason that the region highlight goes to the edge of the window, I think.

We left the region as it was to avoid too strong outcry.  In general,
region should also not extend, IMO.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> From: Stephen Berman <stephen.berman@gmx.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  39360@debbugs.gnu.org
> Date: Fri, 31 Jan 2020 17:03:41 +0100
> 
> What also looks weird and infelicitous to me is that a nonextending
> background color face on multiple lines shows up on the empty space at
> the beginning of a line, i.e., it appears to extend into this space, see
> attached screenshot.  I think this only happens with white space
> characters, not with lines indented e.g. with wrap-prefix.  It would be
> nice to treat all "empty" space the same in this respect.

That space is not empty, there are characters there.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> Cc: 39360@debbugs.gnu.org
> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> Date: Fri, 31 Jan 2020 11:12:37 -0500
> 
> I couldn't find a program on my machine that behave like Emacs now does (highlighting explicit newlines).

MS Office does.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 31 Jan 2020 17:45:39 +0100
> Cc: 39360@debbugs.gnu.org
> 
>  > What also looks weird and infelicitous to me is that a nonextending
>  > background color face on multiple lines shows up on the empty space at
>  > the beginning of a line, i.e., it appears to extend into this space, see
>  > attached screenshot.  I think this only happens with white space
>  > characters, not with lines indented e.g. with wrap-prefix.  It would be
>  > nice to treat all "empty" space the same in this respect.
> 
> It would be nice to have that.  But I doubt that it can be done.

It can be done, it just isn't trivial.  The implementation could be
similar to trailing-whitespace (which isn't trivial, either).

But again, I'm not sure we should discuss these changes so soon.  The
face-extension feature is not out of the woods yet, IMO.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Dmitry Gutov]

On 31.01.2020 20:12, Eli Zaretskii wrote:
> MS Office does

Does it highlight just one space at the end of a line?

I just tried LibreOffice Writer, and it highlights the lines until the 
end of the "window", like like Emacs has been doing for decades.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> Date: Fri, 31 Jan 2020 19:12:19 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 39360@debbugs.gnu.org
> 
> > Cc: 39360@debbugs.gnu.org
> > From: Clément Pit-Claudel <cpitclaudel@gmail.com>
> > Date: Fri, 31 Jan 2020 11:12:37 -0500
> > 
> > I couldn't find a program on my machine that behave like Emacs now does (highlighting explicit newlines).
> 
> MS Office does.

Also, most text buffers in Emacs have explicit newlines at the end of
each line, so highlighting them is more like LibreOffice and Firefox
do.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> Cc: 39360@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 31 Jan 2020 20:19:52 +0300
> 
> On 31.01.2020 20:12, Eli Zaretskii wrote:
> > MS Office does
> 
> Does it highlight just one space at the end of a line?

Yes.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Clément Pit-Claudel]

On 2020-01-31 12:22, Eli Zaretskii wrote:
>> Date: Fri, 31 Jan 2020 19:12:19 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: 39360@debbugs.gnu.org
>>
>>> Cc: 39360@debbugs.gnu.org
>>> From: Clément Pit-Claudel <cpitclaudel@gmail.com>
>>> Date: Fri, 31 Jan 2020 11:12:37 -0500
>>>
>>> I couldn't find a program on my machine that behave like Emacs now does (highlighting explicit newlines).
>>
>> MS Office does.
> 
> Also, most text buffers in Emacs have explicit newlines at the end of
> each line, so highlighting them is more like LibreOffice and Firefox
> do.

As far as I can tell, Firefox highlights a space at the end of the line only if there is an actual space (ascii 32), but not if there's an explicit newline.  In other words, it highlights over continuation lines, but not over explicit newlines.

That is to say, we highlight this (with show-paren-mode and visual-line-mode, assuming the line is too long to fit in the window):

(test test test test test test test test test test test test test test test test test test test test test test test test test)

the same way as this:

(test test test test test test test test test test test test test test test
test test test test test test test test test test)

Whereas firefox highlights the end-of-line blank only in the first case, right where the line wraps.

Clément.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Dmitry Gutov]

On 31.01.2020 20:03, Eli Zaretskii wrote:
>>> The newline is inside the parentheses, but the empty space to the
>>> right of the newline is not part of the text.  Why should it be
>>> highlighted?
>> For the same reason that the region highlight goes to the edge of the window, I think.
> We left the region as it was to avoid too strong outcry.  In general,
> region should also not extend, IMO.

How about the reasons of consistency?

I'd be fine if region was not extended. But since we do extend it, why 
not make other face defaults in similar situations consistent with that?

bug#39360: Add :extend attribute to show-paren-mode expression face

[Juri Linkov]

>>>> The newline is inside the parentheses, but the empty space to the
>>>> right of the newline is not part of the text.  Why should it be
>>>> highlighted?
>>> For the same reason that the region highlight goes to the edge of the window, I think.
>> We left the region as it was to avoid too strong outcry.  In general,
>> region should also not extend, IMO.
>
> How about the reasons of consistency?
>
> I'd be fine if region was not extended. But since we do extend it, why not
> make other face defaults in similar situations consistent with that?

There are not too many faces that could be extended for the same
reason why the region face is extended.  From all faces that I used
only a few required customizing to add :extend t, namely:

mm-uu-extract
org-block
org-block-begin-line
xref-file-header (because I customized it to have a grey background color
                  like in diff-mode)

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> Cc: 39360@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 7 Feb 2020 03:27:15 +0300
> 
> > We left the region as it was to avoid too strong outcry.  In general,
> > region should also not extend, IMO.
> 
> How about the reasons of consistency?

It takes a back seat in this case.  (And in general, consistency is a
very weak argument in this case, because each face is used in
different circumstances.)

> I'd be fine if region was not extended. But since we do extend it, why 
> not make other face defaults in similar situations consistent with that?

Because, as explained previously, the fact that region extends is a
kind-of "retreat".  Doing that for many other faces would mean the
feature itself is useless, and probably should not have been coded in
the first place.  I don't think this is the case.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Clément Pit-Claudel
>  <cpitclaudel@gmail.com>,  39360@debbugs.gnu.org
> Date: Fri, 07 Feb 2020 02:40:17 +0200
> 
> There are not too many faces that could be extended for the same
> reason why the region face is extended.  From all faces that I used
> only a few required customizing to add :extend t, namely:
> 
> mm-uu-extract
> org-block
> org-block-begin-line

These are up to the developers of the corresponding packages.
Although IMNSHO, the last one doesn't sound to me like a face that
should be extended by default.

> xref-file-header (because I customized it to have a grey background color
>                   like in diff-mode)

If you customize that face, you could also make it extend while at
that, no?  IOW, what you say doesn't sound like an argument for making
it extend by default, since by default it specifies only the
foreground color and the weight.

bug#39360: Add :extend attribute to show-paren-mode expression face

[Lars Ingebrigtsen]

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

> I've attached a patch to add :extend the show-paren-mode face applied
> to expressions.  Is that the right way to return to the previous
> behavior?

[...]

>  (defface show-paren-match-expression
> -  '((t :inherit show-paren-match))
> +  '((t :inherit show-paren-match :extend t))

After playing with this a bit, I think the current behaviour looks
slightly more logical -- it'd be even better if show-paren-mode didn't
highlight the blank space at the start of the line, either, but
highlighting to the end of the line looks pretty overwhelming.

So I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no