From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.bugs Subject: bug#23871: 25.1.50; Undo unexpectedly leads to blank buffer Date: Mon, 04 Jul 2016 17:32:45 -0400 Message-ID: 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> <87vb0lta67.fsf@russet.org.uk> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1467668003 14053 80.91.229.3 (4 Jul 2016 21:33:23 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 4 Jul 2016 21:33:23 +0000 (UTC) Cc: 23871@debbugs.gnu.org, triska@metalevel.at To: phillip.lord@russet.org.uk (Phillip Lord) Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Jul 04 23:33:11 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 1bKBUM-0003UZ-BL for geb-bug-gnu-emacs@m.gmane.org; Mon, 04 Jul 2016 23:33:10 +0200 Original-Received: from localhost ([::1]:50619 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKBUL-0005Mh-Ls for geb-bug-gnu-emacs@m.gmane.org; Mon, 04 Jul 2016 17:33:09 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44293) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKBUG-0005Ma-Fp for bug-gnu-emacs@gnu.org; Mon, 04 Jul 2016 17:33:05 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bKBUE-0003aR-CZ for bug-gnu-emacs@gnu.org; Mon, 04 Jul 2016 17:33:03 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:53361) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bKBUE-0003aN-8v for bug-gnu-emacs@gnu.org; Mon, 04 Jul 2016 17:33:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bKBUE-0005Dg-1P for bug-gnu-emacs@gnu.org; Mon, 04 Jul 2016 17:33:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 04 Jul 2016 21:33:01 +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.146766797920056 (code B ref 23871); Mon, 04 Jul 2016 21:33:01 +0000 Original-Received: (at 23871) by debbugs.gnu.org; 4 Jul 2016 21:32:59 +0000 Original-Received: from localhost ([127.0.0.1]:37465 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bKBUA-0005DQ-Rg for submit@debbugs.gnu.org; Mon, 04 Jul 2016 17:32:59 -0400 Original-Received: from smtp-as-02-03.vtxnet.net ([194.38.175.142]:48275 helo=smtp-as-02.vtxnet.net) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bKBU7-0005DD-T3 for 23871@debbugs.gnu.org; Mon, 04 Jul 2016 17:32:57 -0400 Original-Received: from smtp-as-02.vtxnet.net (localhost [127.0.0.1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by smtp-as-02.vtxnet.net (Postfix) with ESMTPS id 923A8451B5; Mon, 4 Jul 2016 23:32:53 +0200 (CEST) Original-Received: from smtp-pri-01.vtxnet.net (smtp-pri-01.vtxnet.net [212.147.62.135]) by smtp-as-02.vtxnet.net (Postfix) with ESMTP id F18D14519B; Mon, 4 Jul 2016 23:32:48 +0200 (CEST) Original-Received: from fmsmemgm.homelinux.net (dyn.144-85-234-142.dsl.vtx.ch [144.85.234.142]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by smtp-pri-01.vtxnet.net (VTX Services SA) with ESMTP id DCB8BCFCEC; Mon, 4 Jul 2016 23:32:48 +0200 (CEST) Original-Received: by fmsmemgm.homelinux.net (Postfix, from userid 20848) id E6195AE5F2; Mon, 4 Jul 2016 17:32:45 -0400 (EDT) In-Reply-To: <87vb0lta67.fsf@russet.org.uk> (Phillip Lord's message of "Mon, 04 Jul 2016 21:34:08 +0100") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) 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:120418 Archived-At: >> 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. In record_property_change we have (among other things) the exact same code as in prepare_record (i.e. there's code duplication). So we could/should replace those 4 lines of code with a call to prepare_record. But if we do, then your (previous) patch changes the behavior of record_property_change, whereas if we don't, your (previous) patch doesn't change its behavior. Anyway, your new patch addresses this. > That's a possibility, but it appears more complex than re-ordering the > methods. [...] > 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. More complex but more robust. I think it'd be worthwhile to put a FIXME comment in there, at least. E.g. the above explanation should be put inside the code. > + /* 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))); > } I think the comment should explain the intention better. It is currently too close to a simple paraphrase of the code. I suggest "If it's the first change since the last boundary, and the upcoming undo record wouldn't restore point correctly, then record where it was". Other than that it looks good, thank you for the detailed explanation. Stefan