GNU bug report logs - #63089
[PATCH] Display offscreen matched openparen

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 63089 in the body.
You can then email your comments to 63089 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Shynur Xie <one.last.kiss <at> outlook.com>
To: "bug-gnu-emacs <at> gnu.org" <bug-gnu-emacs <at> gnu.org>
Subject: [PATCH] Display offscreen matched openparen
Date: Wed, 26 Apr 2023 13:39:17 +0000

[Message part 1 (text/plain, inline)]

A line containing the matched open parenthesis will be displayed in
the echo area if that parenthesis is offscreen when the user types a
close parenthesis.

However, for example, the matched line may contain so many parentheses

```
(... (... ((((((((
...
...
```

that user will be confused by the text displayed in the echo area:

```
(... (... (((((
```

This patch changed a Lisp function `blink-matching-open' and rewrote a
Lisp function `blink-paren-open-paren-line-string' in order to help
user recognize the matched parenthesis more easily.

--
shynur

[0001-Display-offscreen-matched-openparen.patch (application/octet-stream, attachment)]

Message #8 received at 63089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Shynur Xie <one.last.kiss <at> outlook.com>
Cc: 63089 <at> debbugs.gnu.org
Subject: Re: bug#63089: [PATCH] Display offscreen matched openparen
Date: Fri, 28 Apr 2023 09:28:53 +0300

> From: Shynur Xie <one.last.kiss <at> outlook.com>
> Date: Wed, 26 Apr 2023 13:39:17 +0000
> 
> A line containing the matched open parenthesis will be displayed in
> the echo area if that parenthesis is offscreen when the user types a
> close parenthesis.
> 
> However, for example, the matched line may contain so many parentheses
> 
> ```
> (... (... ((((((((
> ...
> ...
> ```
> 
> that user will be confused by the text displayed in the echo area:
> 
> ```
> (... (... (((((
> ```
> 
> This patch changed a Lisp function `blink-matching-open' and rewrote a
> Lisp function `blink-paren-open-paren-line-string' in order to help
> user recognize the matched parenthesis more easily.

Thanks.

First, I think this should be an opt-in behavior, not the default.  We
remove properties from the text we show in the echo-area for a reason.
So please add a user option to enable this behavior.

Also, please don't unnecessarily introduce whitespace differences into
the code, like here:

> -       ((save-excursion (skip-chars-backward " \t") (not (bolp)))
> -        (setq regions (list (cons (line-beginning-position)
> -                                  (1+ pos)))))
> +       ;; Show what precedes the open in its line, if anything.
> +       ((save-excursion
> +          (skip-chars-backward " \t")
> +          (not (bolp)))

> +         #("Matches %s"
> +           ;; Make the following text (i.e., %s) prominent.
> +           0 7 (face (:weight light)))
> +         (blink-paren-open-paren-line-string blinkpos)))))))

This assume the font used in the echo-area must have a light variant
installed; if it doesn't, Emacs might select a different font and/or
the same "normal" weight.  So I'm not sure this is a good idea, in
general.  Why not use the 'shadow' face instead?

I'm not sure I understand why you use backticks in some parts of the
patch.  For example:

> -       ((save-excursion (skip-chars-backward " \t") (not (bolp)))
> -        (setq regions (list (cons (line-beginning-position)
> -                                  (1+ pos)))))
> +       ;; 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 `((,bol . ,(1+ pos)))
> +                openparen-idx (- pos bol))))

The original code didn't use backticks, so why do you need it in the
new version?

> +            (1+openparen-idx (1+ openparen-idx)))
                ^^^^^^^^^^^^^^^
This is a strange and confusing notation, please find a better name
for this variable.

Finally, this patch is too large to accept without copyright
assignment.  What is the status of your legal paperwork?

Message #11 received at 63089 <at> debbugs.gnu.org (full text, mbox):

From: Shynur Xie <one.last.kiss <at> outlook.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: "63089 <at> debbugs.gnu.org" <63089 <at> debbugs.gnu.org>
Subject: Re: bug#63089: [PATCH] Display offscreen matched openparen
Date: Fri, 28 Apr 2023 12:36:03 +0000

[Message part 1 (text/plain, inline)]

>    From: Eli Zaretskii
> Subject: bug#63089: [PATCH] Display offscreen matched openparen
>    Date: Fri, 28 Apr 2023 09:28:53 +0300
>      To: Shynur Xie
>
> please add a user option to enable this behavior.

I think it can be defcustomed in file "lisp/paren.el".  New version of
the patch is complete, please see the attached.

> don't unnecessarily introduce whitespace differences into the code

Got it.

> Why not use the 'shadow' face instead?

Thanks.  Have used the face 'shadow'.

> The original code didn't use backticks, so why do you need it in the
> new version?

My _original_ modification made some lines too long with `list' and
`cons', so I used all backticks in that function.  Since there's no
such problem in the subsequent modifications, I will use `list' and
`cons' if you think backticks are unnecessary (or weird).

>> +            (1+openparen-idx (1+ openparen-idx)))
>                ^^^^^^^^^^^^^^^
> This is a strange and confusing notation, please find a better name
> for this variable.

Have changed it to 'openparen-next-char-idx'.

> What is the status of your legal paperwork?

My assignment process with the FSF is complete.

Thanks again for your review of my patch!

--
shynur

[0001-Display-offscreen-matched-openparen.patch (application/octet-stream, attachment)]

Message #14 received at 63089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Shynur Xie <one.last.kiss <at> outlook.com>
Cc: 63089 <at> debbugs.gnu.org
Subject: Re: bug#63089: [PATCH] Display offscreen matched openparen
Date: Sat, 29 Apr 2023 14:05:56 +0300

> From: Shynur Xie <one.last.kiss <at> outlook.com>
> CC: "63089 <at> debbugs.gnu.org" <63089 <at> debbugs.gnu.org>
> Date: Fri, 28 Apr 2023 12:36:03 +0000
> 
> > The original code didn't use backticks, so why do you need it in the
> > new version?
> 
> My _original_ modification made some lines too long with `list' and
> `cons', so I used all backticks in that function.  Since there's no
> such problem in the subsequent modifications, I will use `list' and
> `cons' if you think backticks are unnecessary (or weird).

Backticks usually imply some run-time processing, which AFAIU here is
not required.

> > What is the status of your legal paperwork?
> 
> My assignment process with the FSF is complete.

Yes, I see it on file now.

> +(defcustom show-paren-openparen-face-in-message '(:foreground "green")
> +  "Set face for the matched offscreen openparen shown in the echo area.

 "Face for showing in the echo area matched open paren that is off-screen."

Also, I think the default value should be the default face, so that
the default behavior is not changed.

> +By default, the line containing the matched offscreen openparen is
> +shown in the echo area, where the openparen's face will be propertized
> +by this option."

"face will be propertized: is incorrect: we propertize text with a
face, we don't propertize the face.

> +  :type '(choice face sexp (const nil))
> +  :version "30.0")

This should be "30.1".  Emacs doesn't have NN.0 versions.

>  (defun blink-paren-open-paren-line-string (pos)
> -  "Return the line string that contains the openparen at POS."
> +  "Return the line string that contains the openparen at POS.
> +Remove the line string's properties but give the openparen a face."

This should include the name of the face, so that users could find it
easier.

Thanks.

Message #17 received at 63089 <at> debbugs.gnu.org (full text, mbox):

From: Shynur Xie <one.last.kiss <at> outlook.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: "63089 <at> debbugs.gnu.org" <63089 <at> debbugs.gnu.org>
Subject: Re: bug#63089: [PATCH] Display offscreen matched openparen
Date: Sun, 30 Apr 2023 10:09:18 +0000

[Message part 1 (text/plain, inline)]

>    From: Eli Zaretskii
> Subject: bug#63089
>    Date: Sat, 29 Apr 2023 14:05:56 +0300
>      To: Shynur Xie
>
> Backticks usually imply some run-time processing, which AFAIU here
> is not required.

Have replaced backtickes with `list's and `cons'es.

>> +  "Set face for the matched offscreen openparen shown in the echo area.
>
> "Face for showing in the echo area matched open paren that is off-screen."

Have changed to the latter.

> I think the default value should be the default face.

Its default value is nil now.  I struggled with whether `nil' (it can
be seen as an empty anonymous face) was better or `default'.

> "face will be propertized" is incorrect: we propertize text with a
> face, we don't propertize the face.

Have replaced
"the openparen's face will be propertized by this option"
with
"the openparen will be propertized with a face based on the value of
 this option".

> This should be "30.1".  Emacs doesn't have NN.0 versions.

Got it.

>> +  "Return the line string that contains the openparen at POS.
>> +Remove the line string's properties but give the openparen a face."
>
> This should include the name of the face,

The name of the face has been added:
"...... give the openparen a face based on the option
 `show-paren-openparen-face-in-message'."

______________________

New patch is attached.

Will keep your guidance in mind.  Thanks!

--
shynur

[0001-Display-offscreen-matched-openparen.patch (application/octet-stream, attachment)]

Message #20 received at 63089 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Shynur Xie <one.last.kiss <at> outlook.com>
Cc: 63089 <at> debbugs.gnu.org
Subject: Re: bug#63089: [PATCH] Display offscreen matched openparen
Date: Mon, 01 May 2023 16:17:44 +0300

> From: Shynur Xie <one.last.kiss <at> outlook.com>
> CC: "63089 <at> debbugs.gnu.org" <63089 <at> debbugs.gnu.org>
> Date: Sun, 30 Apr 2023 10:09:18 +0000
> 
> > I think the default value should be the default face.
> 
> Its default value is nil now.  I struggled with whether `nil' (it can
> be seen as an empty anonymous face) was better or `default'.
> 
> > "face will be propertized" is incorrect: we propertize text with a
> > face, we don't propertize the face.
> 
> Have replaced
> "the openparen's face will be propertized by this option"
> with
> "the openparen will be propertized with a face based on the value of
>  this option".
> 
> > This should be "30.1".  Emacs doesn't have NN.0 versions.
> 
> Got it.
> 
> >> +  "Return the line string that contains the openparen at POS.
> >> +Remove the line string's properties but give the openparen a face."
> >
> > This should include the name of the face,
> 
> The name of the face has been added:
> "...... give the openparen a face based on the option
>  `show-paren-openparen-face-in-message'."

Thanks.  Did you try "make bootstrap" with these changes?  The fact
that the new option is in paren.el but the code is in simple.el
worries me a bit: simple.el is preloaded before paren.el, so this
variable might not be known.  I think we should move the option to
simple,el, and rename it to blink-paren-SOMETHING.

Also, it is unusual to have a defcustom that names a face without a
corresponding defface that can be used to customize the face.  So I
think we should add a defface for the face used when the user option
is non-nil.

> Propertize the matched openparen displayed in the echo area in order to make
> it prominent; use light font for non-context characters (i.e., 'Matches').
> * lisp/simple.el (blink-matching-open): Set face shadow for 'Matches'.
> * lisp/simple.el (blink-paren-open-paren-line-string): Propertize the macthed
                                                                        ^^^^^^^
Typo there.

Also, the lines in the log message are too long, please make them at
most 70 column long.

Thanks.

Message #23 received at 63089 <at> debbugs.gnu.org (full text, mbox):

From: Shynur Xie <one.last.kiss <at> outlook.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: "63089 <at> debbugs.gnu.org" <63089 <at> debbugs.gnu.org>
Subject: Re: bug#63089: [PATCH] Display offscreen matched openparen
Date: Mon, 1 May 2023 17:52:44 +0000

[Message part 1 (text/plain, inline)]

>    From: Eli Zaretskii
> Subject: Re: bug#63089
>    Date: Mon, 01 May 2023 16:17:44 +0300
>      To: Shynur Xie
>
> I think we should move the option to simple.el, and rename it to
> blink-paren-SOMETHING.

There're several blink-matching-paren-* options in file <simple.el>,
but no option's name is blink-paren-*.  So I think perhaps it's better
to rename it to blink-matching-paren-highlight-offscreen.

> Also, it is unusual to have a defcustom that names a face without a
> corresponding defface that can be used to customize the face.  So I
> think we should add a defface for the face used when the user option
> is non-nil.

Following your instruction, I defface blink-matching-paren-offscreen.

> Also, the lines in the log message are too long, please make them at
> most 70 column long.

Got it.  It seems that <www.gnu.org/software/emacs/CONTRIBUTE> need to
be changed; it said "Limit lines in commit messages to 78 characters".

____________________

New patch is attached.  I have checked that there's no conflict
between the 2 newly introduced names and the original names.

--
shynur

[0001-Display-matched-offscreen-openparen.patch (application/octet-stream, attachment)]

Message #28 received at 63089-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Shynur Xie <one.last.kiss <at> outlook.com>
Cc: 63089-done <at> debbugs.gnu.org
Subject: Re: bug#63089: [PATCH] Display offscreen matched openparen
Date: Tue, 02 May 2023 21:38:02 +0300

> From: Shynur Xie <one.last.kiss <at> outlook.com>
> CC: "63089 <at> debbugs.gnu.org" <63089 <at> debbugs.gnu.org>
> Date: Mon, 1 May 2023 17:52:44 +0000
> 
> New patch is attached.  I have checked that there's no conflict
> between the 2 newly introduced names and the original names.

Thanks, installed on the master branch, and closing the bug.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.