Package: emacs;
Reported by: Stefan Monnier <monnier <at> iro.umontreal.ca>
Date: Fri, 29 Mar 2024 16:17:01 UTC
Severity: normal
Tags: patch
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Bug is archived. No further changes may be made.
Message #119 received at 70077 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Stefan Monnier <monnier <at> iro.umontreal.ca> 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 18:53:22 +0300
> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 70077 <at> debbugs.gnu.org, acm <at> muc.de, yantar92 <at> 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.