bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Basil L. Contovounesios
Severity: wishlist

Currently, the user option dired-dwim-target applies only to windows
displaying Dired buffers on the selected frame.  IWBNI this were
customisable, so that Dired buffers displayed on other frames were also
considered as default targets for file operations.  This would be useful
for users like me who enable pop-up-frames.

Patch implementing this to follow.

Thanks,

--
Basil



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Basil L. Contovounesios
tags 35385 patch
quit



"Basil L. Contovounesios" <[hidden email]> writes:

> Patch implementing this to follow.

Now attached.  WDYT?

Thanks,

--
Basil

0001-Make-dired-dwim-target-aware-of-other-frames.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
> Now attached.  WDYT?

>  @vindex dired-dwim-target
> +@vindex dired-dwim-target-frames

Should we also have dired-dwim-target-windows?
Because I have a problem when there are 3 or more
Dired windows on the same frame.  Often dired-dwim
chooses a wrong window.  Maybe it should use something
like implemented in compare-windows-get-recent-window:

  (or (get-mru-window 'visible t t)
      (get-mru-window 0 t t)
      (get-mru-window t t t))



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Lars Ingebrigtsen
In reply to this post by Basil L. Contovounesios
"Basil L. Contovounesios" <[hidden email]> writes:

> Currently, the user option dired-dwim-target applies only to windows
> displaying Dired buffers on the selected frame.  IWBNI this were
> customisable, so that Dired buffers displayed on other frames were also
> considered as default targets for file operations.  This would be useful
> for users like me who enable pop-up-frames.
>
> Patch implementing this to follow.

This makes sense to me, and the code in the patch looked OK.  But what
did you think of Juri's comments about dired-dwim-target-windows?

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Lars Ingebrigtsen
Lars Ingebrigtsen <[hidden email]> writes:

> "Basil L. Contovounesios" <[hidden email]> writes:
>
>> Currently, the user option dired-dwim-target applies only to windows
>> displaying Dired buffers on the selected frame.  IWBNI this were
>> customisable, so that Dired buffers displayed on other frames were also
>> considered as default targets for file operations.  This would be useful
>> for users like me who enable pop-up-frames.
>>
>> Patch implementing this to follow.
>
> This makes sense to me, and the code in the patch looked OK.  But what
> did you think of Juri's comments about dired-dwim-target-windows?

That was 12 weeks ago.  Basil, have you had an opportunity to look at
this?

--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
>>> Currently, the user option dired-dwim-target applies only to windows
>>> displaying Dired buffers on the selected frame.  IWBNI this were
>>> customisable, so that Dired buffers displayed on other frames were also
>>> considered as default targets for file operations.  This would be useful
>>> for users like me who enable pop-up-frames.
>>>
>>> Patch implementing this to follow.
>>
>> This makes sense to me, and the code in the patch looked OK.  But what
>> did you think of Juri's comments about dired-dwim-target-windows?
>
> That was 12 weeks ago.  Basil, have you had an opportunity to look at
> this?

I'd like to elaborate on my comments: the thought was that instead of
extending the scope of the search for the first random Dired window
from the selected frame to all frames, would it be better to improve the
heuristic of finding the window that the user really meant to use
(remember that the user option name contains the word "DWIM").

The proposed heuristic was to use `get-mru-window' to get
the most recently used window from all frames, and even
better way is to traverse all windows ordered by their visiting
recency on all frames to find the window with Dired mode buffer.



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
> I'd like to elaborate on my comments: the thought was that instead of
> extending the scope of the search for the first random Dired window
> from the selected frame to all frames, would it be better to improve the
> heuristic of finding the window that the user really meant to use
> (remember that the user option name contains the word "DWIM").
>
> The proposed heuristic was to use `get-mru-window' to get
> the most recently used window from all frames, and even
> better way is to traverse all windows ordered by their visiting
> recency on all frames to find the window with Dired mode buffer.

I can't find an existing function that would sort windows by recency,
but fortunately the implementation is straightforward:

  (sort (window-list-1)
        (lambda (a b)
          (> (window-use-time a)
             (window-use-time b))))

using `>' gives the mru order, `<' - lru order.

BTW, while looking at windows walking functions, I noticed
an opportunity for simplification.

Martin, could you please confirm if I'm not mistaken with this patch:


diff --git a/lisp/window.el b/lisp/window.el
index cf733153b8..aedebd9d46 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -2217,6 +2217,10 @@ walk-windows
 
 - A frame means consider all windows on that frame only.
 
+If ALL-FRAMES specifies a frame, the first window walked is the
+first window on that frame (the one returned by `frame-first-window'),
+not necessarily the selected window.
+
 Anything else means consider all windows on the selected frame
 and no others.
 
@@ -2226,14 +2230,11 @@ walk-windows
   ;; back to it.
   (when (window-minibuffer-p)
     (setq minibuf t))
-  ;; Make sure to not mess up the order of recently selected
-  ;; windows.  Use `save-selected-window' and `select-window'
-  ;; with second argument non-nil for this purpose.
-  (save-selected-window
-    (when (framep all-frames)
-      (select-window (frame-first-window all-frames) 'norecord))
-    (dolist (walk-windows-window (window-list-1 nil minibuf all-frames))
-      (funcall fun walk-windows-window))))
+  (dolist (walk-windows-window
+           (window-list-1 (and (framep all-frames)
+                               (frame-first-window all-frames))
+                          minibuf all-frames))
+      (funcall fun walk-windows-window)))
 
 (defun window-at-side-p (&optional window side)
   "Return t if WINDOW is at SIDE of its containing frame.
Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Basil L. Contovounesios
In reply to this post by Lars Ingebrigtsen
Lars Ingebrigtsen <[hidden email]> writes:

> Lars Ingebrigtsen <[hidden email]> writes:
>
>> This makes sense to me, and the code in the patch looked OK.  But what
>> did you think of Juri's comments about dired-dwim-target-windows?
>
> That was 12 weeks ago.  Basil, have you had an opportunity to look at
> this?

Sorry, I haven't had a chance to catch up with this stuff since my
summer holidays.  I'll try to have a look within the next week if no-one
beats me to it.

Thanks,

--
Basil



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

martin rudalics
In reply to this post by Juri Linkov-2
 > BTW, while looking at windows walking functions, I noticed
 > an opportunity for simplification.
 >
 > Martin, could you please confirm if I'm not mistaken with this patch:

Removing the

     (when (framep all-frames)
       (select-window (frame-first-window all-frames) 'norecord))

LGTM.  Whether we should remove the

   (save-selected-window

envelope is another question.  If FUN changes the selected window,
we'd now have a side effect we didn't have so far.

Personally, I would always try to use 'walk-window-tree' instead of
'walk-windows' reserving the latter for operations that are allowed to
create or delete windows.

martin



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
>> BTW, while looking at windows walking functions, I noticed

>> an opportunity for simplification.
>>
>> Martin, could you please confirm if I'm not mistaken with this patch:
>
> Removing the
>
>     (when (framep all-frames)
>       (select-window (frame-first-window all-frames) 'norecord))
>
> LGTM.  Whether we should remove the
>
>   (save-selected-window
>
> envelope is another question.  If FUN changes the selected window,
> we'd now have a side effect we didn't have so far.
So I left a comment explaining why save-selected-window is still needed.

Now I prepared a patch for dired-dwim-target to DWIM most recently used
windows from all frames:


diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index a321247b0b..7c477fa89d 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -1965,6 +1968,18 @@ dired-mark-read-file-name
    #'read-file-name
    (format prompt (dired-mark-prompt arg files)) dir default))
 
+(defun dired-dwim-target-directories ()
+  ;; Return directories from all visible windows with dired-mode buffers
+  ;; ordered by most-recently-used.
+  (mapcar #'cdr (sort (mapcan (lambda (w)
+                                (with-current-buffer (window-buffer w)
+                                  (when (eq major-mode 'dired-mode)
+                                    (list (cons (window-use-time w)
+                                                (dired-current-directory))))))
+                              (delq (selected-window)
+                                    (window-list-1 nil 'nomini 'visible)))
+                      (lambda (a b) (> (car a) (car b))))))
+
 (defun dired-dwim-target-directory ()
   ;; Try to guess which target directory the user may want.
   ;; If there is a dired buffer displayed in one of the next windows,
@@ -1973,15 +1988,7 @@ dired-dwim-target-directory
        (dired-current-directory))))
     ;; non-dired buffer may want to profit from this function, e.g. vm-uudecode
     (if dired-dwim-target
- (let* ((other-win (get-window-with-predicate
-   (lambda (window)
-     (with-current-buffer (window-buffer window)
-       (eq major-mode 'dired-mode)))))
-       (other-dir (and other-win
-       (with-current-buffer (window-buffer other-win)
- (and (eq major-mode 'dired-mode)
-      (dired-current-directory))))))
-  (or other-dir this-dir))
+        (or (car (dired-dwim-target-directories)) this-dir)
       this-dir)))
 
 (defun dired-dwim-target-defaults (fn-list target-dir)
@@ -1999,15 +2006,11 @@ dired-dwim-target-defaults
  (and (consp fn-list) (null (cdr fn-list)) (car fn-list)))
  (current-dir (and (eq major-mode 'dired-mode)
   (dired-current-directory)))
- dired-dirs)
-    ;; Get a list of directories of visible buffers in dired-mode.
-    (walk-windows (lambda (w)
-    (with-current-buffer (window-buffer w)
-      (and (eq major-mode 'dired-mode)
-   (push (dired-current-directory) dired-dirs)))))
+        ;; Get a list of directories of visible buffers in dired-mode.
+        (dired-dirs (dired-dwim-target-directories)))
     ;; Force the current dir to be the first in the list.
     (setq dired-dirs
-  (delete-dups (delq nil (cons current-dir (nreverse dired-dirs)))))
+          (delete-dups (delq nil (cons current-dir dired-dirs))))
     ;; Remove the target dir (if specified) or the current dir from
     ;; default values, because it should be already in initial input.
     (setq dired-dirs (delete (or target-dir current-dir) dired-dirs))
diff --git a/lisp/dired.el b/lisp/dired.el
index 854bc9f7d7..b0d40da57f 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -185,9 +185,9 @@ dired-keep-marker-symlink
 
 (defcustom dired-dwim-target nil
   "If non-nil, Dired tries to guess a default target directory.
-This means: if there is a Dired buffer displayed in the next
-window, use its current directory, instead of this Dired buffer's
-current directory.
+This means: if there is a Dired buffer displayed in one of recently
+selected windows, use its current directory, instead of this Dired
+buffer's current directory.
 
 The target is used in the prompt for file copy, rename etc."
   :type 'boolean
Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
> Now I prepared a patch for dired-dwim-target to DWIM most recently used
> windows from all frames:

Pushed to master.  Can this be closed now?



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Michael Heerdegen
Juri Linkov <[hidden email]> writes:

> > Now I prepared a patch for dired-dwim-target to DWIM most recently
> > used windows from all frames:
>
> Pushed to master.  Can this be closed now?

I wished this could be made configurable somehow.  FWIW I liked the
former behavior more so I :override advice
`dired-dwim-target-directories'.  It would be good if the used function
would at least be bound to a new defvar.  A defcustom with some
reasonable choices would probably be even better (old behavior + new
behavior would already be a good start).

Michael.



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
> I wished this could be made configurable somehow.  FWIW I liked the
> former behavior more so I :override advice
> `dired-dwim-target-directories'.  It would be good if the used function
> would at least be bound to a new defvar.  A defcustom with some
> reasonable choices would probably be even better (old behavior + new
> behavior would already be a good start).

The idea to use the recent buffer was borrowed from compare-windows
that for backward-compatibility has such defcustom:

  (defcustom compare-windows-get-window-function
    'compare-windows-get-recent-window
    "Function that provides the window to compare with."
    :type '(choice
            (function-item :tag "Most recently used window"
                           compare-windows-get-recent-window)
            (function-item :tag "Next window"
                           compare-windows-get-next-window)
            (function :tag "Your function"))
    :group 'compare-windows
    :version "25.1")

So dired-dwim-target could have a similar defcustom
with choices for recent/next window.



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Michael Heerdegen
Juri Linkov <[hidden email]> writes:

> So dired-dwim-target could have a similar defcustom
> with choices for recent/next window.

There are lots of imaginable user preferences, so I would prefer a
solution where one can specify a function.  Would you want to use
`dired-dwim-target' for that?


Michael.



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
>> So dired-dwim-target could have a similar defcustom
>> with choices for recent/next window.
>
> There are lots of imaginable user preferences, so I would prefer a
> solution where one can specify a function.  Would you want to use
> `dired-dwim-target' for that?

This would be a natural choice, indeed.

Please try this patch:


diff --git a/lisp/dired.el b/lisp/dired.el
index 05789a3516..cfee990a74 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -190,8 +190,16 @@ dired-dwim-target
 Dired buffer's current directory.
 
 The target is used in the prompt for file copy, rename etc."
-  :type 'boolean
-  :group 'dired)
+  :type '(choice
+          (const :tag "No guess" nil)
+          (function-item :tag "Prefer most recently used windows"
+                         dired-dwim-target-recent)
+          (function-item :tag "Prefer next windows"
+                         dired-dwim-target-next)
+          (function :tag "Your function")
+          (other :tag "Try to guess" t))
+  :group 'dired
+  :version "27.1")
 
 (defcustom dired-copy-preserve-time t
   "If non-nil, Dired preserves the last-modified time in a file copy.
diff --git a/lisp/dired-aux.el b/lisp/dired-aux.el
index 722d036e3f..a3d0ad61dd 100644
--- a/lisp/dired-aux.el
+++ b/lisp/dired-aux.el
@@ -1993,6 +1993,12 @@ dired-mark-read-file-name
    (format prompt (dired-mark-prompt arg files)) dir default))
 
 (defun dired-dwim-target-directories ()
+  (cond ((functionp dired-dwim-target)
+         (funcall dired-dwim-target))
+        (dired-dwim-target
+         (dired-dwim-target-recent))))
+
+(defun dired-dwim-target-recent ()
   ;; Return directories from all visible windows with dired-mode buffers
   ;; ordered by most-recently-used.
   (mapcar #'cdr (sort (mapcan (lambda (w)
@@ -2004,6 +2010,15 @@ dired-dwim-target-directories
                                     (window-list-1 nil 'nomini 'visible)))
                       (lambda (a b) (> (car a) (car b))))))
 
+(defun dired-dwim-target-next ()
+  (mapcan (lambda (w)
+            (with-current-buffer (window-buffer w)
+              (when (eq major-mode 'dired-mode)
+                (list (dired-current-directory)))))
+          (delq (selected-window) (window-list-1
+                                   (next-window nil 'nomini 'visible)
+                                   'nomini 'visible))))
+
 (defun dired-dwim-target-directory ()
   ;; Try to guess which target directory the user may want.
   ;; If there is a dired buffer displayed in one of the next windows,
Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
>>> So dired-dwim-target could have a similar defcustom
>>> with choices for recent/next window.
>>
>> There are lots of imaginable user preferences, so I would prefer a
>> solution where one can specify a function.  Would you want to use
>> `dired-dwim-target' for that?
>
> This would be a natural choice, indeed.

Now pushed to master.  Please close if everything is alright.



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Michael Heerdegen
Juri Linkov <[hidden email]> writes:

> Now pushed to master.  Please close if everything is alright.

Thanks.  Works well for me.  But I'm not yet happy with the docstring of
the option `dired-dwim-target'.

I think we should add that the non-nil value can be a function, and that
in this case that function will be called with zero arguments and is
expected to return a list of directories which will be used as defaults
(i.e. default target and "future history") (though,
`dired-dwim-target-defaults' might modify it a bit).

The sentence "You can customize it to prefer either the next window with
a Dired buffer, or the most recently used window with a Dired buffer."
could get appended "or something else".

Michael.



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Juri Linkov-2
> I think we should add that the non-nil value can be a function, and that
> in this case that function will be called with zero arguments and is
> expected to return a list of directories which will be used as defaults
> (i.e. default target and "future history") (though,
> `dired-dwim-target-defaults' might modify it a bit).
>
> The sentence "You can customize it to prefer either the next window with
> a Dired buffer, or the most recently used window with a Dired buffer."
> could get appended "or something else".

Do you think this is good enough?

diff --git a/doc/emacs/dired.texi b/doc/emacs/dired.texi
index 8fab508dea..d1863510d4 100644
--- a/doc/emacs/dired.texi
+++ b/doc/emacs/dired.texi
@@ -659,7 +659,10 @@ Operating on Files
 some window, that other buffer's directory is suggested instead.
 You can customize @code{dired-dwim-target} to prefer either the next
 window with a Dired buffer, or the most recently used window with
-a Dired buffer.
+a Dired buffer, or to use any other function.  When the value is
+a function, it will be called with no arguments and is expected to
+return a list of directories which will be used as defaults
+(i.e. default target and ``future history'').
 
   Here are the file-manipulating Dired commands that operate on files.
 
diff --git a/lisp/dired.el b/lisp/dired.el
index 009018fafe..15286cc9b0 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -190,7 +190,11 @@ dired-dwim-target
 current directory.
 
 You can customize it to prefer either the next window with a Dired buffer,
-or the most recently used window with a Dired buffer.
+or the most recently used window with a Dired buffer, or to use any other
+function.  When the value is a function, it will be called with no
+arguments and is expected to return a list of directories which will
+be used as defaults (i.e. default target and \"future history\")
+(though, `dired-dwim-target-defaults' might modify it a bit).
 
 The target is used in the prompt for file copy, rename etc."
   :type '(choice



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Date: Sun, 10 Nov 2019 22:38:38 +0200
> Cc: "Basil L. Contovounesios" <[hidden email]>,
>  Lars Ingebrigtsen <[hidden email]>, [hidden email]
>
> +          (function-item :tag "Prefer next windows"
> +                         dired-dwim-target-next)

This is confusing, IMO.  What does "next windows" allude to?

> +          (function :tag "Your function")

Instead of "Your" I'd say "Custom" here.

Note that dired-dwim-target is described in the user manual, so there
should be a corresponding change in the manual (and in NEWS).

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#35385: 27.0.50; Make dired-dwim-target aware of other frames

Michael Heerdegen
In reply to this post by Juri Linkov-2
Juri Linkov <[hidden email]> writes:

> Do you think this is good enough?

Looks quite good so far.  An open question is what the behavior of a
non-nil and not functionp value is.

BTW, when configuring `dired-dwim-target' to `dired-dwim-target-next'
and when the current dired buffer's window is the only dired window in
the selected frame, then the default target is not the current buffer's
directory but the directory of some other frame's dired.  I don't like
that.  I would want that always all dired windows in the current frame
are considered, including the current one.  I mean, operating on files
in one dired buffer is a quite common need, so defaulting to a directory
of some dired buffer in some other frame can be quite confusing.  I'm
accustomed to the old behavior of course but...it made sense to me.


Michael.



12