bug#70541: track-changes-mode logs warnings (with input method, in Eglot buffer)

[Stefan Monnier]

> I don't know that this is a "performance bug".  I mean, misleading the
> server, crashing it is only such a thing if one squints very hard.

🙂

> So while may be "significantly simpler" solution isn't in the cards,
> I still think some simplification can happen (I don't understand
> why post-command-hook needs to be used for example).

Here's the reason I used `post-command-hook`:

As you know, the affected code is run from an idle timer, and the change
I introduced is to try and detect when the timer is run at an
"inconvenient time".  So my code delays the execution to "later".
In general calling `run-with-idle-timer` from an idle timer is a bit
tricky: if the wait time is smaller (or equal) than the current timer,
then the new timer will fire immediately.  Furthermore, in order for the
buffer state to change, we generally want to wait for the end of the
current idle time, so we want to fire a new timer that will run at the
*next* idle time.  I used `post-command-hook` to wait for the end of the
current idle time.

Arguably, we could/should move this trick to `timer.el` to provide
a `run-with-idle-timer-but-not-now` kind of function.
But AFAICT even using "insider info" we'd still have to use
a contraption like `post-command-hook`, or otherwise we'd have to make
more pervasive changes to the way timers are implemented.  🙁

> Anyway,  I would need to  see the quil failure scenario encoded as a
> test in eglot-tests.el so I can have a clear shot at it.  Should be
> possible, no?

Possible yes, but probably not very easy.  You can probably start with

    (insert "a")
    (with-silent-modifications
      (insert "^")
      (sit-for (* 2 eglot-send-changes-idle-time))
      (delete-region (1- (point)) (point)))

The first `insert` is there to make sure Eglot is told that there's been
some change, so it arms the idle-timer, and then the insert+wait+delete mimics
the Quail behavior.

> Thanks. That's great. It would also help to document your new
> eglot--track-changes-fetch.  I'm afraid I've lost track of how the
> code is supposed to work after track-changes.el
>
> * why is :emacs-messup still a thing?

Because `track-changes` can't hide the errors.  The only thing it can do
is do the detection for you and send you a "clean" error indication.  🙁

> By the way did :emacs-messup solve this very problem too via
> a full resync?

Yes/no.  With the old code, I think in the Quail situation your code
would *not* have detected the inconsistency (so no `:emacs-messup`) and
it would have occasionally sent inconsistent info the LSP server.

`track-changes` works a tiny bit harder to detect inconsistencies, so in
this Quail case it does detect it, which means it sends `:error` which
Eglot turns into `:emacs-messup` which means the LSP server receives
consistent info *but* it's costly and that info is equivalent to if the
user had manually typed an "^" followed by deleting that "^" and
replacing it with a `ᵃ` (for example), so the LSP server will sometimes
see the buffer with a `^` char which the user may never have meant to
include in the buffer.

And with the new code, Eglot postpones sending the state of the buffer
until we're not in the middle of a Quail insertion (so track-changes
never sends a `:error`, we don't use any `:emacs-messup`, and the LSP
server never sees that `^` either).
[ "never" here applies only to the Quail case, there can still be cases
  where we get `:error`, sadly.  ]

> * what exactly does the local eglot--track-changes do and why would
> it be re-registered in the same buffer (why isn't the typical add-hook
> enough).

Every client of `track-changes` can consume the changes at its own
rhythm, so track-changes needs to keep track of which changes each
client has already consumed.

> * There seems to be a track-changes.el signalling function and the
> longstanding LSP signalling function that informs the server of
> things.  Why can't it be the same function, i.e. why can't Eglot
> tell track-changes.el to call Eglot's function when track-changes.el
> knows it's a safe time to call such a function?  The flow of
> information via the eglot--recent-changes variable is complicated
> and would seem not worth bringing in track-changes.el at all.
> Is it only to accommodate Eglot versions when track-changes.el
> isn't available or is there some deeper reason?

We could turn `eglot--recent-changes` into a boolean, or probably even
eliminate it altogether, yes.  The reason I kept it is that if we don't
have it, then all the changes since the last
`eglot--signal-textDocument/didChange` get combined into a single
change, which can become quite large if the changes are spread out.


        Stefan