Re: Re: PATCH: proposed improvements to org-src-mode

[Dan Davison <davison@stats.ox.ac.uk>]

Carsten Dominik <carsten.dominik@gmail.com> writes:

> Hi Dan,
>
> OK, I have now applied the patch.  If you don't mind, could
> you please double-check the commit?

[Re.: commit 4b6988bf36cb458c9d113ee4332e016990c1eb04 ]

Thanks Carsten, I looked over the commit and it looked good. However
after using it a bit over the last week I have discovered two bugs and a
code issue. These follow below, together with proposed patches. There
are 3 separate patches. Just in case you want to accept all 3 of them,
I've created a single patch against current git right at the bottom of
the email.

But first, there are a couple of aesthetic choices being made here that
others might have better ideas for than the ones I've come up with so
far. Firstly, how should we name edit buffers? At the moment we're using
my proposal of (actually I just added spaces inside the square brackets)

(concat "*Org Src " org-buffer-name "[ " lang " ]*")

e.g.

"*Org Src testing.org[ python ]*"

Secondly, how shall we name edit buffers for Fixed Width / ASCII drawing
regions? The patch below uses

(concat "*Org Src " org-buffer-name "[ " "Fixed Width" " ]*")

e.g.

"*Org Src testing.org[ Fixed Width ]*"

(Maybe "Literal" would be better? Neither seems perfect.)

Now for the bugs & patches.

* Bug 1: C-x s switches active buffer
  1. open two edit buffers simultaneously and modify them
  2. now issue C-x s in one of the edit buffers
  3. when it comes to saving the second edit buffer, it will steal focus.

Would an appropriate solution be to wrap the body of org-edit-src-save
in a save-window-excursion, as in this patch? I've tried it quickly
and it seems to have the desired effect.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/org-src.el b/lisp/org-src.el
index fbf395f..df09f39 100644
--- a/lisp/org-src.el
+++ b/lisp/org-src.el
@@ -474,14 +474,15 @@ the language, a switch telling of the content should be in a single line."
 (defun org-edit-src-save ()
   "Save parent buffer with current state source-code buffer."
   (interactive)
-  (let ((p (point)) (m (mark)) msg)
-    (org-edit-src-exit)
-    (save-buffer)
-    (setq msg (current-message))
-    (org-edit-src-code)
-    (push-mark m 'nomessage)
-    (goto-char (min p (point-max)))
-    (message (or msg ""))))
+  (save-window-excursion
+    (let ((p (point)) (m (mark)) msg)
+      (org-edit-src-exit)
+      (save-buffer)
+      (setq msg (current-message))
+      (org-edit-src-code)
+      (push-mark m 'nomessage)
+      (goto-char (min p (point-max)))
+      (message (or msg "")))))
--8<---------------cut here---------------end--------------->8---


* Bug 2: org-edit-src-find-buffer fails to find buffer
  - Use C-c ' to switch to an edit buffer and make a modification
  - Return to the org buffer without killing edit buffer
  - Try C-c ' on the same code block
  - You are now in a new edit buffer instead of returning to the original one!

  This is because it had not been updated to reflect the new edit buffer
  naming scheme. I propose that we ensure consistency by creating a new
  function org-src-construct-edit-buffer-name which takes the parent
  buffer name and the language and generates the edit buffer name, as in
  this patch.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/org-src.el b/lisp/org-src.el
index df09f39..b0932c1 100644
--- a/lisp/org-src.el
+++ b/lisp/org-src.el
@@ -185,7 +185,8 @@ the edited version."
                (org-delete-overlay org-edit-src-overlay)))
          (kill-buffer buffer))
        (setq buffer (generate-new-buffer
-                     (concat "*Org Src " (file-name-nondirectory buffer-file-name) "[" lang "]*")))
+                     (org-src-construct-edit-buffer-name
+                      (file-name-nondirectory buffer-file-name) lang)))
        (setq ovl (org-make-overlay beg end))
        (org-overlay-put ovl 'face 'secondary-selection)
        (org-overlay-put ovl 'edit-buffer buffer)
@@ -231,13 +232,17 @@ the edited version."
     (if buf (switch-to-buffer buf)
       (error "Something is wrong here"))))
 
+(defun org-src-construct-edit-buffer-name (org-buffer-name lang)
+  "Construct the buffer name for a source editing buffer"
+  (concat "*Org Src " org-buffer-name "[ " lang " ]*"))
+
 (defun org-edit-src-find-buffer (beg end)
   "Find a source editing buffer that is already editing the region BEG to END."
   (catch 'exit
     (mapc
      (lambda (b)
        (with-current-buffer b
-        (if (and (string-match "\\`*Org Edit " (buffer-name))
+        (if (and (string-match "\\`*Org Src " (buffer-name))
                  (local-variable-p 'org-edit-src-beg-marker (current-buffer))
                  (local-variable-p 'org-edit-src-end-marker (current-buffer))
                  (equal beg org-edit-src-beg-marker)
@@ -289,7 +294,9 @@ the fragment in the Org-mode buffer."
            (if (boundp 'org-edit-src-overlay)
                (org-delete-overlay org-edit-src-overlay)))
          (kill-buffer buffer))
-       (setq buffer (generate-new-buffer "*Org Edit Src Example*"))
+       (setq buffer (generate-new-buffer
+                     (org-src-construct-edit-buffer-name
+                      (file-name-nondirectory buffer-file-name) "Fixed Width")))
        (setq ovl (org-make-overlay beg end))
        (org-overlay-put ovl 'face 'secondary-selection)
        (org-overlay-put ovl 'edit-buffer buffer)
--8<---------------cut here---------------end--------------->8---

* Code issue:
  I can't work out why I was using (file-name-nondirectory
  buffer-file-name) in order to construct the edit buffer name instead
  of (buffer-name). Unless someone can think of a reason for this,
  please apply the following patch.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/org-src.el b/lisp/org-src.el
index b0932c1..5f21dfd 100644
--- a/lisp/org-src.el
+++ b/lisp/org-src.el
@@ -185,8 +185,7 @@ the edited version."
                (org-delete-overlay org-edit-src-overlay)))
          (kill-buffer buffer))
        (setq buffer (generate-new-buffer
-                     (org-src-construct-edit-buffer-name
-                      (file-name-nondirectory buffer-file-name) lang)))
+                     (org-src-construct-edit-buffer-name (buffer-name) lang)))
        (setq ovl (org-make-overlay beg end))
        (org-overlay-put ovl 'face 'secondary-selection)
        (org-overlay-put ovl 'edit-buffer buffer)
@@ -295,8 +294,7 @@ the fragment in the Org-mode buffer."
                (org-delete-overlay org-edit-src-overlay)))
          (kill-buffer buffer))
        (setq buffer (generate-new-buffer
-                     (org-src-construct-edit-buffer-name
-                      (file-name-nondirectory buffer-file-name) "Fixed Width")))
+                     (org-src-construct-edit-buffer-name (buffer-name) "Fixed Width")))
        (setq ovl (org-make-overlay beg end))
        (org-overlay-put ovl 'face 'secondary-selection)
        (org-overlay-put ovl 'edit-buffer buffer)
--8<---------------cut here---------------end--------------->8---


* Single unified patch

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/org-src.el b/lisp/org-src.el
index fbf395f..5f21dfd 100644
--- a/lisp/org-src.el
+++ b/lisp/org-src.el
@@ -185,7 +185,7 @@ the edited version."
 		(org-delete-overlay org-edit-src-overlay)))
 	  (kill-buffer buffer))
 	(setq buffer (generate-new-buffer
-		      (concat "*Org Src " (file-name-nondirectory buffer-file-name) "[" lang "]*")))
+		      (org-src-construct-edit-buffer-name (buffer-name) lang)))
 	(setq ovl (org-make-overlay beg end))
 	(org-overlay-put ovl 'face 'secondary-selection)
 	(org-overlay-put ovl 'edit-buffer buffer)
@@ -231,13 +231,17 @@ the edited version."
     (if buf (switch-to-buffer buf)
       (error "Something is wrong here"))))
 
+(defun org-src-construct-edit-buffer-name (org-buffer-name lang)
+  "Construct the buffer name for a source editing buffer"
+  (concat "*Org Src " org-buffer-name "[ " lang " ]*"))
+
 (defun org-edit-src-find-buffer (beg end)
   "Find a source editing buffer that is already editing the region BEG to END."
   (catch 'exit
     (mapc
      (lambda (b)
        (with-current-buffer b
-	 (if (and (string-match "\\`*Org Edit " (buffer-name))
+	 (if (and (string-match "\\`*Org Src " (buffer-name))
 		  (local-variable-p 'org-edit-src-beg-marker (current-buffer))
 		  (local-variable-p 'org-edit-src-end-marker (current-buffer))
 		  (equal beg org-edit-src-beg-marker)
@@ -289,7 +293,8 @@ the fragment in the Org-mode buffer."
 	    (if (boundp 'org-edit-src-overlay)
 		(org-delete-overlay org-edit-src-overlay)))
 	  (kill-buffer buffer))
-	(setq buffer (generate-new-buffer "*Org Edit Src Example*"))
+	(setq buffer (generate-new-buffer
+		      (org-src-construct-edit-buffer-name (buffer-name) "Fixed Width")))
 	(setq ovl (org-make-overlay beg end))
 	(org-overlay-put ovl 'face 'secondary-selection)
 	(org-overlay-put ovl 'edit-buffer buffer)
@@ -474,14 +479,15 @@ the language, a switch telling of the content should be in a single line."
 (defun org-edit-src-save ()
   "Save parent buffer with current state source-code buffer."
   (interactive)
-  (let ((p (point)) (m (mark)) msg)
-    (org-edit-src-exit)
-    (save-buffer)
-    (setq msg (current-message))
-    (org-edit-src-code)
-    (push-mark m 'nomessage)
-    (goto-char (min p (point-max)))
-    (message (or msg ""))))
+  (save-window-excursion
+    (let ((p (point)) (m (mark)) msg)
+      (org-edit-src-exit)
+      (save-buffer)
+      (setq msg (current-message))
+      (org-edit-src-code)
+      (push-mark m 'nomessage)
+      (goto-char (min p (point-max)))
+      (message (or msg "")))))
 
 (defun org-src-mode-configure-edit-buffer ()
   (when org-edit-src-from-org-mode
--8<---------------cut here---------------end--------------->8---


Dan


>
> Thanks.
>
> - Carsten
>
> On Aug 28, 2009, at 4:36 AM, Dan Davison wrote:
>
>> Carsten Dominik <carsten.dominik@gmail.com> writes:
>>
>>> Hi Dan,
>>>
>>> I am now finally looking at your patch.
>>>
>>> A few questions:
>>>
>>> On Aug 19, 2009, at 1:03 PM, Dan Davison wrote:
>>>
>>>> Dan Davison <davison@stats.ox.ac.uk> writes:
>>>>
>>>>> Carsten Dominik <carsten.dominik@gmail.com> writes:
>>>>>
>>>>>> Hi Dan,
>>>>>>
>>>>>> thank you for studying and describing these issues, and for
>>>>>> proposing
>>>>>> a patch.
>>>>
>>>> I have noticed a bug in the patch I proposed: the configuration of
>>>> the
>>>> edit buffer for saving must be done only after C-c ', and not for
>>>> example when entering org-src-mode during HTML export. Here's the
>>>> revised patch (again, assuming org-edit-src-from-org-mode is a valid
>>>> test that C-c ' has just been done).
>>>>
>>>> Dan
>>>>
>>>> --8<---------------cut here---------------start------------->8---
>>>> diff --git a/lisp/org-src.el b/lisp/org-src.el
>>>> index 2a6c087..6ba58f5 100644
>>>> --- a/lisp/org-src.el
>>>> +++ b/lisp/org-src.el
>>>> @@ -113,7 +113,6 @@ but which mess up the display of a snippet in
>>>> Org exported files.")
>>>>
>>>> (defvar org-src-mode-map (make-sparse-keymap))
>>>> (define-key org-src-mode-map "\C-c'" 'org-edit-src-exit)
>>>> -(define-key org-src-mode-map "\C-x\C-s" 'org-edit-src-save)
>>>> (defvar org-edit-src-force-single-line nil)
>>>> (defvar org-edit-src-from-org-mode nil)
>>>> (defvar org-edit-src-picture nil)
>>>> @@ -168,7 +167,8 @@ the edited version."
>>>> 	    (if (boundp 'org-edit-src-overlay)
>>>> 		(org-delete-overlay org-edit-src-overlay)))
>>>> 	  (kill-buffer buffer))
>>>> -	(setq buffer (generate-new-buffer "*Org Edit Src Example*"))
>>>> +	(setq buffer (generate-new-buffer
>>>> +		      (concat "*Org Src " (file-name-nondirectory
>>>> buffer-file-
>>>> name) "[" lang "]*")))
>>>> 	(setq ovl (org-make-overlay beg end))
>>>> 	(org-overlay-put ovl 'face 'secondary-selection)
>>>> 	(org-overlay-put ovl 'edit-buffer buffer)
>>>> @@ -186,8 +186,7 @@ the edited version."
>>>> 				'(display nil invisible nil intangible nil))
>>>> 	(org-do-remove-indentation)
>>>> 	(let ((org-inhibit-startup t))
>>>> -	  (funcall lang-f)
>>>> -	  (org-src-mode))
>>>
>>> You are moving the call to org-src-mode only so that you have
>>> org-edit-
>>> src-from-org defined in the hook, right?
>>
>> I also use the value of org-edit-src-beg-marker in the hook.
>>
>>>
>>>> +	  (funcall lang-f))
>>>> 	(set (make-local-variable 'org-edit-src-force-single-line) single)
>>>> 	(set (make-local-variable 'org-edit-src-from-org-mode) org-mode-p)
>>>> 	(when lfmt
>>>> @@ -201,6 +200,7 @@ the edited version."
>>>> 	(org-set-local 'org-edit-src-end-marker end)
>>>> 	(org-set-local 'org-edit-src-overlay ovl)
>>>> 	(org-set-local 'org-edit-src-nindent nindent)
>>>> +	(org-src-mode)
>>>> 	(and org-edit-src-persistent-message
>>>> 	     (org-set-local 'header-line-format msg)))
>>>>      (message "%s" msg)
>>>> @@ -400,12 +400,13 @@ the language, a switch telling of the content
>>>> should be in a single line."
>>>> (defun org-edit-src-exit ()
>>>>  "Exit special edit and protect problematic lines."
>>>>  (interactive)
>>>> -  (unless (string-match "\\`*Org Edit " (buffer-name (current-
>>>> buffer)))
>>>> -    (error "This is not an sub-editing buffer, something is
>>>> wrong..."))
>>>> +  (unless org-edit-src-from-org-mode
>>>> +    (error "This is not a sub-editing buffer, something is
>>>> wrong..."))
>>>>  (let ((beg org-edit-src-beg-marker)
>>>> 	(end org-edit-src-end-marker)
>>>> 	(ovl org-edit-src-overlay)
>>>> 	(buffer (current-buffer))
>>>> +	(buffer-file-name nil)
>>>
>>> What is the above line for?
>>
>> I did this so that emacs allows us to kill a modified buffer without
>> warning (Since, in the case of the src edit buffer, saving it involves
>> killing it). It seems that if emacs believes a buffer has no
>> associated
>> file it doesn't warn about modifications. However I think you've
>> pointed
>> to a less hackish way of doing this below. So instead of setting
>> buffer-file-name to nil in this line that you are querying, perhaps we
>> should alter the end of org-edit-src-exit as follows:
>>
>>    ...
>>    (setq code (buffer-string))
>> +   (set-buffer-modified-p nil)
>>    (switch-to-buffer (marker-buffer beg))
>>    (kill-buffer buffer)
>>    (goto-char beg)
>>    (delete-region beg end)
>>    (insert code)
>>    ...
>>
>> ? This would be in addition to your suggestion of (set-buffer-
>> modified-p
>> nil) when entering org-src-mode.
>>
>>>
>>>> 	(nindent org-edit-src-nindent)
>>>> 	code line)
>>>>    (untabify (point-min) (point-max))
>>>> @@ -444,7 +445,6 @@ the language, a switch telling of the content
>>>> should be in a single line."
>>>>    (switch-to-buffer (marker-buffer beg))
>>>>    (kill-buffer buffer)
>>>>    (goto-char beg)
>>>> -    (org-delete-overlay ovl)
>>>
>>> Why are you removing this line?
>>
>> The overlay should have been deleted already by the kill-buffer-hook.
>>
>>>
>>>>    (delete-region beg end)
>>>>    (insert code)
>>>>    (goto-char beg)
>>>> @@ -464,6 +464,19 @@ the language, a switch telling of the content
>>>> should be in a single line."
>>>>    (goto-char (min p (point-max)))
>>>>    (message (or msg ""))))
>>>>
>>>> +(defun org-src-mode-configure-edit-buffer ()
>>>> +  (when org-edit-src-from-org-mode
>>>> +    (setq buffer-offer-save t)
>>>> +    (setq buffer-file-name
>>>> +	  (concat (buffer-file-name (marker-buffer org-edit-src-beg-
>>>> marker))
>>>> +		  "[" (buffer-name) "]"))
>>>> +    (set (if (featurep 'xemacs) 'write-contents-hooks 'write-
>>>> contents-functions)
>>>> +	 '(org-edit-src-save))
>>>> +    (org-add-hook 'kill-buffer-hook
>>>> +		  '(lambda () (org-delete-overlay
>>>> org-edit-src-overlay)) nil 'local)))
>>>> +
>>>> +(org-add-hook 'org-src-mode-hook 'org-src-mode-configure-edit-
>>>> buffer)
>>>> +
>>>> (provide 'org-src)
>>>>
>>>> ;; arch-tag: 6a1fc84f-dec7-47be-a416-64be56bea5d8
>>>> --8<---------------cut here---------------end--------------->8---
>>>
>>>
>>> I believe an important addition to your patch would be to
>>>
>>>   (set-buffer-modified-p nil)t
>>>
>>> when entering org-src-mode.  Otherwise, if I exit Emacs and reply "y"
>>> to all safe-this-buffer questions, then I still get a complaint about
>>> a buffer with changes....
>>
>> Yes, I think I agree (see above).
>>
>> Dan
>>
>>>
>>> Thanks!
>>>
>>>
>>>
>>>
>>> - Carsten
>>>
>>>
>>>
>>> _______________________________________________
>>> Emacs-orgmode mailing list
>>> Remember: use `Reply All' to send replies to the list.
>>> Emacs-orgmode@gnu.org
>>> http://lists.gnu.org/mailman/listinfo/emacs-orgmode
>
>
>
> _______________________________________________
> Emacs-orgmode mailing list
> Remember: use `Reply All' to send replies to the list.
> Emacs-orgmode@gnu.org
> http://lists.gnu.org/mailman/listinfo/emacs-orgmode