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: Fri, 16 Oct 2015 22:02:01 +0100 Message-ID: <87h9lqbk8m.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 1445029367 7582 80.91.229.3 (16 Oct 2015 21:02:47 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 16 Oct 2015 21:02:47 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Oct 16 23:02:38 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 1ZnC95-0000kZ-LY for ged-emacs-devel@m.gmane.org; Fri, 16 Oct 2015 23:02:35 +0200 Original-Received: from localhost ([::1]:55861 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZnC95-0001m6-2G for ged-emacs-devel@m.gmane.org; Fri, 16 Oct 2015 17:02:35 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:48807) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZnC8c-0001ik-Uc for emacs-devel@gnu.org; Fri, 16 Oct 2015 17:02:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZnC8Z-0006WB-Io for emacs-devel@gnu.org; Fri, 16 Oct 2015 17:02:06 -0400 Original-Received: from cheviot22.ncl.ac.uk ([128.240.234.22]:48631) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZnC8Z-0006Vu-Ak for emacs-devel@gnu.org; Fri, 16 Oct 2015 17:02:03 -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 1ZnC8Y-0003vB-EQ; Fri, 16 Oct 2015 22:02:02 +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 1ZnC8Y-0008S5-3s; Fri, 16 Oct 2015 22:02:02 +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-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:191790 Archived-At: So, I think I mostly have this now, although I need to do some more testing on it to make sure, and clearly the code needs cleaning up (all the debug infrastructure will need to go, and docstrings be put in place). I have a few questions inline, however. Stefan Monnier writes: > 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. In the end, I have left this on post-command-hook. It is this hook that actually does all the work and using the hook gives the (other) developer the option of disabling this "intelligent" behavior if it turns out not to work. This seems sensible to me. The alternative is sticking a direct call to simple.el into the command loop, but I would just put it immediately before post-command-hook. It seems duplicative to me. Against this, is the use of a hook for "core" functionality rather than an extension. Happy to change if you prefer. > > At least, calling the function directly is safer in the sense that it > is closer to the pre-existing code. > >>> And it should also be called from delete-char. >> Yes, next on my list. > > Ah, fine, then. delete-char does indeed call "remove_excessive_boundaries", but for the life of me I can not reproduce any affect that this has at the user level. If I type "abcde" into a clean buffer, then "undo" all the letters disappear at once (since they have amalgamated). If, though I have "abcde" in the buffer and do 5x delete followed by "undo" the letters reappear one at a time. > >>> 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. I am also a bit confused by this code from cmds.c #+begin_src c if (!EQ (Vthis_command, KVAR (current_kboard, Vlast_command))) nonundocount = 0; if (NILP (Vexecuting_kbd_macro)) { if (nonundocount <= 0 || nonundocount >= 20) { remove_boundary = false; nonundocount = 0; } nonundocount++; } #+end_src current_kboard? KVAR? And why the "executing_kbd_macro" check? And, secondly, in undo.c I now have the following. #+begin_src c /* Allocate a cons cell to be the undo boundary after this command. */ if (NILP (pending_boundary)) pending_boundary = Fcons (Qnil, Qnil); /* Switch temporarily to the buffer that was changed. */ current_buffer = buf; // PWL running with the wrong current-buffer run_undoable_change (); if (MODIFF <= SAVE_MODIFF) record_first_change (); #+end_src Now, I know that you have told me previously that this is bad because the `current-buffer = buf` line only impacts on C and the call to lisp is likely to be wrong. Do I just change the middle bit to ... #+begin_src c /* Switch temporarily to the buffer that was changed. */ current_buffer = buf; // PWL running with the wrong current-buffer set_buffer_internal(b); run_undoable_change (); #+end_src and then switch back again later like so? #+begin_src c current_buffer = obuf; set_buffer_internal(current_buffer); #+end_src Cheers! Phil