org-src--contents-for-write-back : preserve original major-mode, and avoid mixing tabs and spaces in org-mode buffers

org-src--contents-for-write-back : preserve original major-mode, and avoid mixing tabs and spaces in org-mode buffers

[Brent Goodrick]

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

Hi,

I found a bug in org-mode where emacs-lisp code that is in a
already-indented source block in an org-mode buffer is improperly
indented when editing it via C-c '. Take the following contrived
example emacs-lisp source code:

 1. Here is a list item with a emacs-lisp source block:
    #+BEGIN_SRC emacs-lisp :results value
      (let ((uuid "c2327c73-6da3-4421-8bda-194783a00e8f"))
        (progn
          (let ((xxx 'yyy))
            (let ((xxx 'yyy))
              (while t
                (message "infinite loop"))))))
    #+END_SRC

After C-c ', indenting it, and C-c ' again, it renders as
follows (tabs converted to spaces for this email, since I have
`indent-tabs-mode' set to t in my emacs-lisp mode, which
is the Emacs default):

 1. Here is a list item with a emacs-lisp source block:
    #+BEGIN_SRC emacs-lisp :results value
      (let ((uuid "c2327c73-6da3-4421-8bda-194783a00e8f"))
        (progn
          (let ((xxx 'yyy))
        (let ((xxx 'yyy))
          (while t
            (message "infinite loop"))))))
    #+END_SRC

Notice how the indentation looks bad due to the mixture of tabs
and spaces.

The bug is in the `org-src--contents-for-write-back' function. It
uses a temp buffer. The temp buffer's major-mode is left to be
the default, which is fundamental-mode, which knows nothing about
how to indent lisp code properly. So in the fix below, I run the
major-mode function from the original buffer. But even with that
fix, the indentation must also use spaces in order to avoid
mixing tabs and spaces in the resulting Org buffer.  My fix,
shown below, is to initialize the major-mode, and set
`indent-tabs-mode' to nil, before it runs the `write-back'
code.

See "CHANGE" comments for the changes made:

(defun org-src--contents-for-write-back ()
  "Return buffer contents in a format appropriate for write back.
        Assume point is in the corresponding edit buffer."
  (let ((indentation (or org-src--block-indentation 0))
    (preserve-indentation org-src--preserve-indentation)
    (contents (org-with-wide-buffer (buffer-string)))
    (write-back org-src--allow-write-back))
    ;; CHANGE: Save off the original mode into orig-major-mode:
    (let ((orig-major-mode major-mode))
      (with-temp-buffer
        (insert (org-no-properties contents))
        ;; CHANGE: Switch to the original mode to obtain mode-specific
indentation behavior:
        (funcall orig-major-mode)
        ;; CHANGE: Do not mix tabs and spaces during indentation:
        (setq indent-tabs-mode nil)
    (goto-char (point-min))
    (when (functionp write-back) (funcall write-back))
    (unless (or preserve-indentation (= indentation 0))
      (let ((ind (make-string indentation ?\s)))
        (goto-char (point-min))
        (while (not (eobp))
          (when (looking-at-p "[ \t]*\\S-") (insert ind))
          (forward-line))))
    (buffer-string)))))

If the above change seems correct, can someone apply it to org-mode?

Thanks,
Brent Goodrick

[-- Attachment #2: Type: text/html, Size: 3574 bytes --]

Re: org-src--contents-for-write-back : preserve original major-mode, and avoid mixing tabs and spaces in org-mode buffers

[Nicolas Goaziou]

Hello,

Brent Goodrick <bgoodr@gmail.com> writes:

> I found a bug in org-mode where emacs-lisp code that is in a
> already-indented source block in an org-mode buffer is improperly
> indented when editing it via C-c '. Take the following contrived
> example emacs-lisp source code:
>
>  1. Here is a list item with a emacs-lisp source block:
>
>     #+BEGIN_SRC emacs-lisp :results value
>       (let ((uuid "c2327c73-6da3-4421-8bda-194783a00e8f"))
>         (progn
>           (let ((xxx 'yyy))
>             (let ((xxx 'yyy))
>               (while t
>                 (message "infinite loop"))))))
>     #+END_SRC
>
>
> After C-c ', indenting it, and C-c ' again, it renders as
> follows (tabs converted to spaces for this email, since I have
> `indent-tabs-mode' set to t in my emacs-lisp mode, which
> is the Emacs default):
>
>  1. Here is a list item with a emacs-lisp source block:
>
>     #+BEGIN_SRC emacs-lisp :results value
>       (let ((uuid "c2327c73-6da3-4421-8bda-194783a00e8f"))
>         (progn
>           (let ((xxx 'yyy))
>         (let ((xxx 'yyy))
>           (while t
>             (message "infinite loop"))))))
>     #+END_SRC
>
> Notice how the indentation looks bad due to the mixture of tabs
> and spaces.

Indeed. Thank you.

> The bug is in the `org-src--contents-for-write-back' function. It
> uses a temp buffer. The temp buffer's major-mode is left to be
> the default, which is fundamental-mode, which knows nothing about
> how to indent lisp code properly.

And it doesn't need to. This function doesn't care about the code, but
indents it rigidly according to original source block.  IOW, I don't
think changing the major mode is required.

> So in the fix below, I run the major-mode function from the original
> buffer. But even with that fix, the indentation must also use spaces
> in order to avoid mixing tabs and spaces in the resulting Org buffer.

Why do you think it is a good thing that tabs and spaces shouldn't be
mixed. For example, imagine that the source code requires
`indent-tabs-mode' being non-nil, but Org source buffer indentation is
space only, i.e., with `indent-tabs-mode' being nil.

Shouldn't the resulting block be indented with spaces from column 0 to
block boundaries' indentation, and then follow with space indentation?

WDYT?

Regards,

-- 
Nicolas Goaziou

Re: org-src--contents-for-write-back : preserve original major-mode, and avoid mixing tabs and spaces in org-mode buffers

[Brent Goodrick]

On Sat, Apr 22, 2017 at 1:41 AM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
>
> > The bug is in the `org-src--contents-for-write-back' function. It
> > uses a temp buffer. The temp buffer's major-mode is left to be
> > the default, which is fundamental-mode, which knows nothing about
> > how to indent lisp code properly.
>
> And it doesn't need to. This function doesn't care about the code, but
> indents it rigidly according to original source block.  IOW, I don't
> think changing the major mode is required.
>
> > So in the fix below, I run the major-mode function from the original
> > buffer. But even with that fix, the indentation must also use spaces
> > in order to avoid mixing tabs and spaces in the resulting Org buffer.
>
> Why do you think it is a good thing that tabs and spaces shouldn't be
> mixed. For example, imagineh that the source code requires
> `indent-tabs-mode' being non-nil, but Org source buffer indentation is
> space only, i.e., with `indent-tabs-mode' being nil.

Thanks for looking into this!

In the current implementation of org-src--contents-for-write-back, the
`with-temp-buffer` uses fundamental-mode.  Later, while inside that
temp buffer, spaces are inserted in to indent the entire source block
over to where it needs to be (in my original post, notice that I have
the source block within a list item so the source block needs to be
aligned properly under that list item, no matter to what depth that
list item is). It is in mode hook functions that certain changes to
indentation can occur, so that is why I'm switching into that
mode. But that is not enough: In order for the text to be aligned
properly inside the org mode buffer after indentation, there cannot be
a mix of tabs and spaces, as shown in my original post. IIRC,
`indent-to' is called within the `write-back' function, and
`indent-to' is affected by the `indent-tabs-mode' value, which by
default in emacs lisp mode buffers, is t.

You might try my implementation both without the change to
`indent-tabs-mode' to see how improperly indented the resulting source
block looks.

>
> Shouldn't the resulting block be indented with spaces from column 0 to
> block boundaries' indentation, and then follow with space indentation?
>

Yes, if I understand what you are meaning. In fact, I think that is,
in effect, what my replacement function is doing.  Basically the
bottom line is that you can't mix tabs and spaces in the final Org
buffer and have it look correctly indented in that buffer, and the
change to indent-tabs-mode to nil will be buffer local inside that
with-temp-buffer so nothing is affected in any other buffer.

For your reference, below is my replacement function that has
elaborated comments that hopefully clarify things a bit more:

(defun org-src--contents-for-write-back-fix-indentation ()
  "Return buffer contents in a format appropriate for write back.
        Assume point is in the corresponding edit buffer."
  (let ((indentation (or org-src--block-indentation 0))
        (preserve-indentation org-src--preserve-indentation)
        (contents (org-with-wide-buffer (buffer-string)))
        (write-back org-src--allow-write-back))
    ;; --- BEGIN CHANGES FROM ORIGINAL DEFINITION ---
    ;;
    ;; Save off the original mode into orig-major-mode:
    ;;
    (let ((orig-major-mode major-mode))
      (with-temp-buffer
        ;;
        ;; Insert the contents as was done before:
        ;;
        (insert (org-no-properties contents))
        ;;
        ;; First change: Switch to the original mode for indentation by
        ;; switching its mode to be the original one. This is because that mode
        ;; handles mode-specific indentation behavior:
        ;;
        (funcall orig-major-mode)
        ;;
        ;; Second change: Do not use tabs here. If the mode being switched to
        ;; has indent-tabs-mode set to t, that is fine for separate buffers, but
        ;; for when org source blocks are shown in Org buffers, mixing tabs and
        ;; spaces will mess up the view (see this for emacs lisp code blocks
        ;; when indent-tabs-mode is set to t) when write-back calls `indent-to'.
        ;;
        ;; The alternative is to require everyone to set indent-tabs-mode to nil
        ;; in their mode hooks for all modes used in Org mode, but that seems
        ;; slightly heavy-handed.
        ;;
        (setq indent-tabs-mode nil)
        ;; --- END CHANGES FROM ORIGINAL DEFINITION ---
        (goto-char (point-min))
        (when (functionp write-back) (funcall write-back))
        (unless (or preserve-indentation (= indentation 0))
          (let ((ind (make-string indentation ?\s)))
            (goto-char (point-min))
            (while (not (eobp))
              (when (looking-at-p "[ \t]*\\S-") (insert ind))
              (forward-line))))
        (buffer-string)))))

I am curious to see if there is a better fix that what I've coded up above.

Thanks again for your help,
-Brent

Re: org-src--contents-for-write-back : preserve original major-mode, and avoid mixing tabs and spaces in org-mode buffers

[Nicolas Goaziou]

Hello,

Brent Goodrick <bgoodr@gmail.com> writes:

> In the current implementation of org-src--contents-for-write-back, the
> `with-temp-buffer` uses fundamental-mode.

Correct.

> Later, while inside that temp buffer, spaces are inserted in to indent
> the entire source block over to where it needs to be (in my original
> post, notice that I have the source block within a list item so the
> source block needs to be aligned properly under that list item, no
> matter to what depth that list item is).

Correct.

> It is in mode hook functions that certain changes to indentation can
> occur, so that is why I'm switching into that mode.

This is where I don't follow you. At this point, indentation changes are
tailored for the source, i.e., Org, buffer. Special indentation rules
from the source block mode do not apply here.

> But that is not enough: In order for the text to be aligned properly
> inside the org mode buffer after indentation, there cannot be a mix of
> tabs and spaces, as shown in my original post. IIRC, `indent-to' is
> called within the `write-back' function, and `indent-to' is affected
> by the `indent-tabs-mode' value, which by default in emacs lisp mode
> buffers, is t.

You cannot set `indent-tabs-mode' to nil and discard user's
configuration. What if I want it to be non-nil in Org buffers?

Another option is to delete any leading indentation and indent it again
according to `indent-tabs-mode' value in source buffer.

WDYT?

Regards,

-- 
Nicolas Goaziou

Re: org-src--contents-for-write-back : preserve original major-mode, and avoid mixing tabs and spaces in org-mode buffers

[Brent Goodrick]

On Thu, Apr 27, 2017 at 4:04 PM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
>
> Hello,
>
> Brent Goodrick <bgoodr@gmail.com> writes:
>
> > In the current implementation of org-src--contents-for-write-back, the
> > `with-temp-buffer` uses fundamental-mode.
>
> Correct.
>
> > Later, while inside that temp buffer, spaces are inserted in to indent
> > the entire source block over to where it needs to be (in my original
> > post, notice that I have the source block within a list item so the
> > source block needs to be aligned properly under that list item, no
> > matter to what depth that list item is).
>
> Correct.
>
> > It is in mode hook functions that certain changes to indentation can
> > occur, so that is why I'm switching into that mode.
>
> This is where I don't follow you. At this point, indentation changes are
> tailored for the source, i.e., Org, buffer. Special indentation rules
> from the source block mode do not apply here.

I do not understand what is meant by "tailored for the source" which
is the Org buffer. All of the indentation changes being made here are
within the temporary buffer created by with-temp-buffer, which is
using fundamental-mode which is not the same as either emacs-lisp-mode
or org-mode. That is the wrong mode to be in when running `indent-line-to`
function since it is that particular editing mode that the user has
control over the `indent-tabs-mode` variable (typically from mode hook
functions). So I conclude that the temporary buffer, at that point in
execution, has to be in the mode of the language used when indenting.

> > But that is not enough: In order for the text to be aligned properly
> > inside the org mode buffer after indentation, there cannot be a mix of
> > tabs and spaces, as shown in my original post. IIRC, `indent-to' is
> > called within the `write-back' function, and `indent-to' is affected
> > by the `indent-tabs-mode' value, which by default in emacs lisp mode
> > buffers, is t.
>
> You cannot set `indent-tabs-mode' to nil and discard user's
> configuration. What if I want it to be non-nil in Org buffers?

I agree with that. I now see that what I proposed earlier is a not a
solution for that reason. So I have a further suggested change below.

Also I mistakenly referred to `indent-to` when the function is actually
`indent-line-to`.

> Another option is to delete any leading indentation and indent it again
> according to `indent-tabs-mode' value in source buffer.

By "in source buffer", do you mean that after the text is inserted
into the Org mode buffer, the code then reindents it again, while the
current mode in that buffer is org-mode (i.e., no longer inside the
temporary buffer, which is fundamental-mode)? If that is the
interpretation, then how is Org mode to know about the syntax of emacs
lisp code? I think it won't and it will indent the emacs lisp code
without proper handling of parens. In fact I played with that a bit
and it is also not going to work (yields left justified text since
fundamental mode knows nothing the parens in Emacs lisp).

Here is my somewhat improved fix. I'm still switching into the
original language mode inside the temp buffer.

(defun org-src--contents-for-write-back ()
  "Return buffer contents in a format appropriate for write back.
        Assume point is in the corresponding edit buffer."
  (let ((indentation (or org-src--block-indentation 0))
        (preserve-indentation org-src--preserve-indentation)
        (contents (org-with-wide-buffer (buffer-string)))
        (write-back org-src--allow-write-back))
    (let ((orig-major-mode major-mode))
      (with-temp-buffer
        (insert (org-no-properties contents))
        ;; CHANGE: Switch into the mode of the language that was being edited so
        ;; that `indent-tabs-mode` will be respected:
        (funcall orig-major-mode)
        (goto-char (point-min))
        (when (functionp write-back)
          ;; CHANGE: Hack to allow stepping through through this function in
          ;; edebug: I commented out this line:
          ;;
          ;;   (funcall write-back)
          ;;
          ;; and inlined the value of write-back here and reindented for
          ;; readability:
          (when (> org-edit-src-content-indentation 0)
            (while (not (eobp))
              (unless (looking-at "[     ]*$")
                (indent-line-to (+ (org-get-indentation)
org-edit-src-content-indentation)))
              (forward-line))))
        ;; CHANGE: Switch into org-mode so that `indent-tabs-mode` in org mode
        ;; buffers will also be respected:
        (org-mode)
        (unless (or preserve-indentation (= indentation 0))
          (let ((ind (make-string indentation ?\s)))
            (goto-char (point-min))
            (while (not (eobp))
              (when (looking-at-p "[ \t]*\\S-")
                ;; CHANGE: Replace this:
                ;;   (insert ind)
                ;; with this:
                (indent-line-to (+ (org-get-indentation) indentation)))
              (forward-line))))
        (buffer-string)))))

I've tested this with org-mode's indent-tabs-mode set to t and nil and
it seems to display correctly.

Hence, I'm now thinking the possible bug here was the "(insert ind)"
in the last change above, in addition to the need to switch modes.

WDYT?

Thanks,
Brent

P.S.: Thanks for your help on this!

Re: org-src--contents-for-write-back : preserve original major-mode, and avoid mixing tabs and spaces in org-mode buffers

[Nicolas Goaziou]

Hello,

Brent Goodrick <bgoodr@gmail.com> writes:

> I do not understand what is meant by "tailored for the source" which
> is the Org buffer. All of the indentation changes being made here are
> within the temporary buffer created by with-temp-buffer, which is
> using fundamental-mode which is not the same as either emacs-lisp-mode
> or org-mode. That is the wrong mode to be in when running `indent-line-to`
> function since it is that particular editing mode that the user has
> control over the `indent-tabs-mode` variable (typically from mode hook
> functions). So I conclude that the temporary buffer, at that point in
> execution, has to be in the mode of the language used when indenting.

This is not necessary. `org-src--contents-for-write-back' merely adds up
indentation to the existing one. In particular, it doesn't re-indent
lines. The indentation being added depends on Org mode (was the block in
a list? Is it a src block where special indentation rules apply...), not
on the major mode from the edit buffer.

However, you have a point, as we need to somehow retain the values of
`indent-tabs-mode' and `tab-width' from Org source buffer, since those
may differ from the ones used in the temporary buffer.

Also, calling `org-mode' again in the temporary buffer, in addition to
being slow, wouldn't preserve, e.g., local values from the source
buffer. So I think the best thing to do is to store `indent-tabs-mode'
and `tab-width' from source buffer and apply them back into the
temporary buffer.

I committed a change along those lines, along with tests, in the maint
branch. Does it fix the issues you were encountering?

Regards,

-- 
Nicolas Goaziou

Re: org-src--contents-for-write-back : preserve original major-mode, and avoid mixing tabs and spaces in org-mode buffers

[Brent Goodrick]

On Sat, Apr 29, 2017 at 2:59 AM, Nicolas Goaziou <mail@nicolasgoaziou.fr> wrote:
> Hello,
>
> Brent Goodrick <bgoodr@gmail.com> writes:
>
>> I do not understand what is meant by "tailored for the source" which
>> is the Org buffer. All of the indentation changes being made here are
>> within the temporary buffer created by with-temp-buffer, which is
>> using fundamental-mode which is not the same as either emacs-lisp-mode
>> or org-mode. That is the wrong mode to be in when running `indent-line-to`
>> function since it is that particular editing mode that the user has
>> control over the `indent-tabs-mode` variable (typically from mode hook
>> functions). So I conclude that the temporary buffer, at that point in
>> execution, has to be in the mode of the language used when indenting.
>
> This is not necessary. `org-src--contents-for-write-back' merely adds up
> indentation to the existing one.

Agreed.

> In particular, it doesn't re-indent
> lines. The indentation being added depends on Org mode (was the block in
> a list? Is it a src block where special indentation rules apply...), not
> on the major mode from the edit buffer.
>
> However, you have a point, as we need to somehow retain the values of
> `indent-tabs-mode' and `tab-width' from Org source buffer, since those
> may differ from the ones used in the temporary buffer.
>
> Also, calling `org-mode' again in the temporary buffer, in addition to
> being slow,

Yes, I wondered about the performance impact. Agreed.

> wouldn't preserve, e.g., local values from the source
> buffer. So I think the best thing to do is to store `indent-tabs-mode'
> and `tab-width' from source buffer and apply them back into the
> temporary buffer.
>
> I committed a change along those lines, along with tests, in the maint
> branch. Does it fix the issues you were encountering?

It works splendidly!

Thanks Nicolas.
--Brent