PATCH: proposed improvements to org-src-mode

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

I'm attaching a patch which attempts to make some improvements to
org-src-mode. A quick recap: currently, C-c ' on a source code block
displays the code in a language major mode buffer with minor mode
org-src-mode, which features the following two useful key-bindings:

| C-x s | org-edit-src-save | save the code in the source code block in the parent org file |
| C-c ' | org-edit-src-exit | return to the parent org file with new code                   |

Furthermore, while the edit buffer is alive, the originating code block
is subject to a special overlay which links to the edit buffer when you
click on it. This is all excellent, and I use it every day, but I think
there's still a couple of improvements that we should make.

Specifically, I'm proposing that the following are bugs:

* Proposed bug I
      C-x k kills the edit buffer without questions; the overlay
      remains, but now links to a deleted buffer.
* Proposed bug II
      C-x C-c kills a modified edit buffer silently, without offering to
      save your work. I have lost work like that a number of times
      recently.
* Proposed bug III
      C-x s does not offer to save a modified edit buffer

The attached patch does the following.
- C-x s offers to save edit buffers
- C-x C-c offers to save edit buffers
- C-x k warns that you're killing an edit buffer
- If you do kill an edit buffer, the overlay in the parent buffer is removed
- Edit buffers are named *Org Src <orgbuf>[<lang>]*, where
  <orgbuf> is the name of the org-mode buffer containing this
  source code block, and lang is the language major mode.
- An internal detail is that org-edit-src-save is added to the
  write-contents-functions list, which means that it is no longer
  necessary to explicitly remap C-x C-s to org-edit-src-save

* Notes
  This patch gives the desired behaviour, at the cost of being forced to
  assign a buffer-file-name to the edit buffer. The consequence is that
  the edit buffer is considered to always be modified, since a file of
  that name is never actually written to (doesn't even exist). I didn't
  manage to come up with a way to trick emacs into holding the
  appropriate beliefs about whether the buffer had been modified. But in
  any case, I think there's an argument that these modifications
  warnings are a good thing, because one should not leave active edit
  buffers around: you should always have exited with C-c ' first.

Just in case it is helpful, I am including the notes I made in the
course of making these changes at the very bottom of the email.

Dan

p.s. In these two lines:
-  (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..."))

I assumed that org-edit-src-from-org-mode was an appropriate test. But
that may be incorrect as I am not certain what the intention was for
that variable.

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/org-src.el b/lisp/org-src.el
index 2a6c087..a5816d2 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))
+	  (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)
 	(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)
     (delete-region beg end)
     (insert code)
     (goto-char beg)
@@ -464,6 +464,18 @@ 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-buffer ()
+  (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-buffer)
+
 (provide 'org-src)
 
 ;; arch-tag: 6a1fc84f-dec7-47be-a416-64be56bea5d8
--8<---------------cut here---------------end--------------->8---

* Notes on patch
*** write-contents-functions
    A good start seems to be to use org-src-mode-hook to add
    org-edit-src-save to the write-contents-functions list. This
    means that when it comes to saving, org-edit-src-save will be
    called and no subsequent attempt will be made to save the buffer
    in the normal way. (This should obviate the remapping of C-x C-s
    to org-edit-src-save in org-src.el)
*** buffer-offer-save
    We also want to set this to t.

*** Where does this get us?

    - C-x s still does *not* offer to save the edit buffer. That's
      because buffer-file-name is nil.
    
    - C-x C-c does ask us whether we want to save the
      edit buffer. However, since buffer-file-name is nil it asks us
      for a file name. The check in org-edit-src-exit throws an error
      unless the buffer is named '* Org Edit '...

    - C-x k kills the buffer silently, leaving a broken overlay
      link. If buffer-file-name were set, it would have warned that
      the buffer was modified.

*** buffer-file-name
    So, that all suggests that we need to set buffer-file-name, even
    though we don't really want to associate this buffer with a file
    in the normal way. As for the file name, my current suggestion
    is parent-org-filename[edit-buffer-name].
    
    [I had to move the (org-src-mode) call to the end of
    org-edit-src-code to make sure that the required variables were
    defined when the hook was called.]
    
*** And so where are we now?
    - C-x s *does* offer to save the edit buffer, but in saving
      produces a warning that the edit buffer is modified.
    - C-x k now gives a warning that the edit buffer is modified
      (even if it's not).
    - C-x C-c is working as desired, except that again we get
      warnings that the edit buffer is modified, once when we save,
      and again just before exiting emacs.
    - And C-c ' now issues a warning that the edit buffer is
      modified when we leave it, which we don't want.
*** So, we need to get rid of the buffer modification warnings.
    I've made buffer-file-name nil inside the let binding in
    org-edit-src-exit.
*** And?
    - C-x s behaves as desired, except that as was already the case,
      the edit buffer is always considered modified, and so repeated
      invocations keep saving it.
    - As was already the case, C-x k always gives a warning that the
      edit buffer has been modified.
    - C-x C-c is as desired (offers to save the edit buffer) except
      that it warns of the modified buffer just before exiting.
    - C-c ' is as it should be (silent)


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

> Hi Dan,
>
> On Jun 2, 2009, at 5:02 PM, Dan Davison wrote:
>
>> Following on from the recent improvements to the *Org Edit Src
>> Example*
>> buffer, I have one more proposal: I have remapped C-x C-s so that it
>> saves the code in the org buffer, rather than offering to save the
>> Edit
>> buffer itself (as it used to be with the indirect edit buffer). I find
>> this essential, although I recognise that remapping C-x C-s is a
>> rather
>> radical thing to do to an emacs buffer.
>
>
> But allowed:  From the Emacs lisp docs, under Major Mode Conventions:
>
>      It is legitimate for a major mode to rebind a standard key
>      sequence if it provides a command that does "the same job" in a
>      way better suited to the text this mode is used for.
>
> I'd say, your's is a perfect example for this rule.
>
>
>> I am using the simple-minded approach below; it seems to work fine (I
>> don't even notice a flicker -- should I be surprised at that?), but if
>> someone can suggest an improved implementation I'd be happy to learn
>> how
>> it should be done (perhaps there are buffer variables other than point
>> and mark that I should restore? Is there a general mechanism I should
>> use for this?).
>>
>> Dan
>>
>> (defun org-edit-src-save ()
>>  "Update the parent org buffer with the edited source code, save
>> the parent org-buffer, and return to the source code edit
>> buffer."
>>  (interactive)
>>  (let ((p (point))
>> 	(m (mark)))
>>    (org-edit-src-exit)
>>    (save-buffer)
>>    (org-edit-src-code)
>>    (set-mark m)
>>    (goto-char p)))
>
> This is already excellent.  I have changed it only slightly,
> in order to get a better message when this command finishes, and
> because `push-mark' should be used here instead of `set-mark'.
>
> (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 p)
>     (message (or msg ""))))
>
> I have added your code, thanks.
>
> - Carsten