bug#39555: [PATCH] Allow tempo-define-template to reassign tags to new templates

bug#39555: [PATCH] Allow tempo-define-template to reassign tags to new templates

[Federico Tedin]

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

(This may apply as a bug and as a wishlist item at the same time - not sure)

Two problems I've found using tempo.el are:

1) If I'm using templates + tags (with `tempo-complete-tag') in a buffer, and I
define new templates + tags from another buffer, I can't immediately use
them in the original buffer (I have to do M-: (setq
tempo-dirty-collection t) ).

2) If I want to assign a different template to an already existing tag,
I have to remove the tag manually from the tags collection.

These two problems can become a bit annoying specially when
writing/trying out new templates with tags. For someone learning about
tempo.el for the first time, I can imagine they could be a problem as
well. To fix these, I'm attaching a patch with some changes.

In `tempo-invalidate-collection', I decided to loop over every buffer to
check if `tempo-dirty-collection' has a local value, and only set it to
t if it does. I am not sure if this is the ideal way of setting a
buffer-local variable to a certain value for all buffers that have bound
a local value to it. Feedback is welcome.

- Fede


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 5201 bytes --]

From d5495b81919b4dbe48a7d81f4eb89bb5d4d4a45b Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Mon, 10 Feb 2020 23:18:16 +0100
Subject: [PATCH 1/1] Allow tempo-define-template to reassign tags to new
 templates

* lisp/tempo.el (tempo-define-template): Update documentation string
to mention that existing tags can be reassigned new templates.
(tempo-add-tag): Allow reassigning tags to new templates.
Additionally, invalidate tag collections in all buffers if the global
tags list is being modified.
(tempo-invalidate-collection): Allow invalidating tag collections in
all buffers at the same time.
* test/lisp/tempo-tests.el (tempo-define-tag-globally-test): Add a
test to check that new templates plus tags can be defined from any
buffer and then immediately used in other buffers.
(tempo-overwrite-tag-test): Add a test to check that tags can be
reassigned templates.
---
 lisp/tempo.el            | 31 +++++++++++++++++++++++--------
 test/lisp/tempo-tests.el | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 8 deletions(-)

diff --git a/lisp/tempo.el b/lisp/tempo.el
index 9de5ac66c7..3d19fc135b 100644
--- a/lisp/tempo.el
+++ b/lisp/tempo.el
@@ -220,7 +220,9 @@ tempo-define-template
 DOCUMENTATION is the documentation string for the insertion command
 created, and TAGLIST (a symbol) is the tag list that TAG (if provided)
 should be added to.  If TAGLIST is nil and TAG is non-nil, TAG is
-added to `tempo-tags'.
+added to `tempo-tags'.  If TAG already corresponds to a template in
+the tag list, modify the list so that TAG now corresponds to the newly
+defined template.
 
 The elements in ELEMENTS can be of several types:
 
@@ -579,14 +581,20 @@ tempo-backward-mark
 (defun tempo-add-tag (tag template &optional tag-list)
   "Add a template tag.
 Add the TAG, that should complete to TEMPLATE to the list in TAG-LIST,
-or to `tempo-tags' if TAG-LIST is nil."
+or to `tempo-tags' if TAG-LIST is nil.  If TAG was already in the list,
+replace its template with TEMPLATE."
 
   (interactive "sTag: \nCTemplate: ")
   (if (null tag-list)
       (setq tag-list 'tempo-tags))
-  (if (not (assoc tag (symbol-value tag-list)))
-      (set tag-list (cons (cons tag template) (symbol-value tag-list))))
-  (tempo-invalidate-collection))
+  (let ((entry (assoc tag (symbol-value tag-list))))
+    (if entry
+        ;; Tag is already in the list, assign a new template to it
+        (setcdr entry template)
+      ;; Tag is not present in the list, add it with its template
+      (set tag-list (cons (cons tag template) (symbol-value tag-list)))))
+  ;; Invalidate globally if we're modifying `tempo-tags'
+  (tempo-invalidate-collection (eq tag-list 'tempo-tags)))
 
 ;;;
 ;;; tempo-use-tag-list
@@ -609,10 +617,17 @@ tempo-use-tag-list
 ;;;
 ;;; tempo-invalidate-collection
 
-(defun tempo-invalidate-collection ()
+(defun tempo-invalidate-collection (&optional global)
   "Marks the tag collection as obsolete.
-Whenever it is needed again it will be rebuilt."
-  (setq tempo-dirty-collection t))
+Whenever it is needed again it will be rebuilt.  When GLOBAL is
+non-nil, mark the tag collection of all buffers as obsolete, not just
+the current one."
+  (if global
+      (dolist (buffer (buffer-list))
+        (with-current-buffer buffer
+          (when (assq 'tempo-dirty-collection (buffer-local-variables))
+            (setq tempo-dirty-collection t))))
+    (setq tempo-dirty-collection t)))
 
 ;;;
 ;;; tempo-build-collection
diff --git a/test/lisp/tempo-tests.el b/test/lisp/tempo-tests.el
index 0dd310b853..bfe475910d 100644
--- a/test/lisp/tempo-tests.el
+++ b/test/lisp/tempo-tests.el
@@ -216,6 +216,45 @@ tempo-expand-tag-test
     (tempo-complete-tag)
     (should (equal (buffer-string) "Hello, World!"))))
 
+(ert-deftest tempo-define-tag-globally-test ()
+  "Testing usage of a template tag defined from another buffer."
+  (tempo-define-template "test" '("Hello, World!") "hello")
+
+  (with-temp-buffer
+    ;; Use a tag in buffer 1
+    (insert "hello")
+    (tempo-complete-tag)
+    (should (equal (buffer-string) "Hello, World!"))
+    (erase-buffer)
+
+    ;; Collection should not be dirty
+    (should-not tempo-dirty-collection)
+
+    ;; Define a tag on buffer 2
+    (with-temp-buffer
+      (tempo-define-template "test2" '("Now expanded.") "mytag"))
+
+    ;; I should be able to use this template back in buffer 1
+    (insert "mytag")
+    (tempo-complete-tag)
+    (should (equal (buffer-string) "Now expanded."))))
+
+(ert-deftest tempo-overwrite-tag-test ()
+  "Testing ability to reassign templates to tags."
+  (with-temp-buffer
+    ;; Define a tag and use it
+    (tempo-define-template "test-tag-1" '("abc") "footag")
+    (insert "footag")
+    (tempo-complete-tag)
+    (should (equal (buffer-string) "abc"))
+    (erase-buffer)
+
+    ;; Define a new template with the same tag
+    (tempo-define-template "test-tag-2" '("xyz") "footag")
+    (insert "footag")
+    (tempo-complete-tag)
+    (should (equal (buffer-string) "xyz"))))
+
 (ert-deftest tempo-expand-partial-tag-test ()
   "Testing expansion of a template with a tag, with a partial match."
   (with-temp-buffer
-- 
2.17.1

bug#39555: [PATCH] Allow tempo-define-template to reassign tags to new templates

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Mon, 10 Feb 2020 23:34:04 +0100
> 
> (This may apply as a bug and as a wishlist item at the same time - not sure)

I think this should go to master in any case.

Given that tempo.el is not actively maintained lately, I think you can
install your changes, after taking care of the minor issues below.

> -(defun tempo-invalidate-collection ()
> +(defun tempo-invalidate-collection (&optional global)
>    "Marks the tag collection as obsolete.
> -Whenever it is needed again it will be rebuilt."
> -  (setq tempo-dirty-collection t))
> +Whenever it is needed again it will be rebuilt.  When GLOBAL is
                                                    ^^^^
Please use "if", not "when".  That's our usual style of describing
optional arguments.

I think the API changes should be in NEWS.

Thanks.

bug#39555: [PATCH] Allow tempo-define-template to reassign tags to new templates

[Federico Tedin]

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

> I think this should go to master in any case.
>
> Given that tempo.el is not actively maintained lately, I think you can
> install your changes, after taking care of the minor issues below.
>
>> -(defun tempo-invalidate-collection ()
>> +(defun tempo-invalidate-collection (&optional global)
>>    "Marks the tag collection as obsolete.
>> -Whenever it is needed again it will be rebuilt."
>> -  (setq tempo-dirty-collection t))
>> +Whenever it is needed again it will be rebuilt.  When GLOBAL is
>                                                     ^^^^
> Please use "if", not "when".  That's our usual style of describing
> optional arguments.
>
> I think the API changes should be in NEWS.
>
> Thanks.

No problem, I'm attaching a new patch with those corrections applied.

- Fede


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Allow-tempo-define-template-to-reassign-tags-to-new-.patch --]
[-- Type: text/x-patch, Size: 5781 bytes --]

From 5dc2c0a4674177b04302e0cc3d90a1177817feab Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Mon, 17 Feb 2020 22:24:40 +0100
Subject: [PATCH] Allow tempo-define-template to reassign tags to new templates

* lisp/tempo.el (tempo-define-template): Update documentation string
to mention that existing tags can be reassigned new templates.
(tempo-add-tag): Allow reassigning tags to new templates.
Additionally, invalidate tag collections in all buffers if the global
tags list is being modified.
(tempo-invalidate-collection): Allow invalidating tag collections in
all buffers at the same time.
* test/lisp/tempo-tests.el (tempo-define-tag-globally-test): Add a
test to check that new templates plus tags can be defined from any
buffer and then immediately used in other buffers.
(tempo-overwrite-tag-test): Add a test to check that tags can be
reassigned templates.
* etc/NEWS: Announce changes.
---
 etc/NEWS                 |  8 ++++++++
 lisp/tempo.el            | 31 +++++++++++++++++++++++--------
 test/lisp/tempo-tests.el | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 1f8e6049a8..17992d541a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -126,6 +126,14 @@ supplied error message.
 *** New connection method "media", which allows accessing media devices
 like cell phones, tablets or cameras.
 
+** Tempo
+
+---
+*** 'tempo-define-template' can now re-assign templates to tags.
+Previously, assigning a new template to an already defined tag had no
+effect.
+
+
 ** map.el
 
 *** Pcase 'map' pattern added keyword symbols abbreviation.
diff --git a/lisp/tempo.el b/lisp/tempo.el
index 9de5ac66c7..2da90f08c8 100644
--- a/lisp/tempo.el
+++ b/lisp/tempo.el
@@ -220,7 +220,9 @@ tempo-define-template
 DOCUMENTATION is the documentation string for the insertion command
 created, and TAGLIST (a symbol) is the tag list that TAG (if provided)
 should be added to.  If TAGLIST is nil and TAG is non-nil, TAG is
-added to `tempo-tags'.
+added to `tempo-tags'.  If TAG already corresponds to a template in
+the tag list, modify the list so that TAG now corresponds to the newly
+defined template.
 
 The elements in ELEMENTS can be of several types:
 
@@ -579,14 +581,20 @@ tempo-backward-mark
 (defun tempo-add-tag (tag template &optional tag-list)
   "Add a template tag.
 Add the TAG, that should complete to TEMPLATE to the list in TAG-LIST,
-or to `tempo-tags' if TAG-LIST is nil."
+or to `tempo-tags' if TAG-LIST is nil.  If TAG was already in the list,
+replace its template with TEMPLATE."
 
   (interactive "sTag: \nCTemplate: ")
   (if (null tag-list)
       (setq tag-list 'tempo-tags))
-  (if (not (assoc tag (symbol-value tag-list)))
-      (set tag-list (cons (cons tag template) (symbol-value tag-list))))
-  (tempo-invalidate-collection))
+  (let ((entry (assoc tag (symbol-value tag-list))))
+    (if entry
+        ;; Tag is already in the list, assign a new template to it
+        (setcdr entry template)
+      ;; Tag is not present in the list, add it with its template
+      (set tag-list (cons (cons tag template) (symbol-value tag-list)))))
+  ;; Invalidate globally if we're modifying `tempo-tags'
+  (tempo-invalidate-collection (eq tag-list 'tempo-tags)))
 
 ;;;
 ;;; tempo-use-tag-list
@@ -609,10 +617,17 @@ tempo-use-tag-list
 ;;;
 ;;; tempo-invalidate-collection
 
-(defun tempo-invalidate-collection ()
+(defun tempo-invalidate-collection (&optional global)
   "Marks the tag collection as obsolete.
-Whenever it is needed again it will be rebuilt."
-  (setq tempo-dirty-collection t))
+Whenever it is needed again it will be rebuilt.  If GLOBAL is non-nil,
+mark the tag collection of all buffers as obsolete, not just the
+current one."
+  (if global
+      (dolist (buffer (buffer-list))
+        (with-current-buffer buffer
+          (when (assq 'tempo-dirty-collection (buffer-local-variables))
+            (setq tempo-dirty-collection t))))
+    (setq tempo-dirty-collection t)))
 
 ;;;
 ;;; tempo-build-collection
diff --git a/test/lisp/tempo-tests.el b/test/lisp/tempo-tests.el
index 0dd310b853..bfe475910d 100644
--- a/test/lisp/tempo-tests.el
+++ b/test/lisp/tempo-tests.el
@@ -216,6 +216,45 @@ tempo-expand-tag-test
     (tempo-complete-tag)
     (should (equal (buffer-string) "Hello, World!"))))
 
+(ert-deftest tempo-define-tag-globally-test ()
+  "Testing usage of a template tag defined from another buffer."
+  (tempo-define-template "test" '("Hello, World!") "hello")
+
+  (with-temp-buffer
+    ;; Use a tag in buffer 1
+    (insert "hello")
+    (tempo-complete-tag)
+    (should (equal (buffer-string) "Hello, World!"))
+    (erase-buffer)
+
+    ;; Collection should not be dirty
+    (should-not tempo-dirty-collection)
+
+    ;; Define a tag on buffer 2
+    (with-temp-buffer
+      (tempo-define-template "test2" '("Now expanded.") "mytag"))
+
+    ;; I should be able to use this template back in buffer 1
+    (insert "mytag")
+    (tempo-complete-tag)
+    (should (equal (buffer-string) "Now expanded."))))
+
+(ert-deftest tempo-overwrite-tag-test ()
+  "Testing ability to reassign templates to tags."
+  (with-temp-buffer
+    ;; Define a tag and use it
+    (tempo-define-template "test-tag-1" '("abc") "footag")
+    (insert "footag")
+    (tempo-complete-tag)
+    (should (equal (buffer-string) "abc"))
+    (erase-buffer)
+
+    ;; Define a new template with the same tag
+    (tempo-define-template "test-tag-2" '("xyz") "footag")
+    (insert "footag")
+    (tempo-complete-tag)
+    (should (equal (buffer-string) "xyz"))))
+
 (ert-deftest tempo-expand-partial-tag-test ()
   "Testing expansion of a template with a tag, with a partial match."
   (with-temp-buffer
-- 
2.21.1 (Apple Git-122.3)

bug#39555: [PATCH] Allow tempo-define-template to reassign tags to new templates

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmail.com>
> Cc: 39555@debbugs.gnu.org
> Date: Mon, 17 Feb 2020 22:27:51 +0100
> 
> No problem, I'm attaching a new patch with those corrections applied.

Thanks, pushed to the master branch.

Please see my follow-up commit with minor punctuation fixes.  Also,
please try to always mention in the log message the bug number when
it's known (this is impossible if your original message that opens a
bug report already includes the patch, but it _is_ possible when you
post additional versions to the same bug report).

bug#39555: [PATCH] Allow tempo-define-template to reassign tags to new templates

[Federico Tedin]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Federico Tedin <federicotedin@gmail.com>
>> Cc: 39555@debbugs.gnu.org
>> Date: Mon, 17 Feb 2020 22:27:51 +0100
>> 
>> No problem, I'm attaching a new patch with those corrections applied.
>
> Thanks, pushed to the master branch.
>
> Please see my follow-up commit with minor punctuation fixes.  Also,
> please try to always mention in the log message the bug number when
> it's known (this is impossible if your original message that opens a
> bug report already includes the patch, but it _is_ possible when you
> post additional versions to the same bug report).

Noted! Thanks.