(core) Tweak Add Column menu

Summary:
Tweaking behavior of the unreleased Add Column menu per feedback from
Anais and Dmitry.

Test Plan: WIP

Reviewers: jarek

Reviewed By: jarek

Differential Revision: https://phab.getgrist.com/D4089

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() {
+  insertFieldBefore: function(maybeKeyboardEvent) {
-    if (GRIST_NEW_COLUMN_MENU()) {
+    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() {
+  insertFieldAfter: function(maybeKeyboardEvent) {
-    if (GRIST_NEW_COLUMN_MENU()) {
+    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});
     }
   },

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),
+    isSummaryTable ? null : [
-    buildShortcutsMenuItems(gridView, index),
+      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) => ({
+      viewSection.columns().map((col) => {
-        cleanText: col.label().trim().toLowerCase(),
+        function buildFormula() {
-        label: col.label(),
+          if (isListType(col.type())) {
-        action: async () => {
+            return `any([len(${col.table().tableId()}.lookupRecords(${col.colId()}` +
-          await gridView.gristDoc.docData.bundleActions(t('Adding duplicates column'), async () => {
+              `=CONTAINS(x))) > 1 for x in $${col.colId()}])`;
-            const newColInfo = await gridView.insertColumn(
+          } else {
-              t('Duplicate in {{- label}}', {label: col.label()}),
+            return `$${col.colId()} != "" and $${col.colId()} is not None and ` +
-              {
+              `len(${col.table().tableId()}.lookupRecords(` +
-                colInfo: {
+              `${col.colId()}=$${col.colId()})) > 1`;
-                  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,
-              }
-            );
 
-            // TODO: do the steps below as part of the AddColumn action.
+        return {
-            const newField = viewSection.viewFields().all()
+          cleanText: col.label().trim().toLowerCase(),
-              .find(field => field.colId() === newColInfo.colId);
+          label: col.label(),
-            if (!newField) {
+          action: async () => {
-              throw new Error(`Unable to find field for column ${newColInfo.colId}`);
+            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();
+              await newField.addEmptyRule();
-            const newRule = newField.rulesCols()[0];
+              const newRule = newField.rulesCols()[0];
-            if (!newRule) {
+              if (!newRule) {
-              throw new Error(`Unable to find conditional rule for field ${newField.label()}`);
+                throw new Error(`Unable to find conditional rule for field ${newField.label()}`);
-            }
+              }
 
-            await newRule.formula.setAndSave(`$${newColInfo.colId}`);
+              await newRule.formula.setAndSave(`$${newColInfo.colId}`);
-          }, {nestInActiveBundle: true});
+            }, {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
+    // from the referenced table and offer to create a formula column with aggregation in case
-// our column is RefList.
+    // our column is RefList.
-    function buildRefColMenu(
+    function buildRefColMenu(ref: ColumnRec, col: ColumnRec): SearchableMenuItem {
-      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(
+      () => searchableMenu(
-          ref.refTable()?.visibleColumns().map(buildRefColMenu.bind(null, ref)) ?? [],
+        ref.refTable()?.visibleColumns().map(buildRefColMenu.bind(null, ref)) ?? [],
-          {
+        {
-            searchInputPlaceholder: t('Search columns')
+          searchInputPlaceholder: t('Search columns')
-          }
+        }
-        ),
+      ),
-        {allowNothingSelected: true},
+      {allowNothingSelected: true},
-        ref.label(),
+      `${ref.refTable()?.tableNameDef()} [${ref.label()}]`,
-        testId(`new-columns-menu-lookups-${ref.colId()}`),
+      testId(`new-columns-menu-lookups-${ref.colId()}`),
-      )
+    ));
-    );
   }
 
+  interface RefTable {
+    tableId: string,
+    tableName: string,
+    columns: ColumnRec[],
+    referenceFields: ColumnRec[]
+  }
 
   function buildReverseLookupsMenuItems() {
-    interface refTable {
+    const getReferencesToThisTable = (): RefTable[] => {
-      tableId: string,
-      columns: ColumnRec[],
-      referenceFields: ColumnRec[]
-    }
-
-    const getReferencesToThisTable = (): refTable[] => {
       const {viewSection} = gridView;
-      const otherTables = gridView.gristDoc.docModel.allTables.all()
+      const otherTables = gridView.gristDoc.docModel.allTables.all().filter((tab) =>
-        .filter((tab) => tab.tableId.peek() != viewSection.tableId());
+        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)
     ));
   }

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

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);
+          }
         });
       });