[slime-devel] [Patch] Improve slime-edit-definition-other-window and ...-frame
Hi, I am sending a little patch that improves the commands slime-edit-definition-other-window and ...-frame. Would someone like to merge it into CVS? Cheers, Matthias 2005-03-27 Matthias Koeppe <mkoeppe@mail.math.uni-magdeburg.de> * slime.el (slime-keys): Bind slime-edit-definition-other-window to `C-x 4 .' and slime-edit-definition-other-frame to `C-x 5 .', shadowing the equivalent find-tag... bindings. (slime-goto-definition-other-window-aux): New function, adapted from the Emacs function find-tag-other-window. (slime-goto-definition): In the other-window and other-frame cases, make sure point does not move in the originating window, even when the definition is found in the same buffer. --- slime.el.~1.473.~ 2005-03-27 16:35:04.000000000 +0200 +++ slime.el 2005-03-27 17:02:06.000000000 +0200 @@ -555,6 +555,8 @@ ("\C-i" slime-complete-symbol :prefixed t :inferior t) ("\M-i" slime-fuzzy-complete-symbol :prefixed t :inferior t) ("\M-." slime-edit-definition :inferior t :sldb t) + ("\C-x4." slime-edit-definition-other-window :inferior t :sldb t) + ("\C-x5." slime-edit-definition-other-frame :inferior t :sldb t) ("\M-," slime-pop-find-definition-stack :inferior t :sldb t) ("\M-*" slime-pop-find-definition-stack :inferior t :sldb t) ("\C-q" slime-close-parens-at-point :prefixed t :inferior t) @@ -5122,18 +5124,41 @@ (error "No known definition for: %s" name)) (slime-goto-definition name definitions where)))) +(defun slime-goto-definition-other-window-aux (definition) + ;; Taken from the Emacs function `find-tag-other-window': + ;; This hair is to deal with the case where the tag is found in the + ;; selected window's buffer; without the hair, point is moved in both + ;; windows. To prevent this, we save the selected window's point before + ;; doing find-tag-noselect, and restore it after. + (let ((window-point (window-point (selected-window)))) + (slime-goto-source-location (slime-definition.location + definition)) + (let ((tagbuf (current-buffer)) + (tagpoint (point))) + (set-window-point (prog1 + (selected-window) + (switch-to-buffer-other-window tagbuf) + ;; We have to set this new window's point; it + ;; might already have been displaying a + ;; different portion of tagbuf, in which case + ;; switch-to-buffer-other-window doesn't set + ;; the window's point from the buffer. + (set-window-point (selected-window) tagpoint)) + window-point)))) + (defun slime-goto-definition (name definitions &optional where) (slime-push-definition-stack) (cond ((slime-length> definitions 1) (slime-show-definitions name definitions)) (t - (slime-goto-source-location (slime-definition.location - (car definitions))) (cond ((equal where 'window) - (switch-to-buffer-other-window (current-buffer))) + (slime-goto-definition-other-window-aux (car definitions))) ((equal where 'frame) - (switch-to-buffer-other-frame (current-buffer))) + (let ((pop-up-frames t)) + (slime-goto-definition-other-window-aux (car definitions)))) (t + (slime-goto-source-location (slime-definition.location + (car definitions))) (switch-to-buffer (current-buffer))))))) (defun slime-edit-definition-other-window (name) -- Matthias Koeppe -- http://www.math.uni-magdeburg.de/~mkoeppe
Howdy Matthias, Matthias Koeppe <mkoeppe+slime@mail.math.uni-magdeburg.de> writes:
I am sending a little patch that improves the commands slime-edit-definition-other-window and ...-frame. Would someone like to merge it into CVS?
Can you try to rewrite the function below in a hairless way? Often it's possible to do a clearer job than Emacs itself does. The name could probably be more informative too. Our coding standard (http://www.norvig.com/luv-slides.ps) wisely suggests we be wary of symbol names ending in "-AUX".
+(defun slime-goto-definition-other-window-aux (definition) + ;; Taken from the Emacs function `find-tag-other-window': + ;; This hair is to deal with the case where the tag is found in the + ;; selected window's buffer; without the hair, point is moved in both + ;; windows. To prevent this, we save the selected window's point before + ;; doing find-tag-noselect, and restore it after. + (let ((window-point (window-point (selected-window)))) + (slime-goto-source-location (slime-definition.location + definition)) + (let ((tagbuf (current-buffer)) + (tagpoint (point))) + (set-window-point (prog1 + (selected-window) + (switch-to-buffer-other-window tagbuf) + ;; We have to set this new window's point; it + ;; might already have been displaying a + ;; different portion of tagbuf, in which case + ;; switch-to-buffer-other-window doesn't set + ;; the window's point from the buffer. + (set-window-point (selected-window) tagpoint)) + window-point)))) +
Hi Luke, Luke Gorrie <luke@synap.se> writes:
Matthias Koeppe <mkoeppe+slime@mail.math.uni-magdeburg.de> writes:
I am sending a little patch that improves the commands slime-edit-definition-other-window and ...-frame. Would someone like to merge it into CVS?
Can you try to rewrite the function below in a hairless way? Often it's possible to do a clearer job than Emacs itself does.
Sorry, I don't have time to work on that. I copied the code from Emacs exactly because I wanted the _same_ behavior as Emacs.
The name could probably be more informative too. Our coding standard (http://www.norvig.com/luv-slides.ps) wisely suggests we be wary of symbol names ending in "-AUX".
I don't mind if you rename it. Cheers, Matthias -- Matthias Koeppe -- http://www.math.uni-magdeburg.de/~mkoeppe
Matthias Koeppe <mkoeppe+slime@mail.math.uni-magdeburg.de> writes:
Luke Gorrie <luke@synap.se> writes:
Can you try to rewrite the function below in a hairless way? Often it's possible to do a clearer job than Emacs itself does.
Sorry, I don't have time to work on that. I copied the code from Emacs exactly because I wanted the _same_ behavior as Emacs.
No worries. I agree that the behaviour is good but I think we can do this more clearly than Emacs does. The tricky part seems to be that they first change the original window, then split, then try to put the original window back the way it was by hand. I think it's simpler to split first and then only change the new window, like this: (defun slime-goto-definition-other-window (definition) (slime-pop-to-other-window) (slime-goto-source-location (slime-definition.location definition)) (switch-to-buffer (current-buffer))) (defun slime-pop-to-other-window () "Pop to the other window, but not to any particular buffer." (pop-to-buffer (current-buffer) t)) I made this change and then committed. Let me know if I mucked up the behaviour somehow. Cheers, Luke
Luke Gorrie <luke@synap.se> writes:
I made this change and then committed. Let me know if I mucked up the behaviour somehow.
When the definition is not found by C-x 4 ., an error is signalled (for instance, "Definition of type :OPERATOR not found for SETF", or "Unknown source location for ..."), but the contents of other-window is changed. This is wrong. Cheers -- Matthias Köppe -- http://www.math.uni-magdeburg.de/~mkoeppe
Matthias Koeppe <mkoeppe+slime@mail.math.uni-magdeburg.de> writes:
Luke Gorrie <luke@synap.se> writes:
I made this change and then committed. Let me know if I mucked up the behaviour somehow.
When the definition is not found by C-x 4 ., an error is signalled (for instance, "Definition of type :OPERATOR not found for SETF", or "Unknown source location for ..."), but the contents of other-window is changed. This is wrong.
Nice catch! Fixed now.
Luke Gorrie <luke@synap.se> writes:
Nice catch! Fixed now.
Thanks! By the way, I see a strange error when M-. wants to present an XREF buffer for choosing the right definition: Debugger entered--Lisp error: (void-variable package) (slime-init-xref-buffer package G30383 G30384) (progn (slime-init-xref-buffer package G30383 G30384) (make-local-variable (quote slime-xref-saved-window-configuration)) (setq slime-xref-saved-window-configuration (current-window-configuration))) (prog2 (progn (slime-init-xref-buffer package G30383 G30384) (make-local-variable ...) (setq slime-xref-saved-window-configuration ...)) (progn (slime-insert-xrefs xrefs) (goto-char ...) (forward-line) (skip-chars-forward " ")) (setq buffer-read-only t) (select-window (or ... ...)) (shrink-window-if-larger-than-buffer)) (save-current-buffer (set-buffer (get-buffer-create ...)) (prog2 (progn ... ... ...) (progn ... ... ... ...) (setq buffer-read-only t) (select-window ...) (shrink-window-if-larger-than-buffer))) (with-current-buffer (get-buffer-create (format "*XREF[%s: %s]*" G30383 G30384)) (prog2 (progn ... ... ...) (progn ... ... ... ...) (setq buffer-read-only t) (select-window ...) (shrink-window-if-larger-than-buffer))) (let ((G30383 type) (G30384 symbol)) (with-current-buffer (get-buffer-create ...) (prog2 ... ... ... ... ...))) (slime-with-xref-buffer (package type symbol) (slime-insert-xrefs xrefs) (goto-char (point-min)) (forward-line) (skip-chars-forward " ")) (if (null xrefs) (message "No references found for %s." symbol) (setq slime-next-location-function (quote slime-goto-next-xref)) (slime-with-xref-buffer (package type symbol) (slime-insert-xrefs xrefs) (goto-char ...) (forward-line) (skip-chars-forward " "))) slime-show-xrefs((("format" ("(:COMPILER-MACRO FORMAT)" :error "Definition of type :COMPILER-MACRO not found for FORMAT") ("(:OPERATOR FORMAT)" :error "Definition of type :OPERATOR not found for FORMAT"))) definition "format" ":LRESTR") slime-show-definitions("format" (("(:COMPILER-MACRO FORMAT)" (:error "Definition of type :COMPILER-MACRO not found for FORMAT")) ("(:OPERATOR FORMAT)" (:error "Definition of type :OPERATOR not found for FORMAT")))) (if (slime-length> definitions 1) (slime-show-definitions name definitions) (let (...) (destructure-case ... ... ...))) slime-goto-definition("format" (("(:COMPILER-MACRO FORMAT)" (:error "Definition of type :COMPILER-MACRO not found for FORMAT")) ("(:OPERATOR FORMAT)" (:error "Definition of type :OPERATOR not found for FORMAT"))) nil) (if (null definitions) (if slime-edit-definition-fallback-function (funcall slime-edit-definition-fallback-function name) (error "No known definition for: %s" name)) (slime-goto-definition name definitions where)) (let ((definitions ...)) (if (null definitions) (if slime-edit-definition-fallback-function ... ...) (slime-goto-definition name definitions where))) slime-edit-definition("format") * call-interactively(slime-edit-definition) Does anyone else see this? The funny thing is that I can work around this error like this: --- slime.el.~1.476.~ 2005-04-01 14:50:11.000000000 +0200 +++ slime.el 2005-04-01 15:16:48.405926000 +0200 @@ -5943,12 +5943,12 @@ (backward-char 1) (delete-char 1)) -(defun slime-show-xrefs (xrefs type symbol package) +(defun slime-show-xrefs (xrefs type symbol yyy-package) "Show the results of an XREF query." (if (null xrefs) (message "No references found for %s." symbol) (setq slime-next-location-function 'slime-goto-next-xref) - (slime-with-xref-buffer (package type symbol) + (slime-with-xref-buffer (yyy-package type symbol) (slime-insert-xrefs xrefs) (goto-char (point-min)) (forward-line) I observed with edebug that the binding for `package' magically disappears in the buffer that `with-current-buffer' (in the macroexpansion of slime-with-xref-buffer) switches to. Any idea? Cheers -- Matthias Koeppe -- http://www.math.uni-magdeburg.de/~mkoeppe
participants (2)
-
Luke Gorrie -
Matthias Koeppe