bug#4061: 23.1.50; C-x C-v and saveplace

bug#4061: 23.1.50; C-x C-v and saveplace

[Leo]

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

With saveplace enabled, it will be nice if place (point) can survive C-x
C-v. At the moment, C-x C-v will move the point to a 'random' place.

Leo

bug#4061: 23.1.50; C-x C-v and saveplace

[Leo]

On 2009-08-06 04:29 +0100, Leo wrote:
> Please describe exactly what actions triggered the bug
> and the precise symptoms of the bug:
>
> With saveplace enabled, it will be nice if place (point) can survive C-x
> C-v. At the moment, C-x C-v will move the point to a 'random' place.
>
> Leo

In `find-alternate-file', the old buffer is renamed, the new buffer is
created and then the renamed old buffer is killed. so the place is not
saved before re-opening the file. It seems at the moment for a user the
clean way to implement this feature is by using defadvice.

But I think this is a very useful feature to have.

-- 
Leo's Emacs uptime: 11 days, 19 hours, 50 minutes, 33 seconds

bug#4061: 23.1.50; C-x C-v and saveplace

[Chong Yidong]

Hi Karl, are you still maintaining saveplace.el?  Could you take a look
at this bug report?


Leo <sdl.web@gmail.com> wrote:

> With saveplace enabled, it will be nice if place (point) can survive
> C-x C-v. At the moment, C-x C-v will move the point to a 'random'
> place.

bug#4061: 23.1.50; C-x C-v and saveplace

[Karl Fogel]

Chong Yidong <cyd@stupidchicken.com> writes:
> Hi Karl, are you still maintaining saveplace.el?  Could you take a look
> at this bug report?

Yes, I'll take a look.  Thank you for calling my attention to it.

-Karl

bug#4061: 23.1.50; C-x C-v and saveplace

[Leo]

On 2009-08-16 17:11 +0100, Karl Fogel wrote:
> Chong Yidong <cyd@stupidchicken.com> writes:
>> Hi Karl, are you still maintaining saveplace.el?  Could you take a look
>> at this bug report?
>
> Yes, I'll take a look.  Thank you for calling my attention to it.
>
> -Karl

I have been using the following simple defadvice.

(defadvice find-alternate-file (before save-place activate)
  "Save place before `find-alternate-file'."
  (save-place-to-alist))

But I think a solution involving no defadvice is better.

Hope this helps.

Leo

-- 
Leo's Emacs uptime: 13 days, 13 hours, 56 minutes, 25 seconds

bug#4061: 23.1.50; C-x C-v and saveplace

[Karl Fogel]

I know what causes this now.  saveplace.el works like this:

  (add-hook 'kill-buffer-hook 'save-place-to-alist)

Now, `save-place-to-alist' checks `buffer-file-name' and (properly) does
nothing if there is no associated file.  Since `find-alternate-file'
unsets `buffer-file-name' after renaming the old buffer but before
killing it, that effectively makes `save-place-to-alist' a no-op in the
old buffer.

It's not even clear what the most desirable behavior is.  For example,
in `find-alternate-file' (without my patch), if the old buffer is
modified, should we still save place before killing it?  I think so; or
rather, I think we should do whatever saveplace.el does if one kills a
modified buffer the normal way.

I'm still thinking.  My patch below isn't really the right thing (see
below for why), but I wanted to record this all here to remember it.

[[[
* emacs/emacs-cvs/lisp/files.el
  (find-alternate-file): Restore certain state in the old buffer
    before killing it, so that hooks behave as expected.  This addresses
    http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4061.

NOTE: DRAFT PATCH ONLY, DO NOT COMMIT.  With this patch, doing C-x C-v
in a modified buffer visiting a file causes the user to be prompted to
save buffer " **lose**" (see files.el:find-alternate-file for why)
after they have successfully found their new file.  That is hardly a
desirable behavior.

I will post for others' thoughts on whether the original bug is a bug,
and if it is what is the best way to fix it.
]]]

[[[
* emacs/emacs-cvs/lisp/files.el
  (find-alternate-file): Restore certain state in the old buffer
    before killing it, so that hooks behave as expected.  This addresses
    http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4061.

NOTE: DRAFT PATCH ONLY, DO NOT COMMIT.  With this patch, doing C-x C-v
in a modified buffer visiting a file causes the user to be prompted to
save buffer " **lose**" (see files.el:find-alternate-file for why)
after they have successfully found their new file.  That is hardly a
desirable behavior.

I will post for others' thoughts on whether the original bug is a bug,
and if it is what is the best way to fix it.
]]]

Index: lisp/files.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/files.el,v
retrieving revision 1.1076
diff -u -r1.1076 files.el
--- lisp/files.el	4 Sep 2009 03:18:11 -0000	1.1076
+++ lisp/files.el	4 Sep 2009 21:30:00 -0000
@@ -1507,17 +1507,24 @@
 	  ;; Likewise for dired buffers.
 	  (setq dired-directory nil)
 	  (find-file filename wildcards))
-      (when (eq obuf (current-buffer))
-	;; This executes if find-file gets an error
-	;; and does not really find anything.
-	;; We put things back as they were.
-	;; If find-file actually finds something, we kill obuf below.
-	(setq buffer-file-name ofile)
-	(setq buffer-file-number onum)
-	(setq buffer-file-truename otrue)
-	(setq dired-directory odir)
-	(lock-buffer)
-	(rename-buffer oname)))
+      (progn
+        ;; There's some state that we want to restore in obuf before
+        ;; we kill obuf, whether find-file succeeded or not.  For
+        ;; example, we restore buffer-file-name so that certain hooks
+        ;; (e.g., 'save-place-to-alist in 'kill-buffer-hook) can
+        ;; behave as expected.
+        (save-excursion
+          (set-buffer obuf)
+          (setq buffer-file-name ofile)
+          (setq buffer-file-number onum)
+          (setq buffer-file-truename otrue)
+          (setq dired-directory odir))
+	;; On the other hand, if find-file got an error and did not
+        ;; really find anything, we want to put everything back as it
+        ;; was, including the lock and the buffer name.
+        (when (eq obuf (current-buffer))
+          (lock-buffer)
+          (rename-buffer oname))))
     (unless (eq (current-buffer) obuf)
       (with-current-buffer obuf
 	;; We already asked; don't ask again.

bug#4061: 23.1.50; C-x C-v and saveplace

[Leo]

On 2009-09-04 22:39 +0100, Karl Fogel wrote:
> I know what causes this now.  saveplace.el works like this:
> Now, `save-place-to-alist' checks `buffer-file-name' and (properly) does
> nothing if there is no associated file.  Since `find-alternate-file'
> unsets `buffer-file-name' after renaming the old buffer but before
> killing it, that effectively makes `save-place-to-alist' a no-op in the
> old buffer.

Thank you for looking into it.

>
> It's not even clear what the most desirable behavior is.  For example,
> in `find-alternate-file' (without my patch), if the old buffer is
> modified, should we still save place before killing it?  I think so; or
> rather, I think we should do whatever saveplace.el does if one kills a
> modified buffer the normal way.

I agree with this.

-- 
Leo's Emacs uptime: 2 days, 13 hours, 17 minutes, 9 seconds

bug#4061: marked as done (23.1.50; C-x C-v and saveplace)

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

Your message dated Sat, 05 Sep 2009 12:39:02 -0400
with message-id <873a7173dl.fsf@red-bean.com>
and subject line Re: bug#4061: 23.1.50; C-x C-v and saveplace
has caused the Emacs bug report #4061,
regarding 23.1.50; C-x C-v and saveplace
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
4061: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4061
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 2751 bytes --]

From: Leo <sdl.web@gmail.com>
To: emacs-pretest-bug@gnu.org
Subject: 23.1.50; C-x C-v and saveplace
Date: Thu, 06 Aug 2009 04:29:21 +0100
Message-ID: <m0zlady60e.fsf@cam.ac.uk>

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

With saveplace enabled, it will be nice if place (point) can survive C-x
C-v. At the moment, C-x C-v will move the point to a 'random' place.

Leo


[-- Attachment #3: Type: message/rfc822, Size: 2898 bytes --]

From: Karl Fogel <kfogel@red-bean.com>
To: 4061-done@emacsbugs.donarmstrong.com
Cc: Leo <sdl.web@gmail.com>
Subject: Re: bug#4061: 23.1.50; C-x C-v and saveplace
Date: Sat, 05 Sep 2009 12:39:02 -0400
Message-ID: <873a7173dl.fsf@red-bean.com>

Okay, this is fixed now:

  $ cat log-message.txt
  * lisp/files.el (find-alternate-file): Run `kill-buffer-hook' manually
    before killing the old buffer, since by the time `kill-buffer' is
    run so many buffer variables have been set to nil that it may not
    behave as expected.  (Bug#4061)
  $ cvs ci -F log-message.txt files.el ChangeLog
  /sources/emacs/emacs/lisp/files.el,v  <--  files.el
  new revision: 1.1078; previous revision: 1.1077
  /sources/emacs/emacs/lisp/ChangeLog,v  <--  ChangeLog
  new revision: 1.16100; previous revision: 1.16099
  Mailing notification to emacs-diffs@gnu.org... sent.
  $ 

Leo, please test if you get a chance and let us know if you run into any
problems.

Notes on closing:

I'm closing this report now by sending this mail to 4061-done@, as
documented on http://emacsbugs.donarmstrong.com/Developer#closing.  I'm
not positive that's the Right Way to do it, but since much of the
documentation is still written in terms of Debian GNU/Linux instead of
Emacs, and http://emacsbugs.donarmstrong.com/server-control#fixed
doesn't clarify either (for example, what should the "version" be?  The
version the bug was reported against, or the version the fix is likely
to be be released in?), this is as good a guess as any.  If someone
knows the Right Way, please tell me, or better yet, document it where
developers are likely to find it.

-Karl

bug#4061: marked as done (23.1.50; C-x C-v and saveplace)

[Leo]

On 2009-09-05 17:45 +0100, Emacs bug Tracking System wrote:
> Leo, please test if you get a chance and let us know if you run into
> any problems.

I downloaded the latest files.el and loaded with Emacs -q. After a brief
testing, it seems the bug has indeed been fixed. Thank you very much.

Best wishes,
Leo

-- 
Leo's Emacs uptime: 3 days, 5 hours, 56 minutes, 57 seconds