Opinionated defaults

[phikal]

Hi, I would like to question some of the defaults in sly-mrelpl.el, specifically in the sly-mrepl-mode definitions. Specifically the modified values of comint-history-isearch and comint-prompt-read-only are annoying to me, and should be opted in by the user (just as they have to be by default). Another annoyance is that the sly-mreplcommand has a custom display action as an argument, instead of providing a user option to configure how it repl should be displayed (e.g. I prefer a pop-up in a new window if the buffer isn't being displayed yet).

Are points like these up to debate, or is there some reason for these decisions?

[joaotavora]

SLY is almost 10 years old and the read-only prompt is found very early on in much older SLIME. As to comint-history-isearch, it has a similar backstory, though not inherited from SLIME, but is designed to emulate common readline-like bindings found in common non CL shells and non-Emacs Common Lisp REPLs.

So the SLY REPL uses comint.el as a library to provide a specific behavior that was desired. If something better than comint.el had been available, SLY would have used that. I'm very happy you're into Common Lisp and you probably noticed that SLY and SLIME are popular packages. Both are used intensively by many engineers in daily Common Lisp development in at least two medium-sized CL shops I personally worked at.

IME these people don't care particularly about Emacs itself, they care about their Common Lisp programs. Should we break years -- maybe decades -- of muscle memory of existing users to cure the annoyances of a relative newcomer? Wouldn't that be the a very "opinionated" thing? I'm sure you can trivially change these things with one-liners in your customization. One day -- when that doesn't cut it -- just fork SLY to PHLY and continue the legacy. Good luck!

As to the sly-mrepl, just use display-actions-alist & friends.

[monnier]

As to the `sly-mrepl`, just use `display-actions-alist` & friends.

I think what Philip is saying is that `sly-mrepl` should probably rely on `pop-to-buffer` (or some other function calling `display-buffer`) to do the "use existing window". As the code stands, you can't use `display-actions-alist` to force `sly-mrepl` to use a new window, nor can you use it for the other case unless you first customize `switch-to-buffer-obey-display-actions` to t. Sample 100% untested patch attached.

[monnier]

Here's the patch:

diff --git a/contrib/sly-mrepl.el b/contrib/sly-mrepl.el
index 25d49fe1cf..88aa3559c7 100644
--- a/contrib/sly-mrepl.el
+++ b/contrib/sly-mrepl.el
@@ -972,17 +972,19 @@ arglist for the most recently enclosed macro or function."
 ;;;###autoload
 (defun sly-mrepl (&optional display-action)
   "Find or create the first useful REPL for the default connection.
-If supplied, DISPLAY-ACTION is called on the
-buffer. Interactively, DISPLAY-ACTION defaults to using
-`switch-to-buffer' unless the intended buffer is already visible
-in some window, in which case that window is selected."
-  (interactive (list (lambda (buf)
-                       (let ((w (get-buffer-window buf)))
-                         (if w (select-window w) (switch-to-buffer buf))))))
+If supplied, buffer is displayed (and selected) using DISPLAY-ACTION.
+It can either be a function (called with buffer) or an \"action\"
+to pass to `pop-to-buffer'.
+Interactively, DISPLAY-ACTION defaults to using either an
+existing window that already displays the buffer, or the current window."
+  (interactive
+   (list '((display-buffer-reuse-window display-buffer-same-window)
+           (inhibit-same-window . nil))))
   (let* ((buffer
           (sly-mrepl--find-create (sly-current-connection))))
-    (when display-action
-      (funcall display-action buffer))
+    (cond
+     ((functionp display-action) (funcall display-action buffer))
+     (display-action (pop-to-buffer buffer display-action)))
     buffer))
 
 (defun sly-mrepl-on-connection ()

[joaotavora]

Sample 100% untested patch attached.

OK. It's the "untested" part I'm worried about, but OK I'll have a look.

[phikal]

monnier ***@***.***> writes:

Here's the patch: ``` diff --git a/contrib/sly-mrepl.el b/contrib/sly-mrepl.el index 25d49fe1cf..88aa3559c7 100644 --- a/contrib/sly-mrepl.el +++ b/contrib/sly-mrepl.el @@ -972,17 +972,19 @@ arglist for the most recently enclosed macro or function." ;;;###autoload (defun sly-mrepl (&optional display-action) "Find or create the first useful REPL for the default connection. -If supplied, DISPLAY-ACTION is called on the -buffer. Interactively, DISPLAY-ACTION defaults to using -`switch-to-buffer' unless the intended buffer is already visible -in some window, in which case that window is selected." - (interactive (list (lambda (buf) - (let ((w (get-buffer-window buf))) - (if w (select-window w) (switch-to-buffer buf)))))) +If supplied, buffer is displayed (and selected) using DISPLAY-ACTION. +It can either be a function (called with buffer) or an \"action\" +to pass to `pop-to-buffer'. +Interactively, DISPLAY-ACTION defaults to using either an +existing window that already displays the buffer, or the current window." + (interactive + (list '((display-buffer-reuse-window display-buffer-same-window) + (inhibit-same-window . nil)))) (let* ((buffer (sly-mrepl--find-create (sly-current-connection)))) - (when display-action - (funcall display-action buffer)) + (cond + ((functionp display-action) (funcall display-action buffer)) + (display-action (pop-to-buffer buffer display-action))) buffer)) (defun sly-mrepl-on-connection () ```

I still do not get why the argument should be a display action though? If we want to distinguish between interactive and non-interactive calls.

…

-- Philip Kaludercic

[joaotavora]

I still do not get why the argument should be a display action though?

Because it's part of the interface. It's called with that argument in other places in SLY and in user's configs.

[monnier]

> Sample 100% untested patch attached. OK. It's the "untested" part I'm worried about,

Yes, it's nasty especially because there can be so many different circumstances. Even if I didn't mess up, there is absolutely no doubt that the patch will change the default behavior in some corner cases. The upside is that some of those changes will be for the better, and (more importantly) that it should be easier for users to fix it when it's not.

[joaotavora]

that the patch will change the default behavior in some corner cases.

I didn't notice this myself from my cursory reading. Can you state one of these corner cases?

[monnier]

I still do not get why the argument should be a display action though?

What would you use instead?

If we want to distinguish between interactive and non-interactive calls.

... ? You stopped mid-sentence and I'm having a hard time guessing how you intended it to end.

[monnier]

> that the patch will change the default behavior in some corner cases. I didn't notice this myself from my cursory reading. Can you state one of these corner cases?

I'm sure there are many cases I can't even imagine, but here are some I do know: - if the user has a matching entry in `display-buffer-alist`. - if the command is used from a minibuffer-only frame or a dedicated window (in which case `switch-to-buffer` signals an error whereas `pop-to-buffer` may do various things such as create a new window or frame). - I suspect my code might reuse an existing window showing the buffer in another frame, whereas the existing code only looks at windows in the selected frame.

[phikal]

monnier ***@***.***> writes:

> I still do not get why the argument should be a display action though? What would you use instead?

A user option/variable that could be bound dynamically. That way it would be easy to set a default behaviour.

> If we want to distinguish between interactive and non-interactive calls. ... ? You stopped mid-sentence and I'm having a hard time guessing how you intended it to end.

I got distracted and sent the massage off too early, I am not sure what I was going at.

…

-- Philip Kaludercic

[joaotavora]

A user option/variable that could be bound dynamically. That way it would be easy to set a default behaviour.

If you want to bind things dinamically, I presume you're willing to write code.

So why not write (defun phly-mrepl () (interactive) (sly-mrepl #'pop-to-buffer) or (defun phly-mrepl () (interactive) (sly-mrepl #'phly-display-dwim) ?

Anyway, I just wanted to note that this is the first time I've heard this request. Most people I know switch to the REPL with C-c C-z or C-c C-~. I find myself often just switching with C-x b half the time. IOW the SLY REPL is just another buffer which happens to have a shortcut command installed whose semantics were discovered by long-time SLIME users and whose semantics you opine to be annoying and opinionated. But you don't have to use it: it's just another buffer.

That said, I find the drawbacks that Stefan listed not serious at all, and I'll merge a pull request of that patch, especially if someone gives it some minor testing.

Don't hold your breath for the user option/variable though.

[phikal]

I don't want to bind things dynamically, I'm saying the other functions might do so, to override a user option. But sure, that would be fix as well, though I am not a fan of these small wrapper functions.

Most people I know switch to the REPL with C-c C-z or C-c C-~.

I use C-c C-z and coming from SLIME I found it intuitive that the REPL buffer would pop up instead of replace current buffer (as I belive it is common for a lot of REPLs to do).

But you don't have to use it: it's just another buffer.

So it does not create the buffer, eg. after I have previously killed it (context, I kill the REPL buffer from time to time, especially when I cannot clear it using erase-buffer due to the read-only prompts).

[joaotavora]

context, I kill the REPL buffer from time to time, especially when I cannot clear it using erase-buffer due to the read-only prompts).

The way I (and most people) clear the REPL buffer is with sly-mrepl-clear-repl which synchronizes this cleanup with the Slynk side so you can still use things like backreferences, and *, etc.

You can call it with C-c M-o or using the "comma shortcut". You can also clear the most recent amount of input which input with C-c C-o. I think this last command could probably be improved to erase an arbitrary number of output sections but keep the backreferenceable return values.

erase-buffer would be a very bad idea: SLY's REPL is not your average dumb REPL. It's not a simple stdin/stdout of a inferior process or network connection, rather based on carefully maintaining state between two sides.