-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTML Notebook carriage-return handling, take 2 #1674
Conversation
I think this should apply cleanly (the web interface doesn't tell me as a non-ipython developer). Let me know if it's still messed up. |
Yes, this applies cleanly now. I will test again this evening. Thanks for your patience! |
It looks like fixConsole is being called twice on stream output, because I am now seeing color escapes as raw HTML: print '\033[31m red' which should print 'red' in red. |
@@ -636,15 +633,22 @@ var IPython = (function (IPython) { | |||
// latest output was in the same stream, | |||
// so append directly into its pre tag | |||
// escape ANSI & HTML specials: | |||
var text = utils.fixConsole(json.text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put this call to fixConsole back, and remove the one above. append_text below calls fixConsole (as it should), so the above call is a duplicate everywhere outside this block of appending to the existing div.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turned out to be a little more complicated than that. The existing content already has the console escapes converted to HTML. So it needs to get that as an HTML string, append the new text with console escapes converted to HTML, and set the whole thing back as an HTML string (not as a raw text string). See the attached commit that seems to resolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, that reminds me that fixCR is being called on html, which is probably not ideal. It might be best if you get the original output back, and transform the two together. It also suggests that fixCR should be called before fixConsole.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would I get the original output back? Would that require an additional request to the server? Does it still have it?
I'm calling fixConsole before fixCR here so that fixConsole isn't called again on the existing text. It probably wouldn't hurt anything, but it's additional unnecessary computation.
This is turning out to be more problematic than I suspected...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easy to get the last output, it's just this.outputs[this.outputs.length-1]
, but I realize that after one such append, that would stop working and become immediately too complicated. So scratch that suggestion.
I should note that there is not an actual problem now with fixCR(fixConsole), but it just makes more sense to erase output before adding all of the escapes. Since that's not reasonably compatible with the appending here, let's ignore that, and stick with what you are doing. I will make a note that this whole thing should be cleaned up and rewritten in a more sensible way.
It probably wouldn't hurt anything, but it's additional unnecessary computation.
That's not quite accurate - fixConsole must not be called twice on the same text, because it turns colored text into escaped html.
This is performance acceptably well, in my tests, so I think it can be merged. I don't think we should be in the business of a terminal emulator in the browser, but as we add pseudo-support for terminal features like this, I think people may start expecting it. For instance, here is a test case that highlights just how different the notebook and a terminal will behave: def test():
print '\033[31m red',
sys.stdout.flush()
print 'who\rda'
test() In the terminal, this produces a red |
From discussion on #1659: I feel that we should remove overwritten content at some point before saving. Even if we'd rather they didn't, people are going to use |
Sorry to drop this for a while. I think given all of the various issues with this, and the fact that, for good reason, IPython notebook output sections will never be real terminals, I wonder if the better solution isn't to just have a set of standard widgets for this (such as what's in the animation examples) and not try to make this work. Ideally, there would be some sort of library that didn't depend on IPython directly, that would work for displaying progress at both a regular console, the standard python interactive prompt, the IPython console, IPython notebook etc... |
I'm not sure that such a library is any easier, though. By design, the It could simply have an option that the user set to use a notebook |
Good point. |
I am trying to move notebook related PRs forward. What is the status of this one? |
I think it's working well (it requires #1663 in order to behave properly on load from file), but @takluyver is inclined to require that the With this as-is |
To explain my case, consider a progress counter, a common use of The alternative is to encourage people to use a notebook-specific API to display progress information. But library authors presumably want to have one set of code that displays progress. And if they do include an option to use the notebook-specific API, I don't see any good way to automatically select that, because our design is that the kernel doesn't know about frontends. I'm trying to think if it can be done with the display protocol, but it would be awkward at best. |
I should clarify: I don't disagree that saving the transformed output is probably better, I just think disagree that it should be a criterion for this PR. |
When people are developing custom display code that they want to work in the notebook, it is very reasonable that they need to use notebook specific constructs such as |
@ellisonbg: Both before/after this PR there are significant disadvantages to using
Also note that #1663 is required for |
On Thu, May 24, 2012 at 3:42 PM, Min RK
In general, dynamic output should only "run" after direct code
Brian E. Granger |
Yes, |
On 24 May 2012 23:37, Brian E. Granger
I don't see how this is practical. Say I'm writing some long-running I could make the function:
|
@takluyver You can already do simultaneous text/html/javascript display, which get used selectively by the frontends (including terminal). |
On 25 May 2012 00:21, Min RK
I know, but I've spent a while trying to work out the implementation |
@minrk I was very deliberate to have the text repr of the Javascript output displayed when the Javascript is not run. Running JavaScript code does have a security risk associated with it and when people load a notebook from someone else, I want them to see that if they run that cell, JavaScript code will run that could do harmful things. I realize the text repr doesn't communicate that explicitely, but at least it indicates that JavaScript code would be run by that cell. I don't think we should change that. |
@takluyver Having notebook specific code is inevitable. We already have this in |
I take your point about security, so maybe we shouldn't change it. Though I can say from my experience writing various js notebooks that the current behavior is terrifically unpleasant. |
On 26 May 2012 05:58, Brian E. Granger
I don't see this view flying, especially because people can write a |
I think we should go ahead and merge this one. But I just saw that it needs to be rebased. @mdboom can you do that? I will merge it then. |
Conflicts: IPython/frontend/html/notebook/static/js/utils.js
…nts -- but only when there wasn't already something there.
Rebased. |
// so don't append any elements, which might add undesirable space | ||
return; | ||
} | ||
var text = utils.fixConsole(json.text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the rebase didn't go quite right. This now gets double-fixed, which results in escaped HTML in the notebook output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I think the behavior is correct if this is just: var text = json.text
, with no other changes.
I think this is fixed now. Sorry -- it was kind of a hairy rebase... |
So can we merge this now? |
Yes, merging now. Thanks @mdboom! |
HTML Notebook handles carriage-return special character
HTML Notebook handles carriage-return special character
This is a fixed version of #1659 that doesn't erase all content returned from shell commands, such as
!ls
.