[emacs bookmark.el] Sorting by last set

[emacs bookmark.el] Sorting by last set

[Manuel Giraud]

Hi,

I'd like to code a way to sort the bookmark list by the "last modified
date" (i.e. not the creation date of the bookmark but last time the
bookmark was modified).

AFAIU, the current order of the buffer list is always "last created
first" and there is no date associated with the modification.

In order to have this feature, I could modify the default order of the
bookmark list to "last modified first" or add a new field for each
record with the modification date? The former modifies the default order
of the bookmark list. The latter modifies the bookmark files format.

FWIW, I've tried doing the latter but I think I'd prefer the former as
it would work automatically with a 'bookmark-sort-flag' to nil.

What do you guys think?
-- 
Manuel Giraud

RE: [External] : [emacs bookmark.el] Sorting by last set

[Drew Adams]

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

> I'd like to code a way to sort the bookmark list by the "last modified
> date" (i.e. not the creation date of the bookmark but last time the
> bookmark was modified).
> 
> AFAIU, the current order of the buffer list is always "last created
> first" and there is no date associated with the modification.
> 
> In order to have this feature, I could modify the default order of the
> bookmark list to "last modified first" or add a new field for each
> record with the modification date? The former modifies the default
> order of the bookmark list. The latter modifies the bookmark files format.
> 
> FWIW, I've tried doing the latter but I think I'd prefer the former as
> it would work automatically with a 'bookmark-sort-flag' to nil.
> 
> What do you guys think?

1. Please don't change the bookmark-file format.

2. You can find code for sorting bookmarks (in
   many ways) in Bookmark+.  See attached
   screenshot for the predefined ways.

Sorting bookmarks with Bookmark+:

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

 You can easily define your own sorting
 commands and sort orders.  See macro
 `bmkp-define-sort-command' and the
 doc for option `bmkp-sort-comparer'.
___


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



[-- Attachment #2: throw-bmk+-Sort-menu.png --]
[-- Type: image/png, Size: 48653 bytes --]

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

>> What do you guys think?
>
> 1. Please don't change the bookmark-file format.

Yes I've seen this comment in bookmark.el. It seems that it should not
be taken lightly. So this rules out adding a "modified date" field (my
2nd option) but maybe my first option (sorting 'bookmark-alist' in last
modified order by default) is still possible.

> 2. You can find code for sorting bookmarks (in
>    many ways) in Bookmark+.  See attached
>    screenshot for the predefined ways.

Why not. I'm just using the bundled bookmark.el but maybe I should use a
package for such an advance feature.
-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Yes I've seen this comment in bookmark.el. It seems that it should not
> be taken lightly. So this rules out adding a "modified date" field (my
> 2nd option) but maybe my first option (sorting 'bookmark-alist' in last
> modified order by default) is still possible.

Extending the bookmark format (by adding more fields) is totally possible.

I think sorting by last set sounds like a nice feature -- patches
welcome.

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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Karl Fogel]

On 24 May 2022, Lars Ingebrigtsen wrote:
>Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> Yes I've seen this comment in bookmark.el. It seems that it 
>> should not
>> be taken lightly. So this rules out adding a "modified date" 
>> field (my
>> 2nd option) but maybe my first option (sorting 'bookmark-alist' 
>> in last
>> modified order by default) is still possible.
>
>Extending the bookmark format (by adding more fields) is totally 
>possible.
>
>I think sorting by last set sounds like a nice feature -- patches
>welcome.

I agree.  Extending the format to add fields like 'created-date' 
and/or 'last-modified' date seems fine to me, to support features 
like this.

In fact, when I was reviewing and applying Manuel's recent changes 
to toggle sorting in the bookmark menu buffer, I was already 
thinking that this would all be easier if the bookmark itself 
carried its creation date.

Drew, correct me if my guess is wrong, but does your objection 
(earlier in this thread) to extending the bookmark file format 
stem from the fact that long ago there was at least one -- maybe 
more? -- format revision that was not done in a compatible way? 
(I think it may have been my fault, too, but it's too long ago for 
me to remember clearly.)

Anyway, we fixed things and nowadays the format is quite easily 
extensible in a backwards-compatible way.  The PARAM-ALIST 
component of each bookmark object is just an alist with a known 
set key/value pairs.  If we add a new pair, old versions of 
bookmark.el will just ignore it while new versions will make use 
of it.

If you have some other reason for objecting to extending the 
format, though, please say.

Best regards,
-Karl

RE: [External] : [emacs bookmark.el] Sorting by last set

[Drew Adams]

> >> Yes I've seen this comment in bookmark.el. It seems that it
> >> should not be taken lightly. So this rules out adding a
> >> "modified date" field (my 2nd option) but maybe my first
> >> option (sorting 'bookmark-alist' in last modified order by 
> >> default) is still possible.
> >
> >Extending the bookmark format (by adding more fields) is totally
> >possible.
> >
> >I think sorting by last set sounds like a nice feature -- patches
> >welcome.
> 
> I agree.  Extending the format to add fields like 'created-date'
> and/or 'last-modified' date seems fine to me, to support features
> like this.
> 
> In fact, when I was reviewing and applying Manuel's recent changes
> to toggle sorting in the bookmark menu buffer, I was already
> thinking that this would all be easier if the bookmark itself
> carried its creation date.
> 
> Drew, correct me if my guess is wrong, but does your objection
> (earlier in this thread) to extending the bookmark file format
> stem from the fact that long ago there was at least one -- maybe
> more? -- format revision that was not done in a compatible way?
> (I think it may have been my fault, too, but it's too long ago for
> me to remember clearly.)
> 
> Anyway, we fixed things and nowadays the format is quite easily
> extensible in a backwards-compatible way.  The PARAM-ALIST
> component of each bookmark object is just an alist with a known
> set key/value pairs.  If we add a new pair, old versions of
> bookmark.el will just ignore it while new versions will make use
> of it.
> 
> If you have some other reason for objecting to extending the
> format, though, please say.


Anyone (app, user), anywhere, can of course add
any fields, for any kind of bookmark.

I'd rather that nothing depend on the existence
of some new "standard" fields.  Adding "standard"
fields is likely to result in such dependencies
or expectations.  That's my concern.

I don't feel strongly about this.  But in general,
I expect that the fewer "standard" fields vanilla
Emacs adds, the less chance of breakage.
___

That said, nothing I say will change anything,
I expect.  And if such a dependency breaks
something then I'll just have to deal with that
when it happens.
___

As for this particular proposed addition: I
haven't seen the need for a last-modified field,
and I've never had anyone ask for it or point
out its utility.

I don't recall whether I at one point considered
it.  But yes, such a field is not uncommon for
metadata of various sorts (e.g. file attributes).

But was there any particular use case mentioned?
Was it requested to solve some problem?  If so,
I missed that.

The request seemed to be only to be able to sort
by such a field.  How about a reason why such a
sort is requested/useful?  I'm not claiming it
can't be useful; I'm asking what its use is.

I can imagine it might sometime be useful to
know when a particular bookmark was last
modified.  But that would likely be for some
forensic reason (for me): when/why/how was that
bookmark last changed?  A bit like checking a
history when looking for the cause of a problem.

But sorting all bookmarks by last-modified time?
How is that useful?  (Again, just curious; not
claiming it's not useful.)
___

What I do know is useful, however, is the time
(date) of last visit/access (as well as time of
creation, of course).  That's related to the
number of times a bookmark has been visited
(accessed).

Sorting by visit time is like seeing a recentf
list.  Sorting by number of visits is like 
seeing your favorites list.  I often want to
sort these ways.
___

Now you may say that `ls' sorts by last mod
time as one of its main sort orders.  And I
agree that that order is very useful.  I'm
not (yet) convinced the same is true for
bookmarks, but I guess there's some similarity.

(Even for files, I think that most of my uses
of `ls' sorting by date would be handled just
as well if the date were last access instead
of last mod.)
___

Bookmark+ has a `created' field, for the time
(date) of creation.  I'd of course prefer, if
you add this, that you use the same field
name: `created' instead of `created-date'.
(One less thing to work around.)
___

Bookmark+'s version of `C-h v bookmark-alist':


Current list of bookmarks (bookmark records).
Bookmark functions update the value automatically.
You probably do not want to change the value yourself.

The value is an alist with entries of the form
 (BOOKMARK-NAME . PARAM-ALIST)
or the deprecated form (BOOKMARK-NAME PARAM-ALIST).

 BOOKMARK-NAME is the name you gave to the bookmark when creating it.
 PARAM-ALIST is an alist of bookmark data.  The order of the entries
  in PARAM-ALIST is not important.  The possible entries are described
  below.

Bookmarks created using vanilla Emacs (`bookmark.el'):

 (filename . FILENAME)
 (location . LOCATION)
 (position . POS)
 (front-context-string . STR-AFTER-POS)
 (rear-context-string  . STR-BEFORE-POS)
 (handler . HANDLER)
 (annotation . ANNOTATION)

 FILENAME names the bookmarked file.
 LOCATION names the bookmarked file, URL, or other place (Emacs 23+).
  FILENAME or LOCATION is what is shown in the bookmark list
  (`C-x r l') when you use `M-t'.
 POS is the bookmarked buffer position (position in the file).
 STR-AFTER-POS is buffer text that immediately follows POS.
 STR-BEFORE-POS is buffer text that immediately precedes POS.
 ANNOTATION is a string that you can provide to identify the bookmark.
  See options `bookmark-use-annotations' and
  `bookmark-automatically-show-annotations'.
 HANDLER is a function that provides the bookmark-jump behavior
  for a specific kind of bookmark.  This is the case for Info
  bookmarks, for instance (starting with Emacs 23).

Bookmarks created using Bookmark+ are the same as for vanilla Emacs,
except for the following differences.

1. Time of creation is recorded when you create a new bookmark:

 (created . CREATION-TIME)

 CREATION-TIME is an Emacs time representation, returned by function
 `current-time'.

2. Visit information is recorded, using entries `visits' and `time':

 (visits . NUMBER-OF-VISITS)
 (time . TIME-LAST-VISITED)

 NUMBER-OF-VISITS is a whole-number counter.

 TIME-LAST-VISITED is an Emacs time representation, returned by
 `current-time'.

3. The buffer name is recorded, using entry `buffer-name'.  It need
not be associated with a file.

4. If no file is associated with the bookmark, then FILENAME is
   `   - no file -'.

5. Bookmarks can be tagged by users.  The tag information is recorded
using entry `tags':

 (tags . TAGS-ALIST)

 TAGS-ALIST is an alist with string keys.

6. A bookmark can be simply a wrapper for a file, in which case it has
entry `file-handler' instead of `handler'.  When you "jump" to such
a bookmark, the `file-handler' function or shell-command is applied to
the `filename' entry.  Any `handler' entry present is ignored, as are
entries such as `position'.  It is only the target file that is
important.

7. Bookmarks can have individual highlighting, provided by users.
This overrides any default highlighting.

 (lighting . HIGHLIGHTING)

 HIGHLIGHTING is a property list that contain any of these keyword
 pairs:

   `:style' - Highlighting style.  Cdrs of `bmkp-light-styles-alist'
              entries are the possible values.
   `:face'  - Highlighting face, a symbol.
   `:when'  - A sexp to be evaluated.  Return value of `:no-light'
              means do not highlight.

8. The following additional entries are used to record region
information.  When a region is bookmarked, POS represents the region
start position.

 (end-position . END-POS)
 (front-context-region-string . STR-BEFORE-END-POS)
 (rear-context-region-string . STR-AFTER-END-POS))

 END-POS is the region end position.
 STR-BEFORE-END-POS is buffer text that precedes END-POS.
 STR-AFTER-END-POS is buffer text that follows END-POS.

The two context region strings are non-nil only when a region is
bookmarked.

 NOTE: The relative locations of `front-context-region-string' and
 `rear-context-region-string' are reversed from those of
 `front-context-string' and `rear-context-string'.  For example,
 `front-context-string' is the text that *follows* `position', but
 `front-context-region-string' *precedes* `end-position'.

9. The following additional entries are used for a Dired bookmark.

 (dired-marked . MARKED-FILES)
 (dired-subdirs . INSERTED-SUBDIRS)
 (dired-hidden-dirs . HIDDEN-SUBDIRS)
 (dired-switches . SWITCHES)

 MARKED-FILES is the list of files that were marked `*'.
 INSERTED-SUBDIRS is the list of subdirectores that were inserted.
 HIDDEN-SUBDIRS is the list of inserted subdirs that were hidden.
 SWITCHES is the string of `dired-listing-switches'.

10. The following additional entries are used for a Gnus bookmark.

 (group . GNUS-GROUP-NAME)
 (article . GNUS-ARTICLE-NUMBER)
 (message-id . GNUS-MESSAGE-ID)

 GNUS-GROUP-NAME is the name of a Gnus group.
 GNUS-ARTICLE-NUMBER is the number of a Gnus article.
 GNUS-MESSAGE-ID is the identifier of a Gnus message.

11. For a URL bookmark, FILENAME or LOCATION is a URL.

12. A sequence bookmark has this additional entry:

 (sequence . COMPONENT-BOOKMARKS)

 COMPONENT-BOOKMARKS is the list of component bookmark names.

13. A function bookmark has this additional entry, which records the
FUNCTION:

 (function . FUNCTION)

14. A bookmark-list bookmark has this additional entry, which records
the state of buffer `*Bookmark List*' at the time it is created:

 (bookmark-list . STATE)

 STATE records the sort order, filter function, omit list, and title.
___

HTH.

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

[...]

> But sorting all bookmarks by last-modified time?
> How is that useful?  (Again, just curious; not
> claiming it's not useful.)

My use case would be, when calling 'bookmark-bmenu-list', to have the
last modified bookmarks at the top of the list. Why? Because then it
looks like the stack of books you have next to your bed: at the top is
the one you are most likely currently reading so you just pick it
up. But you are right that for the rest of the stack that might be less
useful.

> What I do know is useful, however, is the time
> (date) of last visit/access (as well as time of
> creation, of course).  That's related to the
> number of times a bookmark has been visited
> (accessed).
>
> Sorting by visit time is like seeing a recentf
> list.  Sorting by number of visits is like 
> seeing your favorites list.  I often want to
> sort these ways.

Yes maybe that is what I want.
-- 
Manuel Giraud

RE: [External] : [emacs bookmark.el] Sorting by last set

[Drew Adams]

> > But sorting all bookmarks by last-modified time?
> > How is that useful?  (Again, just curious; not
> > claiming it's not useful.)
> 
> My use case would be, when calling 'bookmark-bmenu-list', to have the
> last modified bookmarks at the top of the list. Why? Because then it
> looks like the stack of books you have next to your bed: at the top is
> the one you are most likely currently reading so you just pick it
> up. But you are right that for the rest of the stack that might be less
> useful.

Thanks very much for making this clear.

I think this use case is handled well by a
last-accessed (aka visited) field.  The one used
by Bookmark+ is called `time'.  It could have
been called `accessed' or `used', but I think the
"time" of a bookmark is pretty clearly the last
time it was used/visited/accessed, and a name like
`accessed' or `used' could suggest the number of
times it was accessed/used, rather than the time.
(Bookmark+ uses field `visits' for that info.)

> > What I do know is useful, however, is the time
> > (date) of last visit/access (as well as time of
> > creation, of course).  That's related to the
> > number of times a bookmark has been visited
> > (accessed).
> >
> > Sorting by visit time is like seeing a recentf
> > list.  Sorting by number of visits is like
> > seeing your favorites list.  I often want to
> > sort these ways.
> 
> Yes maybe that is what I want.

Great.  Then if vanilla Emacs adds field `time',
and assuming it uses it in the same or a similar
or compatible way as Bookmark+, I'll have nothing
to be concerned about, I expect. 

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

[...]

> I think sorting by last set sounds like a nice feature -- patches
> welcome.

Here is a patch that does just that.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-sort-bookmark-alist-in-last-modified-order-by-defaul.patch --]
[-- Type: text/x-patch, Size: 5455 bytes --]

From 85834e973413b4ab050923d1dca4f0b4341d9b04 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Wed, 25 May 2022 15:13:17 +0200
Subject: [PATCH] sort `bookmark-alist' in last modified order by default.

---
 lisp/bookmark.el | 53 +++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index c604395dd7..b56ad4fa67 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -251,8 +251,8 @@ bookmark-alist
  (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.
+ordered from most recently modified bookmark at the front to
+least recently modified bookmark at the end.
 
 BOOKMARK-NAME is the name you gave to the bookmark when creating it.
 
@@ -574,22 +574,18 @@ bookmark-store
   (bookmark-maybe-load-default-file)
   (let ((stripped-name (copy-sequence name)))
     (set-text-properties 0 (length stripped-name) nil stripped-name)
-    (if (and (not no-overwrite)
-             (bookmark-get-bookmark stripped-name 'noerror))
-        ;; Already existing bookmark under that name and
-        ;; no prefix arg means just overwrite old bookmark.
-        (let ((bm (bookmark-get-bookmark stripped-name)))
-          ;; First clean up if previously location was fontified.
-          (when bookmark-set-fringe-mark
-            (bookmark--remove-fringe-mark bm))
-          ;; Modify using the new (NAME . ALIST) format.
-          (setcdr bm 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))
+
+    ;; Already existing bookmark under that name and no prefix arg
+    ;; means just overwrite old bookmark.  First remove it.
+    (let ((bm (bookmark-get-bookmark stripped-name 'noerror)))
+      (when (and (not no-overwrite) bm)
+        ;; First clean up if previously location was fontified.
+        (when bookmark-set-fringe-mark
+          (bookmark--remove-fringe-mark bm))
+        (setq bookmark-alist (delq bm bookmark-alist))))
+
+    ;; Put the new (or overwritten) bookmark onto the front of the list.
+    (push (cons stripped-name alist) bookmark-alist)
 
     ;; Added by db
     (setq bookmark-current-bookmark stripped-name)
@@ -1144,7 +1140,7 @@ bookmark-maybe-sort-alist
   "Return `bookmark-alist' for display.
 If `bookmark-sort-flag' is non-nil, then return a sorted copy of the alist.
 Otherwise, just return `bookmark-alist', which by default is ordered
-from most recently created to least recently created bookmark."
+from most recently modified to least recently modified bookmark."
   (if bookmark-sort-flag
       (sort (copy-alist bookmark-alist)
             (lambda (x y) (string-lessp (car x) (car y))))
@@ -1831,19 +1827,19 @@ bookmark-bmenu--revert
           (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
+      ;; according to order of modification, 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.
+      ;; bookmark name instead of by (reverse) modification 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
+      ;; the sort will still be lexical not modification-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 modification-order sort.  This is all expected
       ;; behavior, as documented in `bookmark-bmenu-mode'.
       (setq tabulated-list-entries (reverse entries)))
     ;; Generate the header only after `tabulated-list-sort-key' is
@@ -1901,10 +1897,11 @@ bookmark-bmenu-mode
 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.
+modification order, with most recently modified 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.
+thereafter toggle the direction of) lexical sorting by bookmark
+name.  At any time you may use \\[revert-buffer] to go back to
+sorting by modification order.
 
 \\<bookmark-bmenu-mode-map>
 \\[bookmark-bmenu-mark] -- mark bookmark to be displayed.
-- 
2.36.0


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

-- 
Manuel Giraud

RE: [External] : [emacs bookmark.el] Sorting by last set

[Drew Adams]

> > I think sorting by last set sounds like a nice
> > feature -- patches welcome.
> 
> Here is a patch that does just that.

I took only a brief look.  Unless I'm mistaken
this changes the order of `bookmark-list', and
it does so each time any bookmark is changed.

Being able to sort the _displayed_ bookmark list
(`*Bookmark List*') is one thing.  Sorting
`bookmark-alist' is quite another.

And sorting `bookmark-alist' and modifying it to
change to the new sort order is yet another.

And doing that each time a bookmark is set is yet
another.

I am not at all in favor of changing the order of
`bookmark-alist' like this.

Not mainly because it would be costly to re-sort
the list each time an existing bookmark is updated.
But in particular because `bookmark-alist' is used
in more ways than just display/access in `*Bookmark
List*'.

IMO this is the wrong solution for the request you
made, which was to be able to make more recently
changed bookmarks more apparent.

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

[...]

> Not mainly because it would be costly to re-sort
> the list each time an existing bookmark is updated.
> But in particular because `bookmark-alist' is used
> in more ways than just display/access in `*Bookmark
> List*'.

That, I was not aware of. So then sorting `bookmark-alist' (differently)
might not be an option.

> IMO this is the wrong solution for the request you
> made, which was to be able to make more recently
> changed bookmarks more apparent.

FWIW, it works as I intended: each I set a bookmark (a new one or an
existing one with the same default name) it ends up at the top of
*Bookmark List* with `bookmark-sort-flag' set to nil.

I'm not against adding a "time" field (as you proposed in your other
mail) but, as it is, bookmark.el just have the `bookmark-sort-flag'
toggle for which t is «sort by name» and nil is «use the default
`bookmark-alist' order».

So I'd have to piggyback on `bookmark-sort-flag' to have the order I
want by default. (… hum, why not after all … `bookmark-sort-flag' could
be t, nil or 'time without disturbing current default or users'
setting).
-- 
Manuel Giraud

RE: [External] : [emacs bookmark.el] Sorting by last set

[Drew Adams]

> > Not mainly because it would be costly to re-sort
> > the list each time an existing bookmark is updated.
> > But in particular because `bookmark-alist' is used
> > in more ways than just display/access in `*Bookmark
> > List*'.
> 
> That, I was not aware of. So then sorting `bookmark-alist'
> (differently) might not be an option.

If what you want is to sort the displayed list,
buffer `*Bookmark List*', then that's what you
should sort.  There's no reason to sort
`bookmark-alist' in order to accomplish that.

What's displayed in `*Bookmark List*' need not
even be all of the bookmarks in `bookmark-alist';
some can be intentionally omitted (filtered out).

Beyond displaying its bookmarks in `*Bookmark
List*', there can be any number of other ways
to make use of `bookmark-alist', including
other interactive ways such as choosing a
bookmark with `bookmark-completing-read'.

And yes, sorting can be useful also for some
such uses. But even then it's generally wiser
to sort a copy of `bookmark-alist', rather than
modify that list.

> > IMO this is the wrong solution for the request you
> > made, which was to be able to make more recently
> > changed bookmarks more apparent.
> 
> FWIW, it works as I intended: each I set a bookmark (a new one or an
> existing one with the same default name) it ends up at the top of
> *Bookmark List* with `bookmark-sort-flag' set to nil.

Yes, of course.  If you sort `bookmark-alist',
modifying it, then you'll see it sorted. ;-)

> I'm not against adding a "time" field (as you proposed in your other
> mail) but, as it is, bookmark.el just have the `bookmark-sort-flag'
> toggle for which t is «sort by name» and nil is «use the default
> `bookmark-alist' order».
> 
> So I'd have to piggyback on `bookmark-sort-flag' to have the order I
> want by default. (… hum, why not after all … `bookmark-sort-flag' could
> be t, nil or 'time without disturbing current default or users'
> setting).

IMHO, it was a mistake for `bookmark.el' to have
moved its `*Bookmark List*' display to depend on
the straitjacket that is `tabulated-list-mode'.

That limits things quite a bit, including sorting.
Far better to have `bookmark-bmenu-list' display
the listing itself, as was the case in the past,
and as is still the case for Bookmark+.

In particular, without `tabulated-list-mode'
there are no limits on sorting or on users
defining their own sort functions.  It makes
little sense for sorting to be essentially
limited to sorting by columns.

Just because listings of things such as files
(Dired), buffers (Ibuffer), and bookmarks use
columns, and just because `tabulated-list-mode'
provides a rudimentary ability to list things
using columns, these alone are not reasons to
use `tabulated-list-mode'.

Thank goodness Dired and Ibuffer have so far
been left alone, remaining sane, flexible, and
powerful.  May their bountiful gardens be kept
free of indiscriminate lawn-mowing.  No army,
one-size-fits-all crew cuts for them, thank
you very much.
___

(Option `bookmark-sort-flag' is also limiting,
providing for only two sort orders.)

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

[...]

> IMHO, it was a mistake for `bookmark.el' to have
> moved its `*Bookmark List*' display to depend on
> the straitjacket that is `tabulated-list-mode'.
>
> That limits things quite a bit, including sorting.
> Far better to have `bookmark-bmenu-list' display
> the listing itself, as was the case in the past,
> and as is still the case for Bookmark+.
>
> In particular, without `tabulated-list-mode'
> there are no limits on sorting or on users
> defining their own sort functions.  It makes
> little sense for sorting to be essentially
> limited to sorting by columns.

From my limited experience on this patch, I agree with you. For
instance, in one version of the patch I thought it would be a good idea
to put the modified timestamp in a column. As my timestamps came from
(current-time) it was not human readable so I had to format-time-string
those. But then I have to be able to sort on this formatted string. So I
choose to format those timestamps with "%F %T" to be able to sort
lexicographically (or maybe I could have parse them back)… and at this
point, it feels like your fighting the system.
-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Karl Fogel]

On 25 May 2022, Manuel Giraud wrote:
>Drew Adams <drew.adams@oracle.com> writes:
>That, I was not aware of. So then sorting `bookmark-alist' 
>(differently)
>might not be an option.

Currently, the order of bookmarks in `bookmark-alist' is the only 
record of the order in which the bookmarks were created -- and 
that order is needed for some displayed sorts .  Since bookmarks 
up till now don't have anything like a creation-date element, we 
should just keep the list in creation order, so that this 
information is preserved.

I suppose we could in theory do fancier things, like detect when 
all the bookmarks have a creation date and abandon the need to 
keep the alist in order then, or make sure that just the subset of 
`bookmark-alist' that *doesn't* have creation dates is kept in 
creation order... but there is no need to be fancy.  Let's just 
keep the whole alist in order of creation, even after we add 
creation-date or other date-ish elements.

Best regards,
-Karl

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

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

> Let's just keep the whole alist in order of creation, even after we
> add creation-date or other date-ish elements.

Yes, I think that would be the best.

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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Karl Fogel <kfogel@red-bean.com> writes:
>
>> Let's just keep the whole alist in order of creation, even after we
>> add creation-date or other date-ish elements.
>
> Yes, I think that would be the best.

Ok. Strike 3.

This time I add a timestamp field to each bookmark upon setting it. I
also add the possibility to set `bookmark-sort-flag' to 'timestamp which
means sort the displayed bookmark list from most recently set to the
least recently set.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-add-a-timestamp-when-a-bookmark-is-set.patch --]
[-- Type: text/x-patch, Size: 8534 bytes --]

From 2124f0e77207ad7281ff19fa5533ceeb0719cfea Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Thu, 26 May 2022 18:25:52 +0200
Subject: [PATCH] add a timestamp when a bookmark is set.

add the 'timestamp option to `bookmark-sort-flag' to display bookmark
list sorted by those timestamp.
---
 etc/NEWS         |  5 +++
 lisp/bookmark.el | 92 ++++++++++++++++++++++++++++--------------------
 2 files changed, 58 insertions(+), 39 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 4ebaf6e07a..a93d917df0 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1673,6 +1673,11 @@ manual for more details.
 Types are registered via a 'bookmark-handler-type' symbol property on
 the jumping function.
 
++++
+*** 'bookmark-sort-flag' can now be set to 'timestamp.
+This will display bookmark list from most recently set to least
+recently set.
+
 ---
 *** New minor mode 'elide-head-mode'.
 Enabling this minor mode turns on hiding header material, like
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index c604395dd7..ce925df8fc 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -115,10 +115,14 @@ bookmark-completion-ignore-case
 
 
 (defcustom bookmark-sort-flag t
-  "Non-nil means that bookmarks will be displayed sorted by bookmark name.
+  "T means that bookmarks will be displayed sorted by bookmark name.
+TIMESTAMP means that bookmarks will be displayed sorted from most
+recently set to last recently set.
 Otherwise they will be displayed in LIFO order (that is, most
-recently set ones come first, oldest ones come last)."
-  :type 'boolean)
+recently created ones come first, oldest ones come last)."
+  :type '(choice (const :tag "By name" t)
+                 (const :tag "By modified time" timestamp)
+                 (const :tag "By creation time" nil)))
 
 
 (defcustom bookmark-menu-confirm-deletion nil
@@ -460,6 +464,10 @@ bookmark-get-handler
   "Return the handler function for BOOKMARK-NAME-OR-RECORD, or nil if none."
   (bookmark-prop-get bookmark-name-or-record 'handler))
 
+(defun bookmark-get-timestamp (bookmark-name-or-record)
+  "Return the timestamp for BOOKMARK-NAME-OR-RECORD, or nil if none."
+  (bookmark-prop-get bookmark-name-or-record 'timestamp))
+
 (defvar bookmark-history nil
   "The history list for bookmark functions.")
 
@@ -497,6 +505,21 @@ bookmark--remove-fringe-mark
               (when (eq 'bookmark (overlay-get temp 'category))
                 (delete-overlay (setq found temp))))))))))
 
+(defun bookmark-maybe-sort-alist ()
+  "Return `bookmark-alist' for display.
+If `bookmark-sort-flag' is T, then return a sorted by name copy of the alist.
+If `bookmark-sort-flag' is TIMESTAMP, then return a sorted by last modified
+copy of the alist.  Otherwise, just return `bookmark-alist', which by default
+is ordered from most recently created to least recently created bookmark."
+  (let ((copy (copy-alist bookmark-alist)))
+    (cond ((eq bookmark-sort-flag t)
+           (sort copy (lambda (x y) (string-lessp (car x) (car y)))))
+          ((eq bookmark-sort-flag 'timestamp)
+           (sort copy (lambda (x y)
+                        (time-less-p (bookmark-get-timestamp y)
+                                     (bookmark-get-timestamp x)))))
+          (t copy))))
+
 (defun bookmark-completing-read (prompt &optional default)
   "Prompting with PROMPT, read a bookmark name in completion.
 PROMPT will get a \": \" stuck on the end no matter what, so you
@@ -506,10 +529,8 @@ bookmark-completing-read
   (bookmark-maybe-load-default-file) ; paranoia
   (if (listp last-nonmenu-event)
       (bookmark-menu-popup-paned-menu t prompt
-				      (if bookmark-sort-flag
-					  (sort (bookmark-all-names)
-						'string-lessp)
-					(bookmark-all-names)))
+                                      (mapcar 'bookmark-name-from-full-record
+                                              (bookmark-maybe-sort-alist)))
     (let* ((completion-ignore-case bookmark-completion-ignore-case)
            (default (unless (equal "" default) default)))
       (completing-read (format-prompt prompt default)
@@ -630,7 +651,8 @@ bookmark-make-record-default
                                    (point)
                                    (- (point) bookmark-search-size))
                                   nil))))
-    (position . ,(or posn (point)))))
+    (position . ,(or posn (point)))
+    (timestamp . ,(current-time))))
 
 \f
 ;;; File format stuff
@@ -1140,15 +1162,6 @@ bookmark-maybe-load-default-file
                                   (car bookmark-bookmarks-timestamp)))))))
          (bookmark-load (car bookmark-bookmarks-timestamp) t t))))
 
-(defun bookmark-maybe-sort-alist ()
-  "Return `bookmark-alist' for display.
-If `bookmark-sort-flag' is non-nil, then return a sorted copy of the alist.
-Otherwise, just return `bookmark-alist', which by default is ordered
-from most recently created to least recently created bookmark."
-  (if bookmark-sort-flag
-      (sort (copy-alist bookmark-alist)
-            (lambda (x y) (string-lessp (car x) (car y))))
-    bookmark-alist))
 
 
 (defvar bookmark-after-jump-hook nil
@@ -1825,27 +1838,28 @@ bookmark-bmenu--revert
               entries)))
     ;; The value of `bookmark-sort-flag' might have changed since the
     ;; last time the buffer contents were generated, so re-check it.
-    (if bookmark-sort-flag
-        (progn
-          (setq tabulated-list-sort-key '("Bookmark Name" . nil))
-          (setq tabulated-list-entries entries))
-      (setq tabulated-list-sort-key nil)
-      ;; And since we're not sorting by bookmark name, show bookmarks
-      ;; according to order of creation, with the most recently
-      ;; created bookmarks at the top and the least recently created
-      ;; at the bottom.
-      ;;
-      ;; Note that clicking the column sort toggle for the bookmark
-      ;; name column will invoke the `tabulated-list-mode' sort, which
-      ;; uses `bookmark-bmenu--name-predicate' to sort lexically by
-      ;; bookmark name instead of by (reverse) creation order.
-      ;; Clicking the toggle again will reverse the lexical sort, but
-      ;; the sort will still be lexical not creation-order.  However,
-      ;; if the user reverts the buffer, then the above check of
-      ;; `bookmark-sort-flag' will happen again and the buffer will
-      ;; go back to a creation-order sort.  This is all expected
-      ;; behavior, as documented in `bookmark-bmenu-mode'.
-      (setq tabulated-list-entries (reverse entries)))
+    (cond ((eq bookmark-sort-flag t)
+           (setq tabulated-list-sort-key '("Bookmark Name" . nil)
+                 tabulated-list-entries entries))
+          ((or (null bookmark-sort-flag)
+               (eq bookmark-sort-flag 'timestamp))
+           (setq tabulated-list-sort-key nil)
+           ;; And since we're not sorting by bookmark name, show bookmarks
+           ;; according to order of creation, with the most recently
+           ;; created bookmarks at the top and the least recently created
+           ;; at the bottom.
+           ;;
+           ;; Note that clicking the column sort toggle for the bookmark
+           ;; name column will invoke the `tabulated-list-mode' sort, which
+           ;; uses `bookmark-bmenu--name-predicate' to sort lexically by
+           ;; bookmark name instead of by (reverse) creation order.
+           ;; Clicking the toggle again will reverse the lexical sort, but
+           ;; the sort will still be lexical not creation-order.  However,
+           ;; if the user reverts the buffer, then the above check of
+           ;; `bookmark-sort-flag' will happen again and the buffer will
+           ;; go back to a creation-order sort.  This is all expected
+           ;; behavior, as documented in `bookmark-bmenu-mode'.
+           (setq tabulated-list-entries (reverse entries))))
     ;; Generate the header only after `tabulated-list-sort-key' is
     ;; settled, because if that's non-nil then the sort-direction
     ;; indicator will be shown in the named column, but if it's
@@ -1953,7 +1967,7 @@ bookmark-bmenu-mode
           ,@(if bookmark-bmenu-toggle-filenames
                 '(("File" 0 bookmark-bmenu--file-predicate)))])
   (setq tabulated-list-padding bookmark-bmenu-marks-width)
-  (when bookmark-sort-flag
+  (when (eq bookmark-sort-flag t)
     (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)
-- 
2.36.0


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

-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Stefan Monnier]

Manuel Giraud [2022-05-26 18:42:19] wrote:
> This time I add a timestamp field to each bookmark upon setting it. I
> also add the possibility to set `bookmark-sort-flag' to 'timestamp which
> means sort the displayed bookmark list from most recently set to the
> least recently set.

"timestamp" doesn't say which time it's referring to.
I think we should call it "last-modified" or something like that, so
there's no ambiguity.


        Stefan

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Manuel Giraud [2022-05-26 18:42:19] wrote:
>> This time I add a timestamp field to each bookmark upon setting it. I
>> also add the possibility to set `bookmark-sort-flag' to 'timestamp which
>> means sort the displayed bookmark list from most recently set to the
>> least recently set.
>
> "timestamp" doesn't say which time it's referring to.
> I think we should call it "last-modified" or something like that, so
> there's no ambiguity.

Yes, that makes sense.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-add-a-last-modified-field-when-a-bookmark-is-set.patch --]
[-- Type: text/x-patch, Size: 8596 bytes --]

From cd0e1165c4eb5a45382317f73109cc2b46fd4d54 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Thu, 26 May 2022 18:25:52 +0200
Subject: [PATCH] add a last-modified field when a bookmark is set.

add the 'last-modified option to `bookmark-sort-flag' to display
bookmark list sorted by those timestamp.
---
 etc/NEWS         |  5 +++
 lisp/bookmark.el | 92 ++++++++++++++++++++++++++++--------------------
 2 files changed, 58 insertions(+), 39 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 4ebaf6e07a..fb5fafe5bd 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1673,6 +1673,11 @@ manual for more details.
 Types are registered via a 'bookmark-handler-type' symbol property on
 the jumping function.
 
++++
+*** 'bookmark-sort-flag' can now be set to 'last-modified.
+This will display bookmark list from most recently set to least
+recently set.
+
 ---
 *** New minor mode 'elide-head-mode'.
 Enabling this minor mode turns on hiding header material, like
diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index c604395dd7..5460a9a297 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -115,10 +115,14 @@ bookmark-completion-ignore-case
 
 
 (defcustom bookmark-sort-flag t
-  "Non-nil means that bookmarks will be displayed sorted by bookmark name.
+  "T means that bookmarks will be displayed sorted by bookmark name.
+LAST-MODIFIED means that bookmarks will be displayed sorted from most
+recently set to last recently set.
 Otherwise they will be displayed in LIFO order (that is, most
-recently set ones come first, oldest ones come last)."
-  :type 'boolean)
+recently created ones come first, oldest ones come last)."
+  :type '(choice (const :tag "By name" t)
+                 (const :tag "By modified time" last-modified)
+                 (const :tag "By creation time" nil)))
 
 
 (defcustom bookmark-menu-confirm-deletion nil
@@ -460,6 +464,10 @@ bookmark-get-handler
   "Return the handler function for BOOKMARK-NAME-OR-RECORD, or nil if none."
   (bookmark-prop-get bookmark-name-or-record 'handler))
 
+(defun bookmark-get-last-modified (bookmark-name-or-record)
+  "Return the last-modified for BOOKMARK-NAME-OR-RECORD, or nil if none."
+  (bookmark-prop-get bookmark-name-or-record 'last-modified))
+
 (defvar bookmark-history nil
   "The history list for bookmark functions.")
 
@@ -497,6 +505,21 @@ bookmark--remove-fringe-mark
               (when (eq 'bookmark (overlay-get temp 'category))
                 (delete-overlay (setq found temp))))))))))
 
+(defun bookmark-maybe-sort-alist ()
+  "Return `bookmark-alist' for display.
+If `bookmark-sort-flag' is T, then return a sorted by name copy of the alist.
+If `bookmark-sort-flag' is LAST-MODIFIED, then return a sorted by last modified
+copy of the alist.  Otherwise, just return `bookmark-alist', which by default
+is ordered from most recently created to least recently created bookmark."
+  (let ((copy (copy-alist bookmark-alist)))
+    (cond ((eq bookmark-sort-flag t)
+           (sort copy (lambda (x y) (string-lessp (car x) (car y)))))
+          ((eq bookmark-sort-flag 'last-modified)
+           (sort copy (lambda (x y)
+                        (time-less-p (bookmark-get-last-modified y)
+                                     (bookmark-get-last-modified x)))))
+          (t copy))))
+
 (defun bookmark-completing-read (prompt &optional default)
   "Prompting with PROMPT, read a bookmark name in completion.
 PROMPT will get a \": \" stuck on the end no matter what, so you
@@ -506,10 +529,8 @@ bookmark-completing-read
   (bookmark-maybe-load-default-file) ; paranoia
   (if (listp last-nonmenu-event)
       (bookmark-menu-popup-paned-menu t prompt
-				      (if bookmark-sort-flag
-					  (sort (bookmark-all-names)
-						'string-lessp)
-					(bookmark-all-names)))
+                                      (mapcar 'bookmark-name-from-full-record
+                                              (bookmark-maybe-sort-alist)))
     (let* ((completion-ignore-case bookmark-completion-ignore-case)
            (default (unless (equal "" default) default)))
       (completing-read (format-prompt prompt default)
@@ -630,7 +651,8 @@ bookmark-make-record-default
                                    (point)
                                    (- (point) bookmark-search-size))
                                   nil))))
-    (position . ,(or posn (point)))))
+    (position . ,(or posn (point)))
+    (last-modified . ,(current-time))))
 
 \f
 ;;; File format stuff
@@ -1140,15 +1162,6 @@ bookmark-maybe-load-default-file
                                   (car bookmark-bookmarks-timestamp)))))))
          (bookmark-load (car bookmark-bookmarks-timestamp) t t))))
 
-(defun bookmark-maybe-sort-alist ()
-  "Return `bookmark-alist' for display.
-If `bookmark-sort-flag' is non-nil, then return a sorted copy of the alist.
-Otherwise, just return `bookmark-alist', which by default is ordered
-from most recently created to least recently created bookmark."
-  (if bookmark-sort-flag
-      (sort (copy-alist bookmark-alist)
-            (lambda (x y) (string-lessp (car x) (car y))))
-    bookmark-alist))
 
 
 (defvar bookmark-after-jump-hook nil
@@ -1825,27 +1838,28 @@ bookmark-bmenu--revert
               entries)))
     ;; The value of `bookmark-sort-flag' might have changed since the
     ;; last time the buffer contents were generated, so re-check it.
-    (if bookmark-sort-flag
-        (progn
-          (setq tabulated-list-sort-key '("Bookmark Name" . nil))
-          (setq tabulated-list-entries entries))
-      (setq tabulated-list-sort-key nil)
-      ;; And since we're not sorting by bookmark name, show bookmarks
-      ;; according to order of creation, with the most recently
-      ;; created bookmarks at the top and the least recently created
-      ;; at the bottom.
-      ;;
-      ;; Note that clicking the column sort toggle for the bookmark
-      ;; name column will invoke the `tabulated-list-mode' sort, which
-      ;; uses `bookmark-bmenu--name-predicate' to sort lexically by
-      ;; bookmark name instead of by (reverse) creation order.
-      ;; Clicking the toggle again will reverse the lexical sort, but
-      ;; the sort will still be lexical not creation-order.  However,
-      ;; if the user reverts the buffer, then the above check of
-      ;; `bookmark-sort-flag' will happen again and the buffer will
-      ;; go back to a creation-order sort.  This is all expected
-      ;; behavior, as documented in `bookmark-bmenu-mode'.
-      (setq tabulated-list-entries (reverse entries)))
+    (cond ((eq bookmark-sort-flag t)
+           (setq tabulated-list-sort-key '("Bookmark Name" . nil)
+                 tabulated-list-entries entries))
+          ((or (null bookmark-sort-flag)
+               (eq bookmark-sort-flag 'last-modified))
+           (setq tabulated-list-sort-key nil)
+           ;; And since we're not sorting by bookmark name, show bookmarks
+           ;; according to order of creation, with the most recently
+           ;; created bookmarks at the top and the least recently created
+           ;; at the bottom.
+           ;;
+           ;; Note that clicking the column sort toggle for the bookmark
+           ;; name column will invoke the `tabulated-list-mode' sort, which
+           ;; uses `bookmark-bmenu--name-predicate' to sort lexically by
+           ;; bookmark name instead of by (reverse) creation order.
+           ;; Clicking the toggle again will reverse the lexical sort, but
+           ;; the sort will still be lexical not creation-order.  However,
+           ;; if the user reverts the buffer, then the above check of
+           ;; `bookmark-sort-flag' will happen again and the buffer will
+           ;; go back to a creation-order sort.  This is all expected
+           ;; behavior, as documented in `bookmark-bmenu-mode'.
+           (setq tabulated-list-entries (reverse entries))))
     ;; Generate the header only after `tabulated-list-sort-key' is
     ;; settled, because if that's non-nil then the sort-direction
     ;; indicator will be shown in the named column, but if it's
@@ -1953,7 +1967,7 @@ bookmark-bmenu-mode
           ,@(if bookmark-bmenu-toggle-filenames
                 '(("File" 0 bookmark-bmenu--file-predicate)))])
   (setq tabulated-list-padding bookmark-bmenu-marks-width)
-  (when bookmark-sort-flag
+  (when (eq bookmark-sort-flag t)
     (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)
-- 
2.36.0


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

-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

>> "timestamp" doesn't say which time it's referring to.
>> I think we should call it "last-modified" or something like that, so
>> there's no ambiguity.
>
> Yes, that makes sense.

I tried the patch, and it seems to lead to three test failures:

3 unexpected results:
   FAILED  bookmark-tests-make-record
   FAILED  bookmark-tests-make-record-list
   FAILED  bookmark-tests-set


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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I tried the patch, and it seems to lead to three test failures:
>
> 3 unexpected results:
>    FAILED  bookmark-tests-make-record
>    FAILED  bookmark-tests-make-record-list
>    FAILED  bookmark-tests-set

Oups, I didn't run a "make check" 😅.

With the patch a call to (bookmark-make-record) will have an up to date
last-modified field so it cannot be equal to a static bookmark record.
For the same reason, two successive calls won't be equal.  Should I try
to fix those tests or fix my code?
-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> With the patch a call to (bookmark-make-record) will have an up to date
> last-modified field so it cannot be equal to a static bookmark record.
> For the same reason, two successive calls won't be equal.  Should I try
> to fix those tests or fix my code?

Sounds like fixing the tests is the right thing.

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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> With the patch a call to (bookmark-make-record) will have an up to date
>> last-modified field so it cannot be equal to a static bookmark record.
>> For the same reason, two successive calls won't be equal.  Should I try
>> to fix those tests or fix my code?
>
> Sounds like fixing the tests is the right thing.

Sorry, I've just checked and I could leave 'bookmark-make-record'
untouched and slap a last-modified on the record in
'bookmark-set-internal'. WDYT?
-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Sorry, I've just checked and I could leave 'bookmark-make-record'
> untouched and slap a last-modified on the record in
> 'bookmark-set-internal'. WDYT?

I'm not quite sure -- can you propose a patch, and I'll have a look?

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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Sorry, I've just checked and I could leave 'bookmark-make-record'
>> untouched and slap a last-modified on the record in
>> 'bookmark-set-internal'. WDYT?
>
> I'm not quite sure -- can you propose a patch, and I'll have a look?

Hi Lars,

You were right, it feels more correct to fix the tests then to handle
last-modified separately.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-add-a-last-modified-field-when-a-bookmark-is-set.patch --]
[-- Type: text/x-patch, Size: 12288 bytes --]

From ce1e77769925a8035981cdcc9a9acb5d99edee12 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Thu, 26 May 2022 18:25:52 +0200
Subject: [PATCH] add a last-modified field when a bookmark is set.

* test/lisp/bookmark-tests.el (bookmark-tests-make-record)
(bookmark-tests-make-record-list, bookmark-tests-set): fix tests
to not consider last-modified in bookmark equality.
* lisp/bookmark.el (bookmark-make-record-default): add a
last-modified field.
(bookmark-sort-flag): add the 'last-modified choice.
(bookmark-get-last-modified): new function to get last-modified
bookmark field.
(bookmark-maybe-sort-alist): sort in last-modified first order.
(bookmark-completing-read): use `bookmark-maybe-sort-alist'.
---
 etc/NEWS                    |  5 ++
 lisp/bookmark.el            | 92 +++++++++++++++++++++----------------
 test/lisp/bookmark-tests.el | 21 +++++----
 3 files changed, 70 insertions(+), 48 deletions(-)

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


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

-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> You were right, it feels more correct to fix the tests then to handle
> last-modified separately.

Thanks; pushed to Emacs 29 (with some whitespace and naming changes to
make it fit our conventions).

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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Juri Linkov]

>> You were right, it feels more correct to fix the tests then to handle
>> last-modified separately.
>
> Thanks; pushed to Emacs 29 (with some whitespace and naming changes to
> make it fit our conventions).

I tried it out, but noticed that it writes unintelligible numbers
in the bookmarks file, such as (last-modified 25239 1033 644055 97000).
Wouldn't it be more user-friendly to write timestamps in a more
recognizable format?  E.g. with ISO date: (last-modified "20220601").

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

Juri Linkov <juri@linkov.net> writes:

>> Thanks; pushed to Emacs 29 (with some whitespace and naming changes to
>> make it fit our conventions).
>
> I tried it out, but noticed that it writes unintelligible numbers
> in the bookmarks file, such as (last-modified 25239 1033 644055 97000).
> Wouldn't it be more user-friendly to write timestamps in a more
> recognizable format?  E.g. with ISO date: (last-modified "20220601").

Yes, I have used (current-time) output directly so it can be used with
`time-less-p'. If the bookmark file, needs to stay more or less human
readable I could add a format-time-string/parse-time-string dance.

But I have one question, it seems that `time-less-p' works with
`parse-time-string' outputs:

--8<---------------cut here---------------start------------->8---
(let ((past (current-time)))
  (sit-for 5)
  (time-less-p (parse-time-string (format-time-string "%FT%T%z" past))
	       (parse-time-string (format-time-string "%FT%T%z" (current-time)))))
--8<---------------cut here---------------end--------------->8---

Is that true or is it pure luck?
-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> But I have one question, it seems that `time-less-p' works with
> `parse-time-string' outputs:
>
> (let ((past (current-time)))
>   (sit-for 5)
>   (time-less-p (parse-time-string (format-time-string "%FT%T%z" past))
> 	       (parse-time-string (format-time-string "%FT%T%z" (current-time)))))
>
> Is that true or is it pure luck?

I think that's pure luck?

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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I think that's pure luck?

😅 so, as Stefan said, it might be better to keep this format.
-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Stefan Monnier]

>>> You were right, it feels more correct to fix the tests then to handle
>>> last-modified separately.
>> Thanks; pushed to Emacs 29 (with some whitespace and naming changes to
>> make it fit our conventions).
> I tried it out, but noticed that it writes unintelligible numbers
> in the bookmarks file, such as (last-modified 25239 1033 644055 97000).

AFAIK the bookmarks format is not designed to be consumed by a human, so
it's not a bad choice.

> Wouldn't it be more user-friendly to write timestamps in a more
> recognizable format?  E.g. with ISO date: (last-modified "20220601").

That would make it slower to compare when sorting, so I think the
current choice of using an Emacs time object is fine.


        Stefan

RE: [External] : [emacs bookmark.el] Sorting by last set

[Drew Adams]

> it writes unintelligible numbers in the bookmarks file,
> such as (last-modified 25239 1033 644055 97000).
> Wouldn't it be more user-friendly to write timestamps in a more
> recognizable format?  E.g. with ISO date: (last-modified "20220601").

No.  There's a UI for user interaction with
bookmarks.  (And there can be more such.)

There's no need for a bookmark record to be
immediately intelligible.  What's important
is that it be Lisp-readable.

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Thanks; pushed to Emacs 29 (with some whitespace and naming changes to
> make it fit our conventions).

Sorry, here is two issues I found. The first is a typo in a
docstring. The second sorts bookmark entries without a last-modified
field at the end when `bookmark-sort-flag' is 'last-modified: I think it
would help transitioning.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-typo-in-bookmark.el.patch --]
[-- Type: text/x-patch, Size: 752 bytes --]

From e6fabc074c85d403e4644388af231c5db4fcc8dd Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Wed, 1 Jun 2022 14:43:58 +0200
Subject: [PATCH 1/2] typo in bookmark.el

---
 lisp/bookmark.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 8e251e9de8..6f7dcc7f0e 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -120,7 +120,7 @@ bookmark-sort-flag
 recently created ones come first, oldest ones come last).
 
 `last-modified' means that bookmarks will be displayed sorted
-from most recently set to last recently set.
+from most recently set to least recently set.
 
 Other values means that bookmarks will be displayed sorted by
 bookmark name."
-- 
2.36.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-place-bookmarks-without-last-modified-at-the-end.patch --]
[-- Type: text/x-patch, Size: 1176 bytes --]

From d2601de9c8a3c7c9eeaa6547bdc6d1387fbbe78e Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Wed, 1 Jun 2022 15:30:19 +0200
Subject: [PATCH 2/2] place bookmarks without last-modified at the end

---
 lisp/bookmark.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 6f7dcc7f0e..849303fac7 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -520,8 +520,11 @@ bookmark-maybe-sort-alist
            (sort copy (lambda (x y) (string-lessp (car x) (car y)))))
           ((eq bookmark-sort-flag 'last-modified)
            (sort copy (lambda (x y)
-                        (time-less-p (bookmark-get-last-modified y)
-                                     (bookmark-get-last-modified x)))))
+                        (let ((tx (bookmark-get-last-modified x))
+                              (ty (bookmark-get-last-modified y)))
+                          (cond ((null tx) nil)
+                                ((null ty) t)
+                                (t (time-less-p ty tx)))))))
           (t copy))))
 
 (defun bookmark-completing-read (prompt &optional default)
-- 
2.36.0


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

-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Sorry, here is two issues I found. The first is a typo in a
> docstring. The second sorts bookmark entries without a last-modified
> field at the end when `bookmark-sort-flag' is 'last-modified: I think it
> would help transitioning.

Thanks; pushed to Emacs 29.

(In future patched, ChangeLog-style commit messages would be
appreciated.)

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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Manuel Giraud]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Thanks; pushed to Emacs 29.
>
> (In future patched, ChangeLog-style commit messages would be
> appreciated.)

Yes Eli already told me that 😅. Do you have any "method" to generate
those kind of commit messages?
-- 
Manuel Giraud

Re: [External] : [emacs bookmark.el] Sorting by last set

[Lars Ingebrigtsen]

Manuel Giraud <manuel@ledu-giraud.fr> writes:

> Yes Eli already told me that 😅. Do you have any "method" to generate
> those kind of commit messages?

Yes, just type `C-x 4 a' from the part of the code you're fixing.

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

Re: [External] : [emacs bookmark.el] Sorting by last set

[Karl Fogel]

On 31 May 2022, Lars Ingebrigtsen wrote:
>Manuel Giraud <manuel@ledu-giraud.fr> writes:
>
>> You were right, it feels more correct to fix the tests then to 
>> handle
>> last-modified separately.
>
>Thanks; pushed to Emacs 29 (with some whitespace and naming 
>changes to
>make it fit our conventions).

Manuel, thank you, and Lars, thank you (and Drew and Stefan) for 
the reviews.

Although I normally try to be responsive to bookmark.el threads, 
and to do prompt review of bookmark.ela patches, right now I'm 
"out" for a while (helping with some parental health issues) and 
thus am not able to participate very much.  Thanks for making this 
one happen.  I think it's a quite nice improvement to bookmarks.

Best regards,
-Karl

Re: [External] : [emacs bookmark.el] Sorting by last set

[Stefan Monnier]

>> 1. Please don't change the bookmark-file format.
> Yes I've seen this comment in bookmark.el.  It seems that it should not
> be taken lightly. So this rules out adding a "modified date" field (my
> 2nd option) but maybe my first option (sorting 'bookmark-alist' in last
> modified order by default) is still possible.

AFAICT the current format is quite extensible and we could easily add
an optional `modified-date` element without changing the format.


        Stefan