bug#39722: Support for bookmark.el in VC directory buffers

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

bug#39722: Support for bookmark.el in VC directory buffers

Matthias Meulien-2
Being able to bookmark VC directory buffers is useful when working on
multiple projects; The proposed patch implements this on the lines of
what I read in info.el.
--
Matthias

0001-Bookmark-locations-can-refer-to-VC-directory-buffers.patch (4K) Download Attachment
signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#39722: Support for bookmark.el in VC directory buffers

Juri Linkov-2
> Being able to bookmark VC directory buffers is useful when working on
> multiple projects; The proposed patch implements this on the lines of
> what I read in info.el.

Thanks, this is a useful addition.  Please push it after fixing minor
details:

> +(defun vc-dir-bookmark-make-record ()
> +  "This implements the `bookmark-make-record-function' type for
> +`vc-dir' buffers."
> ...
> +(defun vc-dir-bookmark-jump (bmk)
> +  "This implements the `handler' function interface for the record
> +type returned by `vc-dir-bookmark-make-record'."

According to the documentation standard described in
(info "(elisp) Documentation Tips")
the first line of the docstring should consist of a complete sentence.

I see that you copied the docstring from Info-bookmark-jump,
but the docstrings of info.el contains a mistake
that should be fixed in info.el.



Reply | Threaded
Open this post in threaded view
|

bug#39722: Support for bookmark.el in VC directory buffers

Juri Linkov-2
tags 39722 fixed
close 39722 28.0.50
thanks

>> (...) Thanks, this is a useful addition. Please push it after fixing
>> minor details:
>
> Thanks Juri for your remark. I updated the patch. Unfortunately I am not
> authorized to push.

Thanks for the patch, I pushed it to master.



Reply | Threaded
Open this post in threaded view
|

bug#39722: Support for bookmark.el in VC directory buffers

Juri Linkov-2
In reply to this post by Matthias Meulien-2
> Being able to bookmark VC directory buffers is useful when working on
> multiple projects; The proposed patch implements this on the lines of
> what I read in info.el.

> +(defun vc-dir-bookmark-jump (bmk)
> +  "This implements the `handler' function interface for the record
> +type returned by `vc-dir-bookmark-make-record'."
> +  (let* ((file (bookmark-prop-get bmk 'filename))
> +         (buf (save-window-excursion
> +                 (vc-dir file) (current-buffer))))
> +    (bookmark-default-handler
> +     `("" (buffer . ,buf) . ,(bookmark-get-bookmark-record bmk)))))

While using the new keybinding 'C-x t t' from bug#41691
(instead of adding a new command bookmark-jump-other-tab)
I noticed that vc-dir-bookmark-jump doesn't work with it nicely -
it creates two tabs instead of one new tab.  This is because
'C-x t t' modifies display-buffer-overriding-action
with a function that creates a new tab, but the problem is that
visiting a bookmark with a vc-dir buffer calls display-buffer
*twice*.  A new tab is created for every display-buffer call.

I see that you tried to implement a workaround for this problem
by using (save-window-excursion (vc-dir file) that handles some cases.

Another workaround could fix the tab problem:

diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index a86c37c24a..1c5be268f4 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -1496,8 +1496,10 @@ vc-dir-bookmark-jump
 This implements the `handler' function interface for the record
 type returned by `vc-dir-bookmark-make-record'."
   (let* ((file (bookmark-prop-get bmk 'filename))
+         (display-buffer-overriding-action '(nil))
          (buf (save-window-excursion
-                 (vc-dir file) (current-buffer))))
+                (vc-dir file)
+                (current-buffer))))
     (bookmark-default-handler
      `("" (buffer . ,buf) . ,(bookmark-get-bookmark-record bmk)))))

But I wonder what could be a proper solution?

The first call of pop-to-buffer is in 'vc-dir':

  (let (pop-up-windows)      ; based on cvs-examine; bug#6204
    (pop-to-buffer (vc-dir-prepare-status-buffer "*vc-dir*" dir backend)))

(Maybe there should be a separate function 'vc-dir-no-select'
 that doesn't call pop-to-buffer?)

The second call is in bookmark-jump with:

  (bookmark--jump-via bookmark (or display-func 'pop-to-buffer-same-window))

Also in bookmark-bmenu-other-window:

    (bookmark--jump-via bookmark 'switch-to-buffer-other-window)

And in bookmark-bmenu-switch-other-window:

  (let ((bookmark (bookmark-bmenu-bookmark))
        (fun (lambda (b) (display-buffer b t))))
    (bookmark--jump-via bookmark fun))



Reply | Threaded
Open this post in threaded view
|

bug#39722: Support for bookmark.el in VC directory buffers

Juri Linkov-2
> But I wonder what could be a proper solution?

I still have no idea how to avoid double call of display-buffer,
but at least here is the patch that avoids creating two tabs.
It creates a new tab only for the first call of display-buffer,
and resets display-buffer-overriding-action afterwards:


diff --git a/lisp/window.el b/lisp/window.el
index c047150413..2249b8d1d1 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8590,15 +8590,18 @@ display-buffer-override-next-command
   (let* ((old-window (or (minibuffer-selected-window) (selected-window)))
          (new-window nil)
          (minibuffer-depth (minibuffer-depth))
+         (clearfun (make-symbol "clear-display-buffer-overriding-action"))
          (action (lambda (buffer alist)
                    (unless (> (minibuffer-depth) minibuffer-depth)
                      (let* ((ret (funcall pre-function buffer alist))
                             (window (car ret))
                             (type (cdr ret)))
                        (setq new-window (window--display-buffer buffer window
-                                                                type alist))))))
+                                                                type alist))
+                       (funcall clearfun)
+                       (setq post-function nil)
+                       new-window))))
          (command this-command)
-         (clearfun (make-symbol "clear-display-buffer-overriding-action"))
          (exitfun
           (lambda ()
             (setq display-buffer-overriding-action
diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index a86c37c24a..e9ec22c86e 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -1496,8 +1496,9 @@ vc-dir-bookmark-jump
 This implements the `handler' function interface for the record
 type returned by `vc-dir-bookmark-make-record'."
   (let* ((file (bookmark-prop-get bmk 'filename))
-         (buf (save-window-excursion
-                 (vc-dir file) (current-buffer))))
+         (buf (progn
+                (vc-dir file)
+                (current-buffer))))
     (bookmark-default-handler
      `("" (buffer . ,buf) . ,(bookmark-get-bookmark-record bmk)))))
 
Reply | Threaded
Open this post in threaded view
|

bug#39722: Support for bookmark.el in VC directory buffers

Juri Linkov-2
>> But I wonder what could be a proper solution?
>
> I still have no idea how to avoid double call of display-buffer,
> but at least here is the patch that avoids creating two tabs.
> It creates a new tab only for the first call of display-buffer,
> and resets display-buffer-overriding-action afterwards:

Now pushed to master.