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

classic Classic list List threaded Threaded
96 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

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

Theodor Thornhill


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


project-bindings.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Theodor Thornhill
Hello :)

"Eli Zaretskii" <[hidden email]> 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





Reply | Threaded
Open this post in threaded view
|

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

Theodor Thornhill

Hello,

"Eli Zaretskii" <[hidden email]> 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


project-bindings.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
Theodor Thornhill <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Theodor Thornhill
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


project-bindings.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
Theodor Thornhill <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Dmitry Gutov
In reply to this post by Theodor Thornhill
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?



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
In reply to this post by Theodor Thornhill
> +    (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?



Reply | Threaded
Open this post in threaded view
|

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!



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
> 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 :(



Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Philip K.
In reply to this post by Juri Linkov-2
Juri Linkov <[hidden email]> 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.


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
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Dmitry Gutov
> Cc: [hidden email]
> From: Dmitry Gutov <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Dmitry Gutov
> From: Dmitry Gutov <[hidden email]>
> Cc: [hidden email]
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Dmitry Gutov
In reply to this post by Eli Zaretskii
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.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Dmitry Gutov <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

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

> "Basil L. Contovounesios" <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
In reply to this post by Dmitry Gutov
>> 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?



Reply | Threaded
Open this post in threaded view
|

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.



12345