Skip to content
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

Compress notebook output streams before rendering #160667

Merged
merged 8 commits into from
Sep 12, 2022

Conversation

DonJayamanne
Copy link
Contributor

Fixes microsoft/vscode-jupyter#11031
Corresponding PR in Jupyter is here microsoft/vscode-jupyter#11350

@DonJayamanne DonJayamanne force-pushed the donjayamanne/moveTerminalStreamCompressionIntoCore branch from 2e48126 to 8b907e5 Compare September 11, 2022 23:05
@DonJayamanne DonJayamanne self-assigned this Sep 11, 2022
@DonJayamanne DonJayamanne marked this pull request as ready for review September 11, 2022 23:06
@vscodenpa vscodenpa added this to the September 2022 milestone Sep 11, 2022
Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in overall, changes are made in right places and look great. Only two questions before we have it merged.

@DonJayamanne DonJayamanne force-pushed the donjayamanne/moveTerminalStreamCompressionIntoCore branch from c400e4a to 77a6d7a Compare September 12, 2022 19:11

// Remove the previous line if required.
const command = stream.subarray(0, MOVE_CURSOR_1_LINE_COMMAND.length);
if (command[0] === MOVE_CURSOR_1_LINE_COMMAND_BYTES[0] && command[1] === MOVE_CURSOR_1_LINE_COMMAND_BYTES[0] && command[2] === MOVE_CURSOR_1_LINE_COMMAND_BYTES[0]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

rebornix
rebornix previously approved these changes Sep 12, 2022
@DonJayamanne DonJayamanne force-pushed the donjayamanne/moveTerminalStreamCompressionIntoCore branch from e28d0f2 to 1fd5437 Compare September 12, 2022 21:32
@@ -4,9 +4,11 @@
*--------------------------------------------------------------------------------------------*/

import type * as nbformat from '@jupyterlab/nbformat';
import { TextDecoder } from 'util';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this import, not sure if having it explicit would break the browser build.

@DonJayamanne DonJayamanne merged commit 4230c22 into main Sep 12, 2022
@DonJayamanne DonJayamanne deleted the donjayamanne/moveTerminalStreamCompressionIntoCore branch September 12, 2022 22:56
DonJayamanne added a commit that referenced this pull request Sep 14, 2022
DonJayamanne added a commit that referenced this pull request Sep 26, 2022
DonJayamanne added a commit that referenced this pull request Sep 26, 2022
DonJayamanne added a commit that referenced this pull request Oct 9, 2022
DonJayamanne added a commit that referenced this pull request Oct 11, 2022
DonJayamanne added a commit that referenced this pull request Oct 11, 2022
DonJayamanne added a commit that referenced this pull request Oct 11, 2022
DonJayamanne added a commit that referenced this pull request Oct 11, 2022
* Revert "Compress notebook output streams before rendering (#160667)"

This reverts commit 4230c22.

* Compress stream output items

* Minor perf improvements

* Misc

* Comments

* Added tests

* Merge issue

* More merge issues

* Misc

* Address code review comments
@github-actions github-actions bot locked and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VS Code crashes when running a cell that produces a lot of output
3 participants