Is this tempo.el patch correct?

Is this tempo.el patch correct?

[Richard Stallman]

David K}gedal, author of tempo.el, seems not to be responding,
so we are on our own unless someone can find a better way to contact him.

Would someone please DTRT and then ack?

------- Start of forwarded message -------
To: emacs-pretest-bug@gnu.org
From: Lawrence Mitchell <wence@gmx.li>
Date: Fri, 02 Mar 2007 14:39:11 +0000
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: r> insertion marker not handled correctly in tempo
X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=failed 
	version=3.0.4


If you define a tempo template that includes the "r>" insertion
marker, attempting to use that template results in an error that the
function "r>" is undefined.

This bug may be reproduced as follows:

emacs -Q

Evaluate:

(progn
  (require 'tempo)
  (tempo-define-template
   "show-bug"
   '("Some string" n> (r> "body: ")))
  (setq debug-on-error t)
  (tempo-template-show-bug))


The following patch fixes this problem in, I believe, the correct
manner:

Index: tempo.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/tempo.el,v
retrieving revision 1.20.6.11
diff -c -r1.20.6.11 tempo.el
*** tempo.el	26 Jan 2007 06:15:15 -0000	1.20.6.11
- --- tempo.el	2 Mar 2007 14:36:39 -0000
***************
*** 352,357 ****
- --- 352,364 ----
  					 (goto-char tempo-region-stop)
  				       (tempo-insert-prompt-compat
  					(cdr element))))
+         ((and (consp element)
+               (eq (car element) 'r>)) (if on-region
+                                           (progn
+                                             (goto-char tempo-region-stop)
+                                             (indent-region (mark) (point) nil))
+                                         (tempo-insert-prompt-compat
+                                          (cdr element))))
  	((and (consp element)
  	      (eq (car element) 's)) (tempo-insert-named (car (cdr element))))
  	((and (consp element)

Changelog entry:

2007-03-02  Lawrence Mitchell  <wence@gmx.li>

	* tempo.el (tempo-insert): Deal with 'r> if it appears
	specified with a prompt argument.


Cheers,

Lawrence

- -- 
Lawrence Mitchell <wence@gmx.li>


_______________________________________________
emacs-pretest-bug mailing list
emacs-pretest-bug@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-pretest-bug
------- End of forwarded message -------

Re: Is this tempo.el patch correct?

[David Kågedal]

9 mar 2007 kl. 22.26 skrev Richard Stallman:

> David K}gedal, author of tempo.el, seems not to be responding,
> so we are on our own unless someone can find a better way to  
> contact him.
>
> Would someone please DTRT and then ack?

I'm sorry.  I'm here, and I put your mail on the pile of things to  
take care of RSN.

Since I haven't really done anything with tempo.el for ten years, it  
takes a bit of mental effort to review patches.  But I'll have a look  
tomorrow.

> ------- Start of forwarded message -------
> To: emacs-pretest-bug@gnu.org
> From: Lawrence Mitchell <wence@gmx.li>
> Date: Fri, 02 Mar 2007 14:39:11 +0000
> MIME-Version: 1.0
> Content-Type: text/plain; charset=us-ascii
> Subject: r> insertion marker not handled correctly in tempo
> X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=failed
> 	version=3.0.4
>
>
> If you define a tempo template that includes the "r>" insertion
> marker, attempting to use that template results in an error that the
> function "r>" is undefined.
>
> This bug may be reproduced as follows:
>
> emacs -Q
>
> Evaluate:
>
> (progn
>   (require 'tempo)
>   (tempo-define-template
>    "show-bug"
>    '("Some string" n> (r> "body: ")))
>   (setq debug-on-error t)
>   (tempo-template-show-bug))
>
>
> The following patch fixes this problem in, I believe, the correct
> manner:
>
> Index: tempo.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/tempo.el,v
> retrieving revision 1.20.6.11
> diff -c -r1.20.6.11 tempo.el
> *** tempo.el	26 Jan 2007 06:15:15 -0000	1.20.6.11
> - --- tempo.el	2 Mar 2007 14:36:39 -0000
> ***************
> *** 352,357 ****
> - --- 352,364 ----
>   					 (goto-char tempo-region-stop)
>   				       (tempo-insert-prompt-compat
>   					(cdr element))))
> +         ((and (consp element)
> +               (eq (car element) 'r>)) (if on-region
> +                                           (progn
> +                                             (goto-char tempo- 
> region-stop)
> +                                             (indent-region (mark)  
> (point) nil))
> +                                         (tempo-insert-prompt-compat
> +                                          (cdr element))))
>   	((and (consp element)
>   	      (eq (car element) 's)) (tempo-insert-named (car (cdr  
> element))))
>   	((and (consp element)
>
> Changelog entry:
>
> 2007-03-02  Lawrence Mitchell  <wence@gmx.li>
>
> 	* tempo.el (tempo-insert): Deal with 'r> if it appears
> 	specified with a prompt argument.
>
>
> Cheers,
>
> Lawrence
>
> - --
> Lawrence Mitchell <wence@gmx.li>
>
>
> _______________________________________________
> emacs-pretest-bug mailing list
> emacs-pretest-bug@gnu.org
> http://lists.gnu.org/mailman/listinfo/emacs-pretest-bug
> ------- End of forwarded message -------
>

-- 
David Kågedal
davidk@lysator.liu.se

Re: Is this tempo.el patch correct?

[Richard Stallman]

    Since I haven't really done anything with tempo.el for ten years, it  
    takes a bit of mental effort to review patches.  But I'll have a look  
    tomorrow.

Thank you in advance.

Re: Is this tempo.el patch correct?

[David Hansen]

On Fri, 09 Mar 2007 16:26:39 -0500 Richard Stallman wrote:

> Would someone please DTRT and then ack?

It's not a bug.  The documentation doesn't mention "r>" as a valid
prompt argument.

I do use tempo.el but not these features (prompting, wrapping around
the region).  If you think it's worth I can test the patch though.

David

Re: Is this tempo.el patch correct?

[Lawrence Mitchell]

David Hansen wrote:

> On Fri, 09 Mar 2007 16:26:39 -0500 Richard Stallman wrote:

>> Would someone please DTRT and then ack?

> It's not a bug.  The documentation doesn't mention "r>" as a valid
> prompt argument.

I presumed that a prompt argument should be handled since the
manual says, for r>:

| This element works like `r', but it also indents the region it is
| expanded around.

And "r" behaves like:

| (r PROMPT [NAME [NOINSERT]])

Perhaps that was a misreading of the feature set.


Cheers,

Lawrence

[...]

-- 
Lawrence Mitchell <wence@gmx.li>

Re: Is this tempo.el patch correct?

[Richard Stallman]

Since the file's maintainer got back in touch, we can leave it to him
to DTRT.

Is this tempo.el patch correct?

[Richard Stallman]

Please tell us whether to install this patch.

------- Start of forwarded message -------
To: emacs-pretest-bug@gnu.org
From: Lawrence Mitchell <wence@gmx.li>
Date: Fri, 02 Mar 2007 14:39:11 +0000
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: r> insertion marker not handled correctly in tempo
X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=failed 
	version=3.0.4


If you define a tempo template that includes the "r>" insertion
marker, attempting to use that template results in an error that the
function "r>" is undefined.

This bug may be reproduced as follows:

emacs -Q

Evaluate:

(progn
  (require 'tempo)
  (tempo-define-template
   "show-bug"
   '("Some string" n> (r> "body: ")))
  (setq debug-on-error t)
  (tempo-template-show-bug))


The following patch fixes this problem in, I believe, the correct
manner:

Index: tempo.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/tempo.el,v
retrieving revision 1.20.6.11
diff -c -r1.20.6.11 tempo.el
*** tempo.el	26 Jan 2007 06:15:15 -0000	1.20.6.11
- --- tempo.el	2 Mar 2007 14:36:39 -0000
***************
*** 352,357 ****
- --- 352,364 ----
  					 (goto-char tempo-region-stop)
  				       (tempo-insert-prompt-compat
  					(cdr element))))
+         ((and (consp element)
+               (eq (car element) 'r>)) (if on-region
+                                           (progn
+                                             (goto-char tempo-region-stop)
+                                             (indent-region (mark) (point) nil))
+                                         (tempo-insert-prompt-compat
+                                          (cdr element))))
  	((and (consp element)
  	      (eq (car element) 's)) (tempo-insert-named (car (cdr element))))
  	((and (consp element)

Changelog entry:

2007-03-02  Lawrence Mitchell  <wence@gmx.li>

	* tempo.el (tempo-insert): Deal with 'r> if it appears
	specified with a prompt argument.


Cheers,

Lawrence

- -- 
Lawrence Mitchell <wence@gmx.li>


_______________________________________________
emacs-pretest-bug mailing list
emacs-pretest-bug@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-pretest-bug
------- End of forwarded message -------

Re: Is this tempo.el patch correct?

[David Kågedal]

Richard Stallman <rms@gnu.org> writes:

> Please tell us whether to install this patch.
>
> From: Lawrence Mitchell <wence@gmx.li>
> Subject: r> insertion marker not handled correctly in tempo
> To: emacs-pretest-bug@gnu.org
> Date: Fri, 02 Mar 2007 14:39:11 +0000
>
>
> If you define a tempo template that includes the "r>" insertion
> marker, attempting to use that template results in an error that the
> function "r>" is undefined.
...
> The following patch fixes this problem in, I believe, the correct
> manner:
>
> Index: tempo.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/tempo.el,v
> retrieving revision 1.20.6.11
> diff -c -r1.20.6.11 tempo.el
> *** tempo.el	26 Jan 2007 06:15:15 -0000	1.20.6.11
> - --- tempo.el	2 Mar 2007 14:36:39 -0000
> ***************
> *** 352,357 ****
> - --- 352,364 ----
>   					 (goto-char tempo-region-stop)
>   				       (tempo-insert-prompt-compat
>   					(cdr element))))
> +         ((and (consp element)
> +               (eq (car element) 'r>)) (if on-region
> +                                           (progn
> +                                             (goto-char tempo-region-stop)
> +                                             (indent-region (mark) (point) nil))
> +                                         (tempo-insert-prompt-compat
> +                                          (cdr element))))
>   	((and (consp element)
>   	      (eq (car element) 's)) (tempo-insert-named (car (cdr element))))
>   	((and (consp element)
>
> Changelog entry:
>
> 2007-03-02  Lawrence Mitchell  <wence@gmx.li>
>
> 	* tempo.el (tempo-insert): Deal with 'r> if it appears
> 	specified with a prompt argument.

It looks good, but since it adds a new form, the documentation needs
an update as well.

This diff is against CVS head:

--- tempo.el	21 Jan 2007 03:53:10 -0000	1.33
+++ tempo.el	11 Mar 2007 13:27:37 -0000
@@ -266,6 +266,8 @@
    that you often should place this item after the text you want on
    the line.
  - `r>': Like `r', but it also indents the region.
+ - (r> PROMPT <NAME> <NOINSERT>): Like (r ...), but is also indents
+   the region.
  - `n>': Inserts a newline and indents line.
  - `o': Like `%' but leaves the point before the newline.
  - nil: It is ignored.
@@ -352,6 +354,13 @@
 					 (goto-char tempo-region-stop)
 				       (tempo-insert-prompt-compat
 					(cdr element))))
+        ((and (consp element)
+              (eq (car element) 'r>)) (if on-region
+                                          (progn
+                                            (goto-char tempo-region-stop)
+                                            (indent-region (mark) (point) nil))
+                                        (tempo-insert-prompt-compat
+                                         (cdr element))))
 	((and (consp element)
 	      (eq (car element) 's)) (tempo-insert-named (car (cdr element))))
 	((and (consp element)

-- 
David Kågedal

Re: Is this tempo.el patch correct?

[Richard Stallman]

Would someone please install the patch as David has modified it?
Then please ack this message.

Re: Is this tempo.el patch correct?

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

> Would someone please install the patch as David has modified it?
> Then please ack this message.

Done.

I marked it as (tiny change) since Lawrence Mitchell has only signed papers
for ERC and EMMS.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk