bug#20150: 24.4: bookmarks: return from editing annotations (FIX INCLUDED)

[Stefan Kangas <stefan@marxist.se>]

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

Stefan Kangas <stefan@marxist.se> writes:
> > > When completing a bookmark annotation edit, the point on the bmenu-list
> > > would move to point-max and the selected window would be wherever the
> > > annotation buffer had been displayed.
> >
> > I can confirm this bug, and suggest the attached fix.
>
> Please see this somewhat improved patch instead.

Now that the test suite for bookmark.el has hit master, I reworked
this to also include tests.  Please find attached an updated patch.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Restore-focus-to-Bookmark-List-after-editing-annotat-3.patch --]
[-- Type: application/octet-stream, Size: 5873 bytes --]

From 795ff94c63530fe7a22fa88c25b53fe51f5e6758 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Tue, 2 Jul 2019 03:06:37 +0200
Subject: [PATCH] Restore focus to Bookmark List after editing annotation

* lisp/bookmark.el (bookmark-send-edited-annotation): Restore focus to
Bookmark List, quit window and move point back to edited
bookmark. (Bug#20150)

(text-property-search): Require.
(bookmark-annotation-name): Improve docstring.
(bookmark-bmenu-buffer): New variable.
(bookmark-bmenu-surreptitiously-rebuild-list)
(bookmark-bmenu-list): Use it.

* test/lisp/bookmark-tests.el (with-bookmark-bmenu-test): New macro.
(bookmark-benu-edit-annotation/show-annotation)
(bookmark-bmenu-send-edited-annotation)
(bookmark-bmenu-send-edited-annotation/restore-focus): New test cases.
---
 lisp/bookmark.el            | 29 ++++++++++++++++++-----------
 test/lisp/bookmark-tests.el | 36 +++++++++++++++++++++++++++++++++++-
 2 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 92d0da3594..d3d631067b 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -32,6 +32,7 @@
 ;;; Code:
 
 (require 'pp)
+(require 'text-property-search)
 (eval-when-compile (require 'cl-lib))
 
 ;;; Misc comments:
@@ -122,6 +123,9 @@ bookmark-automatically-show-annotations
   "Non-nil means show annotations when jumping to a bookmark."
   :type 'boolean)
 
+(defconst bookmark-bmenu-buffer "*Bookmark List*"
+  "Name of buffer used for Bookmark List.")
+
 (defcustom bookmark-bmenu-use-header-line t
   "Non-nil means to use an immovable header line.
 This is as opposed to inline text at the top of the buffer."
@@ -882,11 +886,9 @@ bookmark-kill-line
       (delete-char 1))))
 
 
-;; Defvars to avoid compilation warnings:
 (defvar bookmark-annotation-name nil
-  "Variable holding the name of the bookmark.
-This is used in `bookmark-edit-annotation' to record the bookmark
-whose annotation is being edited.")
+  "Name of bookmark whose annotation is being edited.
+This is used by `bookmark-edit-annotation-mode'.")
 
 
 (defun bookmark-default-annotation-text (bookmark-name)
@@ -947,12 +949,18 @@ bookmark-send-edited-annotation
       (forward-line 1)))
   ;; Take no chances with text properties.
   (let ((annotation (buffer-substring-no-properties (point-min) (point-max)))
-	(bookmark-name bookmark-annotation-name))
+        (bookmark-name bookmark-annotation-name)
+        (old-buffer (current-buffer)))
     (bookmark-set-annotation bookmark-name annotation)
     (setq bookmark-alist-modification-count
           (1+ bookmark-alist-modification-count))
-    (bookmark-bmenu-surreptitiously-rebuild-list))
-  (kill-buffer (current-buffer)))
+    (quit-window)
+    (pop-to-buffer (get-buffer bookmark-bmenu-buffer))
+    (bookmark-bmenu-list)
+    (goto-char (point-min))
+    (text-property-search-forward 'bookmark-name-prop bookmark-name)
+    (message "Annotation updated for \"%s\"" bookmark-name)
+    (kill-buffer old-buffer)))
 
 
 (defun bookmark-edit-annotation (bookmark-name-or-record)
@@ -1547,10 +1555,9 @@ bookmark-load
 	(progress-reporter-done reporter)))))
 
 \f
-;;; Code supporting the dired-like bookmark menu.
+;;; Code supporting the dired-like bookmark list.
 ;; Prefix is "bookmark-bmenu" for "buffer-menu":
 
-
 (defvar bookmark-bmenu-hidden-bookmarks ())
 
 
@@ -1634,7 +1641,7 @@ bookmark-bmenu-mode-map
 (defun bookmark-bmenu-surreptitiously-rebuild-list ()
   "Rebuild the Bookmark List if it exists.
 Don't affect the buffer ring order."
-  (if (get-buffer "*Bookmark List*")
+  (if (get-buffer bookmark-bmenu-buffer)
       (save-excursion
         (save-window-excursion
           (bookmark-bmenu-list)))))
@@ -1648,7 +1655,7 @@ bookmark-bmenu-list
 deletion, or > if it is flagged for displaying."
   (interactive)
   (bookmark-maybe-load-default-file)
-  (let ((buf (get-buffer-create "*Bookmark List*")))
+  (let ((buf (get-buffer-create bookmark-bmenu-buffer)))
     (if (called-interactively-p 'interactive)
         (switch-to-buffer buf)
       (set-buffer buf)))
diff --git a/test/lisp/bookmark-tests.el b/test/lisp/bookmark-tests.el
index 6fa0b1e98d..a8f75b9c61 100644
--- a/test/lisp/bookmark-tests.el
+++ b/test/lisp/bookmark-tests.el
@@ -313,7 +313,41 @@ bookmark-tests-load
                     (list bookmark-tests-bookmark
                           (cons "name<2>" (cdr bookmark-tests-bookmark))))))))
 
-;; TODO: Add tests for bookmark-bmenu.
+;; TODO: Add more tests for bookmark-bmenu.
+
+(defmacro with-bookmark-bmenu-test (&rest body)
+  "Create environment for testing `bookmark-bmenu-list' and evaluate BODY.
+Same as `with-bookmark-test' but with additions suitable for
+testing `bookmark-bmenu-list'."
+  `(with-bookmark-test
+    (let ((bookmark-bmenu-buffer "*Bookmark List - Testing*"))
+      (unwind-protect
+          (save-window-excursion
+            (bookmark-bmenu-list)
+            ,@body)
+        (kill-buffer bookmark-bmenu-buffer)))))
+
+(ert-deftest bookmark-benu-edit-annotation/show-annotation ()
+  (with-bookmark-bmenu-test
+   (bookmark-set-annotation "name" "foo")
+   (bookmark-bmenu-edit-annotation)
+   (should (string-match "foo" (buffer-string)))))
+
+(ert-deftest bookmark-bmenu-send-edited-annotation ()
+  (with-bookmark-bmenu-test
+   (bookmark-bmenu-edit-annotation)
+   (insert "foo")
+   (bookmark-send-edited-annotation)
+   (should (equal (bookmark-get-annotation "name") "foo"))))
+
+(ert-deftest bookmark-bmenu-send-edited-annotation/restore-focus ()
+  "Test for https://debbugs.gnu.org/20150 ."
+  (with-bookmark-bmenu-test
+   (bookmark-bmenu-edit-annotation)
+   (insert "foo")
+   (bookmark-send-edited-annotation)
+   (should (equal (buffer-name (current-buffer)) bookmark-bmenu-buffer))
+   (should (looking-at "name"))))
 
 (provide 'bookmark-tests)
 ;;; bookmark-tests.el ends here
-- 
2.21.0