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

Previous Next

To reply to this bug, email your comments to 59580 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Gabriel <gabriel376 <at> hotmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag
Date: Fri, 25 Nov 2022 15:26:49 -0300

[Message part 1 (text/plain, inline)]

Severity: wishlist

Description:

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.

[1] https://lists.gnu.org/archive/html/emacs-devel/2022-11/msg00995.html

[0001-Add-more-choices-to-bookmark-sort-flag.patch (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

---
Gabriel

Message #8 received at 59580 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Gabriel <gabriel376 <at> hotmail.com>, Karl Fogel <kfogel <at> red-bean.com>
Cc: 59580 <at> debbugs.gnu.org
Subject: Re: bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag
Date: Thu, 01 Dec 2022 10:23:41 +0200

> From: Gabriel <gabriel376 <at> 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?

Thanks.

Message #11 received at 59580 <at> debbugs.gnu.org (full text, mbox):

From: Karl Fogel <kfogel <at> red-bean.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59580 <at> debbugs.gnu.org, Gabriel <gabriel376 <at> hotmail.com>,
 59581 <at> debbugs.gnu.org
Subject: Re: bug#59580 and bug#59581
Date: Mon, 19 Dec 2022 15:00:26 -0600

I am just seeing tickets #59580 and #59581 now.  I will review the 
patches as soon as I can and reply back.  This won't be till the 
end of the week, however, as I am traveling before then and won't 
have much working time while on the road.

Thanks for the patches Gabriel, and thanks for the ping, Eli.

Best regards,
-Karl

Message #14 received at 59580 <at> debbugs.gnu.org (full text, mbox):

From: Karl Fogel <kfogel <at> red-bean.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59580 <at> debbugs.gnu.org, Gabriel <gabriel376 <at> hotmail.com>
Subject: Re: bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag
Date: Mon, 18 Dec 2023 17:26:09 -0600

On 01 Dec 2022, Eli Zaretskii wrote:
>> From: Gabriel <gabriel376 <at> 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

Message #17 received at 59580 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Karl Fogel <kfogel <at> red-bean.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: "59580 <at> debbugs.gnu.org" <59580 <at> debbugs.gnu.org>,
 Gabriel <gabriel376 <at> hotmail.com>
Subject: RE: [External] : bug#59580: 29.0.50; [PATCH] Add new choices to
 bookmark-sort-flag
Date: Tue, 19 Dec 2023 00:44:57 +0000

FWIW -

An option name `*-flag' is explicitly
intended to signify a Boolean option.
"Adding more choices" to such an option
should entail renaming it.

As for bookmark sort orders, there are
many more that make sense - see here for
several that are predefined, and for how
users can easily combine them or define
their own:

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

(Related:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39293#8)

Message #20 received at 59580 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Karl Fogel <kfogel <at> red-bean.com>
Cc: 59580 <at> debbugs.gnu.org, gabriel376 <at> hotmail.com
Subject: Re: bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag
Date: Tue, 19 Dec 2023 14:25:31 +0200

> From: Karl Fogel <kfogel <at> red-bean.com>
> Cc: Gabriel <gabriel376 <at> hotmail.com>,  59580 <at> debbugs.gnu.org
> Date: Mon, 18 Dec 2023 17:26:09 -0600
> 
> 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).

It is IMO un-Emacsy to do things in locale-dependent manner.  Which
other feature do you know of that sorts text in locale-dependent
fashion?

So I still think using string-collate-lessp here is not the best idea.

Message #23 received at 59580 <at> debbugs.gnu.org (full text, mbox):

From: Karl Fogel <kfogel <at> red-bean.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 59580 <at> debbugs.gnu.org, gabriel376 <at> hotmail.com
Subject: Re: bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag
Date: Tue, 19 Dec 2023 12:59:07 -0600

On 19 Dec 2023, Eli Zaretskii wrote:
>> From: Karl Fogel <kfogel <at> red-bean.com>
>> Cc: Gabriel <gabriel376 <at> hotmail.com>,  59580 <at> debbugs.gnu.org
>> Date: Mon, 18 Dec 2023 17:26:09 -0600
>> 
>> 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).
>
>It is IMO un-Emacsy to do things in locale-dependent manner. 
>Which
>other feature do you know of that sorts text in locale-dependent
>fashion?

I have no idea -- I doubt I would ever notice it if a feature did 
sort that way, since I'd be accustomed to that sorting behavior. 
I also don't see why such behavior is particularly un-Emacsy.  But 
I don't feel strongly about it; if you think `string-lessp' is 
better, let's use that.

Best regards,
-Karl

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.