bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

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

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Theodor Thornhill

Hello,

Attached is a patch to change the default behaviour of 'project-shell'.

My use case is:

- Edit some file in some project.
- 'M-x project-shell'
- Do something, then "C-x 0"
- Oops forgot to read output.
- 'M-x project-shell'
- See the same buffer.

Right now 'project-shell' opens a new buffer every time it is invoked. Now I have to find it in the buffer list. Putting previous behaviour on universal argument will in addition enable this workflow

- Edit some file in some project.
- 'M-x project-shell'
- 'C-u M-x project-shell'
- Now emacs is split it two, with two separate shell processes


In addition, I let default name be *'project-root-dir'-shell* to signify this shell buffer to be unique from other shell buffers invoked by 'M-x shell'.

All the best,
Theodor Thornhill


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

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Dmitry Gutov
Hi!

On 14.06.2020 21:29, Theodor Thornhill wrote:

> Attached is a patch to change the default behaviour of 'project-shell'.
>
> My use case is:
>
> - Edit some file in some project.
> - 'M-x project-shell'
> - Do something, then "C-x 0"
> - Oops forgot to read output.
> - 'M-x project-shell'
> - See the same buffer.
>
> Right now 'project-shell' opens a new buffer every time it is invoked. Now I have to find it in the buffer list. Putting previous behaviour on universal argument will in addition enable this workflow
>
> - Edit some file in some project.
> - 'M-x project-shell'
> - 'C-u M-x project-shell'
> - Now emacs is split it two, with two separate shell processes
>
>
> In addition, I let default name be *'project-root-dir'-shell* to signify this shell buffer to be unique from other shell buffers invoked by 'M-x shell'.

Sounds good to me.

Juri, will you the review?

NB: It's difficult for me to open the attachment because it has to file
extension.



Reply | Threaded
Open this post in threaded view
|

bug#41858: Add the patch as a .patch

Theodor Thornhill
In reply to this post by Theodor Thornhill


Hello,

Sorry, seemed to forget the extension to the attached file.

This file should be more accessible.

Hope everything works now.

Theodor Thornhill



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

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Juri Linkov-2
In reply to this post by Theodor Thornhill
> Right now 'project-shell' opens a new buffer every time it is invoked.
> Now I have to find it in the buffer list.  Putting previous behaviour
> on universal argument will in addition enable this workflow
>
> - Edit some file in some project.
> - 'M-x project-shell'
> - 'C-u M-x project-shell'
> - Now emacs is split it two, with two separate shell processes
>
> In addition, I let default name be *'project-root-dir'-shell* to
> signify this shell buffer to be unique from other shell buffers
> invoked by 'M-x shell'.

Thanks, it's a nice idea to prepend the project root dir name
to the shell buffer name.

Formerly, I tried to do something like this using 'uniquify',
but after creating more shell buffers in the same project,
it begins to generate inappropriately long names.

So eventually I arrived to the following line for the init file:

  (add-hook 'shell-mode-hook 'rename-uniquely)

that just appends a number to the buffer name,
so your patch won't affect my usage at all,
I'll continue getting new shell buffers every time
after 'M-x project-shell'.

Regarding the C-u argument, I don't know maybe someone might want
to use it.  But shouldn't the C-u argument of 'project-shell' be
more compatible with the C-u argument of 'shell' in its behaviour?

Another possible point for better consistency between them
is to use 'pop-to-buffer' (like in 'shell') instead of
'switch-to-buffer-other-window'.



Reply | Threaded
Open this post in threaded view
|

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Theodor Thornhill
Hi, and thanks for looking at the code!

"Juri Linkov" <[hidden email]> writes:

[...]
> Thanks, it's a nice idea to prepend the project root dir name
> to the shell buffer name.

Cool!

[...]
> So eventually I arrived to the following line for the init file:
>
>   (add-hook 'shell-mode-hook 'rename-uniquely)
>
> that just appends a number to the buffer name,
> so your patch won't affect my usage at all,
> I'll continue getting new shell buffers every time
> after 'M-x project-shell'.
>
How about we just add this behaviour instead of appending the file-path? so we get the "<n>" appended to the shell buffer names. Then you can remove that line from init :)

> Regarding the C-u argument, I don't know maybe someone might want
> to use it.  But shouldn't the C-u argument of 'project-shell' be
> more compatible with the C-u argument of 'shell' in its behaviour?

I kind of agree, though it seems like the original "project-shell" was intended to be used only interactively. I kind of view it as a specialized case of "shell", maybe as a sort of "shell-dwim" but only for a very limited case. We are always free of course to add a normal shell buffer in root, and use it non-interactively in code.

However, I can add an "&optional buffer" argument to the code and do like it's done in "shell". It just seems like it is complicating the behaviour a little bit. What do you think?

In addition, how do I then avoid duplication of code between the interactive call and the default behaviour? Specifically, I am thinking of the 'default-directory' which is not set. I guess I have to set that twice then?

> Another possible point for better consistency between them
> is to use 'pop-to-buffer' (like in 'shell') instead of
> 'switch-to-buffer-other-window'.

Agreed. Done.

I've attached another patch for you to look at, with only the easy changes, awaiting some feedback on the other parts :)

Thanks again,

Theodor


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

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Dmitry Gutov
In reply to this post by Juri Linkov-2
On 15.06.2020 01:46, Juri Linkov wrote:
> But shouldn't the C-u argument of 'project-shell' be
> more compatible with the C-u argument of 'shell' in its behaviour?

Looking at the docstring, shell's C-u behavior seems pretty weird to me.

Why would someone input an existing buffer this way, if they could just
switch to it instead, with the same completion interface?



Reply | Threaded
Open this post in threaded view
|

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Theodor Thornhill


Looking at the docstring, shell's C-u behavior seems pretty weird to me.

Why would someone input an existing buffer this way, if they could just
switch to it instead, with the same completion interface?



+1. 

Also, if you choose an existing non-shell buffer, the one you are in for example, it will actually just turn that buffer into a shell buffer. That behavior should actually be a bug imo.

Theo

Reply | Threaded
Open this post in threaded view
|

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Juri Linkov-2
In reply to this post by Theodor Thornhill
> How about we just add this behaviour instead of appending the
> file-path?  so we get the "<n>" appended to the shell buffer names.
> Then you can remove that line from init :)

Thanks, I see you did exactly this in your new patch.

>> Regarding the C-u argument, I don't know maybe someone might want
>> to use it.  But shouldn't the C-u argument of 'project-shell' be
>> more compatible with the C-u argument of 'shell' in its behaviour?
>
> I kind of agree, though it seems like the original "project-shell" was
> intended to be used only interactively. I kind of view it as a specialized
> case of "shell", maybe as a sort of "shell-dwim" but only for a very
> limited case. We are always free of course to add a normal shell buffer in
> root, and use it non-interactively in code.
>
> However, I can add an "&optional buffer" argument to the code and do
> like it's done in "shell".  It just seems like it is complicating the
> behaviour a little bit.  What do you think?

I agree there is no need to complicate "project-shell" with all
idiosyncrasies of "shell".

> I've attached another patch for you to look at, with only the easy
> changes, awaiting some feedback on the other parts :)

I see no more problems with your patch, it seems it covers the needs
of most users, thanks.



Reply | Threaded
Open this post in threaded view
|

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Theodor Thornhill
Hello!

"Juri Linkov" <[hidden email]> writes:
> I see no more problems with your patch, it seems it covers the needs
> of most users, thanks.

If this one seems ok, then maybe a commit message could be:

-------------------------------------------------------------------
Change default behaviour of 'project-shell'

* lisp/progmodes/project.el (project-shell):   If a shell already is open, pop to that buffer. Otherwise, or if universal argument is used, open a subsequent shell buffer and jump to it.

Also sets shell buffer name with a prefix of project root name.
-------------------------------------------------------------------

Have a nice day!

Theo


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

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Eli Zaretskii
> Date: Thu, 18 Jun 2020 09:57:55 +0000
> From: Theodor Thornhill <[hidden email]>
> Cc: [hidden email], Dmitry Gutov <[hidden email]>
>
> Change default behaviour of 'project-shell'
>
> * lisp/progmodes/project.el (project-shell):   If a shell already is open, pop to that buffer. Otherwise, or if universal argument is used, open a subsequent shell buffer and jump to it.

Please always mention the bug number, if there is any, in the commit
log message.  A reference to a bug makes it easy to find discussions
relevant to a change, which is very important when one needs to
understand better the rationale for a change.

Also, please format the log message according to our conventions, as
described in CONTRIBUTE.

TIA.

>  (defun project-shell ()
> -  "Open Shell in the current project."
> +  "Open Shell in the current project.

This doesn't say enough about what the command does.  How about

  Start an inferior shell in the current project's root directory.



Reply | Threaded
Open this post in threaded view
|

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Theodor Thornhill
Hello!

"Eli Zaretskii" <[hidden email]> writes:

> Please always mention the bug number, if there is any, in the commit
> log message.
Ok!

> Also, please format the log message according to our conventions, as
> described in CONTRIBUTE.

Read, and new attempt further down.

> This doesn't say enough about what the command does.  How about
>
>   Start an inferior shell in the current project's root directory.

Yeah, that is clearer. I've added it in the attached patch.

Second attempt of a commit message:

-------------------------------------------------------------------
Change default behaviour of 'project-shell'

* lisp/progmodes/project.el (project-shell): If a shell
already is open, pop to that buffer. Otherwise, or if
universal argument is used, open a subsequent shell buffer
and jump to it. Also sets shell buffer name with a prefix of
project root name. (Bug#41858)
-------------------------------------------------------------------



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

bug#41858: 28.0.50; [PATCH]: Make project-shell use universal argument

Dmitry Gutov
On 18.06.2020 21:00, Theodor Thornhill wrote:
> Yeah, that is clearer. I've added it in the attached patch.

Thank you. I've pushed the patch with a modified commit message.

Closing.

We should probably do something like this for project-eshell as well.