Hi Ihor,

Thank you for the feedback.

Ihor Radchenko writes:
>> +      ;; Apply WRITE-BACK function on edit buffer contents.
>> +      (goto-char (point-min))
>> +      (when (functionp write-back) (save-excursion (funcall write-back)))
>>         (set-marker marker nil))))
> `save-excursion' is no longer necessary here.

Done.

>>   (defun org-src--edit-element
>> @@ -1150,7 +1149,14 @@ Throw an error when not at such a table."
>>          (lambda ()
>>   	 ;; Blank lines break things, replace with a single newline.
>>   	 (while (re-search-forward "\n[ \t]*\n" nil t) (replace-match "\n"))
>> -	 ;; If within a table a newline would disrupt the structure,
>> +         ;; Trim contents.
> It would be nice to have a bit more context about the purpose in the
> comment here.

I was confused. We need only trim the beginning of the result, for the
indentation added by `org-src--contents-for-write-back'. I've added an
explanation.

>>   ...
>> -	  (skip-chars-forward " \t")
>> -          (when (or (not (eolp))                               ; not a blank line
>> -                    (and (eq (point) (marker-position marker)) ; current line
>> +          (when (or (not (eolp)) ; not an empty line
>> +                    ;; If the current line is empty, we may
>> +                    ;; want to indent it.
>> +                    (and (eq (point) (marker-position marker))
>>                            preserve-blank-line))
> Do we still need this dance with special case for current line?

Yes. This is fragile, but what it does is, if the line from which the
original edit was called was blank and not empty, and the line from
which the edit is ended is empty, we assume these two lines are the
same, and we add the common block indentation to this empty line.
This, and the pre-indentation in =org-indent-line=, make `TAB` work
to indent an empty line.

The logic in =org-edit-element= can now be simplified though, see the
third patch attached.

>> +    ;; Display tab indentation characters preceded by spaces as spaces
>> +    (unless org-src-preserve-indentation
> unless? Don't we rather want to preserve the original indentation
> alignment when `org-src-preserve-indentation' is t?
>> +      (save-excursion
>> +        (goto-char start)
>> +        (while (re-search-forward "^[ ]+\\([\t]+\\)" end t)
> Why just tabs at indentation? It would make sense to preserve width of
> all the tabs.
>> +          (let* ((b (match-beginning 1))
>> +                 (e (match-end 1))
>> +                 (s (make-string (* (- e b) native-tab-width) ? )))
>> +            (add-text-properties b e `(display ,s))))))
> Will the actual tab width be always equal to native-tab-width? What
> about tab stops? May it be more reliable to use different of column
> numbers in the src mode buffer after/before the tab?

I was trying to be conservative. When =org-src-preserve-indentation=
is t, I guess we can do the same, for consistency.

As you say, the tabs at the beginning of indentation are the only ones
we can be somewhat sure of the width, if we assume the native
indentation was done sanely (using tabs then spaces). I'm inclined to
only deal with those.

To get the true tab widths, we'd need to get the columns numbers in
yet a third different buffer, with the common indentation removed. I
don't think that's worth it.

Anyway, what I wrote doesn't deal correctly with the case where the
org indentation may use tabs. I've updated the patch: I use the value
of what should be the org indentation, and only change the display of
the following consecutive tabs.

>> @@ -318,19 +318,21 @@ This is a tab:\t.
>>                 argument2))
>>     #+END_SRC"
>>         (setq-local indent-tabs-mode t)
>> -      (let ((org-edit-src-content-indentation 2)
>> +      (let ((tab-width 8)
>> +            (org-edit-src-content-indentation 2)
> Why is setting tab-width necessary? 8 is the default value.

Removed, though I have left an instance of =(setq-local tab-width 8)=,
for clarity.

>>     #+BEGIN_SRC emacs-lisp<point>
>> -    (progn\n      (function argument1\n\t\targument2))
>> +    (progn\n      (function argument1\n    \targument2))
> I think it would be a bit more readable to convert this string into
> actual multi-line, where the alignment is visible when reading the test
> source.

I've left it this way. I think the tests using tab characters are
often written this way, so as to be understandable without whitespace
mode.

-- 
Sébastien Miquel