From c96bf8ba8c44ecabe6b6a6bb521e6e6599f21828 Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Thu, 3 Oct 2024 15:30:52 +0200 Subject: [PATCH 1/8] Reordering columns --- .../GraphEditor/widgets/WidgetTableEditor.vue | 20 +++++++++++-- .../WidgetTableEditor/tableNewArgument.ts | 30 ++++++++++++++++++- .../src/components/shared/AgGridTableView.vue | 4 +++ app/gui2/src/util/data/iterable.ts | 8 +++++ app/ydoc-shared/src/ast/tree.ts | 16 ++++++++++ 5 files changed, 75 insertions(+), 3 deletions(-) diff --git a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue index a8a7ba720acd..7c4efeaa8882 100644 --- a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue +++ b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue @@ -17,7 +17,12 @@ import { Rect } from '@/util/data/rect' import { Vec2 } from '@/util/data/vec2' import '@ag-grid-community/styles/ag-grid.css' import '@ag-grid-community/styles/ag-theme-alpine.css' -import type { CellEditingStartedEvent, CellEditingStoppedEvent, Column } from 'ag-grid-enterprise' +import type { + CellEditingStartedEvent, + CellEditingStoppedEvent, + Column, + ColumnMovedEvent, +} from 'ag-grid-enterprise' import { computed, ref } from 'vue' import type { ComponentExposed } from 'vue-component-type-helpers' @@ -26,7 +31,7 @@ const graph = useGraphStore() const suggestionDb = useSuggestionDbStore() const grid = ref>>() -const { rowData, columnDefs } = useTableNewArgument( +const { rowData, columnDefs, moveColumn } = useTableNewArgument( () => props.input, graph, suggestionDb.entries, @@ -143,6 +148,14 @@ const widgetStyle = computed(() => { } }) +// === Column Dragging === + +function onColumnMoved(event: ColumnMovedEvent) { + if (event.column && event.toIndex != null) { + moveColumn(event.column.getColId(), event.toIndex) + } +} + // === Column Default Definition === const defaultColDef = { @@ -186,6 +199,8 @@ export const widgetDefinition = defineWidget( :components="{ agColumnHeader: TableHeader }" :singleClickEdit="true" :stopEditingWhenCellsLoseFocus="true" + :suppressDragLeaveHidesColumns="true" + :suppressMoveWhenColumnDragging="true" @keydown.enter.stop @keydown.arrow-left.stop @keydown.arrow-right.stop @@ -198,6 +213,7 @@ export const widgetDefinition = defineWidget( @rowDataUpdated="cellEditHandler.rowDataChanged()" @pointerdown.stop @click.stop + @columnMoved="onColumnMoved" /> diff --git a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts index 445a5647c4ef..78353b5ddb51 100644 --- a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts +++ b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts @@ -4,6 +4,7 @@ import type { SuggestionDb } from '@/stores/suggestionDatabase' import { assert } from '@/util/assert' import { Ast } from '@/util/ast' import { tryEnsoToNumber, tryNumberToEnso } from '@/util/ast/abstract' +import * as iterable from '@/util/data/iterable' import { Err, Ok, transposeResult, unwrapOrWithLog, type Result } from '@/util/data/result' import { qnLastSegment, type QualifiedName } from '@/util/qualifiedName' import type { ToValue } from '@/util/reactivity' @@ -271,6 +272,7 @@ export function useTableNewArgument( }, mainMenuItems: ['autoSizeThis', 'autoSizeAll'], contextMenuItems: ['paste', 'separator', removeRowMenuItem], + lockPosition: 'right', })) const rowIndexColumnDef = computed(() => ({ @@ -281,6 +283,7 @@ export function useTableNewArgument( mainMenuItems: ['autoSizeThis', 'autoSizeAll'], contextMenuItems: [removeRowMenuItem], cellStyle: { color: 'rgba(0, 0, 0, 0.4)' }, + lockPosition: 'left', })) const columnDefs = computed(() => { @@ -372,5 +375,30 @@ export function useTableNewArgument( return ast } - return { columnDefs, rowData } + function moveColumn(colId: string, toIndex: number) { + if (columnsAst.value.ok && columnsAst.value.value) { + const edit = graph.startEdit() + const columns = edit.getVersion(columnsAst.value.value) + const fromIndex = iterable.find(columns.enumerate(), ([, ast]) => ast?.id === colId)?.[0] + if (fromIndex != null) { + columns.move(fromIndex, toIndex - 1) + onUpdate({ edit }) + } + } + } + + return { + /** All column definitions, to be passed to AgGrid component. */ + columnDefs, + /** + * Row Data, to be passed to AgGrid component. They do not contain values, but AstIds. + * The column definitions have proper getters for obtaining value from AST. + */ + rowData, + /** Move column in AST. Do not change colunDefs, it is updated upon expected widgetInput change. + * @param colId the id of moved column (as got from `getColId()`) + * @param toIndex the new index of column as in view (counting in the row index column). + */ + moveColumn, + } } diff --git a/app/gui2/src/components/shared/AgGridTableView.vue b/app/gui2/src/components/shared/AgGridTableView.vue index 03531a0e2513..f050e4b9dc3b 100644 --- a/app/gui2/src/components/shared/AgGridTableView.vue +++ b/app/gui2/src/components/shared/AgGridTableView.vue @@ -38,6 +38,8 @@ const _props = defineProps<{ components?: Record singleClickEdit?: boolean stopEditingWhenCellsLoseFocus?: boolean + suppressDragLeaveHidesColumns?: boolean + suppressMoveWhenColumnDragging?: boolean textFormatOption?: TextFormatOptions }>() const emit = defineEmits<{ @@ -189,6 +191,8 @@ const { AgGridVue } = await import('ag-grid-vue3') :components="components" :singleClickEdit="singleClickEdit" :stopEditingWhenCellsLoseFocus="stopEditingWhenCellsLoseFocus" + :suppressDragLeaveHidesColumns="suppressDragLeaveHidesColumns" + :suppressMoveWhenColumnDragging="suppressMoveWhenColumnDragging" @gridReady="onGridReady" @firstDataRendered="updateColumnWidths" @rowDataUpdated="updateColumnWidths($event), emit('rowDataUpdated', $event)" diff --git a/app/gui2/src/util/data/iterable.ts b/app/gui2/src/util/data/iterable.ts index b2dc6378b513..6502292638fd 100644 --- a/app/gui2/src/util/data/iterable.ts +++ b/app/gui2/src/util/data/iterable.ts @@ -11,6 +11,14 @@ export function every(iter: Iterable, f: (value: T) => boolean): boolean { return true } +/** Return the first element returned by the iterable which meets the condition. */ +export function find(iter: Iterable, f: (value: T) => boolean): T | undefined { + for (const value of iter) { + if (f(value)) return value + } + return undefined +} + /** * Return last element returned by the iterable. * NOTE: Linear complexity. This function always visits the whole iterable. Using this with an diff --git a/app/ydoc-shared/src/ast/tree.ts b/app/ydoc-shared/src/ast/tree.ts index 8aa8b4c7a6cd..153e02d19cb6 100644 --- a/app/ydoc-shared/src/ast/tree.ts +++ b/app/ydoc-shared/src/ast/tree.ts @@ -2500,6 +2500,22 @@ export class MutableVector extends Vector implements MutableAst { this.fields.set('elements', elements) } + /** + * Move an element inside vector. + * @param fromIndex index of moved element. If outside array's index range, no element will be + * moved. + * @param toIndex new index of moved element. If outside array's index range, the element will + * be placed at the end. + */ + move(fromIndex: number, toIndex: number) { + const elements = [...this.fields.get('elements')] + const [element] = elements.splice(fromIndex, 1) + if (element != null) { + elements.splice(toIndex, 0, element) + this.fields.set('elements', elements) + } + } + keep(predicate: (ast: Ast) => boolean) { const elements = this.fields.get('elements') const filtered = elements.filter( From 4c0ef4265f0e9da26fc8fffe8c62255366441a21 Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Mon, 7 Oct 2024 17:45:25 +0200 Subject: [PATCH 2/8] Reordering rows --- .../GraphEditor/widgets/WidgetTableEditor.vue | 13 +- .../__tests__/tableNewArgument.test.ts | 140 +++++++++++------- .../WidgetTableEditor/tableNewArgument.ts | 52 +++++-- 3 files changed, 141 insertions(+), 64 deletions(-) diff --git a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue index 7c4efeaa8882..28b4f06510bc 100644 --- a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue +++ b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue @@ -22,6 +22,7 @@ import type { CellEditingStoppedEvent, Column, ColumnMovedEvent, + RowDragEndEvent, } from 'ag-grid-enterprise' import { computed, ref } from 'vue' import type { ComponentExposed } from 'vue-component-type-helpers' @@ -31,7 +32,7 @@ const graph = useGraphStore() const suggestionDb = useSuggestionDbStore() const grid = ref>>() -const { rowData, columnDefs, moveColumn } = useTableNewArgument( +const { rowData, columnDefs, moveColumn, moveRow } = useTableNewArgument( () => props.input, graph, suggestionDb.entries, @@ -148,7 +149,7 @@ const widgetStyle = computed(() => { } }) -// === Column Dragging === +// === Column and Row Dragging === function onColumnMoved(event: ColumnMovedEvent) { if (event.column && event.toIndex != null) { @@ -156,11 +157,18 @@ function onColumnMoved(event: ColumnMovedEvent) { } } +function onRowDragEnd(event: RowDragEndEvent) { + if (event.node.data != null) { + moveRow(event.node.data?.index, event.overIndex) + } +} + // === Column Default Definition === const defaultColDef = { editable: true, resizable: true, + sortable: false, headerComponentParams: { onHeaderEditingStarted: headerEditHandler.headerEditedInGrid.bind(headerEditHandler), onHeaderEditingStopped: headerEditHandler.headerEditingStoppedInGrid.bind(headerEditHandler), @@ -214,6 +222,7 @@ export const widgetDefinition = defineWidget( @pointerdown.stop @click.stop @columnMoved="onColumnMoved" + @rowDragEnd="onRowDragEnd" /> diff --git a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts index fba4cdf1c431..8151113a1b95 100644 --- a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts +++ b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts @@ -11,6 +11,7 @@ import { assert } from '@/util/assert' import { Ast } from '@/util/ast' import { GetContextMenuItems, GetMainMenuItems } from 'ag-grid-enterprise' import { expect, test, vi } from 'vitest' +import { m } from 'vitest/dist/reporters-yx5ZTtEV.js' function suggestionDbWithNothing() { const db = new SuggestionDb() @@ -118,6 +119,35 @@ test.each([ expect(tableNewCallMayBeHandled(ast)).toBeFalsy() }) +function tableEditFixture(code: string, expectedCode: string) { + const ast = Ast.parseBlock(code) + const inputAst = [...ast.statements()][0] + assert(inputAst != null) + const input = WidgetInput.FromAst(inputAst) + const startEdit = vi.fn(() => ast.module.edit()) + const onUpdate = vi.fn((update) => { + const inputAst = [...update.edit.getVersion(ast).statements()][0] + expect(inputAst?.code()).toBe(expectedCode) + }) + const addMissingImports = vi.fn((_, imports) => { + // the only import we're going to add is Nothing. + expect(imports).toEqual([ + { + kind: 'Unqualified', + from: 'Standard.Base.Nothing', + import: 'Nothing', + }, + ]) + }) + const tableNewArgs = useTableNewArgument( + input, + { startEdit, addMissingImports }, + suggestionDbWithNothing(), + onUpdate, + ) + return { tableNewArgs, startEdit, onUpdate, addMissingImports } +} + test.each([ { code: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", @@ -211,29 +241,7 @@ test.each([ expected: "Table.new [['a', [1, 5]], ['a', [3, 4]]]", }, ])('Editing table $code: $description', ({ code, edit, expected, importExpected }) => { - const ast = Ast.parseBlock(code) - const inputAst = [...ast.statements()][0] - assert(inputAst != null) - const input = WidgetInput.FromAst(inputAst) - const onUpdate = vi.fn((update) => { - const inputAst = [...update.edit.getVersion(ast).statements()][0] - expect(inputAst?.code()).toBe(expected) - }) - const addMissingImports = vi.fn((_, imports) => { - expect(imports).toEqual([ - { - kind: 'Unqualified', - from: 'Standard.Base.Nothing', - import: 'Nothing', - }, - ]) - }) - const tableNewArgs = useTableNewArgument( - input, - { startEdit: () => ast.module.edit(), addMissingImports }, - suggestionDbWithNothing(), - onUpdate, - ) + const { tableNewArgs, onUpdate, addMissingImports } = tableEditFixture(code, expected) const editedRow = tableNewArgs.rowData.value[edit.row] assert(editedRow != null) tableNewArgs.columnDefs.value[edit.column]?.valueSetter?.({ @@ -280,21 +288,8 @@ test.each([ expected: "Table.new [['a', []], ['b', []]]", }, ])('Removing $removedRowIndex row in $code', ({ code, removedRowIndex, expected }) => { - const ast = Ast.parseBlock(code) - const inputAst = [...ast.statements()][0] - assert(inputAst != null) - const addMissingImports = vi.fn() - const onUpdate = vi.fn((update) => { - const inputAst = [...update.edit.getVersion(ast).statements()][0] - expect(inputAst?.code()).toBe(expected) - }) - const input = WidgetInput.FromAst(inputAst) - const tableNewArgs = useTableNewArgument( - input, - { startEdit: () => ast.module.edit(), addMissingImports }, - new SuggestionDb(), - onUpdate, - ) + const { tableNewArgs, onUpdate, addMissingImports } = tableEditFixture(code, expected) + const removedRow = tableNewArgs.rowData.value[removedRowIndex] assert(removedRow != null) // Context menu of all cells in given row should work (even the "virtual" columns). @@ -330,21 +325,7 @@ test.each([ expected: 'Table.new []', }, ])('Removing $removedColIndex column in $code', ({ code, removedColIndex, expected }) => { - const ast = Ast.parseBlock(code) - const inputAst = [...ast.statements()][0] - assert(inputAst != null) - const addMissingImports = vi.fn() - const onUpdate = vi.fn((update) => { - const inputAst = [...update.edit.getVersion(ast).statements()][0] - expect(inputAst?.code()).toBe(expected) - }) - const input = WidgetInput.FromAst(inputAst) - const tableNewArgs = useTableNewArgument( - input, - { startEdit: () => ast.module.edit(), addMissingImports }, - new SuggestionDb(), - onUpdate, - ) + const { tableNewArgs, onUpdate, addMissingImports } = tableEditFixture(code, expected) const removedCol = tableNewArgs.columnDefs.value[removedColIndex] assert(removedCol != null) const removeAction = getCustomMenuItemByName('Remove Column', removedCol.mainMenuItems) @@ -359,3 +340,56 @@ test.each([ expect(addMissingImports).not.toHaveBeenCalled() }) + +test.each([ + { + code: "Table.new [['a', [1, 2]], ['b', [3, 4]], ['c', [5, 6]]]", + fromIndex: 1, + toIndex: 3, + expected: "Table.new [ ['b', [3, 4]], ['c', [5, 6]],['a', [1, 2]]]", + }, + { + code: "Table.new [['a', [1, 2]], ['b', [3, 4]], ['c', [5, 6]]]", + fromIndex: 3, + toIndex: 2, + expected: "Table.new [['a', [1, 2]], ['c', [5, 6]], ['b', [3, 4]]]", + }, +])( + 'Moving column $fromIndex to $toIndex in table $code', + ({ code, fromIndex, toIndex, expected }) => { + const { tableNewArgs, onUpdate, addMissingImports } = tableEditFixture(code, expected) + const movedColumnDef = tableNewArgs.columnDefs.value[fromIndex] + assert(movedColumnDef?.colId != null) + tableNewArgs.moveColumn(movedColumnDef.colId, toIndex) + expect(onUpdate).toHaveBeenCalledOnce() + expect(addMissingImports).not.toHaveBeenCalled() + }, +) + +test.each([ + { + code: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", + fromIndex: 1, + toIndex: 2, + expected: "Table.new [['a', [1, 3, 2]], ['b', [4, 6, 5]]]", + }, + { + code: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", + fromIndex: 2, + toIndex: 0, + expected: "Table.new [['a', [ 3,1, 2]], ['b', [ 6,4, 5]]]", + }, + { + code: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", + fromIndex: 1, + toIndex: -1, + expected: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", + }, +])('Moving row $fromIndex to $toIndex in table $code', ({ code, fromIndex, toIndex, expected }) => { + const { tableNewArgs, onUpdate, addMissingImports } = tableEditFixture(code, expected) + tableNewArgs.moveRow(fromIndex, toIndex) + if (code !== expected) { + expect(onUpdate).toHaveBeenCalledOnce() + } + expect(addMissingImports).not.toHaveBeenCalled() +}) diff --git a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts index 78353b5ddb51..ca84db5d4b54 100644 --- a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts +++ b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts @@ -37,6 +37,7 @@ export interface ColumnDef extends ColDef { valueSetter?: ({ data, newValue }: { data: RowData; newValue: any }) => boolean mainMenuItems: (string | MenuItem)[] contextMenuItems: (string | MenuItem)[] + rowDrag?: ({ data }: { data: RowData | undefined }) => boolean } namespace cellValueConversion { @@ -284,6 +285,7 @@ export function useTableNewArgument( contextMenuItems: [removeRowMenuItem], cellStyle: { color: 'rgba(0, 0, 0, 0.4)' }, lockPosition: 'left', + rowDrag: ({ data }) => data?.index != null && data.index < rowCount.value, })) const columnDefs = computed(() => { @@ -376,15 +378,40 @@ export function useTableNewArgument( } function moveColumn(colId: string, toIndex: number) { - if (columnsAst.value.ok && columnsAst.value.value) { - const edit = graph.startEdit() - const columns = edit.getVersion(columnsAst.value.value) - const fromIndex = iterable.find(columns.enumerate(), ([, ast]) => ast?.id === colId)?.[0] - if (fromIndex != null) { - columns.move(fromIndex, toIndex - 1) - onUpdate({ edit }) - } + if (!columnsAst.value.ok) { + columnsAst.value.error.log('Cannot reorder columns: The table AST is not available') + return + } + if (!columnsAst.value.value) { + console.error('Cannot reorder columns on placeholders! This should not be possible in the UI') + return + } + const edit = graph.startEdit() + const columns = edit.getVersion(columnsAst.value.value) + const fromIndex = iterable.find(columns.enumerate(), ([, ast]) => ast?.id === colId)?.[0] + if (fromIndex != null) { + columns.move(fromIndex, toIndex - 1) + onUpdate({ edit }) + } + } + + function moveRow(rowIndex: number, overIndex: number) { + if (!columnsAst.value.ok) { + columnsAst.value.error.log('Cannot reorder rows: The table AST is not available') + return + } + if (!columnsAst.value.value) { + console.error('Cannot reorder rows on placeholders! This should not be possible in the UI') + return } + // If dragged out of grid, we do nothing. + if (overIndex === -1) return + const edit = graph.startEdit() + for (const col of columns.value) { + const editedCol = edit.getVersion(col.data) + editedCol.move(rowIndex, overIndex) + } + onUpdate({ edit }) } return { @@ -395,10 +422,17 @@ export function useTableNewArgument( * The column definitions have proper getters for obtaining value from AST. */ rowData, - /** Move column in AST. Do not change colunDefs, it is updated upon expected widgetInput change. + /** Move column in AST. Do not change colunDefs, they are updated upon expected widgetInput change. * @param colId the id of moved column (as got from `getColId()`) * @param toIndex the new index of column as in view (counting in the row index column). */ moveColumn, + /** + * Move row in AST. Do not change rowData, its updated upon expected widgetInput change. + * @param rowIndex the index of moved row. + * @param overIndex the index of row over which this row was dropped, as in RowDragEndEvent's + * `overIndex` (the -1 case is handled) + */ + moveRow, } } From 74483fb036a077aaa9a11cca6cc2c4d1173d4db1 Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Tue, 8 Oct 2024 15:28:18 +0200 Subject: [PATCH 3/8] Fix spacing and tests --- .../__tests__/tableNewArgument.test.ts | 8 ++-- .../src/util/ast/__tests__/abstract.test.ts | 30 +++++++++++---- app/ydoc-shared/src/ast/tree.ts | 37 +++++++++++++++++-- 3 files changed, 59 insertions(+), 16 deletions(-) diff --git a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts index 8151113a1b95..7d240483bad4 100644 --- a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts +++ b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts @@ -270,7 +270,7 @@ test.each([ { code: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", removedRowIndex: 0, - expected: "Table.new [['a', [ 2, 3]], ['b', [ 5, 6]]]", + expected: "Table.new [['a', [2, 3]], ['b', [5, 6]]]", }, { code: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", @@ -307,7 +307,7 @@ test.each([ { code: "Table.new [['a', [1, 2]], ['b', [3, 4]], ['c', [5, 6]]]", removedColIndex: 1, - expected: "Table.new [ ['b', [3, 4]], ['c', [5, 6]]]", + expected: "Table.new [['b', [3, 4]], ['c', [5, 6]]]", }, { code: "Table.new [['a', [1, 2]], ['b', [3, 4]], ['c', [5, 6]]]", @@ -346,7 +346,7 @@ test.each([ code: "Table.new [['a', [1, 2]], ['b', [3, 4]], ['c', [5, 6]]]", fromIndex: 1, toIndex: 3, - expected: "Table.new [ ['b', [3, 4]], ['c', [5, 6]],['a', [1, 2]]]", + expected: "Table.new [['b', [3, 4]], ['c', [5, 6]], ['a', [1, 2]]]", }, { code: "Table.new [['a', [1, 2]], ['b', [3, 4]], ['c', [5, 6]]]", @@ -377,7 +377,7 @@ test.each([ code: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", fromIndex: 2, toIndex: 0, - expected: "Table.new [['a', [ 3,1, 2]], ['b', [ 6,4, 5]]]", + expected: "Table.new [['a', [3, 1, 2]], ['b', [6, 4, 5]]]", }, { code: "Table.new [['a', [1, 2, 3]], ['b', [4, 5, 6]]]", diff --git a/app/gui2/src/util/ast/__tests__/abstract.test.ts b/app/gui2/src/util/ast/__tests__/abstract.test.ts index ef60d49544b9..06837297350d 100644 --- a/app/gui2/src/util/ast/__tests__/abstract.test.ts +++ b/app/gui2/src/util/ast/__tests__/abstract.test.ts @@ -1119,14 +1119,13 @@ test.each` expect(vector.code()).toBe(expected) }) -// TODO[ao]: Fix cases where expected spacing feels odd. test.each` initial | predicate | expected ${'[1, 2, "Foo"]'} | ${(ast: Ast.Ast) => ast instanceof Ast.NumericLiteral} | ${'[1, 2]'} ${'[1, "Foo", 3]'} | ${(ast: Ast.Ast) => ast instanceof Ast.NumericLiteral} | ${'[1, 3]'} - ${'["Foo", 2, 3]'} | ${(ast: Ast.Ast) => ast instanceof Ast.NumericLiteral} | ${'[ 2, 3]'} - ${'[1, 2, "Foo"]'} | ${(ast: Ast.Ast) => !(ast instanceof Ast.NumericLiteral)} | ${'[ "Foo"]'} - ${'[1, "Foo", 3]'} | ${(ast: Ast.Ast) => !(ast instanceof Ast.NumericLiteral)} | ${'[ "Foo"]'} + ${'["Foo", 2, 3]'} | ${(ast: Ast.Ast) => ast instanceof Ast.NumericLiteral} | ${'[2, 3]'} + ${'[1, 2, "Foo"]'} | ${(ast: Ast.Ast) => !(ast instanceof Ast.NumericLiteral)} | ${'["Foo"]'} + ${'[1, "Foo", 3]'} | ${(ast: Ast.Ast) => !(ast instanceof Ast.NumericLiteral)} | ${'["Foo"]'} ${'["Foo", 2, 3]'} | ${(ast: Ast.Ast) => !(ast instanceof Ast.NumericLiteral)} | ${'["Foo"]'} ${'[]'} | ${(ast: Ast.Ast) => ast instanceof Ast.NumericLiteral} | ${'[]'} ${'[1, 2, 3]'} | ${(ast: Ast.Ast) => ast.code() != '4'} | ${'[1, 2, 3]'} @@ -1138,7 +1137,6 @@ test.each` expect(vector.code()).toBe(expected) }) -// TODO[ao]: Fix cases where expected spacing feels odd. test.each` initial | expectedVector | expectedValue ${'[1, 2, 3]'} | ${'[1, 2]'} | ${'3'} @@ -1152,12 +1150,11 @@ test.each` expect(vector.code()).toBe(expectedVector) }) -// TODO[ao]: Fix cases where expected spacing feels odd. test.each` initial | start | deletedCount | expectedVector ${'[1, 2, 3]'} | ${1} | ${1} | ${'[1, 3]'} - ${'[1, 2, 3]'} | ${0} | ${1} | ${'[ 2, 3]'} - ${'[1, 2, 3]'} | ${0} | ${2} | ${'[ 3]'} + ${'[1, 2, 3]'} | ${0} | ${1} | ${'[2, 3]'} + ${'[1, 2, 3]'} | ${0} | ${2} | ${'[3]'} ${'[1, 2, 3]'} | ${0} | ${3} | ${'[]'} ${'[3]'} | ${0} | ${1} | ${'[]'} ${'[1, 2, 3]'} | ${2} | ${1} | ${'[1, 2]'} @@ -1168,6 +1165,23 @@ test.each` expect(vector.code()).toBe(expectedVector) }) +test.each` + initial | fromIndex | toIndex | expectedVector + ${'[1, 2, 3]'} | ${0} | ${1} | ${'[2, 1, 3]'} + ${'[1, 2, 3]'} | ${2} | ${0} | ${'[3, 1, 2]'} + ${'[1, 2, 3]'} | ${1} | ${3} | ${'[1, 3, 2]'} + ${'[1, 2, 3]'} | ${3} | ${0} | ${'[1, 2, 3]'} + ${'[]'} | ${0} | ${0} | ${'[]'} +`( + 'Moving element in vector $initial -> $expectedVector', + ({ initial, fromIndex, toIndex, expectedVector }) => { + const vector = Ast.Vector.tryParse(initial) + assertDefined(vector) + vector.move(fromIndex, toIndex) + expect(vector.code()).toBe(expectedVector) + }, +) + test.each` initial | index | value | expected ${'[1, 2, 3]'} | ${0} | ${'4'} | ${'[4, 2, 3]'} diff --git a/app/ydoc-shared/src/ast/tree.ts b/app/ydoc-shared/src/ast/tree.ts index 153e02d19cb6..12998a8cd5f2 100644 --- a/app/ydoc-shared/src/ast/tree.ts +++ b/app/ydoc-shared/src/ast/tree.ts @@ -2496,31 +2496,60 @@ export class MutableVector extends Vector implements MutableAst { splice(start: number, deletedCount: number) { const elements = [...this.fields.get('elements')] + // Spacing around opening brackets should be preserved, as it's more natural (see tests). + const firstSpacing = elements[0]?.value?.whitespace elements.splice(start, deletedCount) + if (start === 0 && firstSpacing != null && elements[0]?.value != null) { + elements[0].value.whitespace = firstSpacing + } this.fields.set('elements', elements) } /** * Move an element inside vector. - * @param fromIndex index of moved element. If outside array's index range, no element will be - * moved. - * @param toIndex new index of moved element. If outside array's index range, the element will - * be placed at the end. + * @param fromIndex index of moved element. + * @param toIndex new index of moved element. + * + * If any index is outside array index range, it's interpreted same as in {@link Array.splice}. */ move(fromIndex: number, toIndex: number) { const elements = [...this.fields.get('elements')] + const firstSpacing = elements[0]?.value?.whitespace const [element] = elements.splice(fromIndex, 1) if (element != null) { elements.splice(toIndex, 0, element) + // Restore/automate spacing to look make array look more natural. + // 1. Keep the spacing after opening bracket + if ( + (toIndex === 0 || fromIndex === 0) && + firstSpacing != null && + elements[0]?.value != null + ) { + elements[0].value.whitespace = firstSpacing + } + // 2. If the first element was moved, automate the spacing (often from no-space to + // single-space) + if (fromIndex === 0 && toIndex !== 0 && elements[toIndex]?.value != null) { + elements[toIndex].value.whitespace = undefined + } + // 3. If the first element was shifted by inserting element before, also automate its spacing + if (toIndex === 0 && elements[1]?.value != null) { + elements[1].value.whitespace = undefined + } this.fields.set('elements', elements) } } keep(predicate: (ast: Ast) => boolean) { const elements = this.fields.get('elements') + // Spacing around opening brackets should be preserved, as it's more natural (see tests). + const firstSpacing = elements[0]?.value?.whitespace const filtered = elements.filter( element => element.value && predicate(this.module.get(element.value.node)), ) + if (firstSpacing != null && filtered[0]?.value != null) { + filtered[0].value.whitespace = firstSpacing + } this.fields.set('elements', filtered) } } From 0672b297cca3aef9efb60c31bf2f9d0ad37e5b5b Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Mon, 14 Oct 2024 11:26:35 +0200 Subject: [PATCH 4/8] Fix problems with column swap. --- .../src/components/GraphEditor/widgets/WidgetTableEditor.vue | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue index 28b4f06510bc..24919da0fa6a 100644 --- a/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue +++ b/app/gui2/src/components/GraphEditor/widgets/WidgetTableEditor.vue @@ -152,7 +152,7 @@ const widgetStyle = computed(() => { // === Column and Row Dragging === function onColumnMoved(event: ColumnMovedEvent) { - if (event.column && event.toIndex != null) { + if (event.column && event.toIndex != null && event.finished) { moveColumn(event.column.getColId(), event.toIndex) } } @@ -169,6 +169,7 @@ const defaultColDef = { editable: true, resizable: true, sortable: false, + lockPinned: true, headerComponentParams: { onHeaderEditingStarted: headerEditHandler.headerEditedInGrid.bind(headerEditHandler), onHeaderEditingStopped: headerEditHandler.headerEditingStoppedInGrid.bind(headerEditHandler), From 0288b6f07b25e8b35e8c8f34b931b12d7e391457 Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Mon, 14 Oct 2024 13:54:31 +0200 Subject: [PATCH 5/8] CHANGELOG --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca6699cceece..79a4ac58429d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,11 @@ - [Rows and Columns may be now removed in Table Input Widget][11151]. The option is available in right-click context menu. +- [Rows and Columns may be now reordered by dragging in Table Input + Widget][11271] [11151]: https://github.com/enso-org/enso/pull/11151 +[11271]: https://github.com/enso-org/enso/pull/11271 #### Enso Standard Library From 5405fe349e53f829c67af93ec478c2686d365df7 Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Tue, 15 Oct 2024 12:08:22 +0200 Subject: [PATCH 6/8] WIP --- .../__tests__/tableNewArgument.test.ts | 1 - app/ydoc-shared/src/ast/tree.ts | 25 ++++++------------- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/app/gui/src/project-view/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts b/app/gui/src/project-view/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts index 7d240483bad4..5d42b4ee6e66 100644 --- a/app/gui/src/project-view/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts +++ b/app/gui/src/project-view/components/GraphEditor/widgets/WidgetTableEditor/__tests__/tableNewArgument.test.ts @@ -11,7 +11,6 @@ import { assert } from '@/util/assert' import { Ast } from '@/util/ast' import { GetContextMenuItems, GetMainMenuItems } from 'ag-grid-enterprise' import { expect, test, vi } from 'vitest' -import { m } from 'vitest/dist/reporters-yx5ZTtEV.js' function suggestionDbWithNothing() { const db = new SuggestionDb() diff --git a/app/ydoc-shared/src/ast/tree.ts b/app/ydoc-shared/src/ast/tree.ts index 12998a8cd5f2..9e6d789c4362 100644 --- a/app/ydoc-shared/src/ast/tree.ts +++ b/app/ydoc-shared/src/ast/tree.ts @@ -2496,11 +2496,9 @@ export class MutableVector extends Vector implements MutableAst { splice(start: number, deletedCount: number) { const elements = [...this.fields.get('elements')] - // Spacing around opening brackets should be preserved, as it's more natural (see tests). - const firstSpacing = elements[0]?.value?.whitespace elements.splice(start, deletedCount) - if (start === 0 && firstSpacing != null && elements[0]?.value != null) { - elements[0].value.whitespace = firstSpacing + if (start === 0 && elements[0]?.value != null) { + elements[0].value.whitespace = undefined } this.fields.set('elements', elements) } @@ -2514,23 +2512,14 @@ export class MutableVector extends Vector implements MutableAst { */ move(fromIndex: number, toIndex: number) { const elements = [...this.fields.get('elements')] - const firstSpacing = elements[0]?.value?.whitespace const [element] = elements.splice(fromIndex, 1) if (element != null) { + element.value = autospaced(element.value?.node) elements.splice(toIndex, 0, element) - // Restore/automate spacing to look make array look more natural. - // 1. Keep the spacing after opening bracket - if ( - (toIndex === 0 || fromIndex === 0) && - firstSpacing != null && - elements[0]?.value != null - ) { - elements[0].value.whitespace = firstSpacing - } - // 2. If the first element was moved, automate the spacing (often from no-space to - // single-space) - if (fromIndex === 0 && toIndex !== 0 && elements[toIndex]?.value != null) { - elements[toIndex].value.whitespace = undefined + // Automate spacing to make array look more natural. + + if (fromIndex === 0 && elements[0]?.value != null) { + elements[0].value.whitespace = undefined } // 3. If the first element was shifted by inserting element before, also automate its spacing if (toIndex === 0 && elements[1]?.value != null) { From 73564ac35057741f327cb478d30ad80055b9fac3 Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Wed, 16 Oct 2024 10:09:14 +0200 Subject: [PATCH 7/8] Apply @kazcw review --- app/ydoc-shared/src/ast/tree.ts | 41 ++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/app/ydoc-shared/src/ast/tree.ts b/app/ydoc-shared/src/ast/tree.ts index 9e6d789c4362..d9968d8c53bc 100644 --- a/app/ydoc-shared/src/ast/tree.ts +++ b/app/ydoc-shared/src/ast/tree.ts @@ -2465,10 +2465,7 @@ export class MutableVector extends Vector implements MutableAst { push(value: Owned) { const elements = this.fields.get('elements') - const element = mapRefs( - delimitVectorElement({ value: autospaced(value) }), - ownedToRaw(this.module, this.id), - ) + const element = this.valueToElement(value) this.fields.set('elements', [...elements, element]) } @@ -2494,12 +2491,11 @@ export class MutableVector extends Vector implements MutableAst { this.fields.set('elements', elements) } - splice(start: number, deletedCount: number) { + splice(start: number, deletedCount: number, ...newValues: Owned[]) { const elements = [...this.fields.get('elements')] - elements.splice(start, deletedCount) - if (start === 0 && elements[0]?.value != null) { - elements[0].value.whitespace = undefined - } + const newElements = newValues.map(value => this.valueToElement(value)) + elements.splice(start, deletedCount, ...newElements) + MutableVector.autospaceElement(elements[start + newValues.length]) this.fields.set('elements', elements) } @@ -2514,17 +2510,10 @@ export class MutableVector extends Vector implements MutableAst { const elements = [...this.fields.get('elements')] const [element] = elements.splice(fromIndex, 1) if (element != null) { - element.value = autospaced(element.value?.node) + MutableVector.autospaceElement(element) elements.splice(toIndex, 0, element) - // Automate spacing to make array look more natural. - - if (fromIndex === 0 && elements[0]?.value != null) { - elements[0].value.whitespace = undefined - } - // 3. If the first element was shifted by inserting element before, also automate its spacing - if (toIndex === 0 && elements[1]?.value != null) { - elements[1].value.whitespace = undefined - } + MutableVector.autospaceElement(elements[fromIndex]) + MutableVector.autospaceElement(elements[toIndex + 1]) this.fields.set('elements', elements) } } @@ -2541,6 +2530,20 @@ export class MutableVector extends Vector implements MutableAst { } this.fields.set('elements', filtered) } + + private valueToElement(value: Owned) { + return mapRefs( + delimitVectorElement({ value: autospaced(value) }), + ownedToRaw(this.module, this.id), + ) + } + + private static autospaceElement(element: VectorElement | undefined) { + if (element != null) { + element.delimiter.whitespace = undefined + element.value = autospaced(element.value?.node) + } + } } export interface MutableVector extends Vector, MutableAst { values(): IterableIterator From efd70c7a84cfccbfb7b34a048cc1ba2bdf6d5d54 Mon Sep 17 00:00:00 2001 From: Adam Obuchowicz Date: Wed, 16 Oct 2024 16:45:10 +0200 Subject: [PATCH 8/8] Fix lint --- .../GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/gui/src/project-view/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts b/app/gui/src/project-view/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts index 9e0757851367..1e3b86178e2a 100644 --- a/app/gui/src/project-view/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts +++ b/app/gui/src/project-view/components/GraphEditor/widgets/WidgetTableEditor/tableNewArgument.ts @@ -432,7 +432,8 @@ export function useTableNewArgument( * The column definitions have proper getters for obtaining value from AST. */ rowData, - /** Move column in AST. Do not change colunDefs, they are updated upon expected widgetInput change. + /** + * Move column in AST. Do not change colunDefs, they are updated upon expected widgetInput change. * @param colId the id of moved column (as got from `getColId()`) * @param toIndex the new index of column as in view (counting in the row index column). */