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.bugs Subject: bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer Date: Mon, 04 Jul 2016 21:34:08 +0100 Message-ID: <87vb0lta67.fsf@russet.org.uk> References: <83h9cavdgj.fsf@gnu.org> <87poqyy2tc.fsf@metalevel.at> <87vb0qqrkz.fsf@russet.org.uk> <87h9c9zx75.fsf@metalevel.at> <834m89vmyv.fsf@gnu.org> <878txlsbdb.fsf@russet.org.uk> <87furtccdv.fsf@metalevel.at> <877fd5q9te.fsf@russet.org.uk> <83bn2gtruk.fsf@gnu.org> <87k2h37pvb.fsf@russet.org.uk> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: ger.gmane.org 1467664529 28972 80.91.229.3 (4 Jul 2016 20:35:29 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 4 Jul 2016 20:35:29 +0000 (UTC) Cc: 23871@debbugs.gnu.org, triska@metalevel.at To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Jul 04 22:35:19 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@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 1bKAaL-000474-TK for geb-bug-gnu-emacs@m.gmane.org; Mon, 04 Jul 2016 22:35:18 +0200 Original-Received: from localhost ([::1]:50401 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKAaK-0007ow-RB for geb-bug-gnu-emacs@m.gmane.org; Mon, 04 Jul 2016 16:35:16 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:53522) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKAa8-0007lE-R2 for bug-gnu-emacs@gnu.org; Mon, 04 Jul 2016 16:35:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKAa6-0007r5-NH for bug-gnu-emacs@gnu.org; Mon, 04 Jul 2016 16:35:04 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:53341) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKAa6-0007qz-JT for bug-gnu-emacs@gnu.org; Mon, 04 Jul 2016 16:35:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bKAa6-0003we-BT for bug-gnu-emacs@gnu.org; Mon, 04 Jul 2016 16:35:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: phillip.lord@russet.org.uk (Phillip Lord) Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 04 Jul 2016 20:35:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 23871 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 23871-submit@debbugs.gnu.org id=B23871.146766446415114 (code B ref 23871); Mon, 04 Jul 2016 20:35:02 +0000 Original-Received: (at 23871) by debbugs.gnu.org; 4 Jul 2016 20:34:24 +0000 Original-Received: from localhost ([127.0.0.1]:37445 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bKAZT-0003vh-9g for submit@debbugs.gnu.org; Mon, 04 Jul 2016 16:34:23 -0400 Original-Received: from cloud103.planethippo.com ([31.216.48.48]:43615) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bKAZQ-0003vT-UX for 23871@debbugs.gnu.org; Mon, 04 Jul 2016 16:34:21 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=russet.org.uk; s=default; h=Content-Type:MIME-Version:References:Message-ID :Date:In-Reply-To:Subject:Cc:To:From; bh=cbwuxuQOl0biyGlEAl5I6csRhHIkNdZoYBkriBoHmwA=; b=poq44/bm4TK52oXp7lEHYCxikq zfc5NFSKQNslx/CCOzqrTtfuSlny0PNah9e3pxlbCltgVW5r9YyrBnEEN+SjxOw49WLrx+MZ1swNu roLSGUNqst53sPC3VZqoHMOgz2lhgV9O5nVH8gHVc/JSW5mMU4h4RLG8JMQMH8roBZoeX4v4sc0eb Vka5UXKm5U/RUndS1VRP4cyT+B+ao2ZmRuYfoWy0jXNcqsd39/tmzfNv+eTkTF7SQXDiMvhSB4FY+ kBlnHO6bbxWuyMv22iWX8KNpVqYJfo49uxNsr91YsocyOTakgQ2E+LyOZCSoTVHM4qn2eTZdt20f7 d2uvRHQA==; Original-Received: from cpc1-benw10-2-0-cust373.gate.cable.virginm.net ([77.98.219.118]:50811 helo=russet.org.uk) by cloud103.planethippo.com with esmtpsa (TLSv1.2:DHE-RSA-AES128-SHA:128) (Exim 4.86_1) (envelope-from ) id 1bKAZJ-002ZWp-U9; Mon, 04 Jul 2016 21:34:14 +0100 In-Reply-To: (Stefan Monnier's message of "Sun, 03 Jul 2016 17:33:15 -0400") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.95 (gnu/linux) X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - cloud103.planethippo.com X-AntiAbuse: Original Domain - debbugs.gnu.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - russet.org.uk X-Get-Message-Sender-Via: cloud103.planethippo.com: authenticated_id: phillip.lord@russet.org.uk X-Authenticated-Sender: cloud103.planethippo.com: phillip.lord@russet.org.uk X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:120417 Archived-At: --=-=-= Content-Type: text/plain Stefan Monnier writes: >>> From d4e9e44402fdf248ba4bc895e914d4cc5580f229 Mon Sep 17 00:00:00 2001 >> From: Phillip Lord >> Date: Thu, 30 Jun 2016 22:06:00 +0100 >> Subject: [PATCH] Fix missing point information in undo > >> * src/undo.c (record_insert): Use record_point instead of >> prepare_record, and do so unconditionally. >> (prepare_record): Do not record first change. >> (record_point): Now conditional on state before the last command. >> (record_delete): Call record_point unconditionally. > > I haven't had time to look in detail at this patch, so it's hard for me > to give a strong agreement. Maybe some explanations would help. This stems from two reported bugs: one that point was not being correctly restored after an insertion away from point, and another than point was not being correctly restored after the first change in a buffer. > Could you give a verbosish "commit log" explaining the reason behind > each hunk? Yes. That is below with the final patch (including changes you have requested). >> @@ -40,16 +40,13 @@ prepare_record (void) >> /* Allocate a cons cell to be the undo boundary after this command. */ >> if (NILP (pending_boundary)) >> pending_boundary = Fcons (Qnil, Qnil); >> - >> - if (MODIFF <= SAVE_MODIFF) >> - record_first_change (); >> } > > Not sure why/how this is related to the other changes, nor why this code > was there in the first place. It was a refactor since these two conditinals were always called together. A mistake unfortunately (see below). > But in any case, the comment before prepare_record needs to be updated, > because it seems to describe neither the current behavior, nor the > behavior after applying the above hunk. Done. > BTW, I notice that in the current code (emacs-25), there's one other > call to record_first_change (in record_property_change), and it could be > replaced with a call to prepare_record, so it begs the question whether > the above hunk should also apply to record_property_change as well. Don't understand. Think that the call to record_first_change is necessary. >> /* Record point as it was at beginning of this command. >> - PT is the position of point that will naturally occur as a result of the >> + BEG is the position of point that will naturally occur as a result of the >> undo record that will be added just after this command terminates. */ >> static void >> -record_point (ptrdiff_t pt) >> +record_point (ptrdiff_t beg) >> { >> /* Don't record position of pt when undo_inhibit_record_point holds. */ >> if (undo_inhibit_record_point) >> @@ -60,13 +57,16 @@ record_point (ptrdiff_t pt) >> at_boundary = ! CONSP (BVAR (current_buffer, undo_list)) >> || NILP (XCAR (BVAR (current_buffer, undo_list))); > > You said: > > The problem was caused because of undo only records point after a > boundary (or the first element). I'd changed things during slightly when > I update undo.c so that the timestamp list got added before checking > whether I was at a boundary, hence blocking addition of the point > restoration information. > > so maybe the computation of at_boundary above should be tweaked to > ignore timestamps? That's a possibility, but it appears more complex that re-ordering the methods. >> /* If we are just after an undo boundary, and >> point wasn't at start of deleted range, record where it was. */ >> - if (at_boundary) >> + if (at_boundary >> + && point_before_last_command_or_undo != beg >> + && buffer_before_last_command_or_undo == current_buffer ) >> bset_undo_list (current_buffer, >> - Fcons (make_number (pt), >> + Fcons (make_number (point_before_last_command_or_undo), >> BVAR (current_buffer, undo_list))); > > I like this part. Good! >> - if (point_before_last_command_or_undo != beg >> - && buffer_before_last_command_or_undo == current_buffer) >> - record_point (point_before_last_command_or_undo); >> + prepare_record (); >> + >> + record_point (beg); > > And I like this part too. Good! Here is a hunk-by-hunk description. Comments come before the hunk they apply to, and I've split some hunks where necessary. The final patch is attached (it has some more doc, and another call to "prepare_record" added) also. Having read it though again, I cannot understand why the boundary is pre-allocated at all, but that is a discussion for another time. Sorry for the hassle this late in the development process. Phil `prepare_record` had been refactored out when undo.c was re-written. as the two conditionals were being called together in many places. This turns out to have been been a mistake, as we need to call something between these two conditionals in one place (`record_point`). #+begin_src diff @@ -31,25 +31,21 @@ along with GNU Emacs. If not, see . */ an undo-boundary. */ static Lisp_Object pending_boundary; -/* Record point as it was at beginning of this command (if necessary) - and prepare the undo info for recording a change. - Prepare the undo info for recording a change. */ +/* Prepare the undo info for recording a change. */ static void prepare_record (void) { /* Allocate a cons cell to be the undo boundary after this command. */ if (NILP (pending_boundary)) pending_boundary = Fcons (Qnil, Qnil); - - if (MODIFF <= SAVE_MODIFF) - record_first_change (); } #+end_src The semantics of record_point had been changed also, so that pt became the point that was to be recorded, rather than the point that we shouldn't record. The point we should record is available as global state (`point_before_last_command_or_undo`) anyway. #+begin_src diff -/* Record point as it was at beginning of this command. - PT is the position of point that will naturally occur as a result of the - undo record that will be added just after this command terminates. */ +/* Record point, if necessary, as it was at beginning of this command. + BEG is the position of point that will naturally occur as a result + of the undo record that will be added just after this command + terminates. */ static void -record_point (ptrdiff_t pt) +record_point (ptrdiff_t beg) { /* Don't record position of pt when undo_inhibit_record_point holds. */ if (undo_inhibit_record_point) #+end_src Doc added #+begin_src diff @@ -57,16 +53,23 @@ record_point (ptrdiff_t pt) bool at_boundary; + /* Check whether we are at a boundary now, in case we record the + first change. */ at_boundary = ! CONSP (BVAR (current_buffer, undo_list)) || NILP (XCAR (BVAR (current_buffer, undo_list))); #+end_src Record point is called from only two places, in both cases before immediately after `prepare_record`, so we no longer call it here. However, we must bring back the conditional statement because this is no longer called in prepare_record, as in this case, it must be called after the `at_boundary` check. The alternative, which is making the `at_boundary` check accept "nil" then a timestamp as a boundary sounds more complex. #+begin_src diff - prepare_record (); + /* If this is the first change since save, then record this.*/ + if (MODIFF <= SAVE_MODIFF) + record_first_change (); #+end_src `record_point` is now called unconditionally in the places where it is called, so we more incorporate the conditional checks here. Take the point to be recorded from global state. #+begin_src diff - /* If we are just after an undo boundary, and - point wasn't at start of deleted range, record where it was. */ - if (at_boundary) + /* If we are just after an undo boundary, and point was not at start + of deleted range, and the recorded point was in this buffer, then + record where it was. */ + if (at_boundary + && point_before_last_command_or_undo != beg + && buffer_before_last_command_or_undo == current_buffer ) bset_undo_list (current_buffer, - Fcons (make_number (pt), + Fcons (make_number (point_before_last_command_or_undo), BVAR (current_buffer, undo_list))); } #+end_src This record_point was simply missing -- point was never recorded after an insertion, and this has been directly added to address the bug. #+begin_src diff @@ -85,6 +88,8 @@ record_insert (ptrdiff_t beg, ptrdiff_t length) prepare_record (); + record_point (beg); + /* If this is following another insertion and consecutive with it in the buffer, combine the two. */ if (CONSP (BVAR (current_buffer, undo_list))) #+end_src Refactor for completeness. #+begin_src diff @@ -120,9 +125,7 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to) register struct Lisp_Marker *m; register ptrdiff_t charpos, adjustment; - /* Allocate a cons cell to be the undo boundary after this command. */ - if (NILP (pending_boundary)) - pending_boundary = Fcons (Qnil, Qnil); + prepare_record(); for (m = BUF_MARKERS (current_buffer); m; m = m->next) { #+end_src Remove the conditionality, as this is now inside record_point. Change the parameter to `record_point` to reflect the changed semantics of that method. Move `prepare_record` call out of both sides of a conditional. The re-ordering of `prepare_record` and `record_point` should not matter (famous last words). #+begin_src diff @@ -163,19 +168,17 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool record_markers) if (EQ (BVAR (current_buffer, undo_list), Qt)) return; - if (point_before_last_command_or_undo != beg - && buffer_before_last_command_or_undo == current_buffer) - record_point (point_before_last_command_or_undo); + prepare_record (); + + record_point (beg); if (PT == beg + SCHARS (string)) { XSETINT (sbeg, -beg); - prepare_record (); } else { XSETFASTINT (sbeg, beg); - prepare_record (); } /* primitive-undo assumes marker adjustments are recorded #+end_src Refactor for completeness. #+begin_src diff @@ -234,9 +237,7 @@ record_property_change (ptrdiff_t beg, ptrdiff_t length, if (EQ (BVAR (buf, undo_list), Qt)) return; - /* Allocate a cons cell to be the undo boundary after this command. */ - if (NILP (pending_boundary)) - pending_boundary = Fcons (Qnil, Qnil); + prepare_record(); if (MODIFF <= SAVE_MODIFF) record_first_change (); +end_src --=-=-= Content-Type: text/x-diff Content-Disposition: attachment; filename=0001-Fix-missing-point-information-in-undo.patch >From c7e116d16299dfe3bb13a6574f9b03542cb72099 Mon Sep 17 00:00:00 2001 From: Phillip Lord Date: Thu, 30 Jun 2016 22:06:00 +0100 Subject: [PATCH] Fix missing point information in undo * src/undo.c (record_insert): Use record_point instead of prepare_record, and do so unconditionally. (prepare_record): Do not record first change. (record_point): Now conditional on state before the last command. (record_delete): Call record_point unconditionally. (record_property_change): Use prepare_record. (record_marker_adjustments): Use prepare_record. Addresses Bug# 21722 --- src/undo.c | 51 +++++++++++++++++++++---------------------- test/automated/simple-test.el | 19 +++++++++++++++- 2 files changed, 43 insertions(+), 27 deletions(-) diff --git a/src/undo.c b/src/undo.c index be5b270..2676c3d 100644 --- a/src/undo.c +++ b/src/undo.c @@ -31,25 +31,21 @@ along with GNU Emacs. If not, see . */ an undo-boundary. */ static Lisp_Object pending_boundary; -/* Record point as it was at beginning of this command (if necessary) - and prepare the undo info for recording a change. - Prepare the undo info for recording a change. */ +/* Prepare the undo info for recording a change. */ static void prepare_record (void) { /* Allocate a cons cell to be the undo boundary after this command. */ if (NILP (pending_boundary)) pending_boundary = Fcons (Qnil, Qnil); - - if (MODIFF <= SAVE_MODIFF) - record_first_change (); } -/* Record point as it was at beginning of this command. - PT is the position of point that will naturally occur as a result of the - undo record that will be added just after this command terminates. */ +/* Record point, if necessary, as it was at beginning of this command. + BEG is the position of point that will naturally occur as a result + of the undo record that will be added just after this command + terminates. */ static void -record_point (ptrdiff_t pt) +record_point (ptrdiff_t beg) { /* Don't record position of pt when undo_inhibit_record_point holds. */ if (undo_inhibit_record_point) @@ -57,16 +53,23 @@ record_point (ptrdiff_t pt) bool at_boundary; + /* Check whether we are at a boundary now, in case we record the + first change. */ at_boundary = ! CONSP (BVAR (current_buffer, undo_list)) || NILP (XCAR (BVAR (current_buffer, undo_list))); - prepare_record (); + /* If this is the first change since save, then record this.*/ + if (MODIFF <= SAVE_MODIFF) + record_first_change (); - /* If we are just after an undo boundary, and - point wasn't at start of deleted range, record where it was. */ - if (at_boundary) + /* If we are just after an undo boundary, and point was not at start + of deleted range, and the recorded point was in this buffer, then + record where it was. */ + if (at_boundary + && point_before_last_command_or_undo != beg + && buffer_before_last_command_or_undo == current_buffer ) bset_undo_list (current_buffer, - Fcons (make_number (pt), + Fcons (make_number (point_before_last_command_or_undo), BVAR (current_buffer, undo_list))); } @@ -85,6 +88,8 @@ record_insert (ptrdiff_t beg, ptrdiff_t length) prepare_record (); + record_point (beg); + /* If this is following another insertion and consecutive with it in the buffer, combine the two. */ if (CONSP (BVAR (current_buffer, undo_list))) @@ -120,9 +125,7 @@ record_marker_adjustments (ptrdiff_t from, ptrdiff_t to) register struct Lisp_Marker *m; register ptrdiff_t charpos, adjustment; - /* Allocate a cons cell to be the undo boundary after this command. */ - if (NILP (pending_boundary)) - pending_boundary = Fcons (Qnil, Qnil); + prepare_record(); for (m = BUF_MARKERS (current_buffer); m; m = m->next) { @@ -163,19 +166,17 @@ record_delete (ptrdiff_t beg, Lisp_Object string, bool record_markers) if (EQ (BVAR (current_buffer, undo_list), Qt)) return; - if (point_before_last_command_or_undo != beg - && buffer_before_last_command_or_undo == current_buffer) - record_point (point_before_last_command_or_undo); + prepare_record (); + + record_point (beg); if (PT == beg + SCHARS (string)) { XSETINT (sbeg, -beg); - prepare_record (); } else { XSETFASTINT (sbeg, beg); - prepare_record (); } /* primitive-undo assumes marker adjustments are recorded @@ -234,9 +235,7 @@ record_property_change (ptrdiff_t beg, ptrdiff_t length, if (EQ (BVAR (buf, undo_list), Qt)) return; - /* Allocate a cons cell to be the undo boundary after this command. */ - if (NILP (pending_boundary)) - pending_boundary = Fcons (Qnil, Qnil); + prepare_record(); if (MODIFF <= SAVE_MODIFF) record_first_change (); diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el index 40cd1d2..c41d010 100644 --- a/test/automated/simple-test.el +++ b/test/automated/simple-test.el @@ -311,6 +311,7 @@ undo-test-point-after-forward-kill (undo-test-point-after-forward-kill)))) (defmacro simple-test-undo-with-switched-buffer (buffer &rest body) + (declare (indent 1) (debug t)) (let ((before-buffer (make-symbol "before-buffer"))) `(let ((,before-buffer (current-buffer))) (unwind-protect @@ -340,8 +341,24 @@ simple-test-undo-with-switched-buffer (point-min) (point-max)))))) +(ert-deftest missing-record-point-in-undo () + "Check point is being restored correctly. - +See Bug#21722." + (should + (= 5 + (with-temp-buffer + (generate-new-buffer " *temp*") + (emacs-lisp-mode) + (setq buffer-undo-list nil) + (insert "(progn (end-of-line) (insert \"hello\"))") + (beginning-of-line) + (forward-char 4) + (undo-boundary) + (eval-defun nil) + (undo-boundary) + (undo) + (point))))) (provide 'simple-test) ;;; simple-test.el ends here -- 2.9.0 --=-=-=--