bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc.

[Stefan Monnier]

> Just to make sure we're on same page: You mean it should (maybe) push
> the events into `unread-command-events` assuming LIFO semantics? 

Yes.

>> Seeing how `calc.el` used the -1 to signal an error (i.e. forcing
>> kmacros to contain "complete sequences"), maybe a half-way behavior
>> between the current one and your new one would be for `read_char` to
>> return -1 (or rather a more explicit `end-of-kbd-macro`) event
>> *once* and then on the next call to go on and read from the keyboard.
>
> Hm, indeed. But to be honest, I'm not convinced that lisp code should
> be able to distinguish this at all when trying to get an event. (If you
> *really* want to detect this, you could still check `executing-kbd-
> macro` after reading an event and see if it has changed from non-nil to
> nil.) Moreover, it would make the C code more complicated, because
> we'll probably have to add another state variable that captures if
> we've returned -1 (or a symbol) already or not.

FWIW, I tend to agree.

> Another compromise, which I find nicer, is to introduce a new variant
> of `read-char` (or add a flag) that suppresses the -1 entirely.

I'd rather not go there, if we can avoid it.

>    1. Work on an improved version of my patch, which brings it closer
>       to the ideal semantics, but doesn't touch too much of the code?

That's what I'd vote for.

>    2. Write a new patch that implements the "full" non-nesting
>       semantics suggest above?

In the long run it might be worth trying it out, but I suspect this
/will/ bump into surprising corner cases, so I'd keep it as a subsequent
step which could be made optional (I suspect it can be implemented fully
in ELisp).

>> IIUC, with your patch, we have the following scenario:
>> - Say we're inside a kmacro with N events left to execute.
>> - Dbus reads those N events and stashes them them onto `unread-
>> command-events`.
>> - Dbus finally can read the actual dbus event and does its thing.
>> - Good!
>> - But now `at_end_of_macro_p` will return true, even though we've yet
>> to
>> execute the last N events. We'll presumably still execute them
>> (since they're in `unread-command-events`) but that won't be
>> considered as coming from a kmacro any more.
> This matches my understanding. My thinking is that this is not a big
> deal in this specific case. The dbus code currently relies on the idea
> of reading events and putting them back to `unread-command-events`.
> While the patch affects the behavior, it doesn't change anything about
> the fact that this is an ugly hack anyway.

You mean they had it coming?  I can agree to some extent, but currently
there aren't very many alternative approaches :-(

>> Yes, I think we should document in a comment somewhere how the end of
>> kmacro is handled from a "global" perspective: what `read_char` does
>> when it's the case, where/when it's supposed to be detected and how
>> this "signal" is propagated from there to the corresponding call to
>> `execute-kbd-macro`, how that relates to setting
>> `executing-kbd-macro` back to nil, ...
>
> Agreed.

Could you try writing such a description?  It doesn't have to be
complete: just write what you happen to know already.


        Stefan