unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
To: Tim Ruffing <crypto@timruffing.de>
Cc: Eli Zaretskii <eliz@gnu.org>, 68272@debbugs.gnu.org
Subject: bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc.
Date: Sat, 13 Jan 2024 12:40:31 -0500	[thread overview]
Message-ID: <jwvzfx9gp8n.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <1b3fa12138838a3fe5643a9e76a65d32a677e34d.camel@timruffing.de> (Tim Ruffing's message of "Sat, 06 Jan 2024 15:32:23 +0100")

Hi Tim,

> Hey, thank you very much for the long email. I was somewhat prepared
> for hesitant reply, and I totally understand that "there be dragons".

:-)

> And I'm also aware that it doesn't spark confidence when someone with
> (almost) zero code contributions submits a patch of this kind.

[ Actually, such carefully written code and (even more so) the
  convention-abiding commit messages coming from someone who's not
  a regular contributor spiked my ears.  It made me think
  that this is either a carefully crafted trojan, or it's a really
  welcome new kid on the block :-)  ]

> First of all, I think the bug is real.

No doubt.  Bug#62018 brought to my attention to nasty situation, and I'm
really glad you went through the trouble to come up with a cleanup.

> But the problem is not just that this is not elegant.  It's worse
> because it's also pretty unclear what the caller should do in
> this case.

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, ...

> One thing the caller can simply do is to error out.

My first intuition is that we should do that more often.
That's what `calc.el` does.

In `read-char-choice-with-read-key`, OTOH we apparently decided to
consider that not as an error but to "transparently" continue execution
past the end of the kmacro by reading from the keyboard (which
is the behavior that your patch implements).

In the case of `dbus.el` my head hurts trying to understand what the code
really does, really should do, and how your patch changes its behavior.
First, the `dbus.el` code is problematic in any case because it relies
on "pre-reading" input events and pushing them back on
`unread-command-events`, which is sadly not 100% "a no-op".  We should
provide better primitives to handle such parallel input streams
without having to "read" all events and them push them back.

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.

> But if the caller wants an event, the only thing it can do is to touch
> executing_kbd_macro and set it nil explicitly (this is what subr.el
> does currently).

My head also hurts trying to understand what are the consequences of
doing that.

> We could also document this, but I feel something like
> "this function can return -1 and if it does, set executing_kbd_macro to
> nil and try again" is really a bit too unelegant (and it has more
> downsides, see next paragraph).

Agreed.  I think the problem is that we haven't clearly decided what
`execute-kbd-macro` is supposed to do.  The problem is that the way it's
expected to work implies a form of "nesting" such that at the end of
processing those events we return from the function, but sometimes
that's not what happens.  E.g. a kmacro can end in the middle of
a recursive edit.

Your patch brings is closer to a non-nesting semantics, a bit as if
`execute-kbd-macro` pushed all those events into the incoming input
queue and returned immediately.

Maybe that's what `execute-kbd-macro` should do, really: push all the
events into `unread-command-events` and return.  But then, how would we
do the boolean tests for `executing-kbd-macro` which we perform at
various places?  [ I guess we could label/wrap the events such that they
get executed in a special way (which let-binds `executing-kbd-macro` to
t?  ]

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.

> 3. commit: only change is that -1 is not propagated to lisp, other
> callers of read_char are unaffected as I explained above. (modulo
> Vlast_event_device and Vlast_event_frame -- which I can also still set
> if you want me to to).

"-1 is not propagated to lisp" doesn't say what happens in its stead.
What this does really is transparently continue reading input from the
keyboard when reaching the end of a kmacro.

I tend to agree that it's closer to the ideal behavior (even though
I still don't know what that is :-).


        Stefan






  parent reply	other threads:[~2024-01-13 17:40 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-05 21:19 bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc Tim Ruffing
2024-01-06  7:42 ` Eli Zaretskii
2024-01-06 14:32   ` Tim Ruffing
2024-01-13  9:39     ` Eli Zaretskii
2024-01-13 17:40     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors [this message]
2024-02-02 18:04       ` Tim Ruffing
2024-02-06 21:04         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-01 12:14         ` Tim Ruffing
2024-03-04 18:42           ` Tim Ruffing
2024-03-05 13:10             ` Eli Zaretskii
2024-03-05 16:45             ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-05 16:55               ` Eli Zaretskii
2024-03-05 17:57                 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-05 18:53                   ` Eli Zaretskii
2024-03-05 19:29                     ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-05 19:55                       ` Eli Zaretskii
2024-03-05 20:18                         ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-06 11:46                           ` Eli Zaretskii
2024-03-09 12:33                             ` Tim Ruffing
2024-03-09 18:08                               ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-09 18:37                                 ` Eli Zaretskii
2024-03-10  8:24                                   ` Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-03-10 14:48                                   ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-01-06  9:15 ` Andreas Schwab

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=jwvzfx9gp8n.fsf-monnier+emacs@gnu.org \
    --to=bug-gnu-emacs@gnu.org \
    --cc=68272@debbugs.gnu.org \
    --cc=crypto@timruffing.de \
    --cc=eliz@gnu.org \
    --cc=monnier@iro.umontreal.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).