Add a last-modified field when a bookmark is set

* test/lisp/bookmark-tests.el (bookmark-tests-make-record)
(bookmark-tests-make-record-list, bookmark-tests-set): fix tests
to not consider last-modified in bookmark equality.
* lisp/bookmark.el (bookmark-make-record-default): add a
last-modified field.
(bookmark-sort-flag): add the 'last-modified choice.
(bookmark-get-last-modified): new function to get last-modified
bookmark field.
(bookmark-maybe-sort-alist): sort in last-modified first order.
(bookmark-completing-read): use `bookmark-maybe-sort-alist'.

3 files changed, 85 insertions, 49 deletions

diff --git a/etc/NEWS b/etc/NEWS
index 0d4532985b2..597f92cfce3 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1690,6 +1690,11 @@ manual for more details.
 Types are registered via a 'bookmark-handler-type' symbol property on
 the jumping function.
 
++++
+*** 'bookmark-sort-flag' can now be set to 'last-modified'.
+This will display bookmark list from most recently set to least
+recently set.
+
 ---
 *** New minor mode 'elide-head-mode'.
 Enabling this minor mode turns on hiding header material, like
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index c604395dd7d..8e251e9de87 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -115,10 +115,18 @@ just use the value of `version-control'."
 
 
 (defcustom bookmark-sort-flag t
-  "Non-nil means that bookmarks will be displayed sorted by bookmark name.
-Otherwise they will be displayed in LIFO order (that is, most
-recently set ones come first, oldest ones come last)."
-  :type 'boolean)
+  "This controls the bookmark display sorting.
+nil means they will be displayed in LIFO order (that is, most
+recently created ones come first, oldest ones come last).
+
+`last-modified' means that bookmarks will be displayed sorted
+from most recently set to last recently set.
+
+Other values means that bookmarks will be displayed sorted by
+bookmark name."
+  :type '(choice (const :tag "By name" t)
+                 (const :tag "By modified time" last-modified)
+                 (const :tag "By creation time" nil)))
 
 
 (defcustom bookmark-menu-confirm-deletion nil
@@ -460,6 +468,10 @@ In other words, return all information but the name."
   "Return the handler function for BOOKMARK-NAME-OR-RECORD, or nil if none."
   (bookmark-prop-get bookmark-name-or-record 'handler))
 
+(defun bookmark-get-last-modified (bookmark-name-or-record)
+  "Return the last-modified for BOOKMARK-NAME-OR-RECORD, or nil if none."
+  (bookmark-prop-get bookmark-name-or-record 'last-modified))
+
 (defvar bookmark-history nil
   "The history list for bookmark functions.")
 
@@ -497,6 +509,21 @@ See user option `bookmark-set-fringe'."
               (when (eq 'bookmark (overlay-get temp 'category))
                 (delete-overlay (setq found temp))))))))))
 
+(defun bookmark-maybe-sort-alist ()
+  "Return `bookmark-alist' for display.
+If `bookmark-sort-flag' is T, then return a sorted by name copy of the alist.
+If `bookmark-sort-flag' is LAST-MODIFIED, then return a sorted by last modified
+copy of the alist.  Otherwise, just return `bookmark-alist', which by default
+is ordered from most recently created to least recently created bookmark."
+  (let ((copy (copy-alist bookmark-alist)))
+    (cond ((eq bookmark-sort-flag t)
+           (sort copy (lambda (x y) (string-lessp (car x) (car y)))))
+          ((eq bookmark-sort-flag 'last-modified)
+           (sort copy (lambda (x y)
+                        (time-less-p (bookmark-get-last-modified y)
+                                     (bookmark-get-last-modified x)))))
+          (t copy))))
+
 (defun bookmark-completing-read (prompt &optional default)
   "Prompting with PROMPT, read a bookmark name in completion.
 PROMPT will get a \": \" stuck on the end no matter what, so you
@@ -506,10 +533,8 @@ If DEFAULT is nil then return empty string for empty input."
   (bookmark-maybe-load-default-file) ; paranoia
   (if (listp last-nonmenu-event)
       (bookmark-menu-popup-paned-menu t prompt
-                                      (if bookmark-sort-flag
-                                          (sort (bookmark-all-names)
-                                                'string-lessp)
-                                        (bookmark-all-names)))
+                                      (mapcar 'bookmark-name-from-full-record
+                                              (bookmark-maybe-sort-alist)))
     (let* ((completion-ignore-case bookmark-completion-ignore-case)
            (default (unless (equal "" default) default)))
       (completing-read (format-prompt prompt default)
@@ -630,7 +655,8 @@ If POSN is non-nil, record POSN as the point instead of `(point)'."
                                    (point)
                                    (- (point) bookmark-search-size))
                                   nil))))
-    (position . ,(or posn (point)))))
+    (position . ,(or posn (point)))
+    (last-modified . ,(current-time))))
 
 
 ;;; File format stuff
@@ -1140,15 +1166,6 @@ it to the name of the bookmark currently being set, advancing
                                   (car bookmark-bookmarks-timestamp)))))))
          (bookmark-load (car bookmark-bookmarks-timestamp) t t))))
 
-(defun bookmark-maybe-sort-alist ()
-  "Return `bookmark-alist' for display.
-If `bookmark-sort-flag' is non-nil, then return a sorted copy of the alist.
-Otherwise, just return `bookmark-alist', which by default is ordered
-from most recently created to least recently created bookmark."
-  (if bookmark-sort-flag
-      (sort (copy-alist bookmark-alist)
-            (lambda (x y) (string-lessp (car x) (car y))))
-    bookmark-alist))
 
 
 (defvar bookmark-after-jump-hook nil
@@ -1825,27 +1842,28 @@ Don't affect the buffer ring order."
               entries)))
     ;; The value of `bookmark-sort-flag' might have changed since the
     ;; last time the buffer contents were generated, so re-check it.
-    (if bookmark-sort-flag
-        (progn
-          (setq tabulated-list-sort-key '("Bookmark Name" . nil))
-          (setq tabulated-list-entries entries))
-      (setq tabulated-list-sort-key nil)
-      ;; And since we're not sorting by bookmark name, show bookmarks
-      ;; according to order of creation, with the most recently
-      ;; created bookmarks at the top and the least recently created
-      ;; at the bottom.
-      ;;
-      ;; Note that clicking the column sort toggle for the bookmark
-      ;; name column will invoke the `tabulated-list-mode' sort, which
-      ;; uses `bookmark-bmenu--name-predicate' to sort lexically by
-      ;; bookmark name instead of by (reverse) creation order.
-      ;; Clicking the toggle again will reverse the lexical sort, but
-      ;; the sort will still be lexical not creation-order.  However,
-      ;; if the user reverts the buffer, then the above check of
-      ;; `bookmark-sort-flag' will happen again and the buffer will
-      ;; go back to a creation-order sort.  This is all expected
-      ;; behavior, as documented in `bookmark-bmenu-mode'.
-      (setq tabulated-list-entries (reverse entries)))
+    (cond ((eq bookmark-sort-flag t)
+           (setq tabulated-list-sort-key '("Bookmark Name" . nil)
+                 tabulated-list-entries entries))
+          ((or (null bookmark-sort-flag)
+               (eq bookmark-sort-flag 'last-modified))
+           (setq tabulated-list-sort-key nil)
+           ;; And since we're not sorting by bookmark name, show bookmarks
+           ;; according to order of creation, with the most recently
+           ;; created bookmarks at the top and the least recently created
+           ;; at the bottom.
+           ;;
+           ;; Note that clicking the column sort toggle for the bookmark
+           ;; name column will invoke the `tabulated-list-mode' sort, which
+           ;; uses `bookmark-bmenu--name-predicate' to sort lexically by
+           ;; bookmark name instead of by (reverse) creation order.
+           ;; Clicking the toggle again will reverse the lexical sort, but
+           ;; the sort will still be lexical not creation-order.  However,
+           ;; if the user reverts the buffer, then the above check of
+           ;; `bookmark-sort-flag' will happen again and the buffer will
+           ;; go back to a creation-order sort.  This is all expected
+           ;; behavior, as documented in `bookmark-bmenu-mode'.
+           (setq tabulated-list-entries (reverse entries))))
     ;; Generate the header only after `tabulated-list-sort-key' is
     ;; settled, because if that's non-nil then the sort-direction
     ;; indicator will be shown in the named column, but if it's
@@ -1953,7 +1971,8 @@ At any time you may use \\[revert-buffer] to go back to sorting by creation orde
           ,@(if bookmark-bmenu-toggle-filenames
                 '(("File" 0 bookmark-bmenu--file-predicate)))])
   (setq tabulated-list-padding bookmark-bmenu-marks-width)
-  (when bookmark-sort-flag
+  (when (and bookmark-sort-flag
+             (not (eq bookmark-sort-flag 'last-modified)))
     (setq tabulated-list-sort-key '("Bookmark Name" . nil)))
   (add-hook 'tabulated-list-revert-hook #'bookmark-bmenu--revert nil t)'
   (setq revert-buffer-function 'bookmark-bmenu--revert)
diff --git a/test/lisp/bookmark-tests.el b/test/lisp/bookmark-tests.el
index ae7331fcc2b..a2d8f2d260b 100644
--- a/test/lisp/bookmark-tests.el
+++ b/test/lisp/bookmark-tests.el
@@ -197,6 +197,9 @@ the lexically-bound variable `buffer'."
     (bookmark-maybe-historicize-string "foo")
     (should (equal (car bookmark-history) "foo"))))
 
+(defun bookmark-remove-last-modified (bmk)
+  (assoc-delete-all 'last-modified bmk))
+
 (ert-deftest bookmark-tests-make-record ()
   (with-bookmark-test-file
    (let* ((record `("example.txt" (filename . ,bookmark-tests-example-file)
@@ -206,9 +209,11 @@ the lexically-bound variable `buffer'."
                     (defaults "example.txt"))))
      (with-current-buffer buffer
        (goto-char 3)
-       (should (equal (bookmark-make-record) record))
+       (should (equal (bookmark-remove-last-modified (bookmark-make-record))
+                      record))
        ;; calling twice gives same record
-       (should (equal (bookmark-make-record) record))))))
+       (should (equal (bookmark-remove-last-modified (bookmark-make-record))
+                      record))))))
 
 (ert-deftest bookmark-tests-make-record-list ()
   (with-bookmark-test-file-list
@@ -219,9 +224,11 @@ the lexically-bound variable `buffer'."
                     (defaults "example.txt"))))
      (with-current-buffer buffer
        (goto-char 3)
-       (should (equal (bookmark-make-record) record))
+       (should (equal (bookmark-remove-last-modified (bookmark-make-record))
+                      record))
        ;; calling twice gives same record
-       (should (equal (bookmark-make-record) record))))))
+       (should (equal (bookmark-remove-last-modified (bookmark-make-record))
+                      record))))))
 
 (ert-deftest bookmark-tests-make-record-function ()
   (with-bookmark-test
@@ -255,15 +262,18 @@ the lexically-bound variable `buffer'."
        ;; Set first bookmark
        (goto-char (point-min))
        (bookmark-set "foo")
-       (should (equal bookmark-alist (list bmk1)))
+       (should (equal (mapcar #'bookmark-remove-last-modified bookmark-alist)
+                      (list bmk1)))
        ;; Replace that bookmark
        (goto-char (point-max))
        (bookmark-set "foo")
-       (should (equal bookmark-alist (list bmk2)))
+       (should (equal (mapcar #'bookmark-remove-last-modified bookmark-alist)
+                      (list bmk2)))
        ;; Push another bookmark with the same name
        (goto-char (point-min))
        (bookmark-set "foo" t)                   ; NO-OVERWRITE is t
-       (should (equal bookmark-alist (list bmk1 bmk2)))
+       (should (equal (mapcar #'bookmark-remove-last-modified bookmark-alist)
+                      (list bmk1 bmk2)))
 
        ;; 2. bookmark-set-no-overwrite
        ;; Don't overwrite
@@ -271,11 +281,13 @@ the lexically-bound variable `buffer'."
        ;; Set new bookmark
        (setq bookmark-alist nil)
        (bookmark-set-no-overwrite "foo")
-       (should (equal bookmark-alist (list bmk1)))
+       (should (equal (mapcar #'bookmark-remove-last-modified bookmark-alist)
+                      (list bmk1)))
        ;; Push another bookmark with the same name
        (goto-char (point-max))
        (bookmark-set-no-overwrite "foo" t)        ; PUSH-BOOKMARK is t
-       (should (equal bookmark-alist (list bmk2 bmk1)))
+       (should (equal (mapcar #'bookmark-remove-last-modified bookmark-alist)
+                      (list bmk2 bmk1)))
 
        ;; 3. bookmark-set-internal
        (should-error (bookmark-set-internal "foo" "bar" t))))))