track-changes and undo

track-changes and undo

[Joost Kremers]

Hi Stefan,

I noticed your track-changes package on ELPA the other day. Since I once (a long
time ago) wrote a package[1] that uses `before-change-functions` and
`after-change-functions` and that commits all the sins that you mention in your
package description, I thought I might try and base my package on yours.

I quickly found, though, that using your package, I was not able to distinguish
between "normal" changes and change made by undo. Currently, my code uses
`undo-in-progress` for that, but that doesn't work in the SIGNAL function passed
to `track-changes-register`, because the undo has obviously already finished
when that function is run.

So I'm wondering: is there a way to know if a change is made during undo? If
not, would it make sense to add it as an option?

TIA

Joost


BTW, feel free to answer on the list.




Footnotes:
[1]  https://github.com/joostkremers/criticmarkup-emacs

-- 
Joost Kremers
Life has its moments

Re: track-changes and undo

[Stefan Monnier]

Hi Joost,

> So I'm wondering: is there a way to know if a change is made during
> undo?  If not, would it make sense to add it as an option?

Very interesting point.  Indeed, this is missing.
Hmm... not completely sure how to expose that info through this new API.
Maybe a concrete example would help figure out how to make that work.
Do you remember how you used (or why you needed) `undo-in-progress`?


        Stefan

Re: track-changes and undo

[Joost Kremers]

Hi Stefan,

> Very interesting point.  Indeed, this is missing.
> Hmm... not completely sure how to expose that info through this new API.
> Maybe a concrete example would help figure out how to make that work.
> Do you remember how you used (or why you needed) `undo-in-progress`?

The short version: The package makes it easier to add CriticMarkup to a
document, i.e., markup to indicate additions, deletions, etc. There are
keybindings for the various markups (`{++...++}` for additions, `{--...--}` for
deletions, etc.).

The follow-changes mode tries to do that automatically: if you delete a word,
instead of actually deleting it, the word is enclosed in deletion markup.

What actually happens in the current implementation is that the text is deleted
(as per normal) and then reinserted in the buffer with deletion markup around it
(in a function in `after-change-functions`).

Similarly for an addition, instead of just inserting the new text, it is
inserted and enclosed in addition markup. That part happens in
`before-change-functions` in the current implementation.

Both hook functions start with `(unless undo-in-progress...)`, so when a change
is triggered by undo, they don't do anything special, and Emacs Does The
Right Thing. The Right Thing here being removing the mark-up, keeping the text
if the original action was a deletion and removing the text if the original
action was an addition. I assume the right thing happens because the changes
made to the buffer in the hook functions are recorded in the same change that
triggers the hooks.

It's probably easier to understand when you look at the code: 

https://github.com/joostkremers/criticmarkup-emacs/blob/master/cm-mode.el#L863

Hope this makes it clearer...

Joost



-- 
Joost Kremers
Life has its moments

Re: track-changes and undo

[Stefan Monnier]

[-- Attachment #1: Type: text/plain, Size: 1466 bytes --]

> The short version: The package makes it easier to add CriticMarkup to a
> document, i.e., markup to indicate additions, deletions, etc. There are
> keybindings for the various markups (`{++...++}` for additions, `{--...--}` for
> deletions, etc.).
>
> The follow-changes mode tries to do that automatically: if you delete a word,
> instead of actually deleting it, the word is enclosed in deletion markup.

Thanks,

Yes, that makes a lot of sense and checking `undo-in-progress` is
indispensable to make undo work.  With the current API (if you want to
use `track-changes`, that is) the best you can do is to use the
`:immediate` option so that your code gets called directly from the
`after-change-functions` where you can (still) see the
`undo-in-progress`.

The gain from `track-changes` is just to provide you with the "before"
string for deletions so it takes care of reading it in
`before-change-functions` and then providing it to you in the
`after-change-functions` (with the advantage that it detects/handles the
various corner cases where that pairing fails).

One other thing that you might have trouble to reproduce with
`track-changes` is the following test:

    (and (= beg (point-min)) (= end (point-max)))

that you have in `cm-before-change`.  I'm not completely sure what this
is for, tho.  Is it for `revert-buffer`?

[ I tend to do "destructive reads", so the patch below is the result of
  reading that part of your code.  ]


        Stefan

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: criticmarkup.diff --]
[-- Type: text/x-diff, Size: 2963 bytes --]

diff --git a/cm-mode.el b/cm-mode.el
index 43c705a..882f77d 100644
--- a/cm-mode.el
+++ b/cm-mode.el
@@ -862,43 +862,31 @@ substitutions, `d' for comments and highlights."
 
 ;;; Follow Changes
 
-(defvar cm-follow-changes nil
-  "Flag indicating whether follow changes mode is active.")
-(make-variable-buffer-local 'cm-follow-changes)
-
 (defvar cm-current-deletion nil
   "The deleted text in follow changes mode.
 The value is actually a list consisting of the text and a flag
 indicating whether the deletion was done with the backspace
 key.")
 
-(defun cm-follow-changes (&optional arg)
-  "Activate follow changes mode.
-If ARG is positive, activate follow changes mode, if ARG is 0 or
-negative, deactivate it.  If ARG is `toggle', toggle follow
-changes mode."
-  (interactive (list (or current-prefix-arg 'toggle)))
-  (let ((enable (if (eq arg 'toggle)
-                    (not cm-follow-changes)
-                  (> (prefix-numeric-value arg) 0))))
-    (if enable
-        (progn
-          (add-to-list 'before-change-functions 'cm-before-change t)
-          (add-to-list 'after-change-functions 'cm-after-change)
-          (setq cm-follow-changes t)
-          (message "Follow changes mode activated."))
-      (setq before-change-functions (delq 'cm-before-change before-change-functions))
-      (setq after-change-functions (delq 'cm-after-change after-change-functions))
-      (setq cm-follow-changes nil)
-      (message "Follow changes mode deactivated."))))
+(define-minor-mode cm-follow-changes  ;FIXME: Shouldn't it end in `mode'?
+  "Minor mode to follow changes."
+  :global nil
+  (if cm-follow-changes
+      (progn
+        (add-hook 'before-change-functions #'cm-before-change t t)
+        (add-hook 'after-change-functions #'cm-after-change nil t))
+    (remove-hook 'before-change-functions #'cm-before-change t)
+    (remove-hook after-change-functions #'cm-after-change t)))
 
 (defun cm-before-change (beg end)
   "Function to execute before a buffer change.
 BEG and END are the beginning and the end of the region to be
 changed."
   (unless (or undo-in-progress
+              ;; FIXME: What do you mean by "buffer switches"?
               (and (= beg (point-min)) (= end (point-max))))  ; This happens on buffer switches.
     (if (= beg end)  ; Addition.
+        ;; FIXME: There can be corner cases where point is not at beg/end.
         (cm-make-addition (cm-markup-at-point))
       ;; When the deletion was done with backspace, point is at end. We record
       ;; this in `cm-current-deletion' so we can position point correctly.
@@ -906,7 +894,7 @@ changed."
 
 (defun cm-after-change (beg end length)
   "Function to execute after a buffer change.
-This function marks deletions.  See cm-before-change for details.
+This function marks deletions.  See `cm-before-change' for details.
 BEG and END mark the region to be changed, LENGTH is the length
 of the affected text."
   (unless (or undo-in-progress

Re: track-changes and undo

[Joost Kremers]

On Mon, Apr 22 2024, Stefan Monnier wrote:
> The gain from `track-changes` is just to provide you with the "before"
> string for deletions so it takes care of reading it in
> `before-change-functions` and then providing it to you in the
> `after-change-functions` (with the advantage that it detects/handles the
> various corner cases where that pairing fails).

I think I'll try and see if I can make it work with the `:immediate` option.
It would of course mean that the buffer is modified inside
`after-change-functions`, which you warn against, but it looks like that's the
only way.

> One other thing that you might have trouble to reproduce with
> `track-changes` is the following test:
>
>     (and (= beg (point-min)) (= end (point-max)))
>
> that you have in `cm-before-change`.  I'm not completely sure what this
> is for, tho.  Is it for `revert-buffer`?

I honestly don't remember... Based on the comment, it looks like
`switch-to-buffer` triggers `before-change-functions`, but a) that doesn't make
much sense; and b) the code seems to work just fine without that line. (I even
fired up a Vagrant box with an old Ubuntu release with Emacs 24, which would be
the most recent version when I wrote that code).

> [ I tend to do "destructive reads", so the patch below is the result of
>   reading that part of your code.  ]

Thanks.

> +(define-minor-mode cm-follow-changes  ;FIXME: Shouldn't it end in `mode'?
> +  "Minor mode to follow changes."
> +  :global nil
> +  (if cm-follow-changes
> +      (progn
> +        (add-hook 'before-change-functions #'cm-before-change t t)
> +        (add-hook 'after-change-functions #'cm-after-change nil t))
> +    (remove-hook 'before-change-functions #'cm-before-change t)
> +    (remove-hook after-change-functions #'cm-after-change t)))

Making it a minor mode makes sense, of course. And don't ask me why I used
`add-to-list` instead of `add-hook`. 🤔
  

-- 
Joost Kremers
Life has its moments

Re: track-changes and undo

[Stefan Monnier]

>> The gain from `track-changes` is just to provide you with the "before"
>> string for deletions so it takes care of reading it in
>> `before-change-functions` and then providing it to you in the
>> `after-change-functions` (with the advantage that it detects/handles the
>> various corner cases where that pairing fails).
> I think I'll try and see if I can make it work with the `:immediate` option.
> It would of course mean that the buffer is modified inside
> `after-change-functions`, which you warn against,

It wouldn't be worse than what you have now since you also modify the
buffer from `before/after-change-functions`.

> but it looks like that's the only way.

In the general case it's tricky to postpone the buffer change to a safer
time, indeed.  In practice, tho, you should be able to distinguish "undo
commands" from all other commands (basically depending on whether they
do all their modifications with `undo-in-progress` or not) and then
ignore only the changes of undo commands: the result should be
good enough.

IOW something like:

    (track-changes-register #'cm-change-signal :immediate t)

    (defvar-local cm-change-pending nil)

    (defun cm-change-signal (id)
      (cond
       (cm-change-pending nil) ;; Nothing to do, we're already waiting.
       (undo-in-progress
        ;; Just ignore this undo change.
        (track-changes-fetch id #'ignore))
       (t
        (setq cm-change-pending
              (run-with-timer 0 nil #'cm-change-do id)))))
       
    (defun cm-change-do (id)
      (track-changes-fetch
       (lambda (beg end before)
         ..DO THE CriticalMarkup THING..
         ;; Ignore the changes we just made.
         (track-changes-fetch id #'ignore)
         (setq cm-change-pending nil))))

>> One other thing that you might have trouble to reproduce with
>> `track-changes` is the following test:
>>
>>     (and (= beg (point-min)) (= end (point-max)))
>>
>> that you have in `cm-before-change`.  I'm not completely sure what this
>> is for, tho.  Is it for `revert-buffer`?
>
> I honestly don't remember... Based on the comment, it looks like
> `switch-to-buffer` triggers `before-change-functions`, but a) that doesn't make
> much sense; and b) the code seems to work just fine without that line. (I even
> fired up a Vagrant box with an old Ubuntu release with Emacs 24, which would be
> the most recent version when I wrote that code).

Have you tried `M-x revert-buffer RET` (assuming the file and the
buffer aren't equal)?

> Making it a minor mode makes sense, of course.

It was already a minor mode, IMO, just one defined by hand instead of
using the dedicated macro.


        Stefan

Re: track-changes and undo

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: "'Help-Gnu-Emacs (help-gnu-emacs@gnu.org)'" <help-gnu-emacs@gnu.org>
> Date: Mon, 22 Apr 2024 19:04:10 -0400
> 
> >> The gain from `track-changes` is just to provide you with the "before"
> >> string for deletions so it takes care of reading it in
> >> `before-change-functions` and then providing it to you in the
> >> `after-change-functions` (with the advantage that it detects/handles the
> >> various corner cases where that pairing fails).
> > I think I'll try and see if I can make it work with the `:immediate` option.
> > It would of course mean that the buffer is modified inside
> > `after-change-functions`, which you warn against,
> 
> It wouldn't be worse than what you have now since you also modify the
> buffer from `before/after-change-functions`.
> 
> > but it looks like that's the only way.
> 
> In the general case it's tricky to postpone the buffer change to a safer
> time, indeed.  In practice, tho, you should be able to distinguish "undo
> commands" from all other commands (basically depending on whether they
> do all their modifications with `undo-in-progress` or not) and then
> ignore only the changes of undo commands: the result should be
> good enough.
> 
> IOW something like:
> 
>     (track-changes-register #'cm-change-signal :immediate t)
> 
>     (defvar-local cm-change-pending nil)
> 
>     (defun cm-change-signal (id)
>       (cond
>        (cm-change-pending nil) ;; Nothing to do, we're already waiting.
>        (undo-in-progress
>         ;; Just ignore this undo change.
>         (track-changes-fetch id #'ignore))
>        (t
>         (setq cm-change-pending
>               (run-with-timer 0 nil #'cm-change-do id)))))
>        
>     (defun cm-change-do (id)
>       (track-changes-fetch
>        (lambda (beg end before)
>          ..DO THE CriticalMarkup THING..
>          ;; Ignore the changes we just made.
>          (track-changes-fetch id #'ignore)
>          (setq cm-change-pending nil))))
> 
> >> One other thing that you might have trouble to reproduce with
> >> `track-changes` is the following test:
> >>
> >>     (and (= beg (point-min)) (= end (point-max)))
> >>
> >> that you have in `cm-before-change`.  I'm not completely sure what this
> >> is for, tho.  Is it for `revert-buffer`?
> >
> > I honestly don't remember... Based on the comment, it looks like
> > `switch-to-buffer` triggers `before-change-functions`, but a) that doesn't make
> > much sense; and b) the code seems to work just fine without that line. (I even
> > fired up a Vagrant box with an old Ubuntu release with Emacs 24, which would be
> > the most recent version when I wrote that code).
> 
> Have you tried `M-x revert-buffer RET` (assuming the file and the
> buffer aren't equal)?
> 
> > Making it a minor mode makes sense, of course.
> 
> It was already a minor mode, IMO, just one defined by hand instead of
> using the dedicated macro.

From the peanut gallery: if anything in this discussion should be
mentioned in the ELisp manual, where track-changes is documented,
please add those nits there.  For example, the whole issue of using
track-changes with undo should perhaps be mentioned there.

Thanks.

Re: track-changes and undo

[Joost Kremers]

On Mon, Apr 22 2024, Stefan Monnier wrote:
> In the general case it's tricky to postpone the buffer change to a safer
> time, indeed.  In practice, tho, you should be able to distinguish "undo
> commands" from all other commands (basically depending on whether they
> do all their modifications with `undo-in-progress` or not) and then
> ignore only the changes of undo commands: the result should be
> good enough.
>
> IOW something like:
[code snipped]

Thanks, I'll see if I can make it work.

>>> One other thing that you might have trouble to reproduce with
>>> `track-changes` is the following test:
>>>
>>>     (and (= beg (point-min)) (= end (point-max)))
>>>
>>> that you have in `cm-before-change`.  I'm not completely sure what this
>>> is for, tho.  Is it for `revert-buffer`?
>>
>> I honestly don't remember... Based on the comment, it looks like
>> `switch-to-buffer` triggers `before-change-functions`, but a) that doesn't make
>> much sense; and b) the code seems to work just fine without that line. (I even
>> fired up a Vagrant box with an old Ubuntu release with Emacs 24, which would be
>> the most recent version when I wrote that code).
>
> Have you tried `M-x revert-buffer RET` (assuming the file and the
> buffer aren't equal)?

I did, but that appears to resets `before-change-functions` (and presumably
`after-change-functions`). After `revert-buffer`, the function that
cm-follow-mode adds to `before-change-functions` is gone, and `cm-mode` itself
is disabled. So I doubt that was the reason for adding that line. (Also, I don't
think I would have written "buffer switches" in the comment if I actually meant
"reverting a buffer". But then again, one's past self can be quite
unpredictable...)

-- 
Joost Kremers
Life has its moments