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: Tue, 09 Apr 2024 22:02:05 -0400 Message-ID: References: <87edbhnu53.fsf@localhost> <87msq2phfv.fsf@localhost> 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="38537"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: Alan Mackenzie , 70077@debbugs.gnu.org To: Ihor Radchenko Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Apr 10 04:03:27 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 1ruNJ8-0009im-OH for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 10 Apr 2024 04:03:27 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ruNIi-0006jY-SP; Tue, 09 Apr 2024 22:03:01 -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 1ruNIc-0006id-TN for bug-gnu-emacs@gnu.org; Tue, 09 Apr 2024 22:02:55 -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 1ruNIc-00088W-KR for bug-gnu-emacs@gnu.org; Tue, 09 Apr 2024 22:02:54 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1ruNIk-0006fY-K3 for bug-gnu-emacs@gnu.org; Tue, 09 Apr 2024 22:03:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 10 Apr 2024 02:03:02 +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.171271454925493 (code B ref 70077); Wed, 10 Apr 2024 02:03:02 +0000 Original-Received: (at 70077) by debbugs.gnu.org; 10 Apr 2024 02:02:29 +0000 Original-Received: from localhost ([127.0.0.1]:51829 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ruNIC-0006d5-Au for submit@debbugs.gnu.org; Tue, 09 Apr 2024 22:02:28 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:48914) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ruNI8-0006cN-9m for 70077@debbugs.gnu.org; Tue, 09 Apr 2024 22:02:26 -0400 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id AE3A0442B68; Tue, 9 Apr 2024 22:02:08 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1712714526; bh=/Dss9zEOTmq8agZwSw14/0F5h/KPR4j/AsuSvnP5YgA=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=Xy/OY4lciVud3oR/bVkRZaNeZbfhfg+eSDU2lt6v+HMqkjUE2sOfcHkudzih3dUn1 k6TiP74TjO+zRewZpsOk4n5i4IZMvWK90P5WWCreuIQSDUIWYevdzHcYrgNjlIH8iT +M2XwJGjGHoVtjwz4hq0UB7Os0DitnR09/dTtOoMqhwu0+TNXlyeT9UTxUdtMVTR2m YyvzsvZ+hgi8/27FTGE8tgKSyUjccfZar6NMIjPn/LhWvT5g3Gvf9i+DhkG2TrqW4C P8o1CTY+6AfE8DOoXq6jzJejk82hO8/Z81kwgJ2dn5SWVCmJgzWf1Q/UM6oAvMUqXQ jg4/QnpVizoXQ== Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 5CD3F442B5F; Tue, 9 Apr 2024 22:02:06 -0400 (EDT) Original-Received: from pastel (unknown [45.72.201.215]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 33B7E120193; Tue, 9 Apr 2024 22:02:06 -0400 (EDT) In-Reply-To: <87msq2phfv.fsf@localhost> (Ihor Radchenko's message of "Tue, 09 Apr 2024 17:35:48 +0000") 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:283022 Archived-At: >> You never want to use this call, it's only there to show in a concise >> manner how BEG/END/BEFORE relate and what information they're supposed >> to hold. > My point is that it is hard to understand that docstring for someone > reading it first. I did not mean that it does not make sense for someone > who is already familiar with the rest of the code. > I believe that it would help if you explain how to interpret the values > when there is a chain of state1 -> next -> next -> nil. How 'bout: "Object holding a description of a buffer state. A buffer state is described by a BEG/END/BEFORE triplet which say how to recover that state from the next state. I.e. if the buffer's contents reflects the next state, you can recover the previous state by replacing the BEG..END region with the BEFORE string. =20=20=20=20 NEXT is the next state object (i.e. a more recent state). If NEXT is nil it means it's the most recent state and it may be incomp= lete \(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 created." >> The second convention is used only when DISJOINT is non-nil, which is >> why it's described where we document the effect of DISJOINT. >> An alternative could be to make the `disjoint` arg hold the function to = call >> to signal disjoint changes. > > Sure. My point is that the current formulation of the docstring might > lead to errors unless one is very careful when reading it. The Texinfo doc is more upfront about the fact that it's called sometimes with one and sometimes with 2 args, but I feel that the function's docstring is already too long so I'd rather not repeat that here. I feel that These calls are distinguished from normal calls by calling SIGNAL with a second argument which ... does make it sufficiently clear that it's different. Also, any errors should be caught pretty quickly, so I'm not worried about seeing a lot of broken code with latent bugs because the SIGNAL function doesn't accept the right number of arguments. >>>> + (unless nobefore >>>> + (setq track-changes--before-no nil) >>>> + (add-hook 'before-change-functions #'track-changes--before nil t)) >>>> + (add-hook 'after-change-functions #'track-changes--after nil t) >>> Note that all the changes made in indirect buffers will be missed. >>> See bug#60333. >> Yup. And misuses of `inhibit-modification-hooks` or >> `with-silent-modifications`. =F0=9F=99=81 > Indirect buffer is not at all a misuse. I didn't claim the opposite here (tho I do admit to thinking that it's an attractive nuisance). I only reminded that cases where changes are missed are things that can and do happen because of various problems in various pieces of code and indirect buffers are just one of those sources of problems. > What I implied in my comment is that it would be nice to implement > something like what whitespace.el does in bug#60333. I'll reply the same as in bug#60333: it should be fixed at a lower level. I don't see any reason why it should be difficult to make it work right. > This point is important from the point of view of Org mode because Org > mode uses a lot of indirect buffers both internally and as a recommended > workflow (org-tree-to-indirect-buffer). Then I encourage you to try and fix it: rename `make-indirect-buffer` to `internal-make-indirect-buffer`, define a new `make-indirect-buffer` in ELisp which registers *-change-functions to propagate the changes to all the related buffers. Personally all the problems with indirect buffers put me off. > You are right. That sneaky (track-changes--before-clean nil) line is so > sneaky that I'd like to see a comment highlighting that it exists ;) > I simply missed it and thought that `track-changes--before-clean' only > has two cond branches when I was reading the code. OK, I made it stand out more. =F0=9F=99=82 >> During FUNC they're moved to BEG or END, and when we restore the >> original state, well... the undo machinery has some support to restore >> the markers where they were, but it's definitely not 100%. =F0=9F=99=81 > > Interesting. I did not know that markers are recovered by undo. That's what the following entries are for: An entry (MARKER . DISTANCE) indicates that the marker MARKER was adjusted in position by the offset DISTANCE (an integer). > I had very bad experience with marker preservation when working with > other low-level APIs like `replace-match'. And previous Org mode > commiters had the same issues with kill/yank. `replace-match`, `kill`, `yank`, etc... can't do very much because they have no idea how the new text relates to the old one, so without additional application-specific information, the only thing they can do is move the inner markers to one of the ends of the change. E.g. when you move a subtree, *you* know that the text you insert is the same text that you previously deleted, but the `insdel.c` primitives don't know that. In some cases you can get the desired behavior "automatically" by using text-properties instead of markers, tho. Undo is different because we're going back to a state for which Emacs can know where the markers should be because we've been there. > May undo be trusted? I never looked closely to see how reliable it is. AFAIK for the markers for which we don't call `move-marker` it should not be too hard to make it reliable (if it's not already). But for those on which we do call `move-marker`, I don't think the current form of `buffer-undo-list` provides sufficient info to know whether the marker should be "moved to its previous position" or should be left alone (if `move-marker` was called in the mean time, the user probably doesn't want that marker to move back). We know this because the `mark` is a marker we move all the time (instead of creating a new marker each time) and we've had a bug report about it. =F0=9F=99=81 > Or may it be more reliable to deploy a more manual > approach explicitly saving the markers and restoring them via > unwind-protect? We don't have a primitive to fetch all the markers in a given region. But if the FUNC in `track-changes--in-revert` doesn't move any of the markers, then the `buffer-undo-list` should be good enough (in theory at least, so it depends on whether we have bugs in there). >>> Maybe first generate a random seed and then log it, so that the >>> failing test can be repeated if necessary with seed assigned manually. >> >> Good idea. >> But my attempt (see patch below) failed. >> I'm not sure what I'm doing wrong, but >> >> make test/lisp/emacs-lisp/track-changes-tests \ >> EMACS_EXTRAOPT=3D"--eval '(setq track-changes-tests--random-see= d \"15216888\")'" >> >> gives a different score each time. =F0=9F=99=81 > > May it be because you used different Emacs builds? Nope, just repeated invocations of the same Emacs binary. It was much more silly than that: I seeded the random number generator too late (after generating the initial buffer content). =F0=9F=99=82 It works now, thank you. Stefan