Another bug with the macro counter

Another bug with the macro counter

[Luc Teirlinck]

Do `emacs -q'.

C-x ( a C-x C-k C-i b RET C-x )
C-x e e e
C-x ( C-g
C-x e

Result:

a0b
a1b
a2b
a3b
a0b

Of course, the newly to be defined macro got its counter reset to 0.
But after we changed our mind, decided not to define a macro and quit,
there is no reason why the counter for the previously defined macro
should be reset to 0.

Sincerely,

Luc.

Re: Another bug with the macro counter

[Luc Teirlinck]

>From my earlier message:

   Do `emacs -q'.

   C-x ( a C-x C-k C-i b RET C-x )
   C-x e e e
   C-x ( C-g
   C-x e

   Result:

   a0b
   a1b
   a2b
   a3b
   a0b

   Of course, the newly to be defined macro got its counter reset to 0.
   But after we changed our mind, decided not to define a macro and quit,
   there is no reason why the counter for the previously defined macro
   should be reset to 0.

Actually, that was a misinterpretation.  There is a problem in the
above, but not the one I suspected.

The problem is that after the user quits while defining a macro, the
old head of the keyboard macro ring is still the head, with a reset
counter (because `kmacro-start-macro' reset it).  That _same_ macro is
now however _also_ the previous macro on the ring (but with the
original counter) because `kmacro-start-macro' already pushed it onto
the macro ring.  That duplication is confusing to the user, _even_ if
the user does not care about the counter.

The patches below fix the bug.  If they look OK, I will install them
after doing some extra final checking.

There is one thing that the patches below do not fix: they do not
reset `kmacro-last-counter' correctly.  To fix that, as well as
several other bugs related to the keyboard macro counter, the elements
of `kmacro-ring' should be quadruples (MACRO COUNTER LAST-COUNTER FORMAT)
rather than triples (MACRO COUNTER FORMAT).  If there is more than one
keyboard macro _all_ the relevant information needs to be stored in
`kmacro-ring'.  The patches below do not do that.

The patch to simple.el also quiets the compiler for `edebug-active'.
This is unrelated, but while getting rid of compiler warnings related
to my patch, I can quite as well get rid of all warnings.  (I checked
that the warnings are really false alarms.)

===File ~/kmacro.el-diff====================================
*** kmacro.el	11 Oct 2004 17:28:10 -0500	1.23
--- kmacro.el	29 Oct 2004 20:17:12 -0500	
***************
*** 218,223 ****
--- 218,226 ----
  ;;;###autoload (global-set-key "\C-x\C-k" 'kmacro-keymap)
  ;;;###autoload (autoload 'kmacro-keymap "kmacro" "Keymap for keyboard macro commands." t 'keymap)
  
+ (defvar appending-to-kbd-macro nil
+   "Non-nil when appending to a keyboard macro definition.")
+ 
  (if kmacro-call-mouse-event
    (global-set-key (vector kmacro-call-mouse-event) 'kmacro-end-call-mouse))
  
***************
*** 564,569 ****
--- 567,573 ----
    (if (or defining-kbd-macro executing-kbd-macro)
        (message "Already defining keyboard macro.")
      (let ((append (and arg (listp arg))))
+       (setq appending-to-kbd-macro append)
        (unless append
  	(if last-kbd-macro
  	    (let ((len (length kmacro-ring)))
============================================================

===File ~/simple-diff=======================================
*** simple.el	25 Oct 2004 07:40:49 -0500	1.664
--- simple.el	29 Oct 2004 20:39:08 -0500	
***************
*** 865,873 ****
    (if (and (integerp value)
             (or (not (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                 (eq this-command last-command)
!                (and (boundp 'edebug-active) edebug-active)))
        (let ((char-string
!              (if (or (and (boundp 'edebug-active) edebug-active)
                       (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                   (prin1-char value))))
          (if char-string
--- 865,874 ----
    (if (and (integerp value)
             (or (not (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                 (eq this-command last-command)
!                (and (boundp 'edebug-active) (with-no-warnings edebug-active))))
        (let ((char-string
!              (if (or (and (boundp 'edebug-active)
! 			  (with-no-warnings edebug-active))
                       (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                   (prin1-char value))))
          (if char-string
***************
*** 3916,3921 ****
--- 3917,3927 ----
  At top-level, as an editor command, this simply beeps."
    (interactive)
    (deactivate-mark)
+   (or (not (featurep 'kmacro))
+       (with-no-warnings appending-to-kbd-macro)
+       (kmacro-ring-empty-p)
+       (kmacro-pop-ring)
+       (with-no-warnings (setq appending-to-kbd-macro nil)))
    (setq defining-kbd-macro nil)
    (signal 'quit nil))
  
============================================================

Re: Another bug with the macro counter

[Luc Teirlinck]

The following is a corrected version of my patch to simple.el:

===File ~/simple-new-diff===================================
*** simple.el	25 Oct 2004 07:40:49 -0500	1.664
--- simple.el	29 Oct 2004 22:13:27 -0500	
***************
*** 865,873 ****
    (if (and (integerp value)
             (or (not (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                 (eq this-command last-command)
!                (and (boundp 'edebug-active) edebug-active)))
        (let ((char-string
!              (if (or (and (boundp 'edebug-active) edebug-active)
                       (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                   (prin1-char value))))
          (if char-string
--- 865,874 ----
    (if (and (integerp value)
             (or (not (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                 (eq this-command last-command)
!                (and (boundp 'edebug-active) (with-no-warnings edebug-active))))
        (let ((char-string
!              (if (or (and (boundp 'edebug-active)
! 			  (with-no-warnings edebug-active))
                       (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                   (prin1-char value))))
          (if char-string
***************
*** 3916,3921 ****
--- 3917,3928 ----
  At top-level, as an editor command, this simply beeps."
    (interactive)
    (deactivate-mark)
+   (or (not (featurep 'kmacro))
+       (with-no-warnings appending-to-kbd-macro)
+       ;; never exits the or.
+       (with-no-warnings (setq appending-to-kbd-macro nil))
+       (kmacro-ring-empty-p)
+       (kmacro-pop-ring))
    (setq defining-kbd-macro nil)
    (signal 'quit nil))
  
============================================================

Re: Another bug with the macro counter

[Stefan]

> ***************
> *** 865,873 ****
>     (if (and (integerp value)
>              (or (not (memq this-command '(eval-last-sexp eval-print-last-sexp)))
>                  (eq this-command last-command)
> !                (and (boundp 'edebug-active) edebug-active)))
>         (let ((char-string
> !              (if (or (and (boundp 'edebug-active) edebug-active)
>                        (memq this-command '(eval-last-sexp eval-print-last-sexp)))
>                    (prin1-char value))))
>           (if char-string
> --- 865,874 ----
>     (if (and (integerp value)
>              (or (not (memq this-command '(eval-last-sexp eval-print-last-sexp)))
>                  (eq this-command last-command)
> !                (and (boundp 'edebug-active) (with-no-warnings edebug-active))))
>         (let ((char-string
> !              (if (or (and (boundp 'edebug-active)
> ! 			  (with-no-warnings edebug-active))
>                        (memq this-command '(eval-last-sexp eval-print-last-sexp)))
>                    (prin1-char value))))
>           (if char-string

I believe replacing (and (boundp 'foo) foo) with (if (boundp 'foo) foo)
is a better choice because it removes the warning wihtout cluttering the
code and without introducing compatibility problems with older Emacsen.


        Stefan

Re: Another bug with the macro counter

[Luc Teirlinck]

Stefan Monnier wrote:

   I believe replacing (and (boundp 'foo) foo) with (if (boundp 'foo) foo)
   is a better choice because it removes the warning wihtout cluttering the
   code and without introducing compatibility problems with older Emacsen.

Actually, on second thought, it is probably better to solve the two
original problems the "classical" way, with a defvar.  However, I need
a `with-no-warnings' at two other places.  Yesterday, I forgot that
one has to compile as the very first thing after `emacs -q', to get
all warnings.  Revised patch:

===File ~/simple-diff-3=====================================
*** simple.el	25 Oct 2004 07:40:49 -0500	1.664
--- simple.el	30 Oct 2004 08:41:38 -0500	
***************
*** 857,862 ****
--- 857,865 ----
    :type 'boolean
    :version "21.1")
  
+ ;; Quiet the compiler.
+ (defvar edebug-active) 
+ 
  (defun eval-expression-print-format (value)
    "Format VALUE as a result of evaluated expression.
  Return a formatted string which is displayed in the echo area
***************
*** 3907,3912 ****
--- 3910,3918 ----
  ;Turned off because it makes dbx bomb out.
  (setq blink-paren-function 'blink-matching-open)
  \f
+ ;; Quit the compiler.
+ (defvar appending-to-kbd-macro)
+ 
  ;; This executes C-g typed while Emacs is waiting for a command.
  ;; Quitting out of a program does not go through here;
  ;; that happens in the QUIT macro at the C code level.
***************
*** 3916,3921 ****
--- 3922,3933 ----
  At top-level, as an editor command, this simply beeps."
    (interactive)
    (deactivate-mark)
+   (or (not (featurep 'kmacro))
+       appending-to-kbd-macro
+       ;; never exits the or.
+       (setq appending-to-kbd-macro nil)
+       (with-no-warnings (kmacro-ring-empty-p))
+       (with-no-warnings (kmacro-pop-ring)))
    (setq defining-kbd-macro nil)
    (signal 'quit nil))
  
============================================================

Re: Another bug with the macro counter

[Stefan]

> + ;; Quiet the compiler.
> + (defvar edebug-active) 
> + 

I'm not sure this is right.  I think such defvars should only be put when we
know from circumstancial evidence (that the byte-compiler does not have
access to) that the variable will be available.  If we need to check with
`boundp', I find it's better not to put a `defvar' (and to rely on the (if
(boundp 'foo) foo) form recognized by the byte-compiler).


        Stefan

Re: Another bug with the macro counter

[David Kastrup]

Stefan <monnier@iro.umontreal.ca> writes:

>> + ;; Quiet the compiler.
>> + (defvar edebug-active) 
>> + 
>
> I'm not sure this is right.  I think such defvars should only be put when we
> know from circumstancial evidence (that the byte-compiler does not have
> access to) that the variable will be available.  If we need to check with
> `boundp', I find it's better not to put a `defvar' (and to rely on the (if
> (boundp 'foo) foo) form recognized by the byte-compiler).

Quieting the byte compiler can probably also be achieved with
(eval-when-compile (defvar edebug-active))

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Another bug with the macro counter

[Luc Teirlinck]

David Kastrup wrote:

   (eval-when-compile (defvar edebug-active))

Stefan can clarify, but I understood his objection as meaning that if
somebody later added a function that assumed that `edebug-active' was
defined, then the compiler would no longer warn about that.  It does
not look as if the `eval-when-compile' would change anything about
that.

Of course, even though (if (boundp 'edebug-active) edebug-active) is
equivalent in its effects to (and (boundp 'edebug-active) edebug-active)
it seems stylistically inferior, since it makes the intent and logical
structure less clear, especially as it interacts with `or'.

It seems that we can not win on both counts.  In the latest patch I
sent, I followed Stefan's suggestion but put in a comment.

But I do not really have any strong opinion on the subject.

Sincerely,

Luc.

Re: Another bug with the macro counter

[Stefan]

> Quieting the byte compiler can probably also be achieved with
> (eval-when-compile (defvar edebug-active))

Hopefully such ugly hack doesn't work.
(defvar foo) only has an effect when seen by the byte-compiler, so putting
it in a `eval-when-compile' is at best pointless.

But what I was objecting to is the fact that using (defvar foo) is really
telling the byte-compiler: "look, I know you can't see it, but trust me,
this is a global or dynamically-bound variable that will be bound at
runtime, so don't warn me when I use it".

In the case of edebug-active, it is simply not true: it's a variable that
might be bound or not, which is why we test it with `boundp'.  This case is
handled in the byte-compiler by recognizing the (if (boundp 'foo) ...) form
so we know that within the first arm of the `if' the variable is bound.
We could extend this trick to also recognize (and (boundp 'foo) ...)
or even (and ... (boundp 'foo) ...) but it's clearly not necessary in
this case.

Whether (and (boundp 'foo) foo) is better stylistically than
(if (boundp 'foo) foo) is debatable, but I do know that the `if'
form is stylistically much better than the use of (defvar foo)
or (with-no-warning foo).


        Stefan

Re: Another bug with the macro counter

[David Kastrup]

Stefan <monnier@iro.umontreal.ca> writes:

>> Quieting the byte compiler can probably also be achieved with
>> (eval-when-compile (defvar edebug-active))
>
> Hopefully such ugly hack doesn't work.
> (defvar foo) only has an effect when seen by the byte-compiler, so putting
> it in a `eval-when-compile' is at best pointless.

Last time I looked, defvar had an effect at run time, namely binding
the variable to NIL if yet unbound.  And that means that if a package
_defining_ foo gets loaded, the initial definition of foo will be nil
instead of the intended initial value.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Another bug with the macro counter

[Andreas Schwab]

David Kastrup <dak@gnu.org> writes:

> Last time I looked, defvar had an effect at run time, namely binding
> the variable to NIL if yet unbound.

It never did that.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Another bug with the macro counter

[Stefan]

> Last time I looked, defvar had an effect at run time, namely binding
> the variable to NIL if yet unbound.

(defvar foo) has never done that AFAIK.


        Stefan

Re: Another bug with the macro counter

[David Kastrup]

Stefan <monnier@iro.umontreal.ca> writes:

>> Last time I looked, defvar had an effect at run time, namely binding
>> the variable to NIL if yet unbound.
>
> (defvar foo) has never done that AFAIK.

Mea culpa.  Is there any intended effect of (defvar foo) then apart
from silencing the byte compiler?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Another bug with the macro counter

[Luc Teirlinck]

David Kastrup wrote:

   Mea culpa.  Is there any intended effect of (defvar foo) then apart
   from silencing the byte compiler?

I believe that it is the express purpose of (defvar foo) to make
clear, to humans and to programs, that the defvar is only there to
silence the compiler and that the real defvar is elsewhere.  I believe
that a computer silencing defvar should never specify a value and a
real defvar always should.

Sincerely,

Luc.

Re: Another bug with the macro counter

[Luc Teirlinck]

>From my original reply:

   + ;; Quit the compiler.
   + (defvar appending-to-kbd-macro)
   + 

Should be:

;; Quiet the compiler.

Re: Another bug with the macro counter

[Luc Teirlinck]

Actually, I believe I can quite as well move the _real_ defvar of
`appending-to-kbd-macro' to simple.el:

===File ~/simple-diff-4=====================================
*** simple.el	25 Oct 2004 07:40:49 -0500	1.664
--- simple.el	30 Oct 2004 09:28:02 -0500	
***************
*** 857,862 ****
--- 857,865 ----
    :type 'boolean
    :version "21.1")
  
+ ;; Quiet the compiler.
+ (defvar edebug-active) 
+ 
  (defun eval-expression-print-format (value)
    "Format VALUE as a result of evaluated expression.
  Return a formatted string which is displayed in the echo area
***************
*** 3907,3912 ****
--- 3910,3918 ----
  ;Turned off because it makes dbx bomb out.
  (setq blink-paren-function 'blink-matching-open)
  \f
+ (defvar appending-to-kbd-macro nil
+   "Non-nil when appending to a keyboard macro definition.")
+ 
  ;; This executes C-g typed while Emacs is waiting for a command.
  ;; Quitting out of a program does not go through here;
  ;; that happens in the QUIT macro at the C code level.
***************
*** 3916,3921 ****
--- 3922,3933 ----
  At top-level, as an editor command, this simply beeps."
    (interactive)
    (deactivate-mark)
+   (or (not (featurep 'kmacro))
+       appending-to-kbd-macro
+       ;; never exits the or.
+       (setq appending-to-kbd-macro nil)
+       (with-no-warnings (kmacro-ring-empty-p))
+       (with-no-warnings (kmacro-pop-ring)))
    (setq defining-kbd-macro nil)
    (signal 'quit nil))
  
============================================================

===File ~/kmacro-diff-4=====================================
*** kmacro.el	11 Oct 2004 17:28:10 -0500	1.23
--- kmacro.el	30 Oct 2004 09:28:52 -0500	
***************
*** 564,569 ****
--- 564,570 ----
    (if (or defining-kbd-macro executing-kbd-macro)
        (message "Already defining keyboard macro.")
      (let ((append (and arg (listp arg))))
+       (setq appending-to-kbd-macro append)
        (unless append
  	(if last-kbd-macro
  	    (let ((len (length kmacro-ring)))
============================================================

Re: Another bug with the macro counter

[Kim F. Storm]

Thanks for working on this problem!


I don't quite follow the logic here:

+   (or (not (featurep 'kmacro))
+	appending-to-kbd-macro
+	;; never exits the or.
+	(setq appending-to-kbd-macro nil)
+	(with-no-warnings (kmacro-ring-empty-p))
+	(with-no-warnings (kmacro-pop-ring)))

If appending-to-kbd-macro is non-nil in the second line, you don't get
to the third line which sets it to nil (i.e. it is only set to nil if
it is already nil).

I think it will be better if kmacro.el defines a function

(defun kmacro-quit ()
   (or appending-to-kbd-macro
       (kmacro-ring-empty-p)
       (kmacro-pop-ring))
   (setq appending-to-kbd-macro nil))

which does the cleanup and then simply call this as

      (if (fboundp 'kmacro-quit)
        (kmacro-quit))

in keyboard-quit.

Or even better, define a keyboard-quit-hook and add kmacro-quit to it.

Here is a patch which does that:

*** simple.el	25 Oct 2004 10:45:18 +0200	1.664
--- simple.el	30 Oct 2004 23:51:40 +0200	
***************
*** 3910,3920 ****
--- 3910,3926 ----
  ;; This executes C-g typed while Emacs is waiting for a command.
  ;; Quitting out of a program does not go through here;
  ;; that happens in the QUIT macro at the C code level.
+ 
+ (defvar keyboard-quit-hook nil
+   "Normal hook run by `keyboard-quit'.
+ It is called before any of the normal cleanup performed by `keyboard-quit'.")
+ 
  (defun keyboard-quit ()
    "Signal a `quit' condition.
  During execution of Lisp code, this character causes a quit directly.
  At top-level, as an editor command, this simply beeps."
    (interactive)
+   (run-hooks 'keyboard-quit-hook)
    (deactivate-mark)
    (setq defining-kbd-macro nil)
    (signal 'quit nil))

*** kmacro.el	19 Oct 2004 12:54:29 +0200	1.23
--- kmacro.el	30 Oct 2004 23:52:43 +0200	
***************
*** 222,227 ****
--- 222,239 ----
    (global-set-key (vector kmacro-call-mouse-event) 'kmacro-end-call-mouse))
  
  
+ ;;; Keyboard Quit Handler
+ 
+ (defvar kmacro-appending-p nil)
+ 
+ (defun kmacro-keyboard-quit ()
+    (or kmacro-appending-p
+        (kmacro-ring-empty-p)
+        (kmacro-pop-ring))
+    (setq kmacro-appending-p nil))
+ 
+ (add-hook 'keyboard-quit-hook 'kmacro-keyboard-quit)
+ 
  
  ;;; Keyboard macro counter
  
***************
*** 564,569 ****
--- 576,582 ----
    (if (or defining-kbd-macro executing-kbd-macro)
        (message "Already defining keyboard macro.")
      (let ((append (and arg (listp arg))))
+       (setq kmacro-appending-p append)
        (unless append
  	(if last-kbd-macro
  	    (let ((len (length kmacro-ring)))

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

Re: Another bug with the macro counter

[Luc Teirlinck]

Kim Storm wrote:

   If appending-to-kbd-macro is non-nil in the second line, you don't get
   to the third line which sets it to nil (i.e. it is only set to nil if
   it is already nil).

Yes, that was just some confusion on my part.

   I think it will be better if kmacro.el defines a function

and:

   If appending-to-kbd-macro is non-nil in the second line, you don't get
   to the third line which sets it to nil (i.e. it is only set to nil if
   it is already nil).

   I think it will be better if kmacro.el defines a function

   (defun kmacro-quit ()
      (or appending-to-kbd-macro
	  (kmacro-ring-empty-p)
	  (kmacro-pop-ring))
      (setq appending-to-kbd-macro nil))

   which does the cleanup and then simply call this as

	 (if (fboundp 'kmacro-quit)
	   (kmacro-quit))

   in keyboard-quit.

   Or even better, define a keyboard-quit-hook and add kmacro-quit to it.

   Here is a patch which does that:

I will take a look at it.

Just in case, here is the corrected version of my patch (although your
one might indeed be better, I still have to look at it.)


===File ~/simple-diff-5=====================================
*** simple.el	25 Oct 2004 07:40:49 -0500	1.664
--- simple.el	30 Oct 2004 16:41:28 -0500	
***************
*** 865,873 ****
    (if (and (integerp value)
             (or (not (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                 (eq this-command last-command)
!                (and (boundp 'edebug-active) edebug-active)))
        (let ((char-string
!              (if (or (and (boundp 'edebug-active) edebug-active)
                       (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                   (prin1-char value))))
          (if char-string
--- 865,874 ----
    (if (and (integerp value)
             (or (not (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                 (eq this-command last-command)
! 	       ;; Use `if' instead of `and' to avoid compiler warning.
!                (if (boundp 'edebug-active) edebug-active)))
        (let ((char-string
!              (if (or (if (boundp 'edebug-active) edebug-active)
                       (memq this-command '(eval-last-sexp eval-print-last-sexp)))
                   (prin1-char value))))
          (if char-string
***************
*** 3907,3912 ****
--- 3908,3918 ----
  ;Turned off because it makes dbx bomb out.
  (setq blink-paren-function 'blink-matching-open)
  \f
+ (defvar appending-to-kbd-macro nil
+   "Non-nil when appending to a keyboard macro definition.
+ The value is nil when defining a new keyboard macro.
+ In other situations, the value is undefined.")
+ 
  ;; This executes C-g typed while Emacs is waiting for a command.
  ;; Quitting out of a program does not go through here;
  ;; that happens in the QUIT macro at the C code level.
***************
*** 3916,3921 ****
--- 3922,3933 ----
  At top-level, as an editor command, this simply beeps."
    (interactive)
    (deactivate-mark)
+   (or appending-to-kbd-macro
+       ;; Just for safety.  kmacro.el should be loaded if we got here.
+       ;; Nothing else sets `appending-to-kbd-macro' to t.
+       (not (featurep 'kmacro))
+       (with-no-warnings (kmacro-ring-empty-p))
+       (with-no-warnings (kmacro-pop-ring)))
    (setq defining-kbd-macro nil)
    (signal 'quit nil))
  
============================================================

Re: Another bug with the macro counter

[Luc Teirlinck]

>From my previous message:

      I think it will be better if kmacro.el defines a function

   and:

      If appending-to-kbd-macro is non-nil in the second line, you don't get
      to the third line which sets it to nil (i.e. it is only set to nil if
      it is already nil).

I just did some erroneous cutting and pasting here and did not notice
it before sending it off.

Sincerely,

Luc.

Re: Another bug with the macro counter

[Kim F. Storm]

I think your patch is imcomplete (so is mine) --

we only want the kmacro cleanup to happen if we are
currently defining a macro, your patch does it always

Here is the correct function:

 (or (not defining-kbd-macro)
     appending-to-kbd-macro
     ;; Just for safety.  kmacro.el should be loaded if we got here.
     ;; Nothing else sets `appending-to-kbd-macro' to t.
     (not (featurep 'kmacro))
     (with-no-warnings (kmacro-ring-empty-p))
     (with-no-warnings (kmacro-pop-ring)))
 (setq appending-to-kbd-macro nil)
 (setq defining-kbd-macro nil)


In any case,  I still think my approach is cleaner and better.

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> ***************
> *** 3916,3921 ****
> --- 3922,3933 ----
>   At top-level, as an editor command, this simply beeps."
>     (interactive)
>     (deactivate-mark)
> +   (or appending-to-kbd-macro
> +       ;; Just for safety.  kmacro.el should be loaded if we got here.
> +       ;; Nothing else sets `appending-to-kbd-macro' to t.
> +       (not (featurep 'kmacro))
> +       (with-no-warnings (kmacro-ring-empty-p))
> +       (with-no-warnings (kmacro-pop-ring)))
>     (setq defining-kbd-macro nil)
>     (signal 'quit nil))
>   
> ============================================================



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

Re: Another bug with the macro counter

[Luc Teirlinck]

Your alternate patch seems OK, but I would make three changes to it.

First one: 

   + (defun kmacro-keyboard-quit ()
   +    (or kmacro-appending-p
   +        (kmacro-ring-empty-p)
   +        (kmacro-pop-ring))
   +    (setq kmacro-appending-p nil))

As you may already have pointed out yourself, you actually need:

   + (defun kmacro-keyboard-quit ()
   +    (or (not defining-kbd-macro)
	    kmacro-appending-p
   +        (kmacro-ring-empty-p)
   +        (kmacro-pop-ring))
   +    (setq kmacro-appending-p nil))

The second one is that I would rename kmacro-appending-p to
kmacro-appending-flag.

>From `(elisp)Tips for Defining':

`...-flag'
     The value is significant only as to whether it is `nil' or not.

The `-p' convention is for functions. 

The third one is that I would keep the docstring I gave for
kmacro-appending-flag:

  "Non-nil when appending to a keyboard macro definition.
The value is nil when defining a new keyboard macro.
In other situations, the value is undefined."

The `(setq kmacro-appending-p nil)' is not sufficient to guarantee
that kmacro-appending-p will be nil outside a macro definition.

There are two reasons to quit inside a keyboard macro.

The first, in my usage by far the most common, is that you start
defining a macro and then mess it up.  You do C-g while Emacs is
waiting for keyboard input to get rid of the erroneous macro.
Currently that duplicates the last correct keyboard macro in a
confusing way.  This is the situation we are trying to correct.

The second is that a function called during the keyboard macro
definition takes a huge amount of time and the user quits to regain
control.  In this situation, `keyboard-quit' is not called and the
quit is handled by QUIT.  Now the C-g terminates the macro definition
without becoming part of it, just like C-x ).  The new macro is likely
to be useless.  Unlike in the first case, the user can not but notice
what happened and hence can remove the macro with C-x C-k C-d.  This
situation remains unchanged after either your or my patch.  If this
situation happens while appending to a keyboard macro, then
kmacro-appending-p will remain t, even though no macro is being defined.
I could give a concrete example if desired.

Sincerely,

Luc.

Re: Another bug with the macro counter

[Kim F. Storm]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> The second one is that I would rename kmacro-appending-p to
> kmacro-appending-flag.

Of course!  But it would be better to get rid of the flag
completely.  

Below you find a patch sets defining-kbd-macro to 'append' when appending.

  
>            Unlike in the first case, the user can not but notice
> what happened and hence can remove the macro with C-x C-k C-d.  This
> situation remains unchanged after either your or my patch.  

Maybe this is ok -- it also gives him the chance to edit it with C-x C-k SPC.

>                                                             If this
> situation happens while appending to a keyboard macro, then
> kmacro-appending-p will remain t, even though no macro is being defined.
> I could give a concrete example if desired.

I suppose that defining-kbd-macro is set to nil in those cases,
so my patch fixes that part...

I think the patch is ready now -- should I install it?


*** kmacro.el	19 Oct 2004 12:54:29 +0200	1.23
--- kmacro.el	01 Nov 2004 00:05:58 +0100	
***************
*** 222,227 ****
--- 222,237 ----
    (global-set-key (vector kmacro-call-mouse-event) 'kmacro-end-call-mouse))
  
  
+ ;;; Called from keyboard-quit
+ 
+ (defun kmacro-keyboard-quit ()
+    (or (not defining-kbd-macro)
+        (eq defining-kbd-macro 'append)
+        (kmacro-ring-empty-p)
+        (kmacro-pop-ring)))
  
  ;;; Keyboard macro counter
  
***************
*** 585,591 ****
  		       (and append
  			    (if kmacro-execute-before-append
  				(> (car arg) 4)
! 			      (= (car arg) 4)))))))
  
  
  ;;;###autoload
--- 595,603 ----
  		       (and append
  			    (if kmacro-execute-before-append
  				(> (car arg) 4)
! 			      (= (car arg) 4))))
!       (if (and defining-kbd-macro append)
! 	  (setq defining-kbd-macro 'append)))))
  

*** simple.el	25 Oct 2004 10:45:18 +0200	1.664
--- simple.el	01 Nov 2004 00:18:07 +0100	
***************
*** 3916,3921 ****
--- 3916,3923 ----
  At top-level, as an editor command, this simply beeps."
    (interactive)
    (deactivate-mark)
+   (if (fboundp 'kmacro-keyboard-quit)
+       (kmacro-keyboard-quit))
    (setq defining-kbd-macro nil)
    (signal 'quit nil))
  

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

Re: Another bug with the macro counter

[Richard Stallman]

    I think it will be better if kmacro.el defines a function

    (defun kmacro-quit ()
       (or appending-to-kbd-macro
	   (kmacro-ring-empty-p)
	   (kmacro-pop-ring))
       (setq appending-to-kbd-macro nil))

    which does the cleanup and then simply call this as

	  (if (fboundp 'kmacro-quit)
	    (kmacro-quit))

That is definitely cleaner.

Re: Another bug with the macro counter

[Richard Stallman]

    I believe replacing (and (boundp 'foo) foo) with (if (boundp 'foo) foo)
    is a better choice because it removes the warning wihtout cluttering the
    code and without introducing compatibility problems with older Emacsen.

Why not extend that compiler feature to handle `and' as well as `if'?
This construction is common enough.