[PATCH] Fix bookmark-bmenu-list sorting.

[PATCH] Fix bookmark-bmenu-list sorting.

[Manuel Giraud]

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

Hi,

Here is a patch that I think fix the default `bookmark-bmenu-list'
sorting when `bookmark-sort-flag' is nil.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-bookmark-bmenu-list-sorting.patch --]
[-- Type: text/x-patch, Size: 1438 bytes --]

From e9ace5a7c965b725b003c910f6608d0bb7e3aaa5 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Thu, 3 Mar 2022 17:32:13 +0100
Subject: [PATCH] Fix bookmark-bmenu-list sorting.

Do not sort bookmarks in `bookmark-bmenu-list' if `bookmark-sort-flag'
is nil.

Also, make the default order of bookmark-bmenu-list be the LIFO order
defined in `bookmark-sort-flag's documentation.
---
 lisp/bookmark.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 2751731817..80fb1cdfc7 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -1819,7 +1819,7 @@ bookmark-bmenu--revert
                        (list location))])
               entries)))
     (tabulated-list-init-header)
-    (setq tabulated-list-entries entries))
+    (setq tabulated-list-entries (reverse entries)))
   (tabulated-list-print t))
 
 ;;;###autoload
@@ -1907,7 +1907,8 @@ bookmark-bmenu-mode
           ,@(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))
+  (when bookmark-sort-flag
+    (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))
-- 
2.35.1


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

-- 
Manuel Giraud

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Karl Fogel]

On 03 Mar 2022, Manuel Giraud wrote:
>Here is a patch that I think fix the default 
>`bookmark-bmenu-list'
>sorting when `bookmark-sort-flag' is nil.

This looks correct to me.  Thank you for the fix.  I'm testing it 
now on 'emacs-28' and 'master' (it should behave the same on both, 
of course, but might as well make sure).  Assuming it behaves as 
expected, I'll apply using 'git patch' on the emacs-28 branch.

Best regards,
-Karl

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Thu, 03 Mar 2022 11:41:55 -0600
> Cc: emacs-devel <emacs-devel@gnu.org>
> 
> On 03 Mar 2022, Manuel Giraud wrote:
> >Here is a patch that I think fix the default 
> >`bookmark-bmenu-list'
> >sorting when `bookmark-sort-flag' is nil.
> 
> This looks correct to me.  Thank you for the fix.  I'm testing it 
> now on 'emacs-28' and 'master' (it should behave the same on both, 
> of course, but might as well make sure).  Assuming it behaves as 
> expected, I'll apply using 'git patch' on the emacs-28 branch.

Please don't install new features on the release branch.

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Karl Fogel]

On 03 Mar 2022, Eli Zaretskii wrote: 
>> From: Karl Fogel <kfogel@red-bean.com> Date: Thu, 03 Mar 2022 
>> 11:41:55 -0600 Cc: emacs-devel <emacs-devel@gnu.org>  On 03 Mar 
>> 2022, Manuel Giraud wrote: 
>> >Here is a patch that I think fix the default 
>> >`bookmark-bmenu-list' sorting when `bookmark-sort-flag' is 
>> >nil. 
>>  This looks correct to me.  Thank you for the fix.  I'm testing 
>> it  now on 'emacs-28' and 'master' (it should behave the same 
>> on both,  of course, but might as well make sure).  Assuming it 
>> behaves as  expected, I'll apply using 'git patch' on the 
>> emacs-28 branch. 
> Please don't install new features on the release branch. 
 
This is a bugfix, not a new feature.  I re-read the "Branches" 
section in CONTRIBUTE before I posted -- the relevant part is 
this, I think:

  > If you are fixing a bug that exists in the current release, 
  > you
  > should generally commit it to the release branch; it will be 
  > merged
  > to the master branch later by the gitmerge function.  However, 
  > when
  > the release branch is for Emacs version NN.2 and later, or 
  > when it
  > is for Emacs version NN.1 that is in the very last stages of 
  > its
  > pretest, that branch is considered to be in a feature freeze: 
  > only
  > bug fixes that are "safe" or are fixing major problems should 
  > go to
  > the release branch, the rest should be committed to the master
  > branch.  This is so to avoid destabilizing the next Emacs 
  > release.
  > If you are unsure whether your bug fix is "safe" enough for 
  > the
  > release branch, ask on the emacs-devel mailing list.

That indicates that 'emacs-28' is the right branch for this 
change.  That branch is on version 28.0.91 right now, not 28.2 nor 
late 28.1.

I'm happy to put this change on whatever branch you prefer, of 
course.  However, independent of this specific case, in general 
how should one determine what branch to put something on, if the 
guidance in CONTRIBUTE isn't enough?  (Or am I misreading that 
guidance?  It seems pretty straightforward...)   This change 
easily fits the description "safe", by the way.

(In the course of testing it, I've discovered another interesting 
corner-case buglet, but I'll commit this first and then deal with 
the buglet separately.  That second fix is likely to be safe too.)

Best regards, -Karl 

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: manuel@ledu-giraud.fr,  emacs-devel@gnu.org
> Date: Thu, 03 Mar 2022 21:13:04 -0600
> 
> >>  This looks correct to me.  Thank you for the fix.  I'm testing 
> >> it  now on 'emacs-28' and 'master' (it should behave the same 
> >> on both,  of course, but might as well make sure).  Assuming it 
> >> behaves as  expected, I'll apply using 'git patch' on the 
> >> emacs-28 branch. 
> > Please don't install new features on the release branch. 
>  
> This is a bugfix, not a new feature.

The original message didn't describe any bugs, so I concluded it was a
new feature.  Sorry about that.

However, if this is a bug, please tell:

  . what is the bug and how to reproduce it?
  . if this is a regression, what was the last Emacs version where the
    reproduction recipe worked correctly?

> I re-read the "Branches" section in CONTRIBUTE before I posted --
> the relevant part is this, I think:
> 
>   > If you are fixing a bug that exists in the current release, 
>   > you
>   > should generally commit it to the release branch; it will be 
>   > merged
>   > to the master branch later by the gitmerge function.  However, 
>   > when
>   > the release branch is for Emacs version NN.2 and later, or 
>   > when it
>   > is for Emacs version NN.1 that is in the very last stages of 
>   > its
>   > pretest, that branch is considered to be in a feature freeze: 
>   > only
>   > bug fixes that are "safe" or are fixing major problems should 
>   > go to
>   > the release branch, the rest should be committed to the master
>   > branch.  This is so to avoid destabilizing the next Emacs 
>   > release.
>   > If you are unsure whether your bug fix is "safe" enough for 
>   > the
>   > release branch, ask on the emacs-devel mailing list.
> 
> That indicates that 'emacs-28' is the right branch for this 
> change.  That branch is on version 28.0.91 right now, not 28.2 nor 
> late 28.1.

How do you know whether the current pretest of 28.1 is or isn't "in
the very latest stages of its pretest"?  The text says to ask if you
aren't sure.  As things are, we are, I hope, in the very latest
stages.

> I'm happy to put this change on whatever branch you prefer, of 
> course.

I will make up my mind after I know the answers to the above
questions.

> However, independent of this specific case, in general 
> how should one determine what branch to put something on, if the 
> guidance in CONTRIBUTE isn't enough?

It's a judgment call.  Which is why the text says to ask.

Thanks.

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Karl Fogel]

On 04 Mar 2022, Eli Zaretskii wrote:
>The original message didn't describe any bugs, so I concluded it 
>was a
>new feature.  Sorry about that.
>
>However, if this is a bug, please tell:
>
>  . what is the bug and how to reproduce it?
>  . if this is a regression, what was the last Emacs version 
>  where the
>    reproduction recipe worked correctly?

The variable `bookmark-sort-flag' is not obeyed in 
`bookmark-bmenu-mode' (that's the Dired-like mode entered 
interactively via `edit-bookmarks').

The behavior is supposed to be that when `bookmark-sort-flag' is 
non-nil, bookmarks are listed in lexical order by bookmark name, 
and when the variable is nil, they are listed in order from 
most-recently-created down to least-recently-created

(As an aside: the latter case relies on the assumption that the 
order of elements in `bookmark-alist' reflects the order in which 
the bookmarks were created.  This happens to be true, and is even 
documented in a couple of places in bookmark.el, but is not 
well-documented at the code site where it is actually implemented. 
That's something I'll take care of separately after handling 
Manuel's patch.)

The bug Manuel found is that if you set `bookmark-sort-flag' to 
nil, the list is still displayed in lexically sorted order, as 
though the flag were still non-nil.  His patch fixes that 
(although there is still a related pre-existing buglet, which I 
discuss farther down in this email).

Manuel's original post said this in a compact and less detailed 
way: "Here is a patch that I think fix the default 
`bookmark-bmenu-list' sorting when `bookmark-sort-flag' is nil."

>How do you know whether the current pretest of 28.1 is or isn't 
>"in
>the very latest stages of its pretest"?  The text says to ask if 
>you
>aren't sure.  As things are, we are, I hope, in the very latest
>stages.

Oh, I see now.  There's an ambiguity (between CONTRIBUTE and how 
version numbers are recorded in the release branch's README), but 
from what you say, I now see that where CONTRIBUTE says "when it 
is for Emacs version NN.1 that is in the very last stages of its 
pretest", that means that the version number in the README on the 
branch will say NN.0.X.  That isn't obvious from CONTRIBUTE, but 
it's a perfectly reasonable interpretation.  (The other 
interpretation -- the one I made -- is that README would contain 
NN.1[.x] already; some projects do release branches one way, some 
do it the other.)

>> I'm happy to put this change on whatever branch you prefer, of
>> course.
>
>I will make up my mind after I know the answers to the above
>questions.

In addition to the two changes described above (Manuel's patch, 
and a separate internal comment fix), there is another buglet I am 
chasing, which I found when I was testing Manuel's patch:

Even after Manuel's fix, the buffer generated by `edit-bookmarks' 
will not reorder the bookmarks after one toggles 
`bookmark-sort-flag' and types `g' (which runs `revert-buffer', 
which in this case invokes a `revert-buffer-function' that 
regenerates the buffer).  But this problem is only for the first 
incarnation of that buffer!  If one kills the buffer and rebuilds 
it from scratch, after that, the toggle-flag-and-revert routine 
*will* correctly reorder the bookmarks each time. 

This is a very minor buglet, in terms of its effects on users, 
since most people set `bookmark-sort-flag' in their init files 
anyway and aren't likely to toggle it during an Emacs session. 
However, it still bothers me, and it certainly interferes with 
testing `bookmark-sort-flag', so I'm planning to fix it too.

So in summary, there are three small changes coming:

1) Manuel's original bugfix

2) Internal comment fix, motivated by (1)

3) Fix for the toggle-regenerate buglet.

I would mildly prefer to put them all on the same branch, but I 
don't care which branch that is; let me know what you'd like.

If you would like them on different branches, I can do that, just 
let me know which change goes to which branch.

Best regards,
-Karl

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: manuel@ledu-giraud.fr,  emacs-devel@gnu.org
> Date: Fri, 04 Mar 2022 02:18:44 -0600
> 
> On 04 Mar 2022, Eli Zaretskii wrote:
> >The original message didn't describe any bugs, so I concluded it 
> >was a
> >new feature.  Sorry about that.
> >
> >However, if this is a bug, please tell:
> >
> >  . what is the bug and how to reproduce it?
> >  . if this is a regression, what was the last Emacs version 
> >  where the
> >    reproduction recipe worked correctly?
> 
> The variable `bookmark-sort-flag' is not obeyed in 
> `bookmark-bmenu-mode' (that's the Dired-like mode entered 
> interactively via `edit-bookmarks').

But it _is_ obeyed in other places in bookmark.el, including in
bookmark-bmenu--revert (and thus in its callers).  So it isn't like
this variable is not obeyed at all, it's just that there seem to be
some specific situations where it isn't.

And you didn't answer my second question (and neither did Manuel).
Knowing whether this is a recent regression or not is important for
the decision whether to fix it now or later.

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> And you didn't answer my second question (and neither did Manuel).
> Knowing whether this is a recent regression or not is important for
> the decision whether to fix it now or later.

I cannot say if it is a regression but it seems that the patch
61e51fee9ca3 of october 17, 2020 rewrote bookmark-bmenu-mode to use
tabulated-list-mode and set the tabulated-list-sort-key on the bookmark
name.
-- 
Manuel Giraud

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Karl Fogel <kfogel@red-bean.com>,  emacs-devel@gnu.org
> Date: Fri, 04 Mar 2022 14:25:37 +0100
> 
> I cannot say if it is a regression but it seems that the patch
> 61e51fee9ca3 of october 17, 2020 rewrote bookmark-bmenu-mode to use
> tabulated-list-mode and set the tabulated-list-sort-key on the bookmark
> name.

Thanks.  So this did work correctly in Emacs 27.2?

Or maybe you can show a simple recipe starting from "emacs -Q", then I
could see for myself what happened in Emacs 27 vs Emacs 28.

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Manuel Giraud <manuel@ledu-giraud.fr>
>> Cc: Karl Fogel <kfogel@red-bean.com>,  emacs-devel@gnu.org
>> Date: Fri, 04 Mar 2022 14:25:37 +0100
>> 
>> I cannot say if it is a regression but it seems that the patch
>> 61e51fee9ca3 of october 17, 2020 rewrote bookmark-bmenu-mode to use
>> tabulated-list-mode and set the tabulated-list-sort-key on the bookmark
>> name.
>
> Thanks.  So this did work correctly in Emacs 27.2?
>
> Or maybe you can show a simple recipe starting from "emacs -Q", then I
> could see for myself what happened in Emacs 27 vs Emacs 28.

This would do:
--8<---------------cut here---------------start------------->8---
(defun dobook (name)
  (with-current-buffer (get-buffer-create name)
    (set-visited-file-name (format "/tmp/%s" name))
    (bookmark-set (buffer-name))))

(progn
  (dobook "a")
  (dobook "b"))
--8<---------------cut here---------------end--------------->8---

And then "M-x bookmark-bmenu-list". As bookmark "b" was the last
defined, it should be at the top of the list… but I've just tested in
Emacs 27.2 and the bookmark list was already alphabetically sorted. So I
guess this is not a regression (and won't make it into 28 then).
-- 
Manuel Giraud

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: kfogel@red-bean.com,  emacs-devel@gnu.org
> Date: Fri, 04 Mar 2022 16:15:51 +0100
> 
> > Or maybe you can show a simple recipe starting from "emacs -Q", then I
> > could see for myself what happened in Emacs 27 vs Emacs 28.
> 
> This would do:
> --8<---------------cut here---------------start------------->8---
> (defun dobook (name)
>   (with-current-buffer (get-buffer-create name)
>     (set-visited-file-name (format "/tmp/%s" name))
>     (bookmark-set (buffer-name))))
> 
> (progn
>   (dobook "a")
>   (dobook "b"))
> --8<---------------cut here---------------end--------------->8---
> 
> And then "M-x bookmark-bmenu-list". As bookmark "b" was the last
> defined, it should be at the top of the list… but I've just tested in
> Emacs 27.2 and the bookmark list was already alphabetically sorted. So I
> guess this is not a regression (and won't make it into 28 then).

OK, thanks.

One more question: your patch included this part:

> diff --git a/lisp/bookmark.el b/lisp/bookmark.el
> index 2751731817..80fb1cdfc7 100644
> --- a/lisp/bookmark.el
> +++ b/lisp/bookmark.el
> @@ -1819,7 +1819,7 @@ bookmark-bmenu--revert
>                         (list location))])
>                entries)))
>      (tabulated-list-init-header)
> -    (setq tabulated-list-entries entries))
> +    (setq tabulated-list-entries (reverse entries)))
>    (tabulated-list-print t))

Why is that needed?

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> One more question: your patch included this part:
>
>> diff --git a/lisp/bookmark.el b/lisp/bookmark.el
>> index 2751731817..80fb1cdfc7 100644
>> --- a/lisp/bookmark.el
>> +++ b/lisp/bookmark.el
>> @@ -1819,7 +1819,7 @@ bookmark-bmenu--revert
>>                         (list location))])
>>                entries)))
>>      (tabulated-list-init-header)
>> -    (setq tabulated-list-entries entries))
>> +    (setq tabulated-list-entries (reverse entries)))
>>    (tabulated-list-print t))
>
> Why is that needed?

It is needed because the 'entries' list is constructed by iterating over
'bookmark-alist' and pushing new element to it. So in the end, 'entries'
is in the exact reverse order of 'bookmark-alist'.

As it seems intended that 'bookmark-alist' is ordered with most recent
bookmark at the beginning, I tried to keep this order in the "Bookmark
List" buffer.
-- 
Manuel Giraud

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: kfogel@red-bean.com,  emacs-devel@gnu.org
> Date: Fri, 04 Mar 2022 18:41:23 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > One more question: your patch included this part:
> >
> >> diff --git a/lisp/bookmark.el b/lisp/bookmark.el
> >> index 2751731817..80fb1cdfc7 100644
> >> --- a/lisp/bookmark.el
> >> +++ b/lisp/bookmark.el
> >> @@ -1819,7 +1819,7 @@ bookmark-bmenu--revert
> >>                         (list location))])
> >>                entries)))
> >>      (tabulated-list-init-header)
> >> -    (setq tabulated-list-entries entries))
> >> +    (setq tabulated-list-entries (reverse entries)))
> >>    (tabulated-list-print t))
> >
> > Why is that needed?
> 
> It is needed because the 'entries' list is constructed by iterating over
> 'bookmark-alist' and pushing new element to it. So in the end, 'entries'
> is in the exact reverse order of 'bookmark-alist'.
> 
> As it seems intended that 'bookmark-alist' is ordered with most recent
> bookmark at the beginning, I tried to keep this order in the "Bookmark
> List" buffer.

Thanks.

Let's install this on master.  We can consider backporting to the
emacs-28 branch after Emacs 28.1 is released.

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Karl Fogel]

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

>Thanks.
>
>Let's install this on master.  We can consider backporting to the
>emacs-28 branch after Emacs 28.1 is released.

Installed in commit a6abd06c73.

Sorry I forgot to respond to your question earlier ("if this is a 
regression, what was the last Emacs version where the reproduction 
recipe worked correctly?"), Eli.  I didn't know the answer, and 
didn't have time to find out then, but I should have said so 
explicitly.  (There was enough other information in my reply that 
I didn't want to delay sending it.)

As I promised, there are two follow-on changes.  I think it would 
make sense to also put them on 'master', but I'll attach them here 
so you can see for yourself; let me know what you think.

Best regards,
-Karl


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-documentation-of-bookmark-default-sorting.patch --]
[-- Type: text/x-diff, Size: 2394 bytes --]

From 053a68966e0911529fffb87f2ea27e6e6597513f Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Sun, 6 Mar 2022 21:16:47 -0600
Subject: [PATCH] Improve documentation of bookmark default sorting

* lisp/bookmark.el (bookmark-alist, bookmark-store,
  bookmark-maybe-sort-alist): Update doc strings and comments.
---
 lisp/bookmark.el | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git lisp/bookmark.el lisp/bookmark.el
index 80fb1cdfc7..525bc9dc15 100644
--- lisp/bookmark.el
+++ lisp/bookmark.el
@@ -246,11 +246,13 @@ bookmark-alist
 Bookmark functions update the value automatically.
 You probably do NOT want to change the value yourself.
 
-The value is an alist with bookmarks of the form
+The value is an alist whose elements are the form
 
  (BOOKMARK-NAME . PARAM-ALIST)
 
-or the deprecated form (BOOKMARK-NAME PARAM-ALIST).
+or the deprecated form (BOOKMARK-NAME PARAM-ALIST).  The alist is
+ordered from most recently created bookmark at the front to least
+recently created bookmark at the end.
 
 BOOKMARK-NAME is the name you gave to the bookmark when creating it.
 
@@ -583,10 +585,10 @@ bookmark-store
           ;; Modify using the new (NAME . ALIST) format.
           (setcdr bm alist))
 
-      ;; otherwise just cons it onto the front (either the bookmark
-      ;; doesn't exist already, or there is no prefix arg.  In either
-      ;; case, we want the new bookmark consed onto the alist...)
-
+      ;; Otherwise just put it onto the front of the list.  Either the
+      ;; bookmark doesn't exist already, or there is no prefix arg.
+      ;; In either case, we want the new bookmark on the front of the
+      ;; list, since the list is kept in reverse order of creation.
       (push (cons stripped-name alist) bookmark-alist))
 
     ;; Added by db
@@ -1140,7 +1142,9 @@ bookmark-maybe-load-default-file
 
 (defun bookmark-maybe-sort-alist ()
   "Return `bookmark-alist' for display.
-If `bookmark-sort-flag' is non-nil, then return a sorted copy of the alist."
+If `bookmark-sort-flag' is non-nil, then return a sorted copy of the alist.
+Otherwise, just return `bookmark-alist', which by default is ordered
+from most recently created to least recently created bookmark."
   (if bookmark-sort-flag
       (sort (copy-alist bookmark-alist)
             (lambda (x y) (string-lessp (car x) (car y))))
-- 
2.34.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Ensure-bookmark-bmenu-buffer-sorts-when-it-should.patch --]
[-- Type: text/x-diff, Size: 1320 bytes --]

From 4e5c163e32431a54b76aff88b82a1f95bb58efcc Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Sun, 6 Mar 2022 21:20:34 -0600
Subject: [PATCH] Ensure bookmark bmenu buffer sorts when it should

* lisp/bookmark.el (bookmark-bmenu--revert): Check `bookmark-sort-flag'
  every time when displaying bookmarks.
---
 lisp/bookmark.el | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git lisp/bookmark.el lisp/bookmark.el
index 525bc9dc15..6c72621bc3 100644
--- lisp/bookmark.el
+++ lisp/bookmark.el
@@ -1823,7 +1823,15 @@ bookmark-bmenu--revert
                        (list location))])
               entries)))
     (tabulated-list-init-header)
-    (setq tabulated-list-entries (reverse entries)))
+    ;; The value of `bookmark-sort-flag' might have changed since the
+    ;; last time the buffer contents were generated, so re-check it.
+    (if bookmark-sort-flag
+        (setq tabulated-list-sort-key '("Bookmark" . nil))
+      (setq tabulated-list-sort-key nil)
+      ;; And since we're not sorting by bookmark name, show bookmarks
+      ;; in reverse order of creation: most recently created at the
+      ;; top, least recently created at the bottom.
+      (setq tabulated-list-entries (reverse entries))))
   (tabulated-list-print t))
 
 ;;;###autoload
-- 
2.34.1

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Manuel Giraud]

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

>>Thanks.
>>
>>Let's install this on master.  We can consider backporting to the
>>emacs-28 branch after Emacs 28.1 is released.
>
> Installed in commit a6abd06c73.

Thanks. I've tested both of your patch and they are working as intended:
the order is modified between two `bookmark-bmenu-list' call if I change
`bookmark-sort-flag'.

There is one minor thing left: the tabulated-list bar (I don't how it is
called) keep the old information « sorted by bookmark name » when I have
revert to `bookmark-sort-flag' as nil.
-- 
Manuel Giraud

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Karl Fogel]

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

On 07 Mar 2022, Manuel Giraud wrote:
>Karl Fogel <kfogel@red-bean.com> writes:
>
>>>Thanks.
>>>
>>>Let's install this on master.  We can consider backporting to 
>>>the
>>>emacs-28 branch after Emacs 28.1 is released.
>>
>> Installed in commit a6abd06c73.
>
>Thanks. I've tested both of your patch and they are working as 
>intended:
>the order is modified between two `bookmark-bmenu-list' call if I 
>change
>`bookmark-sort-flag'.
>
>There is one minor thing left: the tabulated-list bar (I don't 
>how it is
>called) keep the old information « sorted by bookmark name » when 
>I have
>revert to `bookmark-sort-flag' as nil.

Manuel, could you please give me a more verbose reproduction 
recipe for that last paragraph above?  Maybe even include a 
screenshot?

I've attached an updated patch here (please use this version), and 
when I test it, I don't see anything that says "sorted by bookmark 
name".  I don't know what the tabulated-list bar you're referring 
to is, but I don't see anything that would obviously be it.

(I have tried things like turning `menu-bar-mode' on and off. 
I've also tried looking in the code of 
"lisp/emacs-lisp/tabulated-list.el", but I don't see any place 
where a message "sorted by bookmark name" would be set up.)

Now, there is another related UI issue:

In the tabulated-list header line at the top of the buffer, we 
have the expected three columns: "Bookmark", "Type", and "File". 
The first column, "Bookmark", has a down-triangle / up-triangle 
toggle for switching between descending and ascending sort by 
bookmark name.  But if `bookmark-sort-flag' is nil, then when the 
buffer is regenerated the bookmarks will be listed in 
creation-date order -- and the triangular toggle will just keep 
whichever value the triangle had already.

Maybe the right fix for that is for the buffer to somehow indicate 
*which* property the bookmarks are sorted by -- name or 
creation-date -- and make sure that the triangle always reflects 
the sorting direction of whichever thing is currently being sorted 
on.  However, before I go down that road, I'd like to fully 
understand your initial "one minor thing" bug report above first.

Best regards,
-Karl


[-- Attachment #2: 0001-Ensure-bookmark-bmenu-buffer-sorts-when-it-should.patch --]
[-- Type: text/x-diff, Size: 1913 bytes --]

From c8c00cac4bc16e7fccb228756b825ae022b315a0 Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Thu, 17 Mar 2022 22:47:16 -0500
Subject: [PATCH] Ensure bookmark bmenu buffer sorts when it should
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This patch is still WIP, as, as Manuel Giraud has pointed out in his
post "Re: [PATCH] Fix bookmark-bmenu-list sorting." of 7 March 2022
(Message-ID: <87zgm25d9o.fsf@elite.giraud>) the following:

  > There is one minor thing left: the tabulated-list bar (I don't how
  > it is called) keep the old information « sorted by bookmark name »
  > when I have revert to `bookmark-sort-flag' as nil.

I'm finding out more details from him; once we have it all sorted out,
I will update this change.
---
 lisp/bookmark.el | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git lisp/bookmark.el lisp/bookmark.el
index 80fb1cdfc7..400a673447 100644
--- lisp/bookmark.el
+++ lisp/bookmark.el
@@ -1819,7 +1819,17 @@ bookmark-bmenu--revert
                        (list location))])
               entries)))
     (tabulated-list-init-header)
-    (setq tabulated-list-entries (reverse entries)))
+    ;; The value of `bookmark-sort-flag' might have changed since the
+    ;; last time the buffer contents were generated, so re-check it.
+    (if bookmark-sort-flag
+        (progn
+          (setq tabulated-list-sort-key '("Bookmark" . nil))
+          (setq tabulated-list-entries entries))
+      (setq tabulated-list-sort-key nil)
+      ;; And since we're not sorting by bookmark name, show bookmarks
+      ;; in reverse order of creation: most recently created at the
+      ;; top, least recently created at the bottom.
+      (setq tabulated-list-entries (reverse entries))))
   (tabulated-list-print t))
 
 ;;;###autoload
-- 
2.35.1

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Manuel Giraud]

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

> Manuel, could you please give me a more verbose reproduction recipe
> for that last paragraph above?  Maybe even include a screenshot?
>
> I've attached an updated patch here (please use this version), and
> when I test it, I don't see anything that says "sorted by bookmark
> name".  I don't know what the tabulated-list bar you're referring to
> is, but I don't see anything that would obviously be it.
>
> (I have tried things like turning `menu-bar-mode' on and off. I've
> also tried looking in the code of "lisp/emacs-lisp/tabulated-list.el",
> but I don't see any place where a message "sorted by bookmark name"
> would be set up.)

Hi Karl,

I'm sorry I was not clear at all. The «sorted by bookmark name» I was
refering to is the information about the "Bookmark" column entry being
bold and with a down-triangle next to it (ie. what you are describing
later in your message).

> Now, there is another related UI issue:
>
> In the tabulated-list header line at the top of the buffer, we have
> the expected three columns: "Bookmark", "Type", and "File". The first
> column, "Bookmark", has a down-triangle / up-triangle toggle for
> switching between descending and ascending sort by bookmark name.  But
> if `bookmark-sort-flag' is nil, then when the buffer is regenerated
> the bookmarks will be listed in creation-date order -- and the
> triangular toggle will just keep whichever value the triangle had
> already.

So yes that was what I was talking about (only better described 😅).

> Maybe the right fix for that is for the buffer to somehow indicate
> *which* property the bookmarks are sorted by -- name or creation-date
> -- and make sure that the triangle always reflects the sorting
> direction of whichever thing is currently being sorted on.  However,
> before I go down that road, I'd like to fully understand your initial
> "one minor thing" bug report above first.

Or maybe you can «reset the state» of the tabulated-list (as it is on
first bookmark-bmenu-list call)? But I don't really know how
tabulated-list works.

Best regards,
-- 
Manuel Giraud

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Karl Fogel]

On 21 Mar 2022, Manuel Giraud wrote:
>> In the tabulated-list header line at the top of the buffer, we 
>> have
>> the expected three columns: "Bookmark", "Type", and "File". The 
>> first
>> column, "Bookmark", has a down-triangle / up-triangle toggle 
>> for
>> switching between descending and ascending sort by bookmark 
>> name.  But
>> if `bookmark-sort-flag' is nil, then when the buffer is 
>> regenerated
>> the bookmarks will be listed in creation-date order -- and the
>> triangular toggle will just keep whichever value the triangle 
>> had
>> already.
>
>So yes that was what I was talking about (only better described 
>😅).

Ah, got it, thanks!

>> Maybe the right fix for that is for the buffer to somehow 
>> indicate
>> *which* property the bookmarks are sorted by -- name or 
>> creation-date
>> -- and make sure that the triangle always reflects the sorting
>> direction of whichever thing is currently being sorted on. 
>> However,
>> before I go down that road, I'd like to fully understand your 
>> initial
>> "one minor thing" bug report above first.
>
>Or maybe you can «reset the state» of the tabulated-list (as it 
>is on
>first bookmark-bmenu-list call)? But I don't really know how
>tabulated-list works.

Okay -- I'll figure it out and Do The Right Thing.  Thank you, 
Manuel.

Best regards,
-Karl

[PATCH] Improve sorting in the bookmark list buffer.

[Karl Fogel]

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

On 07 Mar 2022, Manuel Giraud wrote:
>There is one minor thing left: the tabulated-list bar (I don't 
>how it is
>called) keep the old information « sorted by bookmark name » when 
>I have
>revert to `bookmark-sort-flag' as nil.

I looked more closely at the situation and came up with the 
attached patch.  The new behavior is a little complex, but I think 
it's the right thing for most users:

If `bookmark-sort-flag' is non-nil, which is the default, then the 
bookmark list [1] will be sorted lexically (case-insensitively, 
and in locale-dependent collation order).

If `bookmark-sort-flag' is nil, then the list starts out sorted by 
bookmark creation order, *but* clicking on the sort-control toggle 
for the Bookmark Name column will sort the list lexically (and 
toggling it changes the direction of that lexical sort each time). 
However, you can use "g" (`revert-buffer') to refresh and thus go 
right back to creation-order sort.  Also, if you change the value 
of `bookmark-sort-flag' and regenerate the buffer, via `g' or 
otherwise, the sort will change accordingly, as one would expect.

Note that there is no option to reverse the direction of creation 
order sort: when sorting by creation order, it's always most 
recent bookmark on top to least recent at bottom.  I didn't see 
any clear need for the reverse direction, and having the column 
sort toggle behave in a familiar (lexical) way seemed much more 
important.

Review / feedback welcome.  This is based against current 'master' 
branch (commit 25308a95f8869c), but once it's been through review 
I'll ask Eli which branch it should go on (or Eli if you want to 
just say now that's fine too).

Best regards,
-Karl

[1] That is, the buffer generated by `bookmark-bmenu-list'.


[-- Attachment #2: 0001-Improve-sorting-in-the-bookmark-list-buffer.patch --]
[-- Type: text/plain, Size: 6228 bytes --]

From dd203d5bd936c1771e068a14f4d24a3921be4396 Mon Sep 17 00:00:00 2001
From: Karl Fogel <kfogel@red-bean.com>
Date: Wed, 20 Apr 2022 12:46:26 -0500
Subject: [PATCH] Improve sorting in the bookmark list buffer

  - Ensure that the bookmark bmenu buffer sorts when it should.
  - Sort case-insensitively and by locale-dependent collation order.
  - Rename "Bookmark" column to "Bookmark Name".
  - Coordinate that column's sort toggle and `bookmark-sort-flag'.
  - Document the new behavior.

* lisp/bookmark.el (bookmark-bmenu--name-predicate,
bookmark-bmenu--type-predicate, bookmark-bmenu--file-predicate): Use
`string-collate-lessp' with IGNORE-CASE argument, instead of plain
`string<'.
(bookmark-bmenu--revert): Sort based on `bookmark-sort-flag'.
(bookmark-bmenu-mode): Document the new behavior.   Rename the
"Bookmark" column to "Bookmark Name" for clarity & documentabilty.
---
 lisp/bookmark.el | 53 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git lisp/bookmark.el lisp/bookmark.el
index 31876c83a2..6c46268a34 100644
--- lisp/bookmark.el
+++ lisp/bookmark.el
@@ -1824,7 +1824,29 @@ bookmark-bmenu--revert
                        (list location))])
               entries)))
     (tabulated-list-init-header)
-    (setq tabulated-list-entries (reverse entries)))
+    ;; The value of `bookmark-sort-flag' might have changed since the
+    ;; last time the buffer contents were generated, so re-check it.
+    (if bookmark-sort-flag
+        (progn
+          (setq tabulated-list-sort-key '("Bookmark Name" . nil))
+          (setq tabulated-list-entries entries))
+      (setq tabulated-list-sort-key nil)
+      ;; And since we're not sorting by bookmark name, show bookmarks
+      ;; according to order of creation, with the most recently
+      ;; created bookmarks at the top and the least recently created
+      ;; at the bottom.
+      ;;
+      ;; Note that clicking the column sort toggle for the bookmark
+      ;; name column will invoke the `tabulated-list-mode' sort, which
+      ;; uses `bookmark-bmenu--name-predicate' to sort lexically by
+      ;; bookmark name instead of by (reverse) creation order.
+      ;; Clicking the toggle again will reverse the lexical sort, but
+      ;; the sort will still be lexical not creation-order.  However,
+      ;; if the user reverts the buffer, then the above check of
+      ;; `bookmark-sort-flag' will happen again and the buffer will
+      ;; go back to a creation-order sort.  This is all expected
+      ;; behavior, as documented in `bookmark-bmenu-mode'.
+      (setq tabulated-list-entries (reverse entries))))
   (tabulated-list-print t))
 
 ;;;###autoload
@@ -1868,6 +1890,18 @@ bookmark-bmenu-mode
 Each line describes one of the bookmarks in Emacs.
 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.
+
 \\<bookmark-bmenu-mode-map>
 \\[bookmark-bmenu-mark] -- mark bookmark to be displayed.
 \\[bookmark-bmenu-mark-all] -- mark all listed bookmarks to be displayed.
@@ -1900,20 +1934,23 @@ 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.
-\\[bookmark-bmenu-search] -- incrementally search for bookmarks."
+\\[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)."
   (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)
+          ("Bookmark Name"
+           ,bookmark-bmenu-file-column bookmark-bmenu--name-predicate)
           ("Type" 8 bookmark-bmenu--type-predicate)
           ,@(if bookmark-bmenu-toggle-filenames
                 '(("File" 0 bookmark-bmenu--file-predicate)))])
   (setq tabulated-list-padding bookmark-bmenu-marks-width)
   (when bookmark-sort-flag
-    (setq tabulated-list-sort-key '("Bookmark" . nil)))
+    (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))
@@ -1922,17 +1959,19 @@ bookmark-bmenu-mode
 (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)))
+  (string-collate-lessp (caar a) (caar b) nil t))
 
 (defun bookmark-bmenu--type-predicate (a b)
   "Predicate to sort \"*Bookmark List*\" buffer by the type column.
 This is used for `tabulated-list-format' in `bookmark-bmenu-mode'."
-  (string< (elt (cadr a) 2) (elt (cadr b) 2)))
+  (string-collate-lessp (elt (cadr a) 2) (elt (cadr b) 2) nil t))
 
 (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))))
+  (string-collate-lessp (bookmark-location (car a))
+                        (bookmark-location (car b))
+                        nil t))
 
 
 (defun bookmark-bmenu-toggle-filenames (&optional show)
-- 
2.35.1

Re: [PATCH] Improve sorting in the bookmark list buffer.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Wed, 20 Apr 2022 13:09:26 -0500
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>
> 
> Review / feedback welcome.  This is based against current 'master' 
> branch (commit 25308a95f8869c), but once it's been through review 
> I'll ask Eli which branch it should go on (or Eli if you want to 
> just say now that's fine too).

I think this should go to master, since this isn't a recent
regression, right?

Re: [PATCH] Improve sorting in the bookmark list buffer.

[Karl Fogel]

On 20 Apr 2022, Eli Zaretskii wrote:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Date: Wed, 20 Apr 2022 13:09:26 -0500
>> Cc: Manuel Giraud <manuel@ledu-giraud.fr>
>> 
>> Review / feedback welcome.  This is based against current 
>> 'master' 
>> branch (commit 25308a95f8869c), but once it's been through 
>> review 
>> I'll ask Eli which branch it should go on (or Eli if you want 
>> to 
>> just say now that's fine too).
>
>I think this should go to master, since this isn't a recent
>regression, right?

That's right, it's not a regression.  The bugs that this fixes 
have been present for a long time.

Best regards,
-Karl

Re: [PATCH] Improve sorting in the bookmark list buffer.

[Karl Fogel]

On 20 Apr 2022, Eli Zaretskii wrote:
>I think this should go to master, since this isn't a recent
>regression, right?

And I replied:
>That's right, it's not a regression.  The bugs that this fixes 
>have been present for a long time. 

Shall I put it on 'master'?

While I'd like to hear from Manuel, I don't want to block on that, 
and I've tested the patch pretty thoroughly.  Based on our 
previous correspondence, it sounds like 'master' is the right 
destination.

Best regards,
-Karl

Re: [PATCH] Improve sorting in the bookmark list buffer.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: emacs-devel@gnu.org,  manuel@ledu-giraud.fr
> Date: Sun, 24 Apr 2022 12:30:34 -0500
> 
> On 20 Apr 2022, Eli Zaretskii wrote:
> >I think this should go to master, since this isn't a recent
> >regression, right?
> 
> And I replied:
> >That's right, it's not a regression.  The bugs that this fixes 
> >have been present for a long time. 
> 
> Shall I put it on 'master'?

Yes, please.

Re: [PATCH] Improve sorting in the bookmark list buffer.

[Karl Fogel]

On 24 Apr 2022, Eli Zaretskii wrote:
>> Shall I put it on 'master'?
>
>Yes, please.

Done; commit 8b071c77b0d7.

Thanks,
-Karl

Re: [PATCH] Improve sorting in the bookmark list buffer.

[Manuel Giraud]

Hi Karl,

I'm late to the party. I've just tested the new behaviour from master
and it works exactly as you have described.

There is one minor thing left though: when you have used one header to
change sorting and then hit 'g', the content order is reverted but the
selected header is still bold and with its down (or up) arrow. You have
to hit 'g' one more time to also revert this state.
-- 
Manuel Giraud

Re: [PATCH] Improve sorting in the bookmark list buffer.

[Karl Fogel]

On 25 Apr 2022, Manuel Giraud wrote:
>I'm late to the party. I've just tested the new behaviour from 
>master
>and it works exactly as you have described.
>
>There is one minor thing left though: when you have used one 
>header to
>change sorting and then hit 'g', the content order is reverted 
>but the
>selected header is still bold and with its down (or up) 
>arrow. You have
>to hit 'g' one more time to also revert this state.

Heh, I'm so nonvisually-oriented that I never even noticed. 
Thanks, Manuel -- fixed just now in commit 42366383c6327e.

Best regards,
-Karl

Re: [PATCH] Improve sorting in the bookmark list buffer.

[Manuel Giraud]

Thanks, it works as expected.
-- 
Manuel Giraud

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Eli Zaretskii]

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: Manuel Giraud <manuel@ledu-giraud.fr>,  emacs-devel@gnu.org
> Date: Sun, 06 Mar 2022 21:32:49 -0600
> 
> --- lisp/bookmark.el
> +++ lisp/bookmark.el
> @@ -246,11 +246,13 @@ bookmark-alist
>  Bookmark functions update the value automatically.
>  You probably do NOT want to change the value yourself.
>  
> -The value is an alist with bookmarks of the form
> +The value is an alist whose elements are the form
                               ^^^^^^^^^^^^^^^^^^^^^              
"...elements are of the form", right?

>  
>   (BOOKMARK-NAME . PARAM-ALIST)
>  
> -or the deprecated form (BOOKMARK-NAME PARAM-ALIST).
> +or the deprecated form (BOOKMARK-NAME PARAM-ALIST).  The alist is
> +ordered from most recently created bookmark at the front to least
> +recently created bookmark at the end.
>  
>  BOOKMARK-NAME is the name you gave to the bookmark when creating it.
>  
> @@ -583,10 +585,10 @@ bookmark-store
>            ;; Modify using the new (NAME . ALIST) format.
>            (setcdr bm alist))
>  
> -      ;; otherwise just cons it onto the front (either the bookmark
> -      ;; doesn't exist already, or there is no prefix arg.  In either
> -      ;; case, we want the new bookmark consed onto the alist...)
> -
> +      ;; Otherwise just put it onto the front of the list.  Either the
> +      ;; bookmark doesn't exist already, or there is no prefix arg.
> +      ;; In either case, we want the new bookmark on the front of the
> +      ;; list, since the list is kept in reverse order of creation.
>        (push (cons stripped-name alist) bookmark-alist))
>  
>      ;; Added by db
> @@ -1140,7 +1142,9 @@ bookmark-maybe-load-default-file
>  
>  (defun bookmark-maybe-sort-alist ()
>    "Return `bookmark-alist' for display.
> -If `bookmark-sort-flag' is non-nil, then return a sorted copy of the alist."
> +If `bookmark-sort-flag' is non-nil, then return a sorted copy of the alist.
> +Otherwise, just return `bookmark-alist', which by default is ordered
> +from most recently created to least recently created bookmark."
>    (if bookmark-sort-flag
>        (sort (copy-alist bookmark-alist)
>              (lambda (x y) (string-lessp (car x) (car y))))

These are documentation changes, so if you like, you can install them
on the emacs-28 branch.

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Sun, 6 Mar 2022 21:20:34 -0600
> Subject: [PATCH] Ensure bookmark bmenu buffer sorts when it should
> 
> * lisp/bookmark.el (bookmark-bmenu--revert): Check `bookmark-sort-flag'
>   every time when displaying bookmarks.
> ---
>  lisp/bookmark.el | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git lisp/bookmark.el lisp/bookmark.el
> index 525bc9dc15..6c72621bc3 100644
> --- lisp/bookmark.el
> +++ lisp/bookmark.el
> @@ -1823,7 +1823,15 @@ bookmark-bmenu--revert
>                         (list location))])
>                entries)))
>      (tabulated-list-init-header)
> -    (setq tabulated-list-entries (reverse entries)))
> +    ;; The value of `bookmark-sort-flag' might have changed since the
> +    ;; last time the buffer contents were generated, so re-check it.
> +    (if bookmark-sort-flag
> +        (setq tabulated-list-sort-key '("Bookmark" . nil))
> +      (setq tabulated-list-sort-key nil)
> +      ;; And since we're not sorting by bookmark name, show bookmarks
> +      ;; in reverse order of creation: most recently created at the
> +      ;; top, least recently created at the bottom.
> +      (setq tabulated-list-entries (reverse entries))))
>    (tabulated-list-print t))

This one is still under debate (or at least incomplete), AFAIU.

Thanks.

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Karl Fogel]

On 07 Mar 2022, Eli Zaretskii wrote:
>> --- lisp/bookmark.el
>> +++ lisp/bookmark.el
>> @@ -246,11 +246,13 @@ bookmark-alist
>>  Bookmark functions update the value automatically.
>>  You probably do NOT want to change the value yourself.
>>  
>> -The value is an alist with bookmarks of the form
>> +The value is an alist whose elements are the form
>                               ^^^^^^^^^^^^^^^^^^^^^              
>"...elements are of the form", right?

Good catch!  Thank you.

>These are documentation changes, so if you like, you can install 
>them
>on the emacs-28 branch.

Will do (with the above correction).

>> From: Karl Fogel <kfogel@red-bean.com>
>> Date: Sun, 6 Mar 2022 21:20:34 -0600
>> Subject: [PATCH] Ensure bookmark bmenu buffer sorts when it 
>> should
>> 
>> * lisp/bookmark.el (bookmark-bmenu--revert): Check 
>> `bookmark-sort-flag'
>>   every time when displaying bookmarks.
>>
>> [...]

>This one is still under debate (or at least incomplete), AFAIU.

Yes; I'll repost another patch addressing Manuel's point about the 
tabulated-list bar (thanks, Manuel), and then we can decide where 
to put it.

Best regards,
-Karl

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Karl Fogel]

>>Thanks.
>>
>>Let's install this on master.  We can consider backporting to the
>>emacs-28 branch after Emacs 28.1 is released.
>
>Installed in commit a6abd06c73.
>
>[...]

Ah, make that commit 29157a9f88c (another commit snuck in before 
my push finished).

Re: [PATCH] Fix bookmark-bmenu-list sorting.

[Manuel Giraud]

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> However, if this is a bug, please tell:
>
>   . what is the bug and how to reproduce it?

The bug goes as follow:

(setq bookmark-sort-flag nil)
M-x bookmark-bmenu-list (C-x r l)

The bookmark list appears sorted alphabetically by bookmark name while
the `bookmark-sort-flag' documentation says:

    Non-nil means that bookmarks will be displayed sorted by bookmark
    name.  Otherwise they will be displayed in LIFO order (that is, most
    recently set ones come first, oldest ones come last)."
-- 
Manuel Giraud