bug#70077: An easier way to track buffer changes

[Eli Zaretskii <eliz@gnu.org>]

> Cc: Alan Mackenzie <acm@muc.de>, Ihor Radchenko <yantar92@posteo.net>
> Date: Fri, 05 Apr 2024 18:12:55 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> +;; This library is a layer of abstraction above `before-change-functions'
> +;; and `after-change-functions' which takes care of accumulating changes
> +;; until a time when its client finds it convenient to react to them.
> +;;
> +;; It provides an API that is easier to use correctly than our
> +;; `*-change-functions` hooks.  Problems that it claims to solve:

Please redo all the quoting from `like this` to one of the two styles
we use in our documentation.

> +(unless (fboundp 'funcall-later)
> +  (defun funcall-later (&rest args)
> +    ;; FIXME: Not sure if `run-with-timer' preserves ordering between
> +    ;; different calls with the same target time.
> +   (apply #'run-with-timer 0 nil args)))

Both funcall-later and run-with-timer??

> +
> +;;;; Internal types and variables.
> +
> +(cl-defstruct (track-changes--tracker
> +               (:noinline t)
> +               (:constructor nil)
> +               (:constructor track-changes--tracker ( signal state
> +                                                      &optional
> +                                                      nobefore immediate)))
> +  signal state nobefore immediate)
> +
> +(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 doc string should IMO include the form of the object, because
without that BEG, END, BEFORE, and NEXT are "out of the blue", and
it's entirely unclear what they allude to.

Also, this doc string (and a few others) have very long lines, so
please refill them.

> +(defvar-local track-changes--trackers ()
> +  "List of trackers currently registered in the current buffer.")
                                            ^^^^^^^^^^^^^^^^^^^^^
I think "in the buffer" is more accurate, since this is not limited to
the current buffer.

> +(defvar-local track-changes--disjoint-trackers ()
> + "List of trackers that want to react to disjoint changes.
> +These trackers' are signaled every time track-changes notices
                 ^
That apostrophe is redundant.

> +(defvar-local track-changes--before-clean 'unset
> +  "If non-nil, the `track-changes--before-*' vars are old.
> +More specifically it means they cover a part of the buffer relevant
> +for the previous state.
> +It can take two non-nil values:
> +- `unset': Means that the vars cover some older state.
> +  This is what it is set right after creating a fresh new state.
                        ^^^
"set to"

> +(cl-defun track-changes-register ( signal &key nobefore disjoint immediate)
> +  "Register a new tracker and return a new tracker ID.

Please mention SIGNAL in the first line of the doc string.

> +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.
                                                 ^^
"we"?

> +By default SIGNAL is called as soon as convenient after a change, which is
                               ^^^^^^^^^^^^^^^^^^^^^
"as soon as it's convenient", I presume?

> +usually right after the end of the current command.

This should explicitly reference funcall-later, so that users could
understand what "as soon as convenient" means.

> +If optional argument DISJOINT is non-nil, SIGNAL is called every time we are
> +about to combine changes from \"distant\" parts of the buffer.        ^^

"we" again?

> +In order to prevent the upcoming change from being combined with the previous
> +changes, SIGNAL needs to call `track-changes-fetch' before it returns."

This seems to contradict what the doc string says previously: that
SIGNAL should NOT call track-changes-fetch.

> +(defun track-changes-fetch (id func)
> +  "Fetch the pending changes.

The first line of a doc string should mention the arguments.

> +ID is the tracker ID returned by a previous `track-changes-register'.
> +FUNC is a function.  It is called with 3 arguments (BEGIN END BEFORE)
> +where BEGIN..END delimit the region that was changed since the last
> +time `track-changes-fetch' was called and BEFORE is a string containing
> +the previous content of that region (or just its length as an integer
> +If the tracker ID was registered with the `nobefore' option).
   ^^
"if", lower-case.

> +If some error caused us to miss some changes, then BEFORE will be the
                        ^^
"we" again?

> +If no changes occurred since the last time, FUNC is not called and
> +we return nil, otherwise we return the value returned by FUNC,
   ^^
And again.

> +      ;; The states are disconnected from the latest state because
> +      ;; we got out of sync!
> +      (cl-assert (eq (track-changes--state-before (car states)) 'error))

This seem to mean Emacs will signal an error in this case, not pass
'error' in BEFORE?

> +(defun track-changes--clean-state ()
> +  (cond
> +   ((null track-changes--state)
> +    (cl-assert track-changes--before-clean)
> +    (cl-assert (null track-changes--buffer-size))
> +    ;; No state has been created yet.  Do it now.
> +    (setq track-changes--buffer-size (buffer-size))
> +    (when track-changes--before-no
> +      (setq track-changes--before-string (buffer-size)))
> +    (setq track-changes--state (track-changes--state)))
> +   (track-changes--before-clean nil)
> +   (t
> +    (cl-assert (<= (track-changes--state-beg track-changes--state)
> +                   (track-changes--state-end track-changes--state)))
> +    (let ((actual-beg (track-changes--state-beg track-changes--state))
> +          (actual-end (track-changes--state-end track-changes--state)))
> +      (if track-changes--before-no
> +          (progn
> +            (cl-assert (integerp track-changes--before-string))
> +            (setf (track-changes--state-before track-changes--state)
> +                  (- track-changes--before-string
> +                     (- (buffer-size) (- actual-end actual-beg))))
> +            (setq track-changes--before-string (buffer-size)))
> +        (cl-assert (<= track-changes--before-beg
> +                       actual-beg actual-end
> +                       track-changes--before-end))
> +        (cl-assert (null (track-changes--state-before track-changes--state)))

Are all those assertions a good idea in this function?  I can envision
using it as a cleanup, in which case assertions will not be
appreciated.

> +(defvar track-changes--disjoint-threshold 100
> +  "Distance below which changes are not considered disjoint.")

This should tell in what units the distance is measured.

> +;;;; Extra candidates for the API.
> +
> +;; This could be a good alternative to using a temp-buffer like I used in
                                                                   ^^^^^^
"I"?

> +;; Eglot, since presumably we've just been changing this very area of the
> +;; buffer, so the gap should be ready nearby,
> +;; It may seem silly to go back to the previous state, since we could have
> +;; used `before-change-functions' to run FUNC right then when we were in
> +;; that state.  The advantage is that with track-changes we get to decide
> +;; retroactively which state is the one for which we want to call FUNC and
> +;; which BEG..END to use: when that state was current we may have known
> +;; then that it would be "the one" but we didn't know what BEG and END
> +;; should be because those depend on the changes that came afterwards.

Suggest to reword (or remove) this comment, as it sounds like
development-time notes.

> +(defun diff--track-changes-function (beg end _before)
> +  (with-demoted-errors "%S"

Why did you need with-demoted-errors here?

Last, but not least: this needs suitable changes in NEWS and ELisp
manual.

Thanks.