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

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

[Boruch Baum]

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

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.

The fix restores focus to the bmenu-list window, and restores point to
where it had been prior to editing the annotation.

(defun bookmark-send-edited-annotation ()
  "Use buffer contents as annotation for a bookmark.
Lines beginning with `#' are ignored."
  (interactive)
  (if (not (derived-mode-p 'bookmark-edit-annotation-mode))
      (error "Not in bookmark-edit-annotation-mode"))
  (goto-char (point-min))
  (while (< (point) (point-max))
    (if (looking-at "^#")
        (bookmark-kill-line t)
      (forward-line 1)))
  ;; Take no chances with text properties.
  (let
    ((annotation (buffer-substring-no-properties (point-min) (point-max)))
	 (bookmark-name bookmark-annotation-name)
     (return-line
       (with-current-buffer "*Bookmark List*"
         (line-number-at-pos)))
     (return-column
       (with-current-buffer "*Bookmark List*"
         (current-column))))
    (bookmark-set-annotation bookmark-name annotation)
    (setq bookmark-alist-modification-count
          (1+ bookmark-alist-modification-count))
    (kill-buffer (current-buffer))
    (pop-to-buffer (get-buffer "*Bookmark List*"))
    (bookmark-bmenu-list)
    (goto-char (point-min))
    (forward-line (1- return-line))
    (forward-char return-column)))

-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0


[-- Attachment #1.2: attachment --]
[-- Type: text/plain, Size: 1124 bytes --]

(defun bookmark-send-edited-annotation ()
  "Use buffer contents as annotation for a bookmark.
Lines beginning with `#' are ignored."
  (interactive)
  (if (not (derived-mode-p 'bookmark-edit-annotation-mode))
      (error "Not in bookmark-edit-annotation-mode"))
  (goto-char (point-min))
  (while (< (point) (point-max))
    (if (looking-at "^#")
        (bookmark-kill-line t)
      (forward-line 1)))
  ;; Take no chances with text properties.
  (let
    ((annotation (buffer-substring-no-properties (point-min) (point-max)))
	 (bookmark-name bookmark-annotation-name)
     (return-line
       (with-current-buffer "*Bookmark List*"
         (line-number-at-pos)))
     (return-column
       (with-current-buffer "*Bookmark List*"
         (current-column))))
    (bookmark-set-annotation bookmark-name annotation)
    (setq bookmark-alist-modification-count
          (1+ bookmark-alist-modification-count))
    (kill-buffer (current-buffer))
    (pop-to-buffer (get-buffer "*Bookmark List*"))
    (bookmark-bmenu-list)
    (goto-char (point-min))
    (forward-line (1- return-line))
    (forward-char return-column)))

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Stefan Monnier]

> The fix restores focus to the bmenu-list window, and restores point to
> where it had been prior to editing the annotation.

Could you send it as a patch, otherwise I can't really see what's different.


        Stefan

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

[Boruch Baum]

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

Pleasure.

On 03/20/2015 10:28 AM, Stefan Monnier wrote:
>> The fix restores focus to the bmenu-list window, and restores point to
>> where it had been prior to editing the annotation.
> 
> Could you send it as a patch, otherwise I can't really see what's different.
> 
> 
>         Stefan
> 


-- 
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: emacs_bug_20150.patch --]
[-- Type: text/x-patch; name="emacs_bug_20150.patch", Size: 1230 bytes --]

--- bookmark.el	2015-03-20 12:08:33.477496667 -0400
+++ bookmark-new.el	2015-03-20 12:09:49.401494206 -0400
@@ -889,13 +889,24 @@
         (bookmark-kill-line t)
       (forward-line 1)))
   ;; Take no chances with text properties.
-  (let ((annotation (buffer-substring-no-properties (point-min) (point-max)))
-	(bookmark-name bookmark-annotation-name))
+  (let
+    ((annotation (buffer-substring-no-properties (point-min) (point-max)))
+	 (bookmark-name bookmark-annotation-name)
+     (return-line
+       (with-current-buffer "*Bookmark List*"
+         (line-number-at-pos)))
+     (return-column
+       (with-current-buffer "*Bookmark List*"
+         (current-column))))
     (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)))
+    (kill-buffer (current-buffer))
+    (pop-to-buffer (get-buffer "*Bookmark List*"))
+    (bookmark-bmenu-list)
+    (goto-char (point-min))
+    (forward-line (1- return-line))
+    (forward-char return-column)))
 
 
 (defun bookmark-edit-annotation (bookmark-name-or-record)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Stefan Kangas]

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

Boruch Baum <boruch_baum@gmx.com> 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.

Thanks,
Stefan Kangas

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

From 4fd37de42f709781c89a3b0db9f453f5d717e180 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 after editing the annotation.  Also take care to delete
window when needed, and move point back to bookmark. (Bug#20150)

(bookmark-annotation-name): Improve docstring.
(bookmark-bmenu-buffer): New variable.
(bookmark-bmenu-surreptitiously-rebuild-list)
(bookmark-bmenu-list): Use it.
---
 lisp/bookmark.el | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index bbef0a927d..e6d5212901 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -900,11 +900,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)
@@ -963,12 +961,24 @@ 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)
+        (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)))
+    ;; If the current window was only ever used for editing the
+    ;; annotation, delete it.
+    (when (not (window-prev-buffers))
+      (delete-window))
+    ;; Restore focus to Bookmark List.
+    (pop-to-buffer (get-buffer bookmark-bmenu-buffer))
+    (bookmark-bmenu-list)
+    (goto-char (point-min))
+    ;; Restore point to the bookmark we just edited.
+    (unless (text-property-search-forward 'bookmark-name-prop bookmark-name)
+      (goto-char (point-min)))
+    (message "Set annotation for \"%s\"" bookmark-name)
+    (kill-buffer buffer)))
 
 
 (defun bookmark-edit-annotation (bookmark-name-or-record)
@@ -1563,9 +1573,11 @@ 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":
 
+(defconst bookmark-bmenu-buffer "*Bookmark List*"
+  "Name of buffer used for Bookmark List.")
 
 (defvar bookmark-bmenu-hidden-bookmarks ())
 
@@ -1650,7 +1662,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)))))
@@ -1664,7 +1676,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)))
-- 
2.21.0

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

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 365 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.

Thanks,
Stefan Kangas

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

From feb894ae1016c2a0735b58f0dabf11e5f750d75b 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 after editing the annotation.  Also take care to delete
window when needed, and move point back to bookmark. (Bug#20150)

(bookmark-annotation-name): Improve docstring.
(bookmark-bmenu-buffer): New variable.
(bookmark-bmenu-surreptitiously-rebuild-list)
(bookmark-bmenu-list): Use it.
---
 lisp/bookmark.el | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index bbef0a927d..faf7d5696e 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -900,11 +900,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)
@@ -963,12 +961,19 @@ 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))
+    (unless (text-property-search-forward 'bookmark-name-prop bookmark-name)
+      (goto-char (point-min)))
+    (message "Set annotation for \"%s\"" bookmark-name)
+    (kill-buffer old-buffer)))
 
 
 (defun bookmark-edit-annotation (bookmark-name-or-record)
@@ -1563,9 +1568,11 @@ 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":
 
+(defconst bookmark-bmenu-buffer "*Bookmark List*"
+  "Name of buffer used for Bookmark List.")
 
 (defvar bookmark-bmenu-hidden-bookmarks ())
 
@@ -1650,7 +1657,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)))))
@@ -1664,7 +1671,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)))
-- 
2.21.0

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

[Stefan Kangas]

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

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

[Stefan Kangas]

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

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

[Stefan Kangas]

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

Stefan Kangas <stefan@marxist.se> writes:
> 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.

Turns out this was not a bug, but only my own confusion.  Gmail had
helpfully put the reply to the bug report in the spam folder, so I
only realized *after* I sent this email that it had been replied to
hours ago.

I've attached an updated patch without this FIXME.  Other than that,
it's identical.

Thanks,
Stefan Kangas

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

From 4ea3adaca6dab4c680399245392a3db4c9d865fc 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 | 49 +++++++++++++++++++++++++++++++++-
 2 files changed, 82 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..a774dae89d 100644
--- a/test/lisp/bookmark-tests.el
+++ b/test/lisp/bookmark-tests.el
@@ -186,6 +186,18 @@ 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))
+     (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 +325,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

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 4 Jul 2019 21:40:45 +0200
> Cc: 20150@debbugs.gnu.org
> 
> I've attached an updated patch without this FIXME.  Other than that,
> it's identical.

Thanks.

> +(define-obsolete-variable-alias 'bookmark-annotation-name
> +  'bookmark-annotation--name "27.1")

Hmm... we declare a non-internal variable obsolete and replace it with
an internal variable?  Why is that a good idea?  Does the new variable
have to be internal?

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

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:

> > +(define-obsolete-variable-alias 'bookmark-annotation-name
> > +  'bookmark-annotation--name "27.1")
>
> Hmm... we declare a non-internal variable obsolete and replace it with
> an internal variable?  Why is that a good idea?  Does the new variable
> have to be internal?

I thought about it a bit more, and I guess it doesn't have to be internal.  I
suggest that the new variable should be kept internal for now though.

I also don't know why I named the new variable bookmark-annotation--name, so
I've changed that to bookmark--annotation-name to better reflect current
practices.

Please see this updated patch with those two changes.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Restore-focus-to-Bookmark-List-after-editing-annotat.patch --]
[-- Type: text/x-patch, Size: 9390 bytes --]

From 6650c4f51eeab4bfaf2e19d0ba209287a05804a8 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): Make buffer-local and improve doc string.
(bookmark--annotation-from-bookmark-list): New buffer-local variable.
(bookmark-edit-annotation): 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            | 49 ++++++++++++++++++++++++++++-----------------
 test/lisp/bookmark-tests.el | 49 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 19 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 2640b2157a..f564cd6b43 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."
@@ -889,13 +893,13 @@ bookmark-kill-line
     (when (and newline-too (= (following-char) ?\n))
       (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 under edit in `bookmark-edit-annotation-mode'.")
+(make-variable-buffer-local 'bookmark-annotation-name)
 
+(defvar bookmark--annotation-from-bookmark-list nil
+  "If non-nil, `bookmark-edit-annotation-mode' should return to bookmark list.")
+(make-variable-buffer-local 'bookmark--annotation-from-bookmark-list)
 
 (defun bookmark-default-annotation-text (bookmark-name)
   "Return default annotation text for BOOKMARK-NAME.
@@ -937,7 +941,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}")
 
@@ -955,21 +959,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 ()
@@ -1555,10 +1569,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 ())
 
 
@@ -1642,7 +1655,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)))))
@@ -1656,7 +1669,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)))
@@ -2059,7 +2072,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..a774dae89d 100644
--- a/test/lisp/bookmark-tests.el
+++ b/test/lisp/bookmark-tests.el
@@ -186,6 +186,18 @@ 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))
+     (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 +325,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.11.0

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 13 Jul 2019 15:09:39 +0200
> Cc: 20150@debbugs.gnu.org
> 
> > Hmm... we declare a non-internal variable obsolete and replace it with
> > an internal variable?  Why is that a good idea?  Does the new variable
> > have to be internal?
> 
> I thought about it a bit more, and I guess it doesn't have to be internal.  I
> suggest that the new variable should be kept internal for now though.

You mean, it doesn't have to be obsolete?

> I also don't know why I named the new variable bookmark-annotation--name, so
> I've changed that to bookmark--annotation-name to better reflect current
> practices.

Hmm... I don't see bookmark--annotation-name in the patch.  Did you
send the correct patch?

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:
> > I also don't know why I named the new variable bookmark-annotation--name, so
> > I've changed that to bookmark--annotation-name to better reflect current
> > practices.
>
> Hmm... I don't see bookmark--annotation-name in the patch.  Did you
> send the correct patch?

Oops, please ignore that part.  I wrote the wrong variable name, and
actually meant to say:

I also don't know why I initially named the new variable
bookmark-annotation--from-bookmark-list, so in the updated patch I've
changed that to bookmark--annotation-from-bookmark-list to better
reflect current practices.  (Note the placement of dashes.)

bookmark-annotation-name is unchanged in the updated patch.  I didn't
rename it nor make it obsolete.  I don't think it matters too much --
it could be internal, but it might not be worth renaming.

Sorry for the confusion.

Thanks,
Stefan Kangas

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

[Glenn Morris]

Off topic, but it seems to me like you should request write access so
you can apply changes directly. I believe you use
"request for inclusion form" at
https://savannah.gnu.org/project/memberlist.php?group=emacs

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

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  20150@debbugs.gnu.org
> Date: Sat, 13 Jul 2019 21:48:30 -0400
> 
> 
> Off topic, but it seems to me like you should request write access

He did.

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 13 Jul 2019 15:09:39 +0200
> Cc: 20150@debbugs.gnu.org
> 
> Please see this updated patch with those two changes.

Thanks, pushed to the master branch.