Re: (Feature Request) have org-edit-special work inside non-environment LaTeX blocks, i.e. \( \) and \[ \]

[Nicolas Goaziou]

Hello,

TEC <address@hidden> writes:

> ---
>  lisp/org-src.el | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  lisp/org.el     |  1 +
>  2 files changed, 46 insertions(+)

Thank you. It looks fine, I will only be nitpicking.

> +(defun org-edit-latex-fragment ()
> +  "Edit LaTeX fragment at point."
> +  (interactive)
> +  (let* ((context (org-element-context))
> +      (_ (unless (and (eq (org-element-type context) 'latex-fragment)
> +                      (org-src--on-datum-p context))
> +           (user-error "Not on a LaTeX fragment")))

This is a fancy way to use a let-binding. I suggest to mimic what is
done elsewhere, i.e., first bind `context', then check if we're at
a LaTeX fragment, then bind the rest.

> +    ;; Grab the LaTeX fragment for propertization

Missing full stop at the end of the comment.

> +      (contents (buffer-substring-no-properties
> +                 (org-element-property :begin context)
> +                 (- (org-element-property :end context)
> +                    (org-element-property :post-blank context))))
> +      (delim-length (if (string-match "\\$[^$]" (substring contents 0 2))

Use 

    (string-match "\\`\\$[^$]" contents)

instead.

or, arguably better,

    (string-match (rx (seq string-start "$" (not (any "$")))) 
                  contents)

> +                        1 2)))
> +    ;; make the LaTeX deliminators read-only

Missing initial capital and final full stop.

> +    (add-text-properties
> +       0 delim-length
> +       '(read-only "Cannot edit LaTeX deliminator" front-sticky t 
> rear-nonsticky t)
> +       contents)
> +    (add-text-properties
> +       (- (length contents) delim-length)
> +       (length contents)
> +       '(read-only "Cannot edit LaTeX deliminator" front-sticky nil 
> rear-nonsticky nil)
> +       contents)

You could factor out (length contents) so it is only called once.

> +    (org-src--edit-element
> +     context
> +     (org-src--construct-edit-buffer-name (buffer-name) "LaTeX fragment")
> +     (org-src-get-lang-mode "latex")
> +     (lambda ()
> +       ;; Blank lines break things, replace with a single newline

See above.

> +       (while (re-search-forward "\n[ \t]*\n" nil t) (replace-match "\n"))
> +       ;; If within a table a newline would disrupt the structure,

This comment is truncated.

> +       (goto-char (point-min))
> +       (when (org-element-lineage context '(table-cell))
> +      (while (search-forward "\n" nil t) (replace-match " ")))
> +       )

Don't leave parenthesis alone.

Also, make sure your indentation is right, e.g., using M-q on the
definition.

You also need to add a proper commit message and use `git format-patch',
and an entry in ORG-NEWS (probably in Miscellaneous part).

Bonus points if you can add some tests in
"testing/lisp/test-org-src.el".

Could you remind me if you signed the FSF papers already?

Regards,

-- 
Nicolas Goaziou