From: Carsten Dominik <carsten.dominik@gmail.com>
To: Dan Davison <davison@stats.ox.ac.uk>
Cc: emacs org-mode mailing list <emacs-orgmode@gnu.org>
Subject: Re: Re: PATCH: proposed improvements to org-src-mode
Date: Sun, 6 Sep 2009 13:41:17 +0200 [thread overview]
Message-ID: <C6BD1F49-2F25-4A4B-8A05-5A251140D54E@gmail.com> (raw)
In-Reply-To: <874orhcn5a.fsf@stats.ox.ac.uk>
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
prev parent reply other threads:[~2009-09-06 11:41 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-06-02 15:02 Saving the *Org Edit Src Example* buffer Dan Davison
2009-06-02 15:49 ` Dan Davison
2009-06-02 17:12 ` Carsten Dominik
2009-06-09 14:51 ` Dan Davison
2009-06-09 18:20 ` Carsten Dominik
2009-06-02 17:04 ` Carsten Dominik
2009-08-11 16:44 ` PATCH: proposed improvements to org-src-mode Dan Davison
2009-08-12 12:22 ` Carsten Dominik
2009-08-12 14:58 ` Dan Davison
2009-08-19 11:03 ` Dan Davison
2009-08-24 12:17 ` Carsten Dominik
2009-08-28 2:36 ` Dan Davison
2009-08-28 7:59 ` Carsten Dominik
2009-09-05 17:33 ` Dan Davison
2009-09-06 11:41 ` Carsten Dominik [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=C6BD1F49-2F25-4A4B-8A05-5A251140D54E@gmail.com \
--to=carsten.dominik@gmail.com \
--cc=davison@stats.ox.ac.uk \
--cc=emacs-orgmode@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).