Bug?

Bug?

[Po Lu]

A bootstrap leads to:

Loading indent...
Loading emacs-lisp/cl-generic...
Loading simple...
Dropping text-properties while making string `Matches %s' pure

Is this not a subtle bug?

Re: Bug?

[Mattias Engdegård]

3 maj 2023 kl. 03.26 skrev Po Lu <luangruo@yahoo.com>:
> 
> A bootstrap leads to:
> 
> Loading indent...
> Loading emacs-lisp/cl-generic...
> Loading simple...
> Dropping text-properties while making string `Matches %s' pure

Good catch, should work a little better now.

By the way, the standard blink-matching-paren-offscreen face (:foreground green) is terrible on a white background.
Don't we have an existing suitable face to inherit from? Would make it easier for theme authors, too.

Re: Bug?

[Mattias Engdegård]

> By the way, the standard blink-matching-paren-offscreen face (:foreground green) is terrible on a white background.
> Don't we have an existing suitable face to inherit from? Would make it easier for theme authors, too.

I also noticed that when the closing bracket is comprised of multiple characters, only the first character is high-lit using that face. For example, in shell-script-mode, when `esac` matches `case`, only the `c` in `case` is high-lit. Perhaps this new feature isn't quite finished yet.

Re: Bug?

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 3 May 2023 11:40:04 +0200
> Cc: emacs-devel <emacs-devel@gnu.org>, Shynur Xie <one.last.kiss@outlook.com>
> 
> By the way, the standard blink-matching-paren-offscreen face (:foreground green) is terrible on a white background.

Feel free to add a different default for bright background.

Thanks.

Re: Bug?

[Shynur Xie]

>    From: Po Lu
> Subject: Bug?
>    Date: Wed, 03 May 2023 09:26:20 +0800
>
> Is this not a subtle bug?

Thanks.

>    From: Mattias Engdegård
> Subject: Re: Bug?
>    Date: Wed, 3 May 2023 11:40:04 +0200
>
> Good catch, should work a little better now.

Thanks for fixing it.

> is terrible on a white background

Sorry for that.

> Don't we have an existing suitable face to inherit from?

Do you mean to inherit from those faces that set colors separately for
light and dark backgrounds?  I hadn't thought about it before; it's a
good idea, I think.

> I also noticed that when the closing bracket is comprised of
> multiple characters, only the first character is high-lit using that
> face.

That is the original behavior of `blink-matching-open' -- it only
highlights the first character.  (What you expected is provided by
`show-paren-mode'.)  It uses `forward-sexp' to move point to the first
character (in your example, it's `c`), but `forward-sexp' doesn't
report the length of this keyword (i.e., `case`).  I have no idea how
to implement highlighting for the entire matched keyword without
making many modifications.

BTW, if you want to give the function `blink-matching-open' the
semantic of highlighting the entire matched keyword, please change the
name of the function `blink-paren-open-paren-line-string' called by
`blink-matching-open', since `case` is not a parenthesis.  Thanks.

--
shynur

Re: Bug?

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 3 May 2023 12:12:09 +0200
> Cc: emacs-devel <emacs-devel@gnu.org>, Po Lu <luangruo@yahoo.com>
> 
> I also noticed that when the closing bracket is comprised of multiple characters, only the first character is high-lit using that face. For example, in shell-script-mode, when `esac` matches `case`, only the `c` in `case` is high-lit. Perhaps this new feature isn't quite finished yet.

Shynur, could you please fix this deficiency?

Re: Bug?

[Shynur Xie]

> From: Eli Zaretskii
> Subject: Re: Bug?
> Date: Wednesday, May 3, 2023 7:24 PM
>   To: Shynur Xie
>
> Shynur, could you please fix this deficiency?

I will try.

Re: Bug?

[Mattias Engdegård]

3 maj 2023 kl. 13.24 skrev Shynur Xie <one.last.kiss@outlook.com>:

>> Don't we have an existing suitable face to inherit from?
> 
> Do you mean to inherit from those faces that set colors separately for
> light and dark backgrounds?

Yes, and it's much easier for the user to customise a single "highlight" face than dozens of them. There is a face literally named "highlight"; would that do? If so, maybe we don't even need a new face at all.

If you really want a new face that uses green text by default, something like

 (defface blink-matching-paren-offscreen
-  '((t :foreground "green"))
+  '((((class color) (background light))
+     :background "yellow")
+    (((class color) (background dark))
+     :foreground "green"))

would be an improvement. Full saturated green text pretty much requires a dark background.

>> I also noticed that when the closing bracket is comprised of
>> multiple characters, only the first character is high-lit using that
>> face.
> 
> That is the original behavior of `blink-matching-open' -- it only
> highlights the first character.  (What you expected is provided by
> `show-paren-mode'.)

Ah yes. In Emacs, there are always layers upon layers.

Re: Bug?

[Juri Linkov]

>>> Don't we have an existing suitable face to inherit from?
>>
>> Do you mean to inherit from those faces that set colors separately for
>> light and dark backgrounds?
>
> Yes, and it's much easier for the user to customise a single
> "highlight" face than dozens of them.  There is a face literally named
> "highlight"; would that do?  If so, maybe we don't even need a new
> face at all.

When the 'highlight' face is used, then no need to remove font-lock
properties.  The highlighted paren with the green background
is clearly seen among font-lock faces.

Re: Bug?

[Shynur Xie]

Hi, Eli.

In my last email, I mentioned that highlighting only 1 character is
the original behavior of the function `blink-matching-open':

> From: Shynur Xie <one.last.kiss@outlook.com>
>   To: Mattias Engdegård
>   Cc: emacs-devel; Po Lu
>
>> I also noticed that when the closing bracket is comprised of
>> multiple characters, only the first character is high-lit using
>> that face.
>
> That is the original behavior of `blink-matching-open' -- it only
> highlights the first character.  (What you expected is provided by
> `show-paren-mode'.)  It uses `forward-sexp' to move point to the
> first character (in your example, it's `c`), but `forward-sexp'
> doesn't report the length of this keyword (i.e., `case`).

I also find that, the mode `show-paren-mode' (which highlights an
entire matched keyword) also emits a message "Matches %s" which will
override what `blink-matching-open' does, when that mode's own option
`show-paren-context-when-offscreen' is t.

In conclusion, before my patch was installed, by default,
`blink-matching-open' highlighted only 1 matched character when that
character is on-screen; `show-paren-mode' highlighted the entire
keyword.  But both of them didn't highlight the matched off-screen
openparen shown in the echo area.  After my patch was installed,
`blink-matching-open' highlights the 1st character when that character
is off-screen, as well as how it does originally when the character is
on-screen.

So I'm now wondering, would it be better to write the functionality
(highlighting an entire matched keyword when it is shown in the echo
area) into <lisp/paren.el> instead of <lisp/simple.el> (and make it a
part of `show-paren-mode')?  My opinion is, if `blink-matching-open'
highlights the entire matched keyword in the echo area, why doesn't it
also highlight an entire keyword that is on-screen?  But if it does,
`blink-matching-open' and `show-paren-mode' will conflict/duplicate in
function/code.  If we make that functionality a part of
`show-paren-mode', not only is behavioral consistency guaranteed in
all respects, but that functionality can simply reuse the logic/code
of `show-paren-mode'.

Any suggstion?  Thanks.

--
shynur

Re: Bug?

[Shynur Xie]

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

>    From: Eli Zaretskii
> Subject: Re: Bug?
>    Date: Wed, 03 May 2023 14:24:49 +0300
>
>> I also noticed that when the closing bracket is comprised of
>> multiple characters, only the first character is high-lit using
>> that face. For example, in shell-script-mode, when `esac` matches
>> `case`, only the `c` in `case` is high-lit. Perhaps this new
>> feature isn't quite finished yet.
>
> Shynur, could you please fix this deficiency?

Patch is attached.  To highlight the entire matched opening delimiter:
    (show-paren-mode)
    (setq blink-matching-paren-offscreen t
          show-paren-context-when-offscreen t)

I also changed the function `blink-paren-open-paren-line-string':
Originally, say here's a shell script
    case case
         ... <- many lines
         esac* <- cursor here
both `blink-matching-open' and `show-paren-mode' emitted "case c";
Now `show-paren-mode' instead emits "case case" if
`show-paren-context-when-offscreen' is t.

>    From: Mattias Engdegård
> Subject: Re: Bug?
>    Date: Wed, 3 May 2023 14:27:37 +0200
>
>>> Don't we have an existing suitable face to inherit from?
>>
>> Do you mean to inherit from those faces that set colors separately
>> for light and dark backgrounds?
>
> Yes, and it's much easier for the user to customise a single
> "highlight" face than dozens of them. There is a face literally
> named "highlight"; would that do? If so, maybe we don't even need a
> new face at all.
>
> If you really want a new face that uses green text by default,
> something like
>
>  (defface blink-matching-paren-offscreen

Thanks.  I finally chose the new face that you defined, because
"highlight" is difficult to distinguish against my dark background.

--
shynur

[-- Attachment #2: 0001-Show-paren-mode-highlights-entire-delimiter-in-the-e.patch --]
[-- Type: application/octet-stream, Size: 9464 bytes --]

From 80c8c26876f82c9c8ab20f47b28ded62f0b13990 Mon Sep 17 00:00:00 2001
From: Shynur <one.last.kiss@outlook.com>
Date: Fri, 5 May 2023 04:30:42 +0800
Subject: [PATCH] Show-paren-mode highlights entire delimiter in the echo area

Show-paren-mode now displays the context string containing the entire
matched delimiter and highlights it conditionally.
* lisp/simple.el (blink-matching-paren-offscreen): Set a color for
light background.
(blink-paren-open-paren-line-string): Can return the context string
containing the entire matched delimiter by specifying the value of the
argument DELIMITER-LENGTH.
* lisp/paren.el (show-paren-function): Can highlight the matched
delimiter in the echo area now.
* doc/emacs/programs.texi (Matching Parentheses): Add the description
of `blink-matching-paren-highlight-offscreen'.
; * doc/emacs/files.texi (Visiting Files): Fix typo.
---
 doc/emacs/files.texi    |  2 +-
 doc/emacs/programs.texi | 13 ++++++++++++-
 lisp/paren.el           | 11 +++++++----
 lisp/simple.el          | 41 ++++++++++++++++++++++++++++-------------
 4 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 74315a05bbd..46056b6d4fa 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -232,7 +232,7 @@ Visiting
 characters, Emacs visits all the files that match it.  (On
 case-insensitive filesystems, Emacs matches the wildcards disregarding
 the letter case.)  Wildcards include @samp{?}, @samp{*}, and
-@samp{[@dots{}]} sequences.  To enter the wild card @samp{?} in a file
+@samp{[@dots{}]} sequences.  To enter the wildcard @samp{?} in a file
 name in the minibuffer, you need to type @kbd{C-q ?}.  @xref{Quoted
 File Names}, for information on how to visit a file whose name
 actually contains wildcard characters.  You can disable the wildcard
diff --git a/doc/emacs/programs.texi b/doc/emacs/programs.texi
index 62df88a731e..3ee2a48de08 100644
--- a/doc/emacs/programs.texi
+++ b/doc/emacs/programs.texi
@@ -930,7 +930,8 @@ Matching
 @vindex blink-matching-paren
 @vindex blink-matching-paren-distance
 @vindex blink-matching-delay
-  Three variables control the display of matching parentheses:
+@vindex blink-matching-paren-highlight-offscreen
+  Four variables control the display of matching parentheses:
 
 @itemize @bullet
 @item
@@ -950,6 +951,16 @@ Matching
 back to search to find the matching opening delimiter.  If the match
 is not found in that distance, Emacs stops scanning and nothing is
 displayed.  The default is 102400.
+
+@item
+@code{blink-matching-paren-highlight-offscreen} says whether to use
+highlighting to indicate the location of the matched opening delimiter
+when it is off the screen and thus shown in the echo area.  The
+default is @code{nil}.  Note that only the 1st character of the
+delimiter is highlit; if you want to highlight the entire delimiter
+in the echo area, enable @code{show-paren-mode} (see below) and set
+both this option and @code{show-paren-context-when-offscreen} to
+@code{t}.
 @end itemize
 
 @cindex Show Paren mode
diff --git a/lisp/paren.el b/lisp/paren.el
index 4c91fd29490..c03605987d8 100644
--- a/lisp/paren.el
+++ b/lisp/paren.el
@@ -500,15 +500,15 @@ show-paren-function
             (move-overlay show-paren--overlay
                           there-beg there-end (current-buffer)))
           ;; If `show-paren-context-when-offscreen' is non-nil and
-          ;; point is at a closing paren, show the context around the
-          ;; opening paren.
+          ;; point is at a closing paren / delimiter, show the
+          ;; context around the opening paren / delimiter.
           (let ((openparen (min here-beg there-beg)))
             (when (and show-paren-context-when-offscreen
                        (not (eql show-paren--last-pos (point)))
                        (< there-beg here-beg)
                        (not (pos-visible-in-window-p openparen)))
               (let ((context (blink-paren-open-paren-line-string
-                              openparen))
+                              openparen (- there-end there-beg)))
                     (message-log-max nil))
                 (cond
                  ((and
@@ -518,7 +518,10 @@ show-paren-function
                  ((eq show-paren-context-when-offscreen 'overlay)
                   (show-paren--show-context-in-overlay context))
                  (show-paren-context-when-offscreen
-                  (minibuffer-message "Matches %s" context))))))
+                  (minibuffer-message
+                   "%s %s"
+                   (propertize "Matches" 'face 'shadow)
+                   context))))))
           (setq show-paren--last-pos (point))
           ;; Always set the overlay face, since it varies.
           (overlay-put show-paren--overlay 'priority show-paren-priority)
diff --git a/lisp/simple.el b/lisp/simple.el
index 58517dd81f9..4ba748c83d6 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9235,7 +9235,10 @@ blink-matching-paren-highlight-offscreen
   :group 'paren-blinking)
 
 (defface blink-matching-paren-offscreen
-  '((t :foreground "green"))
+  '((((class color) (background light))
+     :background "yellow")
+    (((class color) (background dark))
+     :foreground "green"))
   "Face for showing in the echo area matched open paren that is off-screen.
 This face is used only when `blink-matching-paren-highlight-offscreen'
 is non-nil."
@@ -9339,14 +9342,25 @@ blink-matching-open
                  (delete-overlay blink-matching--overlay)))))
        ((not show-paren-context-when-offscreen)
         (minibuffer-message
-         "%s%s"
-         (propertize "Matches " 'face 'shadow)
+         "%s %s"
+         (propertize "Matches" 'face 'shadow)
          (blink-paren-open-paren-line-string blinkpos)))))))
 
-(defun blink-paren-open-paren-line-string (pos)
+(defun blink-paren-open-paren-line-string (pos &optional delimiter-length)
   "Return the line string that contains the openparen at POS.
-Remove the line string's properties but give the openparen a distinct
-face if `blink-matching-paren-highlight-offscreen' is non-nil."
+
+Before returning, the line string's properties are removed,
+and then its substring of DELIMITER-LENGTH characters
+starting from openparen is propertized with a distinct face
+if `blink-matching-paren-highlight-offscreen' is non-nil."
+  ;; If the openparen is essentially the 1st charater of a matched opening
+  ;; delimiter, we may want the string to be returned to contain the
+  ;; entire delimiter.  DELIMITER-LENGTH charaters starting from openparen
+  ;; are guaranteed to be contained.
+  ;; By default, it is 1, assuming that what is matched is an openparen
+  ;; instead of a delimiter whose length >= 2.
+  (when (null delimiter-length)
+    (setq delimiter-length 1))
   (save-excursion
     (goto-char pos)
     ;; Capture the regions in terms of (beg . end) conses whose
@@ -9358,11 +9372,11 @@ blink-paren-open-paren-line-string
        ;; Show what precedes the open in its line, if anything.
        ((save-excursion (skip-chars-backward " \t") (not (bolp)))
         (let ((bol (line-beginning-position)))
-          (setq regions (list (cons bol (1+ pos)))
+          (setq regions (list (cons bol (+ pos delimiter-length)))
                 openparen-idx (- pos bol))))
        ;; Show what follows the open in its line, if anything.
        ((save-excursion
-          (forward-char 1)
+          (forward-char delimiter-length)
           (skip-chars-forward " \t")
           (not (eolp)))
         (setq regions (list (cons pos (line-end-position)))
@@ -9385,9 +9399,9 @@ blink-paren-open-paren-line-string
                                                       ;; (length "...")
                                                       3))
                                eol))
-                            (cons pos (1+ pos)))))
+                            (cons pos (+ pos delimiter-length)))))
        ;; There is nothing to show except the char itself.
-       (t (setq regions (list (cons pos (1+ pos)))
+       (t (setq regions (list (cons pos (+ pos delimiter-length)))
                 openparen-idx 0)))
       ;; Ensure we've font-locked the context region.
       (font-lock-ensure (caar regions) (cdar (last regions)))
@@ -9397,20 +9411,21 @@ blink-paren-open-paren-line-string
                 (buffer-substring (car region) (cdr region)))
               regions
               "..."))
-            (openparen-next-char-idx (1+ openparen-idx)))
+            (highlit-substring-end
+             (+ openparen-idx delimiter-length)))
         (setq line-string (substring-no-properties line-string))
         (concat
          (substring line-string
                     0 openparen-idx)
          (let ((matched-offscreen-openparen
                 (substring line-string
-                           openparen-idx openparen-next-char-idx)))
+                           openparen-idx highlit-substring-end)))
            (if blink-matching-paren-highlight-offscreen
                (propertize matched-offscreen-openparen
                            'face 'blink-matching-paren-offscreen)
              matched-offscreen-openparen))
          (substring line-string
-                    openparen-next-char-idx))))))
+                    highlit-substring-end))))))
 
 (defvar blink-paren-function 'blink-matching-open
   "Function called, if non-nil, whenever a close parenthesis is inserted.
-- 
2.34.1

Re: Bug?

[Eli Zaretskii]

> From: Shynur Xie <one.last.kiss@outlook.com>
> CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>, "luangruo@yahoo.com"
> 	<luangruo@yahoo.com>
> Date: Thu, 4 May 2023 20:57:36 +0000
> msip_labels:
> 
> >> I also noticed that when the closing bracket is comprised of
> >> multiple characters, only the first character is high-lit using
> >> that face. For example, in shell-script-mode, when `esac` matches
> >> `case`, only the `c` in `case` is high-lit. Perhaps this new
> >> feature isn't quite finished yet.
> >
> > Shynur, could you please fix this deficiency?
> 
> Patch is attached.  To highlight the entire matched opening delimiter:
>     (show-paren-mode)
>     (setq blink-matching-paren-offscreen t
>           show-paren-context-when-offscreen t)

Thanks, but I don't think I understand: didn't you tell that the
problem reported above is not a bug, but the expected and long-time
behavior of blink-matching-paren?  If so, what exactly does this patch
fix?

> I also changed the function `blink-paren-open-paren-line-string':
> Originally, say here's a shell script
>     case case
>          ... <- many lines
>          esac* <- cursor here
> both `blink-matching-open' and `show-paren-mode' emitted "case c";
> Now `show-paren-mode' instead emits "case case" if
> `show-paren-context-when-offscreen' is t.

This seems to be about show-paren-mode, not about
blink-matching-paren.  Those are different, though related, features.

Re: Bug?

[Shynur Xie]

>    From: Eli Zaretskii
> Subject: Re: Bug?
>    Date: Fri, 05 May 2023 07:41:14 +0300
>      To: Shynur Xie
>
> didn't you tell that the problem reported above is not a bug, but
> the expected and long-time behavior of blink-matching-paren?

Yes, it is not a bug to the original author of this function.  But now
`show-paren-mode' also uses this function, which leads to a bug, or at
least a counterintuitive behavior:
> From: Mattias Engdegård
> For example, in shell-script-mode, when `esac` matches `case`, only
> the `c` in `case` is high-lit.

_____________________________

Let me explain it in details.

At first (when emacs-version is 28.2), there was only the function
`blink-matching-open'.  Later, someone refactored this function, by
separating out a part of it, thus there is a function named
`blink-paren-open-paren-line-string' now.

Both of the above 2 functions are defined in <lisp/simple.el> to
implement the basic parenthesis matching feature (let's call it
`blink-matching` for now) mentioned in:
> (emacs)26.4.3
> Emacs _briefly_indicates_ the location of the matching opening
> delimiter, ... If it is not on the screen, Emacs displays some of
> the text near it in the echo area."

The goal of `blink-matching` is just to _briefly_indicate_ the
location of delimiter (such as '(' in Lisp and 'case' in Bash), so it
is reasonable to _treat_only_the_1st_character_of_a_delimiter_
specially -- `blink-matching` highlights only the 1st character (to
observe this, you need to disable `show-paren-mode' first) when the
matched delimiter is on-screen; when it's off-screen, only the 1st
character is made sure to be contained in the message that is shown in
the echo area.  For example, in `shell-script-mode', when off-screen:
    case case  <- off-screen
         ^
         ...   <- many empty lines
         esac* <- cursor here
only "case c" is shown in the echo area.  Thus my first patch can only
highlights "c" because "ase" is not contained at all.  So
> From: Mattias Engdegård
> I also noticed that when the closing bracket is comprised of
> multiple characters, only the first character is high-lit using that
> face.

_____________________

> From: Eli Zaretskii
> Shynur, could you please fix this deficiency?

Deficiency, yes, it is, and belongs to `show-paren-mode'.

Let's take a look at what `show-paren-mode' provides first:
> (emacs)26.4.3
> Show Paren mode is a minor mode that provides a _more_powerful_ kind
> of automatic matching.
So this mode treats the entire matched delimiter specially, instead of
only the 1st character.  For example, in `shell-script-mode', when
on-screen:
    case  <- on-screen
    ^^^^
    esac* <- cursor here
the entire "case" is highlit.

But when off-screen, if `show-paren-context-when-offscreen' (an option
of `show-paren-mode') is t, the job of showing the matched off-screen
delimiter in the echo area is taken by `show-paren-mode' instead of
`blink-matching`.

I notice that `show-paren-mode` does this job
_simply_by_invoking_`blink-paren-open-paren-line-string'_, which
assumes that only the 1st character of the matched delimiter needs to
be treated specially -- same example as the first one I mentioned:
    case case  <- off-screen
         ^
         ...   <- many empty lines
         esac* <- cursor here
"case c" is shown in the area.

> From: Eli Zaretskii
> This seems to be about show-paren-mode, not about
> blink-matching-paren.

About both, IMO.  They both use `blink-paren-open-paren-line-string'
but this function only guarantees the returned string containes the
1st character of a delimiter, which is suitable for `blink-matching`
but not for `show-paren-mode'.

_____________________

> From: Eli Zaretskii
> what exactly does this patch fix?

It fixes cases such as "case c".  Thus, if
`show-paren-context-when-offscreen' is t, "case case", which contains
the entire matched delimiter -- "case", will be shown in the echo
area.  And if `blink-matching-paren-highlight-offscreen' is also t,
    "Matches case case"
                  ^^^^
the entire delimiter, instead of only its 1st character,  will be
highlit.

--
shynur

Re: Bug?

[Mattias Engdegård]

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

4 maj 2023 kl. 22.57 skrev Shynur Xie <one.last.kiss@outlook.com>:

> I finally chose the new face that you defined, because
> "highlight" is difficult to distinguish against my dark background.

Maybe we should make the `highlight` face better at highlighting then?
On white background it seems to do alright. Comparison, using `highlight`:




It indeed seems that `highlight` on dark background is a bit too timid. What about, say, #006090 instead of darkolivegreen?


[-- Attachment #2.1: Type: text/html, Size: 1256 bytes --]

[-- Attachment #2.2: Skärmavbild 2023-05-06 kl. 12.24.13.png --]
[-- Type: image/png, Size: 9744 bytes --]

[-- Attachment #2.3: Skärmavbild 2023-05-06 kl. 12.24.22.png --]
[-- Type: image/png, Size: 9441 bytes --]

Re: Bug?

[Shynur Xie]

> From: Mattias Engdegård
> Date: Sat, 6 May 2023 12:44:57 +0200
>
> It indeed seems that `highlight` on dark background is a bit too
> timid. What about, say, #006090 instead of darkolivegreen?

Thanks.  But did you notice that all the faces defined in
<lisp/faces.el> use "pre-defined color names" instead of "#RRGGBB"?
I'm a little worried that if we use "#RRGGBB", it might be not
intuitive and not fit with the overall style of <lisp/faces.el>.

If you change the face `highlight`, I'll rewrite my patch to make
`blink-matching-paren-offscreen` inherit from it.

____________________

Eli, if you think my last patch helps, please don't install it yet,
because it defines a new face for `blink-matching-paren-offscreen` and
thus should be rewritten if Mattias changes face `highlight`.  Thanks.

By the way,

>    From: Eli Zaretskii
> Subject: Re: emacs-29 9b775ddc057 1/2
>    Date: Sat, 06 May 2023 16:22:12 +0300
>
> today, I've been at the keyboard, working on various Emacs issues
> for the last 7 hours without any breaks, and this is my weekend!

I appreciate the effort you're putting into resolving those issues,
but it's important to prioritize your own well-being too!  Perhaps
taking a break to recharge would be helpful.  Wish you all the best.

--
shynur

Re: Bug?

[Eli Zaretskii]

> From: Shynur Xie <one.last.kiss@outlook.com>
> CC: Eli Zaretskii <eliz@gnu.org>, "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Sat, 6 May 2023 14:58:09 +0000
> 
> > today, I've been at the keyboard, working on various Emacs issues
> > for the last 7 hours without any breaks, and this is my weekend!
> 
> I appreciate the effort you're putting into resolving those issues,
> but it's important to prioritize your own well-being too!  Perhaps
> taking a break to recharge would be helpful.  Wish you all the best.

Thanks, but taking a break will just mean I will need to work much
harder when I get back from the break.

Re: Bug?

[Mattias Engdegård]

6 maj 2023 kl. 16.58 skrev Shynur Xie <one.last.kiss@outlook.com>:

> But did you notice that all the faces defined in
> <lisp/faces.el> use "pre-defined color names" instead of "#RRGGBB"?
> I'm a little worried that if we use "#RRGGBB", it might be not
> intuitive and not fit with the overall style of <lisp/faces.el>.

I have no opinion on the matter. I suppose `darkskyblue4` is the closest. Which colour would you prefer?

Or for that matter, what about using the `show-paren-match` face instead? That's what it's there for, isn't it?

Re: Bug?

[Po Lu]

Mattias Engdegård <mattiase@acm.org> writes:

> 6 maj 2023 kl. 16.58 skrev Shynur Xie <one.last.kiss@outlook.com>:
>
>> But did you notice that all the faces defined in
>> <lisp/faces.el> use "pre-defined color names" instead of "#RRGGBB"?
>> I'm a little worried that if we use "#RRGGBB", it might be not
>> intuitive and not fit with the overall style of <lisp/faces.el>.
>
> I have no opinion on the matter. I suppose `darkskyblue4` is the
> closest. Which colour would you prefer?

Please try very hard to avoid using device or intensity specifications
for colors.  This is because many vendors of X servers perform color
calibration for graphics devices by editing the list of pre-defined X
colors.

In addition, using non-predefined colors means that you will be more
likely to allocate a color cell in the default colormap which no other
program can use.  While not as important as it used to be, excessive
color use is still something to keep in mind when writing programs.

Re: Bug?

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Shynur Xie <one.last.kiss@outlook.com>,  Eli Zaretskii <eliz@gnu.org>,
>  "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Sun, 07 May 2023 20:05:33 +0800
> 
> Mattias Engdegård <mattiase@acm.org> writes:
> 
> > 6 maj 2023 kl. 16.58 skrev Shynur Xie <one.last.kiss@outlook.com>:
> >
> >> But did you notice that all the faces defined in
> >> <lisp/faces.el> use "pre-defined color names" instead of "#RRGGBB"?
> >> I'm a little worried that if we use "#RRGGBB", it might be not
> >> intuitive and not fit with the overall style of <lisp/faces.el>.
> >
> > I have no opinion on the matter. I suppose `darkskyblue4` is the
> > closest. Which colour would you prefer?
> 
> Please try very hard to avoid using device or intensity specifications
> for colors.  This is because many vendors of X servers perform color
> calibration for graphics devices by editing the list of pre-defined X
> colors.
> 
> In addition, using non-predefined colors means that you will be more
> likely to allocate a color cell in the default colormap which no other
> program can use.  While not as important as it used to be, excessive
> color use is still something to keep in mind when writing programs.

Personally, I fail to understand why we are considering changes in
default faces for this minor issue.  We have enough faces from which
to inherit, faces whose definitions endured many years of use and
scrutiny.  Let's not over-engineer, OK?

Re: Bug?

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: Shynur Xie <one.last.kiss@outlook.com>,  Eli Zaretskii <eliz@gnu.org>,
>>  "emacs-devel@gnu.org" <emacs-devel@gnu.org>
>> Date: Sun, 07 May 2023 20:05:33 +0800
>> 
>> Mattias Engdegård <mattiase@acm.org> writes:
>> 
>> > 6 maj 2023 kl. 16.58 skrev Shynur Xie <one.last.kiss@outlook.com>:
>> >
>> >> But did you notice that all the faces defined in
>> >> <lisp/faces.el> use "pre-defined color names" instead of "#RRGGBB"?
>> >> I'm a little worried that if we use "#RRGGBB", it might be not
>> >> intuitive and not fit with the overall style of <lisp/faces.el>.
>> >
>> > I have no opinion on the matter. I suppose `darkskyblue4` is the
>> > closest. Which colour would you prefer?
>> 
>> Please try very hard to avoid using device or intensity specifications
>> for colors.  This is because many vendors of X servers perform color
>> calibration for graphics devices by editing the list of pre-defined X
>> colors.
>> 
>> In addition, using non-predefined colors means that you will be more
>> likely to allocate a color cell in the default colormap which no other
>> program can use.  While not as important as it used to be, excessive
>> color use is still something to keep in mind when writing programs.
>
> Personally, I fail to understand why we are considering changes in
> default faces for this minor issue.  We have enough faces from which
> to inherit, faces whose definitions endured many years of use and
> scrutiny.  Let's not over-engineer, OK?

I thought the discussion was about introducing a new face, this time
with a dangerous color.

Re: Bug?

[Shynur Xie]

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

> From: Eli Zaretskii
> Date: Sun, 07 May 2023 15:17:24 +0300
>
> Personally, I fail to understand why we are considering changes in
> default faces for this minor issue.

Let me briefly summarize the discussion between Mattias and me, to
save your time.

`blink-matching-paren-offscreen` has its own face defined by me, then
Mattias suggested a color for it when background is light, or just
inheriting from `highlight`.  I finally chose to use the suggested
color and wrote it into my new patch.  (The primary goal of this patch
is to fix the deficiency mentioned by Mattias.)

The rest of the discussion about `highlight` was just as you said:

> Let's not over-engineer, OK?

Okay, stop talking about these faces.

Mattias, let's put the discussion about faces aside now, and focus on
the problem you mentioned -- only "case c" is shown in the echo area
and only "c" is highlit, OK?  And feel free to change the face
`blink-matching-paren-offscreen` after this problem is solved.

Again, I attached that patch here.  Any opinion?

--
shynur

[-- Attachment #2: 0001-Show-paren-mode-highlights-entire-delimiter-in-the-e.patch --]
[-- Type: application/octet-stream, Size: 9464 bytes --]

From 80c8c26876f82c9c8ab20f47b28ded62f0b13990 Mon Sep 17 00:00:00 2001
From: Shynur <one.last.kiss@outlook.com>
Date: Fri, 5 May 2023 04:30:42 +0800
Subject: [PATCH] Show-paren-mode highlights entire delimiter in the echo area

Show-paren-mode now displays the context string containing the entire
matched delimiter and highlights it conditionally.
* lisp/simple.el (blink-matching-paren-offscreen): Set a color for
light background.
(blink-paren-open-paren-line-string): Can return the context string
containing the entire matched delimiter by specifying the value of the
argument DELIMITER-LENGTH.
* lisp/paren.el (show-paren-function): Can highlight the matched
delimiter in the echo area now.
* doc/emacs/programs.texi (Matching Parentheses): Add the description
of `blink-matching-paren-highlight-offscreen'.
; * doc/emacs/files.texi (Visiting Files): Fix typo.
---
 doc/emacs/files.texi    |  2 +-
 doc/emacs/programs.texi | 13 ++++++++++++-
 lisp/paren.el           | 11 +++++++----
 lisp/simple.el          | 41 ++++++++++++++++++++++++++++-------------
 4 files changed, 48 insertions(+), 19 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 74315a05bbd..46056b6d4fa 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -232,7 +232,7 @@ Visiting
 characters, Emacs visits all the files that match it.  (On
 case-insensitive filesystems, Emacs matches the wildcards disregarding
 the letter case.)  Wildcards include @samp{?}, @samp{*}, and
-@samp{[@dots{}]} sequences.  To enter the wild card @samp{?} in a file
+@samp{[@dots{}]} sequences.  To enter the wildcard @samp{?} in a file
 name in the minibuffer, you need to type @kbd{C-q ?}.  @xref{Quoted
 File Names}, for information on how to visit a file whose name
 actually contains wildcard characters.  You can disable the wildcard
diff --git a/doc/emacs/programs.texi b/doc/emacs/programs.texi
index 62df88a731e..3ee2a48de08 100644
--- a/doc/emacs/programs.texi
+++ b/doc/emacs/programs.texi
@@ -930,7 +930,8 @@ Matching
 @vindex blink-matching-paren
 @vindex blink-matching-paren-distance
 @vindex blink-matching-delay
-  Three variables control the display of matching parentheses:
+@vindex blink-matching-paren-highlight-offscreen
+  Four variables control the display of matching parentheses:
 
 @itemize @bullet
 @item
@@ -950,6 +951,16 @@ Matching
 back to search to find the matching opening delimiter.  If the match
 is not found in that distance, Emacs stops scanning and nothing is
 displayed.  The default is 102400.
+
+@item
+@code{blink-matching-paren-highlight-offscreen} says whether to use
+highlighting to indicate the location of the matched opening delimiter
+when it is off the screen and thus shown in the echo area.  The
+default is @code{nil}.  Note that only the 1st character of the
+delimiter is highlit; if you want to highlight the entire delimiter
+in the echo area, enable @code{show-paren-mode} (see below) and set
+both this option and @code{show-paren-context-when-offscreen} to
+@code{t}.
 @end itemize
 
 @cindex Show Paren mode
diff --git a/lisp/paren.el b/lisp/paren.el
index 4c91fd29490..c03605987d8 100644
--- a/lisp/paren.el
+++ b/lisp/paren.el
@@ -500,15 +500,15 @@ show-paren-function
             (move-overlay show-paren--overlay
                           there-beg there-end (current-buffer)))
           ;; If `show-paren-context-when-offscreen' is non-nil and
-          ;; point is at a closing paren, show the context around the
-          ;; opening paren.
+          ;; point is at a closing paren / delimiter, show the
+          ;; context around the opening paren / delimiter.
           (let ((openparen (min here-beg there-beg)))
             (when (and show-paren-context-when-offscreen
                        (not (eql show-paren--last-pos (point)))
                        (< there-beg here-beg)
                        (not (pos-visible-in-window-p openparen)))
               (let ((context (blink-paren-open-paren-line-string
-                              openparen))
+                              openparen (- there-end there-beg)))
                     (message-log-max nil))
                 (cond
                  ((and
@@ -518,7 +518,10 @@ show-paren-function
                  ((eq show-paren-context-when-offscreen 'overlay)
                   (show-paren--show-context-in-overlay context))
                  (show-paren-context-when-offscreen
-                  (minibuffer-message "Matches %s" context))))))
+                  (minibuffer-message
+                   "%s %s"
+                   (propertize "Matches" 'face 'shadow)
+                   context))))))
           (setq show-paren--last-pos (point))
           ;; Always set the overlay face, since it varies.
           (overlay-put show-paren--overlay 'priority show-paren-priority)
diff --git a/lisp/simple.el b/lisp/simple.el
index 58517dd81f9..4ba748c83d6 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9235,7 +9235,10 @@ blink-matching-paren-highlight-offscreen
   :group 'paren-blinking)
 
 (defface blink-matching-paren-offscreen
-  '((t :foreground "green"))
+  '((((class color) (background light))
+     :background "yellow")
+    (((class color) (background dark))
+     :foreground "green"))
   "Face for showing in the echo area matched open paren that is off-screen.
 This face is used only when `blink-matching-paren-highlight-offscreen'
 is non-nil."
@@ -9339,14 +9342,25 @@ blink-matching-open
                  (delete-overlay blink-matching--overlay)))))
        ((not show-paren-context-when-offscreen)
         (minibuffer-message
-         "%s%s"
-         (propertize "Matches " 'face 'shadow)
+         "%s %s"
+         (propertize "Matches" 'face 'shadow)
          (blink-paren-open-paren-line-string blinkpos)))))))
 
-(defun blink-paren-open-paren-line-string (pos)
+(defun blink-paren-open-paren-line-string (pos &optional delimiter-length)
   "Return the line string that contains the openparen at POS.
-Remove the line string's properties but give the openparen a distinct
-face if `blink-matching-paren-highlight-offscreen' is non-nil."
+
+Before returning, the line string's properties are removed,
+and then its substring of DELIMITER-LENGTH characters
+starting from openparen is propertized with a distinct face
+if `blink-matching-paren-highlight-offscreen' is non-nil."
+  ;; If the openparen is essentially the 1st charater of a matched opening
+  ;; delimiter, we may want the string to be returned to contain the
+  ;; entire delimiter.  DELIMITER-LENGTH charaters starting from openparen
+  ;; are guaranteed to be contained.
+  ;; By default, it is 1, assuming that what is matched is an openparen
+  ;; instead of a delimiter whose length >= 2.
+  (when (null delimiter-length)
+    (setq delimiter-length 1))
   (save-excursion
     (goto-char pos)
     ;; Capture the regions in terms of (beg . end) conses whose
@@ -9358,11 +9372,11 @@ blink-paren-open-paren-line-string
        ;; Show what precedes the open in its line, if anything.
        ((save-excursion (skip-chars-backward " \t") (not (bolp)))
         (let ((bol (line-beginning-position)))
-          (setq regions (list (cons bol (1+ pos)))
+          (setq regions (list (cons bol (+ pos delimiter-length)))
                 openparen-idx (- pos bol))))
        ;; Show what follows the open in its line, if anything.
        ((save-excursion
-          (forward-char 1)
+          (forward-char delimiter-length)
           (skip-chars-forward " \t")
           (not (eolp)))
         (setq regions (list (cons pos (line-end-position)))
@@ -9385,9 +9399,9 @@ blink-paren-open-paren-line-string
                                                       ;; (length "...")
                                                       3))
                                eol))
-                            (cons pos (1+ pos)))))
+                            (cons pos (+ pos delimiter-length)))))
        ;; There is nothing to show except the char itself.
-       (t (setq regions (list (cons pos (1+ pos)))
+       (t (setq regions (list (cons pos (+ pos delimiter-length)))
                 openparen-idx 0)))
       ;; Ensure we've font-locked the context region.
       (font-lock-ensure (caar regions) (cdar (last regions)))
@@ -9397,20 +9411,21 @@ blink-paren-open-paren-line-string
                 (buffer-substring (car region) (cdr region)))
               regions
               "..."))
-            (openparen-next-char-idx (1+ openparen-idx)))
+            (highlit-substring-end
+             (+ openparen-idx delimiter-length)))
         (setq line-string (substring-no-properties line-string))
         (concat
          (substring line-string
                     0 openparen-idx)
          (let ((matched-offscreen-openparen
                 (substring line-string
-                           openparen-idx openparen-next-char-idx)))
+                           openparen-idx highlit-substring-end)))
            (if blink-matching-paren-highlight-offscreen
                (propertize matched-offscreen-openparen
                            'face 'blink-matching-paren-offscreen)
              matched-offscreen-openparen))
          (substring line-string
-                    openparen-next-char-idx))))))
+                    highlit-substring-end))))))
 
 (defvar blink-paren-function 'blink-matching-open
   "Function called, if non-nil, whenever a close parenthesis is inserted.
-- 
2.34.1

Re: Bug?

[Mattias Engdegård]

7 maj 2023 kl. 15.36 skrev Shynur Xie <one.last.kiss@outlook.com>:

> Again, I attached that patch here.  Any opinion?

Did you consider using the face `show-paren-match` instead? It seems fine on dark background, too.

Re: Bug?

[Shynur Xie]

> From: Mattias Engdegård
> Date: Sun, 7 May 2023 15:51:39 +0200
>
> Did you consider using the face `show-paren-match` instead? It seems
> fine on dark background, too.

It is a good choice, and

> From: Shynur
> feel free to change the face `blink-matching-paren-offscreen` after
> this problem is solved.

______________

> From: Shynur
> Again, I attached that patch here.  Any opinion?

Eli, we've talked a lot about that patch and what it fixes.  In case
you forget our conversations, I listed them here:

Mattias:<lists.gnu.org/archive/html/emacs-devel/2023-05/msg00030.html>
     Me:<lists.gnu.org/archive/html/emacs-devel/2023-05/msg00100.html>
    You:<lists.gnu.org/archive/html/emacs-devel/2023-05/msg00109.html>
     Me:<lists.gnu.org/archive/html/emacs-devel/2023-05/msg00144.html>

--
shynur

Re: Bug?

[Eli Zaretskii]

> From: Shynur Xie <one.last.kiss@outlook.com>
> CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Sun, 7 May 2023 14:42:50 +0000
> 
> Eli, we've talked a lot about that patch and what it fixes.  In case
> you forget our conversations, I listed them here:
> 
> Mattias:<lists.gnu.org/archive/html/emacs-devel/2023-05/msg00030.html>
>      Me:<lists.gnu.org/archive/html/emacs-devel/2023-05/msg00100.html>
>     You:<lists.gnu.org/archive/html/emacs-devel/2023-05/msg00109.html>
>      Me:<lists.gnu.org/archive/html/emacs-devel/2023-05/msg00144.html>

I'll get to that eventually.