Re: [Patch] to correctly sort the items with emphasis marks in a list

[Maxim Nikulin <manikulin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

On 21/04/2021 03:37, Nicolas Goaziou wrote:
> Maxim Nikulin writes:
> 
>> (let ((s (org-sort-remove-invisible
>> "A wrapping [[https://orgmode.org?a=b&c=d#e][link]] emphasis/"
>>
>> I expect "A wrapping link emphasis".
> 
> Yet, your expectations are wrong. There is no link in the text above.
> Emphasized text starts at "/wrapping" and ends before "/?".
>
> Granted, this is a situation where the Org syntax is not very intuitive.
> Anyway, the new function is more accurate.

I think, new variant should be committed even it does not fix Juan's 
case. He have not confirmed the fix yet.

> Maybe we should require a space after punctuation following emphasized
> text. I don't know. This is orthogonal to the current discussion.

I still believe in my expectation, however I admit such limitation of 
parser. At first I have not recognized that the issue may be similar to
https://orgmode.org/list/CAH+Wm4-_XHUZKFTf=ZtbfnCPvQWkbEoeGs8EpYm+8SPmu8LHFg@mail.gmail.com/
Anyway for my example workaround is to add more markers before, inside, 
and after the link. Maybe I will look closer at Tom's parser if it 
solves ambiguity in the same way.

>> In the meanwhile I have tried
>>
>>      (benchmark-run 1 (org-sort-list t ?a))
>>
>> in a file (1100 lines) obtained using
> 
> I don't think performance is really an issue. Of course, the suggested
> function is clearly slower than the current one.

It is OK since difference is not really huge, especially taking into 
account that new variant was not compiled.

Do you still have problem with locale dependency of added tests? I can 
not guess what could be its source and expect that test should work 
reliably. Disregard "/3" in the subject of the patches. Third change is 
your code.

[-- Attachment #2: 0001-More-tests-for-org-sort-list.patch --]
[-- Type: text/x-patch, Size: 4913 bytes --]

From c2c46d133e80ffa2323618ac88a1902e63ba1efc Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Mon, 19 Apr 2021 19:06:36 +0700
Subject: [PATCH 1/3] More tests for `org-sort-list'

* lisp/org.el (org-verbatim-re): Add to the docstring a statement
concerning subgroups meaning.
* testing/lisp/test-org-list.el (test-org-list/sort): Add cases with
text emphasis.  Stress in comments that it is significant whether
locale-specific collation rules ignores spaces.
* testing/lisp/test-org.el (test-org/org-sort-remove-invisible): Add
tests for `org-sort-list' helper.
---
 lisp/org.el                   |  3 ++-
 testing/lisp/test-org-list.el | 34 +++++++++++++++++++++++++++++++++-
 testing/lisp/test-org.el      | 25 +++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index c2738100f..3d4f5553a 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -3685,7 +3685,8 @@ After a match, the match groups contain these elements:
 5  The character after the match, empty at the end of a line")
 
 (defvar org-verbatim-re nil
-  "Regular expression for matching verbatim text.")
+  "Regular expression for matching verbatim text.
+Groups 1-5 have the same meaning as in `org-emph-re' pattern.")
 
 (defvar org-emphasis-regexp-components) ; defined just below
 (defvar org-emphasis-alist) ; defined just below
diff --git a/testing/lisp/test-org-list.el b/testing/lisp/test-org-list.el
index 3689a172f..cc7914c8d 100644
--- a/testing/lisp/test-org-list.el
+++ b/testing/lisp/test-org-list.el
@@ -1225,7 +1225,39 @@ b. Item 2<point>"
        (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))))))
+		(buffer-string))))
+      ;; Emphasis in the beginning.
+      (should
+       (equal "- a\n- /z/\n"
+              (org-test-with-temp-text "- /z/\n- a\n"
+                (org-sort-list t ?a)
+                (buffer-string))))
+      (should
+       (equal "- *a*\n- z\n"
+              (org-test-with-temp-text "- z\n- *a*\n"
+                (org-sort-list t ?a)
+                (buffer-string))))
+      ;; Emphasis of second word.
+      ;; Locales with significant spaces (C, es_ES, pl_PL)
+      ;; are more sensitive to problems with sort.
+      (should
+       (equal "- a b\n- a /z/\n"
+              (org-test-with-temp-text "- a /z/\n- a b\n"
+                (org-sort-list t ?a)
+                (buffer-string))))
+      (should
+       (equal "- a *b*\n- a z\n"
+              (org-test-with-temp-text "- a z\n- a *b*\n"
+                (org-sort-list t ?a)
+                (buffer-string))))
+      ;; Space role in sorting.
+      ;; Test would fail for locales with ignored space, e.g. en_US, it works
+      ;; in C and currently rare locales having significant space (es_ES, pl_PL)
+      (should
+       (equal "- Time stamp\n- Timer\n"
+              (org-test-with-temp-text "- Timer\n- Time stamp\n"
+                (org-sort-list t ?a)
+                (buffer-string))))))
   ;; Sort numerically.
   (should
    (equal "- 1\n- 2\n- 10\n"
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index f6fb4b3ca..3f74b3f35 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -8262,4 +8262,29 @@ two
 
 (provide 'test-org)
 
+(ert-deftest test-org/org-sort-remove-invisible ()
+  "Empasis markers are stripped by `org-sort-remove-invisible'."
+  (dolist (case '(("Test *bold* text" . "Test bold text")
+                  ("Test /italic/ text" . "Test italic text")
+                  ("Test _underlined_ text" . "Test underlined text")
+                  ("Test +strike through+ text" . "Test strike through text")
+                  ("Test =verbatim= text" . "Test verbatim text")
+                  ("Test ~code~ text" . "Test code text")
+                  ("*Beginning* of a line" . "Beginning of a line")
+                  ("End =of line=" . "End of line")
+                  ("Braces (*around*)" . "Braces (around)")
+                  ("Multiple *emphasis* options /in the/ same *line*" .
+                   "Multiple emphasis options in the same line")
+                  ("/Adjucent/ *emphasis*" . "Adjucent emphasis")
+                  ("Verbatim =with *emphasis* example=" .
+                   "Verbatim with *emphasis* example")
+                  ("Emphasis [[http://orgmode.org/][inside /a link/]]" .
+                   "Emphasis inside a link")
+                  ("*Wrap [[https:/orgmode.org][link]] emphasis*" .
+                   "Wrap link emphasis")
+                  ("A =verbatim [[https://orgmode.org/][link]] example=" .
+                   "A verbatim [[https://orgmode.org/][link]] example")))
+    (let ((test-string (car case))
+          (expectation (cdr case)))
+      (should (equal expectation (org-sort-remove-invisible test-string))))))
 ;;; test-org.el ends here
-- 
2.25.1


[-- Attachment #3: 0002-testing-lisp-test-org.el-Non-obvious-cases-for-org-s.patch --]
[-- Type: text/x-patch, Size: 2330 bytes --]

From e7a326fff98fdc56b91818af526d398d8387bda4 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Wed, 21 Apr 2021 19:22:18 +0700
Subject: [PATCH 2/3] testing/lisp/test-org.el: Non-obvious cases for
 org-sort-remove-invisible

testing/lisp/test-org.el (test-org/org-sort-remove-invisible): Add test
cases illustrating corner cases in parser behavior.

Fontification in Emacs buffer is not consistent with result of
`org-sort-remove-invisible` for 2 of 3 added examples.  The intention is
to make author of changes in parser aware of consequences, however it is
unlikely that users rely on such nuances.  Update expectations if change
is considered as improvement.
---
 testing/lisp/test-org.el | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 3f74b3f35..4f987f509 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -8283,7 +8283,18 @@ two
                   ("*Wrap [[https:/orgmode.org][link]] emphasis*" .
                    "Wrap link emphasis")
                   ("A =verbatim [[https://orgmode.org/][link]] example=" .
-                   "A verbatim [[https://orgmode.org/][link]] example")))
+                   "A verbatim [[https://orgmode.org/][link]] example")
+                  ;; A bit strange cases to catch changes of behavior.
+                  ;; It may be reasonable to update expectation in the case of failure.
+                  ("/Overlapping [[http://orgmode.org][structure/ with link]]" .
+                   ;; not "Overlapping structure with link"
+                   "Overlapping [[http://orgmode.org][structure with link]]")
+                  ("Another [[http://orgmode.org][overlapping *structure]] with* link" .
+                   ;; not "Another overlapping structure with link"
+                   "Another overlapping *structure with* link")
+                  ("A /wrapping [[https://orgmode.org/?bot=true][link]] emphasis/" .
+                   ;; not "A wrapping link emphasis", lost "/" before "?" instead
+                   "A wrapping [[https://orgmode.org?bot=true][link]] emphasis/")))
     (let ((test-string (car case))
           (expectation (cdr case)))
       (should (equal expectation (org-sort-remove-invisible test-string))))))
-- 
2.25.1