From mboxrd@z Thu Jan 1 00:00:00 1970 From: Carsten Dominik Subject: Re: PATCH: proposed improvements to org-src-mode Date: Wed, 12 Aug 2009 14:22:28 +0200 Message-ID: <9C5369DB-A034-4A0D-84FE-8872BB032BA6@gmail.com> References: <873aain0f7.fsf@stats.ox.ac.uk> <8216B89D-C718-4604-AAC4-82BFCA967161@gmail.com> <873a7y9u63.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 1MbCqj-00050s-Qr for emacs-orgmode@gnu.org; Wed, 12 Aug 2009 08:22:37 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MbCqe-0004xr-M4 for emacs-orgmode@gnu.org; Wed, 12 Aug 2009 08:22:37 -0400 Received: from [199.232.76.173] (port=43071 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MbCqe-0004xn-6o for emacs-orgmode@gnu.org; Wed, 12 Aug 2009 08:22:32 -0400 Received: from mail-ew0-f207.google.com ([209.85.219.207]:46913) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MbCqd-0002ng-MS for emacs-orgmode@gnu.org; Wed, 12 Aug 2009 08:22:31 -0400 Received: by mail-ew0-f207.google.com with SMTP id 3so207957ewy.42 for ; Wed, 12 Aug 2009 05:22:31 -0700 (PDT) In-Reply-To: <873a7y9u63.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, thank you for studying and describing these issues, and for proposing a patch. I am not sure that the implementation you offer is the cleanest possibile, I definitely do not want to attach a file to this temporary editing buffer. It is probably better to install a kill-buffer-hook to force the query, for example, or even to advise the save-buffers-kill-terminal function to handle the special case. First of all, in reading your mail I have a few problems understanding exactly what you mean, because I have the feeling that you do not clearly distinguish between `C-x s' and `C-x C-s'. Because you write that `C-x s' is bound to `org-edit-src-save' which is is not. Could you please review your post to make sure that you are using the correct keys? The I will comment further. Thanks. - Carsten On Aug 11, 2009, at 6:44 PM, Dan Davison wrote: > 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 []*, where > 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 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