Strange behavior of C-u in the presence of sit-for in p-c-h

Strange behavior of C-u in the presence of sit-for in p-c-h

[Stefan Monnier]

Try the following:

  emacs -Q --eval "(add-hook 'post-command-hook (lambda () (sit-for 0.5)))"

you'll notice that all works apparently well, except that if you hit the
universal prefix C-u, then any key pressed before the 0.5s delay will
be ignored, and after the 0.5s delay you end up with "C-u-" in the
echo area.


        Stefan

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Richard Stallman]

Can you debug this?

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Stefan Monnier]

> Can you debug this?

I don't have time right now.  I looked at it briefly and decided it wasn't
obvious enough.  I must say I haven't followed the fit-for development close
enough to have a good feel for what the problem may be.


        Stefan

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Richard Stallman]

Can someone else please debug this?

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Chong Yidong]

Here is an easy way to reproduce the underlying bug:

(defun foo ()
  (interactive)
  (universal-argument)
  (push ?a unread-command-events))

M-x foo RET

The expected result is aaaa, but in fact nothing is printed.

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Chong Yidong]

Chong Yidong <cyd@stupidchicken.com> writes:

> Here is an easy way to reproduce the underlying bug:
>
> (defun foo ()
>   (interactive)
>   (universal-argument)
>   (push ?a unread-command-events))
>
> M-x foo RET
>
> The expected result is aaaa, but in fact nothing is printed.

The problem here is that C-u works through the command
`universal-argument-other-key', which sets prefix-arg and then adds
(this-command-keys) to unread-command-events so that it is executed
with that prefix argument.

This normally works ok.  However, suppose the call to
read_key_sequence in the command loop takes its input from
unread-command-events.  (This happens if the input came during a
sit-for in post-command-hook, as in Stefan's original example, or
directly as in the example above.)  This kind of "reread input" is
explicitly *not* added to this_command_keys.  The rationale for this
is not clear to me, but there may be a good reason since the code
explicitly checks for this; see keyboard.c:789.  Then
`universal-argument-other-key' can't see that input.

OTOH, I don't see a safe way of fixing this.  Any suggestions?

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Stefan Monnier]

> This normally works ok.  However, suppose the call to
> read_key_sequence in the command loop takes its input from
> unread-command-events.  (This happens if the input came during a
> sit-for in post-command-hook, as in Stefan's original example, or
> directly as in the example above.)  This kind of "reread input" is
> explicitly *not* added to this_command_keys.  The rationale for this
> is not clear to me, but there may be a good reason since the code
> explicitly checks for this; see keyboard.c:789.  Then
> `universal-argument-other-key' can't see that input.

I think I understand, but I can't find the code in keyboard.c.
Do you really mean "line 789"?  Of which revision?


        Stefan

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Chong Yidong]

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

>> This normally works ok.  However, suppose the call to
>> read_key_sequence in the command loop takes its input from
>> unread-command-events.  (This happens if the input came during a
>> sit-for in post-command-hook, as in Stefan's original example, or
>> directly as in the example above.)  This kind of "reread input" is
>> explicitly *not* added to this_command_keys.  The rationale for this
>> is not clear to me, but there may be a good reason since the code
>> explicitly checks for this; see keyboard.c:789.  Then
>> `universal-argument-other-key' can't see that input.
>
> I think I understand, but I can't find the code in keyboard.c.
> Do you really mean "line 789"?  Of which revision?

Sorry; by 789, I mean 3262 :-P

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Stefan Monnier]

> This normally works ok.  However, suppose the call to
> read_key_sequence in the command loop takes its input from
> unread-command-events.  (This happens if the input came during a
> sit-for in post-command-hook, as in Stefan's original example, or
> directly as in the example above.)  This kind of "reread input" is
> explicitly *not* added to this_command_keys.  The rationale for this
> is not clear to me, but there may be a good reason since the code
> explicitly checks for this; see keyboard.c:789.  Then
> `universal-argument-other-key' can't see that input.

> OTOH, I don't see a safe way of fixing this.  Any suggestions?

The check on line 3262 seems odd.  If we trace it back it was introduced in
revision 1.489 in the following form:

    if (this_command_key_count == 0 || ! reread)

the log comment doesn't say much useful:

    (read_char): Call the input method if appropriate.
    Change logic for distinguishing rereads from new events;
    use local var `reread'.  Take events from
    Vunread_input_method_events and Vunread_post_input_method_events.

I'm not sure exactly what this was trying to avoid, but the test looks odd.
Maybe the "!reread" could make some sense, but the
"this_command_key_count==0" looks positively odd since it may end up
recording the first (and only the first) of a sequence of keys.

Maybe the problem is that `this-command-keys' has several potential uses and
they are incompatible: in one case one wants this-command-keys to list the
keys the user has typed (independently from whether or not some of those
keys were later read&unread&reread&reunread&rereread), whereas in the other
one wants the exact key-sequence which triggered this command, so we can
push it back on unread-command-events to force re-interpretation of
those keys.


        Stefan

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Chong Yidong]

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

> Maybe the problem is that `this-command-keys' has several potential uses and
> they are incompatible: in one case one wants this-command-keys to list the
> keys the user has typed (independently from whether or not some of those
> keys were later read&unread&reread&reunread&rereread), whereas in the other
> one wants the exact key-sequence which triggered this command, so we can
> push it back on unread-command-events to force re-interpretation of
> those keys.

Given that the code has been around for 8 years, changing it at this
point in the release process might be a disaster.  It's not even clear
to me what TRT is in this case.  I suggest leaving the current
behavior as it is for Emacs 22; any user-level Lisp code that bumps
into this can quite easily work around it by checking if this-command
is eq to universal-argument.

Then maybe we can revisit this issue after the release.  WDYT?

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Stefan Monnier]

> Given that the code has been around for 8 years, changing it at this
> point in the release process might be a disaster.  It's not even clear
> to me what TRT is in this case.  I suggest leaving the current
> behavior as it is for Emacs 22; any user-level Lisp code that bumps
> into this can quite easily work around it by checking if this-command
> is eq to universal-argument.

Huh?  The problem came up because of a minor mode who uses post-command-hook
with a sit-for inside.  How do you suggest to work around this problem?


        Stefan

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Chong Yidong]

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

>> Given that the code has been around for 8 years, changing it at
>> this point in the release process might be a disaster.  It's not
>> even clear to me what TRT is in this case.  I suggest leaving the
>> current behavior as it is for Emacs 22; any user-level Lisp code
>> that bumps into this can quite easily work around it by checking if
>> this-command is eq to universal-argument.
>
> Huh?  The problem came up because of a minor mode who uses
> post-command-hook with a sit-for inside.  How do you suggest to work
> around this problem?

Which minor mode is that?  It could avoid performing the sit-for
during universal-argument:

  (unless (eq this-command 'universal-argument)
    (sit-for 0.5))

Or can you think of a safe way to change the keyboard.c code?

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Stefan Monnier]

> Which minor mode is that?

haskell-doc-mode

> It could avoid performing the sit-for during universal-argument:
>   (unless (eq this-command 'universal-argument)
>     (sit-for 0.5))

Oh, I see.  Well, in the case of haskell-doc-mode, the better fix is to use
an idle-timer rather than a post-command-hook.  But it's still a weird bug
introduced in Emacs-22 (it was there before, but the sit-for changes made
it appear in more cases).

> Or can you think of a safe way to change the keyboard.c code?

Add a parameter to this-command-keys which, if non-nil causes it to return
all keys, even those that were `reread'.
Then use this new parameter in universal-argument.


        Stefan

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Richard Stallman]

    Add a parameter to this-command-keys which, if non-nil causes it to return
    all keys, even those that were `reread'.

That would be hard to do, since there's only one data structure
and the keys that you don't get are not in the data structure.

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Richard Stallman]

    Which minor mode is that?  It could avoid performing the sit-for
    during universal-argument:

      (unless (eq this-command 'universal-argument)
	(sit-for 0.5))

If that works, I'd rather do that
than make any change at a lower level.

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

>     Which minor mode is that?  It could avoid performing the sit-for
>     during universal-argument:
>
>       (unless (eq this-command 'universal-argument)
> 	(sit-for 0.5))
>
> If that works, I'd rather do that
> than make any change at a lower level.

The minor mode that reported contains the code, haskell-doc-mode, is
not distributed with Emacs; and as Stefan noted it's more correct for
it to use an idle-timer.

I suggest not making a change to Emacs at this stage.  I could add a
TODO item about "implementing a new primitive that is like
this-command-keys only it accepts reread events", and look into it
after the release.

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Johan Bockgård]

Chong Yidong <cyd@stupidchicken.com> writes:

>  It could avoid performing the sit-for during universal-argument:
>
>   (unless (eq this-command 'universal-argument)
>     (sit-for 0.5))

Ditto for universal-argument-more, negative-argument, digit-argument,
etc...

-- 
Johan Bockgård

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Richard Stallman]

    I'm not sure exactly what this was trying to avoid, but the test looks odd.
    Maybe the "!reread" could make some sense, but the
    "this_command_key_count==0" looks positively odd since it may end up
    recording the first (and only the first) of a sequence of keys.

I designed that test to do the right thing in the cases that were
encountered, and both conditions proved to be necessary.

!reread is the usual case of reading a new character.

this_command_key_count==0 is the case you get when a command reads
input with read-event and then unreads it.  This usually IS the first
event of a key sequence; it is supposed to be.

To really do the right thing, we would need to associate each unread
key with a flag indicating whether it was previously added to
this-command-keys.  That would be a very painful incompatibility, so I
never wanted to do it, and I don't want to do it now.

    Maybe the problem is that `this-command-keys' has several potential uses and
    they are incompatible: in one case one wants this-command-keys to list the
    keys the user has typed (independently from whether or not some of those
    keys were later read&unread&reread&reunread&rereread), whereas in the other
    one wants the exact key-sequence which triggered this command, so we can
    push it back on unread-command-events to force re-interpretation of
    those keys.

I think that is true.

I suggest that we add a new primitive that does precisely what
`universal-argument-other-key' needs, and use it there.  That will be
safe, in that the change can't break anything else.

Does anyone disagree?  Would someone like to do this?

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

> To really do the right thing, we would need to associate each unread
> key with a flag indicating whether it was previously added to
> this-command-keys.  That would be a very painful incompatibility, so I
> never wanted to do it, and I don't want to do it now.
>
>     Maybe the problem is that `this-command-keys' has several potential uses and
>     they are incompatible: in one case one wants this-command-keys to list the
>     keys the user has typed (independently from whether or not some of those
>     keys were later read&unread&reread&reunread&rereread), whereas in the other
>     one wants the exact key-sequence which triggered this command, so we can
>     push it back on unread-command-events to force re-interpretation of
>     those keys.
>
> I think that is true.
>
> I suggest that we add a new primitive that does precisely what
> `universal-argument-other-key' needs, and use it there.  That will be
> safe, in that the change can't break anything else.
>
> Does anyone disagree?

This would involve lugging around an extra set of variables, exactly
like `this_command_keys' and `this_command_key_count' except reread
events get updated there too.  Sounds ugly.

How about a variable that tells the command loop to re-insert reread
commands into `this-command-keys' until the next command?  The command
loop could automagically reset this at the start of the loop, in case
of a quit.  Then universal-argument could toggle this variable.

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

> How about a variable that tells the command loop to re-insert reread
> commands into `this-command-keys' until the next command?  The command
> loop could automagically reset this at the start of the loop, in case
> of a quit.  Then universal-argument could toggle this variable.

Sounds like a good way to DTRT at this stage...

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

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

> Here is an easy way to reproduce the underlying bug:
>
> (defun foo ()
>   (interactive)
>   (universal-argument)
>   (push ?a unread-command-events))
>
> M-x foo RET
>
> The expected result is aaaa, but in fact nothing is printed.


Here is a simple patch which fixes the problem AFAICS.

It uses a special event code (t . EVENT) in unread-command-events
to specify that EVENT is _not_ reread.  Of course, this should be
documented in unread-command-events if people think this fix is ok.


*** keyboard.c	10 Oct 2006 10:33:25 +0200	1.878
--- keyboard.c	19 Oct 2006 23:00:23 +0200	
***************
*** 2537,2542 ****
--- 2537,2554 ----
        c = XCAR (Vunread_command_events);
        Vunread_command_events = XCDR (Vunread_command_events);
  
+       reread = 1;
+ 
+       /* Undo what sit-for did when it unread additional keys
+ 	 inside universal-argument.  */
+ 
+       if (CONSP (c)
+ 	  && EQ (XCAR (c), Qt))
+ 	{
+ 	  reread = 0;
+ 	  c = XCDR (c);
+ 	}
+ 
        /* Undo what read_char_x_menu_prompt did when it unread
  	 additional keys returned by Fx_popup_menu.  */
        if (CONSP (c)
***************
*** 2550,2556 ****
  	  && (EQ (c, Qtool_bar) || EQ (c, Qmenu_bar)))
  	*used_mouse_menu = 1;
  
-       reread = 1;
        goto reread_for_input_method;
      }
  
--- 2562,2567 ----


Index: subr.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/subr.el,v
retrieving revision 1.530
diff -c -r1.530 subr.el
*** subr.el	18 Oct 2006 10:56:46 -0000	1.530
--- subr.el	19 Oct 2006 21:08:24 -0000
***************
*** 1752,1759 ****
      (or nodisp (redisplay))
      (let ((read (read-event nil nil seconds)))
        (or (null read)
! 	  (progn (push read unread-command-events)
! 		 nil))))))
  \f
  ;;; Atomic change groups.
  
--- 1758,1768 ----
      (or nodisp (redisplay))
      (let ((read (read-event nil nil seconds)))
        (or (null read)
! 	  (progn
! 	    (if (eq overriding-terminal-local-map universal-argument-map)
! 		(setq read (cons t read)))
! 	    (push read unread-command-events)
! 	    nil))))))
  \f
  ;;; Atomic change groups.
  

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

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Richard Stallman]

Your approach is very good-- what I thought would be unacceptably
incompatible, you've made acceptable.  Please install your patch
and document it.

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Kim F. Storm]

Richard Stallman <rms@gnu.org> writes:

> Your approach is very good-- what I thought would be unacceptably
> incompatible, you've made acceptable.  Please install your patch
> and document it.

Done.

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

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Kim F. Storm]

Chong Yidong <cyd@stupidchicken.com> writes:

> Here is an easy way to reproduce the underlying bug:
>
> (defun foo ()
>   (interactive)
>   (universal-argument)
>   (push ?a unread-command-events))
>
> M-x foo RET
>
> The expected result is aaaa, but in fact nothing is printed.


Here is a simple patch which fixes the problem AFAICS.

It uses a special event code (t . EVENT) in unread-command-events
to specify that EVENT is _not_ reread.  Of course, this should be
documented in unread-command-events if people think this fix is ok.


*** keyboard.c	10 Oct 2006 10:33:25 +0200	1.878
--- keyboard.c	19 Oct 2006 23:00:23 +0200	
***************
*** 2537,2542 ****
--- 2537,2554 ----
        c = XCAR (Vunread_command_events);
        Vunread_command_events = XCDR (Vunread_command_events);
  
+       reread = 1;
+ 
+       /* Undo what sit-for did when it unread additional keys
+ 	 inside universal-argument.  */
+ 
+       if (CONSP (c)
+ 	  && EQ (XCAR (c), Qt))
+ 	{
+ 	  reread = 0;
+ 	  c = XCDR (c);
+ 	}
+ 
        /* Undo what read_char_x_menu_prompt did when it unread
  	 additional keys returned by Fx_popup_menu.  */
        if (CONSP (c)
***************
*** 2550,2556 ****
  	  && (EQ (c, Qtool_bar) || EQ (c, Qmenu_bar)))
  	*used_mouse_menu = 1;
  
-       reread = 1;
        goto reread_for_input_method;
      }
  
--- 2562,2567 ----


Index: subr.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/subr.el,v
retrieving revision 1.530
diff -c -r1.530 subr.el
*** subr.el	18 Oct 2006 10:56:46 -0000	1.530
--- subr.el	19 Oct 2006 21:08:24 -0000
***************
*** 1752,1759 ****
      (or nodisp (redisplay))
      (let ((read (read-event nil nil seconds)))
        (or (null read)
! 	  (progn (push read unread-command-events)
! 		 nil))))))
  \f
  ;;; Atomic change groups.
  
--- 1758,1768 ----
      (or nodisp (redisplay))
      (let ((read (read-event nil nil seconds)))
        (or (null read)
! 	  (progn
! 	    (if (eq overriding-terminal-local-map universal-argument-map)
! 		(setq read (cons t read)))
! 	    (push read unread-command-events)
! 	    nil))))))
  \f
  ;;; Atomic change groups.
  

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

Re: Strange behavior of C-u in the presence of sit-for in p-c-h

[Chong Yidong]

storm@cua.dk (Kim F. Storm) writes:

> Here is a simple patch which fixes the problem AFAICS.
>
> It uses a special event code (t . EVENT) in unread-command-events
> to specify that EVENT is _not_ reread.  Of course, this should be
> documented in unread-command-events if people think this fix is ok.

I think it's an excellent idea.

> *** keyboard.c	10 Oct 2006 10:33:25 +0200	1.878
> --- keyboard.c	19 Oct 2006 23:00:23 +0200	
> ***************
> *** 2537,2542 ****
> --- 2537,2554 ----
>         c = XCAR (Vunread_command_events);
>         Vunread_command_events = XCDR (Vunread_command_events);
>   
> +       reread = 1;
> + 
> +       /* Undo what sit-for did when it unread additional keys
> + 	 inside universal-argument.  */
> + 
> +       if (CONSP (c)
> + 	  && EQ (XCAR (c), Qt))
> + 	{
> + 	  reread = 0;
> + 	  c = XCDR (c);
> + 	}
> + 
>         /* Undo what read_char_x_menu_prompt did when it unread
>   	 additional keys returned by Fx_popup_menu.  */
>         if (CONSP (c)
> ***************
> *** 2550,2556 ****
>   	  && (EQ (c, Qtool_bar) || EQ (c, Qmenu_bar)))
>   	*used_mouse_menu = 1;
>   
> -       reread = 1;
>         goto reread_for_input_method;
>       }
>   
> --- 2562,2567 ----
>
>
> Index: subr.el
> ===================================================================
> RCS file: /cvsroot/emacs/emacs/lisp/subr.el,v
> retrieving revision 1.530
> diff -c -r1.530 subr.el
> *** subr.el	18 Oct 2006 10:56:46 -0000	1.530
> --- subr.el	19 Oct 2006 21:08:24 -0000
> ***************
> *** 1752,1759 ****
>       (or nodisp (redisplay))
>       (let ((read (read-event nil nil seconds)))
>         (or (null read)
> ! 	  (progn (push read unread-command-events)
> ! 		 nil))))))
>   \f
>   ;;; Atomic change groups.
>   
> --- 1758,1768 ----
>       (or nodisp (redisplay))
>       (let ((read (read-event nil nil seconds)))
>         (or (null read)
> ! 	  (progn
> ! 	    (if (eq overriding-terminal-local-map universal-argument-map)
> ! 		(setq read (cons t read)))
> ! 	    (push read unread-command-events)
> ! 	    nil))))))
>   \f
>   ;;; Atomic change groups.