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

Previous Next

To reply to this bug, email your comments to 70541 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Copley <rcopley <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: João Távora <joaotavora <at> gmail.com>
Subject: track-changes-mode logs warnings (with input method, in Eglot buffer)
Date: Tue, 23 Apr 2024 21:44:05 +0100

(Moved here from https://github.com/joaotavora/eglot/discussions/1389)
On master.

The warning:

 »  Warning (emacs): Missing/incorrect calls to
‘before/after-change-functions’!!
Details logged to ‘track-changes--error-log’

... is raised, while editing in an Eglot buffer, with an input method.

* Ensure clangd is on the path
* Start editing a scratch file like this in C++ mode:

int main() {
  const char * s = "";
}

* Start Eglot (M-x eglot RET)
* Enable TeX input method (C-u C-\ TeX RET)
* Start typing the superscript alphabet into the string (^ a ^ b ^ c ^ d ...)

How far you get before the warning pops up seems to depend on timing.
In a run of this experiment from emacs -Q on master just now, I got as
far as k.

track-changes--error-log value:

((#3="z.cpp"
     ((t track-changes--recover-from-error nil nil)
      (t track-changes-fetch
(#1=
 #s(track-changes--tracker :signal
   eglot--track-changes-signal :state
   #s(track-changes--state :beg 50
   :end 1
   :before
   nil :next
   nil)
   :nobefore nil :immediate nil)
 #f(compiled-function (beg end before) #<bytecode
      0xae090d111a>))
nil)
      (t eglot--track-changes-fetch (#1#) nil)
      (t eglot--signal-textDocument/didChange nil nil)
      (t run-hooks (eglot--document-changed-hook) nil)
      (t #2=#f(compiled-function () #<bytecode 0x1bb90600ea83b761>)
nil nil)
      (t apply (#2# nil) nil)
      (t timer-event-handler ([t 0 0 500000 nil #2# nil idle 0 nil])
nil))
     [101 #6=(nil . self-insert-command) 94 102 #7=
 (nil . self-insert-command) 94 103 #8=
 (nil . self-insert-command) 94 104 #9=
 (nil . self-insert-command) 94 105 #10=
 (nil . self-insert-command) 94 106 #11=
 (nil . self-insert-command) 94 107
 (nil . self-insert-command)])
 (#3#
  ((t track-changes--recover-from-error nil nil)
   (t track-changes-fetch
      (#1#
       #f(compiled-function (beg end before) #<bytecode 0xae090d111a>))
      nil)
   (t eglot--track-changes-fetch (#1#) nil)
   (t eglot--signal-textDocument/didChange nil nil)
   (t run-hooks (eglot--document-changed-hook) nil)
   (t #5=#f(compiled-function () #<bytecode 0x1bb90600ea83b761>) nil
      nil)
   (t apply (#5# nil) nil)
   (t timer-event-handler ([t 0 0 500000 nil #5# nil idle 0 nil]) nil)
   (t read-key-sequence (nil nil nil t) nil)
   (t quail-start-translation (94) nil)
   (t quail-input-method (94) nil))
  [(nil . self-insert-command) 94 101 #6# 94 102 #7# 94 103 #8# 94 104
   #9# 94 105 #10# 94 106 #11# 94]))

Lossage:

 M-x     ;; execute-extended-command
 e     ;; self-insert-command
 g     ;; self-insert-command
 l     ;; self-insert-command
 o     ;; self-insert-command
 t     ;; self-insert-command
 <return>     ;; minibuffer-complete-and-exit
 C-u     ;; universal-argument
 C-\     ;; toggle-input-method
 T     ;; self-insert-command
 e     ;; self-insert-command
 X     ;; self-insert-command
 <return>     ;; minibuffer-complete-and-exit
 C-s     ;; isearch-forward
 "     ;; isearch-printing-char
 <with-input-method> ;; isearch-with-input-method
 <return>     ;; isearch-exit
 ^ a     ;; self-insert-command
 ^ b     ;; self-insert-command
 ^ c     ;; self-insert-command
 ^ d     ;; self-insert-command
 ^ e     ;; self-insert-command
 ^ f     ;; self-insert-command
 ^ g     ;; self-insert-command
 ^ h     ;; self-insert-command
 ^ i     ;; self-insert-command
 ^ j     ;; self-insert-command
 ^ k     ;; self-insert-command
 C-h l     ;; view-lossage

(Stefan notes:

If you want to silence these messages until the problem is fixed, you
can `(setq track-changes-record-errors nil)`.)

Message #8 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Richard Copley <rcopley <at> gmail.com>
Cc: 70541 <at> debbugs.gnu.org,
 João Távora <joaotavora <at> gmail.com>
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Tue, 23 Apr 2024 23:14:16 -0400

[Message part 1 (text/plain, inline)]

> * Ensure clangd is on the path
> * Start editing a scratch file like this in C++ mode:
>
> int main() {
>   const char * s = "";
> }
>
> * Start Eglot (M-x eglot RET)
> * Enable TeX input method (C-u C-\ TeX RET)
> * Start typing the superscript alphabet into the string (^ a ^ b ^ c ^ d ...)
>
> How far you get before the warning pops up seems to depend on timing.

To make it reliable, here's what you need to do after enabling the TeX
input method:

    type "quickly"; foo^
    wait a couple seconds

and you should get the 

     »  Warning (emacs): Missing/incorrect calls to ‘before/after-change-functions’!!
    Details logged to ‘track-changes--error-log’

The problem is that Quail inserts an underlined `^` in the buffer to
show you the part of the input that's already been typed and it does so
stealthily (i.e. within `with-silent-modifications`), which implies that
the `before/after-change-functions` have not been called, and as
a consequence the size of the buffer is not the one that track-changes
expects (and the content of the buffer doesn't corresponds to what Eglot
will send to the LSP server based on the changes it has witnessed,
which can cause errors since Eglot has to send buffer positions and
those may not mean the same any more for the LSP server).

I suggest the patch below (the second hunk is unrelated, I just bumped
into it while tracking this bug).


        Stefan

[quail.patch (text/x-diff, inline)]

diff --git a/lisp/international/quail.el b/lisp/international/quail.el
index 48d2ccb8828..cb7aa89b252 100644
--- a/lisp/international/quail.el
+++ b/lisp/international/quail.el
@@ -1334,9 +1334,13 @@ quail-input-method
     (quail-setup-overlays (quail-conversion-keymap))
     (with-silent-modifications
       (unwind-protect
-	  (let ((input-string (if (quail-conversion-keymap)
+	  (let* (;; `with-silent-modifications' inhibits the modification
+                 ;; hooks, but that's a part of `with-silent-modifications'
+                 ;; we don't actually want here (bug#70541).
+		 (inhibit-modification-hooks nil)
+		 (input-string (if (quail-conversion-keymap)
 				  (quail-start-conversion key)
-				(quail-start-translation key))))
+				  (quail-start-translation key))))
 	    (setq quail-guidance-str "")
 	    (when (and (stringp input-string)
 		       (> (length input-string) 0))
@@ -1871,10 +1875,9 @@ quail-delete-last-char
 
 (defsubst quail-point-in-conversion-region ()
   "Return non-nil value if the point is in conversion region of Quail mode."
-  (let (start pos)
-    (and (setq start (overlay-start quail-conv-overlay))
-	 (>= (setq pos (point)) start)
-	 (<= pos (overlay-end quail-conv-overlay)))))
+  (let ((start (overlay-start quail-conv-overlay)))
+    (and start
+	 (<= start (point) (overlay-end quail-conv-overlay)))))
 
 (defun quail-conversion-backward-char ()
   (interactive)

Message #11 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Wed, 24 Apr 2024 10:12:35 +0300

> Cc: 70541 <at> debbugs.gnu.org,
>  João Távora <joaotavora <at> gmail.com>
> Date: Tue, 23 Apr 2024 23:14:16 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> The problem is that Quail inserts an underlined `^` in the buffer to
> show you the part of the input that's already been typed and it does so
> stealthily (i.e. within `with-silent-modifications`), which implies that
> the `before/after-change-functions` have not been called, and as
> a consequence the size of the buffer is not the one that track-changes
> expects (and the content of the buffer doesn't corresponds to what Eglot
> will send to the LSP server based on the changes it has witnessed,
> which can cause errors since Eglot has to send buffer positions and
> those may not mean the same any more for the LSP server).
> 
> I suggest the patch below (the second hunk is unrelated, I just bumped
> into it while tracking this bug).

Are you sure we want buffer-change hooks to be called here?  Quail
intentionally hides some of the modifications it does, because it many
times replaces the inserted text with something else, and from the
Emacs Lisp program's POV only the final input is the actual
"insertion" Emacs should know about.

So I'm not sure we should install this, as it could break something
elsewhere.  Aren't there any alternatives to this?  More generally,
why should Eglot care about these low-level details of Quail?

Message #14 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Wed, 24 Apr 2024 10:26:40 -0400

> Are you sure we want buffer-change hooks to be called here?

Adding/removing chars from the buffer without running those hooks breaks
all kinds of assumptions.  It can be acceptable to do it in a very
temporary way, but Quail doesn't do it temporarily: the whole
redisplay+timers+filters gets run in the middle of the input of any
multi-key entry like `^a`, so the "temporary" state is very
much exposed.

> Quail intentionally hides some of the modifications it does, because
> it many times replaces the inserted text with something else, and from
> the Emacs Lisp program's POV only the final input is the actual
> "insertion" Emacs should know about.

I'm not sure why it's important to hide the intermediate steps,
especially since they're not very well hidden (as evidenced by this bug
report, and by the fact that font-lock is also triggered every time the
transient display is modified).

Note that when users use Abbrev instead to turn \lambda into λ, the
intermediate steps are not hidden, and that's not been a problem.

> So I'm not sure we should install this, as it could break something
> elsewhere.  Aren't there any alternatives to this?

We could change Quail so it refrains from temporarily modifying the
buffer, using an `after/before-string` on an overlay instead.
It'd be a fairly significant change in `quail.el` and would probably
come with its own share of problems.

> More generally,
> why should Eglot care about these low-level details of Quail?

Because Eglot syncs up with the LSP server via a timer, so it sees the
buffer with an extra "^" char in it.


        Stefan

Message #17 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Wed, 24 Apr 2024 18:42:13 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: rcopley <at> gmail.com,  70541 <at> debbugs.gnu.org,  joaotavora <at> gmail.com
> Date: Wed, 24 Apr 2024 10:26:40 -0400
> 
> > Are you sure we want buffer-change hooks to be called here?
> 
> Adding/removing chars from the buffer without running those hooks breaks
> all kinds of assumptions.  It can be acceptable to do it in a very
> temporary way, but Quail doesn't do it temporarily: the whole
> redisplay+timers+filters gets run in the middle of the input of any
> multi-key entry like `^a`, so the "temporary" state is very
> much exposed.

It's exposed to some parts of Emacs, but not to others.

> > Quail intentionally hides some of the modifications it does, because
> > it many times replaces the inserted text with something else, and from
> > the Emacs Lisp program's POV only the final input is the actual
> > "insertion" Emacs should know about.
> 
> I'm not sure why it's important to hide the intermediate steps,

Because Quail emulates keyboard input.  Conceptually, everything that
the user types into a buffer that Quail processes and replaces "did
not happen", only the final insertion of the produced character(s) are
real.

> especially since they're not very well hidden (as evidenced by this bug
> report, and by the fact that font-lock is also triggered every time the
> transient display is modified).

Actually, it's hidden quite well, it's just that Eglot gets confused
because it looks at stuff it isn't supposed to see ;-)

> Note that when users use Abbrev instead to turn \lambda into λ, the
> intermediate steps are not hidden, and that's not been a problem.

Abbrev cannot guide the user regarding the next steps when an initial
string typed by user has several possible candidates for further
input.  So Abbrev basically solves a simpler problem.

> > So I'm not sure we should install this, as it could break something
> > elsewhere.  Aren't there any alternatives to this?
> 
> We could change Quail so it refrains from temporarily modifying the
> buffer, using an `after/before-string` on an overlay instead.
> It'd be a fairly significant change in `quail.el` and would probably
> come with its own share of problems.

I'm not at all sure the solution should be in Quail.

> > More generally,
> > why should Eglot care about these low-level details of Quail?
> 
> Because Eglot syncs up with the LSP server via a timer, so it sees the
> buffer with an extra "^" char in it.

So maybe Eglot should learn that when it sees this and a Quail input
is in progress, it should pretend it didn't see anything?  IOW, why
not solve this in Eglot instead?  It's Eglot that makes incorrect
assumptions about what happens, so let's teach Eglot how to do better.

Message #20 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Wed, 24 Apr 2024 15:02:47 -0400

> Actually, it's hidden quite well, it's just that Eglot gets confused
> because it looks at stuff it isn't supposed to see ;-)

Who's supposed to see it and who isn't?
Tree-sitter is told about it, font-lock is told about it, why not others
via the usual mechanism?  What's different about Eglot?

>> Note that when users use Abbrev instead to turn \lambda into λ, the
>> intermediate steps are not hidden, and that's not been a problem.
> Abbrev cannot guide the user regarding the next steps when an initial
> string typed by user has several possible candidates for further
> input.  So Abbrev basically solves a simpler problem.

When combined with appropriate completion tables and UIs, abbrev also
guide the user, so it's not that different.
Also, I fail to see how that's relevant.

The buffer state is modified by Quail.  It's somewhat temporary but
there's still a lot that can happen during that temporary state.

> So maybe Eglot should learn that when it sees this and a Quail input
> is in progress, it should pretend it didn't see anything?

That seems very yucky.  Suddenly packages like Eglot, lsp-mode, crdt,
TeXpresso, CriticalMarkup, ... need to learn about that special
interaction with Quail.  And how are they going to deal with it?

The only sane way I can see to deal with it would be for Quail to
provide a way to temporarily return to the "real" state (e.g. deleting
the `^`) and then to get back to the temporary state (i.e. re-insert the
`^`).

This is pretty ugly in my book, sounds like workarounds on top
of workarounds.  Can we try the patch I suggested first?
It makes more code normal instead of adding more special code to deal
with special code, so if that works it's a much nicer option.

> IOW, why not solve this in Eglot instead?  It's Eglot that makes
> incorrect assumptions about what happens, so let's teach Eglot how to
> do better.

I don't think these are incorrect assumptions because there's not much
else it could do.  If packages like Eglot can't do their work correctly
without knowing about Quail, I think it means we have a poor API.


        Stefan

Message #23 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Wed, 24 Apr 2024 22:24:49 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: rcopley <at> gmail.com,  70541 <at> debbugs.gnu.org,  joaotavora <at> gmail.com
> Date: Wed, 24 Apr 2024 15:02:47 -0400
> 
> > Actually, it's hidden quite well, it's just that Eglot gets confused
> > because it looks at stuff it isn't supposed to see ;-)
> 
> Who's supposed to see it and who isn't?

Those who need to know are supposed to see, the rest aren't ;-)

Seriously, though:

> The buffer state is modified by Quail.  It's somewhat temporary but
> there's still a lot that can happen during that temporary state.

It isn't just temporary: it's a change that "isn't supposed to exist".
It's just a side effect of how Quail is implemented.

> > So maybe Eglot should learn that when it sees this and a Quail input
> > is in progress, it should pretend it didn't see anything?
> 
> That seems very yucky.  Suddenly packages like Eglot, lsp-mode, crdt,
> TeXpresso, CriticalMarkup, ... need to learn about that special
> interaction with Quail.

It isn't suddenly, it's because you switched Eglot to the new
track-changes method, right?  It worked fine before that, with the
same Quail, right?  Or am I missing something?

And why "yucky"?  This is the same Quail in all those cases, and the
same track-changes machinery.  So if Quail gets in the way of
track-changes, how about if Quail sets some flag which tells the
application level that changes in progress are to be ignored?  If we
can handle that as part of track-changes, fine; otherwise, Eglot and
the rest that use track-changes will have to ignore that on the
application level.  Doesn't sound yucky to me.

> And how are they going to deal with it?

By ignoring the changes performed while that flag is set.

> The only sane way I can see to deal with it would be for Quail to
> provide a way to temporarily return to the "real" state (e.g. deleting
> the `^`) and then to get back to the temporary state (i.e. re-insert the
> `^`).

Why is it not enough to ignore the changes?

> This is pretty ugly in my book, sounds like workarounds on top
> of workarounds.  Can we try the patch I suggested first?

We could try, but how many times do we need to make changes like that
in Quail that bite us elsewhere before we learn the simple truth that
we shouldn't try that anymore?

> It makes more code normal instead of adding more special code to deal
> with special code, so if that works it's a much nicer option.

Once again: Quail uses with-silent-modifications for a reason.  You
are basically suggesting to ignore that reason, and hwy? because it
makes the solution much easier?  A simpler solution is only preferable
when we know for sure it is correct, and here we are just guessing,
since we don't really have a clear idea what will that cause in other
cases.

> > IOW, why not solve this in Eglot instead?  It's Eglot that makes
> > incorrect assumptions about what happens, so let's teach Eglot how to
> > do better.
> 
> I don't think these are incorrect assumptions because there's not much
> else it could do.  If packages like Eglot can't do their work correctly
> without knowing about Quail, I think it means we have a poor API.

I'm suggesting a way for Quail to tell those applications that it is
in the process of making changes that should be ignored.  If such a
mechanism is possible, I think those applications could DTRT without
much effort.  Or am I missing something?

Message #26 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Richard Copley <rcopley <at> gmail.com>, 70541 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Wed, 24 Apr 2024 21:53:08 +0100

[Message part 1 (text/plain, inline)]

On Wed, Apr 24, 2024, 20:24 Eli Zaretskii <eliz <at> gnu.org> wrote:

> .
>
> It isn't suddenly, it's because you switched Eglot to the new
> track-changes method, right?  It worked fine before that, with the
> same Quail, right?  Or am I missing something?
>

I haven't much to add to this discussion where I'm being Cc'ed except that
I generally agree with Stefan's stance and fix and that I am fairly sure
this has always a problem before Stefan's track-changes.el framework, only
that we did not know about it because the raw usage eglot.el made of b-c-f
and a-c-f didn't sanity-check anything. You just got subtly wrong server
behaviour because it was being misinformed, and ended up reconnecting to
fix these things.

>
>

[Message part 2 (text/html, inline)]

Message #29 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Sun, 28 Apr 2024 14:21:19 -0400

>> The buffer state is modified by Quail.  It's somewhat temporary but
>> there's still a lot that can happen during that temporary state.
> It isn't just temporary: it's a change that "isn't supposed to exist".
> It's just a side effect of how Quail is implemented.

But what does it mean concretely?  In which sense is it supposed not
to exist?
And more to the point, what makes it important to hide those changes?

>> > So maybe Eglot should learn that when it sees this and a Quail input
>> > is in progress, it should pretend it didn't see anything?
>> 
>> That seems very yucky.  Suddenly packages like Eglot, lsp-mode, crdt,
>> TeXpresso, CriticalMarkup, ... need to learn about that special
>> interaction with Quail.
>
> It isn't suddenly, it's because you switched Eglot to the new
> track-changes method, right?

No, the problem was there before just as well.  The difference is that
`track-changes.el` is more careful both to detect and to report
such problems.

> It worked fine before that, with the same Quail, right?

Yes and no: in some cases the old code failed to detect the problem and
that could result in broken behavior.  When the old and new code detect
the problem, they both "work fine" in the sense that the behavior is
correct but at an extra cost because after detecting the inconsistency
Eglot does a full resync with the server.

> Or am I missing something?

It also works correctly with the new code.  The difference is that we
report it (notice the `Subject:` says "warning").
[ Note also that `track-changes.el` does not warn about it when running
  in a released version of Emacs (see `track-changes-record-errors`),
  because I assume it's less useful.  ]

>> And how are they going to deal with it?
> By ignoring the changes performed while that flag is set.

Define "ignore".
The change are there.  `point`, `point-max`, `current-column`,
etc... are affected.

>> This is pretty ugly in my book, sounds like workarounds on top
>> of workarounds.  Can we try the patch I suggested first?
> We could try, but how many times do we need to make changes like that
> in Quail that bite us elsewhere before we learn the simple truth that
> we shouldn't try that anymore?

Which other times are you referring to?


        Stefan

Message #32 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Mon, 29 Apr 2024 09:09:23 +0300

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: rcopley <at> gmail.com,  70541 <at> debbugs.gnu.org,  joaotavora <at> gmail.com
> Date: Sun, 28 Apr 2024 14:21:19 -0400
> 
> >> The buffer state is modified by Quail.  It's somewhat temporary but
> >> there's still a lot that can happen during that temporary state.
> > It isn't just temporary: it's a change that "isn't supposed to exist".
> > It's just a side effect of how Quail is implemented.
> 
> But what does it mean concretely?  In which sense is it supposed not
> to exist?
> And more to the point, what makes it important to hide those changes?

The actual change is the character(s) inserted when the Quail input
function completes its job.  Everything else is ephemeral, and Quail
deletes it as part of its job.  So those insertions followed by
deletions are not meant to change the buffer, and should therefore be
ignored.

> >> > So maybe Eglot should learn that when it sees this and a Quail input
> >> > is in progress, it should pretend it didn't see anything?
> >> 
> >> That seems very yucky.  Suddenly packages like Eglot, lsp-mode, crdt,
> >> TeXpresso, CriticalMarkup, ... need to learn about that special
> >> interaction with Quail.
> >
> > It isn't suddenly, it's because you switched Eglot to the new
> > track-changes method, right?
> 
> No, the problem was there before just as well.  The difference is that
> `track-changes.el` is more careful both to detect and to report
> such problems.
> 
> > It worked fine before that, with the same Quail, right?
> 
> Yes and no: in some cases the old code failed to detect the problem and
> that could result in broken behavior.  When the old and new code detect
> the problem, they both "work fine" in the sense that the behavior is
> correct but at an extra cost because after detecting the inconsistency
> Eglot does a full resync with the server.
> 
> > Or am I missing something?
> 
> It also works correctly with the new code.  The difference is that we
> report it (notice the `Subject:` says "warning").
> [ Note also that `track-changes.el` does not warn about it when running
>   in a released version of Emacs (see `track-changes-record-errors`),
>   because I assume it's less useful.  ]

So this is actually a non-issue?

> >> And how are they going to deal with it?
> > By ignoring the changes performed while that flag is set.
> 
> Define "ignore".

Do nothing and wait for the flag to become reset again.

> The change are there.  `point`, `point-max`, `current-column`,
> etc... are affected.

Why does Eglot need to react to those changes immediately when they
happen?

> >> This is pretty ugly in my book, sounds like workarounds on top
> >> of workarounds.  Can we try the patch I suggested first?
> > We could try, but how many times do we need to make changes like that
> > in Quail that bite us elsewhere before we learn the simple truth that
> > we shouldn't try that anymore?
> 
> Which other times are you referring to?

For example, we made several changes in Quail and elsewhere to fix the
recording of characters and the related keyboard-macro issues.  It
took quite some effort to get that right and fix all the regressions
the initial too-naïve attempts caused elsewhere.

So I'd very much prefer that Quail signaled to applications that it's
in the middle of handling some complex input, and that applications
which track changes ignored the changes made during this period.

We might already have variables in Quail which could be used as such
flags: quail-translating, quail-converting, quail-current-str, and
quail-guidance-str, to name a few candidates.  Could any of them be
used for this purpose?

Message #35 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Mon, 29 Apr 2024 09:28:47 +0100

[Message part 1 (text/plain, inline)]

On Mon, Apr 29, 2024 at 7:09 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> initial too-naïve attempts caused elsewhere.
>
> So I'd very much prefer that Quail signaled to applications that it's
> in the middle of handling some complex input, and that applications
> which track changes ignored the changes made during this period.

This idea is good, if I understand it correctly, though I would
prefer if Eglot interfaced only with track-changes.el, and
it would tell it that it should momentarily halt reports
of changes to the server.

Can someone clarify in a simple example exactly what Eglot tells
the LSP server as someone is inputting something with the Quail.
I need to understand exactly where the "lie" is happening.

João

[Message part 2 (text/html, inline)]

Message #38 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: João Távora <joaotavora <at> gmail.com>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Mon, 29 Apr 2024 08:36:37 +0000

João Távora <joaotavora <at> gmail.com> writes:

> On Mon, Apr 29, 2024 at 7:09 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>>
>> initial too-naïve attempts caused elsewhere.
>>
>> So I'd very much prefer that Quail signaled to applications that it's
>> in the middle of handling some complex input, and that applications
>> which track changes ignored the changes made during this period.

Would adding `combine-change-calls' to Quail be an option?
AFAIU, calling the change hooks in the middle of quail input is the main
culprit of the problems we are discussing here.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Message #41 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Mon, 29 Apr 2024 11:45:18 +0300

> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org,
>  Stefan Monnier <monnier <at> iro.umontreal.ca>
> Date: Mon, 29 Apr 2024 08:36:37 +0000
> 
> João Távora <joaotavora <at> gmail.com> writes:
> 
> > On Mon, Apr 29, 2024 at 7:09 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> >>
> >> initial too-naïve attempts caused elsewhere.
> >>
> >> So I'd very much prefer that Quail signaled to applications that it's
> >> in the middle of handling some complex input, and that applications
> >> which track changes ignored the changes made during this period.
> 
> Would adding `combine-change-calls' to Quail be an option?

I don't think so, since more than a single function is involved
(AFAIU).

> AFAIU, calling the change hooks in the middle of quail input is the main
> culprit of the problems we are discussing here.

The original Quail code inhibits the modification hooks, so this
cannot be the culprit, can it?

Message #44 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Mon, 29 Apr 2024 09:57:45 +0100

[Message part 1 (text/plain, inline)]

On Mon, Apr 29, 2024 at 7:09 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

>
> > It also works correctly with the new code.  The difference is that we
> > report it (notice the `Subject:` says "warning").
> > [ Note also that `track-changes.el` does not warn about it when running
> >   in a released version of Emacs (see `track-changes-record-errors`),
> >   because I assume it's less useful.  ]
>
> So this is actually a non-issue?
>

FTR I just managed to crash clangd after following the recipe. Eglot
detects the crash and reconnects automatically.  In my book, if Eglot's
misreporting causes a server to crash, it is an issue in Eglot (even
if it's also a server-side issue that a client's misreporting causes a
crash).

João

[Message part 2 (text/html, inline)]

Message #47 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Mon, 29 Apr 2024 19:45:39 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> AFAIU, calling the change hooks in the middle of quail input is the main
>> culprit of the problems we are discussing here.
>
> The original Quail code inhibits the modification hooks, so this
> cannot be the culprit, can it?

You are right.
The problem here is more like
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51766

Then, I tend to agree with Stefan that the current behavior is not right
- a lot of things are happening during the intermediate Quail input,
including, fontification, for example (and that runs custom Elisp!). I
see not how the transient input
to-be-replaced-by-quail-in-the-near-future can be ignored in such
scenario.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Message #50 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: João Távora <joaotavora <at> gmail.com>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Mon, 29 Apr 2024 16:27:42 -0400

> This idea is good, if I understand it correctly, though I would
> prefer if Eglot interfaced only with track-changes.el, and
> it would tell it that it should momentarily halt reports
> of changes to the server.

I guess we could add a function `track-change-inconsistent-state-p`
which Eglot could consult before calling `track-changes-fetch` and if
that returns non-nil, Eglot could reschedule the update to "later".

But that would just be a kludge usable by some packages like Eglot
because they have the luxury to reschedule.  For example, if some code
is run (e.g. via font-lock) which uses `syntax-ppss` while Quail's extra
chars are inserted, then `syntax-ppss` can't reschedule, and it may
record the resulting state in its cache after which
`inhibit-modification-hooks` may in turn prevent the cache from being
properly flushed when Quail removes its extra chars.

Removing the `inhibit-modification-hooks` would be a much
cleaner solution.  I asked Kenichi why he added that binding (back in
2005), but he hasn't replied yet.

> Can someone clarify in a simple example exactly what Eglot tells
> the LSP server as someone is inputting something with the Quail.
> I need to understand exactly where the "lie" is happening.

Here's the scenario: the user wants to insert `xᵃ` using the TeX input
method (the same problem can occur with other methods, of course), so
Emacs will receive the following inputs:

    x ^ a

but let's assume the user pauses between `^` and `a`.
At this point, the `before/after-change-functions` have been called to
announce the insertion of `x` and nothing more, but the buffer also has
an (unannounced) `^` inserted at point.

So the buffer's content as seen by `before/after-change-functions` (and
hence by the LSP server) is out of sync because of this additional `^`.

Since Quail does not call the `before/after-change-functions`, Eglot
doesn't specifically send Quail's input to the LSP server: in the above
scenario, `eglot--signal-textDocument/didChange` is called because the
`before/after-change-functions` was called in response to the previous
`x`.


        Stefan

Message #53 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, joaotavora <at> gmail.com
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Mon, 29 Apr 2024 16:50:07 -0400

>> >> The buffer state is modified by Quail.  It's somewhat temporary but
>> >> there's still a lot that can happen during that temporary state.
>> > It isn't just temporary: it's a change that "isn't supposed to exist".
>> > It's just a side effect of how Quail is implemented.
>> 
>> But what does it mean concretely?  In which sense is it supposed not
>> to exist?
>> And more to the point, what makes it important to hide those changes?
>
> The actual change is the character(s) inserted when the Quail input
> function completes its job.  Everything else is ephemeral, and Quail
> deletes it as part of its job.

While we can think of it as ephemeral, the fact is that it lasts through
redisplay, running timers, etc...

So it's not at all obvious why it should be treated differently from the
case where the users type something and then hit undo because they
realize they made a mistake, or when they type a short string followed
by `M-x expand-abbrev`.

There are many other forms of "ephemeral" changes, where we don't
inhibit modification hooks.  E.g. in `replace-match` we sometimes insert
the replacement and then capitalize it.  The intermediate
non-capitalized string is very much ephemeral: much more ephemeral than
Quail's (there's *very* little code run between the two operations).
Yet we call `before/after-change-functions` separately for both changes,
thereby exposing the intermediate state.

> So those insertions followed by deletions are not meant to change the
> buffer,

Yet they do.

> and should therefore be ignored.

I don't understand the "therefore".

Also, ignored by what?
Clearly they should not be ignored by redisplay (since the whole
purpose is to give guidance to the users).

Also, currently they're not ignored by `font-lock`.  Is that a bug?

>> It also works correctly with the new code.  The difference is that we
>> report it (notice the `Subject:` says "warning").
>> [ Note also that `track-changes.el` does not warn about it when running
>>   in a released version of Emacs (see `track-changes-record-errors`),
>>   because I assume it's less useful.  ]
> So this is actually a non-issue?

It's a performance bug.

>> The change are there.  `point`, `point-max`, `current-column`,
>> etc... are affected.
> Why does Eglot need to react to those changes immediately when they
> happen?

It doesn't: it reacts to the changes in an idle timer.

But the idle timer can see Quail's intermediate state, exactly because
it is not nearly as ephemeral as would be needed for
`with-silent-modifications` to be acceptable.

> So I'd very much prefer that Quail signaled to applications that it's
> in the middle of handling some complex input, and that applications
> which track changes ignored the changes made during this period.

Eglot could pause its work during Quail input.  But note that with some
input methods, being in the middle of a Quail input can be very
common, so it's not a great solution because it could unduly delay the
work of Eglot.

And of course, other packages may not have that luxury.

> We might already have variables in Quail which could be used as such
> flags: quail-translating, quail-converting, quail-current-str, and
> quail-guidance-str, to name a few candidates.  Could any of them be
> used for this purpose?

I definitely would not want Track-Changes to lookup a Quail variable.
We should instead use/introduce some package-neutral var (which other
packages could use).  But I'd first like to know why it is that Quail
needs `inhibit-modification-hooks` to make sure we really need such
a workaround.
[ BTW, let-binding some dynbound variable would not do since
  track-changes would not see it if called from a separate thread.
  It would have to be a buffer-local var.  ]


        Stefan

Message #56 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: João Távora <joaotavora <at> gmail.com>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Fri, 03 May 2024 13:27:45 -0400

> I guess we could add a function `track-change-inconsistent-state-p`
> which Eglot could consult before calling `track-changes-fetch` and if
> that returns non-nil, Eglot could reschedule the update to "later".

I just pushed a patch to `master` which does that.
Richard, can you confirm it fixes the problem on your end?

The problem of Quail binding
`inhibit-modification-hooks` remains, of course.  We should fix it
because it can affect many more things: all the code run in the middle
of a Quail input sequence (e.g. timers and process filters) are
effected, not just because they may see an "inconsistent" buffer state
but because the changes they make to buffers will themselves be
affected by the `inhibit-modification-hooks` binding.


        Stefan

Message #59 received at 70541 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: João Távora <joaotavora <at> gmail.com>
Cc: rcopley <at> gmail.com, 70541 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#70541: track-changes-mode logs warnings (with input method,
 in Eglot buffer)
Date: Fri, 03 May 2024 16:56:05 -0400

> I just pushed a patch to `master` which does that.
> Richard, can you confirm it fixes the problem on your end?

Hmm... apparently my machine failed to read my mind and dutifully used
the Emacs I told it to use rather than the one I was working on, so
no wonder the tests worked fine.

I have pushed further fixes to my typo-laden code.
It seems to work OK for me now, but I'd like to hear other people's
experience with it.


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.