Skip to content

Fixes bugs #11777 and #11726, 'outputPrepend' needed to be cleared on each cell execute. #11871

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 18, 2020
Merged

Fixes bugs #11777 and #11726, 'outputPrepend' needed to be cleared on each cell execute. #11871

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
08fb451
Fixes bugs #11777 and #11726
BarryNolte May 17, 2020
1811e46
Fixes bugs #11777 and #11726
BarryNolte May 17, 2020
fff6920
Merge branch 'Truncation-Message-Fix' of https://github.com/BarryNolt…
BarryNolte May 18, 2020
eee2590
Merge branch 'Truncation-Message-Fix' of https://github.com/BarryNolt…
BarryNolte May 18, 2020
45002a7
Merge branch 'Truncation-Message-Fix' of https://github.com/BarryNolt…
BarryNolte May 18, 2020
e95a1a8
Merge branch 'Truncation-Message-Fix' of https://github.com/BarryNolt…
BarryNolte May 18, 2020
97160bf
Merge branch 'Truncation-Message-Fix' of https://github.com/BarryNolt…
BarryNolte May 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/client/datascience/interactive-ipynb/nativeEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,9 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
// Clear the result if we've run before
await this.clearResult(cell.id);

// Clear 'per run' data passed to WebView before execution
cell.data.metadata.tags = [];
Copy link

Choose a reason for hiding this comment

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

Thanks for submitting this PR, however I fail to see how this will solve the issue.
Doesn't this just blow away all of the tags in the metadata, meaning we're throwing away client data stored in tags.

Will wait for feedback from the rest of the team.

Copy link
Author

Choose a reason for hiding this comment

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

At the moment, this is the only tag that ever gets passed over to the client. Until there is another one added, it's just a single complicated flag.

Copy link

Choose a reason for hiding this comment

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

Yeah this should only be clearing the flag we add. Other metadata tags may have already been there and we don't want to clear them.

In reply to: 426679031 [](ancestors = 426679031)

Copy link

Choose a reason for hiding this comment

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

Additionally we should not blindly push the outputPrepend tag in jupyterNotebook.ts

In reply to: 426727089 [](ancestors = 426727089,426679031)

Copy link

@rchiodo rchiodo May 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hi Barry, should have been more clear there.

cell.data.metadata.tags can be written to by external tools (like this one https://ipypublish.readthedocs.io/en/latest/metadata_tags.html)

We added the outputprepend tag, but we don't want to remove other tags that might already be there. So something like

cell.data.metadata.tags = cell.metadata.tags.filter(t => t !== 'outputPrepend')

would be better.

The real bug is that we're just blindly pushing the outputPrepend tag instead of doing it just once.

Copy link
Author

Choose a reason for hiding this comment

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

Got it. My repro case is too simplistic, as in no external tools involved. That starts to explain a lot. This is a rabbit hole :). I'll submit yet another PR.


const code = concatMultilineStringInput(cell.data.source);
// Send to ourselves.
await this.submitCode(code, Identifiers.EmptyFileName, 0, cell.id, cell.data, undefined, cancelToken);
Expand Down
14 changes: 7 additions & 7 deletions src/client/datascience/jupyter/jupyterNotebook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1283,8 +1283,8 @@ export class JupyterNotebookBase implements INotebook {
trimFunc: (str: string) => string
) {
const data: nbformat.ICodeCell = cell.data as nbformat.ICodeCell;
let originalTextLenght = 0;
let trimmedTextLenght = 0;
let originalTextLength = 0;
Copy link

Choose a reason for hiding this comment

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

Also not sure how this addresses https://github.com/microsoft/vscode-python/issues/11777
I think we need some tests to confirm the fix.
Again will wait for feedback from the rest of the team.

Copy link
Author

Choose a reason for hiding this comment

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

The spelling error fix is a side effect of fixing the actual bug, as in, leave the area nicer than you found it. Clearing the tag, fixes the bug. The truncating always worked, it was when the warning showed up that didn't.

let trimmedTextLength = 0;

// Clear output if waiting for a clear
if (clearState.value) {
Expand All @@ -1301,27 +1301,27 @@ export class JupyterNotebookBase implements INotebook {
// tslint:disable-next-line:restrict-plus-operands
existing.text = existing.text + msg.content.text;
const originalText = formatStreamText(concatMultilineStringOutput(existing.text));
originalTextLenght = originalText.length;
originalTextLength = originalText.length;
existing.text = trimFunc(originalText);
trimmedTextLenght = existing.text.length;
trimmedTextLength = existing.text.length;
} else {
const originalText = formatStreamText(concatMultilineStringOutput(msg.content.text));
originalTextLenght = originalText.length;
originalTextLength = originalText.length;
// Create a new stream entry
const output: nbformat.IStream = {
output_type: 'stream',
name: msg.content.name,
text: trimFunc(originalText)
};
data.outputs = [...data.outputs, output];
trimmedTextLenght = output.text.length;
trimmedTextLength = output.text.length;
cell.data = data;
}

// If the output was trimmed, we add the 'outputPrepend' metadata tag.
// Later, the react side will display a message letting the user know
// the output is trimmed and what setting changes that.
if (trimmedTextLenght < originalTextLenght) {
if (trimmedTextLength < originalTextLength) {
if (!data.metadata.tags) {
data.metadata.tags = ['outputPrepend'];
} else {
Expand Down
4 changes: 2 additions & 2 deletions src/datascience-ui/interactive-common/kernelSelection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,9 @@ export class KernelSelection extends React.Component<IKernelSelectionProps> {
: ImageName.JupyterServerConnected;
}

private getMaxWidth(charLenght: number): string {
private getMaxWidth(charLength: number): string {
// This comes from a linear regression
const width = 0.57674 * charLenght + 1.70473;
const width = 0.57674 * charLength + 1.70473;
const unit = 'em';
return Math.round(width).toString() + unit;
}
Expand Down