bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

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

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill

Hello!

This patch is directly dependent on https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41868
since I reused one of the functions added by Philip K.

For now I rely on that report to be resolved before this.

However, attached is a patch for project-switch-to-buffer, where only file-bound buffers are included. I believe the function should be kind of self explanatory.

I use this daily, but stole the buffer list function since it was better than my initial one, and seems to be accepted soon :)

I find this useful, and hope others will as well. I bind it myself to 'C-x p b'.

The 'project--list-buffers' will not be part of the final patch, but I wanted to provide it so that you have a working example.

Theodor


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

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Philip K.

Cool! Just note that in the following patch I renamed the function to
project--buffer-list, so that it looks and acts more like
buffer-list. Sorry for the inconvenience though, didn't expect anyone
to pick the function up that quickly.

--
        Philip K.




Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Dmitry Gutov
In reply to this post by Theodor Thornhill
On 15.06.2020 20:45, Theodor Thornhill wrote:
> I find this useful, and hope others will as well. I bind it myself to 'C-x p b'.

Sounds good.

> The 'project--list-buffers' will not be part of the final patch, but I wanted to provide it so that you have a working example.

I see that the code uses project--completing-read-strict. I'm not
entirely happy with that function, yet.

Have you tried calling read-buffer with a PREDICATE argument instead?
That would require extracting some code from project--list-buffers
(defined in Philip's patch), but it should be straightforward.



Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill

Hey,
"Dmitry Gutov" <[hidden email]> writes:


[...]
> I see that the code uses project--completing-read-strict. I'm not
> entirely happy with that function, yet.
Ok, I just thought I'd use something already written :)

>
> Have you tried calling read-buffer with a PREDICATE argument instead?
> That would require extracting some code from project--list-buffers
> (defined in Philip's patch), but it should be straightforward.

The attached patch works fine, and is also not reliant on Philip's patch. I omitted the DEF argument, since then I had to do the filtering beforehand, and also in the PREDICATE function. Just seems a little redundant to me, but maybe it should be there

The complete patch is a lot smaller now as well!
What do you think?

Theo


pr-switch-to-buffer.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Juri Linkov-2
In reply to this post by Theodor Thornhill
> However, attached is a patch for project-switch-to-buffer, where only
> file-bound buffers are included.  I believe the function should be
> kind of self explanatory.
>
> I find this useful, and hope others will as well. I bind it myself to 'C-x p b'.

Nice, I consider your new project-switch-to-buffer as an accompanying
command with the existing project-find-file (that could get a keybinding too,
the most natural would be 'C-x p f').



Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Dmitry Gutov
On 16.06.2020 02:10, Juri Linkov wrote:
> Nice, I consider your new project-switch-to-buffer as an accompanying
> command with the existing project-find-file (that could get a keybinding too,
> the most natural would be 'C-x p f')

Would you like to create a patch that adds a global keymap for project
related commands?

I've been putting it off myself. The question I've been considering:

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.

OTOH, if the prefix is customizable, that can also be fine.



Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill
In reply to this post by Theodor Thornhill

Thanks alot for your comments!

"Dmitry Gutov" <[hidden email]> writes:

[...]
> The REQUIRE-MATCH argument should be t, I think.
Done.

>  > +      (lambda (buffer)
>  > +        (when-let* ((path (buffer-file-name (cdr buffer)))

[...]

> Call it file or file-name.
Done.


>  > +                    (true (file-truename path)))
>
> Do we need this conversion? It'll add some runtime overhead, and I'm not
> sure in which conditions the result will be different.
I removed it, seems to work fine :).


>  > +          (when (file-in-directory-p true (project-root pr))
>  > +            'buffer)))))))
>
> Why 'buffer and not t?
I just figured it as readable, but t is there now!

Theo


project-switch-to-buffer.patch (978 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill

Hi!

"Philip K." <[hidden email]> writes:

[...]

> Shouldn't this just be (file-in-directory-p file root), without the
> (when ... t)?
Yes! Thanks a lot :)


Theo


project-switch-to-buffer.patch (946 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill
Hello!

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


[...]
> Nit: For consistency with switch-to-buffer,
> this should be "Switch to buffer: " without the hyphens.

Yes, absolutely!

>> +      (lambda (buffer)
>> +        (when-let ((file (buffer-file-name (cdr buffer))))
>
> Nit: Neither the manual nor read-buffer's docstring documents what
> (cdr buffer) is here, so a comment mentioning Vbuffer_alist or something
> along those lines would be nice.
It kind of does, though indirectly? But sure, I can add a small comment.

Documentation states that:

 "It will be called with each potential candidate, in the form of either a
string or a cons cell whose ‘car’ is a string, and should return non-nil to
accept the candidate for completion, nil otherwise."

[...]
> Otherwise LGTM, thanks,
Nice




Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill
Hello,

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

> I know, but this says nothing about the cdr.

Maybe like this?

Theo


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

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

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

> +;;;###autoload
> +(defun project-switch-to-buffer ()
> +  "Switch to a buffer in the current project."
> +  (interactive)
> +  (let ((root (project-root (project-current t))))
> +    (switch-to-buffer
> +     (read-buffer
> +      "Switch to buffer: " nil t
> +      (lambda (buffer)
> +        ;; buffer is a alist of ("filename" . #<buffer filename>)

That's not right: BUFFER is a flat cons cell, not an alist.  Please also
start sentences with a capital letter and end them with a full stop.
I'd write something like this:

  ;; BUFFER is an entry (BUF-NAME . BUF-OBJ) of Vbuffer_alist.

> +        (when-let ((file (buffer-file-name (cdr buffer))))
> +          (file-in-directory-p file root)))))))

Thanks,

--
Basil



Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill

Hi!
Thanks for your keen eye :)

> I'd write something like this:
>
>   ;; BUFFER is an entry (BUF-NAME . BUF-OBJ) of Vbuffer_alist.
>

Got it. I stole your comment - I hope that is OK?


Theo

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

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Dmitry Gutov
In reply to this post by Basil L. Contovounesios
On 16.06.2020 21:19, Basil L. Contovounesios wrote:
>    ;; BUFFER is an entry (BUF-NAME . BUF-OBJ) of Vbuffer_alist.

I don't think we can/should refer to C level variables from Lisp
documentation.



Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

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

> On 16.06.2020 21:19, Basil L. Contovounesios wrote:
>>    ;; BUFFER is an entry (BUF-NAME . BUF-OBJ) of Vbuffer_alist.
>
> I don't think we can/should refer to C level variables from Lisp documentation.

This is a source comment, not public documentation, and the relevant
code relies on an undocumented C-level implementation detail (the use of
Vbuffer_alist as a completion collection in read-buffer) so I think the
comment (or something along those lines; feel free to suggest
improvements) is appropriate.

--
Basil



Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Dmitry Gutov
In reply to this post by Theodor Thornhill
On 16.06.2020 21:58, Theodor Thornhill wrote:
> Got it. I stole your comment - I hope that is OK?

Also installed. Thanks!



Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill
In reply to this post by Theodor Thornhill

> Hi Theodor,
Hi!

> Can you please tell why only file-bound buffers are included?  I have
> some buffers with inferior python process in each project, would be nice
> to switch to them with project-switch-to-buffer.

Well, I tried a few different ways of including other buffers as well, but emacs seemed to want to include earlier used minibuffers and all sorts of 'junk'.  I agree that these inferior processes should be interesting to include, though.  I can try to find some way to filter out the cruft a bit better!  Do you have a specific idea in mind to go from?  

> Also consider to provide default value like:
>
>     (buffer-name (other-buffer (current-buffer)))
>
> This way one can switch to recent project buffer with just RET, same way
> as switch-to-buffer.

This seems smart! I'll look into that as well :) Thanks!

Theo




Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill
In reply to this post by Theodor Thornhill

On the other hand, what do you think about this one?

Now you will get buffers such as '*xref*' and I assume your inferior buffers as well. Could you try this? I added your 'other-buffer' thing also for good measure :)

Theo


project-switch-to-buffer.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Theodor Thornhill
In reply to this post by Theodor Thornhill

That was a lousy diff, sorry...

Theo


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

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Andrii Kolomoiets
Theodor Thornhill <[hidden email]> writes:

> That was a lousy diff, sorry...

Much better!

Few more things:

- Check if other-buffer is passes the predicate function.  The buffer
  from another project must not be the default.
- Exclude the current buffer from the buffers list to switch to.

Thanks!



Reply | Threaded
Open this post in threaded view
|

bug#41879: 28.0.50; [Patch]: Add project-switch-to-buffer in project.el

Andrii Kolomoiets
Dmitry Gutov <[hidden email]> writes:

> On 18.06.2020 13:48, Andrii Kolomoiets wrote:
>> Much better!
>> Few more things:
>> - Check if other-buffer is passes the predicate function.  The
>> buffer
>>    from another project must not be the default.
>> - Exclude the current buffer from the buffers list to switch to.
>
> Perhaps a good idea is to follow the example of
> internal-complete-buffer-except (called from read-buffer-to-switch).

Isn't it enough to check `(not (eq (cdr buffer) (current-buffer)))` in
the predicate function?



12