Re: Add new functions to mark/unmark/delete all bookmarks

[Karl Fogel]

On 24 Jul 2020, Matthew White wrote:
>Attached here there is a patch to add new functions to mark, unmark,
>and delete all bookmarks at once.
>
>Menu entries and bindings are updated too, and tests are included.
>
>You may consider this both a suggestion and a way to get an opinion
>from the community.
>
>I felt that some functions where missing, I'm used to type "U" in
>dired to unmark all marks...
>
>Summary (just main functions are described here):
>
>* bookmark-delete-all: With ARG nil asks for confirmation. Deletes all
>  bookmarks. Bound to "D" in `bookmark-map'.
>* bookmark-bmenu-mark-all: Operates in the *Bookmark List* buffer.
>  Marks all bookmarks. Bound to "M" in `bookmark-bmenu-mode-map'.
>* bookmark-bmenu-unmark-all: Operates in the *Bookmark List* buffer.
>  Unmarks all bookmarks. Bound to "U" in `bookmark-bmenu-mode-map'.
>* bookmark-bmenu-delete-all: Operates in the *Bookmark List* buffer.
>  Marks all bookmarks for deletion. Bound to "D" in `bookmark-bmenu-mode-map'.

Hi, Matthew.  Thanks so much for this.  I have reviewed the patch against 
'master' (since it applies cleanly there, whereas it does not quite apply 
cleanly to 'emacs-27').  It is very good; I have just some minor comments below.

First, a couple of presentation issues:

1) It helps to include a summary line at the top of your commit message, 
followed by a blank line.  The summary line should be limited to 50 characters 
or fewer, if possible.  (This is all documented in the "Commit messages" 
section in the 'CONTRIBUTE' file in the top level of the Emacs source tree, by 
the way).  In your case, the Subject header of your email almost does the job 
-- we can shorten it a bit to fit within 50 characters:

  "Add ability to mark/unmark/delete all bookmarks"

2) In the commit message, you don't need to repeat the filename ("* 
lisp/bookmark.el") each time.  You can just write it once, and then underneath 
it show everything affected in that file.  Taking the first three entries in 
your original log message as an example:

   * lisp/bookmark.el (bookmark-delete-all): New function to delete all
     bookmarks.
   * lisp/bookmark.el (bookmark-bmenu-mark-all): New function to mark all
     bookmarks in the bookmark list buffer.
   * lisp/bookmark.el (bookmark-bmenu-unmark-all): New function to unmark
     all bookmarks in the bookmark list buffer.

...they would instead be written like this:

   * lisp/bookmark.el (bookmark-delete-all): New function to delete all
     bookmarks.
     (bookmark-bmenu-mark-all): New function to mark all bookmarks in the
     bookmark list buffer. 
     (bookmark-bmenu-unmark-all): New function to unmark all bookmarks in
     the bookmark list buffer.

Quick digression:

You can use 'git log' to see this style used in past Emacs commits.  However, 
note that there a *few* commits that do it the way you did it, and one of them 
happens to be quite recent, 0d7d422b767c.  So if you did look in the logs, and 
you happened to see that one, then it would be understandable for you to have 
taken it as an example :-).  Nevertheless, it is an exception and is not the 
normal style (also, even within 0d7d422b767c's commit message the style is 
inconsistent, if you look closely).  See the 'CONTRIBUTE' file for more details 
on this -- it in turn points to 
https://www.gnu.org/prep/standards/html_node/Change-Logs.html, which, I now 
discover, *also* does not give a clear example of the "multiple changes inside 
one file" scenario we're talking about here even though it is a very common 
case.

Sigh :-).

Anyway, look through the Emacs logs and you'll see that the way I demonstrate 
above is the common way.

Other than those two issues, your commit message is very clear.  I was able to 
understand the intent and manner of the change from reading the commit message.

Below I'll quote the entire log message and patch, so people can know that 
they're seeing everything in this response, even though I only make a few 
inline comments.

>* lisp/bookmark.el (bookmark-delete-all): New function to delete all
>  bookmarks.
>* lisp/bookmark.el (bookmark-bmenu-mark-all): New function to mark all
>  bookmarks in the bookmark list buffer.
>* lisp/bookmark.el (bookmark-bmenu-unmark-all): New function to unmark
>  all bookmarks in the bookmark list buffer.
>* lisp/bookmark.el (bookmark-bmenu-delete-all): New function to mark
>  for deletion all bookmarks in the bookmark list buffer.
>* lisp/bookmark.el (bookmark-map): Map "D" to `bookmark-delete-all'.
>* lisp/bookmark.el (bookmark-bmenu-mode-map): New mappping for "M" to
>  `bookmark-bmenu-mark-all'.
>* lisp/bookmark.el (bookmark-bmenu-mode-map): New mappping for "U" to
>  `bookmark-bmenu-unmark-all'.
>* lisp/bookmark.el (bookmark-bmenu-mode-map): New mappping for "D" to
>  `bookmark-bmenu-delete-all'.
>* lisp/bookmark.el (bookmark-bmenu-mark-all): New bookmark menu to
>  `bookmark-delete-all'.
>* lisp/bookmark.el (easy-menu-define): New bookmark menu to
>  `bookmark-bmenu-mark-all'.
>* lisp/bookmark.el (easy-menu-define): New bookmark menu to
>  `bookmark-bmenu-unmark-all'.
>* lisp/bookmark.el (easy-menu-define): New bookmark menu to
>  `bookmark-bmenu-delete-all'.
>* lisp/bookmark.el (bookmark-bmenu-select): Update docstring to
>  include a reference to `bookmark-bmenu-mark-all'.
>* lisp/bookmark.el (bookmark-bmenu-mode): Update docstring. Add/Update
>  description: `bookmark-bmenu-mark-all', `bookmark-bmenu-delete-all',
>  `bookmark-bmenu-execute-deletions', and `bookmark-bmenu-unmark-all'.
>* test/lisp/bookmark-resources/test-list.bmk: New bookmark file to
>  test a list of bookmarks.
>* test/lisp/bookmark-tests.el (bookmark-tests-bookmark-file-list): New
>  reference to the bookmark file used for testing a list of bookmarks.
>* test/lisp/bookmark-tests.el (bookmark-tests-bookmark-list-0,
>  bookmark-tests-bookmark-list-1, bookmark-tests-bookmark-list-2): New
>  cached values for testing a list of bookmark.
>* test/lisp/bookmark-tests.el (bookmark-tests-cache-timestamp-list):
>  New variable to set `bookmark-bookmarks-timestamp'.
>* test/lisp/bookmark-tests.el (with-bookmark-test-list): New macro
>  environment to test a list of bookmarks.
>* test/lisp/bookmark-tests.el (with-bookmark-test-file-list): New
>  macro environment to test a list of bookmarks with example.txt.
>* test/lisp/bookmark-tests.el (with-bookmark-bmenu-test-list): New
>  macro environment to test functions about a list of bookmarks from
>  `bookmark-bmenu-list'.
>* test/lisp/bookmark-tests.el (bookmark-tests-all-names-list,
>  bookmark-tests-get-bookmark-list,
>  bookmark-tests-get-bookmark-record-list): New functions to test the
>  records of the list of bookmarks.
>* test/lisp/bookmark-tests.el (bookmark-tests-make-record-list): New
>  function to test the creation of a record from example.txt with a
>  list of bookmarks loaded.
>* test/lisp/bookmark-tests.el (bookmark-tests-delete-all): New
>  function to test `bookmark-delete-all'.
>* test/lisp/bookmark-tests.el (bookmark-test-bmenu-mark-all): New
>  function to test `bookmark-bmenu-mark-all'.
>* test/lisp/bookmark-tests.el (bookmark-test-bmenu-unmark-all): New
>  function to test `bookmark-bmenu-unmark-all'.
>* test/lisp/bookmark-tests.el (bookmark-test-bmenu-delete-all): New
>  function to test `bookmark-bmenu-delete-all'.
>---
> lisp/bookmark.el                           |  79 ++++++++-
> test/lisp/bookmark-resources/test-list.bmk |  20 +++
> test/lisp/bookmark-tests.el                | 183 +++++++++++++++++++++
> 3 files changed, 280 insertions(+), 2 deletions(-)
> create mode 100644 test/lisp/bookmark-resources/test-list.bmk
>
>diff --git a/lisp/bookmark.el b/lisp/bookmark.el
>index de7d60f97e..8a9c032930 100644
>--- a/lisp/bookmark.el
>+++ b/lisp/bookmark.el
>@@ -200,6 +200,7 @@ A non-nil value may result in truncated bookmark names."
>     (define-key map "f" 'bookmark-insert-location) ;"f"ind
>     (define-key map "r" 'bookmark-rename)
>     (define-key map "d" 'bookmark-delete)
>+    (define-key map "D" 'bookmark-delete-all)
>     (define-key map "l" 'bookmark-load)
>     (define-key map "w" 'bookmark-write)
>     (define-key map "s" 'bookmark-save)
>@@ -1374,6 +1375,21 @@ probably because we were called from there."
>     (bookmark-save)))
> 
> 
>+;;;###autoload
>+(defun bookmark-delete-all (&optional arg)
>+  "Delete all bookmarks permanently.
>+Doesn't ask for confirmation if ARG is non-nil."
>+  (interactive "P")
>+  (when (or arg (yes-or-no-p "Delete all bookmarks permanently? "))
>+    (bookmark-maybe-load-default-file)
>+    (setq bookmark-alist-modification-count
>+          (+ bookmark-alist-modification-count (length bookmark-alist)))
>+    (setq bookmark-alist nil)
>+    (bookmark-bmenu-surreptitiously-rebuild-list)
>+    (when (bookmark-time-to-save-p)
>+      (bookmark-save))))
>+
>+

Better to call ARG something like NO-CONFIRM, so its name reflects its purpose.

And a suggestion: have the prompt say "Permanently delete all bookmarks? " 
instead.

Because of subtle assumptions associated with English word order, the current 
prompt ("Delete all bookmarks permanently? ") implies that the alternative 
might be to delete the bookmarks non-permanently.  E.g., a user might think 
that if she responds "no", the next thing she'll get is a prompt offering 
various choices of non-permanent ways to delete them :-).  This is not a huge 
problem, of course -- when she doesn't get any followup prompt, she'd figure 
out what's going on.  But it's better to just ask the question in a way that 
makes the choice clear in the first place.

> (defun bookmark-time-to-save-p (&optional final-time)
>   "Return t if it is time to save bookmarks to disk, nil otherwise.
> Optional argument FINAL-TIME means this is being called when Emacs
>@@ -1600,12 +1616,15 @@ unique numeric suffixes \"<2>\", \"<3>\", etc."
>     (define-key map "\C-d" 'bookmark-bmenu-delete-backwards)
>     (define-key map "x" 'bookmark-bmenu-execute-deletions)
>     (define-key map "d" 'bookmark-bmenu-delete)
>+    (define-key map "D" 'bookmark-bmenu-delete-all)
>     (define-key map " " 'next-line)
>     (define-key map "n" 'next-line)
>     (define-key map "p" 'previous-line)
>     (define-key map "\177" 'bookmark-bmenu-backup-unmark)
>     (define-key map "u" 'bookmark-bmenu-unmark)
>+    (define-key map "U" 'bookmark-bmenu-unmark-all)
>     (define-key map "m" 'bookmark-bmenu-mark)
>+    (define-key map "M" 'bookmark-bmenu-mark-all)
>     (define-key map "l" 'bookmark-bmenu-load)
>     (define-key map "r" 'bookmark-bmenu-rename)
>     (define-key map "R" 'bookmark-bmenu-relocate)
>@@ -1627,8 +1646,10 @@ unique numeric suffixes \"<2>\", \"<3>\", etc."
>     ["Select Marked Bookmarks" bookmark-bmenu-select t]
>     "---"
>     ["Mark Bookmark" bookmark-bmenu-mark t]
>+    ["Mark all Bookmarks" bookmark-bmenu-mark-all t]
>     ["Unmark Bookmark" bookmark-bmenu-unmark  t]
>     ["Unmark Backwards" bookmark-bmenu-backup-unmark  t]
>+    ["Unmark all Bookmarks" bookmark-bmenu-unmark-all  t]
>     ["Toggle Display of Filenames" bookmark-bmenu-toggle-filenames  t]
>     ["Display Location of Bookmark" bookmark-bmenu-locate  t]
>     "---"
>@@ -1636,6 +1657,7 @@ unique numeric suffixes \"<2>\", \"<3>\", etc."
>      ["Rename Bookmark" bookmark-bmenu-rename  t]
>      ["Relocate Bookmark's File" bookmark-bmenu-relocate  t]
>      ["Mark Bookmark for Deletion" bookmark-bmenu-delete  t]
>+     ["Mark all Bookmarks for Deletion" bookmark-bmenu-delete-all  t]
>      ["Delete Marked Bookmarks" bookmark-bmenu-execute-deletions  t])
>     ("Annotations"
>      ["Show Annotation for Current Bookmark" bookmark-bmenu-show-annotation  
> t]
>@@ -1748,6 +1770,7 @@ Letters do not insert themselves; instead, they are 
>commands.
> Bookmark names preceded by a \"*\" have annotations.
> \\<bookmark-bmenu-mode-map>
> \\[bookmark-bmenu-mark] -- mark bookmark to be displayed.
>+\\[bookmark-bmenu-mark-all] -- mark all listed bookmarks to be displayed.
> \\[bookmark-bmenu-select] -- select bookmark of line point is on.
>   Also show bookmarks marked using m in other windows.
> \\[bookmark-bmenu-toggle-filenames] -- toggle displaying of filenames (they 
> may obscure long bookmark names).
>@@ -1764,13 +1787,15 @@ Bookmark names preceded by a \"*\" have annotations.
> \\[bookmark-bmenu-relocate] -- relocate this bookmark's file (prompts for new 
> file).
> \\[bookmark-bmenu-delete] -- mark this bookmark to be deleted, and move down.
> \\[bookmark-bmenu-delete-backwards] -- mark this bookmark to be deleted, and 
> move up.
>-\\[bookmark-bmenu-execute-deletions] -- delete bookmarks marked with 
>`\\[bookmark-bmenu-delete]'.
>+\\[bookmark-bmenu-delete-all] -- mark all listed bookmarks as to be deleted.
>+\\[bookmark-bmenu-execute-deletions] -- delete bookmarks marked with 
>`\\[bookmark-bmenu-delete]' or `\\[bookmark-bmenu-delete-all]'.
> \\[bookmark-bmenu-save] -- save the current bookmark list in the default file.
>   With a prefix arg, prompts for a file to save in.
> \\[bookmark-bmenu-load] -- load in a file of bookmarks (prompts for file.)
> \\[bookmark-bmenu-unmark] -- remove all kinds of marks from current line.
>   With prefix argument, also move up one line.
> \\[bookmark-bmenu-backup-unmark] -- back up a line and remove marks.
>+\\[bookmark-bmenu-unmark-all] -- remove all kinds of marks from all listed 
>bookmarks.
> \\[bookmark-bmenu-show-annotation] -- show the annotation, if it exists, for 
> the current bookmark
>   in another buffer.
> \\[bookmark-bmenu-show-all-annotations] -- show the annotations of all 
> bookmarks in another buffer.
>@@ -1937,9 +1962,24 @@ If the annotation does not exist, do nothing."
>      (bookmark-bmenu-ensure-position))))
> 
> 
>+(defun bookmark-bmenu-mark-all ()
>+  "Mark all listed bookmarks to be displayed by 
>\\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-select]."
>+  (interactive)
>+  (save-excursion
>+    (goto-char (point-min))
>+    (bookmark-bmenu-ensure-position)
>+    (with-buffer-modified-unmodified
>+     (let ((inhibit-read-only t))
>+       ;; FIXME: This assumes that the last line is empty.
>+       (while (not (eobp))
>+         (delete-char 1)
>+         (insert ?>)
>+         (forward-line 1))))))
>+
>+

Regarding the "FIXME" comment: that assumption is embedded throughout the 
bookmark-bmenu-* code, so I think there's no need for the "FIXME" (and indeed 
the comment may cause confusion, since its presence calls the assumption into 
question).

It might be good to make a followup change in which we document the 
last-line-is-empty assumption, but that's a separate change of course.

> (defun bookmark-bmenu-select ()
>   "Select this line's bookmark; also display bookmarks marked with `>'.
>-You can mark bookmarks with the 
>\\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-mark] command."
>+You can mark bookmarks with the 
>\\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-mark] or 
>\\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-mark-all] commands."
>   (interactive)
>   (let ((bmrk (bookmark-bmenu-bookmark))
>         (menu (current-buffer))
>@@ -2108,6 +2148,21 @@ Optional BACKUP means move up."
>   (bookmark-bmenu-ensure-position))
> 
> 

Nice -- I appreciate your thoroughness in updating even these secondary aspects 
of the doc strings.

>+(defun bookmark-bmenu-unmark-all ()
>+  "Cancel all requested operations on all listed bookmarks."
>+  (interactive)
>+  (save-excursion
>+    (goto-char (point-min))
>+    (bookmark-bmenu-ensure-position)
>+    (with-buffer-modified-unmodified
>+     (let ((inhibit-read-only t))
>+       ;; FIXME: This assumes that the last line is empty.
>+       (while (not (eobp))
>+         (delete-char 1)
>+         (insert " ")
>+         (forward-line 1))))))
>+
>+

Same point as earlier about the "FIXME" comment.

> (defun bookmark-bmenu-delete ()
>   "Mark bookmark on this line to be deleted.
> To carry out the deletions that you've marked, use 
> \\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-execute-deletions]."
>@@ -2133,6 +2188,23 @@ To carry out the deletions that you've marked, use 
>\\<bookmark-bmenu-mode-map>\\
>   (bookmark-bmenu-ensure-position))
> 
> 
>+(defun bookmark-bmenu-delete-all ()
>+  "Mark all listed bookmarks as to be deleted.
>+To remove all deletion marks, use 
>\\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-unmark-all].
>+To carry out the deletions that you've marked, use 
>\\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-execute-deletions]."
>+  (interactive)
>+  (save-excursion
>+    (goto-char (point-min))
>+    (bookmark-bmenu-ensure-position)
>+    (with-buffer-modified-unmodified
>+     (let ((inhibit-read-only t))
>+       ;; FIXME: This assumes that the last line is empty.
>+       (while (not (eobp))
>+         (delete-char 1)
>+         (insert ?D)
>+         (forward-line 1))))))
>+
>+

Same point as earlier about the "FIXME" comment.

> (defun bookmark-bmenu-execute-deletions ()
>   "Delete bookmarks flagged `D'."
>   (interactive)
>@@ -2292,6 +2364,9 @@ strings returned are not."
>     (bindings--define-key map [delete]
>       '(menu-item "Delete Bookmark..." bookmark-delete
>                 :help "Delete a bookmark from the bookmark list"))
>+    (bindings--define-key map [delete-all]
>+      '(menu-item "Delete all Bookmarks..." bookmark-delete-all
>+                :help "Delete all bookmarks from the bookmark list"))
>     (bindings--define-key map [rename]
>       '(menu-item "Rename Bookmark..." bookmark-rename
>                 :help "Change the name of a bookmark"))
>diff --git a/test/lisp/bookmark-resources/test-list.bmk 
>b/test/lisp/bookmark-resources/test-list.bmk
>new file mode 100644
>index 0000000000..11a5c3f222
>--- /dev/null
>+++ b/test/lisp/bookmark-resources/test-list.bmk
>@@ -0,0 +1,20 @@
>+;;;; Emacs Bookmark Format Version 1 ;;;; -*- coding: utf-8-emacs -*-
>+;;; This format is meant to be slightly human-readable;
>+;;; nevertheless, you probably don't want to edit it.
>+;;; -*- End Of Bookmark File Format Version Stamp -*-
>+(("name0"
>+ (filename . "/some/file0")
>+ (front-context-string . "abc")
>+ (rear-context-string . "def")
>+ (position . 3))
>+("name1"
>+ (filename . "/some/file1")
>+ (front-context-string . "abc")
>+ (rear-context-string . "def")
>+ (position . 3))
>+("name2"
>+ (filename . "/some/file2")
>+ (front-context-string . "abc")
>+ (rear-context-string . "def")
>+ (position . 3))
>+)
>diff --git a/test/lisp/bookmark-tests.el b/test/lisp/bookmark-tests.el
>index b9c6ff9c54..3790498812 100644
>--- a/test/lisp/bookmark-tests.el
>+++ b/test/lisp/bookmark-tests.el
>@@ -83,6 +83,70 @@ the lexically-bound variable `buffer'."
>           ,@body)
>        (kill-buffer buffer))))
> 
>+(defvar bookmark-tests-bookmark-file-list
>+  (expand-file-name "test-list.bmk" bookmark-tests-data-dir)
>+  "Bookmark file used for testing a list of bookmarks.")
>+
>+;; The values below should match `bookmark-tests-bookmark-file-list'
>+;; content.  We cache these values to speed up tests.
>+(eval-and-compile  ; needed by `with-bookmark-test-list' macro
>+  (defvar bookmark-tests-bookmark-list-0 '("name0"
>+                            (filename . "/some/file0")
>+                            (front-context-string . "ghi")
>+                            (rear-context-string . "jkl")
>+                            (position . 4))
>+    "Cached value used in bookmark-tests.el."))
>+
>+;; The values below should match `bookmark-tests-bookmark-file-list'
>+;; content.  We cache these values to speed up tests.
>+(eval-and-compile  ; needed by `with-bookmark-test-list' macro
>+  (defvar bookmark-tests-bookmark-list-1 '("name1"
>+                            (filename . "/some/file1")
>+                            (front-context-string . "mno")
>+                            (rear-context-string . "pqr")
>+                            (position . 5))
>+    "Cached value used in bookmark-tests.el."))
>+
>+;; The values below should match `bookmark-tests-bookmark-file-list'
>+;; content.  We cache these values to speed up tests.
>+(eval-and-compile  ; needed by `with-bookmark-test-list' macro
>+  (defvar bookmark-tests-bookmark-list-2 '("name2"
>+                            (filename . "/some/file2")
>+                            (front-context-string . "stu")
>+                            (rear-context-string . "vwx")
>+                            (position . 6))
>+    "Cached value used in bookmark-tests.el."))
>+
>+(defvar bookmark-tests-cache-timestamp-list
>+  (cons bookmark-tests-bookmark-file-list
>+        (nth 5 (file-attributes
>+                bookmark-tests-bookmark-file-list)))
>+  "Cached value used in bookmark-tests.el.")
>+
>+(defmacro with-bookmark-test-list (&rest body)
>+  "Create environment for testing bookmark.el and evaluate BODY.
>+Ensure a clean environment for testing, and do not change user
>+data when running tests interactively."
>+  `(with-temp-buffer
>+     (let ((bookmark-alist (quote (,(copy-sequence 
>bookmark-tests-bookmark-list-0)
>+                                   ,(copy-sequence 
>bookmark-tests-bookmark-list-1)
>+                                   ,(copy-sequence 
>bookmark-tests-bookmark-list-2))))
>+           (bookmark-default-file bookmark-tests-bookmark-file-list)
>+           (bookmark-bookmarks-timestamp bookmark-tests-cache-timestamp-list)
>+           bookmark-save-flag)
>+       ,@body)))
>+
>+(defmacro with-bookmark-test-file-list (&rest body)
>+  "Create environment for testing bookmark.el and evaluate BODY.
>+Same as `with-bookmark-test-list' but also opens the resource file
>+example.txt in a buffer, which can be accessed by callers through
>+the lexically-bound variable `buffer'."
>+  `(let ((buffer (find-file-noselect bookmark-tests-example-file)))
>+     (unwind-protect
>+         (with-bookmark-test-list
>+          ,@body)
>+       (kill-buffer buffer))))
>+
> (ert-deftest bookmark-tests-all-names ()
>   (with-bookmark-test
>    (should (equal (bookmark-all-names) '("name")))))
>@@ -95,6 +159,22 @@ the lexically-bound variable `buffer'."
>   (with-bookmark-test
>    (should (equal (bookmark-get-bookmark-record "name") (cdr 
> bookmark-tests-bookmark)))))
> 
>+(ert-deftest bookmark-tests-all-names-list ()
>+  (with-bookmark-test-list
>+   (should (equal (bookmark-all-names) '("name0" "name1" "name2")))))
>+
>+(ert-deftest bookmark-tests-get-bookmark-list ()
>+  (with-bookmark-test-list
>+   (should (equal (bookmark-get-bookmark "name0") 
>bookmark-tests-bookmark-list-0))
>+   (should (equal (bookmark-get-bookmark "name1") 
>bookmark-tests-bookmark-list-1))
>+   (should (equal (bookmark-get-bookmark "name2") 
>bookmark-tests-bookmark-list-2))))
>+
>+(ert-deftest bookmark-tests-get-bookmark-record-list ()
>+  (with-bookmark-test-list
>+   (should (equal (bookmark-get-bookmark-record "name0") (cdr 
>bookmark-tests-bookmark-list-0)))
>+   (should (equal (bookmark-get-bookmark-record "name1") (cdr 
>bookmark-tests-bookmark-list-1)))
>+   (should (equal (bookmark-get-bookmark-record "name2") (cdr 
>bookmark-tests-bookmark-list-2)))))
>+
> (ert-deftest bookmark-tests-record-getters-and-setters-new ()
>   (with-temp-buffer
>     (let* ((buffer-file-name "test")
>@@ -130,6 +210,19 @@ the lexically-bound variable `buffer'."
>        ;; calling twice gives same record
>        (should (equal (bookmark-make-record) record))))))
> 
>+(ert-deftest bookmark-tests-make-record-list ()
>+  (with-bookmark-test-file-list
>+   (let* ((record `("example.txt" (filename . ,bookmark-tests-example-file)
>+                    (front-context-string . "is text file is ")
>+                    (rear-context-string)
>+                    (position . 3)
>+                    (defaults "example.txt"))))
>+     (with-current-buffer buffer
>+       (goto-char 3)
>+       (should (equal (bookmark-make-record) record))
>+       ;; calling twice gives same record
>+       (should (equal (bookmark-make-record) record))))))
>+
> (ert-deftest bookmark-tests-make-record-function ()
>   (with-bookmark-test
>    (let ((buffer-file-name "test"))
>@@ -267,6 +360,11 @@ the lexically-bound variable `buffer'."
>    (bookmark-delete "name")
>    (should (equal bookmark-alist nil))))
> 
>+(ert-deftest bookmark-tests-delete-all ()
>+  (with-bookmark-test-list
>+   (bookmark-delete-all t)
>+   (should (equal bookmark-alist nil))))
>+
> (defmacro with-bookmark-test-save-load (&rest body)
>   "Create environment for testing bookmark.el and evaluate BODY.
> Same as `with-bookmark-test' but also sets a temporary
>@@ -340,6 +438,18 @@ testing `bookmark-bmenu-list'."
>             ,@body)
>         (kill-buffer bookmark-bmenu-buffer)))))
> 
>+(defmacro with-bookmark-bmenu-test-list (&rest body)
>+  "Create environment for testing `bookmark-bmenu-list' and evaluate BODY.
>+Same as `with-bookmark-test-list' but with additions suitable for
>+testing `bookmark-bmenu-list'."
>+  `(with-bookmark-test-list
>+    (let ((bookmark-bmenu-buffer "*Bookmark List - Testing*"))
>+      (unwind-protect
>+          (save-window-excursion
>+            (bookmark-bmenu-list)
>+            ,@body)
>+        (kill-buffer bookmark-bmenu-buffer)))))
>+
> (ert-deftest bookmark-test-bmenu-edit-annotation/show-annotation ()
>   (with-bookmark-bmenu-test
>    (bookmark-set-annotation "name" "foo")
>@@ -396,6 +506,28 @@ testing `bookmark-bmenu-list'."
>    (beginning-of-line)
>    (should (looking-at "^>"))))
> 
>+(ert-deftest bookmark-test-bmenu-mark-all ()
>+  (with-bookmark-bmenu-test-list
>+   (let ((here (point-max)))
>+     ;; Expect to not move the point
>+     (goto-char here)
>+     (bookmark-bmenu-mark-all)
>+     (should (equal here (point)))
>+     ;; Verify that all bookmarks are marked
>+     (goto-char (point-min))
>+     (bookmark-bmenu-ensure-position)
>+     (should (looking-at "^> "))
>+     (should (equal bookmark-tests-bookmark-list-0
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark))))
>+     (forward-line 1)
>+     (should (looking-at "^> "))
>+     (should (equal bookmark-tests-bookmark-list-1
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark))))
>+     (forward-line 1)
>+     (should (looking-at "^> "))
>+     (should (equal bookmark-tests-bookmark-list-2
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark)))))))
>+
> (ert-deftest bookmark-test-bmenu-any-marks ()
>   (with-bookmark-bmenu-test
>    (bookmark-bmenu-mark)
>@@ -410,12 +542,63 @@ testing `bookmark-bmenu-list'."
>    (beginning-of-line)
>    (should (looking-at "^  "))))
> 
>+(ert-deftest bookmark-test-bmenu-unmark-all ()
>+  (with-bookmark-bmenu-test-list
>+   (bookmark-bmenu-mark-all)
>+   (let ((here (point-max)))
>+     ;; Expect to not move the point
>+     (goto-char here)
>+     (bookmark-bmenu-unmark-all)
>+     (should (equal here (point)))
>+     ;; Verify that all bookmarks are unmarked
>+     (goto-char (point-min))
>+     (bookmark-bmenu-ensure-position)
>+     (should (looking-at "^  "))
>+     (should (equal bookmark-tests-bookmark-list-0
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark))))
>+     (forward-line 1)
>+     (should (looking-at "^  "))
>+     (should (equal bookmark-tests-bookmark-list-1
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark))))
>+     (forward-line 1)
>+     (should (looking-at "^  "))
>+     (should (equal bookmark-tests-bookmark-list-2
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark)))))))
>+
> (ert-deftest bookmark-test-bmenu-delete ()
>   (with-bookmark-bmenu-test
>    (bookmark-bmenu-delete)
>    (bookmark-bmenu-execute-deletions)
>    (should (equal (length bookmark-alist) 0))))
> 
>+(ert-deftest bookmark-test-bmenu-delete-all ()
>+  (with-bookmark-bmenu-test-list
>+   ;; Verify that unmarked bookmarks aren't deleted
>+   (bookmark-bmenu-execute-deletions)
>+   (should-not (eq bookmark-alist nil))
>+   (let ((here (point-max)))
>+     ;; Expect to not move the point
>+     (goto-char here)
>+     (bookmark-bmenu-delete-all)
>+     (should (equal here (point)))
>+     ;; Verify that all bookmarks are marked for deletion
>+     (goto-char (point-min))
>+     (bookmark-bmenu-ensure-position)
>+     (should (looking-at "^D "))
>+     (should (equal bookmark-tests-bookmark-list-0
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark))))
>+     (forward-line 1)
>+     (should (looking-at "^D "))
>+     (should (equal bookmark-tests-bookmark-list-1
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark))))
>+     (forward-line 1)
>+     (should (looking-at "^D "))
>+     (should (equal bookmark-tests-bookmark-list-2
>+                    (bookmark-get-bookmark (bookmark-bmenu-bookmark))))
>+     ;; Verify that all bookmarks are deleted
>+     (bookmark-bmenu-execute-deletions)
>+     (should (eq bookmark-alist nil)))))
>+
> (ert-deftest bookmark-test-bmenu-locate ()
>   (let (msg)
>     (cl-letf (((symbol-function 'message)

These tests are a huge contribution -- thanks for taking the time to write them.

This patch looks good to me.  I've tested it myself and it works as advertised, 
and the new tests pass 'make check'.

If you have time to revise the patch slightly as per above (if you agree with 
the comments, of course -- otherwise, let's discuss here) that would be great.  
I see that Stefan has already sent you the assignment paperwork.

Best regards,
-Karl

signature.asc
Description: PGP signature