GNU bug report logs - #70077
An easier way to track buffer changes

Previous Next

Full log

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acm <at> muc.de, yantar92 <at> posteo.net, 70077 <at> debbugs.gnu.org
Subject: Re: bug#70077: An easier way to track buffer changes
Date: Mon, 08 Apr 2024 11:24:38 -0400

>> +(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.

AFAIK this docstring is displayed only by `describe-type` which shows
something like:

    track-changes--state is a type (of kind ‘cl-structure-class’) in ‘track-changes.el’.
     Inherits from ‘cl-structure-object’.
    
    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.
    
    Instance Allocated Slots:
    
            Name    Type    Default
            ————    ————    ———————
            beg     t       (point-max)
            end     t       (point-min)
            before  t       nil
            next    t       nil
    
    Specialized Methods:
    
    [...]

so the "form of the object" is included.  We don't have much practice
with docstrings for `cl-defstruct`, but I tried to follow the same kind
of conventions we use for functions, taking object slots as equivalent
to formal arguments.

Maybe `describe-type` should lists the slots first and the docstring
underneath rather than other way around?

>> +(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.

Hmm... having trouble making that fit on a single line.
Could you clarify why you think SIGNAL needs to be on the first line?

The best I could come up with so far is:

    Register a new change tracker handled via SIGNAL.

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

Other than the extra " it's", what is the difference?

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

I don't see the point of telling which function we use: the programmers
who want to know that, can consult the code.  As for explaining what "as
soon as convenient" means, that's what "usually right after the end of
the current command" is there for.

[ Also, I've changed the code to use `run-with-timer` in the mean time.  ]

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

I don't kow where you see the docstring saying that.
The closest I can find is:

    When IMMEDIATE is non-nil, the SIGNAL should preferably not always call
    `track-changes-fetch', since that would defeat the purpose of this library.

Note the "When IMMEDIATE is non-nil", "preferably", and "not always",
and the fact that the reason is not that something will break but that
some other solution would probably work better.
[ I changed "preferably" into "probably" since it's just a guess of mine
  rather than a request: there might be a good reason out there to
  prefer track-changes even for such a use case.  ]

>> +      ;; 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?

No, this verifies that the states were disconnected on purpose by
`track-changes--recover-from-error` rather than due to some bug in
the code.

>> +(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?

The library has a lot of `cl-assert`s which are all placed both as
documentation and as sanity checks.  All those should hopefully be true
all the time barring bugs in the code.

> I can envision using it as a cleanup, in which case assertions will
> not be appreciated.

Not sure what you mean by "using it as a cleanup"?
Its purpose is not to cleanup the general state of track-changes, but
to create a new, clean `track-changes--state`.

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

Yes, that refers to the code I wrote.

>> +;; 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.

This is in the "Extra candidates for the API" section, which holds
a bunch of things which might be useful or might not.

>> +(defun diff--track-changes-function (beg end _before)
>> +  (with-demoted-errors "%S"
> Why did you need with-demoted-errors here?

It used to be `ignore-errors` because back in 1999 we didn't have
`with-demoted-errors`.

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

Working on it.


        Stefan

Previous Next

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