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

[Karl Fogel <kfogel@red-bean.com>]

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