bug#42210: Add -other-window variants of project-prefix-map commands

bug#42210: Add -other-window variants of project-prefix-map commands

[Sean Whitton]

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

Hello,

On Fri 03 Jul 2020 at 05:54PM -07, Sean Whitton wrote:

> It seems like it would be a good idea to have
>
> C-x 4 p f
> be like
> C-x 4 4 C-x p f
>
> C-x 5 p e
> be like
> C-x 5 5 C-x p e
>
> etc., since many of the commands in project-prefix-map involve switching
> to another buffer.  Certainly project-switch-project, project-find-file
> and project-switch-to-buffer would be wanted.

Here is a patch implementing commands under C-x 4 p.  If the approach is
thought sound I can also prepare patches for C-x 5 p and C-x t p.

I have tested the attached change except for the autoload cookies which
I am not sure will work with my new macro.  And I'm not sure I should be
doing this with a macro instead of a function which calls fset -- please
advise.

If you want me to do copyright assignment before investing time giving
me feedback on my patch, I would be happy to.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-define-other-window-command-and-project-other-wi.patch --]
[-- Type: text/x-diff, Size: 3182 bytes --]

From bc52db3611612fc595793fd5f2aebd4a7d9bdb59 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Sat, 4 Jul 2020 22:40:36 -0700
Subject: [PATCH] Add define-other-window-command and
 project-other-window-prefix-map

* lisp/progmodes/project.el: Add project-other-window-prefix-map, bind
to C-x 4 p, and use define-other-window-command to define the map's
commands.
* lisp/window.el: Add define-other-window-command.
---
 lisp/progmodes/project.el | 28 ++++++++++++++++++++++++++++
 lisp/window.el            | 17 +++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 0a15939d24..3a0034b24a 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -512,6 +512,19 @@ project-prefix-map
 
 ;;;###autoload (define-key ctl-x-map "p" project-prefix-map)
 
+;;;###autoload
+(defvar project-other-window-prefix-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "f" 'project-find-file-other-window)
+    (define-key map "b" 'project-switch-to-buffer-other-window)
+    (define-key map "d" 'project-dired-other-window)
+    (define-key map "e" 'project-eshell-other-window)
+    (define-key map "p" 'project-switch-project-other-window)
+    map)
+  "Keymap for -other-window project commands.")
+
+;;;###autoload (define-key ctl-x-4-map "p" project-other-window-prefix-map)
+
 (defun project--value-in-dir (var dir)
   (with-temp-buffer
     (setq default-directory dir)
@@ -864,6 +877,21 @@ project-kill-buffers
                                (length bufs) (project-root pr)))
       (mapc #'kill-buffer bufs))))
 
+;;;###autoload
+(define-other-window-command project-find-file)
+
+;;;###autoload
+(define-other-window-command project-switch-to-buffer)
+
+;;;###autoload
+(define-other-window-command project-dired)
+
+;;;###autoload
+(define-other-window-command project-eshell)
+
+;;;###autoload
+(define-other-window-command project-switch-project)
+
 \f
 ;;; Project list
 
diff --git a/lisp/window.el b/lisp/window.el
index 675aff041b..519e15ac79 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -4042,6 +4042,23 @@ same-window-prefix
            'reuse)))
   (message "Display next command buffer in the same window..."))
 
+(defmacro define-other-window-command (function)
+  "Define a version of FUNCTION which displays its buffer in another window.
+
+The new function will be named 'FUNCTION-other-window'."
+  (let ((other-window-function
+	 (intern (concat (symbol-name function) "-other-window"))))
+    `(defun ,other-window-function (&rest args)
+       ,(concat "Like `" (symbol-name function)
+		"' but prefer to display resultant buffer in another window.")
+       ,@(if (commandp function) '((interactive)) '())
+       (let ((display-buffer-overriding-action
+	      '((display-buffer-pop-up-window)
+		(inhibit-same-window . t))))
+	 (if (called-interactively-p)
+	     (call-interactively ',function)
+	   (apply ',function args))))))
+
 ;; This should probably return non-nil when the selected window is part
 ;; of an atomic window whose root is the frame's root window.
 (defun one-window-p (&optional nomini all-frames)
-- 
2.26.2

bug#42210: Add -other-window variants of project-prefix-map commands

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Sat, 04 Jul 2020 23:13:58 -0700
> Cc: dgutov@yandex.ru, juri@linkov.net
> 
> Here is a patch implementing commands under C-x 4 p.  If the approach is
> thought sound I can also prepare patches for C-x 5 p and C-x t p.

Thanks.  Please accompany these changes with suitable changes to NEWS
and the user manual.

> If you want me to do copyright assignment before investing time giving
> me feedback on my patch, I would be happy to.

Form sent off-list.

bug#42210: Add -other-window variants of project-prefix-map commands

[Drew Adams]

1. I disagree with calling the macro `define-other-window-command'.

My disagreement is this: Defining an other-window command
should do just that.  It should not define a command that
only "prefers" to display in another window.  It should
define a command that actually displays in another window.

And your doc string in fact says the latter, though the
behavior is, I guess, only the former.  Please consider
renaming the macro and fixing the doc string, to make clear
that it's NOT other-window but only maybe-other-window.

2. Why "resultant buffer".  What does the buffer result
from?  It's about whatever FUNCTION displays, no?

3. Presumably FUNCTION must be a _command_.  If so,
the arg should be called COMMAND, and the doc adjusted
accordingly.

#2 and #3 are minor.  I'm more concerned about #1.
Emacs has many commands that are _really_ other-window.
They generally use `pop-to-buffer' or
`switch-to-buffer-other-window'.

If this macro produces commands that only "prefer" to
use another window, then the name and doc string are
false advertising.

4. Why not have a macro that _really_ provides an
other-window command?

bug#42210: Add -other-window variants of project-prefix-map commands

[Sean Whitton]

Hello,

On Sun 05 Jul 2020 at 11:35AM -07, Drew Adams wrote:

> 1. I disagree with calling the macro `define-other-window-command'.
>
> My disagreement is this: Defining an other-window command
> should do just that.  It should not define a command that
> only "prefers" to display in another window.  It should
> define a command that actually displays in another window.
>
> And your doc string in fact says the latter, though the
> behavior is, I guess, only the former.  Please consider
> renaming the macro and fixing the doc string, to make clear
> that it's NOT other-window but only maybe-other-window.

Hmm, but doesn't pop-to-buffer-other-window also only prefer to use
another window, and ultimately defers to the display-buffer-alist
machinery?

> 2. Why "resultant buffer".  What does the buffer result
> from?  It's about whatever FUNCTION displays, no?

Please feel free to suggest alternative phrasing that will be short
enough to fit in one line, which I understand to be required.

> 3. Presumably FUNCTION must be a _command_.  If so,
> the arg should be called COMMAND, and the doc adjusted
> accordingly.

No, it can just be a function too.

-- 
Sean Whitton

bug#42210: Add -other-window variants of project-prefix-map commands

[Drew Adams]

> > 1. I disagree with calling the macro `define-other-window-command'.
> >
> > My disagreement is this: Defining an other-window command
> > should do just that.  It should not define a command that
> > only "prefers" to display in another window.  It should
> > define a command that actually displays in another window.
> >
> > And your doc string in fact says the latter, though the
> > behavior is, I guess, only the former.  Please consider
> > renaming the macro and fixing the doc string, to make clear
> > that it's NOT other-window but only maybe-other-window.
> 
> Hmm, but doesn't pop-to-buffer-other-window also only prefer to use
> another window, and ultimately defers to the display-buffer-alist
> machinery?

1. (There is no `pop-to-buffer-other-window', is there?)

The doc of `pop-to-buffer' goes out of its way to tell
about its relation with `display-buffer' - because it
passes its arg to it.  

The doc of `switch-to-buffer-other-window' just says
directly that it selects the buffer in another window.

Only at its very end does it mention the possibility
that `display-buffer' can alter behavior:

  This uses the function `display-buffer' as a subroutine;
  see its documentation for additional customization
  information.

What's not so good is to (a) not say what THIS function
is for: other-window and (b) not mention `display-buffer',
while suggesting some other behavior than other-window.

2. I suppose `display-buffer(-alist)' can override
anything now.

If that's all that's meant then I think it shouldn't
be put that way.  It's OK (but might not be needed or
appropriate) to add some mention of it at the end.
But I don't think the behavior of the command should
be described that way.

The point of the resulting function is (presumably)
to use another window.  The point of any additional
`display-buffer*' shenanigans - or other Lisp code
that might have an effect on things - could be just
about anything.

This doc should be about what the resulting function
intends to do, not whatever some other code might
make it do instead.  If you want to go the other route,
then make the relation explicit, as does the doc of
`pop-to-buffer' and `switch-to-buffer-other-window'.

> > 2. Why "resultant buffer".  What does the buffer result
> > from?  It's about whatever FUNCTION displays, no?
> 
> Please feel free to suggest alternative phrasing that will be short
> enough to fit in one line, which I understand to be required.

 Define an other-window version of FUNCTION.

or

 Define a function like FUNCTION that outputs to another window.

or

 Define a function like FUNCTION but that outputs to another window.

> > 3. Presumably FUNCTION must be a _command_.  If so,
> > the arg should be called COMMAND, and the doc adjusted
> > accordingly.
> 
> No, it can just be a function too.

You're right; I was wrong about that.

However, isn't the real purpose of this macro to
define commands, which you can bind to keys to get
other-window behavior?  Is there really some other
expected use case?  Would anyone use it for a
non-interactive function?

If you want to be exact then yes, FUNCTION.  But
in that case I think the doc should say that
FUNCTION is typically a command, i.e., give the
use case that's the raison d'etre for the macro.

BTW, this part of your patch doesn't seem right:
(called-interactively-p).

Argument KIND is optional (should it really be?),
but the behavior of calling the function without
KIND is not documented, and it always just returns
nil.

(I filed bug #42222 about KIND being optional etc.)

bug#42210: Add -other-window variants of project-prefix-map commands

[Juri Linkov]

> Here is a patch implementing commands under C-x 4 p.  If the approach is
> thought sound I can also prepare patches for C-x 5 p and C-x t p.
>
> I have tested the attached change except for the autoload cookies which
> I am not sure will work with my new macro.  And I'm not sure I should be
> doing this with a macro instead of a function which calls fset -- please
> advise.

Thanks for the patch.  On emacs-devel you said that rather than add
definitions of project-find-file-other-window, etc.
display-buffer-override-next-command could be used.  But there is no
display-buffer-override-next-command in your patch, and definitions of
project-find-file-other-window etc. are still added (with the help of macro).

Would it be possible to define the 'C-x 4 p' map the same way as
'C-x p p' is defined?  There is a patch in https://debbugs.gnu.org/41890#127
to use the default project keymap 'C-x p' in 'C-x p p'.  You could try to
use the same keymap in 'C-x 4 p', and wrap the command call with
display-buffer-override-next-command.

bug#42210: Add -other-window variants of project-prefix-map commands

[Sean Whitton]

Hello Juri,

Thanks for taking a look.

On Mon 06 Jul 2020 at 03:19AM +03, Juri Linkov wrote:

>> Here is a patch implementing commands under C-x 4 p.  If the approach is
>> thought sound I can also prepare patches for C-x 5 p and C-x t p.
>>
>> I have tested the attached change except for the autoload cookies which
>> I am not sure will work with my new macro.  And I'm not sure I should be
>> doing this with a macro instead of a function which calls fset -- please
>> advise.
>
> Thanks for the patch.  On emacs-devel you said that rather than add
> definitions of project-find-file-other-window, etc.
> display-buffer-override-next-command could be used.  But there is no
> display-buffer-override-next-command in your patch, and definitions of
> project-find-file-other-window etc. are still added (with the help of macro).

Yeah.  After thinking about it a bit more I thought that this would be
more consistent with both the way the other C-x 4 commands work and how
the C-x p subcommands work -- i.e., it's just an ordinary keymap.

Additionally, my approach means that the -other-window functions are
available to be called from lisp, just as switch-to-buffer-other-window
and find-file-other-window already are, which might be useful.

From my perspective, the use of define-other-window-command is
sufficient to resolve my concerns (posted to emacs-devel) about having to
define all the functions.  And I think that the macro could be useful in
user init files and maybe elsewhere.

> Would it be possible to define the 'C-x 4 p' map the same way as
> 'C-x p p' is defined?  There is a patch in https://debbugs.gnu.org/41890#127
> to use the default project keymap 'C-x p' in 'C-x p p'.  You could try to
> use the same keymap in 'C-x 4 p', and wrap the command call with
> display-buffer-override-next-command.

I could give it a shot, but wouldn't it be less useful, since the
-other-window functions would no longer be available to be called from
lisp?

As I say, I think the concerns about having to define the functions is
resolved by define-other-window-command (or whatever it ends up called).

Maybe you could explain why you think doing it like C-x p p would be
better.  Thanks again.

-- 
Sean Whitton

bug#42210: Add -other-window variants of project-prefix-map commands

[Juri Linkov]

>> Thanks for the patch.  On emacs-devel you said that rather than add
>> definitions of project-find-file-other-window, etc.
>> display-buffer-override-next-command could be used.  But there is no
>> display-buffer-override-next-command in your patch, and definitions of
>> project-find-file-other-window etc. are still added (with the help of macro).
>
> Yeah.  After thinking about it a bit more I thought that this would be
> more consistent with both the way the other C-x 4 commands work and how
> the C-x p subcommands work -- i.e., it's just an ordinary keymap.
>
> Additionally, my approach means that the -other-window functions are
> available to be called from lisp, just as switch-to-buffer-other-window
> and find-file-other-window already are, which might be useful.

Actually, keeping consistency with existing C-x 4 commands
is not a requirement.  It's quite possible that existing commands
will be declared deprecated as currently is discussed on emacs-devel.

>> Would it be possible to define the 'C-x 4 p' map the same way as
>> 'C-x p p' is defined?  There is a patch in https://debbugs.gnu.org/41890#127
>> to use the default project keymap 'C-x p' in 'C-x p p'.  You could try to
>> use the same keymap in 'C-x 4 p', and wrap the command call with
>> display-buffer-override-next-command.
>
> I could give it a shot, but wouldn't it be less useful, since the
> -other-window functions would no longer be available to be called from
> lisp?

I see it as an advantage that makes the namespace cleaner - there will be
no more such useless duplicates as

switch-to-buffer
switch-to-buffer-other-frame
switch-to-buffer-other-tab
switch-to-buffer-other-window

and similarly for dozens of other commands.  When browsing command names
e.g. in command completions of M-x or C-h f this will reduce the mess.

For a rare case when the users might want to bind an other-window command
directly to a non-prefix key in an init file, then maybe it's possible
to provide a macro or better a wrapper function that could be used like

(global-set-key (kbd "C-x w b") (with-other-window 'switch-to-buffer))

> Maybe you could explain why you think doing it like C-x p p would be
> better.  Thanks again.

Let's see what the emacs-devel discussion will bring, maybe C-x 4
will be bound to a command like C-x p p is implemented in
https://debbugs.gnu.org/41890#50

bug#42210: Add -other-window variants of project-prefix-map commands

[Sean Whitton]

Hello Juri,

On Tue 07 Jul 2020 at 01:59AM +03, Juri Linkov wrote:

> Actually, keeping consistency with existing C-x 4 commands
> is not a requirement.  It's quite possible that existing commands
> will be declared deprecated as currently is discussed on emacs-devel.

Okay.

>>> Would it be possible to define the 'C-x 4 p' map the same way as
>>> 'C-x p p' is defined?  There is a patch in https://debbugs.gnu.org/41890#127
>>> to use the default project keymap 'C-x p' in 'C-x p p'.  You could try to
>>> use the same keymap in 'C-x 4 p', and wrap the command call with
>>> display-buffer-override-next-command.

I'm not completely clear on the changes you're planning to C-x 4 -- do
they interact with my proposal, such that I should wait to reroll my
patch, or can I go ahead and try something equivalent to the patch in
#41890 you linked to?

-- 
Sean Whitton

bug#42210: Add -other-window variants of project-prefix-map commands

[Juri Linkov]

>>>> Would it be possible to define the 'C-x 4 p' map the same way as
>>>> 'C-x p p' is defined?  There is a patch in https://debbugs.gnu.org/41890#127
>>>> to use the default project keymap 'C-x p' in 'C-x p p'.  You could try to
>>>> use the same keymap in 'C-x 4 p', and wrap the command call with
>>>> display-buffer-override-next-command.
>
> I'm not completely clear on the changes you're planning to C-x 4 -- do
> they interact with my proposal, such that I should wait to reroll my
> patch, or can I go ahead and try something equivalent to the patch in
> #41890 you linked to?

While it's still unclear whether the global keymap ‘C-x 4’ will be replaced
by a command, if you want, feel free to try implementing the same to a more
localized keymap ‘C-x 4 p’ based on patches in bug#41890 and on the ELPA
package ‘other-frame-window’.

bug#42210: Add -other-window variants of project-prefix-map commands

[Sean Whitton]

Hello Juri,

On Thu 09 Jul 2020 at 03:10AM +03, Juri Linkov wrote:

> While it's still unclear whether the global keymap ‘C-x 4’ will be replaced
> by a command, if you want, feel free to try implementing the same to a more
> localized keymap ‘C-x 4 p’ based on patches in bug#41890 and on the ELPA
> package ‘other-frame-window’.

Have taken a look at this and I think it's a good idea.

I've posted to #41890 to ask about the status of that patch since my
work will interact with it.

-- 
Sean Whitton