From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] fix/no-undo-boundary-on-secondary-buffer-change f59d1be: Move undo amalgamation to lisp. Date: Sun, 18 Oct 2015 12:51:41 -0400 Message-ID: References: <20151005134118.10933.50859@vcs.savannah.gnu.org> <87h9m52sh8.fsf@russet.org.uk> <87h9lqbk8m.fsf@russet.org.uk> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1445187131 25105 80.91.229.3 (18 Oct 2015 16:52:11 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Sun, 18 Oct 2015 16:52:11 +0000 (UTC) Cc: emacs-devel@gnu.org To: phillip.lord@russet.org.uk (Phillip Lord) Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Oct 18 18:52:03 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 1ZnrBi-0000ya-Ek for ged-emacs-devel@m.gmane.org; Sun, 18 Oct 2015 18:52:02 +0200 Original-Received: from localhost ([::1]:34641 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZnrBh-0004T2-Pv for ged-emacs-devel@m.gmane.org; Sun, 18 Oct 2015 12:52:01 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:40069) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZnrBS-0004SP-Ju for emacs-devel@gnu.org; Sun, 18 Oct 2015 12:51:47 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZnrBP-0006Lm-DC for emacs-devel@gnu.org; Sun, 18 Oct 2015 12:51:46 -0400 Original-Received: from ironport2-out.teksavvy.com ([206.248.154.181]:57518) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZnrBP-0006K3-5D for emacs-devel@gnu.org; Sun, 18 Oct 2015 12:51:43 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: A0CYDQA731xV/0qOCkxcgxCEAshgBAICgTw8EQEBAQEBAQGBCkEFg10BAQMBViMFCws0BwsUGA1QiAsIzyMBAQgCAR+KOIEChC1YB4QtBZAhE45jg2uDZopDghSBRSOBZlWBWSKBNYFDAQEB X-IPAS-Result: A0CYDQA731xV/0qOCkxcgxCEAshgBAICgTw8EQEBAQEBAQGBCkEFg10BAQMBViMFCws0BwsUGA1QiAsIzyMBAQgCAR+KOIEChC1YB4QtBZAhE45jg2uDZopDghSBRSOBZlWBWSKBNYFDAQEB X-IronPort-AV: E=Sophos;i="5.13,465,1427774400"; d="scan'208";a="170104653" Original-Received: from 76-10-142-74.dsl.teksavvy.com (HELO fmsmemgm.homelinux.net) ([76.10.142.74]) by ironport2-out.teksavvy.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 18 Oct 2015 12:51:41 -0400 Original-Received: by fmsmemgm.homelinux.net (Postfix, from userid 20848) id 5B58AAE0E7; Sun, 18 Oct 2015 12:51:41 -0400 (EDT) In-Reply-To: <87h9lqbk8m.fsf@russet.org.uk> (Phillip Lord's message of "Fri, 16 Oct 2015 22:02:01 +0100") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 206.248.154.181 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:191972 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. See some comments on the latest code below. > 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. The current behavior is to call it right *before* running the command, i.e. after running post-command-hook, and even after running the next pre-command-hook. I recommend you keep the bikeshed of the same color. If someone wants to change the behavior she can use an advice anyway. And we can move it to a hook later on if we want. > 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. Try C-d C-d C-d C-d C-x u. It should undo all 4 C-d rather than only the last one. This is new in Emacs-25. > 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. Have you tested with Emacs<25 by any chance? > current_kboard? KVAR? KVAR (current_kboard, Vlast_command) is the C code needed to find the value of Elisp's `last-command', because it is a "keyboard-local" variable. > And why the "executing_kbd_macro" check? IIUC this is inherited from Emacs<19 and I just blindly preserved it. I recommend you just do the same unless it causes problems. > current_buffer = buf; > set_buffer_internal(b); This break because set_buffer_internal needs to know what was the previous current_buffer to do its job correctly. IOW either you use "current_buffer = buf;" and you're super extra careful to only do trivial things, or you do "set_buffer_internal(b);", but you don't want to do both. Stefan > ;;; Code: > - > (eval-when-compile (require 'cl-lib)) Spurious change. > + ;; We need to set `undo-last-boundary' to nil as we are about to > + ;; delete the last boundary, so we want to not assume anything about > + ;; the boundary before this one > + (setq undo-last-boundary nil) Why is this needed? The undo-last-boundary handling should work for "any" command without any extra code (except for self-insert-command or delete-char, obviously) and I don't see why undo should be different here. I could imagine the above code as something hypothetically useful as an optimization, but if it's really needed, I think it hints at a problem elsewhere. E.g. maybe uses of undo-last-boundary should always check (eq this-command last-command), or undo-last-boundary should (just like the old last_undo_boundary) keep a reference to the value of buffer-undo-list, or the *command-loop* should set undo-last-boundary to nil whenever this-command is different from last-command, ... > +;; This section helps to prevent undo-lists from getting too large. It > +;; achieves by checking that no buffer has an undo-list which is large > +;; and has no `undo-boundary', a condition that will block garbage > +;; collection of that list. This happens silently and in most cases > +;; not at all, as generally, buffers add their own undo-boundary. > +;; > +;; It will still fail if a large amount of material is added or > +;; removed from a buffer with any rapidity and no undo-boundary. In > +;; this case, the `undo-outer-limit' machinary will operate; this is > +;; considered to be exceptional the user is warned. I think this comment reflects an older version of the code. > +(defun undo-needs-boundary-p () I recommend you use an "undo--" prefix for most of the funs&vars you've introduced, unless you explicitly want to encourage people to make use of it from third-party packages. > + "Returns non-nil if `buffer-undo-list' needs a boundary at the start." ^^^^^^^ M-x checkdoc-current-buffer RET will tell you how to fix this. > + (and > + ;; `buffer-undo-list' can be t. > + (listp buffer-undo-list) > + ;; The first element of buffer-undo-list is not nil. > + (car buffer-undo-list))) aka (car-safe buffer-undo-list). > +(defun undo-last-boundary-sic-p (last-boundary) The arg is always undo-last-boundary, so you could just drop the arg. > + (mapc > + (lambda (b) Use dolist instead. > +(defun undo-auto-pre-self-insert-command() ^^ Missing space > + (condition-case err > + (let ((last-sic-count > + (undo-last-boundary-sic-p undo-last-boundary))) > + (when > + last-sic-count > + (if > + (< last-sic-count 20) > + (progn (undo-auto-message "(changed) Removing last undo") > + (setq buffer-undo-list > + (cdr buffer-undo-list))) > + (progn (undo-auto-message "Reset sic to 0") > + (setq undo-last-boundary > + '(command self-insert-command 0)))))) > + (error > + (undo-auto-message "pre-command-error %s" > + (error-message-string err))))) After self-insert-command, undo-undoably-changed-buffers now tells us all the buffers that self-insert-command modified, so if we keep this list in undo-last-boundary, we can (better: should) here remove the boundaries in all those buffers. > + // PWL remove for now > + //if (XFASTINT (n) < 2) > + //remove_excessive_undo_boundaries (); > + call0(Qundo_auto_pre_self_insert_command); Better keep the (XFASTINT (n) < 2) test. > - 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)); > - } Here's where you want to call your undo-auto-post-command-hook (which should be renamed to describe what it does rather than when it's called). > + call0(Qundo_undoable_change); ^^ Missing space. > + // PWL running with the wrong current-buffer Please use /*...*/ comments rather than //...\n comments. It's not really important, but that's what we use everywhere else. > + Fset(Qundo_last_boundary,Qnil); ^^ Another missing space. > + DEFVAR_LISP ("undo-last-boundary", > + Vundo_last_boundary, > + doc: /* TODO > +*/); This belongs in the Lisp code.