bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

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

The attached patch changes bookmark-bmenu-mode to be based on
tabulated-list-mode instead of special-mode.

This allows us to simplify the code in several cases.  In addition, we
get many features for free, such as sorting columns by clicking on the
column headers, changing size of columns.  In the future, this will
obviously include any new feature added to 'tabulated-list-mode'.

The only functional step backwards is that we no longer support the
optional "inline" header line -- a bookmark.el-specific hack to have a
header without using 'header-line-format'.  I don't believe this
feature is very useful since the lack of such support for anything
similar in e.g. 'package-menu-mode' has not caused any problems.  It
seems to have been added together with 'header-line-format' as a fire
escape if the latter caused any problems.

I recently added a number of tests to bookmark.el on master, which
were developed as part of this suggested change.  These tests pass
using both the new and the old code, which gives some degree of
confidence in this change.

Any comments, objections or suggestions?  Thanks.

Best regards,
Stefan Kangas


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Base-bookmark-bmenu-mode-on-tabulated-list-mode.patch --]
[-- Type: text/x-diff, Size: 20854 bytes --]

From 92413619fa28043eeab72dab1d7278f482028833 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sun, 26 Jan 2020 04:06:15 +0100
Subject: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

Rewriting bookmark-bmenu-mode to be based on 'tabulated-list-mode'
allows us to greatly simplify the code in several cases.  In addition,
we get some features for free, such as sorting by column.

The only functional step backwards is that we no longer support the
optional "inline" header line, a bookmark.el-specific feature to have
a header without using 'header-line-format'.  This feature is believed
to be not very useful or widely used.

* lisp/bookmark.el (tabulated-list): Require.
(bookmark-bmenu-mode): Inherit from 'tabulated-list-mode' instead of
'special-mode' and make the necessary changes to support that.
(bookmark-bmenu-mode-map): Inherit from 'tabulated-list-mode-map'
instead of 'special-mode-map'.  Remove now duplicate key bindings.
(bookmark-bmenu--revert): New function to show the bookmark list using
'tabulated-list-mode'.
(bookmark-bmenu-list): Simplify by using above new function.
(bookmark-bmenu-bookmark): Adapt to 'tabulated-list-mode'.
(bookmark-bmenu--name-predicate)
(bookmark-bmenu--file-predicate): New functions used by
'tabulated-list-mode' to sort.

(bookmark-bmenu-set-header): Redefine as obsolete function alias for
'tabulated-list-init-header'.
(bookmark-bmenu-toggle-filenames, bookmark-bmenu-show-filenames)
(bookmark-bmenu-hide-filenames, bookmark-bmenu-mark)
(bookmark-bmenu-unmark, bookmark-bmenu-delete)
bookmark-bmenu-delete-backwards): Simplify now that we can depend on
'tabulated-list-mode' to do more work.

(bookmark-bmenu-use-header-line)
(bookmark-bmenu-inline-header-height): Declare variables relating to
the now unsupported "inline" header obsolete.
(bookmark-bmenu-ensure-position)
(bookmark-bmenu-execute-deletions): Remove code to handle "inline" header.

* test/lisp/bookmark-tests.el
(bookmark-test-bmenu-edit-annotation/show-annotation)
(bookmark-test-bmenu-unmark, bookmark-test-bmenu-mark): Update tests
for minor changes when using 'tabulated-list-mode'.
---
 etc/NEWS                    |  10 ++
 lisp/bookmark.el            | 270 ++++++++++++------------------------
 test/lisp/bookmark-tests.el |   4 +
 3 files changed, 105 insertions(+), 179 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index c3a71ade8a..fab6228167 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -87,6 +87,16 @@ line numbers that were previously jumped to.
 \f
 * Changes in Specialized Modes and Packages in Emacs 28.1
 
+---
+** The 'list-bookmark' menu is now based on 'tabulated-list-mode'.
+The interactive bookmark list will now benefit from features in
+'tabulated-list-mode' like sorting columns or changing column width.
+
+Support for the optional "inline" header line, allowing for a header
+without using 'header-line-format', has been dropped.  Consequently,
+the variables 'bookmark-bmenu-use-header-line' and
+'bookmark-bmenu-inline-header-height' are now declared obsolete.
+
 ---
 ** The sb-image.el library is now marked obsolete.
 This file was a compatibility kludge which is no longer needed.
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 720ad18c16..5722d2f3fd 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -32,6 +32,7 @@
 ;;; Code:
 
 (require 'pp)
+(require 'tabulated-list)
 (require 'text-property-search)
 (eval-when-compile (require 'cl-lib))
 
@@ -126,16 +127,16 @@ bookmark-automatically-show-annotations
 (defconst bookmark-bmenu-buffer "*Bookmark List*"
   "Name of buffer used for Bookmark List.")
 
-(defcustom bookmark-bmenu-use-header-line t
+(defvar 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."
-  :version "24.4"
-  :type 'boolean)
+This is as opposed to inline text at the top of the buffer.")
+(make-obsolete-variable 'bookmark-bmenu-use-header-line "no longer used." "28.1")
 
 (defconst bookmark-bmenu-inline-header-height 2
   "Number of lines used for the *Bookmark List* header.
 \(This is only significant when `bookmark-bmenu-use-header-line'
 is nil.)")
+(make-obsolete-variable 'bookmark-bmenu-inline-header-height "no longer used." "28.1")
 
 (defconst bookmark-bmenu-marks-width 2
   "Number of columns (chars) used for the *Bookmark List* marks column.
@@ -165,6 +166,7 @@ bookmark-search-delay
   "Time before `bookmark-bmenu-search' updates the display."
   :type  'number)
 
+;; FIXME: Should be declared obsolete.
 (defface bookmark-menu-heading
   '((t (:inherit font-lock-type-face)))
   "Face used to highlight the heading in bookmark menu buffers."
@@ -975,7 +977,7 @@ bookmark-send-edited-annotation
     (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))
+      (bookmark-bmenu-bookmark))
     (kill-buffer old-buffer)))
 
 
@@ -1580,7 +1582,7 @@ bookmark-bmenu-hidden-bookmarks
 
 (defvar bookmark-bmenu-mode-map
   (let ((map (make-keymap)))
-    (set-keymap-parent map special-mode-map)
+    (set-keymap-parent map tabulated-list-mode-map)
     (define-key map "v" 'bookmark-bmenu-select)
     (define-key map "w" 'bookmark-bmenu-locate)
     (define-key map "5" 'bookmark-bmenu-other-frame)
@@ -1599,8 +1601,6 @@ bookmark-bmenu-mode-map
     (define-key map "x" 'bookmark-bmenu-execute-deletions)
     (define-key map "d" 'bookmark-bmenu-delete)
     (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 "m" 'bookmark-bmenu-mark)
@@ -1663,6 +1663,30 @@ bookmark-bmenu-surreptitiously-rebuild-list
         (save-window-excursion
           (bookmark-bmenu-list)))))
 
+(defun bookmark-bmenu--revert ()
+  "Re-populate `tabulated-list-entries'."
+  (let (entries)
+    (dolist (full-record (bookmark-maybe-sort-alist))
+      (let* ((name       (bookmark-name-from-full-record full-record))
+             (annotation (bookmark-get-annotation full-record))
+             (location   (bookmark-location full-record)))
+        (push (list
+               full-record
+               `[,(if (and annotation (not (string-equal annotation "")))
+                      "*" "")
+                 ,(if (display-mouse-p)
+                      (propertize name
+                                  'font-lock-face 'bookmark-menu-bookmark
+                                  'mouse-face 'highlight
+                                  'follow-link t
+                                  'help-echo "mouse-2: go to this bookmark in other window")
+                    name)
+                 ,@(if bookmark-bmenu-toggle-filenames
+                       (list location))])
+              entries)))
+    (tabulated-list-init-header)
+    (setq tabulated-list-entries entries))
+  (tabulated-list-print t))
 
 ;;;###autoload
 (defun bookmark-bmenu-list ()
@@ -1676,70 +1700,18 @@ bookmark-bmenu-list
     (if (called-interactively-p 'interactive)
         (switch-to-buffer buf)
       (set-buffer buf)))
-  (let ((inhibit-read-only t))
-    (erase-buffer)
-    (if (not bookmark-bmenu-use-header-line)
-      (insert "% Bookmark\n- --------\n"))
-    (add-text-properties (point-min) (point)
-			 '(font-lock-face bookmark-menu-heading))
-    (dolist (full-record (bookmark-maybe-sort-alist))
-      (let ((name        (bookmark-name-from-full-record full-record))
-            (annotation  (bookmark-get-annotation full-record))
-            (start       (point))
-            end)
-        ;; if a bookmark has an annotation, prepend a "*"
-        ;; in the list of bookmarks.
-        (insert (if (and annotation (not (string-equal annotation "")))
-                    " *" "  ")
-                name)
-        (setq end (point))
-        (put-text-property
-         (+ bookmark-bmenu-marks-width start) end 'bookmark-name-prop name)
-        (when (display-mouse-p)
-          (add-text-properties
-           (+ bookmark-bmenu-marks-width start) end
-           '(font-lock-face bookmark-menu-bookmark
-	     mouse-face highlight
-             follow-link t
-             help-echo "mouse-2: go to this bookmark in other window")))
-        (insert "\n")))
-    (set-buffer-modified-p (not (= bookmark-alist-modification-count 0)))
-    (goto-char (point-min))
-    (bookmark-bmenu-mode)
-    (if bookmark-bmenu-use-header-line
-	(bookmark-bmenu-set-header)
-      (forward-line bookmark-bmenu-inline-header-height))
-    (when (and bookmark-alist bookmark-bmenu-toggle-filenames)
-      (bookmark-bmenu-toggle-filenames t))))
+  (bookmark-bmenu-mode)
+  (bookmark-bmenu--revert))
 
 ;;;###autoload
 (defalias 'list-bookmarks 'bookmark-bmenu-list)
 ;;;###autoload
 (defalias 'edit-bookmarks 'bookmark-bmenu-list)
 
-;; FIXME: This could also display the current default bookmark file
-;; according to `bookmark-bookmarks-timestamp'.
-(defun bookmark-bmenu-set-header ()
-  "Set the immutable header line."
-  (let ((header (concat "%% " "Bookmark")))
-    (when bookmark-bmenu-toggle-filenames
-      (setq header (concat header
-			   (make-string (- bookmark-bmenu-file-column
-					   (- (length header) 3))  ?\s)
-			   "File")))
-    (let ((pos 0))
-      (while (string-match "[ \t\n]+" header pos)
-	(setq pos (match-end 0))
-	(put-text-property (match-beginning 0) pos 'display
-			   (list 'space :align-to (- pos 1))
-			   header)))
-    (put-text-property 0 2 'face 'fixed-pitch header)
-    (setq header (concat (propertize " " 'display '(space :align-to 0))
-			 header))
-    ;; Code derived from `buff-menu.el'.
-    (setq header-line-format header)))
-
-(define-derived-mode bookmark-bmenu-mode special-mode "Bookmark Menu"
+(define-obsolete-function-alias 'bookmark-bmenu-set-header
+  #'tabulated-list-init-header "28.1")
+
+(define-derived-mode bookmark-bmenu-mode tabulated-list-mode "Bookmark Menu"
   "Major mode for editing a list of bookmarks.
 Each line describes one of the bookmarks in Emacs.
 Letters do not insert themselves; instead, they are commands.
@@ -1773,8 +1745,30 @@ bookmark-bmenu-mode
   in another buffer.
 \\[bookmark-bmenu-show-all-annotations] -- show the annotations of all bookmarks in another buffer.
 \\[bookmark-bmenu-edit-annotation] -- edit the annotation for the current bookmark."
-  (setq truncate-lines t)
-  (setq buffer-read-only t))
+  ;; FIXME: The header could also display the current default bookmark file
+  ;; according to `bookmark-bookmarks-timestamp'.
+  (setq tabulated-list-format
+        `[("" 1) ;; Space to add "*" for bookmark with annotation
+          ("Bookmark" ,bookmark-bmenu-file-column bookmark-bmenu--name-predicate)
+          ,@(if bookmark-bmenu-toggle-filenames
+                '(("File" 0 bookmark-bmenu--file-predicate)))])
+  (setq tabulated-list-padding bookmark-bmenu-marks-width)
+  (setq tabulated-list-sort-key '("Bookmark" . nil))
+  (add-hook 'tabulated-list-revert-hook #'bookmark-bmenu--revert nil t)'
+  (setq revert-buffer-function 'bookmark-bmenu--revert)
+  (tabulated-list-init-header))
+
+
+(defun bookmark-bmenu--name-predicate (a b)
+  "Predicate to sort \"*Bookmark List*\" buffer by the name column.
+This is used for `tabulated-list-format' in `bookmark-bmenu-mode'."
+  (string< (caar a) (caar b)))
+
+
+(defun bookmark-bmenu--file-predicate (a b)
+  "Predicate to sort \"*Bookmark List*\" buffer by the file column.
+This is used for `tabulated-list-format' in `bookmark-bmenu-mode'."
+  (string< (bookmark-location (car a)) (bookmark-location (car b))))
 
 
 (defun bookmark-bmenu-toggle-filenames (&optional show)
@@ -1783,100 +1777,42 @@ bookmark-bmenu-toggle-filenames
   (interactive)
   (cond
    (show
-    (setq bookmark-bmenu-toggle-filenames nil)
-    (bookmark-bmenu-show-filenames)
     (setq bookmark-bmenu-toggle-filenames t))
    (bookmark-bmenu-toggle-filenames
-    (bookmark-bmenu-hide-filenames)
     (setq bookmark-bmenu-toggle-filenames nil))
    (t
-    (bookmark-bmenu-show-filenames)
     (setq bookmark-bmenu-toggle-filenames t)))
-  (when bookmark-bmenu-use-header-line
-    (bookmark-bmenu-set-header)))
-
-
-(defun bookmark-bmenu-show-filenames (&optional force)
-  "In an interactive bookmark list, show filenames along with bookmarks.
-Non-nil FORCE forces a redisplay showing the filenames.  FORCE is used
-mainly for debugging, and should not be necessary in normal use."
-  (if (and (not force) bookmark-bmenu-toggle-filenames)
-      nil ;already shown, so do nothing
-    (with-buffer-modified-unmodified
-     (save-excursion
-       (save-window-excursion
-         (goto-char (point-min))
-	 (if (not bookmark-bmenu-use-header-line)
-	     (forward-line bookmark-bmenu-inline-header-height))
-         (setq bookmark-bmenu-hidden-bookmarks ())
-         (let ((inhibit-read-only t))
-           (while (< (point) (point-max))
-             (let ((bmrk (bookmark-bmenu-bookmark)))
-               (push bmrk bookmark-bmenu-hidden-bookmarks)
-               (let ((start (line-end-position)))
-                 (move-to-column bookmark-bmenu-file-column t)
-                 ;; Strip off `mouse-face' from the white spaces region.
-                 (if (display-mouse-p)
-                     (remove-text-properties start (point)
-                                             '(mouse-face nil help-echo nil))))
-               (delete-region (point) (progn (end-of-line) (point)))
-               (insert "  ")
-               ;; Pass the NO-HISTORY arg:
-               (bookmark-insert-location bmrk t)
-               (forward-line 1)))))))))
-
-
-(defun bookmark-bmenu-hide-filenames (&optional force)
-  "In an interactive bookmark list, hide the filenames of the bookmarks.
-Non-nil FORCE forces a redisplay showing the filenames.  FORCE is used
-mainly for debugging, and should not be necessary in normal use."
-  (when (and (not force) bookmark-bmenu-toggle-filenames)
-    ;; nothing to hide if above is nil
-    (with-buffer-modified-unmodified
-     (save-excursion
-       (goto-char (point-min))
-       (if (not bookmark-bmenu-use-header-line)
-	   (forward-line bookmark-bmenu-inline-header-height))
-       (setq bookmark-bmenu-hidden-bookmarks
-             (nreverse bookmark-bmenu-hidden-bookmarks))
-       (let ((inhibit-read-only t))
-         (while bookmark-bmenu-hidden-bookmarks
-           (move-to-column bookmark-bmenu-marks-width t)
-           (bookmark-kill-line)
-           (let ((name  (pop bookmark-bmenu-hidden-bookmarks))
-                 (start (point)))
-             (insert name)
-             (put-text-property start (point) 'bookmark-name-prop name)
-             (if (display-mouse-p)
-                 (add-text-properties
-                  start (point)
-                  '(font-lock-face bookmark-menu-bookmark
-		    mouse-face highlight
-		    follow-link t help-echo
-                    "mouse-2: go to this bookmark in other window"))))
-           (forward-line 1)))))))
+  (bookmark-bmenu-surreptitiously-rebuild-list))
+
+
+(defun bookmark-bmenu-show-filenames (&optional _)
+  "In an interactive bookmark list, show filenames along with bookmarks."
+  (setq bookmark-bmenu-toggle-filenames t)
+  (bookmark-bmenu-surreptitiously-rebuild-list))
+
+
+(defun bookmark-bmenu-hide-filenames (&optional _)
+  "In an interactive bookmark list, hide the filenames of the bookmarks."
+  (setq bookmark-bmenu-toggle-filenames nil)
+  (bookmark-bmenu-surreptitiously-rebuild-list))
 
 
 (defun bookmark-bmenu-ensure-position ()
   "If point is not on a bookmark line, move it to one.
-If before the first bookmark line, move to the first; if after the
-last full line, move to the last full line.  The return value is undefined."
-  (cond ((and (not bookmark-bmenu-use-header-line)
-	      (< (count-lines (point-min) (point))
-		 bookmark-bmenu-inline-header-height))
-         (goto-char (point-min))
-         (forward-line bookmark-bmenu-inline-header-height))
-        ((and (bolp) (eobp))
+If after the last full line, move to the last full line.  The
+return value is undefined."
+  (cond ((and (bolp) (eobp))
          (beginning-of-line 0))))
 
 
 (defun bookmark-bmenu-bookmark ()
   "Return the bookmark for this line in an interactive bookmark list buffer."
   (bookmark-bmenu-ensure-position)
-  (save-excursion
-    (beginning-of-line)
-    (forward-char bookmark-bmenu-marks-width)
-    (get-text-property (point) 'bookmark-name-prop)))
+  (let* ((id (tabulated-list-get-id))
+         (entry (and id (assoc id tabulated-list-entries))))
+    (if entry
+        (caar entry)
+      "")))
 
 
 (defun bookmark-show-annotation (bookmark-name-or-record)
@@ -1924,14 +1860,8 @@ bookmark-show-all-annotations
 (defun bookmark-bmenu-mark ()
   "Mark bookmark on this line to be displayed by \\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-select]."
   (interactive)
-  (beginning-of-line)
   (bookmark-bmenu-ensure-position)
-  (with-buffer-modified-unmodified
-   (let ((inhibit-read-only t))
-     (delete-char 1)
-     (insert ?>)
-     (forward-line 1)
-     (bookmark-bmenu-ensure-position))))
+  (tabulated-list-put-tag ">" t))
 
 
 (defun bookmark-bmenu-select ()
@@ -2082,17 +2012,12 @@ bookmark-bmenu-unmark
   "Cancel all requested operations on bookmark on this line and move down.
 Optional BACKUP means move up."
   (interactive "P")
-  (beginning-of-line)
+  ;; any flags to reset according to circumstances?  How about a
+  ;; flag indicating whether this bookmark is being visited?
+  ;; well, we don't have this now, so maybe later.
   (bookmark-bmenu-ensure-position)
-  (with-buffer-modified-unmodified
-   (let ((inhibit-read-only t))
-     (delete-char 1)
-     ;; any flags to reset according to circumstances?  How about a
-     ;; flag indicating whether this bookmark is being visited?
-     ;; well, we don't have this now, so maybe later.
-     (insert " "))
-   (forward-line (if backup -1 1))
-   (bookmark-bmenu-ensure-position)))
+  (tabulated-list-put-tag " ")
+  (forward-line (if backup -1 1)))
 
 
 (defun bookmark-bmenu-backup-unmark ()
@@ -2109,14 +2034,8 @@ 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]."
   (interactive)
-  (beginning-of-line)
   (bookmark-bmenu-ensure-position)
-  (with-buffer-modified-unmodified
-   (let ((inhibit-read-only t))
-     (delete-char 1)
-     (insert ?D)
-     (forward-line 1)
-     (bookmark-bmenu-ensure-position))))
+  (tabulated-list-put-tag "D" t))
 
 
 (defun bookmark-bmenu-delete-backwards ()
@@ -2124,10 +2043,7 @@ bookmark-bmenu-delete-backwards
 To carry out the deletions that you've marked, use \\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-execute-deletions]."
   (interactive)
   (bookmark-bmenu-delete)
-  (forward-line -2)
-  (bookmark-bmenu-ensure-position)
-  (forward-line 1)
-  (bookmark-bmenu-ensure-position))
+  (forward-line -2))
 
 
 (defun bookmark-bmenu-execute-deletions ()
@@ -2143,8 +2059,6 @@ bookmark-bmenu-execute-deletions
                        (progn (end-of-line) (point))))))
         (o-col     (current-column)))
     (goto-char (point-min))
-    (unless bookmark-bmenu-use-header-line
-      (forward-line 1))
     (while (re-search-forward "^D" (point-max) t)
       (bookmark-delete (bookmark-bmenu-bookmark) t)) ; pass BATCH arg
     (bookmark-bmenu-list)
@@ -2269,8 +2183,6 @@ bookmark-menu-popup-paned-menu
 ;; We MUST autoload EACH form used to set up this variable's value, so
 ;; that the whole job is done in loaddefs.el.
 
-;; Emacs menubar stuff.
-
 ;;;###autoload
 (defvar menu-bar-bookmark-map
   (let ((map (make-sparse-keymap "Bookmark functions")))
diff --git a/test/lisp/bookmark-tests.el b/test/lisp/bookmark-tests.el
index b9c6ff9c54..ab85a94f3d 100644
--- a/test/lisp/bookmark-tests.el
+++ b/test/lisp/bookmark-tests.el
@@ -361,6 +361,8 @@ bookmark-test-bmenu-send-edited-annotation/restore-focus
    (insert "foo")
    (bookmark-send-edited-annotation)
    (should (equal (buffer-name (current-buffer)) bookmark-bmenu-buffer))
+   (beginning-of-line)
+   (forward-char 4)
    (should (looking-at "name"))))
 
 (ert-deftest bookmark-test-bmenu-toggle-filenames ()
@@ -393,6 +395,7 @@ bookmark-test-bmenu-bookmark
 (ert-deftest bookmark-test-bmenu-mark ()
   (with-bookmark-bmenu-test
    (bookmark-bmenu-mark)
+   (forward-line -1)
    (beginning-of-line)
    (should (looking-at "^>"))))
 
@@ -407,6 +410,7 @@ bookmark-test-bmenu-unmark
    (bookmark-bmenu-mark)
    (goto-char (point-min))
    (bookmark-bmenu-unmark)
+   (forward-line -1)
    (beginning-of-line)
    (should (looking-at "^  "))))
 
-- 
2.20.1

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

> The attached patch changes bookmark-bmenu-mode to be based on
> tabulated-list-mode instead of special-mode.
> 
> This allows us to simplify the code in several cases.  In addition, we
> get many features for free, such as sorting columns by clicking on the
> column headers, changing size of columns.  In the future, this will
> obviously include any new feature added to 'tabulated-list-mode'.
> 
> The only functional step backwards is that we no longer support the
> optional "inline" header line -- a bookmark.el-specific hack to have a
> header without using 'header-line-format'.  I don't believe this
> feature is very useful since the lack of such support for anything
> similar in e.g. 'package-menu-mode' has not caused any problems.  It
> seems to have been added together with 'header-line-format' as a fire
> escape if the latter caused any problems.
> 
> I recently added a number of tests to bookmark.el on master, which
> were developed as part of this suggested change.  These tests pass
> using both the new and the old code, which gives some degree of
> confidence in this change.
> 
> Any comments, objections or suggestions?  Thanks.

I _strongly_ object to this.  And I would say the same
thing if such a suggestion were made for Dired.

If vanilla Emacs does this then I will have to separate
Bookmark+ completely from `bookmark.el', incorporating
its code prior to your change.

I don't want to do that, but I will have to (and it
won't be hard to do - that's not the problem).  Until
now, I've made a concerted effort to be compatible with
vanilla `bookmark.el', for the benefit of users.

It's very wrong to think either that things like the
bookmark-list display and Dired's listings are as
simple as what `tabulated-list-mode' provides, or that
their features can be easily added on top of
`tabulated-list-mode'.

This kind of proposal is, IMO, a consequence of one or
both of the following:

1. Favoring development, or rather maintenance, over
   user convenience, power, and features.

   The imagined gain is a chimera.  The code has been
   used for a very long time, and there is little
   maintenance burden.

2. Not appreciating the specificity of the features
   offered by libraries such as Dired and bookmarking.

   Not being well acquainted with such features, and
   so supposing that they don't exist or they're no
   big deal and not worth bothering about.

   Being acquainted with `tabulated-list-mode', and
   thinking its features are wonderful, abundant, and
   sufficiently general and flexible.

   Consider sorting, as just one example among many
   (yes, many).

   Sorting in Dired or the bookmark list (at least with
   Bookmark+) is _much more_ useful and flexible than
   just sorting columns.  Why?  Because the things
   displayed are of different kinds across rows, not
   just across columns.  The columns, for bookmark-list
   display, are less interesting (this is less true of
   Dired, but the same consideration holds).

   Here are the predefined ways you can sort bookmarks
   with Bookmark+.  And users can easily define their
   own ways of sorting, just as they can define their
   own kinds of bookmarks.

   key     binding
   ---     -------

   s C-r   bmkp-reverse-multi-sort-order
   s *     bmkp-bmenu-sort-modified-before-unmodified
   s 0     bmkp-bmenu-sort-by-creation-time
   s >     bmkp-bmenu-sort-marked-before-unmarked
   s D     bmkp-bmenu-sort-flagged-before-unflagged
   s I     bmkp-bmenu-sort-by-Info-position
   s a     bmkp-bmenu-sort-annotated-before-unannotated
   s b     bmkp-bmenu-sort-by-last-buffer-or-file-access
   s d     bmkp-bmenu-sort-by-last-bookmark-access
   s f d   bmkp-bmenu-sort-by-last-local-file-access
   s f k   bmkp-bmenu-sort-by-local-file-type
   s f n   bmkp-bmenu-sort-by-file-name
   s f s   bmkp-bmenu-sort-by-local-file-size
   s f u   bmkp-bmenu-sort-by-last-local-file-update
   s g     bmkp-bmenu-sort-by-Gnus-thread
   s i     bmkp-bmenu-sort-by-Info-node-name
   s k     bmkp-bmenu-sort-by-bookmark-type
   s n     bmkp-bmenu-sort-by-bookmark-name
   s r     bmkp-reverse-sort-order
   s s     bmkp-bmenu-change-sort-order-repeat
   s t     bmkp-bmenu-sort-tagged-before-untagged
   s u     bmkp-bmenu-sort-by-url
   s v     bmkp-bmenu-sort-by-bookmark-visit-frequency

   What's more, you can combine sort orders - see

https://www.emacswiki.org/emacs/BookmarkPlus#SortingBookmarks

---

May I invite you to please spend your (much-appreciated)
volunteer effort on something else?  This isn't broken,
and your proposal would shatter it.

Do I use `tabulated-list-mode', or am I just grousing,
as an old fart stuck in his ways and unfamiliar with
`tabulated-list-mode'?

Yes, I use it (e.g. in my library `apu.el').  But it
is very limited, and the limitations do not just stem
from a lack of more generic features.  I use it only
when it's appropriate.

There's only so much you will ever be able to get out
of `tabulated-list-mode'.  Apply it so simple listings
that don't need or offer much functionality.  Please
leave the sophisticated, useful listing displays alone.

This is a have-a-hammer-and-see-only-nails story.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

Drew Adams <drew.adams@oracle.com> writes:

>> Any comments, objections or suggestions?  Thanks.
>
> I _strongly_ object to this.  And I would say the same
> thing if such a suggestion were made for Dired.

- This is not about Dired, so let's please leave that to one side.

- This is not just code cleanup.  I think 'tabulated-list-mode' gives
  clear benefits to users today.  Sorting is one example which will
  immediately improve.  Mouse support is another.

- I'm not convinced that it's necessary, but I don't see the problem
  with an interactive bookmark menu in bookmark+.el (or any
  third-party package really) that is not directly based on the code
  in Emacs.

> 2. Not appreciating the specificity of the features
>    offered by libraries such as Dired and bookmarking.
>
>    Not being well acquainted with such features, and
>    so supposing that they don't exist or they're no
>    big deal and not worth bothering about.

Which features are you talking about?

>   Sorting in Dired or the bookmark list (at least with
>   Bookmark+) is _much more_ useful and flexible than
>   just sorting columns.  Why?  Because the things
>   displayed are of different kinds across rows, not
>   just across columns.  The columns, for bookmark-list
>   display, are less interesting (this is less true of
>   Dired, but the same consideration holds).

I don't understand this argument.  It seems to be based on the
misunderstanding that 'tabulated-list-mode' somehow limits you to sort
by column.

> Yes, I use it (e.g. in my library `apu.el').  But it
> is very limited, and the limitations do not just stem
> from a lack of more generic features.  I use it only
> when it's appropriate.

To my mind, this is the most convincing argument you presented.  Yet
you have not shown any limitations in 'tabulated-list-mode' that makes
it unsuitable for 'bookmark-bmenu-mode'.  Could you please explain
which limitations you see, and how you concretely see them affecting
bookmark-bmenu-mode?

Best regards,
Stefan Kangas

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

I think I've said what I have to say about this.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

Drew Adams <drew.adams@oracle.com> writes:

> I think I've said what I have to say about this.

Revisiting this now, I would have been interested to know the answer
to this question: "Could you please explain which limitations you see,
and how you concretely see them affecting bookmark-bmenu-mode?"

I would also be interested to know if there is anything that could be
done to improve 'tabulated-list-mode' to get rid of any such
limitations.

Best regards,
Stefan Kangas

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Matthias Meulien]

Stefan Kangas <stefan@marxist.se> writes:

> The attached patch changes bookmark-bmenu-mode to be based on 
> tabulated-list-mode instead of special-mode. 
> 
> This allows us to simplify the code in several cases.  In 
> addition, we get many features for free, such as sorting columns 
> by clicking on the column headers, changing size of columns.

Both features looks interesting to me. Thanks for working on that!

> In the future, this will obviously include any new feature added 
> to 'tabulated-list-mode'. 
> 
> The only functional step backwards is that we no longer support 
> the optional "inline" header line -- a bookmark.el-specific hack 
> to have a header without using 'header-line-format'.  I don't 
> believe this feature is very useful since the lack of such 
> support for anything similar in e.g. 'package-menu-mode' has not 
> caused any problems.  It seems to have been added together with 
> 'header-line-format' as a fire escape if the latter caused any 
> problems.

I am the one who introduced `header-line-format` in bookmark.el 
(7a78e19f24) and I confirm that I kept the legacy header "just in 
case".  The new behavior has been enabled by default since 2013, 
and I've not heard any complaint.
-- 
Matthias Meulien

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

Matthias Meulien <orontee@gmail.com> writes:

> Both features looks interesting to me. Thanks for working on that!

Thanks!

>> The only functional step backwards is that we no longer support
>> the optional "inline" header line -- a bookmark.el-specific hack
>> to have a header without using 'header-line-format'.  I don't
>> believe this feature is very useful since the lack of such
>> support for anything similar in e.g. 'package-menu-mode' has not
>> caused any problems.  It seems to have been added together with
>> 'header-line-format' as a fire escape if the latter caused any
>> problems.
>
> I am the one who introduced `header-line-format` in bookmark.el
> (7a78e19f24) and I confirm that I kept the legacy header "just in
> case".  The new behavior has been enabled by default since 2013,
> and I've not heard any complaint.

This is useful information which confirms that we can safely remove it.

Best regards,
Stefan Kangas

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

Hi Drew,

Just to bring your attention to the below message, which your email
provider bounced for some reason (and I missed it did so at the
time).

Stefan Kangas <stefan@marxist.se> writes:

> Revisiting this now, I would have been interested to know the answer
> to this question: "Could you please explain which limitations you see,
> and how you concretely see them affecting bookmark-bmenu-mode?"
>
> I would also be interested to know if there is anything that could be
> done to improve 'tabulated-list-mode' to get rid of any such
> limitations.

As you understand, my preference is in favour of the change.  But I've
been uncomfortable pushing a change that basically only received
negative feedback.  Now we have had another person express support for
the change, which would make the consensus lean more in its favour.

Ideally, of course, we would be able to resolve the issues involved to
everyones satisfaction.  I think we need to clarify the above questions
to be able to figure out if that's doable or not.

Thanks in advance.

Best regards,
Stefan Kangas

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

> > The attached patch changes bookmark-bmenu-mode to be based on
> > tabulated-list-mode instead of special-mode.
> >
> > This allows us to simplify the code in several cases.  In
> > addition, we get many features for free, such as sorting columns
> > by clicking on the column headers, changing size of columns.
> 
> Both features looks interesting to me. Thanks for working on that!
> 
> > In the future, this will obviously include any new feature added
> > to 'tabulated-list-mode'.
> >
> > The only functional step backwards is that we no longer support
> > the optional "inline" header line -- a bookmark.el-specific hack
> > to have a header without using 'header-line-format'.  I don't
> > believe this feature is very useful since the lack of such
> > support for anything similar in e.g. 'package-menu-mode' has not
> > caused any problems.  It seems to have been added together with
> > 'header-line-format' as a fire escape if the latter caused any
> > problems.

Please don't do that.  As I said earlier:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39293#8

this is totally incompatible with the many
enhancements I've made to bookmarks.  It will
force me to include the (former) bookmark.el
code in Bookmark+, and separate the latter
completely from bookmark.el.  Until now, it's
been a compatible extension.

Tabulated-list mode is a step backward for
any feature that offers more.  Just because
something is capable of displaying some info
in columns, that doesn't mean it should be
reduced to the limitations of t-l mode.

Thinking that t-l mode is for all columnar
presentations and interactions is akin to
thinking that a soda straw is appropriate
for an oil pipeline.

This is serious for me.  If you don't care,
too bad for me, and for anyone who cares
about Bookmark+ and compatibility with
vanilla Emacs.

> I am the one who introduced `header-line-format in bookmark.el
> (7a78e19f24) and I confirm that I kept the legacy header "just in
> case".  The new behavior has been enabled by default since 2013,
> and I've not heard any complaint.

I would have complained about it.  But I've
been able to ignore it, because a "legacy
header" is still supported.  Enabled only by
default means that hearing no complaints
signifies nothing.

If a user option is provided to let users NOT
have tabulated-list display imposed, and let
them continue as before, then fine.  Otherwise
I'll be forced to fork, I'm afraid.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

Hi Stefan,

Our mails crossed just now.

> Just to bring your attention to the below message, which your email
> provider bounced for some reason (and I missed it did so at the
> time).
> 
> Stefan Kangas <stefan@marxist.se> writes:
> 
> > Revisiting this now, I would have been interested to know the answer
> > to this question: "Could you please explain which limitations you
> > see, and how you concretely see them affecting bookmark-bmenu-mode?"

See my message of Jan 26 (url in previous mail).

> > I would also be interested to know if there is anything that could be
> > done to improve 'tabulated-list-mode' to get rid of any such
> > limitations.

Provide a user option.  Make use of tabulated-list
mode optional.

> As you understand, my preference is in favour of the change.  But I've
> been uncomfortable pushing a change that basically only received
> negative feedback.  Now we have had another person express support for
> the change, which would make the consensus lean more in its favour.
> 
> Ideally, of course, we would be able to resolve the issues involved to
> everyones satisfaction.  I think we need to clarify the above questions
> to be able to figure out if that's doable or not.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

BTW, I already lived through this with buff-menu.el.

My library buff-menu+.el offered features it didn't.

But unlike Bookmark+, it wasn't such a big deal that
I cared to bother about it.  So buff-menu+.el doesn't
work with Emacs > release 24.1.

Not a big deal, in that case, and tabulated-list
mode is, in fact, a reasonable fit for the buffer
menu, which is a small feature with few complications
or bells and whistles.

https://www.emacswiki.org/emacs/BufferMenuPlus

But you'll notice that ibuffer.el has _not_ been
stuffed into the tabulated-list mold.  Why not?
It certainly could be.  But it's more feature-rich
than buff-menu.

(I don't have an ibuffer extension library, so I
won't scream when someone decides to clean house
and shove tabulated-list down ibuffer's throat.)

The difference with Bookmark+ is that ibuffer.el
is part of Emacs proper.  Though Bookmark+ has
users, their complaints wouldn't have the same
effect as the complaints of ibuffer users.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

Drew Adams <drew.adams@oracle.com> writes:

>> > Revisiting this now, I would have been interested to know the answer
>> > to this question: "Could you please explain which limitations you
>> > see, and how you concretely see them affecting bookmark-bmenu-mode?"
>
> See my message of Jan 26 (url in previous mail).

Thanks, that unfortunately takes us back to square one.

As you know, I've tried to understand that email before, and posed a
series of questions in a reply the same day.  Could we please restart
the discussion from there?

1. You wrote that I don't appreciate "the specificity of the features
   offered by libraries such as Dired and bookmarking".  I replied:
   "Which features are you talking about?"

   Could you please clarify this point?

2.  You wrote:

  >   Sorting in Dired or the bookmark list (at least with
  >   Bookmark+) is _much more_ useful and flexible than
  >   just sorting columns.  Why?  Because the things
  >   displayed are of different kinds across rows, not
  >   just across columns.

  I replied that AFAIU you are not limited to sorting across columns
  using tabulated-list-mode.

  Can we therefore consider that part settled?

3. You wrote:

  > Yes, I use it (e.g. in my library `apu.el').  But it
  > is very limited, and the limitations do not just stem
  > from a lack of more generic features.  I use it only
  > when it's appropriate.

  I replied: "Could you please explain which limitations you see, and how
  you concretely see them affecting bookmark-bmenu-mode?"

  Could you please clarify this point?

> Provide a user option.  Make use of tabulated-list
> mode optional.

Maybe, but first we need to understand why it is even needed in the
first place.

Best regards,
Stefan Kangas

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Karl Fogel]

I too would like to hear what specifically would be limited or broken by switching to use `tabulated-list-mode' here.

This does not mean I'm necessarily in favor of applying the patch; I just think Stefan's question is important, and I don't see it given a concrete answer anywhere.  What would break in Bookmark+ if this patch were applied to bookmark.el, and/or what new limitations would bookmark.el after this patch?

IOW, I'd just like to understand the tradeoffs better here.

Best regards,
-Karl

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

> I too would like to hear what specifically would be limited or broken
> by switching to use `tabulated-list-mode' here.
> 
> This does not mean I'm necessarily in favor of applying the patch; I
> just think Stefan's question is important, and I don't see it given a
> concrete answer anywhere.  What would break in Bookmark+ if this patch
> were applied to bookmark.el, and/or what new limitations would
> bookmark.el after this patch?
> 
> IOW, I'd just like to understand the tradeoffs better here.

Sorry, I really don't have the time to deal with
this now.

Bookmark+ has lots of added features, and many
have to do with the bookmark-list display.  If
you're interested in that, please see its doc.

Maybe at some point later I'll have the time and
will to tick off things in Bookmark+ that t-l-mode
interferes with or prohibits.

I think at this point you'll just have to take my
word for it (or not) that it would be far too much
work, too risky, and for no benefit, for me to try
to rewrite such features to adapt to t-l-mode.

And I'm pretty sure that some things would need to
be sacrificed.  In my experience just with `apu.el'
(which uses t-l-mode) I ran into limitations that
I had to work around (no, I don't recall what they
were).

[Can a t-l-mode buffer even have a title (not just
column headings)?  With Bookmark+ the listing
reflects the current sorting and filterings, and
the title at the top tells you what the listing is
about.]

I'm sorry to say it, but I won't try, for Bookmark+.
I don't have the time to waste on that.  Sorry.

Do what you think you have to do.  I'll do what I
have to do, given my limited resources.  I expect
I'll likely just incorporate the former bookmark.el
code that Bookmark+ currently takes for granted.
___

FWIW, I also don't think that bookmark.el's list
of bookmarks is a great candidate for t-l-mode.
I don't think it adds anything important for such
a simple list with 2-3 columns.  As I said in my
Jan 26 mail, sorting by those columns (which is
really all that t-l-mode offers here) is not so
helpful.  (OK, it has some use.)

Maybe consider ibuffer.el instead?  As I said
earlier, trying ibuffer, which offers a bit more
than a rudimentary listing, might point to some
t-l-mode limitations or complications.  And if
it doesn't then so much the better. ;-)
___

To be clear, I don't think I said that anything
would be limited or broken in _bookmark.el_ by
using t-l-mode.  Potential uses of its features,
and existing uses by 3rd-party libraries (e.g.
Bookmark+), could be limited or broken.  But I
doubt that anything bookmark.el offers out of
the box would be affected much, if at all.
(bookmark.el could probably drop a good deal of
its code without breaking anything that people
use much).

Thx - Drew

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Karl Fogel]

On 26 May 2020, Drew Adams wrote:
>Sorry, I really don't have the time to deal with
>this now.
>
>Bookmark+ has lots of added features, and many
>have to do with the bookmark-list display.  If
>you're interested in that, please see its doc.
>
>Maybe at some point later I'll have the time and
>will to tick off things in Bookmark+ that t-l-mode
>interferes with or prohibits.
>
>I think at this point you'll just have to take my
>word for it (or not) that it would be far too much
>work, too risky, and for no benefit, for me to try
>to rewrite such features to adapt to t-l-mode.

Okay, fair enough -- I understand about limited time.

>And I'm pretty sure that some things would need to
>be sacrificed.  In my experience just with `apu.el'
>(which uses t-l-mode) I ran into limitations that
>I had to work around (no, I don't recall what they
>were).
>
>[Can a t-l-mode buffer even have a title (not just
>column headings)?  With Bookmark+ the listing
>reflects the current sorting and filterings, and
>the title at the top tells you what the listing is
>about.]
>
>I'm sorry to say it, but I won't try, for Bookmark+.
>I don't have the time to waste on that.  Sorry.
>
>Do what you think you have to do.  I'll do what I
>have to do, given my limited resources.  I expect
>I'll likely just incorporate the former bookmark.el
>code that Bookmark+ currently takes for granted.

Well, I don't think we "have to" convert bookmark.el to use t-l-mode at all.

>FWIW, I also don't think that bookmark.el's list
>of bookmarks is a great candidate for t-l-mode.
>I don't think it adds anything important for such
>a simple list with 2-3 columns.  As I said in my
>Jan 26 mail, sorting by those columns (which is
>really all that t-l-mode offers here) is not so
>helpful.  (OK, it has some use.)
>
>Maybe consider ibuffer.el instead?  As I said
>earlier, trying ibuffer, which offers a bit more
>than a rudimentary listing, might point to some
>t-l-mode limitations or complications.  And if
>it doesn't then so much the better. ;-)

Well, I mean, bookmark.el seems to be working fine the way it is right now, without t-l-mode nor ibuffer.  So I'm not sure the proposed change is warranted.

Stefan, is there a strong motivation here, other than the obvious attractions of re-using code and avoiding multiple implementations of similar functionality?

(If we decide not to make the change, then we should add a comment to bookmark.el pointing to this bug-ticket discussion and explaining *why* we have left the code as-is.)

>To be clear, I don't think I said that anything
>would be limited or broken in _bookmark.el_ by
>using t-l-mode.  Potential uses of its features,
>and existing uses by 3rd-party libraries (e.g.
>Bookmark+), could be limited or broken.  But I
>doubt that anything bookmark.el offers out of
>the box would be affected much, if at all.
>(bookmark.el could probably drop a good deal of
>its code without breaking anything that people
>use much).

Agreed; that's what I understood you to be saying.

Best regards,
-Karl

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

Sounds reasonable.  Thanks for listening.

Obviously I'd prefer to keep Bookmark+ as
an extension of bookmark.el.  Among other
things, that might make my job easier (dunno),
and I'd pay attention more to changes (e.g.
improvements) to bookmark.el.

The *bmenu* stuff is only one part of
bookmarking.  It would be kind of a shame
for me to give up on bookmark.el just for
that.

 - d

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

Karl Fogel <kfogel@red-bean.com> writes:

> Stefan, is there a strong motivation here, other than the obvious
> attractions of re-using code and avoiding multiple implementations of
> similar functionality?

I don't see any strong motivation here, no.

Some features of tabulated-list-mode would be nice to have, such as
sorting by column and better mouse support.

Best regards,
Stefan Kangas

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Basil L. Contovounesios]

Stefan Kangas <stefan@marxist.se> writes:

> Karl Fogel <kfogel@red-bean.com> writes:
>
>> Stefan, is there a strong motivation here, other than the obvious
>> attractions of re-using code and avoiding multiple implementations of
>> similar functionality?
>
> I don't see any strong motivation here, no.
>
> Some features of tabulated-list-mode would be nice to have, such as
> sorting by column and better mouse support.

FWIW I'm generally in favour of reusing core, general infrastructure
wherever applicable, because IMO the benefits outweigh the downsides,
but I don't know enough about bookmark-bmenu-mode or tabulated-list-mode
to comment on that part of this discussion.

What I will comment on is that, unless bookmark.el provides a specific
public API (not just functions, but rather any format that's documented
and can be relied on externally), then external extensions to it should
not rely on its internal implementation.  Packages should not be limited
by assumptions made by external extensions.  Besides, why are these
extensions external to bookmark.el to begin with?  Surely useful
extensions should be included upstream.

Thanks,

-- 
Basil

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

> unless bookmark.el provides a specific
> public API (not just functions, but rather any format
> that's documented and can be relied on externally), then
> external extensions to it should not rely on its internal
> implementation.  Packages should not be limited by
> assumptions made by external extensions.  Besides, why
> are these extensions external to bookmark.el to begin
> with?  Surely useful extensions should be included upstream.

1. I wonder how familiar you are with bookmark.el, its code,
   and the bookmark formats it defines.

2. Basing the bookmark-list display on `tabulated-list-mode'
   could not, by any stretch of the imagination, be
   considered "internal implementation".  The immediate
   behavior change for users would be minor, yes.  But the
   repercussions of the change would be large for users,
   in terms of limiting future behavior enhancements. 

3. My opinion is anyway that there's nothing "internal"
   in GNU Emacs, or in free software in general.

   You say, "Packages should not be limited by assumptions
   made by external extensions."

   You meant that for packages distributed as part of Emacs.
   But the reverse is also true: 3rd-party packages should
   not be limited by all of the assumptions made by vanilla
   Emacs code at any given time.

   Sure, no one forces anyone else to abide by any sense of
   cooperation.  But there has been some mutual cooperation
   and respect over the years.  3rd-party code depends, at
   any given time, on what Emacs provides, not vice versa,
   of course.

   But in the long run what Emacs provides depends in part
   on what goes on outside its parochial development.  An
   attitude that "core" Emacs development shouldn't care to
   look at what's happening in the wider community is
   self-limiting.

   Telling 3rd-party developers that you don't need to
   listen to them, don't need to care about what they say
   or ask for is, yes, within your rights.  Core Emacs need
   not care, in any sense of legal obligation.  But then
   don't wonder about separation between the core and the
   larger community.

4. The format of bookmarks _is_ documented, in bookmark.el
   comments.  Bookmark+ respects that format, and extends
   it.  That format has changed over time, and Bookmark+
   has adapted, to handle the old formats as well as the
   new ones.

5. Anyone is free to extend the "format" of bookmarks.
   That's entirely expected.  New kinds of bookmarks can
   be defined, and any new fields can be added.  What's
   important is that the basic structure defined by/for
   bookmark.el be respected, but nothing prevents adding
   additional fields etc.  That's as it should be.  There
   is also nothing wrong with enhancing or otherwise
   changing the use (behavior) of existing fields, as long
   as such behavior changes are clearly documented for
   users and use of the 3rd-party library is optional.

6. I'm very conservative in my enhancements of vanilla
   behavior.  I try as much as possible not to modify
   existing code, etc.  (But some of my code that tries
   be compatible with older Emacs releases can't use
   nadvice etc., so there is more actual modification.)

   I avoid changing things gratuitously for several
   reasons, not the least of which is the maintenance
   burden of updating my code as Emacs code changes.

   You have only to notice that many of my libraries are
   named ____+.el, where the ____ is the name of a
   standard Emacs library.

   Those `+' libraries of mine typically start out as
   minor add-ons to the existing vanilla code, sometimes
   as a result of not being able to persuade "core" to add
   some feature, and sometimes because the library is, so
   far, only for my own use - just personal customization.

   Bookmark+ started out that way, for example.  From 2004
   to 2009 it consisted only of some code I used myself, to
   remain compatible with Emacs 20.  Starting in 2009 were
   added (1) the ability to bookmark a region and (2)
   display-list commands to list only W3M, Info, Gnus,
   files, and region bookmarks.  And so on - more features
   were added.

   The point is that I always based the library on vanilla
   bookmark.el.  Even as many more features were added, and
   the bookmark.el code it made use of accounted for little,
   I kept the dependency.  Why?  To be sure it continued to
   work well with vanilla bookmarks, and to oblige it to
   keep up-to-date with any new features that bookmark.el
   might add.

7. Everything in Bookmark+ has, from the beginning, been
   offered to vanilla Emacs for inclusion.  Everything and
   anything it does could be added to GNU Emacs.  I've even
   offered it as a whole, as a drop-in replacement for
   bookmark.el (after incorporating the bit of code from
   that file that Bookmark+ makes use of).

   Stefan Monnier said at one point that such replacement
   would be good to do.  Other than that comment by Stefan,
   there hasn't been any interest by Emacs Dev in the code
   or features provided by Bookmark+.  So I continue to
   maintain it "outside" of Emacs.  So be it.  (I may have
   forgotten some minor uptake of Bookmark+ features; Karl
   can correct me.)

8. My arguments against basing Emacs bookmark-list display
   on `tabulated-list-mode' go beyond nuisance to Bookmark+.
   If bookmark.el changes to base its bookmark-list display
   on `t-l-mode' then I'll just have to change Bookmark+
   so that it works around that, e.g., by incorporating the
   former bookmark.el code that's relevant.  IOW, I'll need
   to abandon dependence on bookmark.el.  Not the end of
   the world.

   My argument against `t-l-mode' for bookmark.el is that
   almost nothing is gained, and much of what could
   otherwise be possible is lost.  Not that anything in the
   current bookmark.el display-list would be lost, but that
   its improvement potential would be reduced - a sacrifice
   for very little gain (sorting by clicking column heads).

   `t-l-mode' is rudimentary.  It's not built for, or
   adaptable to use with, "tabular" displays that are more
   sophisticated than just what it provides/expects.

   You can't even use `t-l-mode' to control only part of a
   buffer - it has to own the whole buffer.  You can't even
   add a title or other text to a buffer displaying tabular
   info, if you give control to `t-l-mode'.

   I do use `t-l-mode' myself - in my library apu.el, for
   instance.  But even for that simple case I had to jump
   through a few hoops to work around some simplistic
   behavior & limitations.  Nothing dramatic; just sayin'.
   `t-l-mode' is what it is.  It isn't bad; it's limited -
   and limiting.

   Those wanting to convert Emacs buffers that apparently
   use columns to `t-l-mode' are, IMO, too often suffering
   from near-sightedness and have-a-hammer-see-only-nails
   syndrome.  They might do well to focus their attention
   on some of the many other things that need improving
   in Emacs.

   Once you impose `t-l-mode' on a buffer you've limited
   what you can do with it - then and thereafter.  And it
   makes zero sense to impose it on a buffer that already
   offers behavior not supported by `t-l-mode'.  (The
   prime example of this is Dired mode.)  Just because you
   see some columns, it doesn't follow that `t-l-mode' is
   called for, or a wise addition.  You have to consider
   what `t-l-mode' locks you into.

   Could `t-l-mode' be improved, to allow it to play well
   and flexibly with other columnar-list displays?  Maybe.
   But I'm not counting on it.  Too much in its design
   depends on total control, I believe.  Perhaps its
   effect could be limited to a particular buffer zone,
   but even then I think it would be imposing limiting
   behavior on that zone.  Anyway, that's a different
   discussion, and no doubt someone else would need to
   take that up, not I.

9. Much, perhaps most, of the progress in Emacs over the
   decades has been outside the "core".  Yes, there have
   also been great developments within the core, including
   in the last couple of decades.  But the widespread use
   of 3rd-party code speaks to the fact that much that's
   progressive and creative in Emacs development happens in
   the larger community, outside the core - for whatever
   reasons.  IMO that's a fact, for better, worse, or both.

   Rather than lament the fact that a library like Bookmark+
   has introduced new features outside the core, it would be
   better to look at what it has to offer - at least as food
   for thought, and perhaps for simple adoption/integration.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Basil L. Contovounesios]

Drew Adams <drew.adams@oracle.com> writes:

>> unless bookmark.el provides a specific
>> public API (not just functions, but rather any format
>> that's documented and can be relied on externally), then
>> external extensions to it should not rely on its internal
>> implementation.  Packages should not be limited by
>> assumptions made by external extensions.  Besides, why
>> are these extensions external to bookmark.el to begin
>> with?  Surely useful extensions should be included upstream.
>
> 1. I wonder how familiar you are with bookmark.el, its code,
>    and the bookmark formats it defines.

Very little, hence my (attempt at) careful phrasing.

> 2. Basing the bookmark-list display on `tabulated-list-mode'
>    could not, by any stretch of the imagination, be
>    considered "internal implementation".  The immediate
>    behavior change for users would be minor, yes.  But the
>    repercussions of the change would be large for users,
>    in terms of limiting future behavior enhancements. 

In what ways exactly would future enhancements be limited?

> 3. My opinion is anyway that there's nothing "internal"
>    in GNU Emacs, or in free software in general.
>
>    You say, "Packages should not be limited by assumptions
>    made by external extensions."
>
>    You meant that for packages distributed as part of Emacs.

No, I meant that for any package.

>    But the reverse is also true: 3rd-party packages should
>    not be limited by all of the assumptions made by vanilla
>    Emacs code at any given time.
>
>    Sure, no one forces anyone else to abide by any sense of
>    cooperation.  But there has been some mutual cooperation
>    and respect over the years.  3rd-party code depends, at
>    any given time, on what Emacs provides, not vice versa,
>    of course.
>
>    But in the long run what Emacs provides depends in part
>    on what goes on outside its parochial development.  An
>    attitude that "core" Emacs development shouldn't care to
>    look at what's happening in the wider community is
>    self-limiting.
>
>    Telling 3rd-party developers that you don't need to
>    listen to them, don't need to care about what they say
>    or ask for is, yes, within your rights.  Core Emacs need
>    not care, in any sense of legal obligation.  But then
>    don't wonder about separation between the core and the
>    larger community.

I neither said nor suggested anything like this.

> 4. The format of bookmarks _is_ documented, in bookmark.el
>    comments.

There's a difference between comments intended for general readership
that document a stable API, and those that explain what code is doing.
Which kind are you referring to?

>    Bookmark+ respects that format, and extends
>    it.  That format has changed over time, and Bookmark+
>    has adapted, to handle the old formats as well as the
>    new ones.

Then in theory it could also handle future changes, no?

[...]

> 7. Everything in Bookmark+ has, from the beginning, been
>    offered to vanilla Emacs for inclusion.  Everything and
>    anything it does could be added to GNU Emacs.  I've even
>    offered it as a whole, as a drop-in replacement for
>    bookmark.el (after incorporating the bit of code from
>    that file that Bookmark+ makes use of).
>
>    Stefan Monnier said at one point that such replacement
>    would be good to do.  Other than that comment by Stefan,
>    there hasn't been any interest by Emacs Dev in the code
>    or features provided by Bookmark+.  So I continue to
>    maintain it "outside" of Emacs.  So be it.  (I may have
>    forgotten some minor uptake of Bookmark+ features; Karl
>    can correct me.)

Do you have any links to these discussions, or would you be willing to
bring them up again?  I don't see why it should be too late to discuss
these inclusions again, especially if that would mean smoother
integration with whatever ways bookmark.el evolves in the future.

> 8. My arguments against basing Emacs bookmark-list display
>    on `tabulated-list-mode' go beyond nuisance to Bookmark+.
>    If bookmark.el changes to base its bookmark-list display
>    on `t-l-mode' then I'll just have to change Bookmark+
>    so that it works around that, e.g., by incorporating the
>    former bookmark.el code that's relevant.  IOW, I'll need
>    to abandon dependence on bookmark.el.  Not the end of
>    the world.
>
>    My argument against `t-l-mode' for bookmark.el is that
>    almost nothing is gained, and much of what could
>    otherwise be possible is lost.  Not that anything in the
>    current bookmark.el display-list would be lost, but that
>    its improvement potential would be reduced - a sacrifice
>    for very little gain (sorting by clicking column heads).

That's just one superficial gain.  There are other benefits for both
developers and users from reusing core infrastructure, such as better
integration, uniform UIs and customisations, etc.  You could say
"improvement potential would be reduced" any time any decision is made.
Is there a real current use case under threat?

>    `t-l-mode' is rudimentary.  It's not built for, or
>    adaptable to use with, "tabular" displays that are more
>    sophisticated than just what it provides/expects.
>
>    You can't even use `t-l-mode' to control only part of a
>    buffer - it has to own the whole buffer.  You can't even
>    add a title or other text to a buffer displaying tabular
>    info, if you give control to `t-l-mode'.
>
>    I do use `t-l-mode' myself - in my library apu.el, for
>    instance.  But even for that simple case I had to jump
>    through a few hoops to work around some simplistic
>    behavior & limitations.  Nothing dramatic; just sayin'.
>    `t-l-mode' is what it is.  It isn't bad; it's limited -
>    and limiting.
>
>    Those wanting to convert Emacs buffers that apparently
>    use columns to `t-l-mode' are, IMO, too often suffering
>    from near-sightedness and have-a-hammer-see-only-nails
>    syndrome.  They might do well to focus their attention
>    on some of the many other things that need improving
>    in Emacs.

There is no need to discourage such contributions.  Even if the current
proposal is not installed, it's worthwhile to make it.

>    Once you impose `t-l-mode' on a buffer you've limited
>    what you can do with it - then and thereafter.  And it
>    makes zero sense to impose it on a buffer that already
>    offers behavior not supported by `t-l-mode'.  (The
>    prime example of this is Dired mode.)  Just because you
>    see some columns, it doesn't follow that `t-l-mode' is
>    called for, or a wise addition.  You have to consider
>    what `t-l-mode' locks you into.

Of course.

>    Could `t-l-mode' be improved, to allow it to play well
>    and flexibly with other columnar-list displays?  Maybe.
>    But I'm not counting on it.  Too much in its design
>    depends on total control, I believe.  Perhaps its
>    effect could be limited to a particular buffer zone,
>    but even then I think it would be imposing limiting
>    behavior on that zone.  Anyway, that's a different
>    discussion, and no doubt someone else would need to
>    take that up, not I.
>
> 9. Much, perhaps most, of the progress in Emacs over the
>    decades has been outside the "core".  Yes, there have
>    also been great developments within the core, including
>    in the last couple of decades.  But the widespread use
>    of 3rd-party code speaks to the fact that much that's
>    progressive and creative in Emacs development happens in
>    the larger community, outside the core - for whatever
>    reasons.  IMO that's a fact, for better, worse, or both.
>
>    Rather than lament the fact that a library like Bookmark+
>    has introduced new features outside the core, it would be
>    better to look at what it has to offer - at least as food
>    for thought, and perhaps for simple adoption/integration.

Yes, of course, I'm always in favour of importing good features.

-- 
Basil

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

> > 2. Basing the bookmark-list display on `tabulated-list-mode'
> >    could not, by any stretch of the imagination, be
> >    considered "internal implementation".  The immediate
> >    behavior change for users would be minor, yes.  But the
> >    repercussions of the change would be large for users,
> >    in terms of limiting future behavior enhancements.
> 
> In what ways exactly would future enhancements be limited?

Covered in the rest of the msg you quoted.  `t-l-mode'
takes over a buffer.  It sees only dumb columns with,
at most, an associated data type (by which it can sort).

> > 3. My opinion is anyway that there's nothing "internal"
> >    in GNU Emacs, or in free software in general.
> >
> >    You say, "Packages should not be limited by assumptions
> >    made by external extensions."
> >
> >    You meant that for packages distributed as part of Emacs.
> 
> No, I meant that for any package.

But in particular, for `emacs -Q' packages, I think -
you were generalizing the idea that bookmark.el
shouldn't be limited by any assumptions made by
Bookmark+.

And you specifically spoke of "internal implementation".
The meaning I took was that outside code shouldn't
depend on "internal implementation".  Isn't that what
you meant?

> >    But the reverse is also true: 3rd-party packages should
> >    not be limited by all of the assumptions made by vanilla
> >    Emacs code at any given time.
> >
> >    Sure, no one forces anyone else to abide by any sense of
> >    cooperation.  But there has been some mutual cooperation
> >    and respect over the years.  3rd-party code depends, at
> >    any given time, on what Emacs provides, not vice versa,
> >    of course.
> >
> >    But in the long run what Emacs provides depends in part
> >    on what goes on outside its parochial development.  An
> >    attitude that "core" Emacs development shouldn't care to
> >    look at what's happening in the wider community is
> >    self-limiting.
> >
> >    Telling 3rd-party developers that you don't need to
> >    listen to them, don't need to care about what they say
> >    or ask for is, yes, within your rights.  Core Emacs need
> >    not care, in any sense of legal obligation.  But then
> >    don't wonder about separation between the core and the
> >    larger community.
> 
> I neither said nor suggested anything like this.

Again, I interpret your "Packages should not be limited
by assumptions made by external extensions." to be an
argument that statements about impact of changes to
bookmark.el on Bookmark+ shouldn't be taken into account.
And that Bookmark+ shouldn't depend on the implementation
of bookmark.el.

To that, I said fine, if that's the way you want it.
But in that case the reverse is true too.  A spirit
of cooperation matters.  Or it doesn't.

> > 4. The format of bookmarks _is_ documented, in bookmark.el
> >    comments.
> 
> There's a difference between comments intended for general readership
> that document a stable API, and those that explain what code is doing.
> Which kind are you referring to?

Call the comments in bookmark.el what you will.
I didn't write them (though I might have filed a bug
or two to offer some improvement for them).

But I understand them to let users, as well as
developers, of bookmark.el, know what the structure of
a bookmark is, as well as what's important about it and
what's not.

> >    Bookmark+ respects that format, and extends
> >    it.  That format has changed over time, and Bookmark+
> >    has adapted, to handle the old formats as well as the
> >    new ones.
> 
> Then in theory it could also handle future changes, no?

Future changes to the structure of a bookmark?  "In
theory", I'd try to accommodate that, yes.

Why - did you have something in mind?  Are you thinking
about changing the bookmark format?  That's not really
the subject of this enhancement request.

(And bonjour les degats, if you do - you may hear from
some bookmark users and library authors.)

> > 7. Everything in Bookmark+ has, from the beginning, been
> >    offered to vanilla Emacs for inclusion.  Everything and
> >    anything it does could be added to GNU Emacs.  I've even
> >    offered it as a whole, as a drop-in replacement for
> >    bookmark.el (after incorporating the bit of code from
> >    that file that Bookmark+ makes use of).
> >
> >    Stefan Monnier said at one point that such replacement
> >    would be good to do.  Other than that comment by Stefan,
> >    there hasn't been any interest by Emacs Dev in the code
> >    or features provided by Bookmark+.  So I continue to
> >    maintain it "outside" of Emacs.  So be it.  (I may have
> >    forgotten some minor uptake of Bookmark+ features; Karl
> >    can correct me.)
> 
> Do you have any links to these discussions, or would you be willing to
> bring them up again?

No.  The code is there.  It's well documented.  And if
there's interest and there are specific questions then
I'll try to find the time to answer them.

I'm OK with vanilla Emacs including Bookmark+.  And I'd
remove, e.g., code that provides for compatibility with
older releases.

And I'm OK with instead continuing as is, regardless of
whether bookmark.el switches to `t-l-mode'.  (In the
latter case, Bookmark+, or at least its display list,
will need to be separated from bookmark.el.)

But I won't spend a lot of time helping integrate this
or that piece of Bookmark+.  I'll help someone understand
something that Bookmark+ does, of course.

> I don't see why it should be too late to discuss
> these inclusions again, especially if that would mean smoother
> integration with whatever ways bookmark.el evolves in the future.

See above.  You, or others, are welcome to start.

> > 8. My arguments against basing Emacs bookmark-list display
> >    on `tabulated-list-mode' go beyond nuisance to Bookmark+.
> >    If bookmark.el changes to base its bookmark-list display
> >    on `t-l-mode' then I'll just have to change Bookmark+
> >    so that it works around that, e.g., by incorporating the
> >    former bookmark.el code that's relevant.  IOW, I'll need
> >    to abandon dependence on bookmark.el.  Not the end of
> >    the world.
> >
> >    My argument against `t-l-mode' for bookmark.el is that
> >    almost nothing is gained, and much of what could
> >    otherwise be possible is lost.  Not that anything in the
> >    current bookmark.el display-list would be lost, but that
> >    its improvement potential would be reduced - a sacrifice
> >    for very little gain (sorting by clicking column heads).
> 
> That's just one superficial gain.

That's the only gain for _users_.  And sure, that includes
the fact that if they know about clicking column headers
to sort then they'll know how to use that (sole) feature
`t-l-mode' provides.

> There are other benefits for both
> developers and users from reusing core infrastructure, such as better
> integration, uniform UIs and customisations, etc.

There are no deffaces or defcustoms.  OK, there's one hook.
No customization, and nothing special for users to gain,
other than click-to-sort column headers.

See what I said in my first post of this thread, starting
with "This kind of proposal is, IMO, a consequence of one
or both of the following:"

> You could say "improvement potential would be reduced"
> any time any decision is made.

You can say that.  I didn't say that.  You seem to want
to argue by making generalizations.

> Is there a real current use case under threat?

I mentioned titles.  There are many other display-list
features whose implementation would need to be totally
revamped (reimplemented using `t-l-mode' "features").

I mentioned sorting in my first message in this thread.
`t-l-mode' lets you sort in only one way, by column
type.  What's the column type for a bookmark name or
a target location?  Click the bookmark-name column
header - what does it sort by?  Not much that's useful.

Many of the features Dired provides exist in Bookmark+,
from omitting (with show/hide) to specialized markings.
And there are other display-list features that Dired
doesn't have: interactive filtering, help, editing, and
highlighting of entries, etc.  Can some of those
accommodate `t-l-mode' or be reimplemented to do so?
Maybe.  I probably won't try/bother, sorry.

> >    `t-l-mode' is rudimentary.  It's not built for, or
> >    adaptable to use with, "tabular" displays that are more
> >    sophisticated than just what it provides/expects.
> >
> >    You can't even use `t-l-mode' to control only part of a
> >    buffer - it has to own the whole buffer.  You can't even
> >    add a title or other text to a buffer displaying tabular
> >    info, if you give control to `t-l-mode'.
> >
> >    I do use `t-l-mode' myself - in my library apu.el, for
> >    instance.  But even for that simple case I had to jump
> >    through a few hoops to work around some simplistic
> >    behavior & limitations.  Nothing dramatic; just sayin'.
> >    `t-l-mode' is what it is.  It isn't bad; it's limited -
> >    and limiting.
> >
> >    Those wanting to convert Emacs buffers that apparently
> >    use columns to `t-l-mode' are, IMO, too often suffering
> >    from near-sightedness and have-a-hammer-see-only-nails
> >    syndrome.  They might do well to focus their attention
> >    on some of the many other things that need improving
> >    in Emacs.
> 
> There is no need to discourage such contributions.  Even if
> the current proposal is not installed, it's worthwhile to make it.

Sure, it was worthwhile to make it.  It was worthwhile
to disagree with it.  And it's worthwhile not to do it.
There's nothing wrong with proposing _any_ particular
change to Emacs.

> >    Once you impose `t-l-mode' on a buffer you've limited
> >    what you can do with it - then and thereafter.  And it
> >    makes zero sense to impose it on a buffer that already
> >    offers behavior not supported by `t-l-mode'.  (The
> >    prime example of this is Dired mode.)  Just because you
> >    see some columns, it doesn't follow that `t-l-mode' is
> >    called for, or a wise addition.  You have to consider
> >    what `t-l-mode' locks you into.
> 
> Of course.

Glad you agree.

> >    Could `t-l-mode' be improved, to allow it to play well
> >    and flexibly with other columnar-list displays?  Maybe.
> >    But I'm not counting on it.  Too much in its design
> >    depends on total control, I believe.  Perhaps its
> >    effect could be limited to a particular buffer zone,
> >    but even then I think it would be imposing limiting
> >    behavior on that zone.  Anyway, that's a different
> >    discussion, and no doubt someone else would need to
> >    take that up, not I.
> >
> > 9. Much, perhaps most, of the progress in Emacs over the
> >    decades has been outside the "core".  Yes, there have
> >    also been great developments within the core, including
> >    in the last couple of decades.  But the widespread use
> >    of 3rd-party code speaks to the fact that much that's
> >    progressive and creative in Emacs development happens in
> >    the larger community, outside the core - for whatever
> >    reasons.  IMO that's a fact, for better, worse, or both.
> >
> >    Rather than lament the fact that a library like Bookmark+
> >    has introduced new features outside the core, it would be
> >    better to look at what it has to offer - at least as food
> >    for thought, and perhaps for simple adoption/integration.
> 
> Yes, of course, I'm always in favour of importing good features.

There are tons of good features out there, in libraries
by lots of people, and with GPL.  A motherlode to mine.

But there's little such mining that goes on by Emacs
"core" developers.  What there is, is some contribution
of 3rd-party libraries to GNU ELPA.  But "core" pulling
in features from 3rd-party code - "importing"?  Not so
much. When was the last time you saw a "core" developer
import some feature from a 3rd-party library?

Library authors and maintainers often find it more
worthwhile to just keep working on their stuff outside
the "core".  And "core" Emacs developers often expect
3rd-party authors to do the work of integrating their
features into Emacs.  The motherlode's still out there,
and growing, for those who are "always in favour of
importing good features."

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Basil L. Contovounesios]

Drew Adams <drew.adams@oracle.com> writes:

>> > 2. Basing the bookmark-list display on `tabulated-list-mode'
>> >    could not, by any stretch of the imagination, be
>> >    considered "internal implementation".  The immediate
>> >    behavior change for users would be minor, yes.  But the
>> >    repercussions of the change would be large for users,
>> >    in terms of limiting future behavior enhancements.
>> 
>> In what ways exactly would future enhancements be limited?
>
> Covered in the rest of the msg you quoted.  `t-l-mode'
> takes over a buffer.  It sees only dumb columns with,
> at most, an associated data type (by which it can sort).

Does bookmark-bmenu-mode provide any other features over
tabulated-list-mode?

>> > 3. My opinion is anyway that there's nothing "internal"
>> >    in GNU Emacs, or in free software in general.
>> >
>> >    You say, "Packages should not be limited by assumptions
>> >    made by external extensions."
>> >
>> >    You meant that for packages distributed as part of Emacs.
>> 
>> No, I meant that for any package.
>
> But in particular, for `emacs -Q' packages, I think -
> you were generalizing the idea that bookmark.el
> shouldn't be limited by any assumptions made by
> Bookmark+.

Yes, but that generalises to any package, not just core ones.

> And you specifically spoke of "internal implementation".
> The meaning I took was that outside code shouldn't
> depend on "internal implementation".  Isn't that what
> you meant?

Yes, which also applies to any package.

>> >    But the reverse is also true: 3rd-party packages should
>> >    not be limited by all of the assumptions made by vanilla
>> >    Emacs code at any given time.
>> >
>> >    Sure, no one forces anyone else to abide by any sense of
>> >    cooperation.  But there has been some mutual cooperation
>> >    and respect over the years.  3rd-party code depends, at
>> >    any given time, on what Emacs provides, not vice versa,
>> >    of course.
>> >
>> >    But in the long run what Emacs provides depends in part
>> >    on what goes on outside its parochial development.  An
>> >    attitude that "core" Emacs development shouldn't care to
>> >    look at what's happening in the wider community is
>> >    self-limiting.
>> >
>> >    Telling 3rd-party developers that you don't need to
>> >    listen to them, don't need to care about what they say
>> >    or ask for is, yes, within your rights.  Core Emacs need
>> >    not care, in any sense of legal obligation.  But then
>> >    don't wonder about separation between the core and the
>> >    larger community.
>> 
>> I neither said nor suggested anything like this.
>
> Again, I interpret your "Packages should not be limited
> by assumptions made by external extensions." to be an
> argument that statements about impact of changes to
> bookmark.el on Bookmark+ shouldn't be taken into account.

I didn't say they shouldn't be taken into account.  But if Bookmark+
relies on internal features then it is unfair to let that limit
bookmark.el development.

> And that Bookmark+ shouldn't depend on the implementation
> of bookmark.el.

Yes, just like any package shouldn't rely on internal details of another
package if it cares about longterm compatibility.

> To that, I said fine, if that's the way you want it.
> But in that case the reverse is true too.  A spirit
> of cooperation matters.  Or it doesn't.

It does.

>> > 4. The format of bookmarks _is_ documented, in bookmark.el
>> >    comments.
>> 
>> There's a difference between comments intended for general readership
>> that document a stable API, and those that explain what code is doing.
>> Which kind are you referring to?
>
> Call the comments in bookmark.el what you will.
> I didn't write them (though I might have filed a bug
> or two to offer some improvement for them).
>
> But I understand them to let users, as well as
> developers, of bookmark.el, know what the structure of
> a bookmark is, as well as what's important about it and
> what's not.

Sure, but not all comments should be taken as gospel.

>> >    Bookmark+ respects that format, and extends
>> >    it.  That format has changed over time, and Bookmark+
>> >    has adapted, to handle the old formats as well as the
>> >    new ones.
>> 
>> Then in theory it could also handle future changes, no?
>
> Future changes to the structure of a bookmark?  "In
> theory", I'd try to accommodate that, yes.
>
> Why - did you have something in mind?  Are you thinking
> about changing the bookmark format?  That's not really
> the subject of this enhancement request.

I was referring to the currently proposed patch in particular and future
changes in general.  I realise now you were referring specifically to
the sexp data that bookmark.el uses, which was not under discussion.

> (And bonjour les degats, if you do - you may hear from
> some bookmark users and library authors.)
>
>> > 7. Everything in Bookmark+ has, from the beginning, been
>> >    offered to vanilla Emacs for inclusion.  Everything and
>> >    anything it does could be added to GNU Emacs.  I've even
>> >    offered it as a whole, as a drop-in replacement for
>> >    bookmark.el (after incorporating the bit of code from
>> >    that file that Bookmark+ makes use of).
>> >
>> >    Stefan Monnier said at one point that such replacement
>> >    would be good to do.  Other than that comment by Stefan,
>> >    there hasn't been any interest by Emacs Dev in the code
>> >    or features provided by Bookmark+.  So I continue to
>> >    maintain it "outside" of Emacs.  So be it.  (I may have
>> >    forgotten some minor uptake of Bookmark+ features; Karl
>> >    can correct me.)
>> 
>> Do you have any links to these discussions, or would you be willing to
>> bring them up again?
>
> No.  The code is there.  It's well documented.  And if
> there's interest and there are specific questions then
> I'll try to find the time to answer them.
>
> I'm OK with vanilla Emacs including Bookmark+.  And I'd
> remove, e.g., code that provides for compatibility with
> older releases.
>
> And I'm OK with instead continuing as is, regardless of
> whether bookmark.el switches to `t-l-mode'.  (In the
> latter case, Bookmark+, or at least its display list,
> will need to be separated from bookmark.el.)
>
> But I won't spend a lot of time helping integrate this
> or that piece of Bookmark+.  I'll help someone understand
> something that Bookmark+ does, of course.

Thanks, that could make an interesting project for anyone interested.

>> I don't see why it should be too late to discuss
>> these inclusions again, especially if that would mean smoother
>> integration with whatever ways bookmark.el evolves in the future.
>
> See above.  You, or others, are welcome to start.
>
>> > 8. My arguments against basing Emacs bookmark-list display
>> >    on `tabulated-list-mode' go beyond nuisance to Bookmark+.
>> >    If bookmark.el changes to base its bookmark-list display
>> >    on `t-l-mode' then I'll just have to change Bookmark+
>> >    so that it works around that, e.g., by incorporating the
>> >    former bookmark.el code that's relevant.  IOW, I'll need
>> >    to abandon dependence on bookmark.el.  Not the end of
>> >    the world.
>> >
>> >    My argument against `t-l-mode' for bookmark.el is that
>> >    almost nothing is gained, and much of what could
>> >    otherwise be possible is lost.  Not that anything in the
>> >    current bookmark.el display-list would be lost, but that
>> >    its improvement potential would be reduced - a sacrifice
>> >    for very little gain (sorting by clicking column heads).
>> 
>> That's just one superficial gain.
>
> That's the only gain for _users_.  And sure, that includes
> the fact that if they know about clicking column headers
> to sort then they'll know how to use that (sole) feature
> `t-l-mode' provides.

That's not the only feature tabulated-list-mode provides.

>> There are other benefits for both
>> developers and users from reusing core infrastructure, such as better
>> integration, uniform UIs and customisations, etc.
>
> There are no deffaces or defcustoms.  OK, there's one hook.
> No customization, and nothing special for users to gain,
> other than click-to-sort column headers.

I count 4 defcustoms, 1 defface, 2 key maps, 1 hook, several functions
that can be advised, and a common UI in Emacs 27/28.  Furthermore, any
future developments to tabulated-list-mode will benefit all modes based
on it.  All of these are big gains in my book, for both users and
developers.

> See what I said in my first post of this thread, starting
> with "This kind of proposal is, IMO, a consequence of one
> or both of the following:"
>
>> You could say "improvement potential would be reduced"
>> any time any decision is made.
>
> You can say that.  I didn't say that.  You seem to want
> to argue by making generalizations.

I made general comments because I'm not familiar enough with the
relevant code to make more specific ones, and for whatever they're
worth.

>> Is there a real current use case under threat?
>
> I mentioned titles.  There are many other display-list
> features whose implementation would need to be totally
> revamped (reimplemented using `t-l-mode' "features").

I'm asking specifically about bookmark-bmenu-mode, which is what is
being discussed.

> I mentioned sorting in my first message in this thread.
> `t-l-mode' lets you sort in only one way, by column
> type.  What's the column type for a bookmark name or
> a target location?  Click the bookmark-name column
> header - what does it sort by?  Not much that's useful.

Every tabulated-list-mode column can be defined with its own custom
sorting predicate.

> Many of the features Dired provides exist in Bookmark+,
> from omitting (with show/hide) to specialized markings.
> And there are other display-list features that Dired
> doesn't have: interactive filtering, help, editing, and
> highlighting of entries, etc.  Can some of those
> accommodate `t-l-mode' or be reimplemented to do so?
> Maybe.  I probably won't try/bother, sorry.

[...]

>> > 9. Much, perhaps most, of the progress in Emacs over the
>> >    decades has been outside the "core".  Yes, there have
>> >    also been great developments within the core, including
>> >    in the last couple of decades.  But the widespread use
>> >    of 3rd-party code speaks to the fact that much that's
>> >    progressive and creative in Emacs development happens in
>> >    the larger community, outside the core - for whatever
>> >    reasons.  IMO that's a fact, for better, worse, or both.
>> >
>> >    Rather than lament the fact that a library like Bookmark+
>> >    has introduced new features outside the core, it would be
>> >    better to look at what it has to offer - at least as food
>> >    for thought, and perhaps for simple adoption/integration.
>> 
>> Yes, of course, I'm always in favour of importing good features.
>
> There are tons of good features out there, in libraries
> by lots of people, and with GPL.  A motherlode to mine.
>
> But there's little such mining that goes on by Emacs
> "core" developers.  What there is, is some contribution
> of 3rd-party libraries to GNU ELPA.  But "core" pulling
> in features from 3rd-party code - "importing"?  Not so
> much. When was the last time you saw a "core" developer
> import some feature from a 3rd-party library?
>
> Library authors and maintainers often find it more
> worthwhile to just keep working on their stuff outside
> the "core".  And "core" Emacs developers often expect
> 3rd-party authors to do the work of integrating their
> features into Emacs.  The motherlode's still out there,
> and growing, for those who are "always in favour of
> importing good features."

I don't see how this relates to the current discussion.

-- 
Basil

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> The attached patch changes bookmark-bmenu-mode to be based on
> tabulated-list-mode instead of special-mode.
>
> This allows us to simplify the code in several cases.  In addition, we
> get many features for free, such as sorting columns by clicking on the
> column headers, changing size of columns.  In the future, this will
> obviously include any new feature added to 'tabulated-list-mode'.

Makes sense to me.  There was an objection about packages that
extend bookmark.el would no longer work...  but I don't think that's an
objection we have to heed.  As far as I can see, the public interface
isn't changed (i.e., the commands are still the same), which is the only
this we try to keep compatible.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Makes sense to me.  There was an objection about packages that
> extend bookmark.el would no longer work...  but I don't think that's an
> objection we have to heed.  As far as I can see, the public interface
> isn't changed (i.e., the commands are still the same), which is the only
> this we try to keep compatible.

The patch no longer applied, so I tried to respin it for the current
emacs, but this leads to three test failures:

3 unexpected results:
   FAILED  bookmark-test-bmenu-delete-all
   FAILED  bookmark-test-bmenu-mark-all
   FAILED  bookmark-test-bmenu-unmark-all

Stefan, could you take a look at this, and then we can get it onto the
trunk?

diff --git a/etc/NEWS b/etc/NEWS
index 79a8d119f3..5bdf18cf23 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -96,6 +96,16 @@ groups.
 Setting it to nil forces the redisplay to do its job even in the
 initial frame used in batch mode.
 
+---
+** The 'list-bookmark' menu is now based on 'tabulated-list-mode'.
+The interactive bookmark list will now benefit from features in
+'tabulated-list-mode' like sorting columns or changing column width.
+
+Support for the optional "inline" header line, allowing for a header
+without using 'header-line-format', has been dropped.  Consequently,
+the variables 'bookmark-bmenu-use-header-line' and
+'bookmark-bmenu-inline-header-height' are now declared obsolete.
+
 ---
 ** Support for the 'strike-through' face attribute on TTY frames.
 If your terminal's termcap or terminfo database entry has the 'smxx'
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index dcb03adadd..7d1cfa0e53 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -32,6 +32,7 @@
 ;;; Code:
 
 (require 'pp)
+(require 'tabulated-list)
 (require 'text-property-search)
 (eval-when-compile (require 'cl-lib))
 
@@ -126,16 +127,16 @@ bookmark-automatically-show-annotations
 (defconst bookmark-bmenu-buffer "*Bookmark List*"
   "Name of buffer used for Bookmark List.")
 
-(defcustom bookmark-bmenu-use-header-line t
+(defvar 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."
-  :version "24.4"
-  :type 'boolean)
+This is as opposed to inline text at the top of the buffer.")
+(make-obsolete-variable 'bookmark-bmenu-use-header-line "no longer used." "28.1")
 
 (defconst bookmark-bmenu-inline-header-height 2
   "Number of lines used for the *Bookmark List* header.
 \(This is only significant when `bookmark-bmenu-use-header-line'
 is nil.)")
+(make-obsolete-variable 'bookmark-bmenu-inline-header-height "no longer used." "28.1")
 
 (defconst bookmark-bmenu-marks-width 2
   "Number of columns (chars) used for the *Bookmark List* marks column.
@@ -165,6 +166,7 @@ bookmark-search-delay
   "Time before `bookmark-bmenu-search' updates the display."
   :type  'number)
 
+;; FIXME: Should be declared obsolete.
 (defface bookmark-menu-heading
   '((t (:inherit font-lock-type-face)))
   "Face used to highlight the heading in bookmark menu buffers."
@@ -976,7 +978,7 @@ bookmark-send-edited-annotation
     (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))
+      (bookmark-bmenu-bookmark))
     (kill-buffer old-buffer)))
 
 
@@ -1587,7 +1589,7 @@ bookmark-bmenu-hidden-bookmarks
 
 (defvar bookmark-bmenu-mode-map
   (let ((map (make-keymap)))
-    (set-keymap-parent map special-mode-map)
+    (set-keymap-parent map tabulated-list-mode-map)
     (define-key map "v" 'bookmark-bmenu-select)
     (define-key map "w" 'bookmark-bmenu-locate)
     (define-key map "5" 'bookmark-bmenu-other-frame)
@@ -1607,8 +1609,6 @@ bookmark-bmenu-mode-map
     (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)
@@ -1676,6 +1676,30 @@ bookmark-bmenu-surreptitiously-rebuild-list
         (save-window-excursion
           (bookmark-bmenu-list)))))
 
+(defun bookmark-bmenu--revert ()
+  "Re-populate `tabulated-list-entries'."
+  (let (entries)
+    (dolist (full-record (bookmark-maybe-sort-alist))
+      (let* ((name       (bookmark-name-from-full-record full-record))
+             (annotation (bookmark-get-annotation full-record))
+             (location   (bookmark-location full-record)))
+        (push (list
+               full-record
+               `[,(if (and annotation (not (string-equal annotation "")))
+                      "*" "")
+                 ,(if (display-mouse-p)
+                      (propertize name
+                                  'font-lock-face 'bookmark-menu-bookmark
+                                  'mouse-face 'highlight
+                                  'follow-link t
+                                  'help-echo "mouse-2: go to this bookmark in other window")
+                    name)
+                 ,@(if bookmark-bmenu-toggle-filenames
+                       (list location))])
+              entries)))
+    (tabulated-list-init-header)
+    (setq tabulated-list-entries entries))
+  (tabulated-list-print t))
 
 ;;;###autoload
 (defun bookmark-bmenu-get-buffer ()
@@ -1702,70 +1726,15 @@ bookmark-bmenu-list
     (if (called-interactively-p 'interactive)
         (switch-to-buffer buf)
       (set-buffer buf)))
-  (let ((inhibit-read-only t))
-    (erase-buffer)
-    (if (not bookmark-bmenu-use-header-line)
-      (insert "% Bookmark\n- --------\n"))
-    (add-text-properties (point-min) (point)
-			 '(font-lock-face bookmark-menu-heading))
-    (dolist (full-record (bookmark-maybe-sort-alist))
-      (let ((name        (bookmark-name-from-full-record full-record))
-            (annotation  (bookmark-get-annotation full-record))
-            (start       (point))
-            end)
-        ;; if a bookmark has an annotation, prepend a "*"
-        ;; in the list of bookmarks.
-        (insert (if (and annotation (not (string-equal annotation "")))
-                    " *" "  ")
-                name)
-        (setq end (point))
-        (put-text-property
-         (+ bookmark-bmenu-marks-width start) end 'bookmark-name-prop name)
-        (when (display-mouse-p)
-          (add-text-properties
-           (+ bookmark-bmenu-marks-width start) end
-           '(font-lock-face bookmark-menu-bookmark
-	     mouse-face highlight
-             follow-link t
-             help-echo "mouse-2: go to this bookmark in other window")))
-        (insert "\n")))
-    (set-buffer-modified-p (not (= bookmark-alist-modification-count 0)))
-    (goto-char (point-min))
-    (bookmark-bmenu-mode)
-    (if bookmark-bmenu-use-header-line
-	(bookmark-bmenu-set-header)
-      (forward-line bookmark-bmenu-inline-header-height))
-    (when (and bookmark-alist bookmark-bmenu-toggle-filenames)
-      (bookmark-bmenu-toggle-filenames t))))
+  (bookmark-bmenu-mode)
+  (bookmark-bmenu--revert))
 
 ;;;###autoload
 (defalias 'list-bookmarks 'bookmark-bmenu-list)
 ;;;###autoload
 (defalias 'edit-bookmarks 'bookmark-bmenu-list)
 
-;; FIXME: This could also display the current default bookmark file
-;; according to `bookmark-bookmarks-timestamp'.
-(defun bookmark-bmenu-set-header ()
-  "Set the immutable header line."
-  (let ((header (copy-sequence "%% Bookmark")))
-    (when bookmark-bmenu-toggle-filenames
-      (setq header (concat header
-			   (make-string (- bookmark-bmenu-file-column
-					   (- (length header) 3))  ?\s)
-			   "File")))
-    (let ((pos 0))
-      (while (string-match "[ \t\n]+" header pos)
-	(setq pos (match-end 0))
-	(put-text-property (match-beginning 0) pos 'display
-			   (list 'space :align-to (- pos 1))
-			   header)))
-    (put-text-property 0 2 'face 'fixed-pitch header)
-    (setq header (concat (propertize " " 'display '(space :align-to 0))
-			 header))
-    ;; Code derived from `buff-menu.el'.
-    (setq header-line-format header)))
-
-(define-derived-mode bookmark-bmenu-mode special-mode "Bookmark Menu"
+(define-derived-mode bookmark-bmenu-mode tabulated-list-mode "Bookmark Menu"
   "Major mode for editing a list of bookmarks.
 Each line describes one of the bookmarks in Emacs.
 Letters do not insert themselves; instead, they are commands.
@@ -1803,8 +1772,30 @@ bookmark-bmenu-mode
 \\[bookmark-bmenu-show-all-annotations] -- show the annotations of all bookmarks in another buffer.
 \\[bookmark-bmenu-edit-annotation] -- edit the annotation for the current bookmark.
 \\[bookmark-bmenu-search] -- incrementally search for bookmarks."
-  (setq truncate-lines t)
-  (setq buffer-read-only t))
+  ;; FIXME: The header could also display the current default bookmark file
+  ;; according to `bookmark-bookmarks-timestamp'.
+  (setq tabulated-list-format
+        `[("" 1) ;; Space to add "*" for bookmark with annotation
+          ("Bookmark" ,bookmark-bmenu-file-column bookmark-bmenu--name-predicate)
+          ,@(if bookmark-bmenu-toggle-filenames
+                '(("File" 0 bookmark-bmenu--file-predicate)))])
+  (setq tabulated-list-padding bookmark-bmenu-marks-width)
+  (setq tabulated-list-sort-key '("Bookmark" . nil))
+  (add-hook 'tabulated-list-revert-hook #'bookmark-bmenu--revert nil t)'
+  (setq revert-buffer-function 'bookmark-bmenu--revert)
+  (tabulated-list-init-header))
+
+
+(defun bookmark-bmenu--name-predicate (a b)
+  "Predicate to sort \"*Bookmark List*\" buffer by the name column.
+This is used for `tabulated-list-format' in `bookmark-bmenu-mode'."
+  (string< (caar a) (caar b)))
+
+
+(defun bookmark-bmenu--file-predicate (a b)
+  "Predicate to sort \"*Bookmark List*\" buffer by the file column.
+This is used for `tabulated-list-format' in `bookmark-bmenu-mode'."
+  (string< (bookmark-location (car a)) (bookmark-location (car b))))
 
 
 (defun bookmark-bmenu-toggle-filenames (&optional show)
@@ -1813,100 +1804,42 @@ bookmark-bmenu-toggle-filenames
   (interactive)
   (cond
    (show
-    (setq bookmark-bmenu-toggle-filenames nil)
-    (bookmark-bmenu-show-filenames)
     (setq bookmark-bmenu-toggle-filenames t))
    (bookmark-bmenu-toggle-filenames
-    (bookmark-bmenu-hide-filenames)
     (setq bookmark-bmenu-toggle-filenames nil))
    (t
-    (bookmark-bmenu-show-filenames)
     (setq bookmark-bmenu-toggle-filenames t)))
-  (when bookmark-bmenu-use-header-line
-    (bookmark-bmenu-set-header)))
+  (bookmark-bmenu-surreptitiously-rebuild-list))
 
 
-(defun bookmark-bmenu-show-filenames (&optional force)
-  "In an interactive bookmark list, show filenames along with bookmarks.
-Non-nil FORCE forces a redisplay showing the filenames.  FORCE is used
-mainly for debugging, and should not be necessary in normal use."
-  (if (and (not force) bookmark-bmenu-toggle-filenames)
-      nil ;already shown, so do nothing
-    (with-buffer-modified-unmodified
-     (save-excursion
-       (save-window-excursion
-         (goto-char (point-min))
-	 (if (not bookmark-bmenu-use-header-line)
-	     (forward-line bookmark-bmenu-inline-header-height))
-         (setq bookmark-bmenu-hidden-bookmarks ())
-         (let ((inhibit-read-only t))
-           (while (< (point) (point-max))
-             (let ((bmrk (bookmark-bmenu-bookmark)))
-               (push bmrk bookmark-bmenu-hidden-bookmarks)
-               (let ((start (line-end-position)))
-                 (move-to-column bookmark-bmenu-file-column t)
-                 ;; Strip off `mouse-face' from the white spaces region.
-                 (if (display-mouse-p)
-                     (remove-text-properties start (point)
-                                             '(mouse-face nil help-echo nil))))
-               (delete-region (point) (progn (end-of-line) (point)))
-               (insert "  ")
-               ;; Pass the NO-HISTORY arg:
-               (bookmark-insert-location bmrk t)
-               (forward-line 1)))))))))
-
-
-(defun bookmark-bmenu-hide-filenames (&optional force)
-  "In an interactive bookmark list, hide the filenames of the bookmarks.
-Non-nil FORCE forces a redisplay showing the filenames.  FORCE is used
-mainly for debugging, and should not be necessary in normal use."
-  (when (and (not force) bookmark-bmenu-toggle-filenames)
-    ;; nothing to hide if above is nil
-    (with-buffer-modified-unmodified
-     (save-excursion
-       (goto-char (point-min))
-       (if (not bookmark-bmenu-use-header-line)
-	   (forward-line bookmark-bmenu-inline-header-height))
-       (setq bookmark-bmenu-hidden-bookmarks
-             (nreverse bookmark-bmenu-hidden-bookmarks))
-       (let ((inhibit-read-only t))
-         (while bookmark-bmenu-hidden-bookmarks
-           (move-to-column bookmark-bmenu-marks-width t)
-           (bookmark-kill-line)
-           (let ((name  (pop bookmark-bmenu-hidden-bookmarks))
-                 (start (point)))
-             (insert name)
-             (put-text-property start (point) 'bookmark-name-prop name)
-             (if (display-mouse-p)
-                 (add-text-properties
-                  start (point)
-                  '(font-lock-face bookmark-menu-bookmark
-		    mouse-face highlight
-		    follow-link t help-echo
-                    "mouse-2: go to this bookmark in other window"))))
-           (forward-line 1)))))))
+(defun bookmark-bmenu-show-filenames (&optional _)
+  "In an interactive bookmark list, show filenames along with bookmarks."
+  (setq bookmark-bmenu-toggle-filenames t)
+  (bookmark-bmenu-surreptitiously-rebuild-list))
+
+
+(defun bookmark-bmenu-hide-filenames (&optional _)
+  "In an interactive bookmark list, hide the filenames of the bookmarks."
+  (setq bookmark-bmenu-toggle-filenames nil)
+  (bookmark-bmenu-surreptitiously-rebuild-list))
 
 
 (defun bookmark-bmenu-ensure-position ()
   "If point is not on a bookmark line, move it to one.
-If before the first bookmark line, move to the first; if after the
-last full line, move to the last full line.  The return value is undefined."
-  (cond ((and (not bookmark-bmenu-use-header-line)
-	      (< (count-lines (point-min) (point))
-		 bookmark-bmenu-inline-header-height))
-         (goto-char (point-min))
-         (forward-line bookmark-bmenu-inline-header-height))
-        ((and (bolp) (eobp))
+If after the last full line, move to the last full line.  The
+return value is undefined."
+  (cond ((and (bolp) (eobp))
          (beginning-of-line 0))))
 
 
 (defun bookmark-bmenu-bookmark ()
   "Return the bookmark for this line in an interactive bookmark list buffer."
   (bookmark-bmenu-ensure-position)
-  (save-excursion
-    (beginning-of-line)
-    (forward-char bookmark-bmenu-marks-width)
-    (get-text-property (point) 'bookmark-name-prop)))
+  (let* ((id (tabulated-list-get-id))
+         (entry (and id (assoc id tabulated-list-entries))))
+    (if entry
+        (caar entry)
+      "")))
 
 
 (defun bookmark-show-annotation (bookmark-name-or-record)
@@ -1954,14 +1887,8 @@ bookmark-show-all-annotations
 (defun bookmark-bmenu-mark ()
   "Mark bookmark on this line to be displayed by \\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-select]."
   (interactive)
-  (beginning-of-line)
   (bookmark-bmenu-ensure-position)
-  (with-buffer-modified-unmodified
-   (let ((inhibit-read-only t))
-     (delete-char 1)
-     (insert ?>)
-     (forward-line 1)
-     (bookmark-bmenu-ensure-position))))
+  (tabulated-list-put-tag ">" t))
 
 
 (defun bookmark-bmenu-mark-all ()
@@ -2126,17 +2053,12 @@ bookmark-bmenu-unmark
   "Cancel all requested operations on bookmark on this line and move down.
 Optional BACKUP means move up."
   (interactive "P")
-  (beginning-of-line)
+  ;; any flags to reset according to circumstances?  How about a
+  ;; flag indicating whether this bookmark is being visited?
+  ;; well, we don't have this now, so maybe later.
   (bookmark-bmenu-ensure-position)
-  (with-buffer-modified-unmodified
-   (let ((inhibit-read-only t))
-     (delete-char 1)
-     ;; any flags to reset according to circumstances?  How about a
-     ;; flag indicating whether this bookmark is being visited?
-     ;; well, we don't have this now, so maybe later.
-     (insert " "))
-   (forward-line (if backup -1 1))
-   (bookmark-bmenu-ensure-position)))
+  (tabulated-list-put-tag " ")
+  (forward-line (if backup -1 1)))
 
 
 (defun bookmark-bmenu-backup-unmark ()
@@ -2167,14 +2089,8 @@ 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]."
   (interactive)
-  (beginning-of-line)
   (bookmark-bmenu-ensure-position)
-  (with-buffer-modified-unmodified
-   (let ((inhibit-read-only t))
-     (delete-char 1)
-     (insert ?D)
-     (forward-line 1)
-     (bookmark-bmenu-ensure-position))))
+  (tabulated-list-put-tag "D" t))
 
 
 (defun bookmark-bmenu-delete-backwards ()
@@ -2182,10 +2098,7 @@ bookmark-bmenu-delete-backwards
 To carry out the deletions that you've marked, use \\<bookmark-bmenu-mode-map>\\[bookmark-bmenu-execute-deletions]."
   (interactive)
   (bookmark-bmenu-delete)
-  (forward-line -2)
-  (bookmark-bmenu-ensure-position)
-  (forward-line 1)
-  (bookmark-bmenu-ensure-position))
+  (forward-line -2))
 
 
 (defun bookmark-bmenu-delete-all ()
@@ -2217,8 +2130,6 @@ bookmark-bmenu-execute-deletions
                        (progn (end-of-line) (point))))))
         (o-col     (current-column)))
     (goto-char (point-min))
-    (unless bookmark-bmenu-use-header-line
-      (forward-line 1))
     (while (re-search-forward "^D" (point-max) t)
       (bookmark-delete (bookmark-bmenu-bookmark) t)) ; pass BATCH arg
     (bookmark-bmenu-list)
@@ -2343,8 +2254,6 @@ bookmark-menu-popup-paned-menu
 ;; We MUST autoload EACH form used to set up this variable's value, so
 ;; that the whole job is done in loaddefs.el.
 
-;; Emacs menubar stuff.
-
 ;;;###autoload
 (defvar menu-bar-bookmark-map
   (let ((map (make-sparse-keymap "Bookmark functions")))
diff --git a/test/lisp/bookmark-tests.el b/test/lisp/bookmark-tests.el
index c5959e46d8..1d24a9012b 100644
--- a/test/lisp/bookmark-tests.el
+++ b/test/lisp/bookmark-tests.el
@@ -479,6 +479,8 @@ bookmark-test-bmenu-send-edited-annotation/restore-focus
    (insert "foo")
    (bookmark-send-edited-annotation)
    (should (equal (buffer-name (current-buffer)) bookmark-bmenu-buffer))
+   (beginning-of-line)
+   (forward-char 4)
    (should (looking-at "name"))))
 
 (ert-deftest bookmark-test-bmenu-toggle-filenames ()
@@ -511,6 +513,7 @@ bookmark-test-bmenu-bookmark
 (ert-deftest bookmark-test-bmenu-mark ()
   (with-bookmark-bmenu-test
    (bookmark-bmenu-mark)
+   (forward-line -1)
    (beginning-of-line)
    (should (looking-at "^>"))))
 
@@ -571,6 +574,7 @@ bookmark-test-bmenu-unmark
    (bookmark-bmenu-mark)
    (goto-char (point-min))
    (bookmark-bmenu-unmark)
+   (forward-line -1)
    (beginning-of-line)
    (should (looking-at "^  "))))
 


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan, could you take a look at this, and then we can get it onto the
> trunk?

Yup, this is already on my TODO list.  But I can't promise I'll get to it soon.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Drew Adams]

> > Makes sense to me.  There was an objection about packages that
> > extend bookmark.el would no longer work...  but I don't think that's an
> > objection we have to heed.  As far as I can see, the public interface
> > isn't changed (i.e., the commands are still the same), which is the only
> > this we try to keep compatible.
> 
> The patch no longer applied, so I tried to respin it for the current
> emacs, but this leads to three test failures:
...
> Stefan, could you take a look at this, and then we can get it onto the
> trunk?

I repeat that this is yet another step backward for Emacs.

And a push to separate Bookmark+ from being an extension
of bookmark.el.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Stefan, could you take a look at this, and then we can get it onto the
>> trunk?
>
> Yup, this is already on my TODO list.  But I can't promise I'll get to
> it soon.

OK, no problem.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Stefan Kangas]

close 39293 28.1
thanks

Lars Ingebrigtsen <larsi@gnus.org> writes:

>>> Stefan, could you take a look at this, and then we can get it onto the
>>> trunk?
>>
>> Yup, this is already on my TODO list.  But I can't promise I'll get to
>> it soon.
>
> OK, no problem.

Now pushed to master as commit 61e51fee9c.

bug#39293: [PATCH] Base bookmark-bmenu-mode on 'tabulated-list-mode'

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Now pushed to master as commit 61e51fee9c.

Thanks; looks good.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no