GNU bug report logs - #66771
[PATCH] Throw out fold spec after a single failure

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 66771 in the body.
You can then email your comments to 66771 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: Tony Zorman <soliditsallgood <at> mailbox.org>
To: bug-auctex <at> gnu.org
Subject: [PATCH] Throw out fold spec after a single failure
Date: Fri, 27 Oct 2023 08:35:03 +0200

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

Hi,

Right now, TeX-fold-expand-spec does not abort its expansion of the
current spec if *any* of the expansions succeeded. This can lead to
some unexpected behaviour. For example, given

    (setopt TeX-fold-math-spec-list
            '(("[1] of {1}||{1}" ("cite"))))

expanding `\cite{this}' would result in the overlay displaying
`… of this' instead of just `this', because one of the arguments (the
mandatory one) succeeded. This is probably not what the user intended.
Instead, in order for the spec to be chosen, *all* arguments should
succeed.

The attached patch does this.

One thing that seems related is that the function claims that it also
matches arguments of the form `(<number>)', but the regular expression
that's used

    "\\([[{<]\\)\\([1-9]\\)\\([]}>]\\)"

does not contain any matches for simple parentheses. I don't know
whether I'm missing something here, so I didn't change this.

Best,
  Tony

[0001-Throw-out-fold-spec-after-a-single-failure.patch (text/x-patch, inline)]

From 676a43071c2f3f3d6e11fd200ba1bcb321359b51 Mon Sep 17 00:00:00 2001
From: Tony Zorman <soliditsallgood <at> mailbox.org>
Date: Thu, 26 Oct 2023 21:26:27 +0200
Subject: [PATCH] Throw out fold spec after a single failure
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* tex-fold.el (TeX-fold-expand-spec): So far, TeX-fold-expand-spec did
not abort its expansion of the current spec if *any* of the expansions
succeeded.  This can lead to some unexpected behaviour.  For example,
given

  (setopt TeX-fold-math-spec-list
          '(("[1] of {1}||{1}" ("cite"))))

expanding `\cite{this}' would result in the overlay displaying
`… of this' instead of just `this', because one of the arguments
(the mandatory one) succeeded.  This is probably not what the user
intended.  Instead, in order for the spec to be chosen, *all
arguments should succeed.
---
 tex-fold.el | 54 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/tex-fold.el b/tex-fold.el
index e3857460..173e9634 100644
--- a/tex-fold.el
+++ b/tex-fold.el
@@ -741,36 +741,30 @@ Return non-nil if a removal happened, nil otherwise."
   "Expand instances of {<num>}, [<num>], <<num>>, and (<num>).
 Replace them with the respective macro argument."
   (let ((spec-list (split-string spec "||"))
-        (delims '((?\{ . ?\}) (?\[ . ?\]) (?< . ?>) (?\( . ?\))))
-        index success)
-    (catch 'success
-      ;; Iterate over alternatives.
-      (dolist (elt spec-list)
-        (setq spec elt
-              index nil)
-        ;; Find and expand every placeholder.
-        (while (and (string-match "\\([[{<]\\)\\([1-9]\\)\\([]}>]\\)" elt index)
-                    ;; Does the closing delim match the opening one?
-                    (string-equal
-                     (match-string 3 elt)
-                     (char-to-string
-                      (cdr (assq (string-to-char (match-string 1 elt))
-                                 delims)))))
-          (setq index (match-end 0))
-          (let ((arg (car (save-match-data
-                            ;; Get the argument.
-                            (TeX-fold-macro-nth-arg
-                             (string-to-number (match-string 2 elt))
-                             ov-start ov-end
-                             (assoc (string-to-char (match-string 1 elt))
-                                    delims))))))
-            (when arg (setq success t))
-            ;; Replace the placeholder in the string.
-            (setq elt (replace-match (or arg TeX-fold-ellipsis) nil t elt)
-                  index (+ index (- (length elt) (length spec)))
-                  spec elt)))
-        (when success (throw 'success nil))))
-    spec))
+        (delims '((?\{ . ?\}) (?\[ . ?\]) (?< . ?>) (?\( . ?\)))))
+    (cl-labels
+        ((expand (spec &optional index)
+           ;; If there is something to replace and the closing delimiter
+           ;; matches the opening one…
+           (if-let (((string-match "\\([[{<]\\)\\([1-9]\\)\\([]}>]\\)" spec index))
+                    (open (string-to-char (match-string 1 spec)))
+                    (num (string-to-number (match-string 2 spec)))
+                    (close (string-to-char (match-string 3 spec)))
+                    ((equal close (cdr (assoc open delims)))))
+               ;; … then replace it and move on.  Otherwise, it must have been
+               ;; a spurious spec, so abort.
+               (when-let ((arg (car (save-match-data
+                                      (TeX-fold-macro-nth-arg
+                                       num ov-start ov-end (assoc open delims)))))
+                          (spec* (replace-match arg nil t spec)))
+                 (expand spec*
+                         (+ (match-end 0) (- (length spec*) (length spec)))))
+             ;; Nothing to replace: return the (completed) spec.
+             spec)))
+      (or (cl-loop for elt in spec-list
+                   do (when-let (expanded (expand elt))
+                        (cl-return expanded)))
+          TeX-fold-ellipsis))))
 
 (defun TeX-fold-hide-item (ov)
   "Hide a single macro or environment.
-- 
2.42.0

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

-- 
Tony Zorman | https://tony-zorman.com/

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

From: Ikumi Keita <ikumi <at> ikumi.que.jp>
To: Tony Zorman <soliditsallgood <at> mailbox.org>
Cc: 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Wed, 08 Nov 2023 18:43:59 +0900

Hi Tony, sorry for late response.

>>>>> Tony Zorman via bug-auctex via Bug reporting list for AUCTeX <bug-auctex <at> gnu.org> writes:
> Right now, TeX-fold-expand-spec does not abort its expansion of the
> current spec if *any* of the expansions succeeded. This can lead to
> some unexpected behaviour. For example, given

>     (setopt TeX-fold-math-spec-list
>             '(("[1] of {1}||{1}" ("cite"))))

> expanding `\cite{this}' would result in the overlay displaying
> `… of this' instead of just `this', because one of the arguments (the
> mandatory one) succeeded. This is probably not what the user intended.
> Instead, in order for the spec to be chosen, *all* arguments should
> succeed.

> The attached patch does this.

Thank you for proposal. I think that AUCTeX can accept it and it
requires copyright assignment. Have you signed FSF copyright assignment
form before? If you haven't and want to have your proposal incorporated
into AUCTeX, please follow this instruction:
https://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future

> One thing that seems related is that the function claims that it also
> matches arguments of the form `(<number>)', but the regular expression
> that's used

>     "\\([[{<]\\)\\([1-9]\\)\\([]}>]\\)"

> does not contain any matches for simple parentheses. I don't know
> whether I'm missing something here, so I didn't change this.

It seems to me that it's just a bug. I think that tex-fold.el should be
corrected to include "(" and ")" in the regexp first, and your proposed
change should follow it. I'll do the former soon.

Regards,
Ikumi Keita
#StandWithUkraine #StopWarInUkraine

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

From: Tony Zorman <soliditsallgood <at> mailbox.org>
To: Ikumi Keita <ikumi <at> ikumi.que.jp>
Cc: 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Thu, 09 Nov 2023 21:02:36 +0100

Hi Ikumi,

On Wed, Nov 08 2023 18:43, Ikumi Keita wrote:
> Hi Tony, sorry for late response.

no worries; thanks for giving this a look!

> Thank you for proposal. I think that AUCTeX can accept it and it
> requires copyright assignment. Have you signed FSF copyright assignment
> form before? If you haven't and want to have your proposal incorporated
> into AUCTeX, please follow this instruction:
> https://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-assign.future

I was almost going to say yes, but turns out I haven't! I've now signed
the papers and sent them back.

>> One thing that seems related is that the function claims that it also
>> matches arguments of the form `(<number>)', but the regular expression
>> that's used
>
>>     "\\([[{<]\\)\\([1-9]\\)\\([]}>]\\)"
>
>> does not contain any matches for simple parentheses. I don't know
>> whether I'm missing something here, so I didn't change this.
>
> It seems to me that it's just a bug. I think that tex-fold.el should be
> corrected to include "(" and ")" in the regexp first, and your proposed
> change should follow it. I'll do the former soon.

Sounds great, thanks! I will post an updated patch based on that as soon
as I have heard back from the FSF regarding the copyright assignment.

  Tony

-- 
Tony Zorman | https://tony-zorman.com/

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

From: Ikumi Keita <ikumi <at> ikumi.que.jp>
To: Tony Zorman <soliditsallgood <at> mailbox.org>
Cc: 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Fri, 10 Nov 2023 21:10:22 +0900

Hi Tony,

>>>>> Tony Zorman <soliditsallgood <at> mailbox.org> writes:
>> It seems to me that it's just a bug. I think that tex-fold.el should be
>> corrected to include "(" and ")" in the regexp first, and your proposed
>> change should follow it. I'll do the former soon.

> Sounds great, thanks! I will post an updated patch based on that as soon
> as I have heard back from the FSF regarding the copyright assignment.

Now I pushed the fix to the git repo. I'll wait for your feedback.

Best regards,
Ikumi Keita
#StandWithUkraine #StopWarInUkraine

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

From: Tony Zorman <soliditsallgood <at> mailbox.org>
To: Ikumi Keita <ikumi <at> ikumi.que.jp>
Cc: 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Fri, 15 Dec 2023 06:56:17 +0100

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

Hi Ikumi,

On Fri, Nov 10 2023 21:10, Ikumi Keita wrote:
> I'll wait for your feedback.

I've heard back from the FSF: everything is signed now, and we should be
good to go! Attached you can find the patch again, rebased to include
commit 2a959a1d43b3f38cf4845b071679a7978e36d95a.

Thank you for your patience!
Tony

[0001-Throw-out-fold-spec-after-a-single-failure.patch (text/x-patch, inline)]

From c0840aab5e791488bae7cdae1607710993ccb13e Mon Sep 17 00:00:00 2001
From: Tony Zorman <soliditsallgood <at> mailbox.org>
Date: Thu, 26 Oct 2023 21:26:27 +0200
Subject: [PATCH] Throw out fold spec after a single failure
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* tex-fold.el (TeX-fold-expand-spec): So far, TeX-fold-expand-spec did
not abort its expansion of the current spec if *any* of the expansions
succeeded.  This can lead to some unexpected behaviour.  For example,
given

  (setopt TeX-fold-math-spec-list
          '(("[1] of {1}||{1}" ("cite"))))

expanding `\cite{this}' would result in the overlay displaying
`… of this' instead of just `this', because one of the arguments
(the mandatory one) succeeded.  This is probably not what the user
intended.  Instead, in order for the spec to be chosen, *all
arguments should succeed.
---
 tex-fold.el | 56 ++++++++++++++++++++++++-----------------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/tex-fold.el b/tex-fold.el
index c6e9f1ca..d4dacc16 100644
--- a/tex-fold.el
+++ b/tex-fold.el
@@ -741,37 +741,31 @@ Return non-nil if a removal happened, nil otherwise."
   "Expand instances of {<num>}, [<num>], <<num>>, and (<num>).
 Replace them with the respective macro argument."
   (let ((spec-list (split-string spec "||"))
-        (delims '((?\{ . ?\}) (?\[ . ?\]) (?< . ?>) (?\( . ?\))))
-        index success)
-    (catch 'success
-      ;; Iterate over alternatives.
-      (dolist (elt spec-list)
-        (setq spec elt
-              index nil)
-        ;; Find and expand every placeholder.
-        (while (and (string-match "\\([[{<(]\\)\\([1-9]\\)\\([]}>)]\\)"
-                                  elt index)
-                    ;; Does the closing delim match the opening one?
-                    (string-equal
-                     (match-string 3 elt)
-                     (char-to-string
-                      (cdr (assq (string-to-char (match-string 1 elt))
-                                 delims)))))
-          (setq index (match-end 0))
-          (let ((arg (car (save-match-data
-                            ;; Get the argument.
-                            (TeX-fold-macro-nth-arg
-                             (string-to-number (match-string 2 elt))
-                             ov-start ov-end
-                             (assoc (string-to-char (match-string 1 elt))
-                                    delims))))))
-            (when arg (setq success t))
-            ;; Replace the placeholder in the string.
-            (setq elt (replace-match (or arg TeX-fold-ellipsis) nil t elt)
-                  index (+ index (- (length elt) (length spec)))
-                  spec elt)))
-        (when success (throw 'success nil))))
-    spec))
+        (delims '((?\{ . ?\}) (?\[ . ?\]) (?< . ?>) (?\( . ?\)))))
+    (cl-labels
+        ((expand (spec &optional index)
+           ;; If there is something to replace and the closing delimiter
+           ;; matches the opening one…
+           (if-let (((string-match "\\([[{<(]\\)\\([1-9]\\)\\([]}>)]\\)"
+                                   elt index))
+                    (open (string-to-char (match-string 1 spec)))
+                    (num (string-to-number (match-string 2 spec)))
+                    (close (string-to-char (match-string 3 spec)))
+                    ((equal close (cdr (assoc open delims)))))
+               ;; … then replace it and move on.  Otherwise, it must have been
+               ;; a spurious spec, so abort.
+               (when-let ((arg (car (save-match-data
+                                      (TeX-fold-macro-nth-arg
+                                       num ov-start ov-end (assoc open delims)))))
+                          (spec* (replace-match arg nil t spec)))
+                 (expand spec*
+                         (+ (match-end 0) (- (length spec*) (length spec)))))
+             ;; Nothing to replace: return the (completed) spec.
+             spec)))
+      (or (cl-loop for elt in spec-list
+                   do (when-let (expanded (expand elt))
+                        (cl-return expanded)))
+          TeX-fold-ellipsis))))
 
 (defun TeX-fold-hide-item (ov)
   "Hide a single macro or environment.
-- 
2.42.0

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

-- 
Tony Zorman | https://tony-zorman.com/

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

From: Ikumi Keita <ikumi <at> ikumi.que.jp>
To: Tony Zorman <soliditsallgood <at> mailbox.org>
Cc: 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Fri, 15 Dec 2023 20:43:17 +0900

Hi Tony,

>>>>> Tony Zorman <soliditsallgood <at> mailbox.org> writes:
> I've heard back from the FSF: everything is signed now, and we should
> be good to go! Attached you can find the patch again, rebased to
> include commit 2a959a1d43b3f38cf4845b071679a7978e36d95a.

> Thank you for your patience!
> Tony

Thanks! I've installed your proposal in the git repository. I'll close
this bug.

Regards,
Ikumi Keita
#StandWithUkraine #StopWarInUkraine
#Gaza #StopMassiveKilling #CeasefireNOW

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

From: Arash Esbati <arash <at> gnu.org>
To: Ikumi Keita <ikumi <at> ikumi.que.jp>
Cc: Tony Zorman <soliditsallgood <at> mailbox.org>, 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Sun, 17 Dec 2023 19:11:15 +0100

Ikumi Keita <ikumi <at> ikumi.que.jp> writes:

>>>>>> Tony Zorman <soliditsallgood <at> mailbox.org> writes:
>> I've heard back from the FSF: everything is signed now, and we should
>> be good to go! Attached you can find the patch again, rebased to
>> include commit 2a959a1d43b3f38cf4845b071679a7978e36d95a.
>
>> Thank you for your patience!
>> Tony
>
> Thanks! I've installed your proposal in the git repository. I'll close
> this bug.

Thanks for this fix.  I think this change now produces:

  In TeX-fold-expand-spec:
  tex-fold.el:750:36: Warning: reference to free variable ‘elt’

Can you please have a look and silence this?

TIA.  Best, Arash

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

From: Tony Zorman <soliditsallgood <at> mailbox.org>
To: Arash Esbati <arash <at> gnu.org>, Ikumi Keita <ikumi <at> ikumi.que.jp>
Cc: 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Sun, 17 Dec 2023 20:18:41 +0100

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

On Sun, Dec 17 2023 19:11, Arash Esbati wrote:
> Thanks for this fix.  I think this change now produces:
>
>   In TeX-fold-expand-spec:
>   tex-fold.el:750:36: Warning: reference to free variable ‘elt’
>
> Can you please have a look and silence this?

Oh, how embarrassing! Sorry about that, it seems like rebasing was too
smart in how it handled the conflicts. Attached is a patch that should
fix this.

Best,
  Tony

[0001-tex-fold.el-TeX-fold-expand-spec-Fix-reference-to-fr.patch (text/x-patch, inline)]

From 6f36aed879b9b2b9bf8821749eb44d9f56d85dc6 Mon Sep 17 00:00:00 2001
From: Tony Zorman <soliditsallgood <at> mailbox.org>
Date: Sun, 17 Dec 2023 20:13:45 +0100
Subject: [PATCH] ; * tex-fold.el (TeX-fold-expand-spec): Fix reference to free
 variable

---
 tex-fold.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tex-fold.el b/tex-fold.el
index d4dacc16..0952bccc 100644
--- a/tex-fold.el
+++ b/tex-fold.el
@@ -747,7 +747,7 @@ Replace them with the respective macro argument."
            ;; If there is something to replace and the closing delimiter
            ;; matches the opening one…
            (if-let (((string-match "\\([[{<(]\\)\\([1-9]\\)\\([]}>)]\\)"
-                                   elt index))
+                                   spec index))
                     (open (string-to-char (match-string 1 spec)))
                     (num (string-to-number (match-string 2 spec)))
                     (close (string-to-char (match-string 3 spec)))
-- 
2.42.0

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

-- 
Tony Zorman | https://tony-zorman.com/

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

From: Arash Esbati <arash <at> gnu.org>
To: Tony Zorman <soliditsallgood <at> mailbox.org>
Cc: Ikumi Keita <ikumi <at> ikumi.que.jp>, 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Mon, 18 Dec 2023 05:58:11 +0100

Tony Zorman <soliditsallgood <at> mailbox.org> writes:

> Oh, how embarrassing! Sorry about that, it seems like rebasing was too
> smart in how it handled the conflicts. Attached is a patch that should
> fix this.

Thanks for your swift reply.  I slightly modified the log and installed
your change.

Best, Arash

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

From: Ikumi Keita <ikumi <at> ikumi.que.jp>
To: Arash Esbati <arash <at> gnu.org>
Cc: Tony Zorman <soliditsallgood <at> mailbox.org>, 66771 <at> debbugs.gnu.org
Subject: Re: bug#66771: [PATCH] Throw out fold spec after a single failure
Date: Mon, 18 Dec 2023 17:12:47 +0900

>>>>> Arash Esbati <arash <at> gnu.org> writes:
> Tony Zorman <soliditsallgood <at> mailbox.org> writes:
>> Oh, how embarrassing! Sorry about that, it seems like rebasing was too
>> smart in how it handled the conflicts. Attached is a patch that should
>> fix this.

> Thanks for your swift reply.  I slightly modified the log and installed
> your change.

Sorry for my overlooking the warning 😿 Thank you both!

Regards,
Ikumi Keita
#StandWithUkraine #StopWarInUkraine
#Gaza #StopMassiveKilling #CeasefireNOW

Previous Next

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