Skip to content

Commit

Permalink
add output diff info when the visual change might be invisible.
Browse files Browse the repository at this point in the history
  • Loading branch information
@rebornix
rebornix committed Nov 10, 2021
1 parent dbdd50b commit 2b401e2
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 47 deletions.
31 changes: 19 additions & 12 deletions src/vs/workbench/contrib/notebook/browser/diff/diffComponents.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import { Disposable, DisposableStore } from 'vs/base/common/lifecycle';
import { Schemas } from 'vs/base/common/network';
import { IEditorOptions } from 'vs/editor/common/config/editorOptions';
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
import { DiffElementViewModelBase, getFormatedMetadataJSON, OUTPUT_EDITOR_HEIGHT_MAGIC, PropertyFoldingState, SideBySideDiffElementViewModel, SingleSideDiffElementViewModel } from 'vs/workbench/contrib/notebook/browser/diff/diffElementViewModel';
import { DiffElementViewModelBase, getFormatedMetadataJSON, getFormatedOutputJSON, OUTPUT_EDITOR_HEIGHT_MAGIC, PropertyFoldingState, SideBySideDiffElementViewModel, SingleSideDiffElementViewModel } from 'vs/workbench/contrib/notebook/browser/diff/diffElementViewModel';
import { CellDiffSideBySideRenderTemplate, CellDiffSingleSideRenderTemplate, DiffSide, DIFF_CELL_MARGIN, INotebookTextDiffEditor, NOTEBOOK_DIFF_CELL_INPUT, NOTEBOOK_DIFF_CELL_PROPERTY, NOTEBOOK_DIFF_CELL_PROPERTY_EXPANDED } from 'vs/workbench/contrib/notebook/browser/diff/notebookDiffEditorBrowser';
import { CodeEditorWidget, ICodeEditorWidgetOptions } from 'vs/editor/browser/widget/codeEditorWidget';
import { DiffEditorWidget } from 'vs/editor/browser/widget/diffEditorWidget';
import { IModelService } from 'vs/editor/common/services/modelService';
import { IModeService } from 'vs/editor/common/services/modeService';
import { CellEditType, CellUri, IOutputDto, NotebookCellMetadata } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { CellEditType, CellUri, NotebookCellMetadata } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { ToolBar } from 'vs/base/browser/ui/toolbar/toolbar';
import { IContextMenuService } from 'vs/platform/contextview/browser/contextView';
import { IMenu, IMenuService, MenuId, MenuItemAction } from 'vs/platform/actions/common/actions';
Expand Down Expand Up @@ -99,6 +99,7 @@ export const fixedDiffEditorOptions: IDiffEditorConstructionOptions = {
class PropertyHeader extends Disposable {
protected _foldingIndicator!: HTMLElement;
protected _statusSpan!: HTMLElement;
protected _description!: HTMLElement;
protected _toolbar!: ToolBar;
protected _menu!: IMenu;
protected _propertyExpanded?: IContextKey<boolean>;
Expand All @@ -109,7 +110,7 @@ class PropertyHeader extends Disposable {
readonly notebookEditor: INotebookTextDiffEditor,
readonly accessor: {
updateInfoRendering: (renderOutput: boolean) => void;
checkIfModified: (cell: DiffElementViewModelBase) => boolean;
checkIfModified: (cell: DiffElementViewModelBase) => false | { reason: string | undefined };
getFoldingState: (cell: DiffElementViewModelBase) => PropertyFoldingState;
updateFoldingState: (cell: DiffElementViewModelBase, newState: PropertyFoldingState) => void;
unChangedLabel: string;
Expand All @@ -132,14 +133,20 @@ class PropertyHeader extends Disposable {
this._foldingIndicator.classList.add(this.accessor.prefix);
this._updateFoldingIcon();
const metadataStatus = DOM.append(this.propertyHeaderContainer, DOM.$('div.property-status'));

this._statusSpan = DOM.append(metadataStatus, DOM.$('span'));
this._description = DOM.append(metadataStatus, DOM.$('span.property-description'));

if (metadataChanged) {
this._statusSpan.textContent = this.accessor.changedLabel;
this._statusSpan.style.fontWeight = 'bold';
if (metadataChanged.reason) {
this._description.textContent = metadataChanged.reason;
}
this.propertyHeaderContainer.classList.add('modified');
} else {
this._statusSpan.textContent = this.accessor.unChangedLabel;
this._description.textContent = '';
this.propertyHeaderContainer.classList.remove('modified');
}

Expand All @@ -162,7 +169,7 @@ class PropertyHeader extends Disposable {
const scopedContextKeyService = this.contextKeyService.createScoped(cellToolbarContainer);
this._register(scopedContextKeyService);
const propertyChanged = NOTEBOOK_DIFF_CELL_PROPERTY.bindTo(scopedContextKeyService);
propertyChanged.set(metadataChanged);
propertyChanged.set(!!metadataChanged);
this._propertyExpanded = NOTEBOOK_DIFF_CELL_PROPERTY_EXPANDED.bindTo(scopedContextKeyService);

this._menu = this.menuService.createMenu(this.accessor.menuId, scopedContextKeyService);
Expand Down Expand Up @@ -224,13 +231,17 @@ class PropertyHeader extends Disposable {
if (metadataChanged) {
this._statusSpan.textContent = this.accessor.changedLabel;
this._statusSpan.style.fontWeight = 'bold';
if (metadataChanged.reason) {
this._description.textContent = metadataChanged.reason;
}
this.propertyHeaderContainer.classList.add('modified');
const actions: IAction[] = [];
createAndFillInActionBarActions(this._menu, undefined, actions);
this._toolbar.setActions(actions);
} else {
this._statusSpan.textContent = this.accessor.unChangedLabel;
this._statusSpan.style.fontWeight = 'normal';
this._description.textContent = '';
this.propertyHeaderContainer.classList.remove('modified');
this._toolbar.setActions([]);
}
Expand Down Expand Up @@ -612,16 +623,12 @@ abstract class AbstractElementRenderer extends Disposable {
}
}

private _getFormatedOutputJSON(outputs: IOutputDto[]) {
return JSON.stringify(outputs.map(op => ({ outputs: op.outputs })), undefined, '\t');
}

private _buildOutputEditor() {
this._outputEditorDisposeStore.clear();

if ((this.cell.type === 'modified' || this.cell.type === 'unchanged') && !this.notebookEditor.textModel!.transientOptions.transientOutputs) {
const originalOutputsSource = this._getFormatedOutputJSON(this.cell.original?.outputs || []);
const modifiedOutputsSource = this._getFormatedOutputJSON(this.cell.modified?.outputs || []);
const originalOutputsSource = getFormatedOutputJSON(this.cell.original?.outputs || []);
const modifiedOutputsSource = getFormatedOutputJSON(this.cell.modified?.outputs || []);
if (originalOutputsSource !== modifiedOutputsSource) {
const mode = this.modeService.create('json');
const originalModel = this.modelService.createModel(originalOutputsSource, mode, undefined, true);
Expand Down Expand Up @@ -664,7 +671,7 @@ abstract class AbstractElementRenderer extends Disposable {
}));

this._outputEditorDisposeStore.add(this.cell.modified!.textModel.onDidChangeOutputs(() => {
const modifiedOutputsSource = this._getFormatedOutputJSON(this.cell.modified?.outputs || []);
const modifiedOutputsSource = getFormatedOutputJSON(this.cell.modified?.outputs || []);
modifiedModel.setValue(modifiedOutputsSource);
this._outputHeader.refresh();
}));
Expand All @@ -684,7 +691,7 @@ abstract class AbstractElementRenderer extends Disposable {
this._outputEditorDisposeStore.add(this._outputEditor);

const mode = this.modeService.create('json');
const originaloutputSource = this._getFormatedOutputJSON(
const originaloutputSource = getFormatedOutputJSON(
this.notebookEditor.textModel!.transientOptions.transientOutputs
? []
: this.cell.type === 'insert'
Expand Down
88 changes: 73 additions & 15 deletions src/vs/workbench/contrib/notebook/browser/diff/diffElementViewModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { NotebookTextModel } from 'vs/workbench/contrib/notebook/common/model/no
import { hash } from 'vs/base/common/hash';
import { format } from 'vs/base/common/jsonFormatter';
import { applyEdits } from 'vs/base/common/jsonEdit';
import { ICellOutput, NotebookCellMetadata } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { ICellOutput, IOutputDto, IOutputItemDto, NotebookCellMetadata } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { DiffNestedCellViewModel } from 'vs/workbench/contrib/notebook/browser/diff/diffNestedCellViewModel';
import { URI } from 'vs/base/common/uri';
import { NotebookDiffEditorEventDispatcher, NotebookDiffViewEventType } from 'vs/workbench/contrib/notebook/browser/diff/eventDispatcher';
Expand Down Expand Up @@ -274,8 +274,8 @@ export abstract class DiffElementViewModelBase extends Disposable {
this.editorEventDispatcher.emit([{ type: NotebookDiffViewEventType.CellLayoutChanged, source: this._layoutInfo }]);
}

abstract checkIfOutputsModified(): boolean;
abstract checkMetadataIfModified(): boolean;
abstract checkIfOutputsModified(): false | { reason: string | undefined; };
abstract checkMetadataIfModified(): false | { reason: string | undefined; };
abstract isOutputEmpty(): boolean;
abstract getRichOutputTotalHeight(): number;
abstract getCellByUri(cellUri: URI): IGenericCellViewModel;
Expand Down Expand Up @@ -375,11 +375,28 @@ export class SideBySideDiffElementViewModel extends DiffElementViewModelBase {
}

checkIfOutputsModified() {
return !this.mainDocumentTextModel.transientOptions.transientOutputs && !outputsEqual(this.original?.outputs ?? [], this.modified?.outputs ?? []);
if (this.mainDocumentTextModel.transientOptions.transientOutputs) {
return false;
}

const ret = outputsEqual(this.original?.outputs ?? [], this.modified?.outputs ?? []);

if (ret === OutputComparison.Unchanged) {
return false;
}

return {
reason: ret === OutputComparison.Metadata ? 'Output metadata is changed' : undefined
};
}

checkMetadataIfModified(): boolean {
return hash(getFormatedMetadataJSON(this.mainDocumentTextModel, this.original?.metadata || {}, this.original?.language)) !== hash(getFormatedMetadataJSON(this.mainDocumentTextModel, this.modified?.metadata ?? {}, this.modified?.language));
checkMetadataIfModified() {
const modified = hash(getFormatedMetadataJSON(this.mainDocumentTextModel, this.original?.metadata || {}, this.original?.language)) !== hash(getFormatedMetadataJSON(this.mainDocumentTextModel, this.modified?.metadata ?? {}, this.modified?.language));
if (modified) {
return { reason: undefined };
} else {
return false;
}
}

updateOutputHeight(diffSide: DiffSide, index: number, height: number) {
Expand Down Expand Up @@ -489,11 +506,11 @@ export class SingleSideDiffElementViewModel extends DiffElementViewModelBase {
}


checkIfOutputsModified(): boolean {
checkIfOutputsModified(): false | { reason: string | undefined } {
return false;
}

checkMetadataIfModified(): boolean {
checkMetadataIfModified(): false | { reason: string | undefined } {
return false;
}

Expand Down Expand Up @@ -536,9 +553,15 @@ export class SingleSideDiffElementViewModel extends DiffElementViewModelBase {
}
}

const enum OutputComparison {
Unchanged = 0,
Metadata = 1,
Other = 2
}

function outputsEqual(original: ICellOutput[], modified: ICellOutput[]) {
if (original.length !== modified.length) {
return false;
return OutputComparison.Other;
}

const len = original.length;
Expand All @@ -547,34 +570,34 @@ function outputsEqual(original: ICellOutput[], modified: ICellOutput[]) {
const b = modified[i];

if (hash(a.metadata) !== hash(b.metadata)) {
return false;
return OutputComparison.Metadata;
}

if (a.outputs.length !== b.outputs.length) {
return false;
return OutputComparison.Other;
}

for (let j = 0; j < a.outputs.length; j++) {
const aOutputItem = a.outputs[j];
const bOutputItem = b.outputs[j];

if (aOutputItem.mime !== bOutputItem.mime) {
return false;
return OutputComparison.Other;
}

if (aOutputItem.data.buffer.length !== bOutputItem.data.buffer.length) {
return false;
return OutputComparison.Other;
}

for (let k = 0; k < aOutputItem.data.buffer.length; k++) {
if (aOutputItem.data.buffer[k] !== bOutputItem.data.buffer[k]) {
return false;
return OutputComparison.Other;
}
}
}
}

return true;
return OutputComparison.Unchanged;
}

export function getFormatedMetadataJSON(documentTextModel: NotebookTextModel, metadata: NotebookCellMetadata, language?: string) {
Expand Down Expand Up @@ -604,3 +627,38 @@ export function getFormatedMetadataJSON(documentTextModel: NotebookTextModel, me

return metadataSource;
}

export function getStreamOutputData(outputs: IOutputItemDto[]) {
if (!outputs.length) {
return null;
}

const first = outputs[0];
const mime = first.mime;
const sameStream = !outputs.find(op => op.mime !== mime);

if (sameStream) {
return outputs.map(opit => opit.data.toString()).join('');
} else {
return null;
}
}

export function getFormatedOutputJSON(outputs: IOutputDto[]) {
if (outputs.length === 1) {
const streamOutputData = getStreamOutputData(outputs[0].outputs);
if (streamOutputData) {
return streamOutputData;
}
}

return JSON.stringify(outputs.map(output => {
return ({
metadata: output.metadata,
outputItems: output.outputs.map(opit => ({
mimeType: opit.mime,
data: opit.data.toString()
}))
});
}), undefined, '\t');
}
7 changes: 6 additions & 1 deletion src/vs/workbench/contrib/notebook/browser/diff/notebookDiff.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,15 @@

.notebook-text-diff-editor .cell-diff-editor-container .output-header-container .property-status span,
.notebook-text-diff-editor .cell-diff-editor-container .metadata-header-container .property-status span {
margin: 0 8px;
margin: 0 0 0 8px;
line-height: 21px;
}

.notebook-text-diff-editor .cell-diff-editor-container .output-header-container .property-status span.property-description,
.notebook-text-diff-editor .cell-diff-editor-container .metadata-header-container .property-status span.property-description {
font-style: italic;
}

.notebook-text-diff-editor {
overflow: hidden;
}
Expand Down
22 changes: 3 additions & 19 deletions src/vs/workbench/contrib/notebook/browser/notebook.contribution.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { NotebookEditor } from 'vs/workbench/contrib/notebook/browser/notebookEd
import { isCompositeNotebookEditorInput, NotebookEditorInput, NotebookEditorInputOptions } from 'vs/workbench/contrib/notebook/common/notebookEditorInput';
import { INotebookService } from 'vs/workbench/contrib/notebook/common/notebookService';
import { NotebookService } from 'vs/workbench/contrib/notebook/browser/notebookServiceImpl';
import { CellKind, CellUri, IResolvedNotebookEditorModel, NotebookDocumentBackupData, NotebookWorkingCopyTypeIdentifier, NotebookSetting, IOutputItemDto } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { CellKind, CellUri, IResolvedNotebookEditorModel, NotebookDocumentBackupData, NotebookWorkingCopyTypeIdentifier, NotebookSetting } from 'vs/workbench/contrib/notebook/common/notebookCommon';
import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
import { IUndoRedoService } from 'vs/platform/undoRedo/common/undoRedo';
import { INotebookEditorModelResolverService } from 'vs/workbench/contrib/notebook/common/notebookEditorModelResolverService';
Expand All @@ -45,7 +45,7 @@ import { NotebookEditorWidgetService } from 'vs/workbench/contrib/notebook/brows
import { IJSONContributionRegistry, Extensions as JSONExtensions } from 'vs/platform/jsonschemas/common/jsonContributionRegistry';
import { IJSONSchema, IJSONSchemaMap } from 'vs/base/common/jsonSchema';
import { Event } from 'vs/base/common/event';
import { getFormatedMetadataJSON } from 'vs/workbench/contrib/notebook/browser/diff/diffElementViewModel';
import { getFormatedMetadataJSON, getStreamOutputData } from 'vs/workbench/contrib/notebook/browser/diff/diffElementViewModel';
import { NotebookModelResolverServiceImpl } from 'vs/workbench/contrib/notebook/common/notebookEditorModelResolverServiceImpl';
import { INotebookKernelService } from 'vs/workbench/contrib/notebook/common/notebookKernelService';
import { NotebookKernelService } from 'vs/workbench/contrib/notebook/browser/notebookKernelServiceImpl';
Expand Down Expand Up @@ -372,22 +372,6 @@ class CellInfoContentProvider {
return result;
}

private _getStreamOutputData(outputs: IOutputItemDto[]) {
if (!outputs.length) {
return null;
}

const first = outputs[0];
const mime = first.mime;
const sameStream = !outputs.find(op => op.mime !== mime);

if (sameStream) {
return outputs.map(opit => opit.data.toString()).join('');
} else {
return null;
}
}

async provideOutputTextContent(resource: URI): Promise<ITextModel | null> {
const existing = this._modelService.getModel(resource);
if (existing) {
Expand All @@ -408,7 +392,7 @@ class CellInfoContentProvider {
if (cell.handle === data.handle) {
if (cell.outputs.length === 1) {
// single output
const streamOutputData = this._getStreamOutputData(cell.outputs[0].outputs);
const streamOutputData = getStreamOutputData(cell.outputs[0].outputs);
if (streamOutputData) {
result = this._modelService.createModel(
streamOutputData,
Expand Down

0 comments on commit 2b401e2

Please sign in to comment.