HTML Notebook carriage-return handling, take 2

[mdboom]

This is a fixed version of #1659 that doesn't erase all content returned from shell commands, such as !ls.

[Carreau]

Sorry, but @minrk has reverted #1659 (e921622) so this does not apply cleanly on master... could you rebased it ?

Thanks.

[mdboom]

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.

[minrk]

Yes, this applies cleanly now. I will test again this evening. Thanks for your patience!

[minrk]

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.

[minrk]

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.

[mdboom]

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.

[minrk]

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.

[mdboom]

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...

[minrk]

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.

[minrk]

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 daed who, and in the notebook it produces a black da.

[takluyver]

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 \r for progress counters, because it works and library authors shouldn't have to detect and handle the notebook. If it's left in, it's extra noise for version control. Also, if one day another application can load ipynb files, but can't handle \r, its users won't appreciate seeing a load of messy progress information.

[mdboom]

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...

[takluyver]

I'm not sure that such a library is any easier, though. By design, the
kernel doesn't know that a request has come from the notebook, so I
don't think a library can reliably determine to use the notebook
methods to display a progress bar.

It could simply have an option that the user set to use a notebook
progress bar, but that's not very user friendly, and doesn't work well
if you're using the same kernel between a notebook and a qtconsole,
for example.

[mdboom]

Good point.

[ellisonbg]

I am trying to move notebook related PRs forward. What is the status of this one?

[minrk]

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 \r blanking should affect the saved ipynb.

With this as-is \r behavior in the notebook at least makes some sense, but has disadvantages relative to a non-terminal-centric approach. I think it's mainly a question of how much we want to discourage/encourage terminal tricks in the notebook.

[takluyver]

To explain my case, consider a progress counter, a common use of \r. It's writing 1\r2\r3\r.... If we store all of that output, it's extra noise for keeping the notebook in version control, and any other application opening the ipynb file has to be aware of \r to display it properly.

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.

[minrk]

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.

[ellisonbg]

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 clear_output. I agree with @minrk that we should strongly discourage the usage of \r. If a user uses it, the only promise we should make is that the notebook doesn't break.

[minrk]

@ellisonbg: Both before/after this PR there are significant disadvantages to using \r:

• before: \r does nothing, and your output will simply grow, and make no sense.
• after: \r behaves sanely for simple cases, but your notebook file stores a complete record of the erased output.

Also note that #1663 is required for \r to make sense across saves.

[ellisonbg]

On Thu, May 24, 2012 at 3:42 PM, Min RK
reply@reply.github.com
wrote:

@ellisonbg: Both before/after this PR there are significant disadvantages to using \r:

• before: \r does nothing, and your output will simply grow, and make no sense.
• after: \r behaves sanely for simple cases, but your notebook file stores a complete record of the erased output.

Also note that #1663 is required for \r to make sense across saves.

In general, dynamic output should only "run" after direct code
execution. For example, dynamic Javascirpt is never run on notebook
load. Are you saying that after this PR, when a notebook is loaded,
the \r logic gets replayed? What is the most conservative thing we
could do? I am a little weary of heading down the road to making the
notebook notebook behave like a terminal.

---

Reply to this email directly or view it on GitHub:
#1674 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

[minrk]

Yes, \r is 'replayed'. That javascript behavior is also a problem we need to address. For instance, let's say you have a progress bar which was advanced with 100 display(Javascript...) calls. When you reload the notebook, you will get 100 text reprs of Javascript.... It's okay (if unpleasant) if that javascript must not be replayed, but it's not okay that it gets displayed as its text repr.

[takluyver]

On 24 May 2012 23:37, Brian E. Granger
reply@reply.github.com
wrote:

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 clear_output.

I don't see how this is practical. Say I'm writing some long-running
function which I want to display progress while it runs. The users
might be using the notebook, but they may well be using Python in a
terminal. (And there may not be a stdout at all, but there are already
ways to detect and handle that).

I could make the function:

• Always use the notebook display API - completely breaks running in
  other environments, so no.
• Try to detect when output is going to a notebook - by design there's
  no way to do this.
• Switch between notebook & terminal APIs manually - this just pushes
  the decision to the user and gives them one more thing to think about.
  If the terminal API vaguely appears to work, users aren't likely to go
  looking for the notebook_display_api flag just because we think it's
  better.

[minrk]

@takluyver You can already do simultaneous text/html/javascript display, which get used selectively by the frontends (including terminal).

[takluyver]

On 25 May 2012 00:21, Min RK
reply@reply.github.com
wrote:

@takluyver You can already do simultaneous text/html/javascript display, which get used selectively by the frontends (including terminal).

I know, but I've spent a while trying to work out the implementation
you'd need for this in the terminal, and as far as I can see it's very
ugly at best. Using print doesn't go through the display interface,
so you'd need to manually invoke sys.displayhook to display an object
whose repr is the current state. But the default sys.displayhook adds
a newline after printing your object, which will stop you from using
\r to erase it. So you need to set a custom displayhook while you're
doing this. But you don't want that to replace the IPython displayhook
if that's in use...and we're back to detecting the environment. Am I
missing something obvious?

[ellisonbg]

@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.

[ellisonbg]

@takluyver Having notebook specific code is inevitable. We already have this in IPython.parallel with the wait_interactive method that uses clear_output to display a status of tasks that are being completed. The reality is that the notebook is much more powerful that the other frontends. To leverage that power, people are going to have to write code that can't work in the other frontends or in regular Python. This gap between the different frontends is only going to get larger over time as we and others build more widgety things for the notebook.

[minrk]

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.

[takluyver]

On 26 May 2012 05:58, Brian E. Granger
reply@reply.github.com
wrote:

 To leverage that power, people are going to have to write code that can't work in the other frontends or in regular Python.

I don't see this view flying, especially because people can write a
progress bar that works in regular Python at a terminal. It's one
thing for our own code to be notebook-specific, but third party
libraries like PyMC are unlikely to use progress bars that will only
work in the notebook. And of course non-Python tools like wget, which
I can easily and usefully invoke from the notebook, are not going to
use the notebook's API.

[ellisonbg]

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.

[mdboom]

Rebased.

[minrk]

I think the rebase didn't go quite right. This now gets double-fixed, which results in escaped HTML in the notebook output.

[minrk]

In fact, I think the behavior is correct if this is just: var text = json.text, with no other changes.

[mdboom]

I think this is fixed now. Sorry -- it was kind of a hairy rebase...

[ellisonbg]

So can we merge this now?

[minrk]

Yes, merging now. Thanks @mdboom!