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

[Karl Fogel]

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

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

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

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

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

However, it would be good to:

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

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

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

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

Thoughts?

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

Best regards,
-Karl