From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: phillip.lord@russet.org.uk (Phillip Lord) Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change 0f3c1db: Changed semantics of first-undoable-change-hook. Date: Wed, 30 Sep 2015 22:16:27 +0100 Message-ID: <87io6rsj4k.fsf@russet.org.uk> References: <20150928134935.25048.30653@vcs.savannah.gnu.org> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1443698136 9533 80.91.229.3 (1 Oct 2015 11:15:36 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 1 Oct 2015 11:15:36 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Oct 01 13:15:28 2015 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Zhbpe-000780-8L for ged-emacs-devel@m.gmane.org; Thu, 01 Oct 2015 13:15:26 +0200 Original-Received: from localhost ([::1]:48174 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zhbpd-0003fg-Jf for ged-emacs-devel@m.gmane.org; Thu, 01 Oct 2015 07:15:25 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44214) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhOjs-0002CE-HU for emacs-devel@gnu.org; Wed, 30 Sep 2015 17:16:37 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZhOjp-0001mx-3d for emacs-devel@gnu.org; Wed, 30 Sep 2015 17:16:36 -0400 Original-Received: from cheviot22.ncl.ac.uk ([128.240.234.22]:32815) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhOjo-0001kZ-S0 for emacs-devel@gnu.org; Wed, 30 Sep 2015 17:16:33 -0400 Original-Received: from smtpauth-vm.ncl.ac.uk ([10.8.233.129] helo=smtpauth.ncl.ac.uk) by cheviot22.ncl.ac.uk with esmtp (Exim 4.63) (envelope-from ) id 1ZhOjl-0003fs-Da; Wed, 30 Sep 2015 22:16:29 +0100 Original-Received: from cpc6-benw10-2-0-cust45.gate.cable.virginm.net ([92.238.179.46] helo=localhost) by smtpauth.ncl.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1ZhOjk-0000yt-5S; Wed, 30 Sep 2015 22:16:28 +0100 In-Reply-To: (Stefan Monnier's message of "Tue, 29 Sep 2015 01:35:59 -0400") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 128.240.234.22 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:190531 Archived-At: Stefan Monnier writes: > Please capitalize and punctuate your comments. > Please always add spaces before the open parens. > Here as well (and ,many other places). > Try to avoid adding spurious empty lines. > It looks pretty good. Remaining problems I noticed: > - run_first_undo_hook is defined to take no argument, yet it's always > called with `current_buffer' as argument. I am working my way through all of these! > - in src/keyboard.c we push undo-boundaries after every command. > This is now made redundant by your code, so you might like to > remove it. > > - when trying to remove that code, you'll notice that > Fself_insert_command (and Fdelete_char, IIRC) depends on that code to > know whether the last boundary can be removed (so as to implement the > "bundling"). Hmmm. So, I had a look at this, and it's actually more complex than it appears. The problem, ironically, is the bundling in Fself_insert_command. My code (first-undoable-change-hook) only runs on the first change after a boundary. And this doesn't mean "after a boundary has been added" but "when there is a boundary on the head of the list immediately after a change has happened". But, self-insert-command removes this boundary. So my hook doesn't run. At least I think that this is what is happening. The global state of Emacs is fairly complex to reason about in this case. So, I have to hook into post-command-hook to get this to work, which obviously runs a lot and isn't quite working yet for me. The bundling implemented by remove_excessive_undo_boundaries is making me scratch my head also. This also depends on global stage (last_undo_boundary); if I understand this code correctly, this is the cons cell that was last added in `keyboard.c'. I have worked out how to replicate this, but my use case still breaks this because of this use of global state; last_undo_boundary only works because we are assuming that Fself_insert_command is only going to add *one* undo boundary. And that's not true if post-command-hook is doing other things. The underlying problem here is that "undo-boundary" is marked by nil and there's only one nil. So all undo boundaries are equivalent; we can't distinguish between those inserted "automatically" and those inserted "explicitly", except by looking at the cons. If we used symbols, say something like '(boundary reason) where "boundary" was always the same symbol, and reason could any of a set of symbols, then life would be easier, because I could distinguish between boundaries from the command-loop and boundaries from everywhere else by looking at buffer-undo-list rather than global state of a single variable. But this is quite a big change. I am sorry this is taking so long and so much discussion; I'm learning C as I go which is a little traumatic. > - It looks like run_first_undo_hook is often called in an "unclean" > state where the C code has set current_buffer rather than calling > set_buffer_internal. This is an optimization that only works when we > know exactly the code that might possibly be run before current_buffer > is set back to its previous value, but it can't be used when running > arbitrary Elisp code. So we should call run_first_undo_hook *before* > setting current_buffer, and run_first_undo_hook should > set_buffer_internal when needed. Okay, will fix that.