Severe regressions in context of keyboard macros

Severe regressions in context of keyboard macros

[Christoph Arenz]

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

I think I have found two regressions that were introduced some time ago
by commit 30a6b1f81412044a, affecting keyboard macros. One is severely
affecting calc to the point of leading to completely wrong results -- if
not stopped by an error first. The other is affecting how dribble writes
out recorded keys.

Calc:
I stumbled across the following bug using calc -- see also
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37057:
90 <RET> S <f3> I S <f4>
This calculates the sine and inverse sine of 90 -- with a result of 90.
It also records "IS" as a keyboard macro.
Now, let's use the macro to complete the same calculation:
90 <RET> S <f4>
This worked until emacs-24.5 but is broken since emacs-25.1 where "ISS"
is recorded as a keyboard macro -- now leading to a result of 1.

It seems all macros that involve calc's prefix keys (e.g. "I" or "H")
have the following key recorded twice in last-kbd-macro. E.g. "IHP" is
recorded as "IHHPP", pushing GAMMA and PI on the stack instead of one
constant, namely PHI.

Hopefully, the calculation breaks due to an error -- otherwise, you get
nonsense results which might be hard to debug, if noticed at all.


Dribble:
According to the documentation in open-dribble-file, this starts
'writing all keyboard characters to a dribble file'.
However, the keys being recorded changed with commit 30a6b1f81412044a
when a keyboard macro is involved.

Prior, the key "<f4>" was recorded when a macro was replayed. With the
patch, the recording contains "<f4>" and additionally all characters
that were replayed by the macro. This gets ugly quickly, e.g. when <f3>
was used in the macro to insert a counter.


I am not suggesting that reverting commit 30a6b1f81412044a is the final
and right solution, but it does resolve the two issues in calc and
dribble in emacs-26.3. I stumbled across the mail thread
https://lists.gnu.org/archive/html/emacs-devel/2015-08/msg00193.html
where there were some doubts whether the quick fix could break something
else.

Keyboard.c seems highly complex and magic to me. I first thought that
sit-for in subr.el might have something to do with it as it has fixme
comments mentioning unread-command-events. Just a wild guess...



[-- Attachment #2: Type: text/html, Size: 3101 bytes --]

Re: Severe regressions in context of keyboard macros

[Eli Zaretskii]

> From: Christoph Arenz <tiga.arenz@web.de>
> Date: Thu, 19 Sep 2019 10:17:17 +0200
> 
> Calc:
> I stumbled across the following bug using calc -- see also
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37057:
> 90 <RET> S <f3> I S <f4>
> This calculates the sine and inverse sine of 90 -- with a result of 90.
> It also records "IS" as a keyboard macro.
> Now, let's use the macro to complete the same calculation:
> 90 <RET> S <f4>
> This worked until emacs-24.5 but is broken since emacs-25.1 where "ISS" is recorded as a keyboard macro
> -- now leading to a result of 1.

Calc seems to have various tricks related to keyboard macros (e.g.,
search for "kbd-macro"), so someone who knows Calc should go over that
and adapt what Calc does there to the new method of recording and
replaying keyboard macros.  I expect the fix to be simple.

> Dribble:
> According to the documentation in open-dribble-file, this starts 'writing all keyboard characters to a dribble
> file'. 
> However, the keys being recorded changed with commit 30a6b1f81412044a when a keyboard macro is
> involved.
> 
> Prior, the key "<f4>" was recorded when a macro was replayed. With the patch, the recording contains "<f4>"
> and additionally all characters that were replayed by the macro. This gets ugly quickly, e.g. when <f3> was
> used in the macro to insert a counter.

Why is that a problem?  I think the current dribble is more accurate,
as it shows what was injected into the Emacs keyboard event queue.

Thanks.

Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

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

[re-sending -- sorry, forgot to cc: emacs-devel the first time]

Thanks for your response!

On 9/20/19 9:23 AM, Eli Zaretskii wrote:
>> From: Christoph Arenz<tiga.arenz@web.de>
>> Date: Thu, 19 Sep 2019 10:17:17 +0200
>>
>> Calc:
>> I stumbled across the following bug using calc -- see also
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37057:
>> 90 <RET> S <f3> I S <f4>
>> This calculates the sine and inverse sine of 90 -- with a result of 90.
>> It also records "IS" as a keyboard macro.
>> Now, let's use the macro to complete the same calculation:
>> 90 <RET> S <f4>
>> This worked until emacs-24.5 but is broken since emacs-25.1 where "ISS" is recorded as a keyboard macro
>> -- now leading to a result of 1.
> Calc seems to have various tricks related to keyboard macros (e.g.,
> search for "kbd-macro"),
Yes, there are a number of hits for "kbd-macro". However as far as I
understand, calc relies on emacs' normal keyboard macro functions to
record the macro in the first place.

> so someone who knows Calc should go over that
hmm... who would that be? It seems that Jay Belanger stepped down as
maintainer of the calc package 4 years ago, putting in emacs-devel as
contact...
> and adapt what Calc does there to the new method of recording and
> replaying keyboard macros.  I expect the fix to be simple.
So, with regard to keyboard macros, is the following change intentional?
Having a scratch buffer with
(push ?a unread-command-events)
the key presses <f3> C-x C-e <f4> resulted in an "a" being inserted, and
a keyboard macro being recorded.
The behavior before 30a6b1f81412044a was that last-kbd-event was set to
"^X^E" and afterwards to "^X^Ea".
>> Dribble:
>> According to the documentation in open-dribble-file, this starts 'writing all keyboard characters to a dribble
>> file'.
>> However, the keys being recorded changed with commit 30a6b1f81412044a when a keyboard macro is
>> involved.
>>
>> Prior, the key "<f4>" was recorded when a macro was replayed. With the patch, the recording contains "<f4>"
>> and additionally all characters that were replayed by the macro. This gets ugly quickly, e.g. when <f3> was
>> used in the macro to insert a counter.
> Why is that a problem?  I think the current dribble is more accurate,
> as it shows what was injected into the Emacs keyboard event queue.
I see your point. The other aspect is which keys were 'pressed'? The
documentation of open-dribble-file (mentioning 'keyboard characters' and
'all characters you type') seems not very precise regarding this...
My thinking was that the new semantics makes it harder to interpret an
occurrence of "<f3>" in dribble file: Is it a start of a new macro
recording or is it an insertion (and incrementation) of a macro counter?
I guess it is as one cannot 'see' in the dribble file, where the
macro-caused events inserted after a "<f4>" end.

However, I have not fully thought this through... maybe it is that prior
behavior was to write out the meta-level only with regard to keyboard
macros and now have meta-level and results mixed together.

Thanks,
Christoph

[-- Attachment #2: Type: text/html, Size: 4555 bytes --]

Re: Severe regressions in context of keyboard macros

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Christoph Arenz <tiga.arenz@web.de>
> Date: Fri, 20 Sep 2019 17:43:13 +0200
> 
> > Calc seems to have various tricks related to keyboard macros (e.g.,
> > search for "kbd-macro"),
> Yes, there are a number of hits for "kbd-macro". However as far as I
> understand, calc relies on emacs' normal keyboard macro functions to
> record the macro in the first place.
> 
> > so someone who knows Calc should go over that
> hmm... who would that be?

You?

> So, with regard to keyboard macros, is the following change intentional?
> Having a scratch buffer with
> (push ?a unread-command-events)
> the key presses <f3> C-x C-e <f4> resulted in an "a" being inserted, and
> a keyboard macro being recorded.
> The behavior before 30a6b1f81412044a was that last-kbd-event was set to
> "^X^E" and afterwards to "^X^Ea".

I see nothing wrong with either behavior.

Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

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

On 9/20/19 7:54 PM, Eli Zaretskii wrote:
>>> Calc seems to have various tricks related to keyboard macros (e.g.,
>>> search for "kbd-macro"),
>> Yes, there are a number of hits for "kbd-macro". However as far as I
>> understand, calc relies on emacs' normal keyboard macro functions to
>> record the macro in the first place.
>>
>>> so someone who knows Calc should go over that
>> hmm... who would that be?
> You?
Not that I understand a lot of the codebase of calc or emacs, but let's
give it a try...
>> So, with regard to keyboard macros, is the following change intentional?
>> Having a scratch buffer with
>> (push ?a unread-command-events)
>> the key presses <f3> C-x C-e <f4> resulted in an "a" being inserted, and
>> a keyboard macro being recorded.
>> The behavior before 30a6b1f81412044a was that last-kbd-event was set to
>> "^X^E" and afterwards to "^X^Ea".
> I see nothing wrong with either behavior.
My understanding is that for calc prefix keys, calc relies on the
capability to have a single key press be handled as two events:
fancy-prefix-other-key triggers the second event by pushing onto
unread-command-events via calc-unread-command. Nonetheless during macro
definition, the key following the prefix key should only be recorded as
a single key press.

This can be achieved by getting back to the former behavior of having
(push ?a unread-command-events) store "^X^E" in last-kbd-macro when
evaluated during a keyboard macro definition.

How about the following patch? It solved the calc bug for me and I could
not find other regressions so far. Running make check showed no difference.
As mentioned, I do not understand all potential effects, so it should be
given some thoughts and tests.

Thanks, Christoph

==================

Parent:     96dd0196c2 * etc/HISTORY: Add Emacs 26.3 release release date.
Follows:    emacs-26.3 (1)

Fix double-recording of events during definition of keyboard macros.

Avoid double-recording of prefix-keys in calc during keyboard
macro definition which leads to errors and wrong results at macro
playback.

This patch changes behavior as follows:
After evaluation of (push ?a unread-command-events) with `<f3> C-x
C-e <f4>' last-kbd-macro is "^X^E" as in emacs-24.5 and no longer
"^X^Ea".

See also:
- bug report #37057:
25.2; Calc; Key erroneously recorded twice in keyboard macro

- mail discussion:
https://lists.gnu.org/archive/html/emacs-devel/2019-09/msg00424.html

Committer: Christoph Arenz <tiga.arenz@web.de>

1 file changed, 4 insertions(+), 2 deletions(-)
src/keyboard.c | 6 ++++--

modified   src/keyboard.c
@@ -3047,8 +3047,10 @@ read_char (int commandflag, Lisp_Object map,
      }
    /* When we consume events from the various unread-*-events lists, we
       bypass the code that records input, so record these events now if
-     they were not recorded already.  */
-  if (!recorded)
+     they were not recorded already.
+     However, avoid double-recording those events in case of a keyboard
+     macro being defined. */
+  if ((!recorded) && (NILP (KVAR (current_kboard, defining_kbd_macro))))
      {
        record_char (c);
        recorded = true;



[-- Attachment #2: Type: text/html, Size: 5139 bytes --]

Re: Severe regressions in context of keyboard macros

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Christoph Arenz <tiga.arenz@web.de>
> Date: Mon, 23 Sep 2019 13:57:00 +0200
> 
> How about the following patch? It solved the calc bug for me and I could not find other regressions so far.
> Running make check showed no difference.
> As mentioned, I do not understand all potential effects, so it should be given some thoughts and tests.

Thanks, but I'd like to avoid changes in keyboard.c on behalf of input
recording issues, unless such changes are really a must.  In this
case, we already have 2 facilities to deal with unwanted repeated
recording of keys:

  . a Lisp program can bind inhibit--record-char to a non-nil value to
    avoid recording input events while some Lisp form is executed (you
    can see an example of using this in quail.el:quail-start-translation
  . a Lisp program can push onto unread-command-events a cons cell of
    the form '(no-record . KEY) to avoid recording KEY more than once
    (you can see an example of using this in
    cua-base.el:cua--prefix-override-replay)

Can you use one of these facilities to solve the issue in Calc?  Note
that you will need to build Emacs from the Git master branch to be
able to use these facilities, they are not available before Emacs 27.

Thanks.

Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

On 9/24/19 10:45 AM, Eli Zaretskii wrote:
>> How about the following patch? It solved the calc bug for me and I could not find other regressions so far.
>> Running make check showed no difference.
>> As mentioned, I do not understand all potential effects, so it should be given some thoughts and tests.
> Thanks, but I'd like to avoid changes in keyboard.c on behalf of input
> recording issues, unless such changes are really a must.  In this
> case, we already have 2 facilities to deal with unwanted repeated
> recording of keys:
Thanks for the pointers!
>    . a Lisp program can bind inhibit--record-char to a non-nil value to
>      avoid recording input events while some Lisp form is executed (you
>      can see an example of using this in quail.el:quail-start-translation
>    . a Lisp program can push onto unread-command-events a cons cell of
>      the form '(no-record . KEY) to avoid recording KEY more than once
>      (you can see an example of using this in
>      cua-base.el:cua--prefix-override-replay)
>
> Can you use one of these facilities to solve the issue in Calc?  Note
> that you will need to build Emacs from the Git master branch to be
> able to use these facilities, they are not available before Emacs 27.
I think we are on to something here!
The sample in quail.el sounded fitting for me at first, but I could not
get it working for the calc case.
So I took a closer look at input-methods -- that is what quail is used
for, right?

I tried some simple tests using the french word for brother (`frère' --
the first `e' is with a ``' on top of it -- hopefully my mailer does not
screw this up...)
using various french input methods: french-prefix, french-postfix and
french-azerty -- all in context of keyboard macro recording and playback.
The calc case should fit nicely with the -postfix case, I thought.

However, on master branch 07367e5b95fe31f3d4e994b42b081075501b9b60, I
got this:

french-prefix:
keys pressed: <f3> f r ` e r e , <spc> <f4> <f4> <f4>
text inserted in buffer: "frère, frère,  frère  "
last-kbd-macro: "fr`ere,  "
--> Note the two(!) <spc> recorded after the `,' though only one was
typed in!

french-postfix:
keys pressed: <f3> f r e ` r e , <spc> <f4> <f4> <f4>
text inserted in buffer: "frère, frèrre,, frèrre,, "
last-kbd-macro: "fre`rre,, "
--> Note the double recording of `r' and `,' !
--> This closely resembles the reported symptoms for the calc package!

french-azerty:
keys pressed (on US keyboard-layout): <f3> f r e 7 r e m <spc> <f4> <f4>
<f4>
text inserted in buffer: "frère, frère, frère, "
last-kbd-macro: "fr7rem "
--> This looks as I would expect it.


A quick check on emacs-24.5 showed that all cases were handled correctly
back then.
So, some of the new ways of handling this are not covering all corner
cases, and work wrong with -postfix and calc prefix handling.

Does this give any clues what still needs to be fixed? I am lost in the
complexity of how the code should handle this...
N.B. For what it's worth, I am physically using a german keyboard with a
US layout in GNOME.
Probably this should not matter and be equivalent to a US keyboard...


Thanks!

Re: Severe regressions in context of keyboard macros

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Christoph Arenz <tiga.arenz@web.de>
> Date: Thu, 26 Sep 2019 12:46:20 +0200
> 
> I tried some simple tests using the french word for brother (`frère' --
> the first `e' is with a ``' on top of it -- hopefully my mailer does not
> screw this up...)
> using various french input methods: french-prefix, french-postfix and
> french-azerty -- all in context of keyboard macro recording and playback.
> The calc case should fit nicely with the -postfix case, I thought.
> 
> However, on master branch 07367e5b95fe31f3d4e994b42b081075501b9b60, I
> got this:
> 
> french-prefix:
> keys pressed: <f3> f r ` e r e , <spc> <f4> <f4> <f4>
> text inserted in buffer: "frère, frère,  frère  "
> last-kbd-macro: "fr`ere,  "
> --> Note the two(!) <spc> recorded after the `,' though only one was
> typed in!
> 
> french-postfix:
> keys pressed: <f3> f r e ` r e , <spc> <f4> <f4> <f4>
> text inserted in buffer: "frère, frèrre,, frèrre,, "
> last-kbd-macro: "fre`rre,, "
> --> Note the double recording of `r' and `,' !
> --> This closely resembles the reported symptoms for the calc package!
> 
> french-azerty:
> keys pressed (on US keyboard-layout): <f3> f r e 7 r e m <spc> <f4> <f4>
> <f4>
> text inserted in buffer: "frère, frère, frère, "
> last-kbd-macro: "fr7rem "
> --> This looks as I would expect it.
> 
> 
> A quick check on emacs-24.5 showed that all cases were handled correctly
> back then.
> So, some of the new ways of handling this are not covering all corner
> cases, and work wrong with -postfix and calc prefix handling.
> 
> Does this give any clues what still needs to be fixed?

What about the other facility -- could Calc use it to avoid recording
keys more than once?

Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

On 9/26/19 12:56 PM, Eli Zaretskii wrote:
> What about the other facility -- could Calc use it to avoid recording
> keys more than once?
I have not yet looked into that -- as I am getting more time constrained.

Re: Severe regressions in context of keyboard macros

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Christoph Arenz <tiga.arenz@web.de>
> Date: Thu, 26 Sep 2019 13:22:50 +0200
> 
> On 9/26/19 12:56 PM, Eli Zaretskii wrote:
> > What about the other facility -- could Calc use it to avoid recording
> > keys more than once?
> I have not yet looked into that -- as I am getting more time constrained.

Thanks.  And the method used by quail.el -- did you actually try to
use it in Calc, or did you just look at what it does in the context of
input methods?

Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

On 9/26/19 2:10 PM, Eli Zaretskii wrote:
>> Cc: emacs-devel@gnu.org
>> From: Christoph Arenz <tiga.arenz@web.de>
>> Date: Thu, 26 Sep 2019 13:22:50 +0200
>>
>> On 9/26/19 12:56 PM, Eli Zaretskii wrote:
>>> What about the other facility -- could Calc use it to avoid recording
>>> keys more than once?
>> I have not yet looked into that -- as I am getting more time constrained.
> Thanks.  And the method used by quail.el -- did you actually try to
> use it in Calc, or did you just look at what it does in the context of
> input methods?
Yes, I tried to get a grip on how inhibit--record-char should be used both
by wrapping some parts in calc.el in a (let ((inhibit--record-char t)) ...)
form, e.g. in calc-fancy-prefix-other-key and/or in calc-fancy-prefix but
could not get it to work.
At that point I tried to understand it better by code reading in quail.el
and did some edebug tries with my simplified test case with
(push ?a unread-command-events).

In case of quail, I used french-prefix etc. input-methods and triggered two
keyboard events like
(progn
     (push ?, unread-command-events)
     (push ?c unread-command-events))
which let me stumble across weird behavior, which again lead me to more
closely do some simple tests as described in my former mail.

Now, I started to get a closer look at the other facility you mentioned:
'(no-record . KEY):
Here is my current result. I did not have much time for tests, though.
Does this go in the right direction?

Thanks!

working on master 07367e5b95fe31f3d4e994b42b081075501b9b60

modified   lisp/calc/calc.el
@@ -3400,7 +3400,17 @@ calc-read-key
      (cons key key)))

  (defun calc-unread-command (&optional input)
-  (push (or input last-command-event) unread-command-events))
+  (let ((event (or input last-command-event)))
+    ;; do not double-record key presses in calc when defining a
keyboard macro
+    ;; FIXME: need to check if 'when form' is correct for all calls of
this function
+    ;; seems to work for calls from calc-fancy-prefix-other-key
+    ;; checked for `<f3> I S <f4>' and `<f3> I H P <f4>'
+    ;; more tests needed
+    (when defining-kbd-macro
+      (setq event (cons 'no-record (if (consp event)
+                                       (cdr event)
+                                     event))))
+    (push event unread-command-events)))

  (defun calc-clear-unread-commands ()
    (setq unread-command-events nil))

Fwd: Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

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

FYI -- the bug regarding the input methods and keyboard macros is now
filed in a separate bug report:
bug#37526: 27.0.50; double-recording of keys with input-methods and
keyboard-macros

Thanks,
Christoph

-------- Forwarded Message --------
Subject: 	Re: Severe regressions in context of keyboard macros
Date: 	Thu, 26 Sep 2019 12:46:20 +0200
From: 	Christoph Arenz <tiga.arenz@web.de>
To: 	Eli Zaretskii <eliz@gnu.org>
CC: 	emacs-devel@gnu.org



On 9/24/19 10:45 AM, Eli Zaretskii wrote:
>> How about the following patch? It solved the calc bug for me and I
>> could not find other regressions so far.
>> Running make check showed no difference.
>> As mentioned, I do not understand all potential effects, so it should
>> be given some thoughts and tests.
> Thanks, but I'd like to avoid changes in keyboard.c on behalf of input
> recording issues, unless such changes are really a must. In this
> case, we already have 2 facilities to deal with unwanted repeated
> recording of keys:
Thanks for the pointers!
> . a Lisp program can bind inhibit--record-char to a non-nil value to
> avoid recording input events while some Lisp form is executed (you
> can see an example of using this in quail.el:quail-start-translation
> . a Lisp program can push onto unread-command-events a cons cell of
> the form '(no-record . KEY) to avoid recording KEY more than once
> (you can see an example of using this in
> cua-base.el:cua--prefix-override-replay)
>
> Can you use one of these facilities to solve the issue in Calc? Note
> that you will need to build Emacs from the Git master branch to be
> able to use these facilities, they are not available before Emacs 27.
I think we are on to something here!
The sample in quail.el sounded fitting for me at first, but I could not
get it working for the calc case.
So I took a closer look at input-methods -- that is what quail is used
for, right?

I tried some simple tests using the french word for brother (`frère' --
the first `e' is with a ``' on top of it -- hopefully my mailer does not
screw this up...)
using various french input methods: french-prefix, french-postfix and
french-azerty -- all in context of keyboard macro recording and playback.
The calc case should fit nicely with the -postfix case, I thought.

However, on master branch 07367e5b95fe31f3d4e994b42b081075501b9b60, I
got this:

french-prefix:
keys pressed: <f3> f r ` e r e , <spc> <f4> <f4> <f4>
text inserted in buffer: "frère, frère,  frère  "
last-kbd-macro: "fr`ere,  "
--> Note the two(!) <spc> recorded after the `,' though only one was
typed in!

french-postfix:
keys pressed: <f3> f r e ` r e , <spc> <f4> <f4> <f4>
text inserted in buffer: "frère, frèrre,, frèrre,, "
last-kbd-macro: "fre`rre,, "
--> Note the double recording of `r' and `,' !
--> This closely resembles the reported symptoms for the calc package!

french-azerty:
keys pressed (on US keyboard-layout): <f3> f r e 7 r e m <spc> <f4> <f4>
<f4>
text inserted in buffer: "frère, frère, frère, "
last-kbd-macro: "fr7rem "
--> This looks as I would expect it.


A quick check on emacs-24.5 showed that all cases were handled correctly
back then.
So, some of the new ways of handling this are not covering all corner
cases, and work wrong with -postfix and calc prefix handling.

Does this give any clues what still needs to be fixed? I am lost in the
complexity of how the code should handle this...
N.B. For what it's worth, I am physically using a german keyboard with a
US layout in GNOME.
Probably this should not matter and be equivalent to a US keyboard...


Thanks!


[-- Attachment #2: Type: text/html, Size: 6051 bytes --]

Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

On 26.09.19 20:27, Christoph Arenz wrote:
>
> Now, I started to get a closer look at the other facility you mentioned:
> '(no-record . KEY):
> Here is my current result. I did not have much time for tests, though.
> Does this go in the right direction?
I spent some more time on this. The problem is bigger than stated in the
original bug report:
Any function key that follows a digit and is not separated by a <spc> or
<return> is recorded twice in calc when defining a keyboard macro, e.g.
`1 <return> <f3> 2 + <f4>' records "2++".

I am getting more confident that the patch below fixes this bug.Itfixes
a severe issue with macros in calc, does not make things worse and does
not interfere in case no macro is being defined.

Your thoughts?

Thanks,
Christoph

Author:     Christoph Arenz <tiga.arenz@web.de>
AuthorDate: Fri Sep 27 20:47:18 2019 +0200

Calc: prevent double-recording of keys in keyboard macros.

1 file changed, 5 insertions(+), 1 deletion(-)
lisp/calc/calc.el | 6 +++++-

modified   lisp/calc/calc.el
@@ -3400,7 +3400,11 @@ calc-read-key
      (cons key key)))

  (defun calc-unread-command (&optional input)
-  (push (or input last-command-event) unread-command-events))
+  (let ((event (or input last-command-event)))
+    ;; do not double-record key presses when defining a keyboard macro
+    (when defining-kbd-macro
+      (setq event (cons 'no-record event)))
+    (push event unread-command-events)))

  (defun calc-clear-unread-commands ()
    (setq unread-command-events nil))

Re: Severe regressions in context of keyboard macros

[Eli Zaretskii]

> From: Christoph Arenz <tiga.arenz@web.de>
> Cc: emacs-devel@gnu.org
> Date: Sat, 28 Sep 2019 11:18:51 +0200
> 
> I spent some more time on this. The problem is bigger than stated in the
> original bug report:
> Any function key that follows a digit and is not separated by a <spc> or
> <return> is recorded twice in calc when defining a keyboard macro, e.g.
> `1 <return> <f3> 2 + <f4>' records "2++".

Is this while using Calc, or is this in some other situation?

> I am getting more confident that the patch below fixes this bug.Itfixes
> a severe issue with macros in calc, does not make things worse and does
> not interfere in case no macro is being defined.
> 
> Your thoughts?

If it works well for you in Calc, I will install it.

Thanks.

P.S.  I'll look into the other bug, regarding input methods, when I
have time.  Thanks for filing it.

Re: Severe regressions in context of keyboard macros

[Stefan Monnier]

>  (defun calc-unread-command (&optional input)
> -  (push (or input last-command-event) unread-command-events))
> +  (let ((event (or input last-command-event)))
> +    ;; do not double-record key presses when defining a keyboard macro
> +    (when defining-kbd-macro
> +      (setq event (cons 'no-record event)))
> +    (push event unread-command-events)))

Over the years, I have convinced myself that there is pretty much no way
to put events back on unread-command-events for reprocessing without
introducing corner case bugs.

For this reason I introduced the `set-transient-map` function, which can
be used in some cases to avoid "read + unread".

Anyway, my point is that maybe a better fix is to try and understand why
Calc does a read+unread and then figure out a way to get the same end
result without needing to read+unread.


        Stefan

Re: Severe regressions in context of keyboard macros

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 28 Sep 2019 08:35:29 -0400
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> Anyway, my point is that maybe a better fix is to try and understand why
> Calc does a read+unread and then figure out a way to get the same end
> result without needing to read+unread.

If someone is willing to do that, I'd of course encourage that.

Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

On 28.09.19 11:46, Eli Zaretskii wrote:
>> I spent some more time on this. The problem is bigger than stated in the
>> original bug report:
>> Any function key that follows a digit and is not separated by a <spc> or
>> <return> is recorded twice in calc when defining a keyboard macro, e.g.
>> `1 <return> <f3> 2 + <f4>' records "2++".
> Is this while using Calc, or is this in some other situation?
Just in Calc. I should have been a bit clearer what I meant with
`function' key:
While calc is reading digits, it recognizes it has rad a complete number
when reading <spc> or <return>.

Calc also recognizes a number when reading a calc function key when
reading digits. This is one path when calc reads this key a second time.


Here is lossage for such a case:
  1             ;; calcDigit-start
  <return>      ;; calcDigit-nondigit
  1             ;; calcDigit-start
  2             ;; calcDigit-key
  +             ;; calcDigit-key
  +             ;; calc-plus


Note the `+' being read twice. With the proposed patch, lossage looks
just the same -- just in case of keyboard macro recording, we avoid the
`+' being recorded twice. Lossage in this case looks like this:
  <f3>         ;; kmacro-start-macro-or-insert-counter
  1            ;; calcDigit-start
  <return>     ;; calcDigit-nondigit
  1            ;; calcDigit-start
  2            ;; calcDigit-key
  +            ;; calcDigit-key
  ;; calc-plus
  <f4>         ;; kmacro-end-or-call-macro
>> I am getting more confident that the patch below fixes this bug.Itfixes
>> a severe issue with macros in calc, does not make things worse and does
>> not interfere in case no macro is being defined.
>>
>> Your thoughts?
> If it works well for you in Calc, I will install it.
>
> Thanks.
Yes, the patch definitely improves the situation. Thanks!
>
> P.S.  I'll look into the other bug, regarding input methods, when I
> have time.  Thanks for filing it.
Thanks for that as well!

Re: Severe regressions in context of keyboard macros

[Christoph Arenz]

On 28.09.19 15:44, Eli Zaretskii wrote:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Sat, 28 Sep 2019 08:35:29 -0400
>> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
>>
>> Anyway, my point is that maybe a better fix is to try and understand why
>> Calc does a read+unread and then figure out a way to get the same end
>> result without needing to read+unread.

That would definitely be the cleanest approach. As I understand it,
there are about 15 places in calc where it calls `calc-unread-command',
which then pushes events into unread-command-events and four more cases
in keypad.el without calling calc-unread-command.

Understanding the 'why' for every case probably requires someone with a
better expertise of calc than me...

> If someone is willing to do that, I'd of course encourage that.

Re: Severe regressions in context of keyboard macros

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Christoph Arenz <tiga.arenz@web.de>
> Date: Sun, 29 Sep 2019 19:42:56 +0200
> 
> On 28.09.19 11:46, Eli Zaretskii wrote:
> >> I spent some more time on this. The problem is bigger than stated in the
> >> original bug report:
> >> Any function key that follows a digit and is not separated by a <spc> or
> >> <return> is recorded twice in calc when defining a keyboard macro, e.g.
> >> `1 <return> <f3> 2 + <f4>' records "2++".
> > Is this while using Calc, or is this in some other situation?
> Just in Calc. I should have been a bit clearer what I meant with
> `function' key:
> While calc is reading digits, it recognizes it has rad a complete number
> when reading <spc> or <return>.
> 
> Calc also recognizes a number when reading a calc function key when
> reading digits. This is one path when calc reads this key a second time.
> 
> 
> Here is lossage for such a case:
>   1             ;; calcDigit-start
>   <return>      ;; calcDigit-nondigit
>   1             ;; calcDigit-start
>   2             ;; calcDigit-key
>   +             ;; calcDigit-key
>   +             ;; calc-plus
> 
> 
> Note the `+' being read twice. With the proposed patch, lossage looks
> just the same -- just in case of keyboard macro recording, we avoid the
> `+' being recorded twice. Lossage in this case looks like this:
>   <f3>         ;; kmacro-start-macro-or-insert-counter
>   1            ;; calcDigit-start
>   <return>     ;; calcDigit-nondigit
>   1            ;; calcDigit-start
>   2            ;; calcDigit-key
>   +            ;; calcDigit-key
>   ;; calc-plus
>   <f4>         ;; kmacro-end-or-call-macro
> >> I am getting more confident that the patch below fixes this bug.Itfixes
> >> a severe issue with macros in calc, does not make things worse and does
> >> not interfere in case no macro is being defined.
> >>
> >> Your thoughts?
> > If it works well for you in Calc, I will install it.
> >
> > Thanks.
> Yes, the patch definitely improves the situation. Thanks!

Thanks, I've finally installed your fix in Calc, it will be in the
next Emacs release.  Sorry for the long delay.