[BUG] colview.el regexp - capture operator when title is empty

[BUG] colview.el regexp - capture operator when title is empty

[Sławomir Grochowski]

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

Hello,

I have found a bug in regexp.
Test that need to pass to fix it is below.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-testing-lisp-test-org-colview.el-add-test-to-pass-to.patch --]
[-- Type: text/x-diff, Size: 1049 bytes --]

From 9710f6befb1bd4eee2d508d7baaee95e825a68dc Mon Sep 17 00:00:00 2001
From: Slawomir Grochowski <slawomir.grochowski@gmail.com>
Date: Mon, 19 Aug 2024 12:30:49 +0200
Subject: [PATCH] testing/lisp/test-org-colview.el: add test to pass to fix a
 bug

---
 testing/lisp/test-org-colview.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/testing/lisp/test-org-colview.el b/testing/lisp/test-org-colview.el
index 6b603c31b..8615ef760 100644
--- a/testing/lisp/test-org-colview.el
+++ b/testing/lisp/test-org-colview.el
@@ -90,7 +90,12 @@
   (should
    (equal `(("ITEM" "ITEM" nil "+" "%.1f"))
           (org-columns-compile-format
-           "%ITEM{+;%.1f}"))))
+           "%ITEM{+;%.1f}")))
+  (should
+   ;; Bug - when the title is empty, the regular expression will not capture the operator.
+   (equal '(("ITEM" "ITEM" nil "X" nil))
+          (org-columns-compile-format
+           "%ITEM(){X}"))))
 
 (ert-deftest test-org-colview/substring-below-width ()
   "Test `org-columns--truncate-below-width'."
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 223 bytes --]


Steps to reproduce:
1. add new column with an empty title and select one of the operators.
2. press 's' when the cursor is the added column - operator should be
selected, but it's empty. 

Regards,
-- 
Slawomir Grochowski

Re: [BUG] colview.el regexp - capture operator when title is empty

[Ihor Radchenko]

Sławomir Grochowski <slawomir.grochowski@gmail.com> writes:

> I have found a bug in regexp.
> Test that need to pass to fix it is below.

Thanks!
May you also fix the regexp in `org-columns-compile-format'?
I recommend using `rx' to make the regexp more readable and less prone
to mistakes.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] colview.el regexp - capture operator when title is empty

[Sławomir Grochowski]

Ihor Radchenko <yantar92@posteo.net> writes:

> May you also fix the regexp in `org-columns-compile-format'?
> I recommend using `rx' to make the regexp more readable and less prone
> to mistakes.

OK. I will try.  String-based Emacs regexp is unreadable and quite scary
for me. That's why I always convert all regexes to the `rx' form. But I
thought that since it's just a small error, maybe someone proficient in
string-based Emacs regexp can easily fix it. Because when transferring
it to the `rx' form, I may introduce more errors. But we have some tests
so I will try to do it this way.

-- 
Slawomir Grochowski

Re: [BUG] colview.el regexp - capture operator when title is empty

[Sławomir Grochowski]

I tried to describe the problem better as well as my thought process:

The `org-columns-compile-format' function has a bug in regexp.
When an empty parentheses `()` is in the 'column format string' -
and it always is when user do not provide a column title - then the
regexp can't capture the operator which is in curly brackets `{}`.

Try yourself with code below:
#+begin_src elisp
(org-columns-compile-format"%ITEM(){operator}") 
; return => (("ITEM" "ITEM" nil nil nil)) 
; should be => (("ITEM" "ITEM" nil operator nil)) 
#+end_src

I'm not very skilled in regex, so following Ihor's advice I decided to
write the regex in the form of 'rx' which is more readable. Below is
the modified function:

#+begin_src elisp
  (defun org-columns-compile-format-rx (fmt)
    "Turn a column format string FMT into an alist of specifications.

    The alist has one entry for each column in the format.  The elements of
    that list are:
    property    the property name, as an upper-case string
    title       the title field for the columns, as a string
    width       the column width in characters, can be nil for automatic width
    operator    the summary operator, as a string, or nil
    printf      a printf format for computed values, as a string, or nil

    This function updates `org-columns-current-fmt-compiled'."
    (setq org-columns-current-fmt-compiled nil)
    (let ((start 0))
      (while (string-match
              (rx "%"
                  (optional (group (+ digit)))
                  (group (one-or-more (in alnum "_-")))
                  (optional "(" (group (one-or-more (not (any ")")))) ")")
                  (optional "{" (group (one-or-more (not (any "}")))) "}")
                  (zero-or-more space))
    	    fmt start)
        (setq start (match-end 0))
        (let* ((width (and (match-end 1) (string-to-number (match-string 1 fmt))))
    	     (prop (match-string-no-properties 2 fmt))
    	     (title (or (match-string-no-properties 3 fmt) prop))
    	     (operator (match-string-no-properties 4 fmt)))
    	(push (if (not operator) (list (upcase prop) title width nil nil)
    		(let (printf)
    		  (when (string-match ";" operator)
    		    (setq printf (substring operator (match-end 0)))
    		    (setq operator (substring operator 0 (match-beginning 0))))
    		  (list (upcase prop) title width operator printf)))
    	      org-columns-current-fmt-compiled)))
      (setq org-columns-current-fmt-compiled
    	  (nreverse org-columns-current-fmt-compiled))))
#+end_src

I am checking if in this particular case it returns the same result 
as 'org-columns-compile-format': 

#+begin_src elisp
 (org-columns-compile-format-rx "%ITEM(){operator}") ;; => (("ITEM" "ITEM" nil nil nil))
#+end_src

#+begin_src elisp
  (equal 
   (org-columns-compile-format "%ITEM(){operator}") 
   (org-columns-compile-format-rx "%ITEM(){operator}")) ;; => t
#+end_src

Yes, in this particular case it works the same. To make it easier to
capture the error, I only take a piece of code responsible for the
regexp:

#+begin_src elisp
(let ((text "%25ITEM(){operator}")
        (pattern (rx "%"
                  (optional (group (+ digit)))
                  (group (one-or-more (in alnum "_-")))
                  (optional "(" (group (one-or-more (not (any ")")))) ")")
                  (optional "{" (group (one-or-more (not (any "}")))) "}")
                  (zero-or-more space))))
    (if (string-match pattern text)
        (mapcar (lambda (i) (match-string i text))
                (number-sequence 0 (/ (length (match-data)) 2)))))
;=> ("%25ITEM" "25" "ITEM" nil)
#+end_src

Through trial and error, I came to a solution that provides correct
results. I changed the expression 'one-or-more' to 'zero-or-more' for
'title' and 'operator':

#+begin_src elisp
(let ((text "%25ITEM(){operator}")
        (pattern (rx "%"
                  (optional (group (+ digit)))
                  (group (one-or-more (in alnum "_-")))
                  (optional "(" (group (zero-or-more (not (any ")")))) ")")
                  (optional "{" (group (zero-or-more (not (any "}")))) "}")
                  (zero-or-more space))))
    (if (string-match pattern text)
        (mapcar (lambda (i) (match-string i text))
                (number-sequence 0 (/ (length (match-data)) 2)))))
;=> ("%25ITEM(){operator}" "25" "ITEM" "" "operator" nil)
#+end_src

This fixed a problem but also changed the return value. Now, when we
have empty parentheses '()', it will return an empty string
instead of null.

Therefore, in the function `org-columns-compile-format`, I added calls
to `org-string-nw-p` for the variables 'title' and 'operator'.

Additionally, I think that adding empty parentheses '()' should be removed
when we do not specify a 'column title'. Because it does not provide
any value. So I added another call to `org-string-nw-p' in function
`org-columns-new'.

Patch below:
<#part type="text/x-diff" filename="~/.emacs.d/straight/repos/org/0001-lisp-org-colview.el-org-columns-compile-format-regex.patch" disposition=inline>
<#/part>

Regards,
-- 
Slawomir Grochowski

Re: [BUG] colview.el regexp - capture operator when title is empty

[Ihor Radchenko]

Sławomir Grochowski <slawomir.grochowski@gmail.com> writes:

> Patch below:
> <#part type="text/x-diff" filename="~/.emacs.d/straight/repos/org/0001-lisp-org-colview.el-org-columns-compile-format-regex.patch" disposition=inline>
> <#/part>

There is no patch attached.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [BUG] colview.el regexp - capture operator when title is empty

[Sławomir Grochowski]

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

Ihor Radchenko <yantar92@posteo.net> writes:
> There is no patch attached.


[-- Attachment #2: 0001-lisp-org-colview.el-org-columns-compile-format-regex.patch --]
[-- Type: text/x-diff, Size: 3702 bytes --]

From 987f66774db4ad1e515cac4b9fc5ba46bd627c27 Mon Sep 17 00:00:00 2001
From: Slawomir Grochowski <slawomir.grochowski@gmail.com>
Date: Sat, 24 Aug 2024 18:47:09 +0200
Subject: [PATCH] lisp/org-colview.el: org-columns-compile-format regexp bugfix
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/org-colview.el (org-columns-compile-format): Refactor
string-based regexp to `rx' form. Fix regexp: When an empty
parentheses `()` is in the column format string then the regexp
can't capture the operator which is in curly brackets `{}`.
(org-columns-new): prevent adding empty parantheses '()' to 'column
format string' when 'column title' is empty.
* testing/lisp.test-org-colview.el (test-org-colview/compile-format):
add new case to test: check if operator will be captured when there is
empty parantheses '()' in 'column format string'.

Reported-by: Sławomir Grochowski <slawomir.grochowski@gmail.com>
Link: https://list.orgmode.org/orgmode/877ccczt83.fsf@gmail.com/
---
 lisp/org-colview.el              | 19 ++++++++++++-------
 testing/lisp/test-org-colview.el |  7 ++++++-
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/lisp/org-colview.el b/lisp/org-colview.el
index bc93941e4..b111c1676 100644
--- a/lisp/org-colview.el
+++ b/lisp/org-colview.el
@@ -935,8 +935,9 @@ details."
 			 (mapcar #'list (org-buffer-property-keys t nil t))
 			 nil nil (nth 0 spec))))
 		   (list prop
-			 (read-string (format "Column title [%s]: " prop)
-				      (nth 1 spec))
+			 (org-string-nw-p
+                          (read-string (format "Column title [%s]: " prop)
+				       (nth 1 spec)))
 			 ;; Use `read-string' instead of `read-number'
 			 ;; to allow empty width.
 			 (let ((w (read-string
@@ -1183,14 +1184,18 @@ This function updates `org-columns-current-fmt-compiled'."
   (setq org-columns-current-fmt-compiled nil)
   (let ((start 0))
     (while (string-match
-	    "%\\([0-9]+\\)?\\([[:alnum:]_-]+\\)\\(?:(\\([^)]+\\))\\)?\
-\\(?:{\\([^}]+\\)}\\)?\\s-*"
-	    fmt start)
+            (rx "%"
+                (optional (group (+ digit)))
+                (group (one-or-more (in alnum "_-")))
+                (optional "(" (group (zero-or-more (not (any ")")))) ")")
+                (optional "{" (group (zero-or-more (not (any "}")))) "}")
+                (zero-or-more space))
+    	    fmt start)
       (setq start (match-end 0))
       (let* ((width (and (match-end 1) (string-to-number (match-string 1 fmt))))
 	     (prop (match-string-no-properties 2 fmt))
-	     (title (or (match-string-no-properties 3 fmt) prop))
-	     (operator (match-string-no-properties 4 fmt)))
+	     (title (or (org-string-nw-p (match-string-no-properties 3 fmt)) prop))
+	     (operator (org-string-nw-p (match-string-no-properties 4 fmt))))
 	(push (if (not operator) (list (upcase prop) title width nil nil)
 		(let (printf)
 		  (when (string-match ";" operator)
diff --git a/testing/lisp/test-org-colview.el b/testing/lisp/test-org-colview.el
index 6b603c31b..623ee6283 100644
--- a/testing/lisp/test-org-colview.el
+++ b/testing/lisp/test-org-colview.el
@@ -90,7 +90,12 @@
   (should
    (equal `(("ITEM" "ITEM" nil "+" "%.1f"))
           (org-columns-compile-format
-           "%ITEM{+;%.1f}"))))
+           "%ITEM{+;%.1f}")))
+  (should
+   ;; Bug https://list.orgmode.org/orgmode/877ccczt83.fsf@gmail.com/
+   (equal '(("ITEM" "ITEM" nil "X" nil))
+          (org-columns-compile-format
+           "%ITEM(){X}"))))
 
 (ert-deftest test-org-colview/substring-below-width ()
   "Test `org-columns--truncate-below-width'."
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 25 bytes --]


-- 
Slawomir Grochowski

Re: [BUG] colview.el regexp - capture operator when title is empty

[Ihor Radchenko]

Sławomir Grochowski <slawomir.grochowski@gmail.com> writes:

> Subject: [PATCH] lisp/org-colview.el: org-columns-compile-format regexp bugfix

Thanks!
Applied, onto main.
Fixed.

I reworded the commit message a bit, making it slightly shorter and
making sure that there is double space between sentences. I also added a
small comment explaining that we discard whitespace-only titles.

https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=07dd3bcae6

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>