-
Notifications
You must be signed in to change notification settings - Fork 20
Add clearOutputs()
method to ISharedCodeCell
#330
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?
Conversation
/** | ||
* Clear all outputs from the cell. | ||
*/ | ||
clearOutputs(): void; |
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.
Missing origin?
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.
The origin parameter wasn’t exposed in the updateOutputs()
method, so I followed the same pattern and didn’t expose it in clearOutputs()
either.
jupyter_ydoc/javascript/src/api.ts
Lines 569 to 573 in d989856
updateOutputs( | |
start: number, | |
end: number, | |
outputs: Array<nbformat.IOutput> | |
): void; |
I’m not entirely sure about the reasoning behind this, but if exposing it in the interface is something we should do, I’m happy to make that change.
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.
What do we need origin for in the first place? E.g. this one does not have origin:
jupyter_ydoc/javascript/src/ycell.ts
Lines 803 to 809 in b9168fd
setOutputs(outputs: Array<nbformat.IOutput>): void { | |
this.transact(() => { | |
this._youtputs.delete(0, this._youtputs.length); | |
const newOutputs = this.createOutputs(outputs); | |
this._youtputs.insert(0, newOutputs); | |
}, false); | |
} |
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.
But this one does:
jupyter_ydoc/javascript/src/ycell.ts
Lines 814 to 825 in b9168fd
removeStreamOutput(index: number, start: number, origin: any = null): void { | |
this.transact( | |
() => { | |
const output = this._youtputs.get(index); | |
const prevText = output.get('text') as Y.Text; | |
const length = prevText.length - start; | |
prevText.delete(start, length); | |
}, | |
false, | |
origin | |
); | |
} |
References jupyterlab/jupyterlab#17632
Description
This PR introduces a
clearOutputs()
method to theISharedCodeCell
interface and its implementation inYCodeCell
. This method provides a clean and direct way to remove all outputs from a code cell in the shared model.This change is motivated by a bug in JupyterLab where uncoalesced stream outputs can persist in the cell output area. The root cause is that the frontend coalesces outputs, but the shared model retains multiple entries, making it tricky to clear all outputs using
updateOutputs()
when their lengths differ.