[RFC] Properly handle keyword + COMMENT keyword

[RFC] Properly handle keyword + COMMENT keyword

[Nicolas Goaziou]

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

Hello,

COMMENT keyword is not always clearly defined in Org. Some parts
consider it is a regular keyword as treat it as such (e.g. `org-todo')
whereas some others see it as an additional keyword (e.g.,
`org-priority').

I think the latter makes more sense, and, as a consequence, Org Syntax
(http://orgmode.org/worg/dev/org-syntax.html) defines it that way. Much
like :ARCHIVE:, COMMENT provides an additional property to the headline,
without limiting it whatsoever.

In other words, the following should be valid:

  * TODO COMMENT Headline

and, according to Org Syntax, so should this:

  * TODO [#A] COMMENT Headline

Though, COMMENT keyword must come after any other keyword and priority,
if any. So the following is /not/ valid:

  * COMMENT TODO Headline

Thus, this patch

 - properly fontifies headlines with both a regular keyword and
   a COMMENT keyword,

 - fixes `org-toggle-comment' and `org-todo' to handle both COMMENT
   keyword and another one

 - adds some consistency to functions implementing their own COMMENT
   matching (e.g., with or without case-sensitivity).


WDYT?

-- 
Nicolas Goaziou

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-COMMENT-keyword-when-stacked-with-a-regular-keyw.patch --]
[-- Type: text/x-diff, Size: 6846 bytes --]

From 0a16a1136dbda9deec40b9a72e7dd0a5d648db71 Mon Sep 17 00:00:00 2001
From: Nicolas Goaziou <n.goaziou@gmail.com>
Date: Mon, 24 Mar 2014 21:46:00 +0100
Subject: [PATCH] Fix COMMENT keyword when stacked with a regular keyword

* lisp/org.el (org-set-font-lock-defaults): Fix headline fontification
  when keywords are stacked.
(org-toggle-comment): Properly toggle COMMENT keyword when a regular
keyword is already present.
(org-todo, org-agenda-prepare-buffers): Correctly match a commented
heading.
* lisp/org-colview.el (org-columns-capture-view): Correctly match
  a commented heading.

* testing/lisp/test-org.el (test-org/toggle-comment): New test.
---
 lisp/org-colview.el      |  8 +++----
 lisp/org.el              | 40 +++++++++++++++++----------------
 testing/lisp/test-org.el | 58 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 24 deletions(-)

diff --git a/lisp/org-colview.el b/lisp/org-colview.el
index 07d140f..9ebea98 100644
--- a/lisp/org-colview.el
+++ b/lisp/org-colview.el
@@ -1201,8 +1201,6 @@ containing the title row and all other rows.  Each row is a list
 of fields."
   (save-excursion
     (let* ((title (mapcar 'cadr org-columns-current-fmt-compiled))
-	   (re-comment (format org-heading-keyword-regexp-format
-			       org-comment-string))
 	   (re-archive (concat ".*:" org-archive-tag ":"))
 	   (n (length title)) row tbl)
       (goto-char (point-min))
@@ -1214,9 +1212,9 @@ of fields."
 				 (/ (1+ (length (match-string 1))) 2)
 			       (length (match-string 1)))))
 		     (get-char-property (match-beginning 0) 'org-columns-key))
-	    (when (save-excursion
-		    (goto-char (point-at-bol))
-		    (or (looking-at re-comment)
+	    (when (or (org-in-commented-heading-p t)
+		      (save-excursion
+			(goto-char (point-at-bol))
 			(looking-at re-archive)))
 	      (org-end-of-subtree t)
 	      (throw 'next t))
diff --git a/lisp/org.el b/lisp/org.el
index ef0bc3f..94bda2a 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -6343,9 +6343,11 @@ needs to be inserted at a specific position in the font-lock sequence.")
 	   ;; Code
 	   '(org-activate-code (1 'org-code t))
 	   ;; COMMENT
-	   (list (format org-heading-keyword-regexp-format
-			 (concat "\\(" org-comment-string "\\)"))
-		 '(2 'org-special-keyword t))
+	   (list (format
+		  "^\\*\\(?: +%s\\)?\\(?: +\\[#[A-Z0-9]\\]\\)? +\\(?9:%s\\)\\(?: \\|$\\)"
+		  org-todo-regexp
+		  org-comment-string)
+		 '(9 'org-special-keyword t))
 	   ;; Blocks and meta lines
 	   '(org-fontify-meta-lines-and-blocks))))
     (setq org-font-lock-extra-keywords (delq nil org-font-lock-extra-keywords))
@@ -12176,17 +12178,17 @@ expands them."
   (interactive)
   (save-excursion
     (org-back-to-heading)
-    (let (case-fold-search)
-      (cond
-       ((looking-at (format org-heading-keyword-regexp-format
-			    org-comment-string))
-	(goto-char (match-end 1))
-	(looking-at (concat " +" org-comment-string))
-	(replace-match "" t t)
-	(when (eolp) (insert " ")))
-       ((looking-at org-outline-regexp)
-	(goto-char (match-end 0))
-	(insert org-comment-string " "))))))
+    (looking-at org-complex-heading-regexp)
+    (goto-char (or (match-end 3) (match-end 2) (match-end 1)))
+    (skip-chars-forward " \t")
+    (unless (memq (char-before) '(?\s ?\t)) (insert " "))
+    (if (org-in-commented-heading-p t)
+	(delete-region (point)
+		       (progn (search-forward " " (line-end-position) 'move)
+			      (skip-chars-forward " \t")
+			      (point)))
+      (insert org-comment-string)
+      (unless (eolp) (insert " ")))))
 
 (defvar org-last-todo-state-is-todo nil
   "This is non-nil when the last TODO state change led to a TODO state.
@@ -12301,7 +12303,7 @@ When called through ELisp, arg is also interpreted in the following way:
       (save-excursion
 	(catch 'exit
 	  (org-back-to-heading t)
-	  (when (looking-at (concat "^\\*+ " org-comment-string))
+	  (when (org-in-commented-heading-p t)
 	    (org-toggle-comment)
 	    (setq commentp t))
 	  (if (looking-at org-outline-regexp) (goto-char (1- (match-end 0))))
@@ -18229,11 +18231,11 @@ When a buffer is unmodified, it is just killed.  When modified, it is saved
 		   (if (org-at-heading-p t)
 		       (add-text-properties (point-at-bol) (org-end-of-subtree t) pa))))
 	       (goto-char (point-min))
-	       (setq re (format org-heading-keyword-regexp-format
-				org-comment-string))
+	       (setq re (format "^\\* .*\\<%s\\>" org-comment-string))
 	       (while (re-search-forward re nil t)
-		 (add-text-properties
-		  (match-beginning 0) (org-end-of-subtree t) pc))))
+		 (when (save-match-data (org-in-commented-heading-p t))
+		   (add-text-properties
+		    (match-beginning 0) (org-end-of-subtree t) pc)))))
 	    (goto-char pos)))))
     (setq org-todo-keywords-for-agenda
           (org-uniquify org-todo-keywords-for-agenda))
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index 0144841..9350f93 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -27,6 +27,64 @@
 \f
 ;;; Comments
 
+(ert-deftest test-org/toggle-comment ()
+  "Test `org-toggle-comment' specifications."
+  ;; Simple headline.
+  (should
+   (equal "* Test"
+	  (org-test-with-temp-text "* COMMENT Test"
+	    (org-toggle-comment)
+	    (buffer-string))))
+  (should
+   (equal "* COMMENT Test"
+	  (org-test-with-temp-text "* Test"
+	    (org-toggle-comment)
+	    (buffer-string))))
+  ;; Headline with a regular keyword.
+  (should
+   (equal "* TODO Test"
+	  (org-test-with-temp-text "* TODO COMMENT Test"
+	    (org-toggle-comment)
+	    (buffer-string))))
+  (should
+   (equal "* TODO COMMENT Test"
+	  (org-test-with-temp-text "* TODO Test"
+	    (org-toggle-comment)
+	    (buffer-string))))
+  ;; Empty headline.
+  (should
+   (equal "* "
+	  (org-test-with-temp-text "* COMMENT"
+	    (org-toggle-comment)
+	    (buffer-string))))
+  (should
+   (equal "* COMMENT"
+	  (org-test-with-temp-text "* "
+	    (org-toggle-comment)
+	    (buffer-string))))
+  ;; Headline with a single keyword.
+  (should
+   (equal "* TODO "
+	  (org-test-with-temp-text "* TODO COMMENT"
+	    (org-toggle-comment)
+	    (buffer-string))))
+  (should
+   (equal "* TODO COMMENT"
+	  (org-test-with-temp-text "* TODO"
+	    (org-toggle-comment)
+	    (buffer-string))))
+  ;; Headline with a keyword, a priority cookie and contents.
+  (should
+   (equal "* TODO [#A] Headline"
+	  (org-test-with-temp-text "* TODO [#A] COMMENT Headline"
+	    (org-toggle-comment)
+	    (buffer-string))))
+  (should
+   (equal "* TODO [#A] COMMENT Headline"
+	  (org-test-with-temp-text "* TODO [#A] Headline"
+	    (org-toggle-comment)
+	    (buffer-string)))))
+
 (ert-deftest test-org/comment-dwim ()
   "Test `comment-dwim' behaviour in an Org buffer."
   ;; No region selected, no comment on current line and line not
-- 
1.9.1

Re: [RFC] Properly handle keyword + COMMENT keyword

[Samuel Wales]

does this preserve todo keyword font locking and sorting?  it is an
interesting change if so, and i would probably find it useful.

does archive work the same way?

i presume it is a coincidence that this message comes after mine which
mentioned font locking and sorting, as the fix to that message is to
not evaluate or export call lines that are not supposed to be
exported.  i would not find this change to be a suitable workaround
for that.

however, i like the idea of commenting out a headline not changing
sorting and font locking.

i have always called these quasi-keywords.

Re: [RFC] Properly handle keyword + COMMENT keyword

[Nicolas Goaziou]

Hello,

Samuel Wales <samologist@gmail.com> writes:

> does this preserve todo keyword font locking

This is one goal of the patch.

> and sorting?

IIRC, TODO-based sorting refers to `org-todo-keywords-1', which doesn't
include "COMMENT", so I don't think this changes sorting (i.e., sorting
ignores COMMENT keywords).

> does archive work the same way?

I didn't touch to archive tag (but there's some work to do here to, as
a first step, `org-in-archived-heading-p' would be nice). Note that
ARCHIVE is a tag, though.

> i presume it is a coincidence that this message comes after mine which
> mentioned font locking and sorting

No it isn't. Your message recalled me this needed to be fixed.

> as the fix to that message is to not evaluate or export call lines
> that are not supposed to be exported. i would not find this change to
> be a suitable workaround for that.

I do not want to provide a workaround for what I do not consider to be
a bug. Export and Babel are two different parts of Org that happen to
have a non-empty intersection. "No Export" doesn't have to mean "No
Babel" (unless you use a COMMENT keyword, that is).

Anyway that's another topic.


Regards,

-- 
Nicolas Goaziou

Re: [RFC] Properly handle keyword + COMMENT keyword

[Nicolas Goaziou]

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Thus, this patch
>
>  - properly fontifies headlines with both a regular keyword and
>    a COMMENT keyword,
>
>  - fixes `org-toggle-comment' and `org-todo' to handle both COMMENT
>    keyword and another one
>
>  - adds some consistency to functions implementing their own COMMENT
>    matching (e.g., with or without case-sensitivity).

Applied.

-- 
Nicolas Goaziou

Re: [RFC] Properly handle keyword + COMMENT keyword

[Bastien]

Hi Nicolas,

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Nicolas Goaziou <n.goaziou@gmail.com> writes:
>
>> Thus, this patch
>>
>>  - properly fontifies headlines with both a regular keyword and
>>    a COMMENT keyword,
>>
>>  - fixes `org-toggle-comment' and `org-todo' to handle both COMMENT
>>    keyword and another one
>>
>>  - adds some consistency to functions implementing their own COMMENT
>>    matching (e.g., with or without case-sensitivity).
>
> Applied.

Thanks -- this is a welcome improvement.

Speaking of consistency, I noticed this minor glitch:

* NEXT COMMENT A headline [0/2]

- [ ] A
- [ ] B

* NEXT [#A] [1/2] COMMENT Another headline

- [X] A
- [ ] B

See that the second COMMENT is not fontified and I think
it will not be processed correctly.

Now that COMMENT will not be always the first word of a
headline, we should mention that TODO keywords and priority
cookies are allowed before a COMMENT keyword, while stats
cookies are not.  (Or allow stats cookies for simplicity's
sake.)

What do you think?

-- 
 Bastien

Re: [RFC] Properly handle keyword + COMMENT keyword

[Nicolas Goaziou]

Bastien <bzg@gnu.org> writes:

> Speaking of consistency, I noticed this minor glitch:
>
> * NEXT COMMENT A headline [0/2]
>
> - [ ] A
> - [ ] B
>
> * NEXT [#A] [1/2] COMMENT Another headline
>
> - [X] A
> - [ ] B
>
> See that the second COMMENT is not fontified and I think
> it will not be processed correctly.
>
> Now that COMMENT will not be always the first word of a
> headline, we should mention that TODO keywords and priority
> cookies are allowed before a COMMENT keyword, while stats
> cookies are not.  (Or allow stats cookies for simplicity's
> sake.)

COMMENT has to be at the start of the headline text, which starts after
any TODO keyword and priority character (see `org-heading-components').
So the COMMENT keyword in the second headline is not valid.

The manual specifies:

  Also entire subtrees starting with the keyword ‘COMMENT’ will never be
  exported

We could be more explicit. In any case, COMMENT cannot be put anywhere.


Regards,

-- 
Nicolas Goaziou

Re: [RFC] Properly handle keyword + COMMENT keyword

[Bastien]

Hi Nicolas,

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> So the COMMENT keyword in the second headline is not valid.

I know, but the users should not have to guess this.
Maybe a note about the allowed structure here in the
manual would be useful.

-- 
 Bastien

Re: [RFC] Properly handle keyword + COMMENT keyword

[Nicolas Goaziou]

Bastien <bzg@altern.org> writes:

> I know, but the users should not have to guess this.
> Maybe a note about the allowed structure here in the
> manual would be useful.

Done in e84c1d8442b857f9275e86bb34f12811f49fcdd6.


Regards,

-- 
Nicolas Goaziou

Re: [RFC] Properly handle keyword + COMMENT keyword

[Bastien]

Nicolas Goaziou <n.goaziou@gmail.com> writes:

> Bastien <bzg@altern.org> writes:
>
>> I know, but the users should not have to guess this.
>> Maybe a note about the allowed structure here in the
>> manual would be useful.
>
> Done in e84c1d8442b857f9275e86bb34f12811f49fcdd6.

Perfect, thanks,

-- 
 Bastien