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

[Carsten Dominik <carsten.dominik@gmail.com>]

Hi Dan,

thanks for doing such a careful job.  I applied all three parts.

- Carsten

On Sep 5, 2009, at 7:33 PM, Dan Davison wrote:

> 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