Re: Lexical binding bug in org-list.el?

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> LGTM.  I’ve probably met my quota of org-related fun for the day (see
> below...), but implementing this in terms of elements will be my next
> org-list related task.

What is "this"?

Unfortunately org-list and Elements do not play nicely with each other
at the moment, because of org-struct, i.e., because of lists outside Org
buffers. Hence the `org-list-struct' and `org-element--list-struct'
duplication.

> It’s very lightly tested so far.  I basically just used the following
> snippet as a test case: put it in an org-mode buffer, put your cursor
> somewhere inside the list, and M-: (org-list-to-subtree2)

OK. Some comments follow.

> OK.  Don’t hesitate to ask if there’s some way we can help, of course.

I'll send a patch once I have the time to do this (depending on the bug
reports). I'd appreciate if you could update Babel accordingly.

> +(defun org-list--partial-parse-contents (parse)
> +  "Get the actual contents of a partial org-mode parse.
> +
> +Specifically, when parsing a piece of text smaller than a
> +headline, `org-element-parse-buffer' wraps its result with a
> +dummy `section' element, as well as the standard `org-data'
> +wrapper.  This function removes these, returning a list of
> +org-elements.
> +
> +TODO: maybe this needs a more general name."
> +  (org-element-contents
> +   ;; strip the org-data element
> +   (nth 0 (org-element-contents
> +	   ;; and the section element
> +	   parse))))

Actually, I often wonder if `section' should appear at all in the parse
tree. I mean, it is an important feature for export, but it could be
added automatically at export time (i.e., in `org-export-as', like many
other tree transforms) without cluttering the original parse tree.

Also, here, you probably really want

  (org-element-map parse org-element-all-elements #'identity nil t)

since you're going to handle a single element anyway.

> +(defun org-list--split-first-line (contents)
> +  "Remove the first line of text from an org-element item.
> +
> +CONTENTS are the contents of the item org-element: at least a
> +paragraph followed by zero or more other elements.
> +
> +Returns a cons of the first textual line and a list of
> +org-elements representing the structure of the item minus this
> +line.
> +
> +TODO: is the first daughter of an item always a paragraph?"

Absolutely not. E.g.,

   - 

     | a |

   - item 2


> +  (let ((graf (nth 0 contents)))
> +    (unless (eq (org-element-type graf) 'paragraph)
> +      (error "`org-list--split-first-line' got confused"))

See above.

> +    (goto-char (org-element-property :begin graf))
> +    (let* ((eol (point-at-eol))

Use the original! -> `line-end-position'. This one is a compatibility
alias for XEmacs.

> +	   (end (org-element-property :end graf))
> +	   (first-line (buffer-substring-no-properties (point) eol)))
> +      (if (> (1+ eol) end)
> +	  ;; One line paragraph: it becomes the entirety of the
> +	  ;; headline, and we remove it from contents
> +	  (setq contents (cdr contents))
> +	;; Multi-line paragraph: narrow the buffer to lines 2-N, parse
> +	;; them, and set them as the contents of the paragraph.
> +	(save-restriction
> +	  (widen)
> +	  (narrow-to-region (1+ eol) end)
> +	  (org-element-set-contents graf
> +				    (org-list--partial-parse-contents
> +				     ;; TODO: We're playing a trick on
> +				     ;; the parser here.  AFAICT, the
> +				     ;; parse does not rely on the
> +				     ;; cache.  But maybe we should
> +				     ;; let org-element-use-cache to
> +				     ;; nil around this call, in case
> +				     ;; that changes in the future.
> +				     (org-element-parse-buffer)))))

It shouldn't change. Also, there is `org-element-copy' if you need to
alter an element obtained through `org-element-at-point'.

> +      (cons first-line contents))))
> +
> +(defun org-list--item-to-headline (item level)
> +  "Convert an org-element list item to a headline.
> +
> +The first line of the list item becomes the "
> +  (unless (eq (car item) 'item)
> +    (error "`org-list--item-to-headline' expects an item argument"))
> +  (let* ((r (org-list--split-first-line (org-element-contents item)))
> +	 (title (car r))
> +	 (other-contents (cdr r)))
> +    (list 'headline
> +	  `(:level ,level
> +		   ,@(when (eq (org-element-property :checkbox item) 'on)
> +		       (list :todo-keyword
> +			     ;; TODO: how to fish the approporiate
> +			     ;; value out of org-todo-keywords?
> +			     "TODO"))

I'd try

  (car org-not-done-keywords)

However, it returns nil if we're parsing a non-Org buffer.

> +		   :title ,title)
> +	  (mapcar (lambda (x) (if (eq (org-element-type x) 'plain-list)
> +				  (org-list--to-headlines x (1+ level))
> +				x))
> +		  other-contents))))
> +
> +(defun org-list--to-headlines (list level)
> +  (unless (eq (car list) 'plain-list)
> +    (error "`org-list-to-subtree' expects a plain-list argument"))
> +  (mapcar (lambda (x) (org-list--item-to-headline x level))
> +	  (org-element-contents list)))
> +
> +(defun org-list-to-subtree2 ()
> +  (let* ((e (org-element-at-point))
> +	 (l (org-element-lineage e))
> +	 (list (cl-find-if (lambda (x) (eq (org-element-type x) 'plain-list))
> +			   (nreverse l)))

Also,

  ;; Find the top-most plain-list containing element at point.
  (org-element-map (nreverse l) 'plain-list #'identity nil t)

> +	 (level (org-reduced-level (or (org-current-level) 0)))
> +	 (begin (org-element-property :begin list))
> +	 (end (org-element-property :end list))
> +	 (parse (save-restriction
> +		  (widen)
> +		  (narrow-to-region begin end)
> +		  (org-element-parse-buffer)))
> +	 (new-subtree (org-list--to-headlines
> +		       (nth 0 (org-list--partial-parse-contents parse))
> +		       level)))
> +    (goto-char end)
> +    ;; Don't eat the blank lines after the list.
> +    (skip-chars-backward " \n\t\f")

Why "\f"?

> +    (delete-region begin (point))
> +    (insert (org-element-interpret-data new-subtree))))

Note that current implementation is slightly smarter since it handles
`org-blank-before-new-entry'.

One issue here is that we are going to duplicate some code since
`org-list-parse-list' is going to do slightly the same but with
a different representation.


Regards,

-- 
Nicolas Goaziou