-
-
Notifications
You must be signed in to change notification settings - Fork 11
Add handling for carriage returns and backspaces #43
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,6 +86,60 @@ async def _get_ycell( | |
return ycell | ||
|
||
|
||
def handle_carriage_return(s: str) -> str: | ||
"""Handle text the same way that a terminal emulator would display it | ||
|
||
Args: | ||
s: The message | ||
Returns: | ||
Message with carriage returns handled in the text | ||
""" | ||
lines = s.split('\n') | ||
processed_lines = [] | ||
|
||
for line in lines: | ||
result = [] | ||
i = 0 | ||
while i < len(line): | ||
if line[i] == '\r': | ||
# Move cursor to start of the line | ||
# Reset the result buffer and prepare to overwrite | ||
i += 1 | ||
overwrite_chars = [] | ||
while i < len(line) and line[i] != '\r': | ||
overwrite_chars.append(line[i]) | ||
i += 1 | ||
for j, c in enumerate(overwrite_chars): | ||
if j < len(result): | ||
result[j] = c | ||
else: | ||
result.append(c) | ||
else: | ||
result.append(line[i]) | ||
i += 1 | ||
processed_lines.append(''.join(result)) | ||
|
||
return '\n'.join(processed_lines) | ||
|
||
|
||
def handle_backspace(s: str) -> str: | ||
"""Simulate backspaces in the text | ||
|
||
Args: | ||
s: The message | ||
Returns: | ||
The message with backspaces applied | ||
""" | ||
new_str = [] | ||
for c in s: | ||
if c == '\b': | ||
if len(new_str) > 0 and new_str[-1] not in ('\n', '\r'): | ||
new_str.pop() | ||
else: | ||
new_str.append(c) | ||
return ''.join(new_str) | ||
|
||
|
||
def _output_hook(outputs: list[NotebookNode], ycell: y.Map | None, msg: dict) -> None: | ||
"""Callback on execution request when an output is emitted. | ||
|
||
|
@@ -99,29 +153,45 @@ def _output_hook(outputs: list[NotebookNode], ycell: y.Map | None, msg: dict) -> | |
# FIXME support for version | ||
output = nbformat.v4.output_from_msg(msg) | ||
outputs.append(output) | ||
|
||
if ycell is not None: | ||
cell_outputs = ycell["outputs"] | ||
if msg_type == "stream": | ||
with cell_outputs.doc.transaction(): | ||
text = output["text"] | ||
# FIXME Logic is quite complex at https://github.com/jupyterlab/jupyterlab/blob/7ae2d436fc410b0cff51042a3350ba71f54f4445/packages/outputarea/src/model.ts#L518 | ||
|
||
if text.endswith((os.linesep, "\n")): | ||
text = text[:-1] | ||
|
||
if (not cell_outputs) or (cell_outputs[-1]["name"] != output["name"]): | ||
output["text"] = [text] | ||
output["text"] = [handle_carriage_return(handle_backspace(text))] | ||
cell_outputs.append(output) | ||
else: | ||
last_output = cell_outputs[-1] | ||
last_output["text"].append(text) | ||
old_text = last_output["text"][-1] if len(last_output["text"]) > 0 else "" | ||
combined_text = old_text + text | ||
if '\r' in combined_text or '\b' in combined_text: | ||
if combined_text[-1] == '\r': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's strange to me that this needs to be special-cased here. I hope it can be folded into the handle_...() functions. Why do I say that? A terminal has a state machine processing incoming characters, but never cares whether they arrived in one buffer vs. char-by-char, and a char's handling is independent of being first/middle/last in some buffer. It's possible that modeling the state as block-beta
columns 6
space space state[("old_text")] space space kernel
space space space space space space
handle_cr space handle_b space space space
kernel-- "+ incoming text" -->state
state --> handle_b
handle_b --> handle_cr
handle_cr --> state
More formally, these functions could take 2 args (state, incoming text) and reduce it to updated state? Not sure whether that matters here.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the special case for when incoming text contains \r at the end to ensure the text is shown. Otherwise, the text may be discarded too early and never be shown. So yeah, I guess that is a weakness of the The other issue that you pointed out is due to appending incoming text as new lines when neither the old text nor the new one contain a special character. I don't remember if that was part of the original behavior, but what would be the ideal way to handle that here? I'm thinking of concatenating the new output to the end of the last cell until \n is encountered or doing away with the lists and replacing them with a (text, cursor) pair as you suggested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah yes that's where it gets interesting 🤔 In an ideal world, now that we've had glass TTYs for decades, there'd be some migration path to simpler semantics for apps, where \b would actually eat chars and \r would clear out everything since the last \n.
But your
|
||
suffix = '\r' | ||
combined_text = combined_text[:-1] | ||
else: | ||
suffix = '' | ||
new_text = handle_carriage_return(handle_backspace(combined_text)) + suffix | ||
last_output["text"][-1] = new_text | ||
else: | ||
last_output["text"].append(text) | ||
cell_outputs[-1] = last_output | ||
else: | ||
with cell_outputs.doc.transaction(): | ||
cell_outputs.append(output) | ||
|
||
elif msg_type == "clear_output": | ||
# FIXME msg.content.wait - if true should clear at the next message | ||
outputs.clear() | ||
|
||
if ycell is not None: | ||
del ycell["outputs"][:] | ||
|
||
elif msg_type == "update_display_data": | ||
# FIXME | ||
... | ||
|
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.
foo\n
should not be treated same asfoo
."text": [...]
to newlines?In regular Jupyterlab, here is what I see:

and here is the .ipynb representation:
=> in doc model, "text" array tends to start new item after every newline. 📜
https://nbformat.readthedocs.io/en/latest/format_description.html says it doesn't matter!
However, keeping item-per-line is nice for .ipynb diff-ability.
=> in doc model, the last line does record whether it had a trailing newline. ✍️
=> the UI does not show whether the the last line had trailing newline 🙈
In a terminal, lack of trailing newline is evident by next prompt just appearing on same line e.g.
no trailing>>>
but that'd be a sin against the structuredness of notebooks. And since 1 newline is the common case, UI didn't want to spend vertical space rendering it. (the first two cells have exactly same height)UI does reflect >1 newlines at the end by showing blank lines.
But in theory, future/alternate UI might reflect trailing newline.
For comparison, here is the structure for same cells I'm getting with this branch:
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.
❓ However, it appears the current Jupyterlab is not as lenient as the spec I cited, and does NOT concat multiple items without newlines into one line?
upstream results in json:
which renders all on one line. With this extension (with or without this branch), I get:
which is supposed to be equivalent yet shows up in browser as each number on a separate line!