I put a test case here that documents it: http://paste.lisp.org/+2GLO
The issue is that when it starts a presentation it saves a marker, but then that marker gets moved when printing text and so when it ends the presentation the start and end markers are the same point, the one immediately after the text we are trying to present.
I'm not sure where to go from here. I don't know how to get the old marker to stay in place any more than it is now. There is a comment: ;; We use markers because text can also be inserted before this presentation. ;; (Output arrives while we are writing presentations within REPL results.) Perhaps that is more relevant to printing with the target :repl-result vs target null?
I tried changing the presentation start function (it's on the paste) to save the number if target is null and that looks like it is mostly working for me, can anyone else comment on the idea?
On 9/23/2010 4:52 PM, Nathan Bird wrote:
I put a test case here that documents it: http://paste.lisp.org/+2GLO
The issue is that when it starts a presentation it saves a marker, but then that marker gets moved when printing text and so when it ends the presentation the start and end markers are the same point, the one immediately after the text we are trying to present.
I think we got this fixed. I changed the repl-emit to use insert rather than insert-before. This leaves presentation-start marker intact as well as leaving the overlay in place rather than extending the end of the overlay with each further insert. This change necessitates a few other tweaks to that function as the other markers that were being moved based on the insert-before need to be updated manually now.
I also change a few items in swank-presentation-streams to get that working: * pass slime-stream-p :dedicated through as other functions use that. <- presentation streams work with dedicated output streams now (again?). * slime-stream-p had a scheme to cache the last-answer that wasn't threadsafe, it is now. * slime-stream-p can be dynamically overridden with *slime-stream-p*; useful when writing to a mem buffer or some other location that will be dumped to a slime stream but is natively detectable as such.
I updated the ChangeLog to include descriptions. Individual patches can be pulled from git://github.com/UnwashedMeme/slime.git, complete patch attached.
Please let me know if I missed anything.
On Mon, Sep 27, 2010 at 2:13 PM, Nathan Bird nathan@acceleration.net wrote:
On 9/23/2010 4:52 PM, Nathan Bird wrote:
I put a test case here that documents it: http://paste.lisp.org/+2GLO
The issue is that when it starts a presentation it saves a marker, but then that marker gets moved when printing text and so when it ends the presentation the start and end markers are the same point, the one immediately after the text we are trying to present.
I think we got this fixed. I changed the repl-emit to use insert rather than insert-before. This leaves presentation-start marker intact as well as leaving the overlay in place rather than extending the end of the overlay with each further insert. This change necessitates a few other tweaks to that function as the other markers that were being moved based on the insert-before need to be updated manually now.
This looks good.
I also change a few items in swank-presentation-streams to get that working:
- pass slime-stream-p :dedicated through as other functions use that.
<- presentation streams work with dedicated output streams now (again?).
- slime-stream-p had a scheme to cache the last-answer that wasn't
threadsafe, it is now.
Excellent.
- slime-stream-p can be dynamically overridden with *slime-stream-p*;
useful when writing to a mem buffer or some other location that will be dumped to a slime stream but is natively detectable as such.
I'm not sure about the last change. I don't think it can work in a non-dedicated stream setup because the presentation boundaries are signalled out-of-band.
Matthias
On 9/29/2010 4:37 PM, Matthias Koeppe wrote:
- slime-stream-p can be dynamically overridden with *slime-stream-p*;
useful when writing to a mem buffer or some other location that will be dumped to a slime stream but is natively detectable as such.
I'm not sure about the last change. I don't think it can work in a non-dedicated stream setup because the presentation boundaries are signalled out-of-band.
Yeah, that use case I gave only works for dedicated output streams, but it at least doesn't get in the way for non-dedicated. The use case I have (a stream logger who's printed objects are automatically presentations where appropriate) is precisely as above though: I want to print to a memory buffer and then dump that to the dedicated stream. When running a bunch of threads this still isn't threadsafe behavior by any means but it reduces the critical section to just (write-sequence <short-message> stream).
I expanded the docstring to make it clearer[1]. I'll poke around a bit more to see if I can come up with a better idea but this works pretty well for me.
Thanks for the feedback!
[1] http://github.com/UnwashedMeme/slime/commit/f381c3341ccd059191a309c628aaa2f4... Do y'all use the nablaone/slime github cvs mirror? are flat patchfiles (updated patch attached) still preferred?
On Thu, Sep 30, 2010 at 7:44 AM, Nathan Bird nathan@acceleration.net wrote:
On 9/29/2010 4:37 PM, Matthias Koeppe wrote:
- slime-stream-p can be dynamically overridden with *slime-stream-p*;
useful when writing to a mem buffer or some other location that will be dumped to a slime stream but is natively detectable as such.
I'm not sure about the last change. I don't think it can work in a non-dedicated stream setup because the presentation boundaries are signalled out-of-band.
Yeah, that use case I gave only works for dedicated output streams, but it at least doesn't get in the way for non-dedicated.
I don't agree. For non-dedicated, it can cause presentation markup on the wrong text.
More importantly, I am concerned about effects on unintended streams, for instance by condition handlers, debugger, etc. So I don't think this is safe.
The use case I have (a stream logger who's printed objects are automatically presentations where appropriate) is precisely as above though: I want to print to a memory buffer and then dump that to the dedicated stream. When running a bunch of threads this still isn't threadsafe behavior by any means but it reduces the critical section to just (write-sequence <short-message> stream).
I would suggest to create a solution with a dynamic variable that is a list of eligible streams. And it should only kick in dedicated mode.
For your use case, an interesting solution could also be a custom string stream class that queues up the strings and presentation markers. This would be in parallel to the pretty printing streams in Allegro CL and my patched version of SBCL's.
Matthias
The use case I have (a stream logger who's printed objects are automatically presentations where appropriate) is precisely as above though: I want to print to a memory buffer and then dump that to the dedicated stream. When running a bunch of threads this still isn't threadsafe behavior by any means but it reduces the critical section to just (write-sequence<short-message> stream).
I would suggest to create a solution with a dynamic variable that is a list of eligible streams. And it should only kick in dedicated mode.
There isn't really a dedicated mode; the behavior is geared around the stream it is writing to, not a general mode we are in.
I made a version with a dynamic variable list of additional eligible streams[1]; the variable is only used if *use-dedicated-output-stream* is true. Beyond that there isn't anything beyond the documentation and good sense that enforces it.
I wrote a macro that tried to provide that guard-and-buffer but it ended up looking fairly specific to this use case so I don't think it is really worth including.
For your use case, an interesting solution could also be a custom string stream class that queues up the strings and presentation markers. This would be in parallel to the pretty printing streams in Allegro CL and my patched version of SBCL's.
While a fun idea that appears to be a lot more work vs an already working system.
[1] http://github.com/UnwashedMeme/slime/blob/master/contrib/swank-presentation-...
On Thu, Sep 30, 2010 at 2:19 PM, Nathan Bird nathan@acceleration.net wrote:
I made a version with a dynamic variable list of additional eligible streams[1]; the variable is only used if *use-dedicated-output-stream* is true. Beyond that there isn't anything beyond the documentation and good sense that enforces it.
[1] http://github.com/UnwashedMeme/slime/blob/master/contrib/swank-presentation-...
This looks fine to me, and I think it would make sense to include this in SLIME.
Matthias
On 10/1/2010 2:14 PM, Matthias Koeppe wrote:
On Thu, Sep 30, 2010 at 2:19 PM, Nathan Birdnathan@acceleration.net wrote:
I made a version with a dynamic variable list of additional eligible streams[1]; the variable is only used if *use-dedicated-output-stream* is true. Beyond that there isn't anything beyond the documentation and good sense that enforces it.
[1] http://github.com/UnwashedMeme/slime/blob/master/contrib/swank-presentation-...
This looks fine to me, and I think it would make sense to include this in SLIME.
Matthias
Cool, I packaged up the changes and made ChangeLog entries; I think it is ready for commit.
Rebased my patches to be in front (based on nablaone/slime's repo) so it should be easy pull git pull git://github.com/UnwashedMeme/slime.git master
or
Patch attached which is the collected diff, should apply cleanly, let me know if otherwise.
Thanks a bunch for the help.
Nathan,
This seems to work fine for me, but please run this by the people who made the most recent changes to the REPL code.
Matthias
On Mon, Oct 4, 2010 at 12:44 PM, Nathan Bird nathan@acceleration.net wrote:
On 10/1/2010 2:14 PM, Matthias Koeppe wrote:
On Thu, Sep 30, 2010 at 2:19 PM, Nathan Birdnathan@acceleration.net wrote:
I made a version with a dynamic variable list of additional eligible streams[1]; the variable is only used if *use-dedicated-output-stream* is true. Beyond that there isn't anything beyond the documentation and good sense that enforces it.
[1]
http://github.com/UnwashedMeme/slime/blob/master/contrib/swank-presentation-...
This looks fine to me, and I think it would make sense to include this in SLIME.
Matthias
Cool, I packaged up the changes and made ChangeLog entries; I think it is ready for commit.
Rebased my patches to be in front (based on nablaone/slime's repo) so it should be easy pull git pull git://github.com/UnwashedMeme/slime.git master
or
Patch attached which is the collected diff, should apply cleanly, let me know if otherwise.
Thanks a bunch for the help.
-- Nathan Bird nathan@acceleration.net http://www.acceleration.net/ Custom Programming, Design, Hosting, and Broadband.
Hi Nathan,
On Thu, Sep 23, 2010 at 1:52 PM, Nathan Bird nathan@acceleration.net wrote:
I put a test case here that documents it: http://paste.lisp.org/+2GLO
The issue is that when it starts a presentation it saves a marker, but then that marker gets moved when printing text and so when it ends the presentation the start and end markers are the same point, the one immediately after the text we are trying to present.
I'm not sure where to go from here. I don't know how to get the old marker to stay in place any more than it is now. There is a comment: ;; We use markers because text can also be inserted before this presentation. ;; (Output arrives while we are writing presentations within REPL results.) Perhaps that is more relevant to printing with the target :repl-result vs target null?
I tried changing the presentation start function (it's on the paste) to save the number if target is null and that looks like it is mostly working for me, can anyone else comment on the idea?
I don't think this is the right fix.
I believe this was broken in revision 1.33 of slime-presentations.el, which replaced `slime-presentation-write'. I think the real bug is that `slime-repl-emit' invokes `insert-before-markers'; I think it should just call `insert' and then manually adjust the markers that should be advanced. The old version of `slime-presentation-write' did something like that.
Unfortunately I don't have the time to fix this.
Matthias
I believe this was broken in revision 1.33 of slime-presentations.el, which replaced `slime-presentation-write'. I think the real bug is that `slime-repl-emit' invokes `insert-before-markers'; I think it should just call `insert' and then manually adjust the markers that should be advanced. The old version of `slime-presentation-write' did something like that.
Matthias
Thanks for the info, that is the conclusion Russ and I stumbled across yesterday after my original fix had the overlays working but still buggy. We then changed the slime-repl-emit function to do precisely as you suggest above: http://common-lisp.net/pipermail/slime-devel/2010-September/017765.html (patch attached there).
Here's the specific commit that is that fix: http://github.com/UnwashedMeme/slime/commit/fd34f0d1924cfec3de6f447907d956dd... As I wrote in the other mail though, there are a few more miscellaneous fixups that make the setup work better.
-
Matthias Koeppe -
Nathan Bird