Skip to content

Handle carriage return characters ("\r") in HTML notebook output. #1659

Merged
merged 3 commits into from Apr 27, 2012

4 participants

@mdboom
mdboom commented Apr 25, 2012

It does not currently save/restore notebooks correctly. Any pointers on that?

@mdboom
mdboom commented Apr 25, 2012

To reproduce:

import time
for i in range(10):
    print ("\r" + "#" * i),
    time.sleep(0.1)
print "Step 2"
for i in range(10):
    print ("\r" + "#" * i),
    time.sleep(0.1)

should produce:

#########
Step 2
#########
@jasongrout jasongrout and 2 others commented on an outdated diff Apr 25, 2012
IPython/frontend/html/notebook/static/js/utils.js
@@ -74,6 +73,16 @@ IPython.utils = (function (IPython) {
return txt;
}
+ // Remove chunks that should be overridden by the effect carriage
@jasongrout
IPython member
jasongrout added a note Apr 25, 2012

"effect of" or "effect from" probably, right?

@mdboom
mdboom added a note Apr 25, 2012

Indeed. "effect of" is what I intended.

@minrk
IPython member
minrk added a note Apr 27, 2012

Did you want to make this change?

@mdboom
mdboom added a note Apr 27, 2012

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jasongrout
IPython member

Thanks for bringing this up; we have the same problem on aleph.sagemath.org with our use of the IPython messaging protocol.

@minrk
IPython member
minrk commented Apr 25, 2012

I had a look at the save/restore, and it's a bit problematic.

When we write the ipynb to JSON, we use splitlines() for the text. So roundtrip to files, '\r#' becomes \n#. The change needed for this is to use splitlines(True), and ''.join(lines) instead of splitlines() and '\n'.join(lines). I believe I have a safe fix that will behave properly for notebooks written before/after the fix, but I will have to test carefully to make sure it doesn't cause any problems.

I do want to make a point of discouraging actually using terminal-centric behaviors in the notebook (\r, and ANSI colors, etc.). It's important that people don't think the notebook is a terminal emulator, and it never will be, nor should it be. But \r is simple enough.

@minrk minrk and 1 other commented on an outdated diff Apr 25, 2012
IPython/frontend/html/notebook/static/js/codecell.js
@@ -623,7 +623,8 @@ var IPython = (function (IPython) {
if (json.stream == undefined){
json.stream = 'stdout';
}
- if (!utils.fixConsole(json.text)){
+ var text = utils.fixConsole(json.text);
+ if (!text){
@minrk
IPython member
minrk added a note Apr 25, 2012

This check depended on fixConsole stripping \r. You will have to make a different fix that prevents creation of the HTML element if there is no text to display.

@mdboom
mdboom added a note Apr 25, 2012

The problem I ran into is that you have to do something now if text == '\r', is the last line of the pre-existing content needs to be erased, so you can't just return anymore in that case. The line removal could be done later, I suppose, if all we had was '\r', but care would need to be taken to not throw the '\r' away.

@minrk
IPython member
minrk added a note Apr 25, 2012

right - the check needs to be later, then.

Note that there is only one problem case:

Creating a new div, with no content, so that's the situation to fix. If it enters the 'append to latest ouput' block, the problem is already avoided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@minrk minrk referenced this pull request Apr 25, 2012
Merged

Keep line-endings in ipynb #1663

@minrk
IPython member
minrk commented Apr 25, 2012

After PR #1663, this should load from saves properly without further changes.

@mdboom mdboom Fix the "test for nothing was streamed" so it doesn't add empty eleme…
…nts -- but only when there wasn't already something there.
13d934d
@mdboom
mdboom commented Apr 26, 2012

I think I've corrected the test in which to not add a div when the content is empty. This merged with #1663 also fixes the saving/loading issue.

Thanks for helping with this!

@minrk minrk merged commit cb4d9d8 into ipython:master Apr 27, 2012
@minrk
IPython member
minrk commented Apr 27, 2012

thanks, merged.

@minrk minrk added a commit that referenced this pull request Apr 30, 2012
@minrk minrk revert PR #1659
caused critical problems with subprocess output.

See `!ls` for an example.
e921622
@minrk
IPython member
minrk commented Apr 30, 2012

Crap - I merged this prematurely, and it caused a pretty critical failure (try !ls, and you will see no output at all). I reverted the original patch, so can you submit a new PR after testing the output of !ls?

@takluyver
IPython member

Hang on, I've just understood this issue. So if there's a percentage counter that gets overwritten 100 times, we store every percentage, separated by \r characters? That seems unnecessary. Shouldn't we handle \r for display, but then just save the final state, as the user sees it when they hit save?

@minrk
IPython member
minrk commented May 3, 2012

Correct, the notebook structure is a transcript of what actually happened, excluding any UI-specific interpretation. \r is a frontend-specific special character, which is interpreted (in this PR) in the notebook as clearing the preceding line, which is distinct from the terminal, where it simply moves the cursor, and discards no information until overwrites start happening.

This is a difference between \r and clear_output, and further reason why real notebook operations are better than faked terminal-behavior in the notebook. This is acceptable to me, because I consider \r support like this decidedly second-class, and have no problem with it suffering such a disadvantage.

We could store the transformed output, but we would have to be careful that we do not store the escaped HTML output (we had this bug before). If we make that change, then we must do fixCR always on the raw text and before fixConsole, because we would be saving the results of fixCR, and must never save the results of fixConsole.

@takluyver
IPython member

I'd side with removing 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.

@minrk
IPython member
minrk commented May 3, 2012

Do you want to add that as a criterion for PR #1674 (note that this one is already closed)?

@minrk
IPython member
minrk commented May 3, 2012

I think it does make sense to do, my main point is that I don't think it's worth the effort. If someone does feel like doing it, they are welcome, but must be very careful with the points I mentioned.

@jasongrout jasongrout referenced this pull request in sagemath/sagecell Sep 25, 2012
Closed

Handle carriage returns correctly #296

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
@minrk minrk revert PR #1659
caused critical problems with subprocess output.

See `!ls` for an example.
06158dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.