From mboxrd@z Thu Jan 1 00:00:00 1970 From: Carsten Dominik Subject: Re: Re: PATCH: proposed improvements to org-src-mode Date: Sun, 6 Sep 2009 13:41:17 +0200 Message-ID: References: <873aain0f7.fsf@stats.ox.ac.uk> <8216B89D-C718-4604-AAC4-82BFCA967161@gmail.com> <873a7y9u63.fsf_-_@stats.ox.ac.uk> <9C5369DB-A034-4A0D-84FE-8872BB032BA6@gmail.com> <87iqgt5ba2.fsf@stats.ox.ac.uk> <87ljlgysjl.fsf@stats.ox.ac.uk> <3845FC6A-EF07-4CB0-BB36-6F2652CD4F26@gmail.com> <87bpm0vf42.fsf@stats.ox.ac.uk> <52087875-668B-4AC5-AB5A-A4B6A461EAAB@gmail.com> <874orhcn5a.fsf@stats.ox.ac.uk> Mime-Version: 1.0 (Apple Message framework v936) Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes Content-Transfer-Encoding: 7bit Return-path: Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MkG7f-0000Z7-Fg for emacs-orgmode@gnu.org; Sun, 06 Sep 2009 07:41:31 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MkG7a-0000SS-4g for emacs-orgmode@gnu.org; Sun, 06 Sep 2009 07:41:31 -0400 Received: from [199.232.76.173] (port=37812 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MkG7Z-0000Ry-ML for emacs-orgmode@gnu.org; Sun, 06 Sep 2009 07:41:25 -0400 Received: from mail-ew0-f211.google.com ([209.85.219.211]:61283) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MkG7Y-0003W4-V3 for emacs-orgmode@gnu.org; Sun, 06 Sep 2009 07:41:25 -0400 Received: by mail-ew0-f211.google.com with SMTP id 7so190919ewy.31 for ; Sun, 06 Sep 2009 04:41:24 -0700 (PDT) In-Reply-To: <874orhcn5a.fsf@stats.ox.ac.uk> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Dan Davison Cc: emacs org-mode mailing list 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 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 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 writes: >>>>> >>>>>> Carsten Dominik 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