[slime-devel] propose changes to `slime-marcoexpand-again' with patch
Apropos the bug report here: https://bugs.launchpad.net/slime/+bug/777405 I'd like to propose the following changes to slime.el's macroexpansion functions: `slime-macroexpand-again' `slime-initialize-macroexpansion-buffer' `slime-create-macroexpansion-buffer' These changes make explicit which buffer should be current during calls to `slime-macroexpand-again' and are IMO in keeping with the intent of the command. The changes to `slime-initialize-macroexpansion-buffer' are w/r/t its optional arg BUFFER and provide additional checks to prevent `erase-buffer' and setting `buffer-undo-list' to nil when current-buffer is not "*slime-macroexpansion*". I've included the proposed changes in full below as well as a diff of slime.el from quicklisp 20110320 with slime.el from CVS 1.1364 ;;; ============================== (defun slime-macroexpand-again () "Reperform the last macroexpansion." (interactive) (slime-eval-async slime-eval-macroexpand-expression (slime-rcurry #'slime-initialize-macroexpansion-buffer (slime-buffer-name :macroexpansion)))) (defun slime-initialize-macroexpansion-buffer (expansion &optional buffer) (let ((expansion-buffer (or (and buffer (get-buffer-create buffer)) (slime-create-macroexpansion-buffer)))) (with-current-buffer expansion-buffer (pop-to-buffer (current-buffer)) (let ((inhibit-read-only t)) (if (eq (current-buffer) (get-buffer (slime-buffer-name :macroexpansion))) (progn ;; Get rid of undo information from previous expansions. (setq buffer-undo-list nil) (erase-buffer) (let ((buffer-undo-list t)) ; Make the initial insertion not be undoable. (insert expansion) (font-lock-fontify-buffer))) (let ((point-now (point))) (save-excursion (insert expansion) (font-lock-fontify-region (point-now) (point))))))))) (defun slime-create-macroexpansion-buffer () (let ((name (slime-buffer-name :macroexpansion))) (slime-with-popup-buffer (name :package t :connection t :mode 'lisp-mode) (slime-mode 1) (slime-macroexpansion-minor-mode 1) (setq font-lock-keywords-case-fold-search t) (get-buffer name)))) ;;; ============================== --- slime-cvs-1.1364.el 2011-05-09 14:46:38.000000000 -0400 +++ slime-20110320-cvs.el 2011-05-09 15:28:43.000000000 -0400 @@ -4988,22 +4973,30 @@ #'slime-initialize-macroexpansion-buffer))) (defun slime-macroexpand-again () - "Reperform the last macroexpansion." - (interactive) - (slime-eval-async slime-eval-macroexpand-expression - (slime-rcurry #'slime-initialize-macroexpansion-buffer - (current-buffer)))) + "Reperform the last macroexpansion." + (interactive) + (slime-eval-async slime-eval-macroexpand-expression + (slime-rcurry #'slime-initialize-macroexpansion-buffer + (slime-buffer-name :macroexpansion)))) (defun slime-initialize-macroexpansion-buffer (expansion &optional buffer) - (pop-to-buffer (or buffer (slime-create-macroexpansion-buffer))) - (setq buffer-undo-list nil) ; Get rid of undo information from - ; previous expansions. - (let ((inhibit-read-only t) - (buffer-undo-list t)) ; Make the initial insertion not be undoable. - (erase-buffer) - (insert expansion) - (goto-char (point-min)) - (font-lock-fontify-buffer))) + (let ((expansion-buffer + (or (and buffer (get-buffer-create buffer)) + (slime-create-macroexpansion-buffer)))) + (with-current-buffer expansion-buffer + (pop-to-buffer (current-buffer)) + (let ((inhibit-read-only t)) + (if (eq (current-buffer) (get-buffer (slime-buffer-name :macroexpansion))) + (progn + ;; Get rid of undo information from previous expansions. + (setq buffer-undo-list nil) + (erase-buffer) + (let ((buffer-undo-list t)) ; Make the initial insertion not be undoable. + (insert expansion) + (font-lock-fontify-buffer))) + (let ((point-now (point))) + (save-excursion (insert expansion) + (font-lock-fontify-region (point-now) (point))))))))) (defun slime-create-macroexpansion-buffer () (let ((name (slime-buffer-name :macroexpansion))) @@ -5012,7 +5005,7 @@ (slime-mode 1) (slime-macroexpansion-minor-mode 1) (setq font-lock-keywords-case-fold-search t) - (current-buffer)))) + (get-buffer name)))) (defun slime-eval-macroexpand-inplace (expander) "Substitute the sexp at point with its macroexpansion.
On Mon, May 9, 2011 at 3:48 PM, MON KEY <monkey@sandpframing.com> wrote:
Apropos the bug report here: https://bugs.launchpad.net/slime/+bug/777405
Apologies, there is a bug in `slime-initialize-macroexpansion-buffer' in previous messages forwarded patch. This: (font-lock-fontify-region (point-now) (point)) Should have been: (font-lock-fontify-region point-now (point)) Following is the modified function in full: (defun slime-initialize-macroexpansion-buffer (expansion &optional buffer) (let ((expansion-buffer (or (and buffer (get-buffer-create buffer)) (slime-create-macroexpansion-buffer)))) (with-current-buffer expansion-buffer (pop-to-buffer (current-buffer)) (let ((inhibit-read-only t)) (if (eq (current-buffer) (get-buffer (slime-buffer-name :macroexpansion))) (progn ;; Get rid of undo information from previous expansions. (setq buffer-undo-list nil) (erase-buffer) (let ((buffer-undo-list t)) ; Make the initial insertion not be undoable. (insert expansion) (font-lock-fontify-buffer))) (let ((point-now (point))) (save-excursion (insert expansion) (font-lock-fontify-region point-now (point)))))))))
MON KEY <monkey@sandpframing.com> writes:
Apropos the bug report here: https://bugs.launchpad.net/slime/+bug/777405
I'd like to propose the following changes to slime.el's macroexpansion functions: `slime-macroexpand-again' `slime-initialize-macroexpansion-buffer' `slime-create-macroexpansion-buffer'
These changes make explicit which buffer should be current during calls to `slime-macroexpand-again' and are IMO in keeping with the intent of the command.
The changes to `slime-initialize-macroexpansion-buffer' are w/r/t its optional arg BUFFER and provide additional checks to prevent `erase-buffer' and setting `buffer-undo-list' to nil when current-buffer is not "*slime-macroexpansion*".
I've included the proposed changes in full below as well as a diff of slime.el from quicklisp 20110320 with slime.el from CVS 1.1364 To reiterate what I said on launchpad. The aforementioned scenario doesn't come up in any normal usage. If you do this accidentally, learn your lesson and don't do that again. Doing M-x slime-thread-control-mode will discard undo history as well, but I don't see you complaining about it.
I don't feel that the goal of slime is to provide an idiot-proof interface. Especially that there are many other parts of emacs which are easily accessible to the user and which can be potentially harmful. Considering this, I see no point in fixing what isn't broken. -- With best regards, Stas.
On Mon, May 9, 2011 at 4:23 PM, Stas Boukarev <stassats@gmail.com> wrote:
To reiterate what I said on launchpad. The aforementioned scenario doesn't come up in any normal usage.
It is AFAIK "normal usage" to use completion when invoking interactive commands. Regardless, who are you to say what is a normal usage pattern for others Slime/Emacs users? If the intent is that `slime-macroexpand-again' _only_ be available as a command in "*slime-macroexpansion*" then the correct thing would be to remove the (interactive) form from `slime-macroexpand-again' and bind "g" inside an anonymous function which is active only when `slime-macroexansion-minor-mode' is in play e.g.: (define-key slime-macroexpansion-minor-mode-map "g" #'(lambda () (interactive) (slime-macroexpand-again)))
If you do this accidentally, learn your lesson and don't do that again. The accident isn't mine -- w/r/t `slime-macroexpand-again' Slime is not careful enough about which buffer is current before erasing the buffer contents.
No doubt I do like many other Slime users and patch my Slime to work correctly rather than rely on memory to prevent myself from using an interactive Slime command which does things it should _never_ do. Fortunately for me I'm capable of locating the bug in slime and patching my local copy of slime to prevent it from occuring again in the future. What I find problematic is that there is an obvious error in `slime-macroexpand-again' which is easily remedied by changing one form from: (slime-rcurry #'slime-initialize-macroexpansion-buffer (current-buffer)) to: (slime-rcurry #'slime-initialize-macroexpansion-buffer (get-buffer-create (slime-buffer-name :macroexpansion))) Why should other users also be forced to needlesly learn to avoid (or fix) a bug when the bug can be so easily remedied?
Doing M-x slime-thread-control-mode will discard undo history as well, but I don't see you complaining about it.
Are you going out of your way to miss the point? Its not only the discarding of buffer-undo-list that is problematic as I've already indicated slime-macroexpand again relies on other functions which erase-buffer! It is the combination of (setq buffer-undo-list nil) (erase-buffer) which is the underlying problem...
I don't feel that the goal of slime is to provide an idiot-proof interface.
It certainly shouldn't be a goal to _provide_ idiotic interfaces nor should it be to promote the feelings of adherents to idiotic interface design. Neither should the goal of Slime be to force its users into acquiescence idiotic opinions on reasonable UI design (esp. where these radically differ from those of the host Emacs). Independent of your "feelings" (which are irrelevant to whether there is or isn't a bug) can you point to any core _interactive_ Emacs command which so blithely destroys both the users buffer content and her ability to recover the destroyed content in a manner similiar to that of `slime-macroexpand-again'? If you can not perhaps you should re-examine your "feelings" w/r/t Slime user's expectations in lieu of Slime's goal to interface with Emacs.
Considering this, I see no point in fixing what isn't broken.
Considering that, maybe you are not the best arbiter of such matters. Patient: "Doctor, my elbow hurts when I do this..." Doctor: "So what, my elbow doesn't hurt when you do that." -- /s_P\
participants (2)
-
MON KEY -
Stas Boukarev