[PATCH 1/6] Fix alphabetic sorting for tables, plain lists

[PATCH 1/6] Fix alphabetic sorting for tables, plain lists

[Sebastian Reuße]

* org-table.el (org-table-sort-lines): Use collated sorting.
* org-list.el (org-sort-list): Use collated sorting.

Cf. commit 551d2f1fe.
---
 etc/ORG-NEWS      | 6 ++++++
 lisp/org-list.el  | 5 +++--
 lisp/org-table.el | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index dbedbf7c9..77373d442 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -106,6 +106,12 @@ document, use =shrink= value instead, or in addition to align:
 ,#+STARTUP: align shrink
 #+END_EXAMPLE
 
+*** Alphabetic sorting in tables and lists
+
+When sorting alphabetically, ~org-table-sort-lines~ and ~org-sort-list~
+now sort according to the locale’s collation rules instead of by
+code-point.
+
 ** New features
 *** Add support for links to LaTeX equations in HTML export
 Use MathJax links when enabled (by ~org-html-with-latex~), otherwise
diff --git a/lisp/org-list.el b/lisp/org-list.el
index 9e015cdc2..d646e264c 100644
--- a/lisp/org-list.el
+++ b/lisp/org-list.el
@@ -2804,7 +2804,8 @@ (defun org-sort-list
 by a time stamp, by a property or by priority.
 
 Comparing entries ignores case by default.  However, with an
-optional argument WITH-CASE, the sorting considers case as well.
+optional argument WITH-CASE, the sorting considers case as well,
+if the current locale allows for it.
 
 The command prompts for the sorting type unless it has been given
 to the function through the SORTING-TYPE argument, which needs to
@@ -2850,7 +2851,7 @@ (defun org-sort-list
 		   (error "Missing key extractor"))))
 	 (sort-func
 	  (cond
-	   ((= dcst ?a) #'string<)
+	   ((= dcst ?a) #'org-string-collate-lessp)
 	   ((= dcst ?f)
 	    (or compare-func
 		(and interactive?
diff --git a/lisp/org-table.el b/lisp/org-table.el
index f303c2581..316533172 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -1794,7 +1794,7 @@ (defun org-table-sort-lines
 	     (predicate
 	      (cl-case sorting-type
 		((?n ?N ?t ?T) #'<)
-		((?a ?A) #'string<)
+		((?a ?A) #'org-string-collate-lessp)
 		((?f ?F)
 		 (or compare-func
 		     (and interactive?
-- 
2.16.2

[PATCH 2/6] Fix alphabetic string matching operators

[Sebastian Reuße]

* org.el (org-string<): Add.
(org-op-to-function): Use it.
(org-string> etc.): Use collated comparison.

Cf. commit 551d2f1fe.
---
 lisp/org.el | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index ac1ad3c75..edae502a8 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -14045,7 +14045,7 @@ (defun org-op-to-function (op &optional stringp)
   "Turn an operator into the appropriate function."
   (setq op
 	(cond
-	 ((equal  op   "<"       ) '(<     string<      org-time<))
+	 ((equal  op   "<"       ) '(<     org-string<  org-time<))
 	 ((equal  op   ">"       ) '(>     org-string>  org-time>))
 	 ((member op '("<=" "=<")) '(<=    org-string<= org-time<=))
 	 ((member op '(">=" "=>")) '(>=    org-string>= org-time>=))
@@ -14054,9 +14054,10 @@ (defun org-op-to-function (op &optional stringp)
   (nth (if (eq stringp 'time) 2 (if stringp 1 0)) op))
 
 (defun org<> (a b) (not (= a b)))
-(defun org-string<= (a b) (or (string= a b) (string< a b)))
-(defun org-string>= (a b) (not (string< a b)))
-(defun org-string>  (a b) (and (not (string= a b)) (not (string< a b))))
+(defun org-string<  (a b) (string-collate-lessp a b))
+(defun org-string<= (a b) (or (string= a b) (string-collate-lessp a b)))
+(defun org-string>= (a b) (not (string-collate-lessp a b)))
+(defun org-string>  (a b) (and (not (string= a b)) (not (string-collate-lessp a b))))
 (defun org-string<> (a b) (not (string= a b)))
 (defun org-time=  (a b) (setq a (org-2ft a) b (org-2ft b)) (and (> a 0) (> b 0) (=     a b)))
 (defun org-time<  (a b) (setq a (org-2ft a) b (org-2ft b)) (and (> a 0) (> b 0) (<     a b)))
-- 
2.16.2

[PATCH] Fix alphabetic string matching operators

[Sebastian Reuße]

* org.el (org-string<): Add.
(org-op-to-function): Use it.
(org-string> etc.): Use collated comparison.

Cf. commit 551d2f1fe.
---
 lisp/org.el | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index ac1ad3c75..24923ad4f 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -14045,7 +14045,7 @@ (defun org-op-to-function (op &optional stringp)
   "Turn an operator into the appropriate function."
   (setq op
 	(cond
-	 ((equal  op   "<"       ) '(<     string<      org-time<))
+	 ((equal  op   "<"       ) '(<     org-string<  org-time<))
 	 ((equal  op   ">"       ) '(>     org-string>  org-time>))
 	 ((member op '("<=" "=<")) '(<=    org-string<= org-time<=))
 	 ((member op '(">=" "=>")) '(>=    org-string>= org-time>=))
@@ -14054,9 +14054,10 @@ (defun org-op-to-function (op &optional stringp)
   (nth (if (eq stringp 'time) 2 (if stringp 1 0)) op))
 
 (defun org<> (a b) (not (= a b)))
-(defun org-string<= (a b) (or (string= a b) (string< a b)))
-(defun org-string>= (a b) (not (string< a b)))
-(defun org-string>  (a b) (and (not (string= a b)) (not (string< a b))))
+(defun org-string<  (a b) (org-string-collate-lessp a b))
+(defun org-string<= (a b) (or (string= a b) (org-string-collate-lessp a b)))
+(defun org-string>= (a b) (not (org-string-collate-lessp a b)))
+(defun org-string>  (a b) (and (not (string= a b)) (not (org-string-collate-lessp a b))))
 (defun org-string<> (a b) (not (string= a b)))
 (defun org-time=  (a b) (setq a (org-2ft a) b (org-2ft b)) (and (> a 0) (> b 0) (=     a b)))
 (defun org-time<  (a b) (setq a (org-2ft a) b (org-2ft b)) (and (> a 0) (> b 0) (<     a b)))
-- 
2.16.2

Re: [PATCH] Fix alphabetic string matching operators

[Nicolas Goaziou]

Sebastian Reuße <seb@wirrsal.net> writes:

> * org.el (org-string<): Add.
> (org-op-to-function): Use it.
> (org-string> etc.): Use collated comparison.
>
> Cf. commit 551d2f1fe.

Applied. Thank you.

Re: [PATCH 2/6] Fix alphabetic string matching operators

[Sebastian Reuße]

Sorry, I committed this one without using the compat-shimmed version of
‘string-collate-lessp’. Amended.

Kind regards,
SR

-- 
Insane cobra split the wood
Trader of the lowland breed
Call a jittney, drive away
In the slipstream we will stay

[PATCH 3/6] Fix org-table-sort-lines test

[Sebastian Reuße]

* test-org-table.el (test-org-table/sort-lines): Fix and improve
testcase.

Sorting and reversing «a C b» should result in «C b a», not in «b a
C».  This test did not fail previously only because
org-table-sort-lines had an issue whereby sorts were always
case-sensitive.
---
 testing/lisp/test-org-table.el | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/testing/lisp/test-org-table.el b/testing/lisp/test-org-table.el
index 850080c7a..d90caa8e9 100644
--- a/testing/lisp/test-org-table.el
+++ b/testing/lisp/test-org-table.el
@@ -1691,13 +1691,13 @@
 	    (buffer-string))))
   ;; Sort alphabetically.
   (should
-   (equal "| a | x |\n| b | 4 |\n| c | 3 |\n"
-	  (org-test-with-temp-text "| <point>a | x |\n| c | 3 |\n| b | 4 |\n"
+   (equal "| a | x |\n| B | 4 |\n| c | 3 |\n"
+	  (org-test-with-temp-text "| <point>a | x |\n| c | 3 |\n| B | 4 |\n"
 	    (org-table-sort-lines nil ?a)
 	    (buffer-string))))
   (should
-   (equal "| c | 3 |\n| b | 4 |\n| a | x |\n"
-	  (org-test-with-temp-text "| <point>a | x |\n| c | 3 |\n| b | 4 |\n"
+   (equal "| c | 3 |\n| B | 4 |\n| a | x |\n"
+	  (org-test-with-temp-text "| <point>a | x |\n| c | 3 |\n| B | 4 |\n"
 	    (org-table-sort-lines nil ?A)
 	    (buffer-string))))
   ;; Sort alphabetically with case.
@@ -1707,7 +1707,7 @@
 	    (org-table-sort-lines t ?a)
 	    (buffer-string))))
   (should
-   (equal "| b |\n| a |\n| C |\n"
+   (equal "| C |\n| b |\n| a |\n"
 	  (org-test-with-temp-text "| <point>a |\n| C |\n| b |\n"
 	    (org-table-sort-lines nil ?A)
 	    (buffer-string))))
-- 
2.16.2

Re: [PATCH 3/6] Fix org-table-sort-lines test

[Nicolas Goaziou]

Sebastian Reuße <seb@wirrsal.net> writes:

> * test-org-table.el (test-org-table/sort-lines): Fix and improve
> testcase.
>
> Sorting and reversing «a C b» should result in «C b a», not in «b a
> C».  This test did not fail previously only because
> org-table-sort-lines had an issue whereby sorts were always
> case-sensitive.

Applied. Thank you.

[PATCH 4/6] Fix string-collate-lessp shim

[Sebastian Reuße]

* org-compat.el (org-string-collate-lessp): When shimming
string-collate-lessp, accept the same arguments as in the unshimmed
case.
---
 lisp/org-compat.el | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lisp/org-compat.el b/lisp/org-compat.el
index 975752224..a22b5f1a6 100644
--- a/lisp/org-compat.el
+++ b/lisp/org-compat.el
@@ -88,10 +88,16 @@ (defvar org-table1-hline-regexp)
 	       (= lastc ?\\))))))
 
 ;; `string-collate-lessp' is new in Emacs 25.
-(defalias 'org-string-collate-lessp
-  (if (fboundp 'string-collate-lessp)
-      'string-collate-lessp
-    'string-lessp))
+(if (fboundp 'string-collate-lessp)
+    (defalias 'org-string-collate-lessp
+      'string-collate-lessp)
+  (defun org-string-collate-lessp (s1 s2 &optional locale ignore-case)
+    "Return non-nil if STRING1 is less than STRING2 in lexicographic order.
+
+Case is significant.
+
+LOCALE and IGNORE-CASE are ignored."
+    (string< s1 s2)))
 
 \f
 ;;; Obsolete aliases (remove them after the next major release).
-- 
2.16.2

Re: [PATCH 4/6] Fix string-collate-lessp shim

[Nicolas Goaziou]

Sebastian Reuße <seb@wirrsal.net> writes:

> * org-compat.el (org-string-collate-lessp): When shimming
> string-collate-lessp, accept the same arguments as in the unshimmed
> case.

Applied. Thank you.

[PATCH 5/6] org-table-sort-lines: Fix case-sensitive sorting

[Sebastian Reuße]

* org-table.el (org-table-sort-lines): Fix case sensitive sorting,
improve docstring.
* test-org-table.el (test-org-table/sort-lines): Enforce C locale when
testing alphabetic sorting.

‘sort-subr’ ignores ‘sort-fold-case’ when a predicate is provided. To
correctly handle case-sensitivity, we now bake it into the predicate.

Since we are now sorting according to the user’s locale, WITH-CASE
will not make a difference in most instances, since most locales
always sort case-insensitively (cf. how GNU sort ignores the ‘-f’
switch).  We now mention this in the function docstring.

In order to meaningfully test case-sensitive sorting, we now enforce
the C locale in the respective unit test.
---
 lisp/org-table.el              |  9 ++++----
 testing/lisp/test-org-table.el | 49 +++++++++++++++++++++++-------------------
 2 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/lisp/org-table.el b/lisp/org-table.el
index 316533172..5ebca54fd 100644
--- a/lisp/org-table.el
+++ b/lisp/org-table.el
@@ -1714,7 +1714,8 @@ (defun org-table-sort-lines
 in the field, or as a HH:MM value).  Sorting in reverse order is
 also possible.
 
-With prefix argument WITH-CASE, alphabetic sorting will be case-sensitive.
+With prefix argument WITH-CASE, alphabetic sorting will be case-sensitive
+if the locale allows for it.
 
 If SORTING-TYPE is specified when this function is called from a Lisp
 program, no prompting will take place.  SORTING-TYPE must be a character,
@@ -1766,8 +1767,7 @@ (defun org-table-sort-lines
       ;; Determine arguments for `sort-subr'.  Also record original
       ;; position.  `org-table-save-field' cannot help here since
       ;; sorting is too much destructive.
-      (let* ((sort-fold-case (not with-case))
-	     (coordinates
+      (let* ((coordinates
 	      (cons (count-lines (point-min) (line-beginning-position))
 		    (current-column)))
 	     (extract-key-from-field
@@ -1794,7 +1794,8 @@ (defun org-table-sort-lines
 	     (predicate
 	      (cl-case sorting-type
 		((?n ?N ?t ?T) #'<)
-		((?a ?A) #'org-string-collate-lessp)
+		((?a ?A) (if with-case #'org-string-collate-lessp
+			   (lambda (s1 s2) (org-string-collate-lessp s1 s2 nil t))))
 		((?f ?F)
 		 (or compare-func
 		     (and interactive?
diff --git a/testing/lisp/test-org-table.el b/testing/lisp/test-org-table.el
index d90caa8e9..873e79abb 100644
--- a/testing/lisp/test-org-table.el
+++ b/testing/lisp/test-org-table.el
@@ -1689,28 +1689,33 @@
 	  (org-test-with-temp-text "| <point>1 | 2 |\n| 5 | 3 |\n| 2 | 4 |\n"
 	    (org-table-sort-lines nil ?N)
 	    (buffer-string))))
-  ;; Sort alphabetically.
-  (should
-   (equal "| a | x |\n| B | 4 |\n| c | 3 |\n"
-	  (org-test-with-temp-text "| <point>a | x |\n| c | 3 |\n| B | 4 |\n"
-	    (org-table-sort-lines nil ?a)
-	    (buffer-string))))
-  (should
-   (equal "| c | 3 |\n| B | 4 |\n| a | x |\n"
-	  (org-test-with-temp-text "| <point>a | x |\n| c | 3 |\n| B | 4 |\n"
-	    (org-table-sort-lines nil ?A)
-	    (buffer-string))))
-  ;; Sort alphabetically with case.
-  (should
-   (equal "| C |\n| a |\n| b |\n"
-	  (org-test-with-temp-text "| <point>a |\n| C |\n| b |\n"
-	    (org-table-sort-lines t ?a)
-	    (buffer-string))))
-  (should
-   (equal "| C |\n| b |\n| a |\n"
-	  (org-test-with-temp-text "| <point>a |\n| C |\n| b |\n"
-	    (org-table-sort-lines nil ?A)
-	    (buffer-string))))
+  ;; Sort alphabetically.  Enforce the C locale for consistent results.
+  (let ((original-string-collate-lessp (symbol-function 'string-collate-lessp)))
+    (cl-letf (((symbol-function 'string-collate-lessp)
+	       (lambda (s1 s2 &optional locale ignore-case)
+		 (funcall original-string-collate-lessp
+			  s1 s2 "C" ignore-case))))
+      (should
+       (equal "| a | x |\n| B | 4 |\n| c | 3 |\n"
+	      (org-test-with-temp-text "| <point>a | x |\n| c | 3 |\n| B | 4 |\n"
+				       (org-table-sort-lines nil ?a)
+				       (buffer-string))))
+      (should
+       (equal "| c | 3 |\n| B | 4 |\n| a | x |\n"
+	      (org-test-with-temp-text "| <point>a | x |\n| c | 3 |\n| B | 4 |\n"
+				       (org-table-sort-lines nil ?A)
+				       (buffer-string))))
+      ;; Sort alphabetically with case.
+      (should
+       (equal "| C |\n| a |\n| b |\n"
+	      (org-test-with-temp-text "| <point>a |\n| C |\n| b |\n"
+				       (org-table-sort-lines t ?a)
+				       (buffer-string))))
+      (should
+       (equal "| C |\n| b |\n| a |\n"
+	      (org-test-with-temp-text "| <point>a |\n| C |\n| b |\n"
+				       (org-table-sort-lines nil ?A)
+				       (buffer-string))))))
   ;; Sort by time (timestamps)
   (should
    (equal
-- 
2.16.2

Re: [PATCH 5/6] org-table-sort-lines: Fix case-sensitive sorting

[Nicolas Goaziou]

Sebastian Reuße <seb@wirrsal.net> writes:

> * org-table.el (org-table-sort-lines): Fix case sensitive sorting,
> improve docstring.
> * test-org-table.el (test-org-table/sort-lines): Enforce C locale when
> testing alphabetic sorting.
>
> ‘sort-subr’ ignores ‘sort-fold-case’ when a predicate is provided. To
> correctly handle case-sensitivity, we now bake it into the predicate.
>
> Since we are now sorting according to the user’s locale, WITH-CASE
> will not make a difference in most instances, since most locales
> always sort case-insensitively (cf. how GNU sort ignores the ‘-f’
> switch).  We now mention this in the function docstring.
>
> In order to meaningfully test case-sensitive sorting, we now enforce
> the C locale in the respective unit test.

Applied. Thank you.

[PATCH 6/6] Improve ‘org-sort-list’ test

[Sebastian Reuße]

* test-org-list.el (test-org-list/sort): Take case-sensitive
vs. insensitive sorting into account.
---
 testing/lisp/test-org-list.el | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/testing/lisp/test-org-list.el b/testing/lisp/test-org-list.el
index 866ee4799..e0c448ce9 100644
--- a/testing/lisp/test-org-list.el
+++ b/testing/lisp/test-org-list.el
@@ -1015,16 +1015,32 @@
 (ert-deftest test-org-list/sort ()
   "Test `org-sort-list'."
   ;; Sort alphabetically.
-  (should
-   (equal "- abc\n- def\n- xyz\n"
-	  (org-test-with-temp-text "- def\n- xyz\n- abc\n"
-	    (org-sort-list nil ?a)
-	    (buffer-string))))
-  (should
-   (equal "- xyz\n- def\n- abc\n"
-	  (org-test-with-temp-text "- def\n- xyz\n- abc\n"
-	    (org-sort-list nil ?A)
-	    (buffer-string))))
+  (let ((original-string-collate-lessp (symbol-function 'string-collate-lessp)))
+    (cl-letf (((symbol-function 'string-collate-lessp)
+	       (lambda (s1 s2 &optional locale ignore-case)
+		 (funcall original-string-collate-lessp
+			  s1 s2 "C" ignore-case))))
+      (should
+       (equal "- abc\n- def\n- XYZ\n"
+	      (org-test-with-temp-text "- def\n- XYZ\n- abc\n"
+		(org-sort-list nil ?a)
+		(buffer-string))))
+      (should
+       (equal "- XYZ\n- def\n- abc\n"
+	      (org-test-with-temp-text "- def\n- XYZ\n- abc\n"
+		(org-sort-list nil ?A)
+		(buffer-string))))
+      ;; Sort alphabetically (with case).
+      (should
+       (equal "- C\n- a\n- b\n"
+	      (org-test-with-temp-text "- b\n- C\n- a\n"
+		(org-sort-list t ?a)
+		(buffer-string))))
+      (should
+       (equal "- b\n- a\n- C\n"
+	      (org-test-with-temp-text "- b\n- C\n- a\n"
+		(org-sort-list t ?A)
+		(buffer-string))))))
   ;; Sort numerically.
   (should
    (equal "- 1\n- 2\n- 10\n"
-- 
2.16.2

Re: [PATCH 6/6] Improve ‘org-sort-list’ test

[Nicolas Goaziou]

Sebastian Reuße <seb@wirrsal.net> writes:

> * test-org-list.el (test-org-list/sort): Take case-sensitive
> vs. insensitive sorting into account.

Applied. Thank you.

Re: [PATCH 1/6] Fix alphabetic sorting for tables, plain lists

[Nicolas Goaziou]

Hello,

Sebastian Reuße <seb@wirrsal.net> writes:

> * org-table.el (org-table-sort-lines): Use collated sorting.
> * org-list.el (org-sort-list): Use collated sorting.
>
> Cf. commit 551d2f1fe.

Applied. Thank you.

Regards,

-- 
Nicolas Goaziou