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

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

[Karl Fogel]

A few thoughts I just had while looking over the recent 
improvement to bookmark.el sorting (commit f461eb8fa -- not the 
later followup commit fccde52158, which isn't relevant for these 
thoughts):

1) We use the value `last-modified' (for `bookmark-sort-flag') to 
   represent sorting by the date the bookmark was most recently 
   updated to point to a particular target.

   But there are other ways to "modify" bookmarks: for example, 
   you could edit a bookmark's annotation without updating its 
   target.  One could make a reasonably good argument as to why an 
   annotation change should count as a "modification" for the 
   purposes of sorting... and, one could make a reasonably good 
   argument why it shouldn't.

   My purpose here is just to ask: is the name "last-modified" 
   really the most appropriate one for the behavior currently 
   implemented?

   The simple solution would be to just change the symbol to 
   `last-set-date'.  I think that would be my choice.  It would 
   reduce the potential for confusion and misunderstanding.

   (I won't go in to the more complex solutions here, as I'm not 
   sure that bookmark.el really needs its sorting capability to be 
   fully operational battle station.)

* Now that we're using a symbol for *one* possible value of 
  `bookmark-sort-flag', should we use symbols for *all* possible 
  values?  (And leave the treatments of `t' and nil as legacy 
  compatibility behaviors, documented as such but deprecated in 
  favor of using the corresponding new symbols instead when 
  writing new configurations.)

* Finally, perhaps `bookmark-sort-flag' is no longer the right 
  name for 
  this variable?  It's not a "flag" anymore -- a "flag" should be 
  either `t' or nil.

  This naming tradition mostly holds throughout Emacs, although 
  there is at least one other exception.  Of the 54 symbols whose 
  names end in "-flag" that I found via `M-x apropos', I did a 
  random sampling of about 15.  A few don't have real doc strings 
  (sigh), but their values at least were either 't' or nil.  Of 
  the documented ones in my sample, all but one were true Boolean 
  flags.  The single exception was `quit-flag', which I would now 
  also argue is misnamed, but I'm not advocating here that we 
  change that one :-).

  However, I think it would be good to deprecate 
  `bookmark-sort-flag' in favor of 'bookmark-sort-behavior' or 
  something, and do whatever the usual compatibility dance is for 
  such situations.  It's useful for the suffix "-flag" to actually 
  mean something, and I'd rather not have bookmark.el contribute 
  to the dilution of that linguistic tradition.

By the way, I still think the change as currently landed is an 
unambiguous improvement.  I would still want to keep it even if we 
didn't take any of my suggestions above.

Best regards,
-Karl

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

[Stefan Monnier]

> * Finally, perhaps `bookmark-sort-flag' is no longer the right   name for
>   this variable?  It's not a "flag" anymore -- a "flag" should be   either
>  `t' or nil.

BTW, I hate the `-flag` convention specifically for this reason.


        Stefan

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

[Drew Adams]

If you decide to consider renaming and redesigning
bookmark sorting to some extent, then I invite you
to consider how Bookmark+ handles it, as food for
thought.


This doc section describes its approach to sorting:

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

(I've also put a plain-text version of that text at
the end of this message.)

I suggest you read that _first_, as it gives an
overall view of sorting from a user point of view.
But below are two doc strings that may also help.

And finally, here is a page that describes the
"apples & oranges" approach to combining sort
predicates that Bookmark+ uses:

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

______

Doc of macro `bmkp-define-sort-command'.  It
defines commands to sort the displayed bookmark
list (buffer `*Bookmark List*').


bmkp-define-sort-command is a Lisp macro in 'bookmark+-mac.el'.

(bmkp-define-sort-command SORT-ORDER COMPARER DOC-STRING)

Define a command to sort bookmarks in the bookmark list by SORT-ORDER.
SORT-ORDER is a short string or symbol describing the sorting method.
Examples: "by last access time", "by bookmark name".

The new command is named by replacing any spaces in SORT-ORDER with
hyphens (`-') and then adding the prefix `bmkp-bmenu-sort-'.  Example:
`bmkp-bmenu-sort-by-bookmark-name', for SORT-ORDER `by bookmark name'.

COMPARER compares two bookmarks, returning non-nil if and only if the
first bookmark sorts before the second.  It must be acceptable as a
value of `bmkp-sort-comparer'.  That is, it is either nil, a
predicate, or a list ((PRED...) FINAL-PRED).  See the doc for
`bmkp-sort-comparer'.

DOC-STRING is the doc string of the new command.

______

Doc of user option `bmkp-sort-comparer'.  It
holds the default value of the predicate(s)
that do the sorting. 


bmkp-sort-comparer is a variable defined in `bookmark+-bmu.el'.

Its value is 
((bmkp-info-node-name-cp bmkp-gnus-cp bmkp-url-cp
 bmkp-local-file-type-cp) bmkp-alpha-p)

Documentation:

Predicate or predicates for sorting (comparing) bookmarks.
This defines the default sort for bookmarks in the bookmark list.

Various sorting commands, such as `s v', change the value of this
option dynamically (but they do not save the changed value).

The value must be one of the following:

* nil, meaning do not sort
* a predicate that takes two bookmarks as args
* a list of the form ((PRED...) FINAL-PRED), where each PRED and
  FINAL-PRED are predicates that take two bookmarks as args

If the value is a list of predicates, then each PRED is tried in turn
until one returns a non-nil value.  In that case, the result is the
car of that value.  If no non-nil value is returned by any PRED, then
FINAL-PRED is used and its value is the result.

Each PRED should return `(t)' for true, `(nil)' for false, or nil for
undecided.  A nil value means that the next PRED decides (or
FINAL-PRED, if there is no next PRED).

Thus, a PRED is a special kind of predicate that indicates either a
boolean value (as a singleton list) or "I cannot decide - let the
next guy else decide".  (Essentially, each PRED is a hook function
that is run using `run-hook-with-args-until-success'.)

Examples:

 nil           - No sorting.
 string-lessp  - Single predicate that returns nil or non-nil.
 ((p1 p2))     - Two predicates `p1' and `p2', which each return
                 (t) for true, (nil) for false, or nil for undecided.
 ((p1 p2) string-lessp)
               - Same as previous, except if both `p1' and `p2' return
                 nil, then the return value of `string-lessp' is used.

Note that these two values are generally equivalent, in terms of their
effect (*):

 ((p1 p2))
 ((p1) p2-plain) where p2-plain is (bmkp-make-plain-predicate p2)

Likewise, these three values generally act equivalently (*):

 ((p1))
 (() p1-plain)
 p1-plain        where p1-plain is (bmkp-make-plain-predicate p1)

The PRED form lets you easily combine predicates: use `p1' unless it
cannot decide, in which case try `p2', and so on.  The value ((p2 p1))
tries the predicates in the opposite order: first `p2', then `p1' if
`p2' returns nil.

Using a single predicate or FINAL-PRED makes it easy to reuse an
existing predicate that returns nil or non-nil.

You can also convert a PRED-type predicate (which returns (t), (nil),
or nil) into an ordinary predicate, by using function
`bmkp-make-plain-predicate'.  That lets you reuse elsewhere, as
ordinary predicates, any PRED-type predicates you define.

For example, this defines a plain predicate to compare by URL:
 (defalias 'bmkp-url-p (bmkp-make-plain-predicate 'bmkp-url-cp))

Note: As a convention, predefined Bookmark+ PRED-type predicate names
have the suffix `-cp' (for "component predicate") instead of `-p'.

--
* If you use `s C-r', then there is a difference in behavior between

   (a) using a plain predicate as FINAL-PRED and
   (b) using the analogous PRED-type predicate (and no FINAL-PRED).

  In the latter case, `s C-r' affects when the predicate is tried and
  its return value.  See `bmkp-reverse-multi-sort-order'.

________________________________________________

Plain-text version of doc section about sorting:

(@* "Sorting Bookmarks")
 *** Sorting Bookmarks ***

Filtering hides certain kinds of bookmarks.  Sometimes,
you want to see bookmarks of various kinds, but you
want them to be grouped or sorted in different ways,
for easy recognition, comparison, and access.

Bookmarks shown in the bookmark list are sorted using
the current value of option `bmkp-sort-comparer'.  (If
that is `nil', they are unsorted, which means they
appear in reverse chronological order of their
creation.)

You can use `s s'... (repeat hitting the `s' key) to
cycle among the various sort orders possible, updating
the display accordingly.  By default, you cycle among
all available sort orders, but you can shorten the
cycling list by customizing option
`bmkp-sort-orders-for-cycling-alist'.

You can also change directly to one of the main sort
orders (without cycling) using `s >', `s n', `s f n',
etc.  There are many such predefined sort orders bound
to keys with the prefix `s' - use `C-h m' or `?'  for
more info.

`s >'   - Sort marked (`>') before unmarked
`s *'   - Sort modified (`*') before unmodified
`s 0'   - Sort by bookmark creation date/time
`s b'   - Sort by last buffer or file access
`s a'   - Sort annotated (`a') before unannotated
`s d'   - Sort by last bookmark access date/time
`s D'   - Sort flagged (`D') before unflagged
`s f d' - Sort by last local file access date/time
`s f k' - Sort by local file kind: file, symlink, dir
`s f n' - Sort by file name
`s f s' - Sort by local file size
`s f u' - Sort by last local file update (edit) date/time
`s g'   - Sort by Gnus thread: group, article, message.
`s i'   - Sort by Info manual, node, position
`s k'   - Sort by bookmark type (kind)
`s n'   - Sort by bookmark name
`s t'   - Sort tagged (`t') before untagged
`s v'   - Sort by visit frequency

You can reverse the current sort direction
(ascending/descending) using `s r'.  Also, repeating
any of the main sort-order commands (e.g. `s n') cycles
among that order, the reverse, and unsorted.

For a complex sort, which involves composing several
sorting conditions, you can also use `s C-r' to reverse
the order of bookmark sorting groups or the order
within each group (depending on whether `s r' is also
used).  Try it, for example, together with sorting by
bookmark kind (`s k').

Be aware that `s C-r' can be a bit unintuitive.  If it
does not do what you expect or want, or if it confuses
you, then don't use it ;-).  (`s C-r' has no noticeable
effect on simple sorting.)

Remember that you can combine sorting with filtering
different sets of bookmarks - bookmarks of different
kinds (e.g. Info) or bookmarks that are marked or
unmarked.

Finally, you can easily define your own sorting
commands and sort orders.  See macro
`bmkp-define-sort-command' and the documentation for
option `bmkp-sort-comparer'.  (Bookmark+ uses option
`bmkp-sort-comparer'; it *ignores* vanilla Emacs option
`bookmark-sort-flag'.)

Of particular note is that you can interactively define
commands that sort by a given list of tags - you use
keys `T s' (command `bmkp-define-tags-sort-command') to
do that.  You are prompted for the tags to sort by.
Bookmarks are sorted first according to whether they
are tagged with the first tag, then the second tag, and
so on.  Otherwise, sorting is by bookmark name.

The tags you specify are used, in order, in the name of
the new command.  For example, if you enter tags
`alpha', `beta', and `gamma', in that order, then the
sorting command created is
`bmkp-bmenu-sort-alpha-beta-gamma'.  The new command is
saved in your bookmark commands file
(`bmkp-bmenu-commands-file').

Note that because you can add a new tag to all
bookmarks that have some given set of tags, you can use
that single (new) tag to represent the entire tag set.
Sorting by that tag is then the same as sorting by the
tag set.  You can of course use overlapping sets in the
composite sort command.  You can, for example, sort
first according to tag `tag1', which represents the set
of tags `alpha', `beta', `gamma', `delta', and then
sort according to tag `tag2', which represents the set
of tags `beta', `delta'.

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

[Manuel Giraud]

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

> A few thoughts I just had while looking over the recent improvement to
> bookmark.el sorting (commit f461eb8fa -- not the later followup commit
> fccde52158, which isn't relevant for these thoughts):

Hi Karl and thanks for your time on this,

[...]

>   The simple solution would be to just change the symbol to
>   `last-set-date'.  I think that would be my choice.  It would
>   reduce the potential for confusion and misunderstanding.

Yes, I forgot about annotations so I think `last-set-date' would be
better. I could prepare this simple patch.

[...]

> * Now that we're using a symbol for *one* possible value of
>   `bookmark-sort-flag', should we use symbols for *all* possible
>   values?  (And leave the treatments of `t' and nil as legacy
>   compatibility behaviors, documented as such but deprecated in
>   favor of using the corresponding new symbols instead when   writing
>  new configurations.)

Why not... but we have to settle for good symbol names. I propose
'last-created (as nil) and 'alphabetical (as t).

[...]

>  However, I think it would be good to deprecate   `bookmark-sort-flag'
>  in favor of 'bookmark-sort-behavior' or   something, and do whatever
>  the usual compatibility dance is for   such situations.  It's useful
>  for the suffix "-flag" to actually   mean something, and I'd rather
>  not have bookmark.el contribute   to the dilution of that linguistic
> tradition.

That also makes sense but I prefer `bookmark-sort'. Also, I don't know
how to deprecate a custom in emacs.

Best regards,
-- 
Manuel Giraud

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

[Drew Adams]

> > But there are other ways to "modify" bookmarks: for example, 
> > you could edit a bookmark's annotation without updating its 
> > target.  One could make a reasonably good argument as to why an 
> > annotation change should count as a "modification" for the 
> > purposes of sorting... and, one could make a reasonably good 
> > argument why it shouldn't.
> >
> > My purpose here is just to ask: is the name "last-modified" 
> > really the most appropriate one for the behavior currently 
> > implemented?

I can't speak to whatever behavior you've
currently implemented.

But why would _setting_ a bookmark be the only
modification that you want reflected in such a
property?

And does this "setting" include RE-setting or
just initial setting (which I guess is about
the same as creating)?

If resetting's included then which kinds of
resetting?  Does relocating count?  Just
repositioning (manually or automatically)?
Renaming?  Changing properties manually (e.g.
editing), or by code?

Just what is "setting" for your "behavior
currently implemented?  And why is that the
most useful/appropriate behavior for such a
time/date bookmark field?

You're not implementing something offhand.
You're changing the basic bookmark data
structure.  It's worth thinking about.

Other ways to modify (beyond "setting") are
not at all limited to adding, removing, or
modifying an annotation.  That seems like a
straw man, to make it seem like all that's
important wrt modifying is setting.

But even just an annotation edit is a change,
and it might well be significant for a user.
Let's not belittle modification other than
setting.

If you're going to introduce a last-modified
time property of some sort, I'd suggest that,
by default at least, it be updated for any
change to the bookmark - maybe even automatic
repositioning.

But including automatic repositioning could
be a user decision (e.g., a user option, off
by default.  Better is to have a (list value)
option that can cover all predefined kinds of
modification.

> > The simple solution would be to just change the symbol to
> > `last-set-date'.  I think that would be my choice.  It would
> > reduce the potential for confusion and misunderstanding.
> 
> Yes, I forgot about annotations so I think `last-set-date' would be
> better. I could prepare this simple patch.

See above.  What does "set" mean, and why is it
(whatever it means) important to the exclusion
of all other kinds of modification?

> > * Now that we're using a symbol for *one* possible value of
> >   `bookmark-sort-flag', should we use symbols for *all* possible
> >   values?  (And leave the treatments of `t' and nil as legacy
> >   compatibility behaviors, documented as such but deprecated in
> >   favor of using the corresponding new symbols instead when   writing
> >  new configurations.)
> 
> Why not... but we have to settle for good symbol names. I propose
> 'last-created (as nil) and 'alphabetical (as t).

Would you please use `created', the same field
name that Bookmark+ uses?  Occam's razor says
not to complicate things gratuitously.  Why
not use the same name, for the same thing?

There's only one creation of a given bookmark.
It makes no sense to talk of a "last" creation
time.

And time is about all that a `created' field
could usefully record - it's understood.

> I prefer `bookmark-sort'.

Please see what I wrote in my previous message,
if for no reason other than it provides useful
food for thought.

You (plural) are just starting to see the value
(usefulness) of different ways to sort bookmarks.

There are as many ways of usefully sorting as
there are kinds of bookmarks.  No, - as there
are _combinations_ of kinds.

No - there are many more useful ways to sort
than even that.  You've already noticed at least
two kinds of time (created, modified) and one
kind of syntax (alphabetical).

You cannot know what ways to sort will be useful
for this or that user in this or that context.
Users should be given an easy way to define
their own sort orders (sorting commands). 

As experience grows you'll see that a general
and flexible approach to sorting is called for.
Again, I invite you to look at what Bookmark+
offers here - and again, at least as food for
thought.

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

[Manuel Giraud]

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

> I can't speak to whatever behavior you've
> currently implemented.
>
> But why would _setting_ a bookmark be the only
> modification that you want reflected in such a
> property?
>
> And does this "setting" include RE-setting or
> just initial setting (which I guess is about
> the same as creating)?

Hi,

It works for this "setting" and RE-"setting" but not for annotations
change nor renaming. Maybe "set" is not a good name. Why not "placed"?
As a bookmark in book?

[...]

> But even just an annotation edit is a change,
> and it might well be significant for a user.
> Let's not belittle modification other than
> setting.

You're right and that is why it should be clear that this behaviour is
for "placing" the bookmark only. Maybe other kind of modified field
could be add later... or the current behaviour changed along with its
documentation.

> If you're going to introduce a last-modified
> time property of some sort, I'd suggest that,
> by default at least, it be updated for any
> change to the bookmark - maybe even automatic
> repositioning.
>
> But including automatic repositioning could
> be a user decision (e.g., a user option, off
> by default.  Better is to have a (list value)
> option that can cover all predefined kinds of
> modification.

A fully customizable "last-modified" sorting might be a bit too much for
bookmark.el but trying to gather many modifications under one umbrella
could be better than just "placed". What others think?

For my usage, I prefer the "placed only" behaviour.

[...]

>> Why not... but we have to settle for good symbol names. I propose
>> 'last-created (as nil) and 'alphabetical (as t).
>
> Would you please use `created', the same field
> name that Bookmark+ uses?  Occam's razor says
> not to complicate things gratuitously.  Why
> not use the same name, for the same thing?
>
> There's only one creation of a given bookmark.
> It makes no sense to talk of a "last" creation
> time.

Yes of course. But in what I proposed, 'created would only be a possible
symbol for `bookmark-sort-flag' (or new name), nothing more.

>> I prefer `bookmark-sort'.
>
> Please see what I wrote in my previous message,
> if for no reason other than it provides useful
> food for thought.

I've read it. But I think that for the bundled bookmark.el having a
predefined set of sorting functions could be enough.

As for composability of sorting, I think keeping it to "one at a time"
could also be enough. And for users that need more there is Bookmark+

Best regards,
-- 
Manuel Giraud

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

[Stefan Monnier]

> It works for this "setting" and RE-"setting" but not for annotations
> change nor renaming.  Maybe "set" is not a good name.  Why not "placed"?
> As a bookmark in book?

Why not change the code so that any change (renaming, annotation
tweaks, ...) update the timestamp?


        Stefan

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

[Drew Adams]

> > It works for this "setting" and RE-"setting" but not for annotations
> > change nor renaming.  Maybe "set" is not a good name.  Why not
> > "placed"? As a bookmark in book?
> 
> Why not change the code so that any change (renaming,
> annotation tweaks, ...) update the timestamp?

... which was also my suggestion.  As the
default, at least.

But whatever behavior is adopted for this,
even more important than the property name	
is documenting just what the property does
- what kinds of modification are involved,
and when.

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

[Manuel Giraud]

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

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

>> > It works for this "setting" and RE-"setting" but not for annotations
>> > change nor renaming.  Maybe "set" is not a good name.  Why not
>> > "placed"? As a bookmark in book?
>> 
>> Why not change the code so that any change (renaming,
>> annotation tweaks, ...) update the timestamp?
>
> ... which was also my suggestion.  As the
> default, at least.

Ok. Here is a patch for this default. I kept the 'last-modified name but
now annotations editing, renaming or relocation of a bookmark will
update this field.

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

From b3b131b3e3ec9b338fae431e3f54e2e435e5d8d1 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 7 Jun 2022 17:35:02 +0200
Subject: [PATCH] Fix last-modified meaning in bookmark.el

Renaming, relocating or editing annotations of a bookmark now updates
the last-modified field.

* lisp/bookmark.el (bookmark-update-last-modified): new function
to update the last-modified field.
(bookmark-send-edited-annotation, bookmark-relocate)
(bookmark-rename): use `bookmark-update-last-modified' in
annotations editing, relocation and renaming.
---
 lisp/bookmark.el | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index 849303fac7..b0b54e52d8 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 least recently set.
+from most recently modified to least recently modified.
 
 Other values means that bookmarks will be displayed sorted by
 bookmark name."
@@ -468,10 +468,17 @@ 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))
 
+
+(defun bookmark-update-last-modified (bookmark-name-or-record)
+  "Update the last-modified date of BOOKMARK-NAME-OR-RECORD to the current time."
+  (bookmark-prop-set bookmark-name-or-record 'last-modified (current-time)))
+
+
 (defvar bookmark-history nil
   "The history list for bookmark functions.")
 
@@ -1069,6 +1076,7 @@ bookmark-send-edited-annotation
         (from-bookmark-list bookmark--annotation-from-bookmark-list)
         (old-buffer (current-buffer)))
     (bookmark-set-annotation bookmark-name annotation)
+    (bookmark-update-last-modified bookmark-name)
     (setq bookmark-alist-modification-count
           (1+ bookmark-alist-modification-count))
     (message "Annotation updated for \"%s\"" bookmark-name)
@@ -1355,6 +1363,7 @@ bookmark-relocate
                     (format "Relocate %s to: " bookmark-name)
                     (file-name-directory bmrk-filename))))))
     (bookmark-set-filename bookmark-name newloc)
+    (bookmark-update-last-modified bookmark-name)
     (setq bookmark-alist-modification-count
           (1+ bookmark-alist-modification-count))
     (if (bookmark-time-to-save-p)
@@ -1417,6 +1426,7 @@ bookmark-rename
               nil
               'bookmark-history))))
     (bookmark-set-name old-name final-new-name)
+    (bookmark-update-last-modified final-new-name)
     (setq bookmark-current-bookmark final-new-name)
     (bookmark-bmenu-surreptitiously-rebuild-list)
     (setq bookmark-alist-modification-count
-- 
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:

> Ok. Here is a patch for this default. I kept the 'last-modified name but
> now annotations editing, renaming or relocation of a bookmark will
> update this field.

Thanks; pushed to Emacs 29.

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

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

[Drew Adams]

> >> I propose 'last-created ...
> >
> > Would you please use `created', the same field
> > name that Bookmark+ uses?  Occam's razor says
> > not to complicate things gratuitously.  Why
> > not use the same name, for the same thing?
> >
> > There's only one creation of a given bookmark.
> > It makes no sense to talk of a "last" creation
> > time.
> 
> Yes of course.  But in what I proposed, 'created
> would only be a possible symbol for
> `bookmark-sort-flag' (or new name), nothing more.

Sure.  But what, besides sorting, are you thinking of?

Are you thinking of sorting using a composition of
predicates?  Sure, you could skip that.

With Bookmark+ you can compose predicates, but
nothing obliges you to.

> > Please see what I wrote in my previous message,
> > if for no reason other than it provides useful
> > food for thought.
> 
> I've read it. But I think that for the bundled
> bookmark.el having a predefined set of sorting
> functions could be enough.
> 
> As for composability of sorting, I think keeping
> it to "one at a time" could also be enough.

Each of the predefined sort orders in Bookmark+ in
fact uses a single predicate, not a composition.
You could incorporate some of the same, or similar,
predicates in `bookmark.el'.

The default value of `bmkp-sort-comparer' is,
however, a composition:

 ((bmkp-info-node-name-cp
   bmkp-url-cp bmkp-gnus-cp
   bmkp-local-file-type-cp
   bmkp-handler-cp)
  bmkp-alpha-p)	

That sorts by bookmark _type_, for some predefined
types, and for other types it falls back to sorting
alphabetically by bookmark name.  It corresponds to
command `bmkp-bmenu-sort-by-bookmark-type', which
is bound to `s k' in the bookmark display list

("k" suggests bookmark "k"ind; `s t' is taken by
sorting "t"agged bookmarks before untagged ones).

But your option could support only a single pred.

A comment even suggests this:

  ;; An alternative default value: `bmkp-alpha-p'

And as the `bmkp-sort-comparer' doc I sent says,

 You can also convert a PRED-type predicate (which
 returns (t), (nil), or nil) into an ordinary predicate,
 by using function `bmkp-make-plain-predicate'.  That
 lets you reuse elsewhere, as ordinary predicates, any
 PRED-type predicates you define.

> And for users that need more there is Bookmark+

If bookmark.el provides something then Bookmark+
need not provide it. ;-)  Bookmark+ exists because
the features it provides weren't wanted by Emacs.

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

[Stefan Monnier]

>>   The simple solution would be to just change the symbol to
>>   `last-set-date'.  I think that would be my choice.  It would
>>   reduce the potential for confusion and misunderstanding.
>
> Yes, I forgot about annotations so I think `last-set-date' would be
> better. I could prepare this simple patch.

FWIW, I don't think we can expect a user to know intuitively the
difference in meaning between "last modified" and "last set", nor can we
expect them to understand that changing a bookmark by editing it
annotations would be treated differently than changing it by making it
point elsewhere.


        Stefan

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

[Karl Fogel]

On 05 Jun 2022, Manuel Giraud wrote:
>Why not... but we have to settle for good symbol names. I propose
>'last-created (as nil) and 'alphabetical (as t).

Presumably a thing can only have one creation date, so I suggest 
`creation-date' for that first one :-).

Best regards,
-Karl

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

[Drew Adams]

> >Why not... but we have to settle for good symbol names. I propose
> >'last-created (as nil) and 'alphabetical (as t).
> 
> Presumably a thing can only have one creation date,

Yes, that echoes what I said.

> so I suggest creation-date' for that first one :-).

Again, please consider using what `Bookmark+' already
uses: `created'.

(There's really no doubt that it's a date, anyway.)

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

[Manuel Giraud]

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

[...]

>> so I suggest creation-date' for that first one :-).
>
> Again, please consider using what `Bookmark+' already
> uses: `created'.

Keep in mind that here we are just discussing the name of a possible
value for `bookmark-sort-flag' (compatible with nil).

There is currently no creation date written in each bookmark: it is
still just the order of the written bookmarks' list that defines this
order. Maybe, we should have one creation date field and in this case I
think 'created' would be the best name (it is clear, simple, we don't
have one and other package already use this one).

Back to the symbol name (compatible with `bookmark-sort-flag' set to
nil), I have thought of 'last-created but here "last" refers to the fact
that the creation date is in reverse order (ie. the "last" created
bookmark being at the head of the list).

(… and to be coherent with myself, I now think that the 'last-modified I
introduced should really be 'modified)
-- 
Manuel Giraud

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

[Drew Adams]

> >> so I suggest creation-date' for that first one :-).
> >
> > Again, please consider using what `Bookmark+'
> > already uses: `created'.
> 
> Keep in mind that here we are just discussing the name of a possible
> value for `bookmark-sort-flag' (compatible with nil).
> 
> There is currently no creation date written in each bookmark: it is
> still just the order of the written bookmarks' list that defines this
> order. Maybe, we should have one creation date field and in this case I
> think 'created' would be the best name (it is clear, simple, we don't
> have one and other package already use this one).
> 
> Back to the symbol name (compatible with `bookmark-sort-flag' set to
> nil), I have thought of 'last-created but here "last" refers to the
> fact that the creation date is in reverse order (ie. the "last"
> created bookmark being at the head of the list).
> 
> (… and to be coherent with myself, I now think that the 'last-modified
> I introduced should really be 'modified)

I see.  I guess I misunderstood; sorry.

I guess you're talking about what to call a sort
order that sorts by creation time/date (whether
or not there's an explicit field recording that
time/date).

In that case I'd suggest something like just
`creation-time' or, if used as part of messaging
or labeling, "by creation time".

IMO, this is better than putting the time
direction in the name (chronological or reverse
chronological).

Presumably users can (or at some point will be
able to) reverse the direction, regardless of
what the current sort order is.

So whether the default sort by creation time
lists most- or least-recent first shouldn't be
part of the sort-order name.  Reversing should
be available for every sort order.
___

FWIW, Bookmark+ uses these names, both as part
of the sort command names and for display
purposes.

by creation time          <======
by last bookmark access
by last buffer or file access
by last local file update
by last local file access
by bookmark visit frequency
by bookmark name
by bookmark type
by file name
by local file type
by url
by Gnus thread
by Info node name
by Info position
by local file size
annotated before unannotated
  flagged before unflagged
 modified before unmodified
   marked before unmarked
   tagged before untagged

E.g., `*Bookmark List*' shows "sorting by
creation time" or "sorting by creation time
(REVERSED)" in the mode-line, and the command
is`bmkp-bmenu-sort-by-creation-time' (bound
to `s 0').

Repeating any sort command (e.g. `s 0) cycles
among that sort, its reversal, and unsorted.

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

[Manuel Giraud]

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

Hi all,

I hope it is not to late for this but I'd like to have this modification
pushed.  This changes the field name from 'last-modified to 'modified
(the symbol for `bookmark-sort-flag' is kept to 'last-modified).

I think it make more sense since the "last" in 'last-modified refers to
the order (ie. the most recently modified will be first).  Afterward,
I'd like to introduce a 'created field too.

Best regards,

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Change-last-modified-field-name-to-modified.patch --]
[-- Type: text/x-patch, Size: 3725 bytes --]

From 7fe15765e2777d5e001558d6648c7d5f250d34bc Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 14 Jun 2022 17:22:04 +0200
Subject: [PATCH] Change 'last-modified field name to 'modified

* lisp/bookmark.el: change 'last-modified field name to 'modified
---
 lisp/bookmark.el | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index b0b54e52d8..64b8ee7f5e 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -469,14 +469,14 @@ bookmark-get-handler
   (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))
+(defun bookmark-get-modified (bookmark-name-or-record)
+  "Return the modified for BOOKMARK-NAME-OR-RECORD, or nil if none."
+  (bookmark-prop-get bookmark-name-or-record 'modified))
 
 
-(defun bookmark-update-last-modified (bookmark-name-or-record)
-  "Update the last-modified date of BOOKMARK-NAME-OR-RECORD to the current time."
-  (bookmark-prop-set bookmark-name-or-record 'last-modified (current-time)))
+(defun bookmark-update-modified (bookmark-name-or-record)
+  "Update the modified date of BOOKMARK-NAME-OR-RECORD to the current time."
+  (bookmark-prop-set bookmark-name-or-record 'modified (current-time)))
 
 
 (defvar bookmark-history nil
@@ -527,8 +527,8 @@ 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)
-                        (let ((tx (bookmark-get-last-modified x))
-                              (ty (bookmark-get-last-modified y)))
+                        (let ((tx (bookmark-get-modified x))
+                              (ty (bookmark-get-modified y)))
                           (cond ((null tx) nil)
                                 ((null ty) t)
                                 (t (time-less-p ty tx)))))))
@@ -666,7 +666,7 @@ bookmark-make-record-default
                                    (- (point) bookmark-search-size))
                                   nil))))
     (position . ,(or posn (point)))
-    (last-modified . ,(current-time))))
+    (modified . ,(current-time))))
 
 \f
 ;;; File format stuff
@@ -1076,7 +1076,7 @@ bookmark-send-edited-annotation
         (from-bookmark-list bookmark--annotation-from-bookmark-list)
         (old-buffer (current-buffer)))
     (bookmark-set-annotation bookmark-name annotation)
-    (bookmark-update-last-modified bookmark-name)
+    (bookmark-update-modified bookmark-name)
     (setq bookmark-alist-modification-count
           (1+ bookmark-alist-modification-count))
     (message "Annotation updated for \"%s\"" bookmark-name)
@@ -1363,7 +1363,7 @@ bookmark-relocate
                     (format "Relocate %s to: " bookmark-name)
                     (file-name-directory bmrk-filename))))))
     (bookmark-set-filename bookmark-name newloc)
-    (bookmark-update-last-modified bookmark-name)
+    (bookmark-update-modified bookmark-name)
     (setq bookmark-alist-modification-count
           (1+ bookmark-alist-modification-count))
     (if (bookmark-time-to-save-p)
@@ -1426,7 +1426,7 @@ bookmark-rename
               nil
               'bookmark-history))))
     (bookmark-set-name old-name final-new-name)
-    (bookmark-update-last-modified final-new-name)
+    (bookmark-update-modified final-new-name)
     (setq bookmark-current-bookmark final-new-name)
     (bookmark-bmenu-surreptitiously-rebuild-list)
     (setq bookmark-alist-modification-count
-- 
2.36.1


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

-- 
Manuel Giraud

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

[Drew Adams]

> I hope it is not to late for this but I'd like to have this
> modification pushed.  This changes the field name from
> 'last-modified to 'modified (the symbol for `bookmark-sort-flag'
> is kept to 'last-modified).
> 
> I think it make more sense since the "last" in 'last-modified refers to
> the order (ie. the most recently modified will be first).  Afterward,
> I'd like to introduce a 'created field too.

My 2 cents:

1. `modified' is fine as the field name. +1.
   It's short and clear enough.

2. Function names should include something like
   `date' or `time'.  This is because there can be
   other functions, which refer to other things
   about modified (i.e., unsaved) bookmarks.

   E.g., the Bookmark+ code has these functions,
   which return things such as an alist of just the
   modified bookmarks:

   bmkp-modified-bookmark-p - predicate
   bmkp-modified-cp         - predicate
   bmkp-modified-bookmarks  - alist
   bmkp-bmenu-sort-modified-before-unmodified
                            - sort command

3. +1 for `created'.  (I suggested this earlier.)

HTH.

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

[Lars Ingebrigtsen]

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

> I hope it is not to late for this but I'd like to have this modification
> pushed.  This changes the field name from 'last-modified to 'modified
> (the symbol for `bookmark-sort-flag' is kept to 'last-modified).

The change is fine by me, but:

> I think it make more sense since the "last" in 'last-modified refers to
> the order (ie. the most recently modified will be first).

I don't think "last" in `last-modified' as any such connotation -- it
just means "the last time this was modified".

-- 
(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 it make more sense since the "last" in 'last-modified refers to
>> the order (ie. the most recently modified will be first).
>
> I don't think "last" in `last-modified' as any such connotation -- it
> just means "the last time this was modified".

It had (at least in my head 😅)… but english is not my native language
(so maybe it could stay that way).
-- 
Manuel Giraud

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

[Karl Fogel]

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

On 14 Jun 2022, Manuel Giraud wrote:
>I hope it is not to late for this but I'd like to have this 
>modification
>pushed.  This changes the field name from 'last-modified to 
>'modified
>(the symbol for `bookmark-sort-flag' is kept to 'last-modified).
>
>I think it make more sense since the "last" in 'last-modified 
>refers to
>the order (ie. the most recently modified will be first). 
>Afterward,
>I'd like to introduce a 'created field too.

Manuel, it looks like this change hasn't been pushed yet.  I was 
about to review it, but I noticed there was some commentary in the 
thread following up to your post.  Did you have a revised version 
of the patch in response to any of the commentary, or should I 
just review the original patch as you posted it on 14 June 2022?

I'll attach the patch below for convenience.

Best regards,
-Karl


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Change-last-modified-field-name-to-modified.patch --]
[-- Type: text/x-diff, Size: 3726 bytes --]

>From 7fe15765e2777d5e001558d6648c7d5f250d34bc Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 14 Jun 2022 17:22:04 +0200
Subject: [PATCH] Change 'last-modified field name to 'modified

* lisp/bookmark.el: change 'last-modified field name to 'modified
---
 lisp/bookmark.el | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/lisp/bookmark.el b/lisp/bookmark.el
index b0b54e52d8..64b8ee7f5e 100644
--- a/lisp/bookmark.el
+++ b/lisp/bookmark.el
@@ -469,14 +469,14 @@ bookmark-get-handler
   (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))
+(defun bookmark-get-modified (bookmark-name-or-record)
+  "Return the modified for BOOKMARK-NAME-OR-RECORD, or nil if none."
+  (bookmark-prop-get bookmark-name-or-record 'modified))
 
 
-(defun bookmark-update-last-modified (bookmark-name-or-record)
-  "Update the last-modified date of BOOKMARK-NAME-OR-RECORD to the current time."
-  (bookmark-prop-set bookmark-name-or-record 'last-modified (current-time)))
+(defun bookmark-update-modified (bookmark-name-or-record)
+  "Update the modified date of BOOKMARK-NAME-OR-RECORD to the current time."
+  (bookmark-prop-set bookmark-name-or-record 'modified (current-time)))
 
 
 (defvar bookmark-history nil
@@ -527,8 +527,8 @@ 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)
-                        (let ((tx (bookmark-get-last-modified x))
-                              (ty (bookmark-get-last-modified y)))
+                        (let ((tx (bookmark-get-modified x))
+                              (ty (bookmark-get-modified y)))
                           (cond ((null tx) nil)
                                 ((null ty) t)
                                 (t (time-less-p ty tx)))))))
@@ -666,7 +666,7 @@ bookmark-make-record-default
                                    (- (point) bookmark-search-size))
                                   nil))))
     (position . ,(or posn (point)))
-    (last-modified . ,(current-time))))
+    (modified . ,(current-time))))
 
 \f
 ;;; File format stuff
@@ -1076,7 +1076,7 @@ bookmark-send-edited-annotation
         (from-bookmark-list bookmark--annotation-from-bookmark-list)
         (old-buffer (current-buffer)))
     (bookmark-set-annotation bookmark-name annotation)
-    (bookmark-update-last-modified bookmark-name)
+    (bookmark-update-modified bookmark-name)
     (setq bookmark-alist-modification-count
           (1+ bookmark-alist-modification-count))
     (message "Annotation updated for \"%s\"" bookmark-name)
@@ -1363,7 +1363,7 @@ bookmark-relocate
                     (format "Relocate %s to: " bookmark-name)
                     (file-name-directory bmrk-filename))))))
     (bookmark-set-filename bookmark-name newloc)
-    (bookmark-update-last-modified bookmark-name)
+    (bookmark-update-modified bookmark-name)
     (setq bookmark-alist-modification-count
           (1+ bookmark-alist-modification-count))
     (if (bookmark-time-to-save-p)
@@ -1426,7 +1426,7 @@ bookmark-rename
               nil
               'bookmark-history))))
     (bookmark-set-name old-name final-new-name)
-    (bookmark-update-last-modified final-new-name)
+    (bookmark-update-modified final-new-name)
     (setq bookmark-current-bookmark final-new-name)
     (bookmark-bmenu-surreptitiously-rebuild-list)
     (setq bookmark-alist-modification-count
-- 
2.36.1

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

[Manuel Giraud]

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

[...]

> Manuel, it looks like this change hasn't been pushed yet.  I was about
> to review it, but I noticed there was some commentary in the thread
> following up to your post.  Did you have a revised version of the
> patch in response to any of the commentary, or should I just review
> the original patch as you posted it on 14 June 2022?
>
> I'll attach the patch below for convenience.

Hi Karl (sorry for this late reply),

With the comment from Lars that "last-modified" could be understood as
"Last time it was modified", I think this patch is not required anymore.

Thanks,
-- 
Manuel Giraud

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

[Karl Fogel]

On 31 Aug 2022, Manuel Giraud wrote:
>Karl Fogel <kfogel@red-bean.com> writes:
>
>[...]
>
>> Manuel, it looks like this change hasn't been pushed yet.  I 
>> was about
>> to review it, but I noticed there was some commentary in the 
>> thread
>> following up to your post.  Did you have a revised version of 
>> the
>> patch in response to any of the commentary, or should I just 
>> review
>> the original patch as you posted it on 14 June 2022?
>>
>> I'll attach the patch below for convenience.
>
>Hi Karl (sorry for this late reply),
>
>With the comment from Lars that "last-modified" could be 
>understood as
>"Last time it was modified", I think this patch is not required 
>anymore.

Thanks, Manuel -- good to know.

And I'm the last person anyone should apologize to for a late 
reply, don't worry.  (My record is 15 years, I think, but that's 
not to say I couldn't beat that some day.)

Best regards,
-Karl

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

[Emanuel Berg]

Karl Fogel wrote:

> My record is 15 years, I think, but that's not to say
> I couldn't beat that some day.

There are a lot of records that seem pretty beatable, if you
think about it.

-- 
underground experts united
https://dataswamp.org/~incal

[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