From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#70077: An easier way to track buffer changes Date: Mon, 08 Apr 2024 11:24:38 -0400 Message-ID: References: <86frvy51af.fsf@gnu.org> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="1835"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: acm@muc.de, yantar92@posteo.net, 70077@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Apr 08 17:26:32 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 1rtqtE-0000JR-EH for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 08 Apr 2024 17:26:32 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rtqsr-0006eC-CH; Mon, 08 Apr 2024 11:26:09 -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 1rtqse-0006bq-54 for bug-gnu-emacs@gnu.org; Mon, 08 Apr 2024 11:25: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 1rtqsc-0005q8-IX for bug-gnu-emacs@gnu.org; Mon, 08 Apr 2024 11:25:54 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rtqsj-0006Ds-Lx for bug-gnu-emacs@gnu.org; Mon, 08 Apr 2024 11:26:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 08 Apr 2024 15:26:01 +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.171258991623729 (code B ref 70077); Mon, 08 Apr 2024 15:26:01 +0000 Original-Received: (at 70077) by debbugs.gnu.org; 8 Apr 2024 15:25:16 +0000 Original-Received: from localhost ([127.0.0.1]:47387 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rtqrx-0006Ac-Mw for submit@debbugs.gnu.org; Mon, 08 Apr 2024 11:25:16 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:26782) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rtqrn-00068m-4q for 70077@debbugs.gnu.org; Mon, 08 Apr 2024 11:25:13 -0400 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id A80A844170E; Mon, 8 Apr 2024 11:24:49 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1712589887; bh=QrvdbIxaISx5Jy6QjtYdnyJxrb+xpLaFO1qeqETiNmI=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=mowZ3c/ze8WqYK7Zwqc3T7JKVkROEPdiB10YuSlGIHrBhgOSmYZGF6z3bKWl/LHql g2yugzfsmkj1u1agL80VqOkGOh520Vczg6q+9ECAlrOf8/2gJAbFQiKUI4RTIjei0a Lln28wJeH/V6t9qUw9qmIpG4xLe2l3grMPMvxYdd50bNNtwUjC1OBwqtGlSm8btFWD IvIjnpdJ3XdDbvRp7OPIy/qdq3wZhnpZn1NNWe1v70gMNsEjiLQnthDpHmsxBOA7u9 Fyj2oVaLN+MKEC6t5DGa9Xh3vC/uKNM8fDtma6uMnhdIHl0Tm2Pe0UEQnjd7qE9EFb E/jE1Qe1otsCQ== Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 774C24416D2; Mon, 8 Apr 2024 11:24:47 -0400 (EDT) Original-Received: from pastel (unknown [45.72.201.215]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 48BC8120679; Mon, 8 Apr 2024 11:24:47 -0400 (EDT) In-Reply-To: <86frvy51af.fsf@gnu.org> (Eli Zaretskii's message of "Sat, 06 Apr 2024 11:43:36 +0300") 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:282942 Archived-At: >> +(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 conten= t. >> +If the current buffer currently holds the content of the next state, yo= u 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 =E2=80=98cl-structure-class=E2= =80=99) in =E2=80=98track-changes.el=E2=80=99. Inherits from =E2=80=98cl-structure-object=E2=80=99. =20=20=20=20 Object holding a description of a buffer state. BEG..END is the area that was changed and BEFORE is its previous conten= t. If the current buffer currently holds the content of the next state, yo= u can get the contents of the previous state with: =20=20=20=20 (concat (buffer-substring (point-min) beg) before (buffer-substring end (point-max))) =20=20=20=20 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. =20=20=20=20 Instance Allocated Slots: =20=20=20=20 Name Type Default =E2=80=94=E2=80=94=E2=80=94=E2=80=94 =E2=80=94=E2=80=94=E2= =80=94=E2=80=94 =E2=80=94=E2=80=94=E2=80=94=E2=80=94=E2=80=94=E2=80=94= =E2=80=94 beg t (point-max) end t (point-min) before t nil next t nil =20=20=20=20 Specialized Methods: =20=20=20=20 [...] 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 immedi= ate) >> + "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 pr= evious >> +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 libr= ary. 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 (<=3D (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 (<=3D track-changes--before-beg >> + actual-beg actual-end >> + track-changes--before-end)) >> + (cl-assert (null (track-changes--state-before track-changes--st= ate))) > > 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 t= he >> +;; buffer, so the gap should be ready nearby, >> +;; It may seem silly to go back to the previous state, since we could h= ave >> +;; 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 deci= de >> +;; 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