bug#70077: An easier way to track buffer changes

[Ihor Radchenko <yantar92@posteo.net>]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> +(cl-defstruct (track-changes--state
> +               (:noinline t)
> +               (:constructor nil)
> +               (:constructor track-changes--state ()))
> +  "Object holding a description of a buffer state.
> +BEG..END is the area that was changed and BEFORE is its previous content.
> +If the current buffer currently holds the content of the next state, you can get
> +the contents of the previous state with:
> +
> +    (concat (buffer-substring (point-min) beg)
> +                before
> +                (buffer-substring end (point-max)))
> +
> +NEXT is the next state object (i.e. a more recent state).
> +If NEXT is nil it means it's most recent state and it may be incomplete
> +\(BEG/END/BEFORE may be nil), in which case those fields will take their
> +values from `track-changes--before-(beg|end|before)' when the next
> +state is create."

This docstring is a bit confusing.
If a state object is not the most recent, how come 

> +    (concat (buffer-substring (point-min) beg)
> +                before
> +                (buffer-substring end (point-max)))

produces the previous content?

And if the state object is the most recent, "it may be incomplete"...

So, when is it safe to use the above (concat ... ) call?

> +(defvar-local track-changes--before-beg (point-min)
> +  "Beginning position of the remembered \"before string\".")
> +(defvar-local track-changes--before-end (point-min)
> +  "End position of the text replacing the \"before string\".")

Why (point-min)? It will make the values dependent on the buffer
narrowing that happens to be active when the library if first loaded.
Which cannot be right.

> +(defvar-local track-changes--buffer-size nil
> +  "Current size of the buffer, as far as this library knows.
> +This is used to try and detect cases where buffer modifications are \"lost\".")

Just looking at the buffer size may miss unregistered edits that do not
change the total size of the buffer. Although I do not know a better
measure. `buffer-chars-modified-tic' may lead to false-positives
(Bug#51766).

> +(cl-defun track-changes-register ( signal &key nobefore disjoint immediate)
> +  "Register a new tracker and return a new tracker ID.
> +SIGNAL is a function that will be called with one argument (the tracker ID)
> +after the current buffer is modified, so that we can react to the change.
> + ...
> +If optional argument DISJOINT is non-nil, SIGNAL is called every time we are
> +about to combine changes from \"distant\" parts of the buffer.
> +This is needed when combining disjoint changes into one bigger change
> +is unacceptable, typically for performance reasons.
> +These calls are distinguished from normal calls by calling SIGNAL with
> +a second argument which is the distance between the upcoming change and
> +the previous changes.

This is a bit confusing. The first paragraph says that SIGNAL is called
with a single argument, but that it appears that two arguments may be
passed. I'd rather tell the calling convention early in the docstring.

> +  (unless nobefore
> +    (setq track-changes--before-no nil)
> +    (add-hook 'before-change-functions #'track-changes--before nil t))
> +  (add-hook 'after-change-functions  #'track-changes--after  nil t)

Note that all the changes made in indirect buffers will be missed.
See bug#60333.

> +(defun track-changes-fetch (id func)
> ...
> +  (unless (equal track-changes--buffer-size (buffer-size))
> +    (track-changes--recover-from-error))
> +  (let ((beg nil)
> +        (end nil)
> +        (before t)
> +        (lenbefore 0)
> +        (states ()))
> +    ;; Transfer the data from `track-changes--before-string'
> +    ;; to the tracker's state object, if needed.
> +    (track-changes--clean-state)

> +(defun track-changes--recover-from-error ()
> ...
> +  (setq track-changes--state (track-changes--state)))

This will create a dummy state with

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

such state will not pass (< beg end) assertion in
`track-changes--clean-state' called in `track-changes-fetch' immediately
after `track-changes--recover-from-error'

> +(defun track-changes--in-revert (beg end before func)
> ...
> +      (atomic-change-group
> +        (goto-char end)
> +        (insert-before-markers before)
> +        (delete-region beg end)

What happens if there are markers inside beg...end?

> +(defun track-changes-tests--random-word ()
> +  (let ((chars ()))
> +    (dotimes (_ (1+ (random 12)))
> +      (push (+ ?A (random (1+ (- ?z ?A)))) chars))
> +    (apply #'string chars)))

If you are using random values for edits, how can such test be
reproduced? Maybe first generate a random seed and then log it, so that
the failing test can be repeated if necessary with seed assigned manually.

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