From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Karl Fogel Newsgroups: gmane.emacs.bugs Subject: bug#59580: 29.0.50; [PATCH] Add new choices to bookmark-sort-flag Date: Mon, 18 Dec 2023 17:26:09 -0600 Message-ID: <87jzpbm6su.fsf@red-bean.com> References: <83bkonilbm.fsf@gnu.org> Reply-To: Karl Fogel Mime-Version: 1.0 Content-Type: text/plain; format=flowed Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="11211"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 59580@debbugs.gnu.org, Gabriel To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Dec 19 00:27:22 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rFN16-0002dz-Oa for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 19 Dec 2023 00:27:21 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rFN0o-0003iC-G1; Mon, 18 Dec 2023 18:27:02 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rFN0n-0003hp-H1 for bug-gnu-emacs@gnu.org; Mon, 18 Dec 2023 18:27:01 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rFN0n-0001mY-8y for bug-gnu-emacs@gnu.org; Mon, 18 Dec 2023 18:27:01 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rFN0o-0002NS-CG for bug-gnu-emacs@gnu.org; Mon, 18 Dec 2023 18:27:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Karl Fogel Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 18 Dec 2023 23:27:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 59580 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 59580-submit@debbugs.gnu.org id=B59580.17029419769008 (code B ref 59580); Mon, 18 Dec 2023 23:27:02 +0000 Original-Received: (at 59580) by debbugs.gnu.org; 18 Dec 2023 23:26:16 +0000 Original-Received: from localhost ([127.0.0.1]:33797 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rFN04-0002LD-5m for submit@debbugs.gnu.org; Mon, 18 Dec 2023 18:26:16 -0500 Original-Received: from sanpietro.red-bean.com ([45.79.25.59]:55514) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rFN01-0002Kw-Gn for 59580@debbugs.gnu.org; Mon, 18 Dec 2023 18:26:14 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=red-bean.com; s=202005newsp; h=Content-Type:MIME-Version:Message-ID:Date: Reply-To:References:In-Reply-To:Subject:Cc:To:From:Sender: Content-Transfer-Encoding:Content-ID:Content-Description; bh=QOlKRJgw6WGTBQDlDsa2hXCpw4Vu8lrhbqHYoicyWvM=; t=1702941971; x=1704151571; b=OxZR6DRMCvPXiERXlq/mzYAvtluAvyqy9hy4Rdjs09GhHgEUhslIdp1zqApwn17vhcod9Oq4f1U GVkOMT9ecbTG5+6CtR8cfjp6pGHYQ+iOJVq6vYMvFOwn/XlnbNuaCctLc1xvhVJ7xuuJUEnVjm9pL 6/9tG3R8prtKrPIVpZ2ORFce/AYuuno5FCAnk/1R75RwrhsnGdQs4lYDH/cZ0fxQ7jhi/1K6qnYVR 0iSgRFWJ9Hii/IELE+vWxpJN2aokGOPcB0sdq8XUYEirgkLl3tzOd3gkVWrL6khdXWKa1CZpu9U5X 1R1bJIgDuFRK+mbpXolxLbRTtWJWRUprGNQw==; Original-Received: from [12.106.183.66] (port=38032 helo=hummy) by sanpietro.red-bean.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rFMzx-007hNx-SV; Mon, 18 Dec 2023 23:26:09 +0000 In-Reply-To: <83bkonilbm.fsf@gnu.org> (Eli Zaretskii's message of "Thu, 01 Dec 2022 10:23:41 +0200") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:276489 Archived-At: On 01 Dec 2022, Eli Zaretskii wrote: >> From: Gabriel >> 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