bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Theodor Thornhill]

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



Hello again!

This patch adds bindings for project.el.

I followed tab-bar's example and added prefix map to subr.el, and added the bindings to bindings.el. I guess these can be removed at any point in time later.

Also, my assignment form and disclaimer is with the fsf-clerk, and has been for a while. I sent an email this morning, hoping to speed up the process.

Theo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: project-bindings.patch --]
[-- Type: text/x-patch; name=project-bindings.patch, Size: 1376 bytes --]

diff --git a/lisp/bindings.el b/lisp/bindings.el
index e3fc5637fa..52890d1896 100644
--- a/lisp/bindings.el
+++ b/lisp/bindings.el
@@ -1394,6 +1394,18 @@ ctl-x-4-map
 (define-key special-event-map [sigusr1] 'ignore)
 (define-key special-event-map [sigusr2] 'ignore)
 
+;; project.el commands
+(define-key project-prefix-map "f" 'project-find-file)
+(define-key project-prefix-map "b" 'project-switch-to-buffer)
+(define-key project-prefix-map "s" 'project-shell)
+(define-key project-prefix-map "d" 'project-dired)
+(define-key project-prefix-map "v" 'project-vc-dir)
+(define-key project-prefix-map "c" 'project-compile)
+(define-key project-prefix-map "e" 'project-eshell)
+(define-key project-prefix-map "p" 'project-switch-project)
+(define-key project-prefix-map "g" 'project-find-regexp)
+(define-key project-prefix-map "r" 'project-query-replace-regexp)
+
 ;; Don't look for autoload cookies in this file.
 ;; Local Variables:
 ;; no-update-autoloads: t
diff --git a/lisp/subr.el b/lisp/subr.el
index 10c37e9413..75da1be8de 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1265,6 +1265,10 @@ tab-prefix-map
   "Keymap for tab-bar related commands.")
 (define-key ctl-x-map "t" tab-prefix-map)
 
+(defvar project-prefix-map (make-sparse-keymap)
+  "Keymap for project commands.")
+(define-key ctl-x-map "p" project-prefix-map)
+
 \f
 ;;;; Event manipulation functions.
 

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Theodor Thornhill]

Hello :)

"Eli Zaretskii" <eliz@gnu.org> writes:


[...]
> Can you tell what is the rationale for having project.el keymap
> defined outside of project.el?

In https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41879#23
Dmitry says:

"Do we keep it in project.el or somewhere outside? Maybe the latter, since
project.el is also an ELPA package, and we generally don't want packages to
alter Emacs' key bindings right after installation."

I guess this concern is for when users not building from master is downloading 'project.el' from ELPA.

I don't really have a strong opinion for any direction. Would you rather want them in 'project.el'? 


[...]
> If nothing happens within a week or two, ping them and CC me.

He responded and said they have what they need from me and will process it further. I guess it is done at some point soon.

Theo

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Date: Tue, 16 Jun 2020 16:44:51 +0000
> From: Theodor Thornhill <theo@thornhill.no>
> Cc: 41890@debbugs.gnu.org
> 
> > Can you tell what is the rationale for having project.el keymap
> > defined outside of project.el?
> 
> In https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41879#23
> Dmitry says:
> 
> "Do we keep it in project.el or somewhere outside? Maybe the latter, since
> project.el is also an ELPA package, and we generally don't want packages to
> alter Emacs' key bindings right after installation."
> 
> I guess this concern is for when users not building from master is downloading 'project.el' from ELPA.
> 
> I don't really have a strong opinion for any direction. Would you rather want them in 'project.el'? 

I certainly would.  It is very unusual for an optional package to have
its bindings in files that are preloaded into every Emacs session.

> > If nothing happens within a week or two, ping them and CC me.
> 
> He responded and said they have what they need from me and will process it further. I guess it is done at some point soon.

Well, if there's no progress within a week or two, my suggestion still
stands.

Thanks.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Theodor Thornhill]

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


Hello,

"Eli Zaretskii" <eliz@gnu.org> writes:

[...]
> I certainly would.  It is very unusual for an optional package to have
> its bindings in files that are preloaded into every Emacs session.
>

Ok! I attached a new patch with them placed in project.el. Also cc'd Dmitry so he can chime in. Maybe we should do it more customizable?

[...]
> Well, if there's no progress within a week or two, my suggestion still
> stands.
Will definitely do :)

> Thanks.
Thank you!

Theodor


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: project-bindings.patch --]
[-- Type: text/x-patch; name=project-bindings.patch, Size: 1156 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index f3df44fa7b..7bb3831647 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -107,6 +107,21 @@ project-find-functions
 (defvar project-current-inhibit-prompt nil
   "Non-nil to skip prompting the user in `project-current'.")
 
+(defvar project-prefix-map (make-sparse-keymap)
+  "Keymap for project commands.")
+
+(define-key ctl-x-map "p" project-prefix-map)
+(define-key project-prefix-map "f" 'project-find-file)
+(define-key project-prefix-map "b" 'project-switch-to-buffer)
+(define-key project-prefix-map "s" 'project-shell)
+(define-key project-prefix-map "d" 'project-dired)
+(define-key project-prefix-map "v" 'project-vc-dir)
+(define-key project-prefix-map "c" 'project-compile)
+(define-key project-prefix-map "e" 'project-eshell)
+(define-key project-prefix-map "p" 'project-switch-project)
+(define-key project-prefix-map "g" 'project-find-regexp)
+(define-key project-prefix-map "r" 'project-query-replace-regexp)
+
 ;;;###autoload
 (defun project-current (&optional maybe-prompt dir)
   "Return the project instance in DIR or `default-directory'.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Basil L. Contovounesios]

Theodor Thornhill <theo@thornhill.no> writes:

> +(defvar project-prefix-map (make-sparse-keymap)
> +  "Keymap for project commands.")
> +
> +(define-key ctl-x-map "p" project-prefix-map)
> +(define-key project-prefix-map "f" 'project-find-file)
> +(define-key project-prefix-map "b" 'project-switch-to-buffer)
> +(define-key project-prefix-map "s" 'project-shell)
> +(define-key project-prefix-map "d" 'project-dired)
> +(define-key project-prefix-map "v" 'project-vc-dir)
> +(define-key project-prefix-map "c" 'project-compile)
> +(define-key project-prefix-map "e" 'project-eshell)
> +(define-key project-prefix-map "p" 'project-switch-project)
> +(define-key project-prefix-map "g" 'project-find-regexp)
> +(define-key project-prefix-map "r" 'project-query-replace-regexp)

This should be:

  (defvar project-prefix-map
    (let ((map (make-sparse-keymap)))
      (define-key map ...)
      ...
      map)
    "...")

  (define-key ctl-x-map "p" project-prefix-map)

See the end of (info "(elisp) Tips for Defining").

Maybe it should also be autoloaded.

Thanks,
      
-- 
Basil

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Theodor Thornhill]

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

Hi and thanks again for tips!

> This should be:
>
>   (defvar project-prefix-map
>     (let ((map (make-sparse-keymap)))
>       (define-key map ...)
>       ...
>       map)
>     "...")
>
>   (define-key ctl-x-map "p" project-prefix-map)
>
> See the end of (info "(elisp) Tips for Defining").
>
> Maybe it should also be autoloaded.

Below is another patch with your suggestions incorporated. Is it correct to also autoload the (define-key ctl-x-map "p" project-prefix-map)?

Theo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: project-bindings.patch --]
[-- Type: text/x-patch; name=project-bindings.patch, Size: 1095 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index f3df44fa7b..d3fb34d925 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -107,6 +107,24 @@ project-find-functions
 (defvar project-current-inhibit-prompt nil
   "Non-nil to skip prompting the user in `project-current'.")
 
+;;;###autoload
+(defvar project-prefix-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "f" 'project-find-file)
+    (define-key map "b" 'project-switch-to-buffer)
+    (define-key map "s" 'project-shell)
+    (define-key map "d" 'project-dired)
+    (define-key map "v" 'project-vc-dir)
+    (define-key map "c" 'project-compile)
+    (define-key map "e" 'project-eshell)
+    (define-key map "p" 'project-switch-project)
+    (define-key map "g" 'project-find-regexp)
+    (define-key map "r" 'project-query-replace-regexp))
+  "Keymap for project commands.")
+
+;;;###autoload
+(define-key ctl-x-map "p" project-prefix-map)
+
 ;;;###autoload
 (defun project-current (&optional maybe-prompt dir)
   "Return the project instance in DIR or `default-directory'.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Theodor Thornhill]

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

"Theodor Thornhill" <theo@thornhill.no> writes:

> Below is another patch with your suggestions incorporated. Is it correct to also autoload the (define-key ctl-x-map "p" project-prefix-map)?

Disregard last one. It was missing a map returned at the end. Sorry for the inconvenience.

Theo



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: project-bindings.patch --]
[-- Type: text/x-patch; name=project-bindings.patch, Size: 1104 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index f3df44fa7b..088c698398 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -107,6 +107,25 @@ project-find-functions
 (defvar project-current-inhibit-prompt nil
   "Non-nil to skip prompting the user in `project-current'.")
 
+;;;###autoload
+(defvar project-prefix-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "f" 'project-find-file)
+    (define-key map "b" 'project-switch-to-buffer)
+    (define-key map "s" 'project-shell)
+    (define-key map "d" 'project-dired)
+    (define-key map "v" 'project-vc-dir)
+    (define-key map "c" 'project-compile)
+    (define-key map "e" 'project-eshell)
+    (define-key map "p" 'project-switch-project)
+    (define-key map "g" 'project-find-regexp)
+    (define-key map "r" 'project-query-replace-regexp)
+    map)
+  "Keymap for project commands.")
+
+;;;###autoload
+(define-key ctl-x-map "p" project-prefix-map)
+
 ;;;###autoload
 (defun project-current (&optional maybe-prompt dir)
   "Return the project instance in DIR or `default-directory'.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

> +    (define-key map "f" 'project-find-file)
> +    (define-key map "b" 'project-switch-to-buffer)
> +    (define-key map "s" 'project-shell)
> +    (define-key map "d" 'project-dired)
> +    (define-key map "v" 'project-vc-dir)
> +    (define-key map "c" 'project-compile)
> +    (define-key map "e" 'project-eshell)
> +    (define-key map "p" 'project-switch-project)
> +    (define-key map "g" 'project-find-regexp)
> +    (define-key map "r" 'project-query-replace-regexp)

I think your choice of keys is better than in project-switch-commands.
Maybe these keys should be copied to project-switch-commands, so it will to be
in sync with project-prefix-map?

Or is it possible to use project-prefix-map directly in project-switch-commands?
For example, by using set-transient-map?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 17.06.2020 00:57, Juri Linkov wrote:
>> +    (define-key map "f" 'project-find-file)
>> +    (define-key map "b" 'project-switch-to-buffer)
>> +    (define-key map "s" 'project-shell)
>> +    (define-key map "d" 'project-dired)
>> +    (define-key map "v" 'project-vc-dir)
>> +    (define-key map "c" 'project-compile)
>> +    (define-key map "e" 'project-eshell)
>> +    (define-key map "p" 'project-switch-project)
>> +    (define-key map "g" 'project-find-regexp)
>> +    (define-key map "r" 'project-query-replace-regexp)
> 
> I think your choice of keys is better than in project-switch-commands.
> Maybe these keys should be copied to project-switch-commands, so it will to be
> in sync with project-prefix-map?

I'll make sure to keep them compatible in the default setup.

> Or is it possible to use project-prefix-map directly in project-switch-commands?
> For example, by using set-transient-map?

We discussed that with Simen in private previously. The current 
implementation is "visual", which is good for discoverability.

I think that kind of limits us, however, to showing only the most 
"essential" commands (think: ones that the user is most likely to use 
right after switching to a different project), and not the whole set.

Or else people will spend more time searching for the key they need to 
press. And they won't use most of the entries in the list anyway.

For that reason also, I just removed the project-shell entry from that 
list because we haven't reached to solid conclusion WRT shell vs eshell, 
and yet it was time to do a release. FWIW, I'm fine with either option, 
but we probably don't need both in the list (we should be fine with 
having both in project-prefix-map, however).

I also forgot to mention that in the commit message (3bff583). Sorry!

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

> For that reason also, I just removed the project-shell entry from that list
> because we haven't reached to solid conclusion WRT shell vs eshell, and yet
> it was time to do a release. FWIW, I'm fine with either option, but we
> probably don't need both in the list (we should be fine with having both in
> project-prefix-map, however).
>
> I also forgot to mention that in the commit message (3bff583). Sorry!

Very sad, this is the keybinding that I used most often :(

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 17.06.2020 02:24, Juri Linkov wrote:
> Very sad, this is the keybinding that I used most often:(

Did you really use it in project-switch-project specifically, or because 
there are no global bindings for project commands yet?

Would you like to whip up a poll, on emacs-devel or Reddit, about which 
of eshell or shell is more popular?

We'll put the winner on ?e in project-switch-commands, and we can put 
the other one on "E" in project-prefix-map as well.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> Very sad, this is the keybinding that I used most often:(
>
> Did you really use it in project-switch-project specifically, or because
> there are no global bindings for project commands yet?

I think global bindings for project commands are more important
than project-switch-project specifically.

> Would you like to whip up a poll, on emacs-devel or Reddit, about which of
> eshell or shell is more popular?

No, it's not an important question.

> We'll put the winner on ?e in project-switch-commands, and we can put the
> other one on "E" in project-prefix-map as well.

Do you want to remove this intuitive binding 's' from 'shell' command
because you want to use 's' for some other command?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 18.06.2020 00:23, Juri Linkov wrote:
>>> Very sad, this is the keybinding that I used most often:(
>>
>> Did you really use it in project-switch-project specifically, or because
>> there are no global bindings for project commands yet?
> 
> I think global bindings for project commands are more important
> than project-switch-project specifically.

Very good.

>> We'll put the winner on ?e in project-switch-commands, and we can put the
>> other one on "E" in project-prefix-map as well.
> 
> Do you want to remove this intuitive binding 's' from 'shell' command
> because you want to use 's' for some other command?

Maybe project-isearch? I recall you wanted that one day.

If not, I'm fine with keeping project-shell on 's' for now.

But if reach the shortage of characters on this keymap someday, and some 
unbound command would really fit that letter, I'd rather not dedicate 
two different characters to shell-like functionality.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> Do you want to remove this intuitive binding 's' from 'shell' command
>> because you want to use 's' for some other command?
>
> Maybe project-isearch? I recall you wanted that one day.

project-isearch should use 'C-x p C-s' like it's used in e.g.

  M-s a C-s       dired-do-isearch
  M-s a C-s       vc-dir-isearch

> If not, I'm fine with keeping project-shell on 's' for now.
>
> But if reach the shortage of characters on this keymap someday, and some
> unbound command would really fit that letter, I'd rather not dedicate 
> two different characters to shell-like functionality.

I don't insist on 's' for shell.  Maybe 's' is more suitable for 'project-search'.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 18.06.2020 01:27, Juri Linkov wrote:
>>> Do you want to remove this intuitive binding 's' from 'shell' command
>>> because you want to use 's' for some other command?
>>
>> Maybe project-isearch? I recall you wanted that one day.
> 
> project-isearch should use 'C-x p C-s' like it's used in e.g.
> 
>    M-s a C-s       dired-do-isearch
>    M-s a C-s       vc-dir-isearch

Sounds good.

>> If not, I'm fine with keeping project-shell on 's' for now.
>>
>> But if reach the shortage of characters on this keymap someday, and some
>> unbound command would really fit that letter, I'd rather not dedicate
>> two different characters to shell-like functionality.
> 
> I don't insist on 's' for shell.  Maybe 's' is more suitable for 'project-search'.

Perhaps. As it is currently, though, I'm not in a hurry to advertise 
this command because of its performance characteristics. And because it 
can simply give up with "gpg: decrypt_message failed: Unexpected error".

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> I don't insist on 's' for shell.  Maybe 's' is more suitable for
>> 'project-search'.
>
> Perhaps. As it is currently, though, I'm not in a hurry to advertise this
> command because of its performance characteristics. And because it can
> simply give up with "gpg: decrypt_message failed: Unexpected error".

BTW, shouldn't 'C-x p v' be bound to the whole vc prefix map?
So 'C-x p v d' will run vc-dir in the project root dir,
'C-x p v =' - vc-root-diff in the project root dir,
'C-x p v v' - project's vc-next-action, etc.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 18.06.2020 02:23, Juri Linkov wrote:
> BTW, shouldn't 'C-x p v' be bound to the whole vc prefix map?
> So 'C-x p v d' will run vc-dir in the project root dir,
> 'C-x p v =' - vc-root-diff in the project root dir,
> 'C-x p v v' - project's vc-next-action, etc.

That might be over-engineering it a bit.

Considering that in the vast majority of cases the VC root and the 
project root are going to be the same.

So the users will get the same results from 'C-x p v =' as from 'C-x v 
D', and 'C-x p v d' would usually be the same as 'C-x v d' (but having 
one binding for the cases when it's different is fine, I guess).

How would 'C-x p v v' differ from 'C-x v v'?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> BTW, shouldn't 'C-x p v' be bound to the whole vc prefix map?
>> So 'C-x p v d' will run vc-dir in the project root dir,
>> 'C-x p v =' - vc-root-diff in the project root dir,
>> 'C-x p v v' - project's vc-next-action, etc.
>
> That might be over-engineering it a bit.
>
> Considering that in the vast majority of cases the VC root and the project
> root are going to be the same.
>
> So the users will get the same results from 'C-x p v =' as from 'C-x v D',
> and 'C-x p v d' would usually be the same as 'C-x v d' (but having one
> binding for the cases when it's different is fine, I guess).
>
> How would 'C-x p v v' differ from 'C-x v v'?

Actually my question would be better formulated like this:

Should we reserve the key 'C-x p v' as a prefix map for possible future
extensions when such need to add more vc keys will arise?
Currently I see no such need, but who knows.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 19.06.2020 01:59, Juri Linkov wrote:
> Actually my question would be better formulated like this:
> 
> Should we reserve the key 'C-x p v' as a prefix map for possible future
> extensions when such need to add more vc keys will arise?
> Currently I see no such need, but who knows.

I'd rather keep it as is. And if such actual need arises, we can change 
the binding for project-vc-dir. It already "reserves" it, in a way. :-)

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> BTW, shouldn't 'C-x p v' be bound to the whole vc prefix map?
>> So 'C-x p v d' will run vc-dir in the project root dir,
>> 'C-x p v =' - vc-root-diff in the project root dir,
>> 'C-x p v v' - project's vc-next-action, etc.
>
> That might be over-engineering it a bit.
>
> Considering that in the vast majority of cases the VC root and the project
> root are going to be the same.
>
> So the users will get the same results from 'C-x p v =' as from 'C-x v D',
> and 'C-x p v d' would usually be the same as 'C-x v d' (but having one
> binding for the cases when it's different is fine, I guess).

So now we finally have a key sequence 'C-x p v' with the same length
as 'C-x v d' but that doesn't require confirmation by RET?  Nice.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 21.06.2020 02:41, Juri Linkov wrote:
> So now we finally have a key sequence 'C-x p v' with the same length
> as 'C-x v d' but that doesn't require confirmation by RET?  Nice.

Indeed. It can fail with non-default project backends, but we'll get 
there when we get there.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Philip K.]

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

Juri Linkov <juri@linkov.net> writes:

> I think your choice of keys is better than in project-switch-commands.
> Maybe these keys should be copied to project-switch-commands, so it will to be
> in sync with project-prefix-map?
>
> Or is it possible to use project-prefix-map directly in project-switch-commands?
> For example, by using set-transient-map?

I tried implementig it, and it seems to work. The patch below isn't a
full commit, since I changed the structure of project-switch-commands
(it's now mapping command to description), but didn't update the
docstring.

-- 
	Philip K.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff, Size: 2111 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index c5301dccd3..6cb9811d3d 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -860,12 +879,12 @@ project-prompt-project-dir
 
 ;;;###autoload
 (defvar project-switch-commands
-  '((?f "Find file" project-find-file)
-    (?r "Find regexp" project-find-regexp)
-    (?d "Dired" project-dired)
-    (?v "VC-Dir" project-vc-dir)
-    (?s "Shell" project-shell)
-    (?e "Eshell" project-eshell))
+  '((project-find-file "Find file")
+    (project-find-regexp "Find regexp")
+    (project-dired "Dired")
+    (project-vc-dir "VC-Dir")
+    (project-shell "Shell")
+    (project-eshell "Eshell"))
   "Alist mapping keys to project switching menu entries.
 Used by `project-switch-project' to construct a dispatch menu of
 commands available upon \"switching\" to another project.
@@ -877,9 +896,10 @@ project-switch-commands
 (defun project--keymap-prompt ()
   "Return a prompt for the project swithing dispatch menu."
   (mapconcat
-   (pcase-lambda (`(,key ,label))
+   (pcase-lambda (`(,cmd ,label))
      (format "[%s] %s"
-             (propertize (key-description `(,key)) 'face 'bold)
+             (propertize (key-description (where-is-internal cmd project-prefix-map t))
+                         'face 'bold)
              label))
    project-switch-commands
    "  "))
@@ -890,14 +910,10 @@ project-switch-project
 The available commands are picked from `project-switch-commands'
 and presented in a dispatch menu."
   (interactive)
-  (let ((dir (project-prompt-project-dir))
-        (choice nil))
-    (while (not choice)
-      (setq choice (assq (read-event (project--keymap-prompt))
-                         project-switch-commands)))
-    (let ((default-directory dir)
-          (project-current-inhibit-prompt t))
-      (call-interactively (nth 2 choice)))))
+  (let ((default-directory (project-prompt-project-dir))
+        (project-current-inhibit-prompt t))
+    (message "%s" (project--keymap-prompt))
+    (set-transient-map project-prefix-map)))
 
 (provide 'project)
 ;;; project.el ends here

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Basil L. Contovounesios]

Theodor Thornhill <theo@thornhill.no> writes:

>> This should be:
>>
>>   (defvar project-prefix-map
>>     (let ((map (make-sparse-keymap)))
>>       (define-key map ...)
>>       ...
>>       map)
>>     "...")
>>
>>   (define-key ctl-x-map "p" project-prefix-map)
>>
>> See the end of (info "(elisp) Tips for Defining").
>>
>> Maybe it should also be autoloaded.
>
> Below is another patch with your suggestions incorporated. Is it correct to also
> autoload the (define-key ctl-x-map "p" project-prefix-map)?

I'm not an autoload expert, so I'd appreciate if someone else chimed in,
but according to the commentary in lisp/bookmark.el,

  ;;;###autoload (define-key ...)

is preferable to

  ;;;###autoload
  (define-key ...)

since the former is copied to lisp/ldefs-boot.el at build time and
skipped at load time (since it's in a comment), so it doesn't override
existing user settings.  Would that do what we want?

Thanks,

-- 
Basil

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Theodor Thornhill]

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


Hello!
"Basil L. Contovounesios" <contovob@tcd.ie> writes:

>   ;;;###autoload (define-key ...)
>
> is preferable to
>
>   ;;;###autoload
>   (define-key ...)
>

Nice idea! Did not know about this :)

Since Eli also mentioned this, I've attached such a patch.

Theo


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: project-bindings.patch --]
[-- Type: text/x-patch; name=project-bindings.patch, Size: 1103 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index f3df44fa7b..87cd015924 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -107,6 +107,24 @@ project-find-functions
 (defvar project-current-inhibit-prompt nil
   "Non-nil to skip prompting the user in `project-current'.")
 
+;;;###autoload
+(defvar project-prefix-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "f" 'project-find-file)
+    (define-key map "b" 'project-switch-to-buffer)
+    (define-key map "s" 'project-shell)
+    (define-key map "d" 'project-dired)
+    (define-key map "v" 'project-vc-dir)
+    (define-key map "c" 'project-compile)
+    (define-key map "e" 'project-eshell)
+    (define-key map "p" 'project-switch-project)
+    (define-key map "g" 'project-find-regexp)
+    (define-key map "r" 'project-query-replace-regexp)
+    map)
+  "Keymap for project commands.")
+
+;;;###autoload (define-key ctl-x-map "p" project-prefix-map)
+
 ;;;###autoload
 (defun project-current (&optional maybe-prompt dir)
   "Return the project instance in DIR or `default-directory'.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Basil L. Contovounesios]

Theodor Thornhill <theo@thornhill.no> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>>   ;;;###autoload (define-key ...)
>>
>> is preferable to
>>
>>   ;;;###autoload
>>   (define-key ...)
>
> Nice idea! Did not know about this :)
>
> Since Eli also mentioned this, I've attached such a patch.

LGTM, thanks.

-- 
Basil

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 17.06.2020 22:10, Theodor Thornhill wrote:
> Since Eli also mentioned this, I've attached such a patch.

Applied, thank you!

In the future, though, please include commit messages with the patches 
(at least, as soon as you're reasonably confident the patch will be 
accepted). See CONTRIBUTE for the message requirements.

Also, I'm happy to report that your assignment is now on file.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 16.06.2020 20:16, Eli Zaretskii wrote:
> I certainly would.  It is very unusual for an optional package to have
> its bindings in files that are preloaded into every Emacs session.

What do you mean by "optional"? It's in Emacs 28.

How is it different from the aforementioned tabs?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 17.06.2020 00:23, Dmitry Gutov wrote:
> On 16.06.2020 20:16, Eli Zaretskii wrote:
>> I certainly would.  It is very unusual for an optional package to have
>> its bindings in files that are preloaded into every Emacs session.
> 
> What do you mean by "optional"? It's in Emacs 28.
> 
> How is it different from the aforementioned tabs?

Let's step back, maybe.

 From multiple discussions, over a certain period of time, mostly here 
in emacs-devel, I have arrived at an impression that there exists a 
general desire to have global bindings for project functions, in 
"default" Emacs, without customization.

And that the 'C-x p' prefix is both unoccupied and looks logical to use 
for that purpose.

What do you think about that?

As an aside, I'd like to do a similar thing for Flymake, with 
Flycheck-inspired prefix of 'C-c !' a bit later.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Cc: 41890@debbugs.gnu.org
> Date: Wed, 17 Jun 2020 00:35:26 +0300
> 
>  From multiple discussions, over a certain period of time, mostly here 
> in emacs-devel, I have arrived at an impression that there exists a 
> general desire to have global bindings for project functions, in 
> "default" Emacs, without customization.
> 
> And that the 'C-x p' prefix is both unoccupied and looks logical to use 
> for that purpose.
> 
> What do you think about that?
> 
> As an aside, I'd like to do a similar thing for Flymake, with 
> Flycheck-inspired prefix of 'C-c !' a bit later.

I don't think I'd object, but this is orthogonal to the issue on which
I commented: where are the key bindings defined.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Cc: 41890@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 17 Jun 2020 00:23:27 +0300
> 
> On 16.06.2020 20:16, Eli Zaretskii wrote:
> > I certainly would.  It is very unusual for an optional package to have
> > its bindings in files that are preloaded into every Emacs session.
> 
> What do you mean by "optional"?

I mean it isn't preloaded, but is loaded on demand.

> How is it different from the aforementioned tabs?

tab-bar _is_ preloaded.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 17.06.2020 17:27, Eli Zaretskii wrote:
> I mean it isn't preloaded, but is loaded on demand.

Okay, but... the commands in the keymap will all be autoloaded. So 
whenever somebody calls them, the aforementioned optional package will 
get loaded. I'm not sure what the practical issue with that is.

The issue on the other side (keeping the keymap definition in 
project.el) is that it's an ELPA package as well. And so far we've said 
that ELPA packages shouldn't significantly modify a user's Emacs just by 
the virtue of being installed.

OTOH, if these bindings will be defined in Emacs 28, perhaps this rule 
can be given exception in these two cases.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Cc: theo@thornhill.no, 41890@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 17 Jun 2020 18:49:47 +0300
> 
> On 17.06.2020 17:27, Eli Zaretskii wrote:
> > I mean it isn't preloaded, but is loaded on demand.
> 
> Okay, but... the commands in the keymap will all be autoloaded. So 
> whenever somebody calls them, the aforementioned optional package will 
> get loaded. I'm not sure what the practical issue with that is.

I don't see how this is related to the issue at hand.  All I'm saying
is that a package, including its key bindings, shouldn't be loaded
until some of its feature is invoked.  Therefore, the best place for a
package's keybindings is in the package itself.  What you describe
seems to fit this principle.

> The issue on the other side (keeping the keymap definition in 
> project.el) is that it's an ELPA package as well. And so far we've said 
> that ELPA packages shouldn't significantly modify a user's Emacs just by 
> the virtue of being installed.

We could make the keybindings autoloaded without having them defined
them when the package loads, couldn't we?  By having the define-key on
the same line as the autoload cookie, like bookmark.el does.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 17.06.2020 19:33, Eli Zaretskii wrote:

>> On 17.06.2020 17:27, Eli Zaretskii wrote:
>>> I mean it isn't preloaded, but is loaded on demand.
>>
>> Okay, but... the commands in the keymap will all be autoloaded. So
>> whenever somebody calls them, the aforementioned optional package will
>> get loaded. I'm not sure what the practical issue with that is.
> 
> I don't see how this is related to the issue at hand.  All I'm saying
> is that a package, including its key bindings, shouldn't be loaded
> until some of its feature is invoked.

But if we autoload the bindings definition forms, wouldn't that have 
essentially the same effect?

> Therefore, the best place for a
> package's keybindings is in the package itself.  What you describe
> seems to fit this principle.

Okay.

>> The issue on the other side (keeping the keymap definition in
>> project.el) is that it's an ELPA package as well. And so far we've said
>> that ELPA packages shouldn't significantly modify a user's Emacs just by
>> the virtue of being installed.
> 
> We could make the keybindings autoloaded without having them defined
> them when the package loads, couldn't we?  By having the define-key on
> the same line as the autoload cookie, like bookmark.el does.

That would generally be considered problematic because the keymap would 
take effect right after the user updates to the newest version of 
project.el. Because package.el also compiles and evaluates autoloads.

Anyway, I'll apply this patch now, but we can continue this discussion.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Cc: 41890@debbugs.gnu.org, theo@thornhill.no
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 18 Jun 2020 01:23:25 +0300
> 
> > I don't see how this is related to the issue at hand.  All I'm saying
> > is that a package, including its key bindings, shouldn't be loaded
> > until some of its feature is invoked.
> 
> But if we autoload the bindings definition forms, wouldn't that have 
> essentially the same effect?

How is this different from bookmark.el?

And if we don't want these key bindings to be available always, we
could have a separate autoloads file for project.el.  Some packages do
that already.

> > We could make the keybindings autoloaded without having them defined
> > them when the package loads, couldn't we?  By having the define-key on
> > the same line as the autoload cookie, like bookmark.el does.
> 
> That would generally be considered problematic because the keymap would 
> take effect right after the user updates to the newest version of 
> project.el. Because package.el also compiles and evaluates autoloads.

Why is that a problem?  A user who updates project.el is most
probably going to use it, right?

And if we do care about this, we could use a separate autoloads file.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 18.06.2020 16:38, Eli Zaretskii wrote:
>> Cc: 41890@debbugs.gnu.org, theo@thornhill.no
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Thu, 18 Jun 2020 01:23:25 +0300
>>
>>> I don't see how this is related to the issue at hand.  All I'm saying
>>> is that a package, including its key bindings, shouldn't be loaded
>>> until some of its feature is invoked.
>>
>> But if we autoload the bindings definition forms, wouldn't that have
>> essentially the same effect?
> 
> How is this different from bookmark.el?

I don't really know much about bookmark.el, or the way it was written 
and why.

> And if we don't want these key bindings to be available always, we
> could have a separate autoloads file for project.el.  Some packages do
> that already.

We might not want them when the package is simply installed through ELPA.

But we'll probably always want them on by default in Emacs 28 and newer.

I'm curious what Stefan thinks about it.

>>> We could make the keybindings autoloaded without having them defined
>>> them when the package loads, couldn't we?  By having the define-key on
>>> the same line as the autoload cookie, like bookmark.el does.
>>
>> That would generally be considered problematic because the keymap would
>> take effect right after the user updates to the newest version of
>> project.el. Because package.el also compiles and evaluates autoloads.
> 
> Why is that a problem?  A user who updates project.el is most
> probably going to use it, right?

Probably. But it's also a dependency of certain packages like eglot or 
xref, so it's not a given that the user chose to update it intentionally.

> And if we do care about this, we could use a separate autoloads file.

Which the users would have to (require '...)?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Cc: 41890@debbugs.gnu.org, theo@thornhill.no,
>  Stefan Monnier <monnier@IRO.UMontreal.CA>
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 18 Jun 2020 18:47:37 +0300
> 
> > How is this different from bookmark.el?
> 
> I don't really know much about bookmark.el, or the way it was written 
> and why.
> 
> > And if we don't want these key bindings to be available always, we
> > could have a separate autoloads file for project.el.  Some packages do
> > that already.
> 
> We might not want them when the package is simply installed through ELPA.

So is Tramp.  Maybe Michael could share his experience with the
separate autoloads file.

> But we'll probably always want them on by default in Emacs 28 and newer.

I suggest to think about crossing that bridge when we get to it.

> >> That would generally be considered problematic because the keymap would
> >> take effect right after the user updates to the newest version of
> >> project.el. Because package.el also compiles and evaluates autoloads.
> > 
> > Why is that a problem?  A user who updates project.el is most
> > probably going to use it, right?
> 
> Probably. But it's also a dependency of certain packages like eglot or 
> xref, so it's not a given that the user chose to update it intentionally.

I don't think it matters whether project.el is update on its own right
or as a dependency.  It will be used regardless, so having its
autoloads loaded doesn't sound like a serious problem.

> > And if we do care about this, we could use a separate autoloads file.
> 
> Which the users would have to (require '...)?

Do they do that with the likes of tramp-loaddefs.el?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> > And if we don't want these key bindings to be available always, we
>> > could have a separate autoloads file for project.el.  Some packages do
>> > that already.
>>
>> We might not want them when the package is simply installed through ELPA.
>
> So is Tramp.  Maybe Michael could share his experience with the
> separate autoloads file.

There's no magic. Tramp isn't used by many users, so it tries to keep
calm. Only the absolute minimum of functions and variables are
;;;###autoloaded. Everything else, which needs to be autoloaded in Tramp
(mainly objects which are shared by different tramp-*.el files), are
autoloaded with the ";;;###tramp-autoload" cookie. A file
tramp-loaddefs.el is generated during compilation, and this file is
required once the Tramp basic file, tramp.el, is loaded. That's it.

Such a *-loaddefs.el could also contain key bindings, which are activated
once the *.loaddefs.el file is loaded. Tramp doesn't define own key bindings,
so there's no need for it in tramp-loaddefs.el.

>> > And if we do care about this, we could use a separate autoloads file.
>>
>> Which the users would have to (require '...)?
>
> Do they do that with the likes of tramp-loaddefs.el?

Of course not! It is required tramp.el.

Best regards, Michael.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Stefan Monnier]

> I certainly would.  It is very unusual for an optional package to have
> its bindings in files that are preloaded into every Emacs session.

[ I'm not sure if your objection is on where the bindings are located in
  the source code (which could be adjusted by moving them to project.el
  with an appropriate autoload cookie), or whether the bindings are
  placed in the global map.  I'll assume below that the problem is with
  the actual bindings rather than their source location.  ]

It's the case for rect.el bindings, for gud-key-prefix bindings,
bookmark bindings, a few others, tho.
[ I'd put register.el bindings in that same boat because I consider this
  package just as optional (I never use it), tho it's admittedly
  preloaded ]

The question is whether project.el is special enough to warrant changing
the default global map.

If not, then the only other sane way is to link them to a minor mode
and only activate the bindings when that minor mode is enabled.

My own take on what Emacs is mostly used for makes me feel that the
notion of project is probably important enough to justify its place in
the default keymap.  But I must admit I haven't looked at the actual
bindings that are discussed, so feel free to ignore me.


        Stefan

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Theodor Thornhill <theo@thornhill.no>,  41890@debbugs.gnu.org
> Date: Thu, 18 Jun 2020 12:25:22 -0400
> 
> > I certainly would.  It is very unusual for an optional package to have
> > its bindings in files that are preloaded into every Emacs session.
> 
> [ I'm not sure if your objection is on where the bindings are located in
>   the source code (which could be adjusted by moving them to project.el
>   with an appropriate autoload cookie), or whether the bindings are
>   placed in the global map.  I'll assume below that the problem is with
>   the actual bindings rather than their source location.  ]

It's with both, actually.

> My own take on what Emacs is mostly used for makes me feel that the
> notion of project is probably important enough to justify its place in
> the default keymap.

I hope this will be the case in some not too distant future.  But
until it does, I see no reason to hurry with that.  (Assuming I
understand correctly what you meant here.)

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 18.06.2020 20:30, Eli Zaretskii wrote:
>> My own take on what Emacs is mostly used for makes me feel that the
>> notion of project is probably important enough to justify its place in
>> the default keymap.
> I hope this will be the case in some not too distant future.  But
> until it does, I see no reason to hurry with that.  (Assuming I
> understand correctly what you meant here.)

I must admit, I have lost track of this discussion.

Eli, do you have problems with what has been added to project.el last night?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Cc: 41890@debbugs.gnu.org, theo@thornhill.no
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 18 Jun 2020 21:22:39 +0300
> 
> Eli, do you have problems with what has been added to project.el last night?

Please specify the commit(s), as "last night" is too ambiguous, and
there were a lot of commits in project.el lately.  I could easily make
a mistake.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 18.06.2020 21:42, Eli Zaretskii wrote:
> Please specify the commit(s), as "last night" is too ambiguous, and
> there were a lot of commits in project.el lately.  I could easily make
> a mistake.

Commit 2f231fcfb7.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Cc: 41890@debbugs.gnu.org, theo@thornhill.no, monnier@iro.umontreal.ca
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 18 Jun 2020 21:54:59 +0300
> 
> On 18.06.2020 21:42, Eli Zaretskii wrote:
> > Please specify the commit(s), as "last night" is too ambiguous, and
> > there were a lot of commits in project.el lately.  I could easily make
> > a mistake.
> 
> Commit 2f231fcfb7.

No objection here.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 18.06.2020 22:04, Eli Zaretskii wrote:
>> Cc: 41890@debbugs.gnu.org, theo@thornhill.no, monnier@iro.umontreal.ca
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Thu, 18 Jun 2020 21:54:59 +0300
>>
>> On 18.06.2020 21:42, Eli Zaretskii wrote:
>>> Please specify the commit(s), as "last night" is too ambiguous, and
>>> there were a lot of commits in project.el lately.  I could easily make
>>> a mistake.
>>
>> Commit 2f231fcfb7.
> 
> No objection here.

Thank you.

So what did you mean by "in some not too distant future" and "no reason 
to hurry"? Project commands are functional, and they are in the global 
map, for all practical purposes.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Cc: 41890@debbugs.gnu.org, theo@thornhill.no, monnier@iro.umontreal.ca
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 19 Jun 2020 00:12:30 +0300
> 
> So what did you mean by "in some not too distant future" and "no reason 
> to hurry"? Project commands are functional, and they are in the global 
> map, for all practical purposes.

I'm glad to see project.el being actively worked on, but IMO it has
yet some turf to cover before it becomes useful enough for us to think
about preloading it.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Philip K.]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 18.06.2020 00:05, Juri Linkov wrote:
>>>> I tried implementig it, and it seems to work. The patch below isn't a
>>>> full commit, since I changed the structure of project-switch-commands
>>>> (it's now mapping command to description), but didn't update the
>>>> docstring.
>>>
>>> This is looking pretty good.
>> 
>> I agree.
>> 
>>> It's a backward incompatibility, though.
>> 
>> Not a problem since it was added recently.
>
> Here's another concern: right now, if the user types some other 
> character by accident, the command will keep showing the prompt until 
> the user hits one of the chars corresponding to the displayed options.
>
> If we just look in the (bigger) project keymap, in some cases we would 
> call commands that are not shown at the screen as a result of accidental 
> presses. And that's a negative.
>
> Perhaps we could add a user option that would default to the current 
> behavior? But then the implementation couldn't use the transient map, 
> though.

The patch below fixes that, but allows changing if you only want the
listed keys to be valid (the default) or every key in
project-prefix-map.

It turned out that the transiment map approach didn't work, as it
ignored the value in default-directory, thus running all commands in
whatever the current project was.

-- 
	Philip K.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-same-keys-in-project-switch-project-as-in-projec.patch --]
[-- Type: text/x-diff, Size: 4134 bytes --]

>From 608a94a2fee42b89b0ae395ce9d5622cb884d9d1 Mon Sep 17 00:00:00 2001
From: Philip K <philip@warpmail.net>
Date: Thu, 18 Jun 2020 16:06:19 +0200
Subject: [PATCH] Use same keys in project-switch-project as in
 project-prefix-map

* project.el (project-switch-commands): Convert to user option and
change structure.
(project-switch-use-entire-map): Add new option.
(project--keymap-prompt): Adapt to change in project-switch-commands
(project-switch-project): Use project-prefix-map instead of
project-switch-commands to query valid commands.
---
 lisp/progmodes/project.el | 63 +++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 22 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index e24d81c1b4..33946f78a8 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -900,27 +900,46 @@ project-prompt-project-dir
 ;;; Project switching
 
 ;;;###autoload
-(defvar project-switch-commands
-  '((?f "Find file" project-find-file)
-    (?g "Find regexp" project-find-regexp)
-    (?d "Dired" project-dired)
-    (?v "VC-Dir" project-vc-dir)
-    (?e "Eshell" project-eshell))
-  "Alist mapping keys to project switching menu entries.
+(defcustom project-switch-commands
+  '((project-find-file . "Find file")
+    (project-find-regexp . "Find regexp")
+    (project-dired . "Dired")
+    (project-vc-dir . "VC-Dir")
+    (project-shell . "Shell")
+    (project-eshell . "Eshell"))
+  "Alist mapping commands to descriptions.
 Used by `project-switch-project' to construct a dispatch menu of
 commands available upon \"switching\" to another project.
 
-Each element looks like (KEY LABEL COMMAND), where COMMAND is the
-command to run when KEY is pressed.  LABEL is used to distinguish
-the choice in the dispatch menu.")
+Each element looks like (COMMAND LABEL), where COMMAND should be
+bound in `project-prefix-map'.  LABEL is used to distinguish the
+choice in the dispatch menu."
+  :type '(alist :key-type function
+                :value-type string)
+  :options (mapcan (lambda (ent)
+                     (and (commandp (cdr ent))
+                          (list (cdr ent))))
+                   (cdr project-prefix-map))
+  :version "28.1")
+
+(defcustom project-switch-use-entire-map t
+  "Make `project-switch-project' use entire `project-prefix-map'.
+If nil, `project-switch-project' will only recognize commands
+listed in `project-switch-commands', and signal an error when
+others are invoked.  Otherwise, all keys in
+`project-switch-commands', are legal even if they aren't listed
+in the minibuffer."
+  :type 'bool
+  :version "28.1")
 
 (defun project--keymap-prompt ()
   "Return a prompt for the project swithing dispatch menu."
   (mapconcat
-   (pcase-lambda (`(,key ,label))
-     (format "[%s] %s"
-             (propertize (key-description `(,key)) 'face 'bold)
-             label))
+   (pcase-lambda (`(,cmd . ,label))
+     (let ((key (where-is-internal cmd project-prefix-map t)))
+       (format "[%s] %s"
+               (propertize (key-description key) 'face 'bold)
+               label)))
    project-switch-commands
    "  "))
 
@@ -930,14 +949,14 @@ project-switch-project
 The available commands are picked from `project-switch-commands'
 and presented in a dispatch menu."
   (interactive)
-  (let ((dir (project-prompt-project-dir))
-        (choice nil))
-    (while (not choice)
-      (setq choice (assq (read-event (project--keymap-prompt))
-                         project-switch-commands)))
-    (let ((default-directory dir)
-          (project-current-inhibit-prompt t))
-      (call-interactively (nth 2 choice)))))
+  (let* ((default-directory (project-prompt-project-dir))
+         (project-current-inhibit-prompt t)
+         (key (read-key-sequence-vector (project--keymap-prompt)))
+         (cmd (lookup-key project-prefix-map key)))
+    (if (and cmd (or project-switch-use-entire-map
+                     (assq cmd project-switch-commands)))
+        (call-interactively cmd)
+      (user-error "%s is undefined" (key-description key)))))
 
 (provide 'project)
 ;;; project.el ends here
-- 
2.20.1

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Basil L. Contovounesios]

Just some minor nits from me.

> From: Philip K <philip@warpmail.net>
> Date: Thu, 18 Jun 2020 16:06:19 +0200
> Subject: [PATCH] Use same keys in project-switch-project as in
>  project-prefix-map
>
> * project.el (project-switch-commands): Convert to user option and
> change structure.
> (project-switch-use-entire-map): Add new option.
> (project--keymap-prompt): Adapt to change in project-switch-commands

Nit: Missing full stop.

> (project-switch-project): Use project-prefix-map instead of
> project-switch-commands to query valid commands.
> ---
>  lisp/progmodes/project.el | 63 +++++++++++++++++++++++++--------------
>  1 file changed, 41 insertions(+), 22 deletions(-)
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index e24d81c1b4..33946f78a8 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -900,27 +900,46 @@ project-prompt-project-dir
>  ;;; Project switching
>  
>  ;;;###autoload
> -(defvar project-switch-commands
> -  '((?f "Find file" project-find-file)
> -    (?g "Find regexp" project-find-regexp)
> -    (?d "Dired" project-dired)
> -    (?v "VC-Dir" project-vc-dir)
> -    (?e "Eshell" project-eshell))
> -  "Alist mapping keys to project switching menu entries.
> +(defcustom project-switch-commands
> +  '((project-find-file . "Find file")
> +    (project-find-regexp . "Find regexp")
> +    (project-dired . "Dired")
> +    (project-vc-dir . "VC-Dir")
> +    (project-shell . "Shell")
> +    (project-eshell . "Eshell"))
> +  "Alist mapping commands to descriptions.
>  Used by `project-switch-project' to construct a dispatch menu of
>  commands available upon \"switching\" to another project.
>  
> -Each element looks like (KEY LABEL COMMAND), where COMMAND is the
> -command to run when KEY is pressed.  LABEL is used to distinguish
> -the choice in the dispatch menu.")
> +Each element looks like (COMMAND LABEL), where COMMAND should be
                           ^^^^^^^^^^^^^^^
                           (COMMAND . LABEL)

> +bound in `project-prefix-map'.  LABEL is used to distinguish the
> +choice in the dispatch menu."
> +  :type '(alist :key-type function
> +                :value-type string)
> +  :options (mapcan (lambda (ent)
> +                     (and (commandp (cdr ent))
> +                          (list (cdr ent))))
> +                   (cdr project-prefix-map))
> +  :version "28.1")
> +
> +(defcustom project-switch-use-entire-map t
> +  "Make `project-switch-project' use entire `project-prefix-map'.
> +If nil, `project-switch-project' will only recognize commands
> +listed in `project-switch-commands', and signal an error when
> +others are invoked.  Otherwise, all keys in
> +`project-switch-commands', are legal even if they aren't listed
                           ^^^
                           Nit: Unnecessary comma.

> +in the minibuffer."
> +  :type 'bool
> +  :version "28.1")

Thanks,

-- 
Basil

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Philip K.]

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


Thanks, the fixed patch is below.

Another minor change to the last patch is settign
project-switch-use-entire-map to nil, so that by default not all keys in
project-prefix-map are usable, as to avoid the potential for confusion
mentioned before.

-- 
	Philip K.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-same-keys-in-project-switch-project-as-in-projec.patch --]
[-- Type: text/x-diff, Size: 4137 bytes --]

From 8418074a6f73ad5dcafe1fbcbfc847155dc4c485 Mon Sep 17 00:00:00 2001
From: Philip K <philip@warpmail.net>
Date: Thu, 18 Jun 2020 16:06:19 +0200
Subject: [PATCH] Use same keys in project-switch-project as in
 project-prefix-map

* project.el (project-switch-commands): Convert to user option and
change structure.
(project-switch-use-entire-map): Add new option.
(project--keymap-prompt): Adapt to change in project-switch-commands.
(project-switch-project): Use project-prefix-map instead of
project-switch-commands to query valid commands.
---
 lisp/progmodes/project.el | 63 +++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 22 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index e24d81c1b4..cf214719e5 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -900,27 +900,46 @@ project-prompt-project-dir
 ;;; Project switching
 
 ;;;###autoload
-(defvar project-switch-commands
-  '((?f "Find file" project-find-file)
-    (?g "Find regexp" project-find-regexp)
-    (?d "Dired" project-dired)
-    (?v "VC-Dir" project-vc-dir)
-    (?e "Eshell" project-eshell))
-  "Alist mapping keys to project switching menu entries.
+(defcustom project-switch-commands
+  '((project-find-file . "Find file")
+    (project-find-regexp . "Find regexp")
+    (project-dired . "Dired")
+    (project-vc-dir . "VC-Dir")
+    (project-shell . "Shell")
+    (project-eshell . "Eshell"))
+  "Alist mapping commands to descriptions.
 Used by `project-switch-project' to construct a dispatch menu of
 commands available upon \"switching\" to another project.
 
-Each element looks like (KEY LABEL COMMAND), where COMMAND is the
-command to run when KEY is pressed.  LABEL is used to distinguish
-the choice in the dispatch menu.")
+Each element looks like (COMMAND . LABEL), where COMMAND should be
+bound in `project-prefix-map'.  LABEL is used to distinguish the
+choice in the dispatch menu."
+  :type '(alist :key-type function
+                :value-type string)
+  :options (mapcan (lambda (ent)
+                     (and (commandp (cdr ent))
+                          (list (cdr ent))))
+                   (cdr project-prefix-map))
+  :version "28.1")
+
+(defcustom project-switch-use-entire-map nil
+  "Make `project-switch-project' use entire `project-prefix-map'.
+If nil, `project-switch-project' will only recognize commands
+listed in `project-switch-commands', and signal an error when
+others are invoked.  Otherwise, all keys in
+`project-switch-commands' are legal even if they aren't listed
+in the minibuffer."
+  :type 'bool
+  :version "28.1")
 
 (defun project--keymap-prompt ()
   "Return a prompt for the project swithing dispatch menu."
   (mapconcat
-   (pcase-lambda (`(,key ,label))
-     (format "[%s] %s"
-             (propertize (key-description `(,key)) 'face 'bold)
-             label))
+   (pcase-lambda (`(,cmd . ,label))
+     (let ((key (where-is-internal cmd project-prefix-map t)))
+       (format "[%s] %s"
+               (propertize (key-description key) 'face 'bold)
+               label)))
    project-switch-commands
    "  "))
 
@@ -930,14 +949,14 @@ project-switch-project
 The available commands are picked from `project-switch-commands'
 and presented in a dispatch menu."
   (interactive)
-  (let ((dir (project-prompt-project-dir))
-        (choice nil))
-    (while (not choice)
-      (setq choice (assq (read-event (project--keymap-prompt))
-                         project-switch-commands)))
-    (let ((default-directory dir)
-          (project-current-inhibit-prompt t))
-      (call-interactively (nth 2 choice)))))
+  (let* ((default-directory (project-prompt-project-dir))
+         (project-current-inhibit-prompt t)
+         (key (read-key-sequence-vector (project--keymap-prompt)))
+         (cmd (lookup-key project-prefix-map key)))
+    (if (and cmd (or project-switch-use-entire-map
+                     (assq cmd project-switch-commands)))
+        (call-interactively cmd)
+      (user-error "%s is undefined" (key-description key)))))
 
 (provide 'project)
 ;;; project.el ends here
-- 
2.20.1

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

> Another minor change to the last patch is settign
> project-switch-use-entire-map to nil, so that by default not all keys in
> project-prefix-map are usable, as to avoid the potential for confusion
> mentioned before.

I realized there is a question: why we need to bind project-switch-project
to 'C-x p p'?

For example, why we need to duplicate such key sequences:

  'C-x p d' - project dired without dispatch prompt
  'C-x p p d' - the same with dispatch prompt

Couldn't 'C-x p' display the same dispatch prompt (that actually is
just an echo-area hint) after a short delay?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 19.06.2020 01:10, Juri Linkov wrote:
>    'C-x p d' - project dired without dispatch prompt
>    'C-x p p d' - the same with dispatch prompt

It's not the same. It asks you to select a _different_ project.

> Couldn't 'C-x p' display the same dispatch prompt (that actually is
> just an echo-area hint) after a short delay?

Sounds like which-key-mode. But it would dispatch on commands for the 
current project. Not a different one.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>>    'C-x p d' - project dired without dispatch prompt
>>    'C-x p p d' - the same with dispatch prompt
>
> It's not the same. It asks you to select a _different_ project.

Oh, I forgot it's about switching projects.
I'm not sure how frequently this key will be used.
If not often, then better to use easier-to-type 'C-x p p'
for other more frequent commands, such as using it as
a prefix for any command to run in the project root dir,
for example, 'C-x p p M-! git pull' will run git commands
in the project root dir.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 19.06.2020 02:24, Juri Linkov wrote:
> I'm not sure how frequently this key will be used.

I think it's rather handy. Some actual usage data would be good, but 
we're unlikely to get that with any accuracy.

> If not often, then better to use easier-to-type 'C-x p p'
> for other more frequent commands, such as using it as
> a prefix for any command to run in the project root dir,
> for example, 'C-x p p M-! git pull' will run git commands
> in the project root dir.

If you like to implement such a prefix command, please go ahead.

Either way, 'C-x p P' is free.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Simen Heggestøyl]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 19.06.2020 02:24, Juri Linkov wrote:
>> I'm not sure how frequently this key will be used.
>
> I think it's rather handy. Some actual usage data would be good, but
> we're unlikely to get that with any accuracy.

Another data point: I use it very often.

-- Simen

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello,

On Thu 18 Jun 2020 at 08:50PM +02, Philip K. wrote:

> From: Philip K <philip@warpmail.net>
> Date: Thu, 18 Jun 2020 16:06:19 +0200
> Subject: [PATCH] Use same keys in project-switch-project as in
>  project-prefix-map
>
> * project.el (project-switch-commands): Convert to user option and
> change structure.
> (project-switch-use-entire-map): Add new option.
> (project--keymap-prompt): Adapt to change in project-switch-commands.
> (project-switch-project): Use project-prefix-map instead of
> project-switch-commands to query valid commands.
> ---
>  lisp/progmodes/project.el | 63 +++++++++++++++++++++++++--------------
>  1 file changed, 41 insertions(+), 22 deletions(-)

I hope no-one will object if I ping to ask about the status of this
patch -- is it likely to get applied soon?

Over in #42210 I'm looking to add a project-other-place-commands
defcustom following the lead of this patch, and I'll need to generalise
project--keymap-prompt a bit, so I'd prefer to see this patch committed
before working on that, if that's possible.

-- 
Sean Whitton

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

Hi!

On 11.07.2020 20:07, Sean Whitton wrote:
> I hope no-one will object if I ping to ask about the status of this
> patch -- is it likely to get applied soon?

As the most recent message to this report probably indicated, I'm on the 
fence.

> Over in #42210 I'm looking to add a project-other-place-commands
> defcustom following the lead of this patch, and I'll need to generalise
> project--keymap-prompt a bit, so I'd prefer to see this patch committed
> before working on that, if that's possible.

You can post the patch you are working on, even if it depends on this one.

But why project--keymap-prompt, though? I thought you were going to do 
something that would look like a "normal" prefix keymap. They don't prompt.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello Dmitry,

Thank you for your reply.

On Sun 12 Jul 2020 at 06:18PM +03, Dmitry Gutov wrote:

> You can post the patch you are working on, even if it depends on this one.
>
> But why project--keymap-prompt, though? I thought you were going to do
> something that would look like a "normal" prefix keymap. They don't prompt.

Over in the other bug I received feedback from Juri which I understood
as saying that only a patch using the prompting approach would be likely
to be merged, so I've been working on that.  Sounds like things are more
undecided than I thought?

I myself am mostly neutral between the standard prefix and prompting
approaches.  Since I'm waiting on assign@gnu.org anyway, maybe it would
be best to wait a bit longer on this discussion.

-- 
Sean Whitton

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 12.07.2020 19:24, Sean Whitton wrote:
> Hello Dmitry,
> 
> Thank you for your reply.
> 
> On Sun 12 Jul 2020 at 06:18PM +03, Dmitry Gutov wrote:
> 
>> You can post the patch you are working on, even if it depends on this one.
>>
>> But why project--keymap-prompt, though? I thought you were going to do
>> something that would look like a "normal" prefix keymap. They don't prompt.
> 
> Over in the other bug I received feedback from Juri which I understood
> as saying that only a patch using the prompting approach would be likely
> to be merged, so I've been working on that.  Sounds like things are more
> undecided than I thought?

There's no decision indeed. I'd like to know what people think, and if 
there's no strong opinion, how the proposal would look in practice.

So unless you're strapped for time, a prototype patch would help.

But we can of course first ask Juri what UI did he mean exactly in his 
feedback.

Juri, could you clarify?

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

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

Hello Dmitry and others,

On Sun 12 Jul 2020 at 11:12PM +03, Dmitry Gutov wrote:

> There's no decision indeed. I'd like to know what people think, and if
> there's no strong opinion, how the proposal would look in practice.
>
> So unless you're strapped for time, a prototype patch would help.

Okay, here's a prototype.  I had to rebase Philip's patch so I thought I
might as well attach my version of that too.

I have been wanting the new C-x 4 p bindings all week as C-x p f with
fido-mode has replaced a lot of my use of C-x C-f, so I hope I can
replace my use of C-x 4 f with C-x 4 p f soon :)

-- 
Sean Whitton


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Use-same-keys-in-project-switch-project-as-in-projec.patch --]
[-- Type: text/x-diff, Size: 4144 bytes --]

From c9f20a5a547631ab12cd3cf8da2ca51b71cfb28e Mon Sep 17 00:00:00 2001
From: Philip K <philip@warpmail.net>
Date: Thu, 18 Jun 2020 16:06:19 +0200
Subject: [PATCH 1/2] Use same keys in project-switch-project as in
 project-prefix-map

* project.el (project-switch-commands): Convert to user option and
change structure.
(project-switch-use-entire-map): Add new option.
(project--keymap-prompt): Adapt to change in project-switch-commands
(project-switch-project): Use project-prefix-map instead of
project-switch-commands to query valid commands.
---
 lisp/progmodes/project.el | 63 +++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 22 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 67ce3dc7d9..4f0233c8b7 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -974,27 +974,46 @@ project-known-project-roots
 ;;; Project switching
 
 ;;;###autoload
-(defvar project-switch-commands
-  '((?f "Find file" project-find-file)
-    (?g "Find regexp" project-find-regexp)
-    (?d "Dired" project-dired)
-    (?v "VC-Dir" project-vc-dir)
-    (?e "Eshell" project-eshell))
-  "Alist mapping keys to project switching menu entries.
+(defcustom project-switch-commands
+  '((project-find-file . "Find file")
+    (project-find-regexp . "Find regexp")
+    (project-dired . "Dired")
+    (project-vc-dir . "VC-Dir")
+    (project-shell . "Shell")
+    (project-eshell . "Eshell"))
+  "Alist mapping commands to descriptions.
 Used by `project-switch-project' to construct a dispatch menu of
 commands available upon \"switching\" to another project.
 
-Each element is of the form (KEY LABEL COMMAND), where COMMAND is the
-command to run when KEY is pressed.  LABEL is used to distinguish
-the menu entries in the dispatch menu.")
+Each element looks like (COMMAND LABEL), where COMMAND should be
+bound in `project-prefix-map'.  LABEL is used to distinguish the
+choice in the dispatch menu."
+  :type '(alist :key-type function
+                :value-type string)
+  :options (mapcan (lambda (ent)
+                     (and (commandp (cdr ent))
+                          (list (cdr ent))))
+                   (cdr project-prefix-map))
+  :version "28.1")
+
+(defcustom project-switch-use-entire-map t
+  "Make `project-switch-project' use entire `project-prefix-map'.
+If nil, `project-switch-project' will only recognize commands
+listed in `project-switch-commands', and signal an error when
+others are invoked.  Otherwise, all keys in
+`project-switch-commands', are legal even if they aren't listed
+in the minibuffer."
+  :type 'bool
+  :version "28.1")
 
 (defun project--keymap-prompt ()
   "Return a prompt for the project swithing dispatch menu."
   (mapconcat
-   (pcase-lambda (`(,key ,label))
-     (format "[%s] %s"
-             (propertize (key-description `(,key)) 'face 'bold)
-             label))
+   (pcase-lambda (`(,cmd . ,label))
+     (let ((key (where-is-internal cmd project-prefix-map t)))
+       (format "[%s] %s"
+               (propertize (key-description key) 'face 'bold)
+               label)))
    project-switch-commands
    "  "))
 
@@ -1004,14 +1023,14 @@ project-switch-project
 The available commands are presented as a dispatch menu
 made from `project-switch-commands'."
   (interactive)
-  (let ((dir (project-prompt-project-dir))
-        (choice nil))
-    (while (not choice)
-      (setq choice (assq (read-event (project--keymap-prompt))
-                         project-switch-commands)))
-    (let ((default-directory dir)
-          (project-current-inhibit-prompt t))
-      (call-interactively (nth 2 choice)))))
+  (let* ((default-directory (project-prompt-project-dir))
+         (project-current-inhibit-prompt t)
+         (key (read-key-sequence-vector (project--keymap-prompt)))
+         (cmd (lookup-key project-prefix-map key)))
+    (if (and cmd (or project-switch-use-entire-map
+                     (assq cmd project-switch-commands)))
+        (call-interactively cmd)
+      (user-error "%s is undefined" (key-description key)))))
 
 (provide 'project)
 ;;; project.el ends here
-- 
2.27.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-WIP-Add-project-other-place-commands-and-functions-w.patch --]
[-- Type: text/x-diff, Size: 3920 bytes --]

From 37bc752289bd101ceb725446b425944630971f61 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Sat, 18 Jul 2020 08:59:19 -0700
Subject: [PATCH 2/2] WIP: Add project-other-place-commands and functions which
 use it

---
 lisp/progmodes/project.el | 51 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 4f0233c8b7..f67698ac96 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -985,6 +985,36 @@ project-switch-commands
 Used by `project-switch-project' to construct a dispatch menu of
 commands available upon \"switching\" to another project.
 
+Each element looks like (COMMAND LABEL), where COMMAND should be
+bound in `project-prefix-map'.  LABEL is used to distinguish the
+choice in the dispatch menu."
+  :type '(alist :key-type function
+                :value-type string)
+  :options (mapcan (lambda (ent)
+                     (and (commandp (cdr ent))
+                          (list (cdr ent))))
+                   (cdr project-prefix-map))
+  :version "28.1")
+
+;; "other-place" because non-prototype patch will also add an entry
+;; in ctl-x-5-map and under C-x t p
+(defcustom project-other-place-commands
+  '((project-find-file . "Find file")
+    (project-switch-to-buffer . "Switch to buffer")
+    (project-dired . "Dired")
+    ;; Eshell uses the current window by default, but Shell defaults
+    ;; to using the other window.  If a user has added an entry to
+    ;; `display-buffer-alist' for Shell, they probably want to add an
+    ;; entry here, too
+    (project-eshell . "Eshell")
+    (project-switch-project . "Switch project"))
+    "Alist mapping commands to descriptions.
+Used by `project-other-window-command' to construct a dispatch menu of
+commands available to be displayed in another window.
+
+Commands in this list should be ones which normally display their
+buffer in the current window.
+
 Each element looks like (COMMAND LABEL), where COMMAND should be
 bound in `project-prefix-map'.  LABEL is used to distinguish the
 choice in the dispatch menu."
@@ -1006,7 +1036,7 @@ project-switch-use-entire-map
   :type 'bool
   :version "28.1")
 
-(defun project--keymap-prompt ()
+(defun project--keymap-prompt (cmds)
   "Return a prompt for the project swithing dispatch menu."
   (mapconcat
    (pcase-lambda (`(,cmd . ,label))
@@ -1014,7 +1044,7 @@ project--keymap-prompt
        (format "[%s] %s"
                (propertize (key-description key) 'face 'bold)
                label)))
-   project-switch-commands
+   cmds
    "  "))
 
 ;;;###autoload
@@ -1025,12 +1055,27 @@ project-switch-project
   (interactive)
   (let* ((default-directory (project-prompt-project-dir))
          (project-current-inhibit-prompt t)
-         (key (read-key-sequence-vector (project--keymap-prompt)))
+         (key (read-key-sequence-vector
+               (project--keymap-prompt project-switch-commands)))
          (cmd (lookup-key project-prefix-map key)))
     (if (and cmd (or project-switch-use-entire-map
                      (assq cmd project-switch-commands)))
         (call-interactively cmd)
       (user-error "%s is undefined" (key-description key)))))
 
+(defun project-other-window-command ()
+  (interactive)
+  (let* ((key (read-key-sequence-vector
+               (project--keymap-prompt project-other-place-commands)))
+         (cmd (lookup-key project-prefix-map key)))
+    (if (and cmd (assq cmd project-other-place-commands))
+        (let ((display-buffer-overriding-action
+               '((display-buffer-pop-up-window)
+		 (inhibit-same-window . t))))
+          (call-interactively cmd))
+      (user-error "%s is undefined" (key-description key)))))
+
+;;;###autoload (define-key ctl-x-4-map "p" 'project-other-window-command)
+
 (provide 'project)
 ;;; project.el ends here
-- 
2.27.0

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 18.07.2020 19:06, Sean Whitton wrote:
> Okay, here's a prototype.  I had to rebase Philip's patch so I thought I
> might as well attach my version of that too.

The code LGTM, but I still wonder whether we do need the prompt in this 
case. The downsides are that we'll need to keep the list up-to-date, and 
at some point (maybe) it will grow too big to fit in the prompt. Will we 
need a variable project-other-place-use-entire-map too?

Juri, did you have this particular approach in mind, or something different?

> I have been wanting the new C-x 4 p bindings all week as C-x p f with
> fido-mode has replaced a lot of my use of C-x C-f, so I hope I can
> replace my use of C-x 4 f with C-x 4 p f soon:)

I'm happy to hear that.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> Okay, here's a prototype.  I had to rebase Philip's patch so I thought I
>> might as well attach my version of that too.
>
> The code LGTM, but I still wonder whether we do need the prompt in this
> case. The downsides are that we'll need to keep the list up-to-date, and 
> at some point (maybe) it will grow too big to fit in the prompt. Will we
> need a variable project-other-place-use-entire-map too?
>
> Juri, did you have this particular approach in mind, or something different?

Indeed, this approach.  I think it's a step forward.  Actually two steps forward.
Both patches improve this part of project functionality greatly.

Regarding project-other-place-use-entire-map, I don't know, first need
to try using new commands for some time to see what's missing.

Currently to open a new project in a new tab I have to type:
‘C-x t t C-x p p’ then a project dir and some key e.g. ‘v’.
When Sean will turn the prototype into the final patch,
then the key sequence will be much shorter:
‘C-x t p v’.

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 20.07.2020 03:30, Juri Linkov wrote:
> Indeed, this approach.  I think it's a step forward.  Actually two steps forward.
> Both patches improve this part of project functionality greatly.
> 
> Regarding project-other-place-use-entire-map, I don't know, first need
> to try using new commands for some time to see what's missing.

OK then, here's a question:

Should project-other-place-commands include an entry for 
project-or-external-find-file?

> Currently to open a new project in a new tab I have to type:
> ‘C-x t t C-x p p’ then a project dir and some key e.g. ‘v’.
> When Sean will turn the prototype into the final patch,
> then the key sequence will be much shorter:
> ‘C-x t p v’.

Sounds neat.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

> OK then, here's a question:
>
> Should project-other-place-commands include an entry for
> project-or-external-find-file?

I don't see why someone might want to use such commands as
project-or-external-find-file or project-or-external-find-regexp.
These commands operate on some obscure directories
(copied from the output of '(project-external-roots (project-current t))'):

"/usr/local/share/emacs/site-lisp/"
"/usr/share/emacs/site-lisp/autoconf/"
"/usr/share/emacs/site-lisp/gtk-doc-tools/"
...

I don't know why the user might want to find files or search in such
non-project directories as /usr/share/emacs/site-lisp/gtk-doc-tools/.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 20.07.2020 23:47, Juri Linkov wrote:
> I don't know why the user might want to find files or search in such
> non-project directories as/usr/share/emacs/site-lisp/gtk-doc-tools/

Apparently these directories are in your load-path.

I think it makes sense to search for files across your load path sometimes.

Or if you have multiple tag files loaded, it would search across 
directories containing those files.

And that's just with the built-in backend. Other projects can have other 
examples of what constitutes external roots.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello,

On Mon 20 Jul 2020 at 02:46AM +03, Dmitry Gutov wrote:

> On 18.07.2020 19:06, Sean Whitton wrote:
>> Okay, here's a prototype.  I had to rebase Philip's patch so I thought I
>> might as well attach my version of that too.
>
> The code LGTM, but I still wonder whether we do need the prompt in this
> case. The downsides are that we'll need to keep the list up-to-date, and
> at some point (maybe) it will grow too big to fit in the prompt. Will we
> need a variable project-other-place-use-entire-map too?

If all one has in mind is C-x 4, then it doesn't seem like this is a big
danger -- many commands display in the other window by default, such as
project-find-regexp, and it reasonable to guess that a good proportion
of new commands which get added will also be like this.

However, if you have C-x 5 and C-x t in mind as well, then it starts to
seem like we'll want project-other-place-use-entire-map.

How about having a project-other-window-commands defcustom for C-x 4 p,
and using the entirety of project-prefix-map for C-x 5 p and C-x t t?
C-x 4 p can prompt as per my patch, and C-x 5 p and C-x t t could just
put a static message in the minibuffer like other-frame-prefix and
other-tab-prefix do at present.

>> I have been wanting the new C-x 4 p bindings all week as C-x p f with
>> fido-mode has replaced a lot of my use of C-x C-f, so I hope I can
>> replace my use of C-x 4 f with C-x 4 p f soon:)
>
> I'm happy to hear that.

I intended to say a bit more: C-x p f has replaced a lot of my use of
C-x b too.  It is very nice not to have to guess whether something is
already visited and just complete across all the project's files.  I use
C-x 4 b a lot so I'm looking forward to C-x 4 p f.

-- 
Sean Whitton

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

> How about having a project-other-window-commands defcustom for C-x 4 p,
> and using the entirety of project-prefix-map for C-x 5 p and C-x t t?
> C-x 4 p can prompt as per my patch, and C-x 5 p and C-x t t could just
> put a static message in the minibuffer like other-frame-prefix and
> other-tab-prefix do at present.

Currently there are separate key sequences ‘C-x p’ and ‘C-x p p’
followed by almost the same list of possibles suffixes, where e.g.
‘C-x p f’ visits a file of the current project, whereas ‘C-x p p f’
prompts for another project and visits its file.

Should the same distinction be preserved in other-place commands?
Then ‘C-x 4 p f’ will visit a file of the current project in another
window, whereas ‘C-x 4 p p f’ will prompt for another project and visit
its file in another window.  The same distinction could apply also to
‘C-x 5 p f’ / ‘C-x 5 p p f’ and ‘C-x t p f’ / ‘C-x t p p f’.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 20.07.2020 23:41, Juri Linkov wrote:
> Should the same distinction be preserved in other-place commands?
> Then ‘C-x 4 p f’ will visit a file of the current project in another
> window, whereas ‘C-x 4 p p f’ will prompt for another project and visit
> its file in another window.

Sure.

Doesn't this work with the latest proposed patch?

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello Juri,

On Mon 20 Jul 2020 at 11:41PM +03, Juri Linkov wrote:

>> How about having a project-other-window-commands defcustom for C-x 4 p,
>> and using the entirety of project-prefix-map for C-x 5 p and C-x t t?
>> C-x 4 p can prompt as per my patch, and C-x 5 p and C-x t t could just
>> put a static message in the minibuffer like other-frame-prefix and
>> other-tab-prefix do at present.
>
> Currently there are separate key sequences ‘C-x p’ and ‘C-x p p’
> followed by almost the same list of possibles suffixes, where e.g.
> ‘C-x p f’ visits a file of the current project, whereas ‘C-x p p f’
> prompts for another project and visits its file.
>
> Should the same distinction be preserved in other-place commands?
> Then ‘C-x 4 p f’ will visit a file of the current project in another
> window, whereas ‘C-x 4 p p f’ will prompt for another project and visit
> its file in another window.  The same distinction could apply also to
> ‘C-x 5 p f’ / ‘C-x 5 p p f’ and ‘C-x t p f’ / ‘C-x t p p f’.

Yes, I think so, but I'm not sure how that directly addresses the
suggestion of mine you quote.

-- 
Sean Whitton

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 20.07.2020 19:49, Sean Whitton wrote:

> How about having a project-other-window-commands defcustom for C-x 4 p,
> and using the entirety of project-prefix-map for C-x 5 p and C-x t t?
> C-x 4 p can prompt as per my patch, and C-x 5 p and C-x t t could just
> put a static message in the minibuffer like other-frame-prefix and
> other-tab-prefix do at present.

Just to clarify: are you proposing this because you really like how the 
prompt works, yet can't find a good way to incorporate it for the two 
other prefixes?

> I intended to say a bit more: C-x p f has replaced a lot of my use of
> C-x b too.  It is very nice not to have to guess whether something is
> already visited and just complete across all the project's files.  I use
> C-x 4 b a lot so I'm looking forward to C-x 4 p f.

Right! That mirrors my experience: it's often handy to just search 
across the project files because you're guaranteed to find the needed 
file, and because there is no ambiguity caused by having only the base 
name available.

One can also "guess" the eventual file name based on the partial matches 
in its name and in its parent directories' names.

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello,

On Tue 21 Jul 2020 at 01:00AM +03, Dmitry Gutov wrote:

> On 20.07.2020 19:49, Sean Whitton wrote:
>
>> How about having a project-other-window-commands defcustom for C-x 4 p,
>> and using the entirety of project-prefix-map for C-x 5 p and C-x t t?
>> C-x 4 p can prompt as per my patch, and C-x 5 p and C-x t t could just
>> put a static message in the minibuffer like other-frame-prefix and
>> other-tab-prefix do at present.
>
> Just to clarify: are you proposing this because you really like how the
> prompt works, yet can't find a good way to incorporate it for the two
> other prefixes?

I wouldn't say that I really like the prompt, but it could be useful to
someone to see the bindings available to them, when we're sure it's
going to fit.

I do think we should avoid binding commands under C-x 4 where the
versions under C-x p would already display in another window -- I think
it is potentially quite confusing to have bindings with identical
behaviour under both C-x 4 p and C-x p.

But that means we need the defcustom, because a user could use
display-buffer-alist to change which commands under C-x p will use
another window.

-- 
Sean Whitton

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>>> How about having a project-other-window-commands defcustom for C-x 4 p,
>>> and using the entirety of project-prefix-map for C-x 5 p and C-x t t?
>>> C-x 4 p can prompt as per my patch, and C-x 5 p and C-x t t could just
>>> put a static message in the minibuffer like other-frame-prefix and
>>> other-tab-prefix do at present.
>>
>> Just to clarify: are you proposing this because you really like how the
>> prompt works, yet can't find a good way to incorporate it for the two
>> other prefixes?
>
> I wouldn't say that I really like the prompt, but it could be useful to
> someone to see the bindings available to them, when we're sure it's
> going to fit.

While the prompt is active, the key '?' and 'C-h' could (and I think should)
display a list of *ALL* available key bindings from the project keymap
in the *Help* buffer (like e.g. 'query-replace' does after typing 'C-h').

> I do think we should avoid binding commands under C-x 4 where the
> versions under C-x p would already display in another window -- I think
> it is potentially quite confusing to have bindings with identical
> behaviour under both C-x 4 p and C-x p.

Shouldn't some key sequence force displaying the project buffer in the
same window (when a version under C-x p displays it in another window)?

> But that means we need the defcustom, because a user could use
> display-buffer-alist to change which commands under C-x p will use
> another window.

I now understand that your top message above implies that a user
could customize display-buffer-alist to display a buffer in another window,
but usually users don't customize display-buffer-alist to display a buffer
in another frame/tab?

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 22.07.2020 02:38, Juri Linkov wrote:

>>> Just to clarify: are you proposing this because you really like how the
>>> prompt works, yet can't find a good way to incorporate it for the two
>>> other prefixes?
>>
>> I wouldn't say that I really like the prompt, but it could be useful to
>> someone to see the bindings available to them, when we're sure it's
>> going to fit.

That's a valid benefit.

There is also a package called 'which-key', but I think it's 
incompatible with the implementation strategy here.

> While the prompt is active, the key '?' and 'C-h' could (and I think should)
> display a list of *ALL* available key bindings from the project keymap
> in the *Help* buffer (like e.g. 'query-replace' does after typing 'C-h').

If we're using the prompt, it shows all allowed bindings already.

> Shouldn't some key sequence force displaying the project buffer in the
> same window (when a version under C-x p displays it in another window)?

At the moment, there's none.

>> But that means we need the defcustom, because a user could use
>> display-buffer-alist to change which commands under C-x p will use
>> another window.
> 
> I now understand that your top message above implies that a user
> could customize display-buffer-alist to display a buffer in another window,
> but usually users don't customize display-buffer-alist to display a buffer
> in another frame/tab?

Either way, I think the concern that someone could type 'C-x p 4 g' and 
see that the search results are *still* displayed in another window 
(gasp), is not too significant.

Like, there's no other behavior that should result from key sequence, 
and if someone wanted to make doubly sure the resulting buffer will be 
displayed in the other window, why not let them.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello,

On Wed 22 Jul 2020 at 03:38AM +03, Dmitry Gutov wrote:

> Either way, I think the concern that someone could type 'C-x p 4 g' and
> see that the search results are *still* displayed in another window
> (gasp), is not too significant.

I think it could be a barrier to learning how to use project.el -- it's
weird for the same thing to be bound to two places, so the user might
think they've misunderstood something when they see it happen.

> Like, there's no other behavior that should result from key sequence,
> and if someone wanted to make doubly sure the resulting buffer will be
> displayed in the other window, why not let them.

This makes sense, and would be useful, so probably overrides my worry
about it being harder to learn.  So, shall I prepare a new patch which
just uses the whole prefix map and drops the defcustom?

-- 
Sean Whitton

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello,

On Tue 21 Jul 2020 at 06:33PM -07, Sean Whitton wrote:

> On Wed 22 Jul 2020 at 03:38AM +03, Dmitry Gutov wrote:
>
>> Like, there's no other behavior that should result from key sequence,
>> and if someone wanted to make doubly sure the resulting buffer will be
>> displayed in the other window, why not let them.
>
> This makes sense, and would be useful, so probably overrides my worry
> about it being harder to learn.  So, shall I prepare a new patch which
> just uses the whole prefix map and drops the defcustom?

Hmm, a further complication: I would like to be able to bind C-x 4 p C-o
to work like C-x 4 C-o.

I think this could be achieved by having an additional keymap, and the
command bound to C-x 4 p can look up keys in both project-prefix-map and
this new keymap.

-- 
Sean Whitton

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 22.07.2020 22:28, Sean Whitton wrote:
>> This makes sense, and would be useful, so probably overrides my worry
>> about it being harder to learn.  So, shall I prepare a new patch which
>> just uses the whole prefix map and drops the defcustom?

Since we're struggling between the choices, and you don't feel a strong 
attachment to the prompt either, yes, please.

Then we can install it and maybe continue the discussion of an optional 
patch which will add the prompt on top of it.

> Hmm, a further complication: I would like to be able to bind C-x 4 p C-o
> to work like C-x 4 C-o.
> 
> I think this could be achieved by having an additional keymap, and the
> command bound to C-x 4 p can look up keys in both project-prefix-map and
> this new keymap.

Yup. The new keymap could inherit from project-prefix-map, then the 
lookup should be just one funcall.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

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

Hello,

On Fri 24 Jul 2020 at 02:46AM +03, Dmitry Gutov wrote:

> On 22.07.2020 22:28, Sean Whitton wrote:
>>> [...]
>
> Since we're struggling between the choices, and you don't feel a strong
> attachment to the prompt either, yes, please.
>
> Then we can install it and maybe continue the discussion of an optional
> patch which will add the prompt on top of it.

Cool, what do you think to the attached?

>> Hmm, a further complication: I would like to be able to bind C-x 4 p C-o
>> to work like C-x 4 C-o.
>>
>> I think this could be achieved by having an additional keymap, and the
>> command bound to C-x 4 p can look up keys in both project-prefix-map and
>> this new keymap.
>
> Yup. The new keymap could inherit from project-prefix-map, then the
> lookup should be just one funcall.

In the end I didn't use inheritance; hopefully it is clear from the
patch why I thought this would not be a good idea.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-project-display-buffer-and-project-display-buffe.patch --]
[-- Type: text/x-diff, Size: 2718 bytes --]

From 5a55c6376a5ccec957adee88e03503f651ee7448 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Thu, 23 Jul 2020 18:00:59 -0700
Subject: [PATCH 1/2] Add project-display-buffer and
 project-display-buffer-other-frame

* lisp/progmodes/project.el (project-switch-to-buffer): Add optional
switching-function argument.
* lisp/progmodes/project.el (project-display-buffer,
project-display-buffer-other-frame): Add commands.
---
 lisp/progmodes/project.el | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index a0930553bd..f47e6bcc1c 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -878,12 +878,15 @@ project-compile
     (compile command comint)))
 
 ;;;###autoload
-(defun project-switch-to-buffer ()
+(defun project-switch-to-buffer (&optional switching-function)
   "Switch to another buffer belonging to the current project.
 This function prompts for another buffer, offering as candidates
 buffers that belong to the same project as the current buffer.
 Two buffers belong to the same project if their project instances,
-as reported by `project-current' in each buffer, are identical."
+as reported by `project-current' in each buffer, are identical.
+
+Optional argument SWITCHING-FUNCTION is the function used to
+switch the buffer.  It defaults to `switch-to-buffer'."
   (interactive)
   (let* ((pr (project-current t))
          (current-buffer (current-buffer))
@@ -896,7 +899,7 @@ project-switch-to-buffer
                  (equal pr
                         (with-current-buffer (cdr buffer)
                           (project-current)))))))
-    (switch-to-buffer
+    (funcall (or switching-function #'switch-to-buffer)
      (read-buffer
       "Switch to buffer: "
       (when (funcall predicate (cons other-name other-buffer))
@@ -904,6 +907,24 @@ project-switch-to-buffer
       nil
       predicate))))
 
+;;;###autoload
+(defun project-display-buffer ()
+  "Display a buffer of the current project without selecting it.
+
+See `project-switch-to-buffer' for what it means for a buffer to
+belong to the current project."
+  (interactive)
+  (project-switch-to-buffer #'display-buffer))
+
+;;;###autoload
+(defun project-display-buffer-other-frame ()
+  "Display a buffer of the current project, preferably in another frame.
+
+See `project-switch-to-buffer' for what it means for a buffer to
+belong to the current project."
+  (interactive)
+  (project-switch-to-buffer #'display-buffer-other-frame))
+
 (defcustom project-kill-buffers-ignores
   '("\\*Help\\*")
   "Conditions for buffers `project-kill-buffers' should not kill.
-- 
2.27.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Add-project-other-place-commands.patch --]
[-- Type: text/x-diff, Size: 3522 bytes --]

From c303620b4a9c04771fe9188b500fc5a8e88c8ea6 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Thu, 23 Jul 2020 18:55:42 -0700
Subject: [PATCH 2/2] Add project other place commands

* lisp/progmodes/project.el (project-other-window-map,
project-other-frame-map, project--other-place-command,
project-other-window-command, project-other-frame-command,
project-other-tab-command): Add these functions and maps.
* lisp/progmodes/project.el: Bind project-other-window-command to C-x
4 p, project-other-frame-command to C-x 5 p and
project-other-tab-command to C-x t p.
---
 lisp/progmodes/project.el | 67 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index f47e6bcc1c..13b1eafe3e 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -592,6 +592,73 @@ project-prefix-map
 
 ;;;###autoload (define-key ctl-x-map "p" project-prefix-map)
 
+;; We can't have these place-specific maps inherit from
+;; project-prefix-map because project--other-place-command needs to
+;; know which map the key binding came from, as if it came from one of
+;; these maps, we don't want to set display-buffer-overriding-action
+
+(defvar project-other-window-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-o" #'project-display-buffer)
+    map)
+  "Keymap for additional project commands to display in other windows.")
+
+(defvar project-other-frame-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-o" #'project-display-buffer-other-frame)
+    map)
+  "Keymap for additional project commands to display in other frames.")
+
+(defun project--other-place-command (action &optional map)
+  (let* ((key (read-key-sequence-vector nil t))
+         (place-cmd (lookup-key map key))
+         (generic-cmd (lookup-key project-prefix-map key))
+         (display-buffer-overriding-action (unless place-cmd action)))
+    (if-let ((cmd (or place-cmd generic-cmd)))
+        (call-interactively cmd)
+      (user-error "%s is undefined" (key-description key)))))
+
+;;;###autoload
+(defun project-other-window-command ()
+  "Invoke a project command but display its buffer in another window.
+
+The following commands are available:
+
+\\{project-prefix-map}
+\\{project-other-window-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-pop-up-window)
+                                  (inhibit-same-window . t))
+                                project-other-window-map))
+
+;;;###autoload (define-key ctl-x-4-map "p" #'project-other-window-command)
+
+;;;###autoload
+(defun project-other-frame-command ()
+  "Invoke a project command but display its buffer in another frame.
+
+The following commands are available:
+
+\\{project-prefix-map}
+\\{project-other-frame-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-pop-up-frame))
+                                project-other-frame-map))
+
+;;;###autoload (define-key ctl-x-5-map "p" #'project-other-frame-command)
+
+;;;###autoload
+(defun project-other-tab-command ()
+  "Invoke a project command but display its buffer in another tab.
+
+The following commands are available:
+
+\\{project-prefix-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-in-new-tab))))
+
+;;;###autoload (define-key tab-prefix-map "p" #'project-other-tab-command)
+
 (defun project--value-in-dir (var dir)
   (with-temp-buffer
     (setq default-directory dir)
-- 
2.27.0

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> Date: Thu, 23 Jul 2020 19:04:13 -0700
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,
>  "Philip K." <philip@warpmail.net>, 41890@debbugs.gnu.org,
>  42210@debbugs.gnu.org
> 
> -(defun project-switch-to-buffer ()
> +(defun project-switch-to-buffer (&optional switching-function)
>    "Switch to another buffer belonging to the current project.
>  This function prompts for another buffer, offering as candidates
>  buffers that belong to the same project as the current buffer.
>  Two buffers belong to the same project if their project instances,
> -as reported by `project-current' in each buffer, are identical."
> +as reported by `project-current' in each buffer, are identical.
> +
> +Optional argument SWITCHING-FUNCTION is the function used to
> +switch the buffer.  It defaults to `switch-to-buffer'."
>    (interactive)

This interface strikes me as unusual and even unexpected for a command
that switches to another buffer.  I would expect it to have an API
similar to that of switch-to-buffer: that it should accept the buffer
to switch to as an argument, and set up that argument in the
'interactive' spec according to the preferences of this command
(offering buffers in the same project etc.).  The API you propose
makes it awkward, to say the least, to invoke this command from Lisp.
Granted, the original API doesn't allow such invocation, either, but
as long as we are changing this API, let's try fixing that, okay?

> +(defun project-display-buffer ()
> +  "Display a buffer of the current project without selecting it.

This doesn't say where that buffer will be displayed.  Please add that
important detail to the doc string.

> +(defun project-display-buffer-other-frame ()
> +  "Display a buffer of the current project, preferably in another frame.
> +
> +See `project-switch-to-buffer' for what it means for a buffer to
> +belong to the current project."

The "preferably" part needs to be explained some more, perhaps by
pointing to display-buffer-other-frame for the details.  Otherwise it
leaves some of the command's MO a mystery.

> +(defvar project-other-window-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map "\C-o" #'project-display-buffer)
> +    map)
> +  "Keymap for additional project commands to display in other windows.")

Do you mean "commands _that_ display in other windows"?  If so, please
use "that" instead of "to".  Also, I think the doc string should
explain what do those commands display in those other windows.

> +(defvar project-other-frame-map
> +  (let ((map (make-sparse-keymap)))
> +    (define-key map "\C-o" #'project-display-buffer-other-frame)
> +    map)
> +  "Keymap for additional project commands to display in other frames.")

Same here.

> +;;;###autoload
> +(defun project-other-window-command ()
> +  "Invoke a project command but display its buffer in another window.
                              ^
Comma is missing there.

More importantly, the "its" part is ambiguous.  What does it allude
to?

> +(defun project-other-frame-command ()
> +  "Invoke a project command but display its buffer in another frame.

Same here.

> +(defun project-other-tab-command ()
> +  "Invoke a project command but display its buffer in another tab.

Same here, except that in this case even the idea of "displaying in a
tab" is unclear.  This needs rewording.

Thanks.

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello Eli,

On Fri 24 Jul 2020 at 09:01AM +03, Eli Zaretskii wrote:

>> Date: Thu, 23 Jul 2020 19:04:13 -0700
>> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>,
>>  "Philip K." <philip@warpmail.net>, 41890@debbugs.gnu.org,
>>  42210@debbugs.gnu.org
>>
>> -(defun project-switch-to-buffer ()
>> +(defun project-switch-to-buffer (&optional switching-function)
>>    "Switch to another buffer belonging to the current project.
>>  This function prompts for another buffer, offering as candidates
>>  buffers that belong to the same project as the current buffer.
>>  Two buffers belong to the same project if their project instances,
>> -as reported by `project-current' in each buffer, are identical."
>> +as reported by `project-current' in each buffer, are identical.
>> +
>> +Optional argument SWITCHING-FUNCTION is the function used to
>> +switch the buffer.  It defaults to `switch-to-buffer'."
>>    (interactive)
>
> This interface strikes me as unusual and even unexpected for a command
> that switches to another buffer.  I would expect it to have an API
> similar to that of switch-to-buffer: that it should accept the buffer
> to switch to as an argument, and set up that argument in the
> 'interactive' spec according to the preferences of this command
> (offering buffers in the same project etc.).  The API you propose
> makes it awkward, to say the least, to invoke this command from Lisp.

I added the argument just so I could reuse the code in
project-switch-to-buffer, so a simple alternative for the purposes of my
patch would be to factor that code out into a new
project--select-project-buffer defun.  Then no existing APIs would be
changed.

Would that be sufficient?

> Granted, the original API doesn't allow such invocation, either, but
> as long as we are changing this API, let's try fixing that, okay?

This is a bit tricky actually -- what should the function do if some
Lisp code passes it a buffer which is not part of the current project?
Throw an error?  But then surely the Lisp code would prefer to just
check if the buffer is part of the project itself, and use
switch-to-buffer.

I'd be grateful if you could say more about the sort of Lisp code you
have in mind.  I'm struggling to imagine wanting to call this function
from Lisp except via call-interactively.

-- 
Sean Whitton

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: dgutov@yandex.ru, juri@linkov.net, contovob@tcd.ie, philip@warpmail.net,
>  41890@debbugs.gnu.org, 42210@debbugs.gnu.org
> Date: Fri, 24 Jul 2020 08:12:13 -0700
> 
> > This interface strikes me as unusual and even unexpected for a command
> > that switches to another buffer.  I would expect it to have an API
> > similar to that of switch-to-buffer: that it should accept the buffer
> > to switch to as an argument, and set up that argument in the
> > 'interactive' spec according to the preferences of this command
> > (offering buffers in the same project etc.).  The API you propose
> > makes it awkward, to say the least, to invoke this command from Lisp.
> 
> I added the argument just so I could reuse the code in
> project-switch-to-buffer, so a simple alternative for the purposes of my
> patch would be to factor that code out into a new
> project--select-project-buffer defun.  Then no existing APIs would be
> changed.
> 
> Would that be sufficient?

What you added is not my problem, my problem is that there's no easy
way of calling this function from Lisp.

> > Granted, the original API doesn't allow such invocation, either, but
> > as long as we are changing this API, let's try fixing that, okay?
> 
> This is a bit tricky actually -- what should the function do if some
> Lisp code passes it a buffer which is not part of the current project?

Just switch to that buffer, I think.

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

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

Hello,

On Fri 24 Jul 2020 at 07:12PM +03, Eli Zaretskii wrote:

> Just switch to that buffer, I think.

Okay, then I think the attached addresses feedback received.  Thanks!

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-Factor-out-project-read-project-buffer-from-proje.patch --]
[-- Type: text/x-diff, Size: 2616 bytes --]

From 08394aa143a5e0fc627e259b4deee3a1c3317960 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 24 Jul 2020 13:36:39 -0700
Subject: [PATCH v2 1/3] Factor out project--read-project-buffer from
 project-switch-buffer

* lisp/progmodes/project.el (project--read-project-buffer): New
function extracted from project-switch-buffer.
* lisp/progmodes/project.el (project-switch-buffer): Instead of
unconditionally reading a project buffer from the user, add
buffer-or-name argument, and populate it using
project--read-project-buffer when called interactively.  Update
docstring.
---
 lisp/progmodes/project.el | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index a0930553bd..9534eb2ef6 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -877,14 +877,7 @@ project-compile
          (default-directory (project-root pr)))
     (compile command comint)))
 
-;;;###autoload
-(defun project-switch-to-buffer ()
-  "Switch to another buffer belonging to the current project.
-This function prompts for another buffer, offering as candidates
-buffers that belong to the same project as the current buffer.
-Two buffers belong to the same project if their project instances,
-as reported by `project-current' in each buffer, are identical."
-  (interactive)
+(defun project--read-project-buffer ()
   (let* ((pr (project-current t))
          (current-buffer (current-buffer))
          (other-buffer (other-buffer current-buffer))
@@ -896,13 +889,22 @@ project-switch-to-buffer
                  (equal pr
                         (with-current-buffer (cdr buffer)
                           (project-current)))))))
-    (switch-to-buffer
-     (read-buffer
-      "Switch to buffer: "
-      (when (funcall predicate (cons other-name other-buffer))
-        other-name)
-      nil
-      predicate))))
+    (read-buffer
+     "Switch to buffer: "
+     (when (funcall predicate (cons other-name other-buffer))
+       other-name)
+     nil
+     predicate)))
+
+;;;###autoload
+(defun project-switch-to-buffer (buffer-or-name)
+  "Display buffer BUFFER-OR-NAME in the selected window.
+When called interactively, prompts for a buffer belonging to the
+current project.  Two buffers belong to the same project if their
+project instances, as reported by `project-current' in each
+buffer, are identical."
+  (interactive (list (project--read-project-buffer)))
+  (switch-to-buffer buffer))
 
 (defcustom project-kill-buffers-ignores
   '("\\*Help\\*")
-- 
2.27.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: v2-0002-Add-project-display-buffer-and-project-display-bu.patch --]
[-- Type: text/x-diff, Size: 1908 bytes --]

From a86e847607b281643603865c7231e52fb467da9c Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 24 Jul 2020 13:54:49 -0700
Subject: [PATCH v2 2/3] Add project-display-buffer and
 project-display-buffer-other-frame

* lisp/progmodes/project.el (project-display-buffer,
project-display-buffer-other-frame): Add commands.
---
 lisp/progmodes/project.el | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 9534eb2ef6..f674749497 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -906,6 +906,30 @@ project-switch-to-buffer
   (interactive (list (project--read-project-buffer)))
   (switch-to-buffer buffer))
 
+;;;###autoload
+(defun project-display-buffer (buffer-or-name)
+  "Display BUFFER-OR-NAME in some window, without selecting it.
+When called interactively, prompts for a buffer belonging to the
+current project.  Two buffers belong to the same project if their
+project instances, as reported by `project-current' in each
+buffer, are identical."
+  (interactive (list (project--read-project-buffer)))
+  (display-buffer buffer))
+
+;;;###autoload
+(defun project-display-buffer-other-frame (buffer-or-name)
+  "Display BUFFER-OR-NAME preferably in another frame.
+When called interactively, prompts for a buffer belonging to the
+current project.  Two buffers belong to the same project if their
+project instances, as reported by `project-current' in each
+buffer, are identical.
+
+This function uses `display-buffer-other-frame' as a subroutine,
+which see for how it is determined where the buffer will be
+displayed."
+  (interactive (list (project--read-project-buffer)))
+  (display-buffer-other-frame buffer))
+
 (defcustom project-kill-buffers-ignores
   '("\\*Help\\*")
   "Conditions for buffers `project-kill-buffers' should not kill.
-- 
2.27.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: v2-0003-Add-project-other-place-commands.patch --]
[-- Type: text/x-diff, Size: 3524 bytes --]

From f2b038a73a6868a0b6a3b1396ce670cdeeeb75cc Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Thu, 23 Jul 2020 18:55:42 -0700
Subject: [PATCH v2 3/3] Add project other place commands

* lisp/progmodes/project.el (project-other-window-map,
project-other-frame-map, project--other-place-command,
project-other-window-command, project-other-frame-command,
project-other-tab-command): Add these functions and maps.
* lisp/progmodes/project.el: Bind project-other-window-command to C-x
4 p, project-other-frame-command to C-x 5 p and
project-other-tab-command to C-x t p.
---
 lisp/progmodes/project.el | 67 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index f674749497..7a0bf1fdbf 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -592,6 +592,73 @@ project-prefix-map
 
 ;;;###autoload (define-key ctl-x-map "p" project-prefix-map)
 
+;; We can't have these place-specific maps inherit from
+;; project-prefix-map because project--other-place-command needs to
+;; know which map the key binding came from, as if it came from one of
+;; these maps, we don't want to set display-buffer-overriding-action
+
+(defvar project-other-window-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-o" #'project-display-buffer)
+    map)
+  "Keymap for project commands that display buffers in other windows.")
+
+(defvar project-other-frame-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-o" #'project-display-buffer-other-frame)
+    map)
+  "Keymap for project commands that display buffers in other frames.")
+
+(defun project--other-place-command (action &optional map)
+  (let* ((key (read-key-sequence-vector nil t))
+         (place-cmd (lookup-key map key))
+         (generic-cmd (lookup-key project-prefix-map key))
+         (display-buffer-overriding-action (unless place-cmd action)))
+    (if-let ((cmd (or place-cmd generic-cmd)))
+        (call-interactively cmd)
+      (user-error "%s is undefined" (key-description key)))))
+
+;;;###autoload
+(defun project-other-window-command ()
+  "Run project command, displaying resultant buffer in another window.
+
+The following commands are available:
+
+\\{project-prefix-map}
+\\{project-other-window-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-pop-up-window)
+                                  (inhibit-same-window . t))
+                                project-other-window-map))
+
+;;;###autoload (define-key ctl-x-4-map "p" #'project-other-window-command)
+
+;;;###autoload
+(defun project-other-frame-command ()
+  "Run project command, displaying resultant buffer in another frame.
+
+The following commands are available:
+
+\\{project-prefix-map}
+\\{project-other-frame-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-pop-up-frame))
+                                project-other-frame-map))
+
+;;;###autoload (define-key ctl-x-5-map "p" #'project-other-frame-command)
+
+;;;###autoload
+(defun project-other-tab-command ()
+  "Run project command, displaying resultant buffer in a new tab.
+
+The following commands are available:
+
+\\{project-prefix-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-in-new-tab))))
+
+;;;###autoload (define-key tab-prefix-map "p" #'project-other-tab-command)
+
 (defun project--value-in-dir (var dir)
   (with-temp-buffer
     (setq default-directory dir)
-- 
2.27.0

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

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

Hi Sean,

On 25.07.2020 00:20, Sean Whitton wrote:
> Okay, then I think the attached addresses feedback received.  Thanks!

These are good patches, working well.

But here's a change on top of yours that simplified things a little, and 
makes a few things unnecessary (I think). All by using the variable 
called switch-to-buffer-obey-display-actions.

What do you and others think?

I'll easily admit to being out of my depth when it comes to window 
management, so if there are reasons to go with the full-on approach, no 
objections from me.

[-- Attachment #2: project-other-place-shorter.diff --]
[-- Type: text/x-patch, Size: 3780 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 7a0bf1fdbf..2cad66e705 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -592,29 +592,12 @@ project-prefix-map
 
 ;;;###autoload (define-key ctl-x-map "p" project-prefix-map)
 
-;; We can't have these place-specific maps inherit from
-;; project-prefix-map because project--other-place-command needs to
-;; know which map the key binding came from, as if it came from one of
-;; these maps, we don't want to set display-buffer-overriding-action
-
-(defvar project-other-window-map
-  (let ((map (make-sparse-keymap)))
-    (define-key map "\C-o" #'project-display-buffer)
-    map)
-  "Keymap for project commands that display buffers in other windows.")
-
-(defvar project-other-frame-map
-  (let ((map (make-sparse-keymap)))
-    (define-key map "\C-o" #'project-display-buffer-other-frame)
-    map)
-  "Keymap for project commands that display buffers in other frames.")
-
-(defun project--other-place-command (action &optional map)
+(defun project--other-place-command (action)
   (let* ((key (read-key-sequence-vector nil t))
-         (place-cmd (lookup-key map key))
-         (generic-cmd (lookup-key project-prefix-map key))
-         (display-buffer-overriding-action (unless place-cmd action)))
-    (if-let ((cmd (or place-cmd generic-cmd)))
+         (cmd (lookup-key project-prefix-map key))
+         (display-buffer-overriding-action action)
+         (switch-to-buffer-obey-display-actions t))
+    (if cmd
         (call-interactively cmd)
       (user-error "%s is undefined" (key-description key)))))
 
@@ -628,8 +611,7 @@ project-other-window-command
 \\{project-other-window-map}"
   (interactive)
   (project--other-place-command '((display-buffer-pop-up-window)
-                                  (inhibit-same-window . t))
-                                project-other-window-map))
+                                  (inhibit-same-window . t))))
 
 ;;;###autoload (define-key ctl-x-4-map "p" #'project-other-window-command)
 
@@ -642,8 +624,7 @@ project-other-frame-command
 \\{project-prefix-map}
 \\{project-other-frame-map}"
   (interactive)
-  (project--other-place-command '((display-buffer-pop-up-frame))
-                                project-other-frame-map))
+  (project--other-place-command '((display-buffer-pop-up-frame))))
 
 ;;;###autoload (define-key ctl-x-5-map "p" #'project-other-frame-command)
 
@@ -971,31 +952,7 @@ project-switch-to-buffer
 project instances, as reported by `project-current' in each
 buffer, are identical."
   (interactive (list (project--read-project-buffer)))
-  (switch-to-buffer buffer))
-
-;;;###autoload
-(defun project-display-buffer (buffer-or-name)
-  "Display BUFFER-OR-NAME in some window, without selecting it.
-When called interactively, prompts for a buffer belonging to the
-current project.  Two buffers belong to the same project if their
-project instances, as reported by `project-current' in each
-buffer, are identical."
-  (interactive (list (project--read-project-buffer)))
-  (display-buffer buffer))
-
-;;;###autoload
-(defun project-display-buffer-other-frame (buffer-or-name)
-  "Display BUFFER-OR-NAME preferably in another frame.
-When called interactively, prompts for a buffer belonging to the
-current project.  Two buffers belong to the same project if their
-project instances, as reported by `project-current' in each
-buffer, are identical.
-
-This function uses `display-buffer-other-frame' as a subroutine,
-which see for how it is determined where the buffer will be
-displayed."
-  (interactive (list (project--read-project-buffer)))
-  (display-buffer-other-frame buffer))
+  (switch-to-buffer buffer-or-name))
 
 (defcustom project-kill-buffers-ignores
   '("\\*Help\\*")

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello Dmitry,

Thanks for taking a look.

On Sat 25 Jul 2020 at 01:54AM +03, Dmitry Gutov wrote:

> On 25.07.2020 00:20, Sean Whitton wrote:
>> Okay, then I think the attached addresses feedback received.  Thanks!
>
> These are good patches, working well.
>
> But here's a change on top of yours that simplified things a little, and
> makes a few things unnecessary (I think). All by using the variable
> called switch-to-buffer-obey-display-actions.
>
> What do you and others think?

I don't think this is going to work, for two reasons:

- for consistency with the existing C-x 4 C-o binding, C-x 4 p C-o
  should be bound to project-display-buffer, but C-x p C-o should not,
  so we cannot add the C-o binding to project-prefix-map, so another map
  is needed

- I suspect that by relying on switch-to-buffer-obey-display-actions,
  when you use project-display-buffer the other window will end up
  selected, which is not meant to happen.

> I'll easily admit to being out of my depth when it comes to window
> management, so if there are reasons to go with the full-on approach, no
> objections from me.

Right, it's really complicated, so I wrote the patch as conservatively
as possible.

-- 
Sean Whitton

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 25.07.2020 02:13, Sean Whitton wrote:
> I don't think this is going to work, for two reasons:
> 
> - for consistency with the existing C-x 4 C-o binding, C-x 4 p C-o
>    should be bound to project-display-buffer, but C-x p C-o should not,
>    so we cannot add the C-o binding to project-prefix-map, so another map
>    is needed
> 
> - I suspect that by relying on switch-to-buffer-obey-display-actions,
>    when you use project-display-buffer the other window will end up
>    selected, which is not meant to happen.

Fair enough, thank you.

I'll wait a day or two for the others to have a chance to leave some 
closing feedback, and then it's off to master.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: dgutov@yandex.ru, juri@linkov.net, contovob@tcd.ie, philip@warpmail.net,
>  41890@debbugs.gnu.org, 42210@debbugs.gnu.org
> Date: Fri, 24 Jul 2020 14:20:59 -0700
> 
> Okay, then I think the attached addresses feedback received.  Thanks!

Thanks, I have only one minor comment:

> +(defun project-display-buffer (buffer-or-name)
> +  "Display BUFFER-OR-NAME in some window, without selecting it.
> +When called interactively, prompts for a buffer belonging to the
> +current project.  Two buffers belong to the same project if their
> +project instances, as reported by `project-current' in each
> +buffer, are identical."

This doc string should mention display-buffer, for the same reasons
and with the same surrounding test as how
project-display-buffer-other-frame mentions
display-buffer-other-frame.

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

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

Hello,

On Sat 25 Jul 2020 at 09:14AM +03, Eli Zaretskii wrote:

> This doc string should mention display-buffer, for the same reasons
> and with the same surrounding test as how
> project-display-buffer-other-frame mentions
> display-buffer-other-frame.

Thank you, fixed, along with fixing some references to function
arguments which were incorrect in the previous series (they were
'buffer-or-name' in one place but 'buffer' in another).

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v3-0001-Factor-out-project-read-project-buffer-from-proje.patch --]
[-- Type: text/x-diff, Size: 2616 bytes --]

From 08394aa143a5e0fc627e259b4deee3a1c3317960 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 24 Jul 2020 13:36:39 -0700
Subject: [PATCH v3 1/3] Factor out project--read-project-buffer from
 project-switch-buffer

* lisp/progmodes/project.el (project--read-project-buffer): New
function extracted from project-switch-buffer.
* lisp/progmodes/project.el (project-switch-buffer): Instead of
unconditionally reading a project buffer from the user, add
buffer-or-name argument, and populate it using
project--read-project-buffer when called interactively.  Update
docstring.
---
 lisp/progmodes/project.el | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index a0930553bd..9534eb2ef6 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -877,14 +877,7 @@ project-compile
          (default-directory (project-root pr)))
     (compile command comint)))
 
-;;;###autoload
-(defun project-switch-to-buffer ()
-  "Switch to another buffer belonging to the current project.
-This function prompts for another buffer, offering as candidates
-buffers that belong to the same project as the current buffer.
-Two buffers belong to the same project if their project instances,
-as reported by `project-current' in each buffer, are identical."
-  (interactive)
+(defun project--read-project-buffer ()
   (let* ((pr (project-current t))
          (current-buffer (current-buffer))
          (other-buffer (other-buffer current-buffer))
@@ -896,13 +889,22 @@ project-switch-to-buffer
                  (equal pr
                         (with-current-buffer (cdr buffer)
                           (project-current)))))))
-    (switch-to-buffer
-     (read-buffer
-      "Switch to buffer: "
-      (when (funcall predicate (cons other-name other-buffer))
-        other-name)
-      nil
-      predicate))))
+    (read-buffer
+     "Switch to buffer: "
+     (when (funcall predicate (cons other-name other-buffer))
+       other-name)
+     nil
+     predicate)))
+
+;;;###autoload
+(defun project-switch-to-buffer (buffer-or-name)
+  "Display buffer BUFFER-OR-NAME in the selected window.
+When called interactively, prompts for a buffer belonging to the
+current project.  Two buffers belong to the same project if their
+project instances, as reported by `project-current' in each
+buffer, are identical."
+  (interactive (list (project--read-project-buffer)))
+  (switch-to-buffer buffer))
 
 (defcustom project-kill-buffers-ignores
   '("\\*Help\\*")
-- 
2.27.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: v3-0002-Add-project-display-buffer-and-project-display-bu.patch --]
[-- Type: text/x-diff, Size: 2121 bytes --]

From 61fc6615a7c438777ca80f71934979dfd029f0de Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 24 Jul 2020 13:54:49 -0700
Subject: [PATCH v3 2/3] Add project-display-buffer and
 project-display-buffer-other-frame

* lisp/progmodes/project.el (project-display-buffer,
project-display-buffer-other-frame): Add commands.
---
 lisp/progmodes/project.el | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 9534eb2ef6..0288635fb8 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -904,7 +904,34 @@ project-switch-to-buffer
 project instances, as reported by `project-current' in each
 buffer, are identical."
   (interactive (list (project--read-project-buffer)))
-  (switch-to-buffer buffer))
+  (switch-to-buffer buffer-or-name))
+
+;;;###autoload
+(defun project-display-buffer (buffer-or-name)
+  "Display BUFFER-OR-NAME in some window, without selecting it.
+When called interactively, prompts for a buffer belonging to the
+current project.  Two buffers belong to the same project if their
+project instances, as reported by `project-current' in each
+buffer, are identical.
+
+This function uses `display-buffer' as a subroutine, which see
+for how it is determined where the buffer will be displayed."
+  (interactive (list (project--read-project-buffer)))
+  (display-buffer buffer-or-name))
+
+;;;###autoload
+(defun project-display-buffer-other-frame (buffer-or-name)
+  "Display BUFFER-OR-NAME preferably in another frame.
+When called interactively, prompts for a buffer belonging to the
+current project.  Two buffers belong to the same project if their
+project instances, as reported by `project-current' in each
+buffer, are identical.
+
+This function uses `display-buffer-other-frame' as a subroutine,
+which see for how it is determined where the buffer will be
+displayed."
+  (interactive (list (project--read-project-buffer)))
+  (display-buffer-other-frame buffer))
 
 (defcustom project-kill-buffers-ignores
   '("\\*Help\\*")
-- 
2.27.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: v3-0003-Add-project-other-place-commands.patch --]
[-- Type: text/x-diff, Size: 3524 bytes --]

From 5cc1998dc965d41955ebf8ee2deed7dbcd8e96a8 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Thu, 23 Jul 2020 18:55:42 -0700
Subject: [PATCH v3 3/3] Add project other place commands

* lisp/progmodes/project.el (project-other-window-map,
project-other-frame-map, project--other-place-command,
project-other-window-command, project-other-frame-command,
project-other-tab-command): Add these functions and maps.
* lisp/progmodes/project.el: Bind project-other-window-command to C-x
4 p, project-other-frame-command to C-x 5 p and
project-other-tab-command to C-x t p.
---
 lisp/progmodes/project.el | 67 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 0288635fb8..3efe0c1ce3 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -592,6 +592,73 @@ project-prefix-map
 
 ;;;###autoload (define-key ctl-x-map "p" project-prefix-map)
 
+;; We can't have these place-specific maps inherit from
+;; project-prefix-map because project--other-place-command needs to
+;; know which map the key binding came from, as if it came from one of
+;; these maps, we don't want to set display-buffer-overriding-action
+
+(defvar project-other-window-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-o" #'project-display-buffer)
+    map)
+  "Keymap for project commands that display buffers in other windows.")
+
+(defvar project-other-frame-map
+  (let ((map (make-sparse-keymap)))
+    (define-key map "\C-o" #'project-display-buffer-other-frame)
+    map)
+  "Keymap for project commands that display buffers in other frames.")
+
+(defun project--other-place-command (action &optional map)
+  (let* ((key (read-key-sequence-vector nil t))
+         (place-cmd (lookup-key map key))
+         (generic-cmd (lookup-key project-prefix-map key))
+         (display-buffer-overriding-action (unless place-cmd action)))
+    (if-let ((cmd (or place-cmd generic-cmd)))
+        (call-interactively cmd)
+      (user-error "%s is undefined" (key-description key)))))
+
+;;;###autoload
+(defun project-other-window-command ()
+  "Run project command, displaying resultant buffer in another window.
+
+The following commands are available:
+
+\\{project-prefix-map}
+\\{project-other-window-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-pop-up-window)
+                                  (inhibit-same-window . t))
+                                project-other-window-map))
+
+;;;###autoload (define-key ctl-x-4-map "p" #'project-other-window-command)
+
+;;;###autoload
+(defun project-other-frame-command ()
+  "Run project command, displaying resultant buffer in another frame.
+
+The following commands are available:
+
+\\{project-prefix-map}
+\\{project-other-frame-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-pop-up-frame))
+                                project-other-frame-map))
+
+;;;###autoload (define-key ctl-x-5-map "p" #'project-other-frame-command)
+
+;;;###autoload
+(defun project-other-tab-command ()
+  "Run project command, displaying resultant buffer in a new tab.
+
+The following commands are available:
+
+\\{project-prefix-map}"
+  (interactive)
+  (project--other-place-command '((display-buffer-in-new-tab))))
+
+;;;###autoload (define-key tab-prefix-map "p" #'project-other-tab-command)
+
 (defun project--value-in-dir (var dir)
   (with-temp-buffer
     (setq default-directory dir)
-- 
2.27.0

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 26.07.2020 08:15, Sean Whitton wrote:

> Thank you, fixed, along with fixing some references to function
> arguments which were incorrect in the previous series (they were
> 'buffer-or-name' in one place but 'buffer' in another).

Applied and pushed. Thanks all!

With that, I'm closing the associated bug report.

Please file new reports for any follow-up patches.

bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello,

On Wed 22 Jul 2020 at 02:38AM +03, Juri Linkov wrote:

> While the prompt is active, the key '?' and 'C-h' could (and I think should)
> display a list of *ALL* available key bindings from the project keymap
> in the *Help* buffer (like e.g. 'query-replace' does after typing 'C-h').

What would determine which of the bindings get shown in the prompt,
then, supposing there were too many to fit them all?

>> I do think we should avoid binding commands under C-x 4 where the
>> versions under C-x p would already display in another window -- I think
>> it is potentially quite confusing to have bindings with identical
>> behaviour under both C-x 4 p and C-x p.
>
> Shouldn't some key sequence force displaying the project buffer in the
> same window (when a version under C-x p displays it in another window)?

It's hard to know what key sequence to use.

>> But that means we need the defcustom, because a user could use
>> display-buffer-alist to change which commands under C-x p will use
>> another window.
>
> I now understand that your top message above implies that a user
> could customize display-buffer-alist to display a buffer in another window,
> but usually users don't customize display-buffer-alist to display a buffer
> in another frame/tab?

I exclusively use it to display buffers in other windows :)

-- 
Sean Whitton

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> While the prompt is active, the key '?' and 'C-h' could (and I think should)
>> display a list of *ALL* available key bindings from the project keymap
>> in the *Help* buffer (like e.g. 'query-replace' does after typing 'C-h').
>
> What would determine which of the bindings get shown in the prompt,
> then, supposing there were too many to fit them all?

Maybe it's possible to show all them by using 'read-char-choice' when only
the initial letters are displayed in the prompt like "(f, g, d, v, s, ?): "
or using 'read-answer' or 'read-multiple-choice', e.g.:

(read-multiple-choice "Select project"
                      '((?f "Find file")
                        (?g "Find regexp")
                        (?d "dired")
                        (?v "vc-dir")
                        (?s "shell")))

>>> I do think we should avoid binding commands under C-x 4 where the
>>> versions under C-x p would already display in another window -- I think
>>> it is potentially quite confusing to have bindings with identical
>>> behaviour under both C-x 4 p and C-x p.
>>
>> Shouldn't some key sequence force displaying the project buffer in the
>> same window (when a version under C-x p displays it in another window)?
>
> It's hard to know what key sequence to use.

Indeed, it's hard to find a key prefix analogous to 'C-x 4 1'.
Maybe 'C-x 4 1 p'?

bug#41890: bug#42210: bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello Juri,

On Thu 23 Jul 2020 at 03:32AM +03, Juri Linkov wrote:

>>> While the prompt is active, the key '?' and 'C-h' could (and I think should)
>>> display a list of *ALL* available key bindings from the project keymap
>>> in the *Help* buffer (like e.g. 'query-replace' does after typing 'C-h').
>>
>> What would determine which of the bindings get shown in the prompt,
>> then, supposing there were too many to fit them all?
>
> Maybe it's possible to show all them by using 'read-char-choice' when only
> the initial letters are displayed in the prompt like "(f, g, d, v, s, ?): "
> or using 'read-answer' or 'read-multiple-choice', e.g.:
>
> (read-multiple-choice "Select project"
>                       '((?f "Find file")
>                         (?g "Find regexp")
>                         (?d "dired")
>                         (?v "vc-dir")
>                         (?s "shell")))

If we're only displaying single letters, then I'm not sure there is much
point in prompting at all?  Either someone knows what letter to press or
they don't -- in either case the prompt seems useless.

>>>> I do think we should avoid binding commands under C-x 4 where the
>>>> versions under C-x p would already display in another window -- I think
>>>> it is potentially quite confusing to have bindings with identical
>>>> behaviour under both C-x 4 p and C-x p.
>>>
>>> Shouldn't some key sequence force displaying the project buffer in the
>>> same window (when a version under C-x p displays it in another window)?
>>
>> It's hard to know what key sequence to use.
>
> Indeed, it's hard to find a key prefix analogous to 'C-x 4 1'.
> Maybe 'C-x 4 1 p'?

I'd be cautious about this, because it breaks the general semantics of
C-x 4 1.  It seems reasonable that someone might have a major-mode in
which 'p' displays another buffer, and then they might want to use C-x 4
1 to reuse the same window.

-- 
Sean Whitton

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Basil L. Contovounesios]

Sean Whitton <spwhitton@spwhitton.name> writes:

> +(defcustom project-switch-use-entire-map t
> +  "Make `project-switch-project' use entire `project-prefix-map'.
> +If nil, `project-switch-project' will only recognize commands
> +listed in `project-switch-commands', and signal an error when
> +others are invoked.  Otherwise, all keys in
> +`project-switch-commands', are legal even if they aren't listed
                           ^^^
                           superfluous comma

> +in the minibuffer."
> +  :type 'bool
                ^^^
                ean

> +  :version "28.1")

[...]

> +;; "other-place" because non-prototype patch will also add an entry
> +;; in ctl-x-5-map and under C-x t p
> +(defcustom project-other-place-commands
> +  '((project-find-file . "Find file")
> +    (project-switch-to-buffer . "Switch to buffer")
> +    (project-dired . "Dired")
> +    ;; Eshell uses the current window by default, but Shell defaults
> +    ;; to using the other window.  If a user has added an entry to
> +    ;; `display-buffer-alist' for Shell, they probably want to add an
> +    ;; entry here, too
                        ^^
                        missing full stop

> +    (project-eshell . "Eshell")
> +    (project-switch-project . "Switch project"))
> +    "Alist mapping commands to descriptions.
> +Used by `project-other-window-command' to construct a dispatch menu of
> +commands available to be displayed in another window.
> +
> +Commands in this list should be ones which normally display their
> +buffer in the current window.
> +
>  Each element looks like (COMMAND LABEL), where COMMAND should be
>  bound in `project-prefix-map'.  LABEL is used to distinguish the
>  choice in the dispatch menu."

Thanks,

-- 
Basil

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Mon, 20 Jul 2020 13:21:22 +0300
> Cc: "Philip K." <philip@warpmail.net>, 41890@debbugs.gnu.org,
>  42210@debbugs.gnu.org, Dmitry Gutov <dgutov@yandex.ru>, juri@linkov.net
> 
> Sean Whitton <spwhitton@spwhitton.name> writes:
> 
> > +(defcustom project-switch-use-entire-map t
> > +  "Make `project-switch-project' use entire `project-prefix-map'.
> > +If nil, `project-switch-project' will only recognize commands
> > +listed in `project-switch-commands', and signal an error when
> > +others are invoked.  Otherwise, all keys in
> > +`project-switch-commands', are legal even if they aren't listed
>                            ^^^
>                            superfluous comma

Also, GNU standards frown on using "legal" when you actually mean
"valid".  "Legal" and "illegal" should be reserved to describing
issues pertaining to laws and breaking the laws.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> But why project--keymap-prompt, though? I thought you were going to do
>> something that would look like a "normal" prefix keymap. They don't prompt.
>
> Over in the other bug I received feedback from Juri which I understood
> as saying that only a patch using the prompting approach would be likely
> to be merged, so I've been working on that.  Sounds like things are more
> undecided than I thought?

Sorry for the confusion.  Actually I referred to the patch that sets
the transient map.  The fact that it also displays a prompt is a minor detail,
not needed for your patch.

The patch that I referred to was posted by Philip in
https://debbugs.gnu.org/41890#50  This patch uses
set-transient-map to set project-prefix-map,
and the prompt is displayed by ‘message’.

But later Philip sent another patch in
https://debbugs.gnu.org/41890#100
that doesn't use set-transient-map
for the reasons that I don't understand.
Providing the explanation Philip said:

  It turned out that the transient map approach didn't work, as it
  ignored the value in default-directory, thus running all commands in
  whatever the current project was.

Maybe the same function can set default-directory
to solve the problem to be able to use set-transient-map?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 13.07.2020 02:48, Juri Linkov wrote:
> But later Philip sent another patch in
> https://debbugs.gnu.org/41890#100
> that doesn't use set-transient-map
> for the reasons that I don't understand.
> Providing the explanation Philip said:
> 
>    It turned out that the transient map approach didn't work, as it
>    ignored the value in default-directory, thus running all commands in
>    whatever the current project was.
> 
> Maybe the same function can set default-directory
> to solve the problem to be able to use set-transient-map?

I think I can explain that: you can set the transient map to handle the 
next command, but you can let-bind a variable to only have the binding 
have the effect during the next command.

And you can't exactly setq that value either, or else it would overwrite 
the buffer-local default-directory value in the current buffer.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>>    It turned out that the transient map approach didn't work, as it
>>    ignored the value in default-directory, thus running all commands in
>>    whatever the current project was.
>> Maybe the same function can set default-directory
>> to solve the problem to be able to use set-transient-map?
>
> I think I can explain that: you can set the transient map to handle the
> next command, but you can let-bind a variable to only have the binding 
> have the effect during the next command.
>
> And you can't exactly setq that value either, or else it would overwrite
> the buffer-local default-directory value in the current buffer.

project-switch-project could set a new global variable
project-default-directory and project commands could use it.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Philip K.]

Juri Linkov <juri@linkov.net> writes:

> project-switch-project could set a new global variable
> project-default-directory and project commands could use it.

Is there something wrong with the second approach?

I'd have to try it out myself, but getting a global variable could
introduce a double-state situation where when I switch to a project
using C-x p p, open another on and then manually switch back to the
first one (C-x b), that the global variable project-switch-project would
still indicate that the second project is "open".

As I said, I haven't tried anything out, and maybe the issue doesn't
exist or is circumventable (eg. by having every function reset the
global value after using it), but is that really worth it just to use a
transient map? Or did I miss something?

-- 
	Philip K.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 13.07.2020 09:56, Philip K. wrote:
> Juri Linkov<juri@linkov.net>  writes:
> 
>> project-switch-project could set a new global variable
>> project-default-directory and project commands could use it.
> Is there something wrong with the second approach?

I think it's fine.

Juri's suggestion could work as well, but it sounds like it would 
require more fiddly management of the new global variable.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 13.07.2020 13:47, Dmitry Gutov wrote:
> 
> Juri's suggestion could work as well, but it sounds like it would 
> require more fiddly management of the new global variable.

...but the same problem might not exist for Sean's patch.

So he can very well try that approach.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Philip K.]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 13.07.2020 13:47, Dmitry Gutov wrote:
>> 
>> Juri's suggestion could work as well, but it sounds like it would 
>> require more fiddly management of the new global variable.
>
> ...but the same problem might not exist for Sean's patch.

I'm missing the context, what exactly is the issue? Or what part of this
patch would Sean want to depend on? I hope I understand correctly that
this is the patch that should merge C-x 4 4 C-x p f into C-x 4 p f?

-- 
	Philip K.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Sean Whitton]

Hello Philip,

On Mon 13 Jul 2020 at 01:02PM +02, Philip K. wrote:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> On 13.07.2020 13:47, Dmitry Gutov wrote:
>>>
>>> Juri's suggestion could work as well, but it sounds like it would
>>> require more fiddly management of the new global variable.
>>
>> ...but the same problem might not exist for Sean's patch.
>
> I'm missing the context, what exactly is the issue? Or what part of this
> patch would Sean want to depend on? I hope I understand correctly that
> this is the patch that should merge C-x 4 4 C-x p f into C-x 4 p f?

Yes, that one.

Juri suggested that I try to make C-x 4 p work like C-x p p, so my patch
is probably going to need to generalise some of the code which powers
C-x p p, but it seems there is some disagreement right now about how C-x
p p is going to work.

-- 
Sean Whitton

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

> As I said, I haven't tried anything out, and maybe the issue doesn't
> exist or is circumventable (eg. by having every function reset the
> global value after using it), but is that really worth it just to use a
> transient map? Or did I miss something?

A new variable could introduce a new notion of "the current project".
This implies that some commands used in other buffers will be applied
to the currently active project.

This is similar to the notion of next-error-last-buffer -
the most recent buffer for next-error commands.

I don't know if a real need exists for such thing,
so please leave project-switch-project without a
transient map if it already works well.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Philip K.]

Juri Linkov <juri@linkov.net> writes:

>> As I said, I haven't tried anything out, and maybe the issue doesn't
>> exist or is circumventable (eg. by having every function reset the
>> global value after using it), but is that really worth it just to use a
>> transient map? Or did I miss something?
>
> A new variable could introduce a new notion of "the current project".
> This implies that some commands used in other buffers will be applied
> to the currently active project.

I'd get a "last active project" variable, as in a fallback when the
project cannot be determined after switching buffer or opening new
files. But when I hear current project, I'd assume you would have to
manually change, which would turn project.el from a tool that assists
your workflow to one that dictates it.

> This is similar to the notion of next-error-last-buffer - the most
> recent buffer for next-error commands.

So we're talking about a "last active project"?

> I don't know if a real need exists for such thing, so please leave
> project-switch-project without a transient map if it already works
> well.

Fine by me, I'm just asking in case there's a need to update the patch.

-- 
	Philip K.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> This is similar to the notion of next-error-last-buffer - the most
>> recent buffer for next-error commands.

BTW, a question for related discussion: should next-error-find-buffer-function
provide an option to support project-local value of next-error-find-buffer?
So calling ‘next-error’ on the current project's files will use the last
next-error buffer belonging to the same project.

> So we're talking about a "last active project"?

Or maybe "the most recently used project" if put in other words.

>> I don't know if a real need exists for such thing, so please leave
>> project-switch-project without a transient map if it already works
>> well.
>
> Fine by me, I'm just asking in case there's a need to update the patch.

Then it seems you patch doesn't need more updates.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 15.07.2020 01:34, Juri Linkov wrote:
> BTW, a question for related discussion: should next-error-find-buffer-function
> provide an option to support project-local value of next-error-find-buffer?
> So calling ‘next-error’ on the current project's files will use the last
> next-error buffer belonging to the same project.

Would that work as an optional value for next-error-find-buffer-function?

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Juri Linkov]

>> BTW, a question for related discussion: should next-error-find-buffer-function
>> provide an option to support project-local value of next-error-find-buffer?
>> So calling ‘next-error’ on the current project's files will use the last
>> next-error buffer belonging to the same project.
>
> Would that work as an optional value for next-error-find-buffer-function?

I believe it should be possible with a new value of next-error-find-buffer-function.
Want give it a try? ;-)

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 16.07.2020 02:59, Juri Linkov wrote:
>>> BTW, a question for related discussion: should next-error-find-buffer-function
>>> provide an option to support project-local value of next-error-find-buffer?
>>> So calling ‘next-error’ on the current project's files will use the last
>>> next-error buffer belonging to the same project.
>> Would that work as an optional value for next-error-find-buffer-function?
> I believe it should be possible with a new value of next-error-find-buffer-function.
> Want give it a try?;-)

I like the idea, but I'm not sure I'm going to use this myself.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Philip K.]

Juri Linkov <juri@linkov.net> writes:

>>> This is similar to the notion of next-error-last-buffer - the most
>>> recent buffer for next-error commands.
>
> BTW, a question for related discussion: should next-error-find-buffer-function
> provide an option to support project-local value of next-error-find-buffer?
> So calling ‘next-error’ on the current project's files will use the last
> next-error buffer belonging to the same project.

Related to this, it might also be worth considering a project-local xref
stack. If I don't clear the stack before switching between projects (or
even just my scratch buffer), I can easily get surprised by landing
somewhere I didn't expect.

-- 
	Philip K.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 15.07.2020 22:21, Philip K. wrote:
> Related to this, it might also be worth considering a project-local xref
> stack. If I don't clear the stack before switching between projects (or
> even just my scratch buffer), I can easily get surprised by landing
> somewhere I didn't expect.

Interesting thought.

We can make the xref stack storage pluggable. One or two user options 
should do the trick.

Personally, I've been using window-local locations history (via a 
third-party package), that can become another value of said options.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Simen Heggestøyl]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 18.06.2020 17:09, Philip K. wrote:
>
>> The patch below fixes that, but allows changing if you only want the
>> listed keys to be valid (the default) or every key in
>> project-prefix-map.
>> It turned out that the transiment map approach didn't work, as it
>> ignored the value in default-directory, thus running all commands in
>> whatever the current project was.
>
> Looks reasonable to me. But let's also hear from the original author.
>
> Simen, what do you think? The patch is at
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41890#127.

Looks good to me too!

My only gripe would be that it makes it a bit harder to add new
commands, since it now requires modifying both project-switch-commands
and project-prefix-map. Maybe we could reintroduce the helper function
we had for that purpose earlier.

-(defvar project-switch-commands
-  '((?f "Find file" project-find-file)
-    (?g "Find regexp" project-find-regexp)
-    (?d "Dired" project-dired)
-    (?v "VC-Dir" project-vc-dir)
-    (?e "Eshell" project-eshell))
-  "Alist mapping keys to project switching menu entries.
+(defcustom project-switch-commands
+  '((project-find-file . "Find file")
+    (project-find-regexp . "Find regexp")
+    (project-dired . "Dired")
+    (project-vc-dir . "VC-Dir")
+    (project-shell . "Shell")
+    (project-eshell . "Eshell"))

The project-shell command is added here, don't know if that was
intentional?

Also why not stick with the easier extensible list format? I could
imagine for instance adding long descriptions as an optimal third
element for the commands later on.

-- Simen

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Philip K.]

Simen Heggestøyl <simenheg@runbox.com> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> On 18.06.2020 17:09, Philip K. wrote:
>>
>>> The patch below fixes that, but allows changing if you only want the
>>> listed keys to be valid (the default) or every key in
>>> project-prefix-map.
>>> It turned out that the transiment map approach didn't work, as it
>>> ignored the value in default-directory, thus running all commands in
>>> whatever the current project was.
>>
>> Looks reasonable to me. But let's also hear from the original author.
>>
>> Simen, what do you think? The patch is at
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41890#127.
>
> Looks good to me too!
>
> My only gripe would be that it makes it a bit harder to add new
> commands, since it now requires modifying both project-switch-commands
> and project-prefix\-map. 

As in for developers, when they want to contribute a new project-*
function or users who want to just change stuff (I know the line might
be blury)?

> Maybe we could reintroduce the helper function we had for that purpose
> earlier.

I missed when this happened. In what commit was it removed, or was it
just in a patch?

> -(defvar project-switch-commands
> -  '((?f "Find file" project-find-file)
> -    (?g "Find regexp" project-find-regexp)
> -    (?d "Dired" project-dired)
> -    (?v "VC-Dir" project-vc-dir)
> -    (?e "Eshell" project-eshell))
> -  "Alist mapping keys to project switching menu entries.
> +(defcustom project-switch-commands
> +  '((project-find-file . "Find file")
> +    (project-find-regexp . "Find regexp")
> +    (project-dired . "Dired")
> +    (project-vc-dir . "VC-Dir")
> +    (project-shell . "Shell")
> +    (project-eshell . "Eshell"))
>
> The project-shell command is added here, don't know if that was
> intentional?

No, this must have been a local git mistake.

> Also why not stick with the easier extensible list format? I could
> imagine for instance adding long descriptions as an optimal third
> element for the commands later on.

The main reason I changed it was so that the alist was recognized as an
alist in customize, but I agree that changing this would be good for
forwards compatibility.

-- 
	Philip K.

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Simen Heggestøyl]

"Philip K." <philip@warpmail.net> writes:

> Simen Heggestøyl <simenheg@runbox.com> writes:
>
>> My only gripe would be that it makes it a bit harder to add new
>> commands, since it now requires modifying both project-switch-commands
>> and project-prefix\-map.
>
> As in for developers, when they want to contribute a new project-*
> function or users who want to just change stuff (I know the line might
> be blury)?

The latter. For the former case I think it's fine.

>> Maybe we could reintroduce the helper function we had for that purpose
>> earlier.
>
> I missed when this happened. In what commit was it removed, or was it
> just in a patch?

Sorry, yes, it was just a patch. Maybe it could look something like
this:

;;;###autoload
(defun project-add-switch-command (key command label)
  (define-key project-prefix-map key command)
  (add-to-list 'project-switch-commands (list command label) t))

-- Simen

bug#41890: 28.0.50; [PATCH]: Add bindings for project.el

[Dmitry Gutov]

On 19.06.2020 13:50, Simen Heggestøyl wrote:
>>> My only gripe would be that it makes it a bit harder to add new
>>> commands, since it now requires modifying both project-switch-commands
>>> and project-prefix\-map.
>> As in for developers, when they want to contribute a new project-*
>> function or users who want to just change stuff (I know the line might
>> be blury)?
> The latter. For the former case I think it's fine.
> 
>>> Maybe we could reintroduce the helper function we had for that purpose
>>> earlier.
>> I missed when this happened. In what commit was it removed, or was it
>> just in a patch?
> Sorry, yes, it was just a patch. Maybe it could look something like
> this:
> 
> ;;;###autoload
> (defun project-add-switch-command (key command label)
>    (define-key project-prefix-map key command)
>    (add-to-list 'project-switch-commands (list command label) t))

Not sure the above will change things too much. It's a function, not a 
Customize interface (which could be added for the current format of 
project-switch-commands), and its valid values would have to be 
documented and understood by the user anyway. We could as well put its 
body in the documentation as customization instructions.

Now, I think the current setup is pretty good already. The extra 
capability that the patch brings will be the ability to turn on 
project-switch-use-entire-map in their local configs.

Are there people here who intend to do that?