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

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

[Matthias Meulien]

[-- Attachment #1.1: Type: text/plain, Size: 182 bytes --]

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

[-- Attachment #1.2: 0001-Bookmark-locations-can-refer-to-VC-directory-buffers.patch --]
[-- Type: text/x-patch, Size: 3155 bytes --]

From 13dbe902e95ccd812e490aa7b71daed67954a850 Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Fri, 21 Feb 2020 22:13:48 +0100
Subject: [PATCH] Bookmark locations can refer to VC directory buffers

* etc/NEWS: Document feature
* lisp/vc/vc-dir.el (vc-dir-mode): Set local bookmark-make-record-function
(bookmark-make-record-default)
(bookmark-prop-get)
(bookmark-default-handler)
(bookmark-get-bookmark-record): Declarations
(vc-dir-bookmark-make-record): Implement bookmark-make-record-function
(vc-dir-bookmark-jump): Implement a bookmark handler for vc-dir buffers
---
 etc/NEWS          |  5 +++++
 lisp/vc/vc-dir.el | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 0279879836..40f4c78191 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -159,6 +159,11 @@ key             binding
 / v             package-menu-filter-by-version
 / /             package-menu-filter-clear
 
+** vc-dir.el
+
+*** Support for bookmark.el.
+Bookmark locations can refer to VC directory buffers.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 033cb27e33..33ad6243c6 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -1061,6 +1061,7 @@ vc-dir-mode
   (set (make-local-variable 'vc-dir-backend) use-vc-backend)
   (set (make-local-variable 'desktop-save-buffer)
        'vc-dir-desktop-buffer-misc-data)
+  (setq-local bookmark-make-record-function #'vc-dir-bookmark-make-record)
   (setq buffer-read-only t)
   (when (boundp 'tool-bar-map)
     (set (make-local-variable 'tool-bar-map) vc-dir-tool-bar-map))
@@ -1410,6 +1411,39 @@ vc-dir-restore-desktop-buffer
 	     '(vc-dir-mode . vc-dir-restore-desktop-buffer))
 
 \f
+;;; Support for bookmark.el (adapted from what info.el does).
+
+(declare-function bookmark-make-record-default
+                  "bookmark" (&optional no-file no-context posn))
+(declare-function bookmark-prop-get "bookmark" (bookmark prop))
+(declare-function bookmark-default-handler "bookmark" (bmk))
+(declare-function bookmark-get-bookmark-record "bookmark" (bmk))
+
+(defun vc-dir-bookmark-make-record ()
+  "This implements the `bookmark-make-record-function' type for
+`vc-dir' buffers."
+  (let* ((bookmark-name
+          (concat "(" (symbol-name vc-dir-backend) ") "
+                  (file-name-nondirectory
+                   (directory-file-name default-directory))))
+         (defaults (list bookmark-name default-directory)))
+    `(,bookmark-name
+      ,@(bookmark-make-record-default 'no-file)
+      (filename . ,default-directory)
+      (handler . vc-dir-bookmark-jump)
+      (defaults . ,defaults))))
+
+;;;###autoload
+(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)))))
+
+\f
 (provide 'vc-dir)
 
 ;;; vc-dir.el ends here
-- 
2.20.1


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Juri Linkov]

> 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.

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

[Matthias Meulien]

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

> (...) 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.

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

I'll keep this in mind.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Bookmark-locations-can-refer-to-VC-directory-buffers.patch --]
[-- Type: text/x-diff, Size: 3342 bytes --]

From 4d4d3e819dc849268e7b3a15ea0d2390c337c23c Mon Sep 17 00:00:00 2001
From: Matthias Meulien <orontee@gmail.com>
Date: Fri, 21 Feb 2020 22:13:48 +0100
Subject: [PATCH] Bookmark locations can refer to VC directory buffers

* etc/NEWS: Document feature
* lisp/vc/vc-dir.el (vc-dir-mode): Set local bookmark-make-record-function
(bookmark-make-record-default)
(bookmark-prop-get)
(bookmark-default-handler)
(bookmark-get-bookmark-record): Declarations
(vc-dir-bookmark-make-record): Make record used to bookmark a `vc-dir' buffer.
(vc-dir-bookmark-jump): Provides the bookmark-jump behavior for a `vc-dir' buffer.
---
 etc/NEWS          |  4 ++++
 lisp/vc/vc-dir.el | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index ae676a9bf8..01c584833b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -348,6 +348,10 @@ symbol property to the browsing functions.  With a new command
 'browse-url-with-browser-kind', an URL can explicitly be browsed with
 either an internal or external browser.
 
+** vc-dir.el
+
+*** Support for bookmark.el.
+Bookmark locations can refer to VC directory buffers.
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/vc/vc-dir.el b/lisp/vc/vc-dir.el
index 0c9e656add..a86c37c24a 100644
--- a/lisp/vc/vc-dir.el
+++ b/lisp/vc/vc-dir.el
@@ -1106,6 +1106,7 @@ vc-dir-mode
   (set (make-local-variable 'vc-dir-backend) use-vc-backend)
   (set (make-local-variable 'desktop-save-buffer)
        'vc-dir-desktop-buffer-misc-data)
+  (setq-local bookmark-make-record-function #'vc-dir-bookmark-make-record)
   (setq buffer-read-only t)
   (when (boundp 'tool-bar-map)
     (set (make-local-variable 'tool-bar-map) vc-dir-tool-bar-map))
@@ -1466,6 +1467,41 @@ vc-dir-restore-desktop-buffer
 	     '(vc-dir-mode . vc-dir-restore-desktop-buffer))
 
 \f
+;;; Support for bookmark.el (adapted from what info.el does).
+
+(declare-function bookmark-make-record-default
+                  "bookmark" (&optional no-file no-context posn))
+(declare-function bookmark-prop-get "bookmark" (bookmark prop))
+(declare-function bookmark-default-handler "bookmark" (bmk))
+(declare-function bookmark-get-bookmark-record "bookmark" (bmk))
+
+(defun vc-dir-bookmark-make-record ()
+  "Make record used to bookmark a `vc-dir' buffer.
+This implements the `bookmark-make-record-function' type for
+`vc-dir' buffers."
+  (let* ((bookmark-name
+          (concat "(" (symbol-name vc-dir-backend) ") "
+                  (file-name-nondirectory
+                   (directory-file-name default-directory))))
+         (defaults (list bookmark-name default-directory)))
+    `(,bookmark-name
+      ,@(bookmark-make-record-default 'no-file)
+      (filename . ,default-directory)
+      (handler . vc-dir-bookmark-jump)
+      (defaults . ,defaults))))
+
+;;;###autoload
+(defun vc-dir-bookmark-jump (bmk)
+  "Provides the bookmark-jump behavior for a `vc-dir' buffer.
+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)))))
+
+\f
 (provide 'vc-dir)
 
 ;;; vc-dir.el ends here
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 22 bytes --]


-- 
Matthias Meulien

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

[Juri Linkov]

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.

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

[Juri Linkov]

> 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))

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

[Juri Linkov]

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

> 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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: debounce-display-buffer-override-next-command.patch --]
[-- Type: text/x-diff, Size: 1883 bytes --]

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)))))
 

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

[Juri Linkov]

>> 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.