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: Wed, 21 Oct 2015 20:27:57 +0100 Message-ID: <87mvvc9g3m.fsf@russet.org.uk> 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 1445455741 21484 80.91.229.3 (21 Oct 2015 19:29:01 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 21 Oct 2015 19:29:01 +0000 (UTC) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Oct 21 21:28:54 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 1Zoz43-0005uH-50 for ged-emacs-devel@m.gmane.org; Wed, 21 Oct 2015 21:28:47 +0200 Original-Received: from localhost ([::1]:53667 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zoz42-0005JG-BD for ged-emacs-devel@m.gmane.org; Wed, 21 Oct 2015 15:28:46 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:47733) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zoz3f-0005G6-CH for emacs-devel@gnu.org; Wed, 21 Oct 2015 15:28:24 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zoz3c-0002ZG-5Q for emacs-devel@gnu.org; Wed, 21 Oct 2015 15:28:23 -0400 Original-Received: from cheviot12.ncl.ac.uk ([128.240.234.12]:33674) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zoz3b-0002Xl-Ty for emacs-devel@gnu.org; Wed, 21 Oct 2015 15:28:20 -0400 Original-Received: from smtpauth-vm.ncl.ac.uk ([10.8.233.129] helo=smtpauth.ncl.ac.uk) by cheviot12.ncl.ac.uk with esmtp (Exim 4.63) (envelope-from ) id 1Zoz3Y-0001ZI-BT; Wed, 21 Oct 2015 20:28:16 +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 1Zoz3X-00054u-9Q; Wed, 21 Oct 2015 20:28:15 +0100 In-Reply-To: (Stefan Monnier's message of "Sun, 18 Oct 2015 12:51:41 -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.12 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:192324 Archived-At: Stefan Monnier writes: >> 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. Okay. >> 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? Yeah, this was my mistake. I have fixed this now. I believe my code means that other "amalgamating" commands could be added freely (i.e. from within lisp) also. >> 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. I've moved all of this to lisp now. I don't currently have the executing_kbd_macro check in, but I guess I could add. >> ;;; Code: >> - >> (eval-when-compile (require 'cl-lib)) > > Spurious change. Oops > >> + ;; 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. Yes. Seemed like a good idea, but wasn't. >> +(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. These and the other stylistic elements that you mentioned have been fixed. >> + (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. I did think about this, or something similar. But, after self-insert-command, actually, undo-undoably-changed-buffers tells all the buffers that were modified since the last time we added an auto-boundary. This will only be the same as the buffers which have changed as a result of self-insert-command iff undo-undoably-changed-buffers was nil before the command. It need not be if buffers are undoably-changing as a result of a timer or a process for instance. My other concern is that after a self-insert-command, I can guarantee that the current-buffer hasn't changed much (normally by one char). But, for example, with lentic a self-insert-command in one buffer can in worse case result in all the characters in another buffer changing. So amalgamating these changes might result in a big buffer-undo-list. It should be possible for package developers who care to run the amalgamation code in the buffers of their choice, I think. Phil