bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag

bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag

[Gabriel]

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

Severity: wishlist

Description:

Add new choices 'type and 'file to `bookmark-sort-flag'.  See [1].

Notes:

1) It feels strange to have value t meaning 'name and value nil meaning
'creation-time, but it's better to not change how things have been
working for many years.  A more flexible approach could offer a custom
function as a choice of `bookmark-sort-flag'.

2) I took the liberty to update some comments and docstrings, please
review.

3) I took the liberty to remove some code in `bookmark-bmenu-mode' that
seems to be unnecessary, please review.  Everything seems to be working
as expected according to my tests.  I can also write some tests.

4) Used `string-collate-lessp' in `bookmark-maybe-sort-alist' to match
`bookmark-bmenu--*-predicate' functions.

[1] https://lists.gnu.org/archive/html/emacs-devel/2022-11/msg00995.html


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-more-choices-to-bookmark-sort-flag.patch --]
[-- Type: text/x-diff, Size: 7350 bytes --]

From c41d0990774a39a942e645aeaaec1126de419db1 Mon Sep 17 00:00:00 2001
From: Gabriel do Nascimento Ribeiro <gabriel376@hotmail.com>
Date: Fri, 25 Nov 2022 15:06:58 -0300
Subject: [PATCH 1/1] Add more choices to bookmark-sort-flag

* lisp/bookmark.el (bookmark-sort-flag): Add new choices 'type and
'file.
(bookmark-maybe-sort-alist, bookmark-bmenu-mode): Handle new choices
of `bookmark-sort-flag'.
(bookmark-bmenu-mode): Update docstring and remove unnecessary code.

* etc/NEWS: Announce the changes.
---
 etc/NEWS         |  5 ++--
 lisp/bookmark.el | 69 +++++++++++++++++++++++-------------------------
 2 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 0ffc849fec..acd0ed1d6e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2554,9 +2554,8 @@ Types are registered via a 'bookmark-handler-type' symbol property on
 the jumping function.
 
 +++
-*** 'bookmark-sort-flag' can now be set to 'last-modified'.
-This will display bookmark list from most recently set to least
-recently set.
+*** 'bookmark-sort-flag' now accepts more choices.
+It now accepts the values 'type', 'file' and 'last-modified'.
 
 ---
 *** When editing a bookmark annotation, 'C-c C-k' will now cancel.
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 7f3a264f53..084f7450fe 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -38,13 +38,6 @@
 (require 'fringe) ; for builds --without-x
 (eval-when-compile (require 'cl-lib))
 
-;;; Misc comments:
-;;
-;; The bookmark list is sorted lexically by default, but you can turn
-;; this off by setting bookmark-sort-flag to nil.  If it is nil, then
-;; the list will be presented in the order it is recorded
-;; (chronologically), which is actually fairly useful as well.
-
 ;;; User Variables
 
 (defgroup bookmark nil
@@ -116,16 +109,16 @@ bookmark-completion-ignore-case
 
 
 (defcustom bookmark-sort-flag t
-  "This controls the bookmark display sorting.
-nil means they will be displayed in LIFO order (that is, most
-recently created ones come first, oldest ones come last).
-
-`last-modified' means that bookmarks will be displayed sorted
-from most recently modified to least recently modified.
+  "Controls the bookmark display sorting.
 
-Other values means that bookmarks will be displayed sorted by
-bookmark name."
+A value of t means to display bookmarks sorted by name.  A value
+of `type' means to display bookmarks sorted by type.  A value of
+`file' means to display bookmarks sorted by file.  A value of
+`last-modified' means to display bookmarks by modification time.
+A value of nil means to display bookmarks by creation time."
   :type '(choice (const :tag "By name" t)
+                 (const :tag "By type" type)
+                 (const :tag "By file" file)
                  (const :tag "By modified time" last-modified)
                  (const :tag "By creation time" nil)))
 
@@ -524,14 +517,22 @@ bookmark--remove-fringe-mark
                 (delete-overlay (setq found temp))))))))))
 
 (defun bookmark-maybe-sort-alist ()
-  "Return `bookmark-alist' for display.
-If `bookmark-sort-flag' is T, then return a sorted by name copy of the alist.
-If `bookmark-sort-flag' is LAST-MODIFIED, then return a sorted by last modified
-copy of the alist.  Otherwise, just return `bookmark-alist', which by default
-is ordered from most recently created to least recently created bookmark."
+  "Return a copy of `bookmark-alist' for display.
+
+The sort order is controlled by `bookmark-sort-flag'."
   (let ((copy (copy-alist bookmark-alist)))
     (cond ((eq bookmark-sort-flag t)
-           (sort copy (lambda (x y) (string-lessp (car x) (car y)))))
+           (sort copy (lambda (x y)
+                        (string-collate-lessp  (bookmark-name-from-full-record x)
+                                               (bookmark-name-from-full-record y)))))
+          ((eq bookmark-sort-flag 'type)
+           (sort copy (lambda (x y)
+                        (string-collate-lessp (bookmark-type-from-full-record x)
+                                              (bookmark-type-from-full-record y)))))
+          ((eq bookmark-sort-flag 'file)
+           (sort copy (lambda (x y)
+                        (string-collate-lessp (bookmark-location x)
+                                              (bookmark-location y)))))
           ((eq bookmark-sort-flag 'last-modified)
            (sort copy (lambda (x y)
                         (let ((tx (bookmark-get-last-modified x))
@@ -1903,6 +1904,12 @@ bookmark-bmenu--revert
     (cond ((eq bookmark-sort-flag t)
            (setq tabulated-list-sort-key '("Bookmark Name" . nil)
                  tabulated-list-entries entries))
+          ((eq bookmark-sort-flag 'type)
+           (setq tabulated-list-sort-key '("Type" . nil)
+                 tabulated-list-entries entries))
+          ((eq bookmark-sort-flag 'file)
+           (setq tabulated-list-sort-key '("File" . nil)
+                 tabulated-list-entries entries))
           ((or (null bookmark-sort-flag)
                (eq bookmark-sort-flag 'last-modified))
            (setq tabulated-list-sort-key nil)
@@ -1971,16 +1978,10 @@ bookmark-bmenu-mode
 Letters do not insert themselves; instead, they are commands.
 Bookmark names preceded by a \"*\" have annotations.
 
-If `bookmark-sort-flag' is non-nil, then sort the list by
-bookmark name (case-insensitively, in collation order); the
-direction of that sort can be reversed by using the column sort
-toggle for the bookmark name column.
-
-If `bookmark-sort-flag' is nil, then sort the list by bookmark
-creation order, with most recently created bookmarks on top.
-However, the column sort toggle will still activate (and
-thereafter toggle the direction of) lexical sorting by bookmark name.
-At any time you may use \\[revert-buffer] to go back to sorting by creation order.
+The sort order can be controlled with `bookmark-sort-flag'.  The
+sort direction can be reversed by using the column sort toggle
+for the bookmark name column.  At any time you may use
+\\[revert-buffer] to go back to the original sort order.
 
 \\<bookmark-bmenu-mode-map>
 \\[bookmark-bmenu-mark] -- mark bookmark to be displayed.
@@ -2015,8 +2016,7 @@ 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.
-\\[revert-buffer] -- refresh the buffer, and thus refresh the sort order (useful
-  if `bookmark-sort-flag' is nil)."
+\\[revert-buffer] -- refresh the buffer, and thus refresh the sort order."
   (setq truncate-lines t)
   (setq buffer-read-only t)
   ;; FIXME: The header could also display the current default bookmark file
@@ -2029,9 +2029,6 @@ bookmark-bmenu-mode
           ,@(if bookmark-bmenu-toggle-filenames
                 '(("File" 0 bookmark-bmenu--file-predicate)))])
   (setq tabulated-list-padding bookmark-bmenu-marks-width)
-  (when (and bookmark-sort-flag
-             (not (eq bookmark-sort-flag 'last-modified)))
-    (setq tabulated-list-sort-key '("Bookmark Name" . nil)))
   (add-hook 'tabulated-list-revert-hook #'bookmark-bmenu--revert nil t)'
   (setq revert-buffer-function 'bookmark-bmenu--revert)
   (tabulated-list-init-header))
-- 
2.34.1


[-- Attachment #3: Type: text/plain, Size: 13 bytes --]


---
Gabriel

bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag

[Eli Zaretskii]

> From: Gabriel <gabriel376@hotmail.com>
> Date: Fri, 25 Nov 2022 15:26:49 -0300
> 
> Add new choices 'type and 'file to `bookmark-sort-flag'.  See [1].
> 
> Notes:
> 
> 1) It feels strange to have value t meaning 'name and value nil meaning
> 'creation-time, but it's better to not change how things have been
> working for many years.  A more flexible approach could offer a custom
> function as a choice of `bookmark-sort-flag'.
> 
> 2) I took the liberty to update some comments and docstrings, please
> review.
> 
> 3) I took the liberty to remove some code in `bookmark-bmenu-mode' that
> seems to be unnecessary, please review.  Everything seems to be working
> as expected according to my tests.  I can also write some tests.
> 
> 4) Used `string-collate-lessp' in `bookmark-maybe-sort-alist' to match
> `bookmark-bmenu--*-predicate' functions.

Is it really a good idea to use string-collate-lessp here?  Its effect
depends on the underlying OS and the user's locale, so it could produce
different and sometimes surprising results.  Why isn't string-lessp
sufficient?  IMO, at the very least, the fact that the sorting is
locale-dependent should be prominently stated in the doc string.  Also, see
the notes in the string-collate-lessp's doc string about MS-Windows, and
maybe do what it suggests (if we decide to keep string-collate-lessp here).

Karl, any comments?

Thanks.

bug#59580: and bug#59581

[Karl Fogel]

I am just seeing tickets #59580 and #59581 now.  I will review the 
patches as soon as I can and reply back.  This won't be till the 
end of the week, however, as I am traveling before then and won't 
have much working time while on the road.

Thanks for the patches Gabriel, and thanks for the ping, Eli.

Best regards,
-Karl

bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag

[Karl Fogel]

On 01 Dec 2022, Eli Zaretskii wrote:
>> From: Gabriel <gabriel376@hotmail.com>
>> Date: Fri, 25 Nov 2022 15:26:49 -0300
>> 
>> Add new choices 'type and 'file to `bookmark-sort-flag'.  See 
>> [1].
>> 
>> Notes:
>> 
>> 1) It feels strange to have value t meaning 'name and value nil 
>> meaning
>> 'creation-time, but it's better to not change how things have 
>> been
>> working for many years.  A more flexible approach could offer a 
>> custom
>> function as a choice of `bookmark-sort-flag'.
>> 
>> 2) I took the liberty to update some comments and docstrings, 
>> please
>> review.
>> 
>> 3) I took the liberty to remove some code in 
>> `bookmark-bmenu-mode' that
>> seems to be unnecessary, please review.  Everything seems to be 
>> working
>> as expected according to my tests.  I can also write some 
>> tests.
>> 
>> 4) Used `string-collate-lessp' in `bookmark-maybe-sort-alist' 
>> to match
>> `bookmark-bmenu--*-predicate' functions.
>
>Is it really a good idea to use string-collate-lessp here?  Its 
>effect
>depends on the underlying OS and the user's locale, so it could 
>produce
>different and sometimes surprising results.  Why isn't 
>string-lessp
>sufficient?  IMO, at the very least, the fact that the sorting is
>locale-dependent should be prominently stated in the doc string. 
>Also, see
>the notes in the string-collate-lessp's doc string about 
>MS-Windows, and
>maybe do what it suggests (if we decide to keep 
>string-collate-lessp here).
>
>Karl, any comments?

I finally had a chance to look more closely at this.

First, overall very nice job on the patch, Gabriel.  You took care 
to strike a balance between, one the one hand, cleaning up things 
that directly touched your changes and, on the other hand, not 
causing unnecessary churn.

Regarding the use of `string-collate-lessp':

I think using `string-collate-lessp' makes sense here.  This is 
about sorted lists presented to the user, so the sorting should 
act according to locale (and when Emacs suspects that the locale 
wouldn't collate correctly, `string-collate-lessp' falls back to 
`string-lessp' anyway).

However, it would be good to:

  - Bind `w32-collate-ignore-punctuation' to non-nil

  - Pass optional arg IGNORE-CASE to `string-collate-lessp'

These two things would result in the behavior(s) that the user is 
most likely to expect, I believe.

If we later learn that people want more control over these 
behaviors, then we could make control variables -- something like, 
e.g., `bookmark-sort-w32-collate-ignore-punctuation' and 
`bookmark-sort-ignore-case'.  But it would be premature to offer 
those right now.  Let's start by choosing reasonable defaults and 
see if people are fine with that.

Thoughts?

The patch is a bit out-of-date now: The diff againt 'etc/NEWS' 
depends on context that has now moved to 'etc/NEWS.29'.  The diff 
against 'bookmark.el' applies cleanly, however, just with some new 
offsets due to things having shifted around.

Best regards,
-Karl

bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag

[Drew Adams]

FWIW -

An option name `*-flag' is explicitly
intended to signify a Boolean option.
"Adding more choices" to such an option
should entail renaming it.

As for bookmark sort orders, there are
many more that make sense - see here for
several that are predefined, and for how
users can easily combine them or define
their own:

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

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

bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: Gabriel <gabriel376@hotmail.com>,  59580@debbugs.gnu.org
> Date: Mon, 18 Dec 2023 17:26:09 -0600
> 
> Regarding the use of `string-collate-lessp':
> 
> I think using `string-collate-lessp' makes sense here.  This is 
> about sorted lists presented to the user, so the sorting should 
> act according to locale (and when Emacs suspects that the locale 
> wouldn't collate correctly, `string-collate-lessp' falls back to 
> `string-lessp' anyway).

It is IMO un-Emacsy to do things in locale-dependent manner.  Which
other feature do you know of that sorts text in locale-dependent
fashion?

So I still think using string-collate-lessp here is not the best idea.

bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag

[Karl Fogel]

On 19 Dec 2023, Eli Zaretskii wrote:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Cc: Gabriel <gabriel376@hotmail.com>,  59580@debbugs.gnu.org
>> Date: Mon, 18 Dec 2023 17:26:09 -0600
>> 
>> Regarding the use of `string-collate-lessp':
>> 
>> I think using `string-collate-lessp' makes sense here.  This is 
>> about sorted lists presented to the user, so the sorting should 
>> act according to locale (and when Emacs suspects that the 
>> locale 
>> wouldn't collate correctly, `string-collate-lessp' falls back 
>> to 
>> `string-lessp' anyway).
>
>It is IMO un-Emacsy to do things in locale-dependent manner. 
>Which
>other feature do you know of that sorts text in locale-dependent
>fashion?

I have no idea -- I doubt I would ever notice it if a feature did 
sort that way, since I'd be accustomed to that sorting behavior. 
I also don't see why such behavior is particularly un-Emacsy.  But 
I don't feel strongly about it; if you think `string-lessp' is 
better, let's use that.

Best regards,
-Karl