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 f59d1be: Move undo amalgamation to lisp. Date: Thu, 08 Oct 2015 20:56:13 +0100 Message-ID: <878u7djfs2.fsf@russet.org.uk> References: <20151005134118.10933.50859@vcs.savannah.gnu.org> <87h9m52sh8.fsf@russet.org.uk> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1444334943 26263 80.91.229.3 (8 Oct 2015 20:09:03 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Thu, 8 Oct 2015 20:09:03 +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 08 22:08:54 2015 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from eggs.gnu.org ([208.118.235.92]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1ZkHLL-0004qn-EP for ged-emacs-devel@m.gmane.org; Thu, 08 Oct 2015 21:59:11 +0200 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkHL1-00011b-CM for ged-emacs-devel@m.gmane.org; Thu, 08 Oct 2015 15:59:10 -0400 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on eggs.gnu.org X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=BAYES_50,T_RP_MATCHES_RCVD autolearn=disabled version=3.3.2 Original-Received: from lists.gnu.org ([2001:4830:134:3::11]:42473) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkHL1-000108-56 for ged-emacs-devel@m.gmane.org; Thu, 08 Oct 2015 15:58:51 -0400 Original-Received: from localhost ([::1]:36840 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkHL0-0007MR-TV for ged-emacs-devel@m.gmane.org; Thu, 08 Oct 2015 15:58:50 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:52525) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkHIl-0006zu-OD for emacs-devel@gnu.org; Thu, 08 Oct 2015 15:56:32 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZkHIi-0007zq-8f for emacs-devel@gnu.org; Thu, 08 Oct 2015 15:56:31 -0400 Original-Received: from cheviot22.ncl.ac.uk ([128.240.234.22]:38189) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZkHIh-0007yp-VA for emacs-devel@gnu.org; Thu, 08 Oct 2015 15:56:28 -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 1ZkHId-0008Mp-FL; Thu, 08 Oct 2015 20:56:23 +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 1ZkHIc-0006zR-EM; Thu, 08 Oct 2015 20:56:22 +0100 In-Reply-To: (Stefan Monnier's message of "Wed, 7 Oct 2015 15:28:44 -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-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 X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2001:4830:134:3::11 Xref: news.gmane.org gmane.emacs.devel:191069 Archived-At: Stefan Monnier writes: >>> I think this code should be called from self-insert-command rather than >>> from pre-command-hook. >> On a hook? Or do I just directly call a function defined in lisp form C? > > Yes, you can just call a Lisp function directly from C. Okay, that's straight-forward enough. >> Does the same argument apply to the post-command-hook and >> after-change-functions also? > > For the after-change-functions: yes, very much so. > > For the post-command-hook, I also think we could/should call the > function directly rather than go through post-command-hook, but there > are arguments in favor of either choice. > > At least, calling the function directly is safer in the sense that it > is closer to the pre-existing code. For me, it also has the advantage that it will occur at a defined time wrt to any functions on post-command-hook, which should make the behaviour more predictable. Set against this, of course, is that it also becomes harder to change from lisp. Does the call2 (Qmy_function,arg1,arg2) work like a normal lisp call? I mean, can I redefine my-function, and will it run the new definition? Is it still open to advice? >>> And it should also be called from delete-char. >> Yes, next on my list. > > Ah, fine, then. > >>> We don't actually know that (cdr last-before-nil) and (car >>> last-before-nil) are numbers. The previous self-insert-command might >>> have performed all kinds of buffer modifications (via abbrev-expansion, >>> post-self-insert-hook, ...). >> Hmmm. That's unfortunate -- I was trying to avoid "global" state and >> just user buffer state; the undo-list seemed like a sensible place to >> get this knowledge from. > > The current logic in remove_excessive_undo_boundaries is far from > perfect, but unless you have a really good idea how to do it > differently, I recommend you just try to reproduce it in Elisp. As I said, the difficulty comes about from trying to work out whether the last undo-boundary is an "automatic" one (i.e. added by the C layer and the command loop) or a "manual" one (i.e. added by a call to undo-boundary). This works like so: in keyboard.c #+begin_src c if (NILP (KVAR (current_kboard, Vprefix_arg))) /* FIXME: Why? --Stef */ { Lisp_Object undo = BVAR (current_buffer, undo_list); Fundo_boundary (); last_undo_boundary = (EQ (undo, BVAR (current_buffer, undo_list)) ? Qnil : BVAR (current_buffer, undo_list)); } call1 (Qcommand_execute, Vthis_command); #+end_src in cmds.c (243) we have #+begin_src c if (remove_boundary && CONSP (BVAR (current_buffer, undo_list)) && NILP (XCAR (BVAR (current_buffer, undo_list))) /* Only remove auto-added boundaries, not boundaries added by explicit calls to undo-boundary. */ && EQ (BVAR (current_buffer, undo_list), last_undo_boundary)) /* Remove the undo_boundary that was just pushed. */ bset_undo_list (current_buffer, XCDR (BVAR (current_buffer, undo_list))); #+end_src In otherwords, we remember the last undo-boundary put in by the command loop, and we only remove the last undo-boundary from bufffer-undo-list if it is that one. What I didn't like about this logic is that it only works for a single buffer; it assumes that there is only one last_undo_boundary. But a self-insert-command might result in changes to more than one buffer, if there is a function on post-command-hook or after-change-functions. As I said before, the ultimate problem is that all undo-boundaries look alike, because they are all nil. I had a brief look to see how hard it would be to change this; I've found about four places in lisp that depend on this "boundary as nil" behaviour. - primitive-undo (while (setq next (pop list))) - undo-make-selective-list (cond ((null undo-elt))) - undo-elt-in-region (eq undo-elt nil) - undo-elt-crosses-region (cond ((atom undo-elt) nil)) There are quite a few places in C also. Fixable, but I suspect I'd introduce more problems than I would solve. Anyway, I'll move the calls away from the hooks over the next few days, and see how it goes! Phil