From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#70077: An easier way to track buffer changes Date: Mon, 08 Apr 2024 18:53:22 +0300 Message-ID: <86msq3yhot.fsf@gnu.org> References: <86frvy51af.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="19338"; mail-complaints-to="usenet@ciao.gmane.io" Cc: acm@muc.de, yantar92@posteo.net, 70077@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Apr 08 17:58:06 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rtrNl-0004ml-Bs for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 08 Apr 2024 17:58:05 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rtrJu-0001GX-Lb; Mon, 08 Apr 2024 11:54:06 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rtrJk-0001Dn-F6 for bug-gnu-emacs@gnu.org; Mon, 08 Apr 2024 11:53:56 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rtrJk-0001on-5K for bug-gnu-emacs@gnu.org; Mon, 08 Apr 2024 11:53:56 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rtrJr-0008Rg-JB for bug-gnu-emacs@gnu.org; Mon, 08 Apr 2024 11:54:03 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 08 Apr 2024 15:54:03 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 70077 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 70077-submit@debbugs.gnu.org id=B70077.171259162632404 (code B ref 70077); Mon, 08 Apr 2024 15:54:03 +0000 Original-Received: (at 70077) by debbugs.gnu.org; 8 Apr 2024 15:53:46 +0000 Original-Received: from localhost ([127.0.0.1]:47451 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rtrJW-0008Pz-Hp for submit@debbugs.gnu.org; Mon, 08 Apr 2024 11:53:45 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:37480) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rtrJU-0008Pb-FA for 70077@debbugs.gnu.org; Mon, 08 Apr 2024 11:53:41 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rtrJF-0001ie-VX; Mon, 08 Apr 2024 11:53:26 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-version:References:Subject:In-Reply-To:To:From: Date; bh=soUjVnmbpiQunzkAFi7eNvXJ9KyFxxLKVFXhWvos4U8=; b=V/9wpSM1SMevPJPHtKWz 6bG2ElQjvGrij4pmQR0/QqYRuwp1P0XcIY6PdEY+3EhfQPYhgSS8lvGkduRWh15gHmu3YYbx4ymb1 WfQVIUN/1sgtgGRdS3eYN85Cb9pwgzmgITFnSDlw4b22fJLKa9C95Gq/TXgYJP1pfHV4GXH2tVQj/ lIp56ZygpliubZy332psj3ieeiYsq+U0G5mCPGC186PM9o2CdGKNgq9yN/I/AqCrwoD624ZQtOkPy FAV38qN08nwbBGrTugyiNCIxxoxYOKImqcqkZ2nVFs23bmxP7UwRsUEsEKx5Oa0OEZJFQmQIHlNBw gLJwrILTXhnIyQ==; In-Reply-To: (message from Stefan Monnier on Mon, 08 Apr 2024 11:24:38 -0400) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:282945 Archived-At: > From: Stefan Monnier > 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.