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