Reset output stream column when flushing listener streams

[dieggsy]

Fixes #581.

I'm not sure this will fix the problem for all supported implementations, but it seems to work for at least Allegro, ECL, and SBCL, and I added the T method so that it at least doesn't cause any regressions or breakage in other implementations.

[dieggsy]

Welp, I spoke to soon. I guess this will break on ABCL as it doesn't use the slynk-gray package. I'll look into that.

[joaotavora]

Welp, I spoke to soon. I guess this will break on ABCL as it doesn't use the slynk-gray package. I'll look into that.

Is looking good nonetheless. I'm OK with not having this particular feature work on ABCL.

But I do need to ask some questions:

• Is this more or less how SLIME handles it, too?

• Once this is functionally fixed, are you willing to spend some time iterating an idiot-proof commit message explaining this change and ensuring that this poor maintainer won't be completely baffled by this change 2 years from now?

[dieggsy]

Ok, I've just added a #-armedbear before the new line in slynk.lisp.

• This is more or less how SLIME handles it, as far as I can tell. The reset-stream-line-column is copied directly. Slime puts a call to this in a handful of read/send/eval input functions, but I felt it was simpler to put it in a single place in that flush-listener-streams function, which in SLY appears to be called in a handful of eval and output handling functions. It doesn't seem like SLIME has an equivalent for that flush function. As far as I can tell this has the same desired effect.

• I don't completely understand how gray streams work, to be honest, but my best guess goes something like: it appears to be the case that Lisp's printing and formatting functions are somehow dependent on the column slot to align and position things correctly. If this slot is not reset before every input, the last set value carries over, which causes whatever the mechanism is in these function that tracks alignment and indentation to go out of wack.

[joaotavora]

This is more or less how SLIME handles it, as far as I can tell. The reset-stream-line-column is copied directly. Slime puts a call to this in a handful of read/send/eval input functions, but I felt it was simpler to put it in a single place in that flush-listener-streams function, which in SLY appears to be called in a handful of eval and output handling functions.

Nice I saw this. This is good: looks better than SLIME's change.

I still don't understand the fundamental problem, but that's only because I've been away from CL from some time and haven't run the test cases. Gray streams aren't very difficult to understand, but I'm kinda rusty. They're programmable streams where things can happen every time you notice something going in or out.

I should be able to look into this soon, like in 2 or 3 days. If I don't, please do ping me.

If you can please address the two extra questions about the methods of the reset-stream-line-column generic you added.

[joaotavora]

In the meantime I've figured out why you see a different output stream for SBCL. It's because this stream is actually a separate socket that is more efficient than a gray stream. See this function in slynk-mrepl.lisp

(defun open-dedicated-output-stream (remote-id)
  "Establish a dedicated output connection to Emacs.

Emacs's channel at REMOTE-ID is notified of a socket listening at an
ephemeral port. Upon connection, the listening socket is closed, and
the resulting connecion socket is used as optimized way for Lisp to
deliver output to Emacs."
  (let ((socket (slynk-backend:create-socket slynk::*loopback-interface*
                                             *dedicated-output-stream-port*))
        (ef (or (some #'slynk::find-external-format '("utf-8-unix" "utf-8"))
                (error "no suitable coding system for dedicated stream"))))
    (unwind-protect
         (let ((port (slynk-backend:local-port socket)))
           (send-to-remote-channel remote-id
                                   `(:open-dedicated-output-stream ,port nil))
           (let ((dedicated (slynk-backend:accept-connection
                             socket
                             :external-format ef
                             :buffering *dedicated-output-stream-buffering*
                             :timeout 30)))
             (slynk:authenticate-client dedicated)
             (slynk-backend:close-socket socket)
             (setf socket nil)
             (let ((result
                     ;; See github issue #21: Only sbcl and cmucl apparently
                     ;; respect :LINE as a buffering type, hence this reader
                     ;; conditional. This could/should be a definterface, but
                     ;; looks harmless enough...
                     ;;
                     #+(or sbcl cmucl)
                     dedicated   ;; <==== special SB-SYS:FD-STREAM probably
                     ;; ...on other implementations we make a relaying gray
                     ;; stream that is guaranteed to use line buffering for
                     ;; WRITE-SEQUENCE. That stream writes to the dedicated
                     ;; socket whenever it sees fit.
                     ;;
                     #-(or sbcl cmucl)
                     (if (eq *dedicated-output-stream-buffering* :line)
                         (slynk-backend:make-output-stream
                          (lambda (string)
                            (write-sequence string dedicated)
                            (force-output dedicated)))
                         dedicated)))
               (prog1 result
                 (format result
                         "~&; Dedicated output stream setup (port ~a)~%"
                         port)
                 (force-output result)))))
      (when socket
        (slynk-backend:close-socket socket)))))

We could spare the SBCL method you added if we also took out that code in open-dedicated-output-stream. But I don't think we want to do that, and your solution is started to look like the perfect one.

I just need to test, write a good commit message to summarize all this, and then I'll merge.

[dieggsy]

Oh, I bet this means that CMUCL will also need its own method for this to work there.

Edit: I have pushed a CMUCL fix in a new commit.

[dieggsy]

For testing purposes, here are some examples (including those in #581) that I know behave oddly without the patch across multiple calls, but as expected with. It would probably be good to test more cases/edge-cases, but I don't have any more off the top of my head.

(format t "~10thello~20thello")

(pprint-logical-block (nil nil :per-line-prefix "  ")
  (princ "First line")
  (terpri)
  (princ "Second line"))

From this reddit post describing the behavior in slime:

(format t "Think, I will focus on ~a" '(writing 20 bad lines of code))

[dieggsy]

I should be able to look into this soon, like in 2 or 3 days. If I don't, please do ping me.

@joaotavora pinging you as requested

[joaotavora]

Whoops, indeed escaped me completely. Will look at this this weekend and merge it. Thanks for the CMUCL commit, by the way.

[joaotavora]

Just merged this! Thanks!