bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer

Antoine Levitt-2
Tested on git master, the bug is not present on 25.

Recipe from emacs -Q:

(setq dired-auto-revert-buffer t)

open dired, open a folder, C-x b back to the previous dired buffer, open
the same folder again, see point jump back to the beginning of buffer.

I'm not sure why, since dired-auto-revert-buffer just seems to call
revert-buffer, which does not jump point.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer

Stephen Berman
On Sun, 04 Jun 2017 16:45:05 -0700 Antoine Levitt <[hidden email]> wrote:

> Tested on git master, the bug is not present on 25.
>
> Recipe from emacs -Q:
>
> (setq dired-auto-revert-buffer t)
>
> open dired, open a folder, C-x b back to the previous dired buffer, open
> the same folder again, see point jump back to the beginning of buffer.
>
> I'm not sure why, since dired-auto-revert-buffer just seems to call
> revert-buffer, which does not jump point.
I think I see why this happens.  I assume when you opened "the same
folder again" you pressed RET on the line in the Dired listing.  That
calls dired-find-file, which calls find-file, which first calls
find-file-noselect (to see if it's a live buffer), which calls (via
run-hook-with-args-until-success) dired-noselect, which calls (via
dired-internal-noselect) revert-buffer (since dired-auto-revert-buffer
is t), which calls dired-revert, which saves point (via
dired-save-positions) but then erases the buffer and inserts it anew --
and this is the problem, because now continuing in find-file, it calls
switch-to-buffer (since it's a live buffer that you're revisiting),
which sets window-point by consulting window-prev-buffers.  Prior to the
invocation of erase-buffer, window-prev-buffers contained this entry for
the buffer being reverted ("test" in my test case):

(#<buffer test> #<marker at 1 in test> #<marker at 232 in test>)

which shows window-point on the first file name in the Dired listing,
but after erase-buffer it's now this:

(#<buffer test> #<marker at 1 in test> #<marker at 1 in test>)

Since dired-revert restored the saved point (232) but does not return it
to the caller, switch-to-buffer does not have this information, but only
what window-prev-buffers shows, which is now 1.  So that's what it sets
window-point to.

One way to fix this is to make dired-restore-positions restore not only
point but also window-point.  The attached patch does this.

Steve Berman


diff --git a/lisp/dired.el b/lisp/dired.el
index 8396652d50..26d3d76817 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1382,9 +1382,13 @@ dired-restore-positions
   (let* ((buf-file-pos (nth 0 positions))
  (buffer (nth 0 buf-file-pos)))
     (unless (and (nth 1 buf-file-pos)
- (dired-goto-file (nth 1 buf-file-pos)))
+ (prog1 (dired-goto-file (nth 1 buf-file-pos))
+   (set-window-buffer nil buffer)
+   (set-window-point (selected-window) (nth 2 buf-file-pos))))
       (goto-char (nth 2 buf-file-pos))
-      (dired-move-to-filename))
+      (dired-move-to-filename)
+      (set-window-buffer nil buffer)
+      (set-window-point (selected-window) (nth 2 buf-file-pos)))
     (dolist (win-file-pos (nth 1 positions))
       ;; Ensure that window still displays the original buffer.
       (when (eq (window-buffer (nth 0 win-file-pos)) buffer)
@@ -1392,7 +1396,8 @@ dired-restore-positions
   (unless (and (nth 1 win-file-pos)
        (dired-goto-file (nth 1 win-file-pos)))
     (goto-char (nth 2 win-file-pos))
-    (dired-move-to-filename)))))))
+    (dired-move-to-filename)
+    (set-window-point nil (point))))))))
 
 (defun dired-remember-marks (beg end)
   "Return alist of files and their marks, from BEG to END."
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer

Eli Zaretskii
> From: Stephen Berman <[hidden email]>
> Date: Mon, 05 Jun 2017 15:37:47 +0200
> Cc: [hidden email]
>
> I think I see why this happens.  I assume when you opened "the same
> folder again" you pressed RET on the line in the Dired listing.  That
> calls dired-find-file, which calls find-file, which first calls
> find-file-noselect (to see if it's a live buffer), which calls (via
> run-hook-with-args-until-success) dired-noselect, which calls (via
> dired-internal-noselect) revert-buffer (since dired-auto-revert-buffer
> is t), which calls dired-revert, which saves point (via
> dired-save-positions) but then erases the buffer and inserts it anew --
> and this is the problem, because now continuing in find-file, it calls
> switch-to-buffer (since it's a live buffer that you're revisiting),
> which sets window-point by consulting window-prev-buffers.  Prior to the
> invocation of erase-buffer, window-prev-buffers contained this entry for
> the buffer being reverted ("test" in my test case):
>
> (#<buffer test> #<marker at 1 in test> #<marker at 232 in test>)
>
> which shows window-point on the first file name in the Dired listing,
> but after erase-buffer it's now this:
>
> (#<buffer test> #<marker at 1 in test> #<marker at 1 in test>)
>
> Since dired-revert restored the saved point (232) but does not return it
> to the caller, switch-to-buffer does not have this information, but only
> what window-prev-buffers shows, which is now 1.  So that's what it sets
> window-point to.

Right, I see the same.

> One way to fix this is to make dired-restore-positions restore not only
> point but also window-point.  The attached patch does this.

An alternative is to bind switch-to-buffer-preserve-window-point to
nil (the whole problem was caused by the fact that this variable is
now non-nil by default, and dired-revert runs afoul of it, as you
point out):

--- lisp/dired.el~0 2017-02-28 06:27:41.000000000 +0200
+++ lisp/dired.el 2017-06-05 17:51:50.633645200 +0300
@@ -2126,7 +2126,11 @@
   (interactive)
   ;; Bind `find-file-run-dired' so that the command works on directories
   ;; too, independent of the user's setting.
-  (let ((find-file-run-dired t))
+  (let ((find-file-run-dired t)
+        (switch-to-buffer-preserve-window-point
+         (if dired-auto-revert-buffer
+             nil
+           switch-to-buffer-preserve-window-point)))
     (find-file (dired-get-file-for-visit))))
 
 (defun dired-find-alternate-file ()


I'm not sure which solution is better.  Maybe someone will come up
with a more elegant one.

Thanks.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer

Stephen Berman
On Mon, 05 Jun 2017 18:02:14 +0300 Eli Zaretskii <[hidden email]> wrote:

>> One way to fix this is to make dired-restore-positions restore not only
>> point but also window-point.  The attached patch does this.
>
> An alternative is to bind switch-to-buffer-preserve-window-point to
> nil (the whole problem was caused by the fact that this variable is
> now non-nil by default, and dired-revert runs afoul of it, as you
> point out):

Oh, I didn't even know about switch-to-buffer-preserve-window-point.

> I'm not sure which solution is better.  Maybe someone will come up
> with a more elegant one.

Your fix is certainly simpler and I think cleaner than mine, so I would
say install it.

Steve Berman



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer

Eli Zaretskii
> From: Stephen Berman <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Mon, 05 Jun 2017 17:15:54 +0200
>
> > An alternative is to bind switch-to-buffer-preserve-window-point to
> > nil (the whole problem was caused by the fact that this variable is
> > now non-nil by default, and dired-revert runs afoul of it, as you
> > point out):
>
> Oh, I didn't even know about switch-to-buffer-preserve-window-point.
>
> > I'm not sure which solution is better.  Maybe someone will come up
> > with a more elegant one.
>
> Your fix is certainly simpler and I think cleaner than mine, so I would
> say install it.

OK, thanks.  I will wait for a couple of days to let others a chance
to chime in, and will install if no further comments come up.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer

Eli Zaretskii
> Date: Mon, 05 Jun 2017 19:20:41 +0300
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email], [hidden email]
>
> > Your fix is certainly simpler and I think cleaner than mine, so I would
> > say install it.
>
> OK, thanks.  I will wait for a couple of days to let others a chance
> to chime in, and will install if no further comments come up.

No further comments, so I pushed the change, and I'm marking this bug
done.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer

Antoine Levitt-2
Thanks for the quick fix! 

Best, 
Antoine 

On 10 Jun 2017 10:25 am, "Eli Zaretskii" <[hidden email]> wrote:
> Date: Mon, 05 Jun 2017 19:20:41 +0300
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email], [hidden email]
>
> > Your fix is certainly simpler and I think cleaner than mine, so I would
> > say install it.
>
> OK, thanks.  I will wait for a couple of days to let others a chance
> to chime in, and will install if no further comments come up.

No further comments, so I pushed the change, and I'm marking this bug
done.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

Antoine Levitt-2
In reply to this post by Antoine Levitt-2
I just noticed this is not yet completely fixed: starting from emacs -Q,
(setq dired-auto-revert-buffer t), open dired, open any file in that
directory, C-x d RET to run dired again, the point jumps back to the
beginning of the buffer.

Best,
Antoine



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

Stephen Berman
On Sat, 17 Jun 2017 14:16:01 +0200 Antoine Levitt <[hidden email]> wrote:

> I just noticed this is not yet completely fixed: starting from emacs -Q,
> (setq dired-auto-revert-buffer t), open dired, open any file in that
> directory, C-x d RET to run dired again, the point jumps back to the
> beginning of the buffer.

I suppose the command `dired' should not use switch-to-buffer.  With the
following patch, executing the above recipe does not move point.


diff --git a/lisp/dired.el b/lisp/dired.el
index 8396652d50..aa59f01af9 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -786,7 +786,8 @@ dired
 If DIRNAME is already in a Dired buffer, that buffer is used without refresh."
   ;; Cannot use (interactive "D") because of wildcards.
   (interactive (dired-read-dir-and-switches ""))
-  (switch-to-buffer (dired-noselect dirname switches)))
+  (set-window-buffer (selected-window)
+                     (set-buffer (dired-noselect dirname switches))))
 
 ;;;###autoload (define-key ctl-x-4-map "d" 'dired-other-window)
 ;;;###autoload


Steve Berman
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

Antoine Levitt-2
Works for me, feel free to merge!

Best,
Antoine

17 June 2017 15:32 +02, Stephen Berman <[hidden email]>:

> On Sat, 17 Jun 2017 14:16:01 +0200 Antoine Levitt <[hidden email]> wrote:
>
>> I just noticed this is not yet completely fixed: starting from emacs -Q,
>> (setq dired-auto-revert-buffer t), open dired, open any file in that
>> directory, C-x d RET to run dired again, the point jumps back to the
>> beginning of the buffer.
>
> I suppose the command `dired' should not use switch-to-buffer.  With the
> following patch, executing the above recipe does not move point.
>
> diff --git a/lisp/dired.el b/lisp/dired.el
> index 8396652d50..aa59f01af9 100644
> --- a/lisp/dired.el
> +++ b/lisp/dired.el
> @@ -786,7 +786,8 @@ dired
>  If DIRNAME is already in a Dired buffer, that buffer is used without refresh."
>    ;; Cannot use (interactive "D") because of wildcards.
>    (interactive (dired-read-dir-and-switches ""))
> -  (switch-to-buffer (dired-noselect dirname switches)))
> +  (set-window-buffer (selected-window)
> +                     (set-buffer (dired-noselect dirname switches))))
>  
>  ;;;###autoload (define-key ctl-x-4-map "d" 'dired-other-window)
>  ;;;###autoload
>
> Steve Berman




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

Stephen Berman
On Sun, 09 Jul 2017 19:37:53 +0200 Antoine Levitt <[hidden email]> wrote:

> Works for me, feel free to merge!
>
> Best,
> Antoine

Thanks for confirming.  I think this is the right fix and would commit
it, but since this exchange has taken place in a closed bug, it may have
fallen under the radar, so I'd like an explicit go-ahead.  Eli, John?

Steve Berman

> 17 June 2017 15:32 +02, Stephen Berman <[hidden email]>:
>> On Sat, 17 Jun 2017 14:16:01 +0200 Antoine Levitt <[hidden email]> wrote:
>>
>>> I just noticed this is not yet completely fixed: starting from emacs -Q,
>>> (setq dired-auto-revert-buffer t), open dired, open any file in that
>>> directory, C-x d RET to run dired again, the point jumps back to the
>>> beginning of the buffer.
>>
>> I suppose the command `dired' should not use switch-to-buffer.  With the
>> following patch, executing the above recipe does not move point.
>>
>> diff --git a/lisp/dired.el b/lisp/dired.el
>> index 8396652d50..aa59f01af9 100644
>> --- a/lisp/dired.el
>> +++ b/lisp/dired.el
>> @@ -786,7 +786,8 @@ dired
>>  If DIRNAME is already in a Dired buffer, that buffer is used without refresh."
>>    ;; Cannot use (interactive "D") because of wildcards.
>>    (interactive (dired-read-dir-and-switches ""))
>> -  (switch-to-buffer (dired-noselect dirname switches)))
>> +  (set-window-buffer (selected-window)
>> +                     (set-buffer (dired-noselect dirname switches))))
>>  
>>  ;;;###autoload (define-key ctl-x-4-map "d" 'dired-other-window)
>>  ;;;###autoload
>>
>> Steve Berman



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

John Wiegley
>>>>> Stephen Berman <[hidden email]> writes:

> Thanks for confirming. I think this is the right fix and would commit it,
> but since this exchange has taken place in a closed bug, it may have fallen
> under the radar, so I'd like an explicit go-ahead. Eli, John?

At first glance it looks OK to me, though I defer to Eli's experience in such
matters.

--
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

Eli Zaretskii
> From: John Wiegley <[hidden email]>
> Cc: Antoine Levitt <[hidden email]>,  [hidden email],  Eli Zaretskii <[hidden email]>
> Date: Fri, 14 Jul 2017 22:52:18 -0700
>
> >>>>> Stephen Berman <[hidden email]> writes:
>
> > Thanks for confirming. I think this is the right fix and would commit it,
> > but since this exchange has taken place in a closed bug, it may have fallen
> > under the radar, so I'd like an explicit go-ahead. Eli, John?
>
> At first glance it looks OK to me, though I defer to Eli's experience in such
> matters.

No objections here.

Thanks.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

martin rudalics
In reply to this post by Stephen Berman
 > +  (set-window-buffer (selected-window)
 > +                     (set-buffer (dired-noselect dirname switches))))

This really should be

      (pop-to-buffer-same-window (dired-noselect dirname switches))

Even if people disliked it in the past and some still dislike it: Try

C-h f
M-x dired

martin



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

Stephen Berman
On Fri, 14 Jul 2017 22:52:18 -0700 John Wiegley <[hidden email]> wrote:

>>>>>> Stephen Berman <[hidden email]> writes:
>
>> Thanks for confirming. I think this is the right fix and would commit it,
>> but since this exchange has taken place in a closed bug, it may have fallen
>> under the radar, so I'd like an explicit go-ahead. Eli, John?
>
> At first glance it looks OK to me, though I defer to Eli's experience in such
> matters.

On Sat, 15 Jul 2017 10:18:06 +0300 Eli Zaretskii <[hidden email]> wrote:

> No objections here.
>
> Thanks.

Thanks for okaying, but...

On Sat, 15 Jul 2017 09:36:41 +0200 martin rudalics <[hidden email]> wrote:

>> +  (set-window-buffer (selected-window)
>> +                     (set-buffer (dired-noselect dirname switches))))
>
> This really should be
>
>      (pop-to-buffer-same-window (dired-noselect dirname switches))
>
> Even if people disliked it in the past and some still dislike it: Try
>
> C-h f
> M-x dired

Wow, that's disastrous!  And frightening: I use the "(set-window-buffer
(selected-window) (set-buffer ...))" idiom a lot in todo-mode.el; I just
checked two commands using `C-h f' followed by the command invocation:
one worked fine but the other caused the same disaster.  So now I have
to check all uses :(.  Is there a general guideline for when to use
set-window-buffer and when to use pop-to-buffer-same-window?

Anyway, I'll commit the dired.el change you recommend; many thanks.

Steve Berman



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

martin rudalics
 > Wow, that's disastrous!  And frightening: I use the "(set-window-buffer
 > (selected-window) (set-buffer ...))" idiom a lot in todo-mode.el; I just
 > checked two commands using `C-h f' followed by the command invocation:
 > one worked fine but the other caused the same disaster.  So now I have
 > to check all uses :(.  Is there a general guideline for when to use
 > set-window-buffer and when to use pop-to-buffer-same-window?

Always try ‘pop-to-buffer-same-window’ or ‘display-buffer-same-window’
first.  If they fail, try to find out why.  Maybe that can be fixed.

martin




Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

Stephen Berman
In reply to this post by Antoine Levitt-2
On Sun, 09 Jul 2017 19:37:53 +0200 Antoine Levitt <[hidden email]> wrote:

> Works for me, feel free to merge!
>
> Best,
> Antoine
>
> 17 June 2017 15:32 +02, Stephen Berman <[hidden email]>:
>> On Sat, 17 Jun 2017 14:16:01 +0200 Antoine Levitt <[hidden email]> wrote:
>>
>>> I just noticed this is not yet completely fixed: starting from emacs -Q,
>>> (setq dired-auto-revert-buffer t), open dired, open any file in that
>>> directory, C-x d RET to run dired again, the point jumps back to the
>>> beginning of the buffer.
>>
>> I suppose the command `dired' should not use switch-to-buffer.  With the
>> following patch, executing the above recipe does not move point.
>>
>> diff --git a/lisp/dired.el b/lisp/dired.el
>> index 8396652d50..aa59f01af9 100644
>> --- a/lisp/dired.el
>> +++ b/lisp/dired.el
>> @@ -786,7 +786,8 @@ dired
>>  If DIRNAME is already in a Dired buffer, that buffer is used without refresh."
>>    ;; Cannot use (interactive "D") because of wildcards.
>>    (interactive (dired-read-dir-and-switches ""))
>> -  (switch-to-buffer (dired-noselect dirname switches)))
>> +  (set-window-buffer (selected-window)
>> +                     (set-buffer (dired-noselect dirname switches))))
>>  
>>  ;;;###autoload (define-key ctl-x-4-map "d" 'dired-other-window)
>>  ;;;###autoload
>>
>> Steve Berman

On Sat, 15 Jul 2017 09:36:41 +0200 martin rudalics <[hidden email]> wrote:

>> +  (set-window-buffer (selected-window)
>> +                     (set-buffer (dired-noselect dirname switches))))
>
> This really should be
>
>      (pop-to-buffer-same-window (dired-noselect dirname switches))
>
> Even if people disliked it in the past and some still dislike it: Try
>
> C-h f
> M-x dired

I installed the fix Martin recommended to master as commit b2150e0
(after confirming that it indeed DTRT).  I also added a test for this
case and the first one.

Steve Berman



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#27243: closed (Re: bug#27243: dired-auto-revert-buffer jumps point to beginning of buffer)

Stephen Berman
In reply to this post by martin rudalics
On Sat, 15 Jul 2017 15:59:07 +0200 martin rudalics <[hidden email]> wrote:

>> Wow, that's disastrous!  And frightening: I use the "(set-window-buffer
>> (selected-window) (set-buffer ...))" idiom a lot in todo-mode.el; I just
>> checked two commands using `C-h f' followed by the command invocation:
>> one worked fine but the other caused the same disaster.  So now I have
>> to check all uses :(.  Is there a general guideline for when to use
>> set-window-buffer and when to use pop-to-buffer-same-window?
>
> Always try ‘pop-to-buffer-same-window’ or ‘display-buffer-same-window’
> first.  If they fail, try to find out why.  Maybe that can be fixed.

Thanks for the advice.

Steve Berman



Loading...