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

[Stefan Kangas <stefan@marxist.se>]

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

Stefan Kangas <stefan@marxist.se> writes:
> Now that the test suite for bookmark.el has hit master, I reworked
> this to also include tests.  Please find attached an updated patch.

I thought about this a bit more and realized there is a problem with
the fix: we can of course also edit an annotation when
bookmark-use-annotations is t.  In this case we would not want to
return to the bookmark list.

The fix will therefore have to be a bit more involved to handle both
cases correctly.  I have attached a fourth (and hopefully final) patch
here.

I also ran into a suspected bug in with-current-buffer (Bug#36497).  I
added a FIXME in one of the test cases so we can clean it up once
that's fixed.

This is my first patch with this many changes, and I did my best to
document it thoroughly.  Not sure if we like that or prefer something
more terse.  Comments would be much appreciated.

Thanks,
Stefan Kangas

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

From f632b049e6037c9beb40f05927f24d7fe6414431 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

There are two entry points to bookmark-edit-annotation-mode: the first
when we add a bookmark and bookmark-use-annotations is non-nil; the
second when bookmark-bmenu-edit-annotation is run from the bookmark
list.  When editing is concluded, in the first case, we should just
quit window.  In the second case, we should instead return to the
bookmark list.

* lisp/bookmark.el (text-property-search): Require.
(bookmark-annotation--name): Rename from bookmark-annotation-name,
make buffer-local and improve doc string.
(bookmark-annotation-name): Mark as obsolete variable alias for
bookmark-annotation--name.
(bookmark-annotation--from-bookmark-list): New buffer-local variable.
(bookmark-edit-annotation): Use above renamed variable.  New argument
from-bookmark-list sets bookmark-annotation--from-bookmark-list.
(bookmark-bmenu-edit-annotation): Call bookmark-edit-annotation with
argument from-bookmark-list set to t.
(bookmark-send-edited-annotation): When editing originated in the
bookmark list, restore focus to bookmark list and move point back to
edited bookmark. (Bug#20150)

(bookmark-edit-annotation-mode): Fix typo.
(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-tests-set/bookmark-use-annotations-t)
(bookmark-bmenu-edit-annotation/show-annotation)
(bookmark-bmenu-send-edited-annotation)
(bookmark-bmenu-send-edited-annotation/restore-focus): New test cases.
---
 lisp/bookmark.el            | 53 ++++++++++++++++++++++++-------------
 test/lisp/bookmark-tests.el | 50 +++++++++++++++++++++++++++++++++-
 2 files changed, 83 insertions(+), 20 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 92d0da3594..9351a9012c 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."
@@ -881,13 +885,15 @@ bookmark-kill-line
     (when (and newline-too (= (following-char) ?\n))
       (delete-char 1))))
 
+(define-obsolete-variable-alias 'bookmark-annotation-name
+  'bookmark-annotation--name "27.1")
+(defvar bookmark-annotation--name nil
+  "Name of bookmark under edit in `bookmark-edit-annotation-mode'.")
+(make-variable-buffer-local 'bookmark-annotation--name)
 
-;; 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.")
-
+(defvar bookmark-annotation--from-bookmark-list nil
+  "If non-nil, return to bookmark list from `bookmark-edit-annotation-mode'.")
+(make-variable-buffer-local 'bookmark-annotation--from-bookmark-list)
 
 (defun bookmark-default-annotation-text (bookmark-name)
   "Return default annotation text for BOOKMARK-NAME.
@@ -929,7 +935,7 @@ bookmark-insert-annotation
 (define-derived-mode bookmark-edit-annotation-mode
   text-mode "Edit Bookmark Annotation"
   "Mode for editing the annotation of bookmarks.
-When you have finished composing, type \\[bookmark-send-annotation].
+When you have finished composing, type \\[bookmark-send-edited-annotation].
 
 \\{bookmark-edit-annotation-mode-map}")
 
@@ -947,21 +953,31 @@ 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)
+        (from-bookmark-list bookmark-annotation--from-bookmark-list)
+        (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)))
+    (message "Annotation updated for \"%s\"" bookmark-name)
+    (quit-window)
+    (bookmark-bmenu-surreptitiously-rebuild-list)
+    (when from-bookmark-list
+      (pop-to-buffer (get-buffer bookmark-bmenu-buffer))
+      (goto-char (point-min))
+      (text-property-search-forward 'bookmark-name-prop bookmark-name))
+    (kill-buffer old-buffer)))
 
 
-(defun bookmark-edit-annotation (bookmark-name-or-record)
-  "Pop up a buffer for editing bookmark BOOKMARK-NAME-OR-RECORD's annotation."
+(defun bookmark-edit-annotation (bookmark-name-or-record &optional from-bookmark-list)
+  "Pop up a buffer for editing bookmark BOOKMARK-NAME-OR-RECORD's annotation.
+If optional argument FROM-BOOKMARK-LIST is non-nil, return to the
+bookmark list when editing is done."
   (pop-to-buffer (generate-new-buffer-name "*Bookmark Annotation Compose*"))
   (bookmark-insert-annotation bookmark-name-or-record)
   (bookmark-edit-annotation-mode)
-  (set (make-local-variable 'bookmark-annotation-name)
-       bookmark-name-or-record))
+  (setq bookmark-annotation--from-bookmark-list from-bookmark-list)
+  (setq bookmark-annotation--name bookmark-name-or-record))
 
 
 (defun bookmark-buffer-name ()
@@ -1547,10 +1563,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 +1649,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 +1663,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)))
@@ -2051,7 +2066,7 @@ bookmark-bmenu-edit-annotation
   "Edit the annotation for the current bookmark in another window."
   (interactive)
   (let ((bookmark (bookmark-bmenu-bookmark)))
-    (bookmark-edit-annotation bookmark)))
+    (bookmark-edit-annotation bookmark t)))
 
 
 (defun bookmark-bmenu-unmark (&optional backup)
diff --git a/test/lisp/bookmark-tests.el b/test/lisp/bookmark-tests.el
index 6fa0b1e98d..c7389f3643 100644
--- a/test/lisp/bookmark-tests.el
+++ b/test/lisp/bookmark-tests.el
@@ -186,6 +186,19 @@ bookmark-tests-set
        ;; 3. bookmark-set-internal
        (should-error (bookmark-set-internal "foo" "bar" t))))))
 
+(ert-deftest bookmark-tests-set/bookmark-use-annotations-t ()
+  (with-bookmark-test-file
+   (let ((bookmark-use-annotations t))
+     ;; FIXME: Can't use with-current-buffer due to Bug#36497.
+     (save-window-excursion
+       (switch-to-buffer buffer)
+       ;; Should jump to edit annotation buffer
+       (bookmark-set "foo")
+       (should (equal major-mode 'bookmark-edit-annotation-mode))
+       ;; Should return to the original buffer
+       (bookmark-send-edited-annotation)
+       (should (equal (current-buffer) buffer))))))
+
 (ert-deftest bookmark-tests-kill-line ()
   (with-temp-buffer
     (insert "foobar\n")
@@ -313,7 +326,42 @@ 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-bmenu.enu-edit-annotation/show-annotation ()
+  (with-bookmark-bmenu-test
+   (bookmark-set-annotation "name" "foo")
+   (bookmark-bmenu-edit-annotation)
+   (should (string-match "foo" (buffer-string)))
+   (kill-buffer (current-buffer))))
+
+(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