diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index be1d7055..bd0f6381 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -310,19 +310,17 @@ GridView.gridCommands = { // Re-define editField after fieldEditSave to make it take precedence for the Enter key. editField: function() { closeRegisteredMenu(); this.scrollToCursor(true); this.activateEditorAtCursor(); }, - insertFieldBefore: function() { - if (GRIST_NEW_COLUMN_MENU()) { + insertFieldBefore: function(maybeKeyboardEvent) { + if (GRIST_NEW_COLUMN_MENU() && !maybeKeyboardEvent) { this._openInsertColumnMenu(this.cursor.fieldIndex()); } else { - // FIXME: remove once New Column menu is enabled by default. this.insertColumn(null, {index: this.cursor.fieldIndex()}); } }, - insertFieldAfter: function() { - if (GRIST_NEW_COLUMN_MENU()) { + insertFieldAfter: function(maybeKeyboardEvent) { + if (GRIST_NEW_COLUMN_MENU() && !maybeKeyboardEvent) { this._openInsertColumnMenu(this.cursor.fieldIndex() + 1); } else { - // FIXME: remove once New Column menu is enabled by default. this.insertColumn(null, {index: this.cursor.fieldIndex() + 1}); } }, diff --git a/app/client/ui/GridViewMenus.ts b/app/client/ui/GridViewMenus.ts index 7f4192a2..dcdc26b4 100644 --- a/app/client/ui/GridViewMenus.ts +++ b/app/client/ui/GridViewMenus.ts @@ -22,7 +22,7 @@ import { } from 'app/client/ui2018/menus'; import {Sort} from 'app/common/SortSpec'; import {dom, DomElementArg, styled} from 'grainjs'; -import {RecalcWhen} from "app/common/gristTypes"; +import {isListType, RecalcWhen} from "app/common/gristTypes"; import {ColumnRec} from "app/client/models/entities/ColumnRec"; import * as weasel from 'popweasel'; import isEqual = require('lodash/isEqual'); @@ -42,6 +42,7 @@ export function buildOldAddColumnMenu(gridView: GridView, viewSection: ViewSecti } export function buildAddColumnMenu(gridView: GridView, index?: number) { + const isSummaryTable = Boolean(gridView.viewSection.table().summarySourceTable()); return [ menuItem( async () => { await gridView.insertColumn(null, {index}); }, @@ -50,8 +51,10 @@ export function buildAddColumnMenu(gridView: GridView, index?: number) { testId('new-columns-menu-add-new'), ), buildHiddenColumnsMenuItems(gridView, index), - buildLookupSection(gridView, index), - buildShortcutsMenuItems(gridView, index), + isSummaryTable ? null : [ + buildLookupSection(gridView, index), + buildShortcutsMenuItems(gridView, index), + ], ]; } @@ -190,51 +193,63 @@ function buildDetectDuplicatesMenuItems(gridView: GridView, index?: number) { const {viewSection} = gridView; return menuItemSubmenu( () => searchableMenu( - viewSection.columns().map((col) => ({ - cleanText: col.label().trim().toLowerCase(), - label: col.label(), - action: async () => { - await gridView.gristDoc.docData.bundleActions(t('Adding duplicates column'), async () => { - const newColInfo = await gridView.insertColumn( - t('Duplicate in {{- label}}', {label: col.label()}), - { - colInfo: { - label: t('Duplicate in {{- label}}', {label: col.label()}), - type: 'Bool', - isFormula: true, - formula: `True if len(${col.table().tableId()}.lookupRecords(` + - `${col.colId()}=$${col.colId()})) > 1 else False`, - recalcWhen: RecalcWhen.DEFAULT, - recalcDeps: null, - widgetOptions: JSON.stringify({ - rulesOptions: [{ - fillColor: '#ffc23d', - textColor: '#262633', - }], - }), - }, - index, - skipPopup: true, - } - ); + viewSection.columns().map((col) => { + function buildFormula() { + if (isListType(col.type())) { + return `any([len(${col.table().tableId()}.lookupRecords(${col.colId()}` + + `=CONTAINS(x))) > 1 for x in $${col.colId()}])`; + } else { + return `$${col.colId()} != "" and $${col.colId()} is not None and ` + + `len(${col.table().tableId()}.lookupRecords(` + + `${col.colId()}=$${col.colId()})) > 1`; + } + } - // TODO: do the steps below as part of the AddColumn action. - const newField = viewSection.viewFields().all() - .find(field => field.colId() === newColInfo.colId); - if (!newField) { - throw new Error(`Unable to find field for column ${newColInfo.colId}`); - } + return { + cleanText: col.label().trim().toLowerCase(), + label: col.label(), + action: async () => { + await gridView.gristDoc.docData.bundleActions(t('Adding duplicates column'), async () => { + const newColInfo = await gridView.insertColumn( + t('Duplicate in {{- label}}', {label: col.label()}), + { + colInfo: { + label: t('Duplicate in {{- label}}', {label: col.label()}), + type: 'Bool', + isFormula: true, + formula: buildFormula(), + recalcWhen: RecalcWhen.DEFAULT, + recalcDeps: null, + widgetOptions: JSON.stringify({ + rulesOptions: [{ + fillColor: '#ffc23d', + textColor: '#262633', + }], + }), + }, + index, + skipPopup: true, + } + ); + + // TODO: do the steps below as part of the AddColumn action. + const newField = viewSection.viewFields().all() + .find(field => field.colId() === newColInfo.colId); + if (!newField) { + throw new Error(`Unable to find field for column ${newColInfo.colId}`); + } - await newField.addEmptyRule(); - const newRule = newField.rulesCols()[0]; - if (!newRule) { - throw new Error(`Unable to find conditional rule for field ${newField.label()}`); - } + await newField.addEmptyRule(); + const newRule = newField.rulesCols()[0]; + if (!newRule) { + throw new Error(`Unable to find conditional rule for field ${newField.label()}`); + } - await newRule.formula.setAndSave(`$${newColInfo.colId}`); - }, {nestInActiveBundle: true}); - }, - })), + await newRule.formula.setAndSave(`$${newColInfo.colId}`); + }, {nestInActiveBundle: true}); + }, + }; + }), {searchInputPlaceholder: t('Search columns')} ), {allowNothingSelected: true}, @@ -354,10 +369,9 @@ function buildLookupSection(gridView: GridView, index?: number){ function buildLookupsMenuItems() { // Function that builds a menu for one of our Ref columns, we will show all columns -// from the referenced table and offer to create a formula column with aggregation in case -// our column is RefList. - function buildRefColMenu( - ref: ColumnRec, col: ColumnRec): SearchableMenuItem { + // from the referenced table and offer to create a formula column with aggregation in case + // our column is RefList. + function buildRefColMenu(ref: ColumnRec, col: ColumnRec): SearchableMenuItem { // Helper for searching for this entry. const cleanText = col.label().trim().toLowerCase(); @@ -422,40 +436,41 @@ function buildLookupSection(gridView: GridView, index?: number){ }); } } + const {viewSection} = gridView; const columns = viewSection.columns(); const onlyRefOrRefList = (c: ColumnRec) => c.pureType() === 'Ref' || c.pureType() === 'RefList'; const references = columns.filter(onlyRefOrRefList); return references.map((ref) => menuItemSubmenu( - () => searchableMenu( - ref.refTable()?.visibleColumns().map(buildRefColMenu.bind(null, ref)) ?? [], - { - searchInputPlaceholder: t('Search columns') - } - ), - {allowNothingSelected: true}, - ref.label(), - testId(`new-columns-menu-lookups-${ref.colId()}`), - ) - ); + () => searchableMenu( + ref.refTable()?.visibleColumns().map(buildRefColMenu.bind(null, ref)) ?? [], + { + searchInputPlaceholder: t('Search columns') + } + ), + {allowNothingSelected: true}, + `${ref.refTable()?.tableNameDef()} [${ref.label()}]`, + testId(`new-columns-menu-lookups-${ref.colId()}`), + )); } + interface RefTable { + tableId: string, + tableName: string, + columns: ColumnRec[], + referenceFields: ColumnRec[] + } function buildReverseLookupsMenuItems() { - interface refTable { - tableId: string, - columns: ColumnRec[], - referenceFields: ColumnRec[] - } - - const getReferencesToThisTable = (): refTable[] => { + const getReferencesToThisTable = (): RefTable[] => { const {viewSection} = gridView; - const otherTables = gridView.gristDoc.docModel.allTables.all() - .filter((tab) => tab.tableId.peek() != viewSection.tableId()); + const otherTables = gridView.gristDoc.docModel.allTables.all().filter((tab) => + tab.summarySourceTable() === 0 && tab.tableId.peek() !== viewSection.tableId()); return otherTables.map((tab) => { return { tableId: tab.tableId(), + tableName: tab.tableNameDef(), columns: tab.visibleColumns(), referenceFields: tab.columns().peek().filter((c) => (c.pureType() === 'Ref' || c.pureType() == 'RefList') && @@ -465,7 +480,7 @@ function buildLookupSection(gridView: GridView, index?: number){ .filter((tab) => tab.referenceFields.length > 0); }; - const buildColumn = async (tab: refTable, col: any, refCol: any, aggregate: string) => { + const buildColumn = async (tab: RefTable, col: ColumnRec, refCol: ColumnRec, aggregate: string) => { const formula = `${tab.tableId}.lookupRecords(${refCol.colId()}= ${refCol.pureType() == 'RefList' ? 'CONTAINS($id)' : '$id'})`; await gridView.insertColumn(`${tab.tableId}_${col.label()}`, { @@ -480,7 +495,7 @@ function buildLookupSection(gridView: GridView, index?: number){ }); }; - const buildSubmenuForRevLookup = (tab: refTable, refCol: any) => { + const buildSubmenuForRevLookup = (tab: RefTable, refCol: any) => { const buildSubmenuForRevLookupMenuItem = (col: ColumnRec): SearchableMenuItem => { const suggestedColumns = suggestAggregation(col); const primarySuggestedColumn = suggestedColumns[0]; @@ -508,11 +523,11 @@ function buildLookupSection(gridView: GridView, index?: number){ tab.columns.map(col => buildSubmenuForRevLookupMenuItem(col)), {searchInputPlaceholder: t('Search columns')} ), - {allowNothingSelected: true}, `${tab.tableId} By ${refCol.label()}`); + {allowNothingSelected: true}, `${tab.tableName} [← ${refCol.label()}]`); }; const tablesWithAnyRefColumn = getReferencesToThisTable(); - return tablesWithAnyRefColumn.map((tab: refTable) => tab.referenceFields.map((refCol) => + return tablesWithAnyRefColumn.map((tab: RefTable) => tab.referenceFields.map((refCol) => buildSubmenuForRevLookup(tab, refCol) )); } diff --git a/app/client/ui2018/menus.ts b/app/client/ui2018/menus.ts index 4ddf75d6..9650ce13 100644 --- a/app/client/ui2018/menus.ts +++ b/app/client/ui2018/menus.ts @@ -576,7 +576,7 @@ export const menuItemAsync: typeof weasel.menuItem = function(action, ...args) { export function menuItemCmd(cmd: Command, label: string, ...args: DomElementArg[]) { return menuItem( - cmd.run, + () => cmd.run(), dom('span', label, testId('cmd-name')), cmd.humanKeys.length ? cssCmdKey(cmd.humanKeys[0]) : null, cssMenuItemCmd.cls(''), // overrides some menu item styles diff --git a/test/nbrowser/GridViewNewColumnMenu.ts b/test/nbrowser/GridViewNewColumnMenu.ts index 3fbbcac1..e49b85b4 100644 --- a/test/nbrowser/GridViewNewColumnMenu.ts +++ b/test/nbrowser/GridViewNewColumnMenu.ts @@ -119,6 +119,22 @@ describe('GridViewNewColumnMenu', function () { assert.deepEqual(columns, ['A', 'B', 'C', 'D']); await gu.undo(); }); + + it('should skip showing menu when inserting with keyboard shortcuts', async function () { + await gu.sendKeys(Key.chord(Key.ALT, '=')); + await gu.waitForServer(); + assert.isFalse(await driver.find('.test-new-columns-menu').isPresent()); + await gu.sendKeys(Key.ENTER); + let columns = await gu.getColumnNames(); + assert.deepEqual(columns, ['A', 'B', 'C', 'D']); + await gu.sendKeys(Key.chord(Key.SHIFT, Key.ALT, '=')); + await gu.waitForServer(); + assert.isFalse(await driver.find('.test-new-columns-menu').isPresent()); + await gu.sendKeys(Key.ENTER); + columns = await gu.getColumnNames(); + assert.deepEqual(columns, ['A', 'B', 'C', 'E', 'D']); + await gu.undo(2); + }); }); describe('hidden columns', function () { @@ -245,7 +261,7 @@ describe('GridViewNewColumnMenu', function () { }); it('should create new column that checks for duplicates in the specified column', async function () { - const menu = await openAddColumnIcon(); + let menu = await openAddColumnIcon(); await menu.findWait('.test-new-columns-menu-shortcuts-duplicates', 100).mouseMove(); await driver.findContentWait('.test-searchable-menu li', 'A', 500).click(); await gu.waitForServer(); @@ -254,12 +270,30 @@ describe('GridViewNewColumnMenu', function () { // Just checking the formula looks plausible - correctness is best left to a python test. assert.equal( await driver.find('.test-formula-editor').getText(), - 'True if len(Table1.lookupRecords(A=$A)) > 1 else False' + '$A != "" and $A is not None and len(Table1.lookupRecords(A=$A)) > 1' ); await gu.sendKeys(Key.ESCAPE); - const columns = await gu.getColumnNames(); + let columns = await gu.getColumnNames(); assert.deepEqual(columns, ['A', 'B', 'C', 'Duplicate in A']); await gu.undo(); + + // Try it with list-based columns; the formula should look a little different. + for (const [label, type] of [['Choice', 'Choice List'], ['Ref', 'Reference List']]) { + await gu.addColumn(label, type); + menu = await openAddColumnIcon(); + await menu.findWait('.test-new-columns-menu-shortcuts-duplicates', 100).mouseMove(); + await driver.findContentWait('.test-searchable-menu li', label, 500).click(); + await gu.waitForServer(); + await gu.sendKeys(Key.ENTER); + assert.equal( + await driver.find('.test-formula-editor').getText(), + `any([len(Table1.lookupRecords(${label}=CONTAINS(x))) > 1 for x in $${label}])` + ); + await gu.sendKeys(Key.ESCAPE); + columns = await gu.getColumnNames(); + assert.deepEqual(columns, ['A', 'B', 'C', label, `Duplicate in ${label}`]); + await gu.undo(4); + } }); });