[PATCH] Fix inconsistency in drawer handling

[PATCH] Fix inconsistency in drawer handling

[Yann Hodique]

Hi,

I've noticed that drawers are not managed similarly, depending on the
workflow. More precisely, the end-of-drawer detection is not exactly
the same for various exports, org-element, or folding. It all boils
down to variations around (re-search-forward ":END:").

In particular, it's expected that property drawers for taskjuggler
export will contain an :end: key (which is arguably a bad idea by
itself), that fools some other parts of org.

This patch is an attempt at making drawers handling more predictable.

Thanks,

Yann.

Yann Hodique (1):
  Fix inconsistency in drawer handling

 lisp/org-element.el |  8 ++++----
 lisp/org.el         | 18 ++++++++++++------
 2 files changed, 16 insertions(+), 10 deletions(-)

-- 
1.7.12

[PATCH] Fix inconsistency in drawer handling

[Yann Hodique]

* lisp/org.el (org-drawer-end-re): Introduce new constant.
(org-clock-drawer-start-re): Fix docstring.
(org-clock-drawer-end-re): Fix docstring.
(org-flag-drawer): Make use of `org-drawer-end-re'.
(org-end-of-meta-data-and-drawers): Make use of `org-drawer-end-re'.

* lisp/org-element.el (org-element-drawer-parser): Make use of
  `org-drawer-end-re'.
(org-element-property-drawer-parser): Make use of `org-drawer-end-re'.

Subtle differences in the definition of drawers delimiters were leading
to inconsistencies. For example, the following drawer:

  :PROPERTIES:
  :start: now
  :end:   then
  :END:

was interpreted by `org-entry-properties' as containing properties
"start" and "end", whereas the cycling code would hide at the
first :end:, and `org-element-drawer-parser' would also consider the
black to end at the first :end:.
---
 lisp/org-element.el |  8 ++++----
 lisp/org.el         | 18 ++++++++++++------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/lisp/org-element.el b/lisp/org-element.el
index bcdd336..9822045 100644
--- a/lisp/org-element.el
+++ b/lisp/org-element.el
@@ -529,7 +529,7 @@ Return a list whose CAR is `drawer' and CDR is a plist containing
 
 Assume point is at beginning of drawer."
   (let ((case-fold-search t))
-    (if (not (save-excursion (re-search-forward "^[ \t]*:END:" limit t)))
+    (if (not (save-excursion (re-search-forward org-drawer-end-re limit t)))
 	;; Incomplete drawer: parse it as a paragraph.
 	(org-element-paragraph-parser limit)
       (let ((drawer-end-line (match-beginning 0)))
@@ -1895,7 +1895,7 @@ Assume point is at the beginning of the property drawer."
 	  (hidden (org-invisible-p2))
 	  (properties
 	   (let (val)
-	     (while (not (looking-at "^[ \t]*:END:"))
+	     (while (not (looking-at org-property-end-re))
 	       (when (looking-at "[ \t]*:\\([A-Za-z][-_A-Za-z0-9]*\\):")
 		 (push (cons (org-match-string-no-properties 1)
 			     (org-trim
@@ -1904,7 +1904,7 @@ Assume point is at the beginning of the property drawer."
 		       val))
 	       (forward-line))
 	     val))
-	  (prop-end (progn (re-search-forward "^[ \t]*:END:" limit t)
+	  (prop-end (progn (re-search-forward org-property-end-re limit t)
 			   (point-at-bol)))
 	  (pos-before-blank (progn (forward-line) (point)))
 	  (end (progn (skip-chars-forward " \r\t\n" limit)
@@ -3395,7 +3395,7 @@ element it has to parse."
         (let ((name (match-string 1)))
 	  (cond
 	   ((not (save-excursion
-		   (re-search-forward "^[ \t]*:END:[ \t]*$" nil t)))
+		   (re-search-forward org-drawer-end-re nil t)))
 	    (org-element-paragraph-parser limit))
 	   ((equal "PROPERTIES" name)
 	    (org-element-property-drawer-parser limit))
diff --git a/lisp/org.el b/lisp/org.el
index 1c18d70..f62bde6 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -6768,6 +6768,9 @@ open and agenda-wise Org files."
 	(while (re-search-forward org-drawer-regexp end t)
 	  (org-flag-drawer t))))))
 
+(eval-when-compile
+  (defvar org-drawer-end-re))
+
 (defun org-flag-drawer (flag)
   "When FLAG is non-nil, hide the drawer we are within.
 Otherwise make it visible."
@@ -6776,7 +6779,7 @@ Otherwise make it visible."
     (when (looking-at "^[ \t]*:[a-zA-Z][a-zA-Z0-9]*:")
       (let ((b (match-end 0)))
 	(if (re-search-forward
-	     "^[ \t]*:END:"
+	     org-drawer-end-re
 	     (save-excursion (outline-next-heading) (point)) t)
 	    (outline-flag-region b (point-at-eol) flag)
 	  (error ":END: line missing at position %s" b))))))
@@ -14308,17 +14311,20 @@ but in some other way.")
   "Some properties that are used by Org-mode for various purposes.
 Being in this list makes sure that they are offered for completion.")
 
+(defconst org-drawer-end-re "^[ \t]*:END:[ \t]*$"
+  "Regular expression matching the last line of a drawer.")
+
 (defconst org-property-start-re "^[ \t]*:PROPERTIES:[ \t]*$"
   "Regular expression matching the first line of a property drawer.")
 
-(defconst org-property-end-re "^[ \t]*:END:[ \t]*$"
+(defconst org-property-end-re org-drawer-end-re
   "Regular expression matching the last line of a property drawer.")
 
 (defconst org-clock-drawer-start-re "^[ \t]*:CLOCK:[ \t]*$"
-  "Regular expression matching the first line of a property drawer.")
+  "Regular expression matching the first line of a clock drawer.")
 
-(defconst org-clock-drawer-end-re "^[ \t]*:END:[ \t]*$"
-  "Regular expression matching the first line of a property drawer.")
+(defconst org-clock-drawer-end-re org-drawer-end-re
+  "Regular expression matching the first line of a clock drawer.")
 
 (defconst org-property-drawer-re
   (concat "\\(" org-property-start-re "\\)[^\000]*\\("
@@ -22080,7 +22086,7 @@ clocking lines, and drawers."
 	  ;; empty or planning line
 	  (forward-line 1)
 	;; a drawer, find the end
-	(re-search-forward "^[ \t]*:END:" end 'move)
+	(re-search-forward org-drawer-end-re end 'move)
 	(forward-line 1)))
     (and (re-search-forward "[^\n]" nil t) (backward-char 1))
     (point)))
-- 
1.7.12

Re: [PATCH] Fix inconsistency in drawer handling

[Nicolas Goaziou]

Hello,

Yann Hodique <yann.hodique@gmail.com> writes:

> I've noticed that drawers are not managed similarly, depending on the
> workflow. More precisely, the end-of-drawer detection is not exactly
> the same for various exports, org-element, or folding. It all boils
> down to variations around (re-search-forward ":END:").
>
> In particular, it's expected that property drawers for taskjuggler
> export will contain an :end: key (which is arguably a bad idea by
> itself), that fools some other parts of org.
>
> This patch is an attempt at making drawers handling more predictable.

This patch is good, but I'd rather hard-code the regexp within
org-element: I'm slowly trying to make this library as low-level as
possible. Do you mind changing it?

Thank you.


Regards,

-- 
Nicolas Goaziou

Re: [PATCH] Fix inconsistency in drawer handling

[Yann Hodique]

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

> This patch is good, but I'd rather hard-code the regexp within
> org-element: I'm slowly trying to make this library as low-level as
> possible. Do you mind changing it?

Hi,

thanks for the quick review. Of course I don't mind, but I'm just
curious to understand the purpose, as duplicating constants seems to
make room for future mistakes (even though in this case I guess we're
stuck with that regexp forever :)).

Do you mean you'd like to get rid of (require 'org) in org-element.el at
some point ? What about (eval-when-compile '(require 'org)) then ? Or
some (require 'org-const) ?

Thanks,

Yann.

-- 
How often it is that the angry man rages denial of what his inner self is 
telling him.

  -- "The Collected Sayings of Muad'Dib" by the Princess Irulan

Re: [PATCH] Fix inconsistency in drawer handling

[Nicolas Goaziou]

Hello,

Yann Hodique <yann.hodique@gmail.com> writes:

> thanks for the quick review. Of course I don't mind, but I'm just
> curious to understand the purpose, as duplicating constants seems to
> make room for future mistakes (even though in this case I guess we're
> stuck with that regexp forever :)).

As far as Org Element goes, there's no duplication as each regexp would
be used but once.

My point is that such constants do not make much sense. You are not
guaranteed to find a real drawer when you use

  (re-search-forward org-drawer-regexp)

even if the regexp matches: you also need to make sure that

  (eq (org-element-type (org-element-at-point)) 'drawer)

is non-nil. Worse, I think that these constants are misleading as they
sound self-sufficient. Therefore, I'd rather not support them in
Elements. But I may be too cautious on this. What do you think?

In the long run, I think that, structure-wise, org.el should handle any
action directly related to headlines, but org-element.el should be the
core library for everything else.


Regards,

-- 
Nicolas Goaziou