From a1baf957639daec38a0446e312a069e00f086973 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Collonval?= Date: Tue, 10 Oct 2023 14:08:05 +0200 Subject: [PATCH] Make the collapse margin customizable --- nbdime/args.py | 8 +++ nbdime/webapp/nbdimeserver.py | 1 + packages/nbdime/src/common/basepanel.ts | 32 ++++++------ packages/nbdime/src/common/interfaces.ts | 32 ++++++++++-- packages/nbdime/src/common/mergeview.ts | 54 ++++++++++++-------- packages/nbdime/src/diff/widget/cell.ts | 6 +++ packages/nbdime/src/diff/widget/metadata.ts | 2 +- packages/nbdime/src/diff/widget/notebook.ts | 3 ++ packages/nbdime/src/diff/widget/output.ts | 2 + packages/nbdime/src/merge/widget/cell.ts | 4 +- packages/nbdime/src/merge/widget/metadata.ts | 4 +- packages/nbdime/src/merge/widget/notebook.ts | 4 +- packages/webapp/src/app/diff.ts | 2 + packages/webapp/src/app/merge.ts | 2 + ui-tests/tests/nbdime-diff-test4.spec.ts | 32 +++++++++--- 15 files changed, 133 insertions(+), 55 deletions(-) diff --git a/nbdime/args.py b/nbdime/args.py index 93108668..6c5b17ef 100644 --- a/nbdime/args.py +++ b/nbdime/args.py @@ -288,6 +288,13 @@ def add_web_args(parser, default_port=8888): default=True, help="show unchanged cells by default" ) + parser.add_argument( + '--identical-lines-margin', + dest='identical_lines_margin', + default=2, + type=int, + help="Margin for collapsing identical lines in editor; set to -1 to deactivate.", + ) def add_diff_args(parser): @@ -539,6 +546,7 @@ def args_for_server(arguments): base_url='base_url', hide_unchanged='hide_unchanged', show_base='show_base', + identical_lines_margin='identical_lines_margin', ) ret = {kmap[k]: v for k, v in vars(arguments).items() if k in kmap} if 'persist' in arguments: diff --git a/nbdime/webapp/nbdimeserver.py b/nbdime/webapp/nbdimeserver.py index f6c157c7..e4d98b6f 100644 --- a/nbdime/webapp/nbdimeserver.py +++ b/nbdime/webapp/nbdimeserver.py @@ -50,6 +50,7 @@ def base_args(self): 'savable': fn is not None, 'baseUrl': self.nbdime_base_url, 'hideUnchanged': self.params.get('hide_unchanged', True), + 'collapseIdentical': self.params.get('identical_lines_margin', 2), } if fn: # For reference, e.g. if user wants to download file diff --git a/packages/nbdime/src/common/basepanel.ts b/packages/nbdime/src/common/basepanel.ts index 17a8566f..0fe6a996 100644 --- a/packages/nbdime/src/common/basepanel.ts +++ b/packages/nbdime/src/common/basepanel.ts @@ -2,35 +2,37 @@ // Distributed under the terms of the Modified BSD License. import { Panel } from '@lumino/widgets'; -import type { IDiffWidgetOptions, IMergeWidgetOptions } from './interfaces'; +import type { + IDiffViewOptions, + IDiffWidgetOptions, + IMergeViewOptions, +} from './interfaces'; import type { CodeEditor } from '@jupyterlab/codeeditor'; /** * Common panel for diff views */ -export class DiffPanel extends Panel { - constructor({ model, editorFactory }: IDiffWidgetOptions) { +export class DiffPanel< + T, + U extends IDiffViewOptions = IDiffViewOptions, +> extends Panel { + constructor({ + model, + editorFactory, + ...viewOptions + }: IDiffWidgetOptions & U) { super(); this._editorFactory = editorFactory; this._model = model; + this._viewOptions = viewOptions as U; } protected _editorFactory: CodeEditor.Factory | undefined; protected _model: T; + protected _viewOptions: U; } /** * Common panel for merge views */ -export class MergePanel extends DiffPanel { - constructor({ - model, - editorFactory, - ...viewOptions - }: IDiffWidgetOptions & IMergeWidgetOptions) { - super({ model, editorFactory }); - this._viewOptions = viewOptions; - } - - protected _viewOptions: IMergeWidgetOptions; -} +export class MergePanel extends DiffPanel {} diff --git a/packages/nbdime/src/common/interfaces.ts b/packages/nbdime/src/common/interfaces.ts index 7053bcd3..f1d76cb3 100644 --- a/packages/nbdime/src/common/interfaces.ts +++ b/packages/nbdime/src/common/interfaces.ts @@ -1,10 +1,22 @@ import type { CodeEditor } from '@jupyterlab/codeeditor'; import type { IRenderMimeRegistry } from '@jupyterlab/rendermime'; +/** + * Diff view options + */ +export interface IDiffViewOptions { + /** + * When true stretches of unchanged text will be collapsed in the text editors. + * When a number is given, this indicates the amount of lines to leave visible + * around such stretches (which defaults to 2). Defaults to true. + */ + collapseIdentical?: boolean | number; +} + /** * Common merge view options */ -export interface IMergeWidgetOptions { +export interface IMergeViewOptions extends IDiffViewOptions { /** * Whether to show the base version (4-panels) or not (3-panels). */ @@ -16,18 +28,28 @@ export interface IMergeWidgetOptions { */ // TODO `T` should be scoped down but more API rework will be needed on the model to achieve that // there is definitely room to rationalize the code with more abstract or mixin classes. -export interface IDiffWidgetOptions { +export interface IDiffWidgetOptions extends IDiffViewOptions { + /** + * Diff model + */ model: T; + /** + * Text editor factory + */ editorFactory?: CodeEditor.Factory; } -export interface IMimeDiffWidgetOptions { - model: T; +export interface IMimeDiffWidgetOptions extends IDiffWidgetOptions { + /** + * Rendermime registry + */ rendermime: IRenderMimeRegistry; - editorFactory?: CodeEditor.Factory; } export interface ICellDiffWidgetOptions extends IMimeDiffWidgetOptions { + /** + * Cell mime type + */ // TODO this seems redundant as mimetype is part of the model mimetype: string; } diff --git a/packages/nbdime/src/common/mergeview.ts b/packages/nbdime/src/common/mergeview.ts index c4548564..19575f30 100644 --- a/packages/nbdime/src/common/mergeview.ts +++ b/packages/nbdime/src/common/mergeview.ts @@ -44,7 +44,7 @@ import { createEditorFactory, } from './editor'; -import type { IMergeWidgetOptions } from './interfaces'; +import type { IMergeViewOptions } from './interfaces'; import { valueIn, hasEntries, splitLines } from './util'; @@ -135,7 +135,7 @@ const baseTheme = EditorView.baseTheme({ backgroundColor: 'var(--jp-layout-color2)', border: 'var(--jp-border-width) solid var(--jp-border-color1)', fontSize: '90%', - padding:'0 3px', + padding: '0 3px', borderRadius: '4px', }, }); @@ -515,7 +515,7 @@ const CollapsedRangesField = StateField.define({ /** * Merge view factory options */ -export interface IMergeViewOptions extends Partial { +export interface IMergeViewFactoryOptions extends Partial { /** * Diff between the reference and a remote version */ @@ -538,14 +538,25 @@ export interface IMergeViewOptions extends Partial { /** * A wrapper view for showing StringDiffModels in a MergeView */ -export function createNbdimeMergeView(options: IMergeViewOptions): MergeView { - const { remote, local, merged, readOnly, factory, showBase } = options; +export function createNbdimeMergeView( + options: IMergeViewFactoryOptions, +): MergeView { + const { + remote, + local, + merged, + readOnly, + factory, + collapseIdentical, + showBase, + } = options; let opts: IMergeViewEditorConfiguration = { remote, local, merged, config: { readOnly }, factory: factory ?? createEditorFactory(), + collapseIdentical, showBase, }; @@ -1425,8 +1436,8 @@ export class MergeView extends Panel { const additionalExtensions = inMergeView ? [listener, mergeControlGutter, getCommonEditorExtensions(inMergeView)] : getCommonEditorExtensions(inMergeView); - if(this._collapseIdentical >= 0) { - additionalExtensions.push(CollapsedRangesField) + if (this._collapseIdentical >= 0) { + additionalExtensions.push(CollapsedRangesField); } this._base = new EditorWidget({ @@ -1608,7 +1619,6 @@ export class MergeView extends Panel { * Align the matching lines of the different editors */ alignViews() { - console.log(this._showBase, this._diffViews.length); let lineHeight = this._showBase ? this._base.cm.defaultLineHeight : this._diffViews[0].remoteEditorWidget.cm.defaultLineHeight; @@ -1634,8 +1644,8 @@ export class MergeView extends Panel { let processViewportAlignement = false; const viewports = editors.map(e => ({ from: offsetToPos(e.state.doc, e.viewport.from), - to: offsetToPos(e.state.doc, e.viewport.to) - })) + to: offsetToPos(e.state.doc, e.viewport.to), + })); const addStartSpacers = () => { sumDeltas.forEach((delta, i) => { @@ -1647,10 +1657,10 @@ export class MergeView extends Panel { widget: new PaddingWidget(delta * lineHeight), block: true, side: -1, - }) + }), ); }); - } + }; for (let alignment_ of linesToAlign) { let alignment = this._showBase ? alignment_.slice(0, 3) : alignment_; @@ -1666,15 +1676,17 @@ export class MergeView extends Panel { let correctedDeltas = lineDeltas.map(line => line - minDelta); const afterViewport = alignment.map((a, i) => a > viewports[i].to.line); - if(!afterViewport.some(after => !after)) { + if (!afterViewport.some(after => !after)) { // All follow-up alignments are after the viewport => bail out break; } - const beforeViewport = alignment.map((a, i) => a < viewports[i].from.line) + const beforeViewport = alignment.map( + (a, i) => a < viewports[i].from.line, + ); - if (beforeViewport.some(before => !before)){ - if(!processViewportAlignement) { + if (beforeViewport.some(before => !before)) { + if (!processViewportAlignement) { // First time we add spacer in the viewport => add initial spacers addStartSpacers(); } @@ -1707,13 +1719,15 @@ export class MergeView extends Panel { } }); } else { - correctedDeltas.forEach((delta, i) => { sumDeltas[i] += delta; }); - } + correctedDeltas.forEach((delta, i) => { + sumDeltas[i] += delta; + }); + } } - if(!processViewportAlignement) { + if (!processViewportAlignement) { // There are no spacer in the viewport, we still have to add top spacer - addStartSpacers() + addStartSpacers(); } // Padding at the last line of the editor diff --git a/packages/nbdime/src/diff/widget/cell.ts b/packages/nbdime/src/diff/widget/cell.ts index bf33ad34..3cfef585 100644 --- a/packages/nbdime/src/diff/widget/cell.ts +++ b/packages/nbdime/src/diff/widget/cell.ts @@ -105,6 +105,7 @@ export class CellDiffWidget extends DiffPanel { editorClasses: CURR_DIFF_CLASSES, rendermime: this._rendermime, editorFactory: this._editorFactory, + ...this._viewOptions, }); sourceView.addClass(SOURCE_ROW_CLASS); if (model.executionCount) { @@ -122,6 +123,7 @@ export class CellDiffWidget extends DiffPanel { editorClasses: CURR_DIFF_CLASSES, rendermime: this._rendermime, editorFactory: this._editorFactory, + ...this._viewOptions, }); metadataView.addClass(METADATA_ROW_CLASS); this.addWidget(metadataView); @@ -140,6 +142,7 @@ export class CellDiffWidget extends DiffPanel { editorClasses: CURR_DIFF_CLASSES, rendermime: this._rendermime, editorFactory: this._editorFactory, + ...this._viewOptions, }); container.addWidget(outputsWidget); changed = changed || !o.unchanged || o.added || o.deleted; @@ -159,6 +162,7 @@ export class CellDiffWidget extends DiffPanel { editorClasses: CURR_DIFF_CLASSES, rendermime: this._rendermime, editorFactory: this._editorFactory, + ...this._viewOptions, }); target.addWidget(outputsWidget); changed = changed || !o.unchanged || o.added || o.deleted; @@ -217,6 +221,7 @@ export class CellDiffWidget extends DiffPanel { editorClasses, rendermime, editorFactory, + ...viewOptions }: ICellDiffViewOptions): Panel { let view: Panel; if (model instanceof StringDiffModel) { @@ -236,6 +241,7 @@ export class CellDiffWidget extends DiffPanel { inner = createNbdimeMergeView({ remote: model, factory: editorFactory, + ...viewOptions, }); } if (model.collapsible) { diff --git a/packages/nbdime/src/diff/widget/metadata.ts b/packages/nbdime/src/diff/widget/metadata.ts index 57e7874d..cfacf5fe 100644 --- a/packages/nbdime/src/diff/widget/metadata.ts +++ b/packages/nbdime/src/diff/widget/metadata.ts @@ -22,7 +22,6 @@ const ROOT_METADATA_CLASS = 'jp-Metadata-diff'; * MetadataWidget for changes to Notebook-level metadata */ export class MetadataDiffWidget extends DiffPanel { - // TODO improve typing hierarchy to avoid `Omit` constructor(options: IDiffWidgetOptions) { super(options); console.assert(!this._model.added && !this._model.deleted); @@ -37,6 +36,7 @@ export class MetadataDiffWidget extends DiffPanel { let view: Widget = createNbdimeMergeView({ remote: model, factory: this._editorFactory, + ...this._viewOptions, }); if (model.collapsible) { view = new CollapsiblePanel( diff --git a/packages/nbdime/src/diff/widget/notebook.ts b/packages/nbdime/src/diff/widget/notebook.ts index c3f1d328..42098351 100644 --- a/packages/nbdime/src/diff/widget/notebook.ts +++ b/packages/nbdime/src/diff/widget/notebook.ts @@ -53,6 +53,7 @@ export class NotebookDiffWidget extends DiffPanel { new MetadataDiffWidget({ model: model.metadata, editorFactory: this._editorFactory, + ...this._viewOptions, }), ); } @@ -67,6 +68,7 @@ export class NotebookDiffWidget extends DiffPanel { rendermime, mimetype: model.mimetype, editorFactory: this._editorFactory, + ...this._viewOptions, }), ); } else { @@ -84,6 +86,7 @@ export class NotebookDiffWidget extends DiffPanel { rendermime, mimetype: model.mimetype, editorFactory: this._editorFactory, + ...this._viewOptions, }), ); } diff --git a/packages/nbdime/src/diff/widget/output.ts b/packages/nbdime/src/diff/widget/output.ts index 4cc52090..aed8fb90 100644 --- a/packages/nbdime/src/diff/widget/output.ts +++ b/packages/nbdime/src/diff/widget/output.ts @@ -233,6 +233,7 @@ export class OutputPanel extends DiffPanel { view = createNbdimeMergeView({ remote: stringModel, factory: this._editorFactory, + ...this._viewOptions, }); } } @@ -241,6 +242,7 @@ export class OutputPanel extends DiffPanel { view = createNbdimeMergeView({ remote: model.stringify(), factory: this._editorFactory, + ...this._viewOptions, }); } return view; diff --git a/packages/nbdime/src/merge/widget/cell.ts b/packages/nbdime/src/merge/widget/cell.ts index 45a9c5f2..5eeb4f42 100644 --- a/packages/nbdime/src/merge/widget/cell.ts +++ b/packages/nbdime/src/merge/widget/cell.ts @@ -18,7 +18,7 @@ import { DragPanel } from '../../common/dragpanel'; import type { ICellDiffWidgetOptions, - IMergeWidgetOptions, + IMergeViewOptions, } from '../../common/interfaces'; import { createNbdimeMergeView, MergeView } from '../../common/mergeview'; @@ -87,7 +87,7 @@ export class CellMergeWidget extends MergePanel { merged, readOnly, ...others - }: ICellMergeViewOptions & IMergeWidgetOptions): Widget | null { + }: ICellMergeViewOptions & IMergeViewOptions): Widget | null { let view: Widget | null = null; if (merged instanceof StringDiffModel) { view = createNbdimeMergeView({ diff --git a/packages/nbdime/src/merge/widget/metadata.ts b/packages/nbdime/src/merge/widget/metadata.ts index 8f857d4f..6503c8d6 100644 --- a/packages/nbdime/src/merge/widget/metadata.ts +++ b/packages/nbdime/src/merge/widget/metadata.ts @@ -6,7 +6,7 @@ import type * as nbformat from '@jupyterlab/nbformat'; import type { IDiffWidgetOptions, - IMergeWidgetOptions, + IMergeViewOptions, } from '../../common/interfaces'; import { createNbdimeMergeView, MergeView } from '../../common/mergeview'; @@ -24,7 +24,7 @@ const ROOT_METADATA_CLASS = 'jp-Metadata-diff'; */ export class MetadataMergeWidget extends MergePanel { constructor( - options: IDiffWidgetOptions & IMergeWidgetOptions, + options: IDiffWidgetOptions & IMergeViewOptions, ) { super(options); this.addClass(ROOT_METADATA_CLASS); diff --git a/packages/nbdime/src/merge/widget/notebook.ts b/packages/nbdime/src/merge/widget/notebook.ts index f8d5a789..586c7cbe 100644 --- a/packages/nbdime/src/merge/widget/notebook.ts +++ b/packages/nbdime/src/merge/widget/notebook.ts @@ -9,7 +9,7 @@ import type { IRenderMimeRegistry } from '@jupyterlab/rendermime'; import { MergePanel } from '../../common/basepanel'; import type { - IMergeWidgetOptions, + IMergeViewOptions, IMimeDiffWidgetOptions, } from '../../common/interfaces'; @@ -37,7 +37,7 @@ export class NotebookMergeWidget extends MergePanel { constructor({ rendermime, ...options - }: IMimeDiffWidgetOptions & IMergeWidgetOptions) { + }: IMimeDiffWidgetOptions & IMergeViewOptions) { super(options); this._rendermime = rendermime; diff --git a/packages/webapp/src/app/diff.ts b/packages/webapp/src/app/diff.ts index 00221d0d..773f6a43 100644 --- a/packages/webapp/src/app/diff.ts +++ b/packages/webapp/src/app/diff.ts @@ -68,12 +68,14 @@ function showDiff(data: { sanitizer: new Sanitizer(), latexTypesetter: new MathJaxTypesetter(), }); + const collapseIdentical = getConfigOption('collapseIdentical', 2); let nbdModel = new NotebookDiffModel(data.base, data.diff); let nbdWidget = new NotebookDiffWidget({ model: nbdModel, rendermime, editorFactory: createEditorFactory(), + collapseIdentical, }); let root = document.getElementById('nbdime-root'); diff --git a/packages/webapp/src/app/merge.ts b/packages/webapp/src/app/merge.ts index 727d268c..4e308558 100644 --- a/packages/webapp/src/app/merge.ts +++ b/packages/webapp/src/app/merge.ts @@ -54,6 +54,7 @@ function showMerge(data: { sanitizer: new Sanitizer(), }); + const collapseIdentical = getConfigOption('collapseIdentical', 2); const showBase = getConfigOption('showBase', true); if (!showBase) { @@ -65,6 +66,7 @@ function showMerge(data: { model: nbmModel, rendermime, editorFactory: createEditorFactory(), + collapseIdentical, showBase, }); diff --git a/ui-tests/tests/nbdime-diff-test4.spec.ts b/ui-tests/tests/nbdime-diff-test4.spec.ts index 1593e6c9..b1e65e89 100644 --- a/ui-tests/tests/nbdime-diff-test4.spec.ts +++ b/ui-tests/tests/nbdime-diff-test4.spec.ts @@ -1,16 +1,32 @@ import { expect, test } from '@playwright/test'; -test.beforeEach(async ({ page }) => { - await page.goto('http://localhost:41000/diff'); - await page.locator('#diff-remote').fill('data/diff_test4/left.ipynb'); - await page.locator('#diff-base').fill('data/diff_test4/center.ipynb'); - await page.getByRole('button', { name: 'Diff files' }).click(); -}); - /* Very long cell */ test.describe('diff test4', () => { test('The file ends must be aligned', async ({ page }) => { - await page.locator('.nbdime-spinner').waitFor({state: 'hidden'}); + const ctxt = page.context(); + page.route(/.+\/diff/, async (route, request) => { + const response = await ctxt.request.fetch(request); + if (!response.ok()) { + route.abort(); + return; + } + + const buffer = await response!.body(); + const content = buffer.toString(); + route.fulfill({ + body: content.replace( + '"collapseIdentical": 2', + '"collapseIdentical": -1', + ), + }); + }); + + await page.goto('http://localhost:41000/diff'); + await page.locator('#diff-remote').fill('data/diff_test4/left.ipynb'); + await page.locator('#diff-base').fill('data/diff_test4/center.ipynb'); + await page.getByRole('button', { name: 'Diff files' }).click(); + + await page.locator('.nbdime-spinner').waitFor({ state: 'hidden' }); await page.waitForTimeout(300); await page.keyboard.press('Control+End'); await page.waitForTimeout(300);