Use cellHandle instead of index when talking to output webview (#14649)

* Use cellHandle instead of index when talking to output webview * Fixed linting errors * Fixed playwright tests - trailing cell was not created
eclipse-theia · Dec 19, 2024 · 8d7fa9e · 8d7fa9e
1 parent ffcea18
commit 8d7fa9e
Show file tree

Hide file tree

Showing 6 changed files with 63 additions and 20 deletions.
diff --git a/packages/notebook/src/browser/view-model/notebook-model.ts b/packages/notebook/src/browser/view-model/notebook-model.ts
@@ -394,7 +394,12 @@ export class NotebookModel implements Saveable, Disposable {
         });
         this.addCellOutputListeners(cells);
 
-        const changes: NotebookCellTextModelSplice<NotebookCellModel>[] = [{ start, deleteCount, newItems: cells }];
+        const changes: NotebookCellTextModelSplice<NotebookCellModel>[] = [{
+            start,
+            deleteCount,
+            newItems: cells,
+            startHandle: this.cells[start]?.handle ?? -1 // -1 in case of new Cells are added at the end.
+        }];
 
         const deletedCells = this.cells.splice(start, deleteCount, ...cells);
 

diff --git a/packages/notebook/src/common/notebook-common.ts b/packages/notebook/src/common/notebook-common.ts
@@ -126,6 +126,11 @@ export interface NotebookCellTextModelSplice<T> {
     start: number,
     deleteCount: number,
     newItems: T[]
+    /**
+     * In case of e.g. deletion, the handle of the first cell that was deleted.
+     * -1 in case of new Cells are added at the end.
+     */
+    startHandle: number,
 };
 
 export enum NotebookCellsChangeType {

diff --git a/packages/plugin-ext/src/main/browser/notebooks/notebook-documents-main.ts b/packages/plugin-ext/src/main/browser/notebooks/notebook-documents-main.ts
@@ -170,7 +170,7 @@ export class NotebookDocumentsMainImpl implements NotebookDocumentsMain {
             if (options.content.cells && options.content.cells.length > 0) {
                 rawEvents.push({
                     kind: NotebookCellsChangeType.ModelChange,
-                    changes: [{ start: 0, deleteCount: 0, newItems: ref.cells.map(NotebookDto.toNotebookCellDto) }]
+                    changes: [{ start: 0, startHandle: 0, deleteCount: 0, newItems: ref.cells.map(NotebookDto.toNotebookCellDto) }]
                 });
             }
             if (options.content.metadata) {

diff --git a/packages/plugin-ext/src/main/browser/notebooks/renderers/cell-output-webview.tsx b/packages/plugin-ext/src/main/browser/notebooks/renderers/cell-output-webview.tsx
@@ -381,22 +381,28 @@ export class CellOutputWebviewImpl implements CellOutputWebview, Disposable {
         const changes: Array<CellsMoved | CellsSpliced> = [];
         const outputChanges: CellOutputChange[] = [];
 
-        const visibleCells = this.notebook.getVisibleCells();
-        const visibleCellLookup = new Set(visibleCells);
+        const visibleCellLookup = new Set(this.notebook.getVisibleCells());
         for (const event of cellEvents) {
             if (event.kind === NotebookCellsChangeType.Move) {
-                changes.push(...event.cells.map((cell, i) => ({
-                    type: 'cellMoved',
-                    cellHandle: event.cells[0].handle,
-                    toIndex: event.newIdx,
-                } as CellsMoved)));
+                changes.push(...event.cells.map((cell, i) => {
+                    const cellMoved: CellsMoved = {
+                        type: 'cellMoved',
+                        cellHandle: event.cells[0].handle, // TODO check this, ask Jonah
+                        toIndex: event.newIdx,
+                    };
+                    return cellMoved;
+                }));
             } else if (event.kind === NotebookCellsChangeType.ModelChange) {
-                changes.push(...event.changes.map(change => ({
-                    type: 'cellsSpliced',
-                    start: this.toVisibleCellIndex(change.start, visibleCells),
-                    deleteCount: change.deleteCount,
-                    newCells: change.newItems.filter(cell => visibleCellLookup.has(cell as NotebookCellModel)).map(cell => cell.handle)
-                } as CellsSpliced)));
+                changes.push(...event.changes.map(change => {
+                    const cellSpliced: CellsSpliced = {
+                        type: 'cellsSpliced',
+                        startCellHandle: change.startHandle,
+                        deleteCount: change.deleteCount,
+                        newCells: change.newItems.filter(cell => visibleCellLookup.has(cell as NotebookCellModel)).map(cell => cell.handle)
+                    };
+                    return cellSpliced;
+                }
+                ));
                 outputChanges.push(...event.changes
                     .flatMap(change => change.newItems)
                     .filter(cell => visibleCellLookup.has(cell as NotebookCellModel) && cell.outputs.length)
@@ -429,8 +435,21 @@ export class CellOutputWebviewImpl implements CellOutputWebview, Disposable {
         }));
     }
 
-    protected toVisibleCellIndex(index: number, visibleCells: Array<NotebookCellModel>): number {
-        return visibleCells.indexOf(this.notebook.cells[index]);
+    /**
+     * Currently not used, but could be useful in a subclasses
+     *
+     * @param index cell index
+     * @param cellHandle cell handle
+     * @param visibleCells  visible cells
+     * @returns visible cell index or -1 if not found
+     */
+    protected toVisibleCellIndex(index: number, cellHandle: number, visibleCells: Array<NotebookCellModel>): number {
+        const cell = this.notebook.cells[index];
+        if (cell.handle === cellHandle) {
+            return visibleCells.indexOf(cell);
+        }
+        // in case of deletion index points to a non-existing cell
+        return -1;
     }
 
     setCellHeight(cell: NotebookCellModel, height: number): void {

diff --git a/packages/plugin-ext/src/main/browser/notebooks/renderers/output-webview-internal.ts b/packages/plugin-ext/src/main/browser/notebooks/renderers/output-webview-internal.ts
@@ -665,8 +665,18 @@ export async function outputWebviewPreload(ctx: PreloadContext): Promise<void> {
                     container.appendChild(cell.element);
                 }
             } else if (change.type === 'cellsSpliced') {
-                const deltedCells = cells.splice(change.start, change.deleteCount, ...change.newCells.map((cellHandle, i) => new OutputCell(cellHandle, change.start + i)));
-                deltedCells.forEach(cell => cell.dispose());
+                // if startCellHandle is negative, it means we should add a trailing new cell
+                const startCellIndex = change.startCellHandle < 0 ? cells.length : cells.findIndex(c => c.cellHandle === change.startCellHandle);
+                if (startCellIndex === -1) {
+                    console.error(`Can't find cell output to splice. Cells: ${cells.length}, startCellHandle: ${change.startCellHandle}`);
+                } else {
+                    const deletedCells = cells.splice(
+                        startCellIndex,
+                        change.deleteCount,
+                        ...change.newCells.map((cellHandle, i) => new OutputCell(cellHandle, startCellIndex + i))
+                    );
+                    deletedCells.forEach(cell => cell.dispose());
+                }
             }
         }
     }

diff --git a/packages/plugin-ext/src/main/browser/notebooks/renderers/webview-communication.ts b/packages/plugin-ext/src/main/browser/notebooks/renderers/webview-communication.ts
@@ -83,7 +83,11 @@ export interface CellsMoved {
 
 export interface CellsSpliced {
     type: 'cellsSpliced';
-    start: number;
+    /**
+     * Cell handle for the start cell.
+     * -1 in case of new Cells are added at the end.
+     */
+    startCellHandle: number;
     deleteCount: number;
     newCells: number[];
 }