bug#70077: An easier way to track buffer changes

[Eli Zaretskii <eliz@gnu.org>]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 70077@debbugs.gnu.org,  acm@muc.de,  yantar92@posteo.net
> 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.

It's included, but _after_ explaining what each member of the object
form means.  That's bad from the methodological POV: we should first
show the form and only afterwards describe each of its members.

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

That'd also be good.  Then the doc string should say something like

  Object holding a description of a buffer state.
  It has the following Allocated Slots:

            Name    Type    Default
            ————    ————    ———————
            beg     t       (point-max)
            end     t       (point-min)
            before  t       nil
            next    t       nil

  BEG..END is the area that was changed and BEFORE is its previous
  content[...]

(Btw, those "t" under "Type" are also somewhat mysterious.  What do
they signify?)

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

    Register a new tracker whose change-tracking function is SIGNAL.
  Return the ID of the new tracker.

> Could you clarify why you think SIGNAL needs to be on the first line?

It's our convention to mention the mandatory arguments on the first
line of the doc string.

> The best I could come up with so far is:
> 
>     Register a new change tracker handled via SIGNAL.

That's a good start, IMO, except that SIGNAL doesn't hadle the
tracker, it handles changes, right?

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

Nothing.  I indeed thing "it's" is missing there.

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

My point is that by referencing funcall-later you can avoid the need
to explain what is already explained in that function's doc string.
You could, for example, simply say

  By default, SIGNAL is arranged to be called later by using
  `funcall-later'.

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

That'd need a trivial change above, and I still think it's worthwhile,
as it makes this doc string easier to grasp: if one already knows how
run-with-timer works, they don't need anything else to be said; and if
they don't, they can later read on that separately.

IOW, you separate one complex description into two simpler ones: a win
IME.

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

Then maybe the sentence on which I commented should say

  Except when IMMEDIATE is non-nil, if SIGNAL needs to prevent the
  upcoming change from being combined with the previous ones, it
  should call `track-changes-fetch' before it returns.

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

In general, when I want to create a clean slate, I don't care too much
about the dirt I remove.  Why is it important to signal errors because
a state I am dumping had some errors?

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

We don't usually leave such style in long-term comments and
documentation.

Thanks.