From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Karl Fogel Newsgroups: gmane.emacs.devel Subject: Re: Add new functions to mark/unmark/delete all bookmarks Date: Fri, 24 Jul 2020 14:07:42 -0500 Message-ID: <87pn8ku3y9.fsf@red-bean.com> References: <20200724005105.11f85d5f@pineapple> Reply-To: Karl Fogel Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="4090"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: Matthew White Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Fri Jul 24 21:08:32 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jz33Y-0000vq-IM for ged-emacs-devel@m.gmane-mx.org; Fri, 24 Jul 2020 21:08:32 +0200 Original-Received: from localhost ([::1]:51376 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jz33X-0002zl-Kc for ged-emacs-devel@m.gmane-mx.org; Fri, 24 Jul 2020 15:08:31 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:33110) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jz32s-0002Pi-Db for emacs-devel@gnu.org; Fri, 24 Jul 2020 15:07:50 -0400 Original-Received: from newsp.red-bean.com ([45.79.25.59]:34944 helo=sanpietro.red-bean.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jz32o-0007Ke-Pl for emacs-devel@gnu.org; Fri, 24 Jul 2020 15:07:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=red-bean.com; s=202005newsp; h=Content-Type:MIME-Version:Message-ID: In-Reply-To:Date:Reply-To:References:Subject:Cc:To:From:Sender: Content-Transfer-Encoding:Content-ID:Content-Description; bh=UwSqS5QoCnAj+8yi5NQuHX35k7kgSG7LdZ3WJzOqNCU=; t=1595617666; x=1596827266; b=Wd3MrF/uJFlMZWYQrCZ4sIi1g6lGR1p12N06s2NnOhoJYuirY41LBLhiMaoSNjzYLpZW9eFU8S mIus0T634jV7O2n44rARMa4ZTs94BUuLjxwH2W/iwPlmXMybtYlVEZQ+pD6c4MFgwITQWEifmDtcq VbXvKo8392t/b1G51bF7A4r36ShUnFdGbVewC603nGb5+uc/dOtKRXQ+jSy6DkEen1cai4N+b+rpZ mtGJS7JtMJnQ27CWntdKRQafa7xccjOR7Z9h9hfaNYCqQ/pUMpWPqFlBRAH5yP1DIJLFPo4TCVqKQ O+tvqaQX0hpxZ8oLD5tiGVsR0F7YDuFREaCNg==; Original-Received: from 99-112-125-163.lightspeed.cicril.sbcglobal.net ([99.112.125.163]:46490 helo=floss) by sanpietro.red-bean.com with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jz32l-0007fg-NN; Fri, 24 Jul 2020 19:07:43 +0000 In-Reply-To: <20200724005105.11f85d5f@pineapple> (Matthew White's message of "Fri, 24 Jul 2020 01:00:58 +0200") Received-SPF: pass client-ip=45.79.25.59; envelope-from=kfogel@red-bean.com; helo=sanpietro.red-bean.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/07/24 15:07:45 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:253202 Archived-At: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable 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-m= ap'. 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 cl= eanly to 'emacs-27'). It is very good; I have just some minor comments bel= ow. First, a couple of presentation issues: 1) It helps to include a summary line at the top of your commit message, fo= llowed by a blank line. The summary line should be limited to 50 character= s 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 t= he 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/bo= okmark.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 y= our 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.=20 (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. Howeve= r, note that there a *few* commits that do it the way you did it, and one o= f 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 fo= r 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 messa= ge the style is inconsistent, if you look closely). See the 'CONTRIBUTE' f= ile for more details on this -- it in turn points to https://www.gnu.org/pr= ep/standards/html_node/Change-Logs.html, which, I now discover, *also* does= not give a clear example of the "multiple changes inside one file" scenari= o 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 demonstra= te 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 i= nline 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))) >=20 >=20 >+;;;###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 purp= ose. And a suggestion: have the prompt say "Permanently delete all bookmarks? " = instead. Because of subtle assumptions associated with English word order, the curre= nt prompt ("Delete all bookmarks permanently? ") implies that the alternati= ve might be to delete the bookmarks non-permanently. E.g., a user might th= ink that if she responds "no", the next thing she'll get is a prompt offeri= ng 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-annotati= on t] >@@ -1748,6 +1770,7 @@ Letters do not insert themselves; instead, they are = commands. > Bookmark names preceded by a \"*\" have annotations. > \\ > \\[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 (th= ey may obscure long bookmark names). >@@ -1764,13 +1787,15 @@ Bookmark names preceded by a \"*\" have annotation= s. > \\[bookmark-bmenu-relocate] -- relocate this bookmark's file (prompts for= new file). > \\[bookmark-bmenu-delete] -- mark this bookmark to be deleted, and move d= own. > \\[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 delet= ed. >+\\[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 liste= d 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 bo= okmarks in another buffer. >@@ -1937,9 +1962,24 @@ If the annotation does not exist, do nothing." > (bookmark-bmenu-ensure-position)))) >=20 >=20 >+(defun bookmark-bmenu-mark-all () >+ "Mark all listed bookmarks to be displayed by \\\\[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 b= ookmark-bmenu-* code, so I think there's no need for the "FIXME" (and indee= d 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-li= ne-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-bm= enu-mark] command." >+You can mark bookmarks with the \\\\[bookmark-bm= enu-mark] or \\\\[bookmark-bmenu-mark-all] command= s." > (interactive) > (let ((bmrk (bookmark-bmenu-bookmark)) > (menu (current-buffer)) >@@ -2108,6 +2148,21 @@ Optional BACKUP means move up." > (bookmark-bmenu-ensure-position)) >=20 >=20 Nice -- I appreciate your thoroughness in updating even these secondary asp= ects 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-execute-deletions]." >@@ -2133,6 +2188,23 @@ To carry out the deletions that you've marked, use = \\\\ > (bookmark-bmenu-ensure-position)) >=20 >=20 >+(defun bookmark-bmenu-delete-all () >+ "Mark all listed bookmarks as to be deleted. >+To remove all deletion marks, use \\\\[bookmark-= bmenu-unmark-all]. >+To carry out the deletions that you've marked, use \\\\[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/bookma= rk-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)))) >=20 >+(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-bookmar= k-list-0) >+ ,(copy-sequence bookmark-tests-bookmar= k-list-1) >+ ,(copy-sequence bookmark-tests-bookmar= k-list-2)))) >+ (bookmark-default-file bookmark-tests-bookmark-file-list) >+ (bookmark-bookmarks-timestamp bookmark-tests-cache-timestamp-l= ist) >+ 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-tes= ts-bookmark))))) >=20 >+(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-te= sts-bookmark-list-0))) >+ (should (equal (bookmark-get-bookmark-record "name1") (cdr bookmark-te= sts-bookmark-list-1))) >+ (should (equal (bookmark-get-bookmark-record "name2") (cdr bookmark-te= sts-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)))))) >=20 >+(ert-deftest bookmark-tests-make-record-list () >+ (with-bookmark-test-file-list >+ (let* ((record `("example.txt" (filename . ,bookmark-tests-example-fil= e) >+ (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)))) >=20 >+(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))))) >=20 >+(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 "^>")))) >=20 >+(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 "^ ")))) >=20 >+(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)))) >=20 >+(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 adverti= sed, and the new tests pass 'make check'. If you have time to revise the patch slightly as per above (if you agree wi= th 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, =2DKarl --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQJIBAEBCgAyFiEEsgfAWXyjn8Fyi3xbCKC3XMXtg0UFAl8bMX4UHGtmb2dlbEBy ZWQtYmVhbi5jb20ACgkQCKC3XMXtg0VxRg//W0J5ZwoGcmNC6gIQ2ZLUofecijH7 Dj2jm+QC7DnMrzVnIuWEka0zbd5ryYjTEnR+XFSWxma2OoBrZvXp+UXHQ7pGSsrI Eqxx8TDPHDEnhIGxH224+sD8XLDP5J6kMMXDDR9ruZejhlhsmvsruNYsJ7X3ZRMj nKY3sXJeerAfKsVola6Zvy1drdYNsxUiOp8CpYHUCzAfJTu6yMLoixicqiQMOfkK Xiz0F58HRMugDZpbPKukT2jEByzeOtHLzlHz2SjXyH1UAfDmNJNotgA6T90Fdmd7 0PTMuwme1BZsET3rpUsTnZwEfEJAoefIc65YTbKPrs2DZNELxcwt+TUtA/szcLY8 CUs5B2vmrV+RBsC7UlP7gIY4/3hSV4faVOTJSSFZ8baQO1Autap4k9bSqxTJH+jQ 5PIodQ8D49tbC/RrySWPwW72GsUfUTANHfRcDSIYxXfAbrgwco3H2m8u11nmxYyu /WnB66yeRfR/Ws+c6ISmIgoEWQt3Qibuvmf0aZNjbQfflsy5qeuQ343R1bi/2DJ4 LhhR601om2/25KYAUOgP0KRIacbYlY5tZu6yyVJiRosE9uMQMEutWFtwkhD4161g gpCWBGn98ezgVioe2I7K12H/Jl6qkqjpSSf67Ng9oA2XMOnDtXtP+Ui4S8OWwaEt 2A1jsJJ1/5IOgAU= =jNCN -----END PGP SIGNATURE----- --=-=-=--