Changes reorder column events to use source and destination (#6733)

## Motivation for features / changes
To prepare for showing shared hparam columns across runs and scalar
tables, we need to unify the column re-ordering logic to use the same
source/destination parameters (introduced in #6727).

## Technical description of changes
- Changes the DataTable component (shared by runs and scalar tables) to
use the new ReorderColumnEvent type when column order is changed. The
changes propagate to scalar card scalar column editor containers. Most
of the changes are getting the tests to pass with the new signature.
- Changes the metrics reducers to correctly handle these new events.
Logic is mostly unchanged, except that sort order will now be preserved
even after column toggle (previously, toggling would group all disabled
columns together, which destroys relative column order

misc:
- creates moveColumn utility to abstract shared logic between the
hparams and metrics reducers. Removes redundant tests in hparams reducer

## Detailed steps to verify changes work correctly (as executed by you)
- Unit tests pass
- Manually tested sorting behavior

Author: hoonji

tensorboard/webapp/hparams/_redux/BUILD

@@ -67,6 +67,7 @@ tf_ts_library(
         "//tensorboard/webapp/hparams:_types",
         "//tensorboard/webapp/runs/actions",
         "//tensorboard/webapp/widgets/data_table:types",
+        "//tensorboard/webapp/widgets/data_table:utils",
         "@npm//@ngrx/store",
     ],
 )
@@ -174,6 +175,7 @@ tf_ts_library(
         "//tensorboard/webapp/util:types",
         "//tensorboard/webapp/webapp_data_source:http_client_testing",
         "//tensorboard/webapp/widgets/data_table:types",
+        "//tensorboard/webapp/widgets/data_table:utils",
         "@npm//@ngrx/effects",
         "@npm//@ngrx/store",
         "@npm//@types/jasmine",

tensorboard/webapp/hparams/_redux/hparams_reducers.ts

@@ -13,7 +13,8 @@ See the License for the specific language governing permissions and
 limitations under the License.
 ==============================================================================*/
 import {Action, ActionReducer, createReducer, on} from '@ngrx/store';
-import {ColumnHeader, Side} from '../../widgets/data_table/types';
+import {Side} from '../../widgets/data_table/types';
+import {DataTableUtils} from '../../widgets/data_table/utils';
 import * as actions from './hparams_actions';
 import {HparamsState} from './types';
 
@@ -141,28 +142,12 @@ const reducer: ActionReducer<HparamsState, Action> = createReducer(
     actions.dashboardHparamColumnOrderChanged,
     (state, {source, destination, side}) => {
       const {dashboardDisplayedHparamColumns: columns} = state;
-      const sourceIndex = columns.findIndex(
-        (column: ColumnHeader) => column.name === source.name
+      const newColumns = DataTableUtils.moveColumn(
+        columns,
+        source,
+        destination,
+        side
       );
-      let destinationIndex = columns.findIndex(
-        (column: ColumnHeader) => column.name === destination.name
-      );
-      if (sourceIndex === -1 || sourceIndex === destinationIndex) {
-        return state;
-      }
-      if (destinationIndex === -1) {
-        // Use side as a backup to determine source position if destination isn't found.
-        if (side !== undefined) {
-          destinationIndex = side === Side.LEFT ? 0 : columns.length - 1;
-        } else {
-          return state;
-        }
-      }
-
-      const newColumns = [...columns];
-      newColumns.splice(sourceIndex, 1);
-      newColumns.splice(destinationIndex, 0, source);
-
       return {
         ...state,
         dashboardDisplayedHparamColumns: newColumns,

tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts

@@ -18,6 +18,7 @@ import * as actions from './hparams_actions';
 import {reducers} from './hparams_reducers';
 import {buildHparamSpec, buildHparamsState, buildMetricSpec} from './testing';
 import {ColumnHeaderType, Side} from '../../widgets/data_table/types';
+import {DataTableUtils} from '../../widgets/data_table/utils';
 
 describe('hparams/_redux/hparams_reducers_test', () => {
   describe('hparamsFetchSessionGroupsSucceeded', () => {
@@ -594,104 +595,15 @@ describe('hparams/_redux/hparams_reducers_test', () => {
       },
     ];
 
-    it('does nothing if source is not found', () => {
+    it('moves source to destination using moveColumn', () => {
       const state = buildHparamsState({
         dashboardDisplayedHparamColumns: fakeColumns,
       });
-      const state2 = reducers(
-        state,
-        actions.dashboardHparamColumnOrderChanged({
-          source: {
-            type: ColumnHeaderType.HPARAM,
-            name: 'nonexistent_param',
-            displayName: 'Nonexistent param',
-            enabled: false,
-          },
-          destination: {
-            type: ColumnHeaderType.HPARAM,
-            name: 'conv_kernel_size',
-            displayName: 'Conv Kernel Size',
-            enabled: true,
-          },
-          side: Side.LEFT,
-        })
-      );
-
-      expect(state2.dashboardDisplayedHparamColumns).toEqual(fakeColumns);
-    });
-
-    it('does nothing if source equals dest', () => {
-      const state = buildHparamsState({
-        dashboardDisplayedHparamColumns: fakeColumns,
-      });
-      const state2 = reducers(
-        state,
-        actions.dashboardHparamColumnOrderChanged({
-          source: {
-            type: ColumnHeaderType.HPARAM,
-            name: 'conv_kernel_size',
-            displayName: 'Conv Kernel Size',
-            enabled: false,
-          },
-          destination: {
-            type: ColumnHeaderType.HPARAM,
-            name: 'conv_kernel_size',
-            displayName: 'Conv Kernel Size',
-            enabled: true,
-          },
-          side: Side.LEFT,
-        })
-      );
-
-      expect(state2.dashboardDisplayedHparamColumns).toEqual(fakeColumns);
-    });
+      const moveColumnSpy = spyOn(
+        DataTableUtils,
+        'moveColumn'
+      ).and.callThrough();
 
-    [
-      {
-        testDesc: 'to front if side is left',
-        side: Side.LEFT,
-        expectedResult: [
-          fakeColumns[1],
-          fakeColumns[0],
-          ...fakeColumns.slice(2),
-        ],
-      },
-      {
-        testDesc: 'to back if side is right',
-        side: Side.RIGHT,
-        expectedResult: [
-          fakeColumns[0],
-          ...fakeColumns.slice(2),
-          fakeColumns[1],
-        ],
-      },
-    ].forEach(({testDesc, side, expectedResult}) => {
-      it(`if destination not found, moves source ${testDesc}`, () => {
-        const state = buildHparamsState({
-          dashboardDisplayedHparamColumns: fakeColumns,
-        });
-        const state2 = reducers(
-          state,
-          actions.dashboardHparamColumnOrderChanged({
-            source: fakeColumns[1],
-            destination: {
-              type: ColumnHeaderType.HPARAM,
-              name: 'nonexistent param',
-              displayName: 'Nonexistent param',
-              enabled: true,
-            },
-            side,
-          })
-        );
-
-        expect(state2.dashboardDisplayedHparamColumns).toEqual(expectedResult);
-      });
-    });
-
-    it('swaps source and destination positions if destination is found', () => {
-      const state = buildHparamsState({
-        dashboardDisplayedHparamColumns: fakeColumns,
-      });
       const state2 = reducers(
         state,
         actions.dashboardHparamColumnOrderChanged({
@@ -701,6 +613,13 @@ describe('hparams/_redux/hparams_reducers_test', () => {
         })
       );
 
+      // Edge cases are tested by moveColumn tests.
+      expect(moveColumnSpy).toHaveBeenCalledWith(
+        fakeColumns,
+        fakeColumns[1],
+        fakeColumns[0],
+        Side.LEFT
+      );
       expect(state2.dashboardDisplayedHparamColumns).toEqual([
         fakeColumns[1],
         fakeColumns[0],

tensorboard/webapp/metrics/actions/index.ts

@@ -234,8 +234,8 @@ export const sortingDataTable = createAction(
   props<SortingInfo>()
 );
 
-export const dataTableColumnEdited = createAction(
-  '[Metrics] Data table columns edited in edit menu',
+export const dataTableColumnOrderChanged = createAction(
+  '[Metrics] Data table columns order changed',
   props<HeaderEditInfo>()
 );
 

tensorboard/webapp/metrics/internal_types.ts

@@ -16,8 +16,8 @@ import {TimeSelection} from '../widgets/card_fob/card_fob_types';
 import {HistogramMode} from '../widgets/histogram/histogram_types';
 import {
   ColumnHeader,
-  ColumnHeaderType,
   DataTableMode,
+  ReorderColumnEvent,
 } from '../widgets/data_table/types';
 
 export {HistogramMode, TimeSelection};
@@ -94,15 +94,14 @@ export interface URLDeserializedState {
   };
 }
 
-export interface HeaderEditInfo {
+export interface HeaderEditInfo extends ReorderColumnEvent {
   dataTableMode: DataTableMode;
-  headers: ColumnHeader[];
 }
 
 export interface HeaderToggleInfo {
   header: ColumnHeader;
   cardId?: CardId;
-  dataTableMode?: DataTableMode;
+  dataTableMode?: DataTableMode | undefined;
 }
 
 export const SCALARS_SMOOTHING_MIN = 0;

tensorboard/webapp/metrics/store/metrics_reducers.ts

@@ -46,11 +46,7 @@ import {
   URLDeserializedState,
 } from '../types';
 import {groupCardIdWithMetdata} from '../utils';
-import {
-  ColumnHeader,
-  ColumnHeaderType,
-  DataTableMode,
-} from '../../widgets/data_table/types';
+import {ColumnHeaderType, DataTableMode} from '../../widgets/data_table/types';
 import {
   buildOrReturnStateWithPinnedCopy,
   buildOrReturnStateWithUnresolvedImportedPins,
@@ -82,6 +78,7 @@ import {
   TimeSeriesData,
   TimeSeriesLoadable,
 } from './metrics_types';
+import {DataTableUtils} from '../../widgets/data_table/utils';
 
 function buildCardMetadataList(tagMetadata: TagMetadata): CardMetadata[] {
   const results: CardMetadata[] = [];
@@ -1434,34 +1431,30 @@ const reducer = createReducer(
       tableEditorSelectedTab: tab,
     };
   }),
-  on(actions.dataTableColumnEdited, (state, {dataTableMode, headers}) => {
-    const enabledNewHeaders: ColumnHeader[] = [];
-    const disabledNewHeaders: ColumnHeader[] = [];
-
-    // All enabled headers appear above all disabled headers.
-    headers.forEach((header) => {
-      if (header.enabled) {
-        enabledNewHeaders.push(header);
-      } else {
-        disabledNewHeaders.push(header);
+  on(
+    actions.dataTableColumnOrderChanged,
+    (state, {source, destination, side, dataTableMode}) => {
+      let headers =
+        dataTableMode === DataTableMode.RANGE
+          ? [...state.rangeSelectionHeaders]
+          : [...state.singleSelectionHeaders];
+      headers = DataTableUtils.moveColumn(headers, source, destination, side);
+
+      if (dataTableMode === DataTableMode.RANGE) {
+        return {
+          ...state,
+          rangeSelectionHeaders: headers,
+        };
       }
-    });
-
-    if (dataTableMode === DataTableMode.RANGE) {
       return {
         ...state,
-        rangeSelectionHeaders: enabledNewHeaders.concat(disabledNewHeaders),
+        singleSelectionHeaders: headers,
       };
     }
-
-    return {
-      ...state,
-      singleSelectionHeaders: enabledNewHeaders.concat(disabledNewHeaders),
-    };
-  }),
+  ),
   on(
     actions.dataTableColumnToggled,
-    (state, {dataTableMode, header, cardId}) => {
+    (state, {dataTableMode, header: toggledHeader, cardId}) => {
       const {cardStateMap, rangeSelectionEnabled, linkedTimeEnabled} = state;
       const rangeEnabled = cardId
         ? cardRangeSelectionEnabled(
@@ -1471,40 +1464,24 @@ const reducer = createReducer(
             cardId
           )
         : dataTableMode === DataTableMode.RANGE;
-
       const targetedHeaders = rangeEnabled
         ? state.rangeSelectionHeaders
         : state.singleSelectionHeaders;
 
-      const currentToggledHeaderIndex = targetedHeaders.findIndex(
-        (element) => element.name === header.name
-      );
-
-      // If the header is being enabled it goes at the bottom of the currently
-      // enabled headers. If it is being disabled it goes to the top of the
-      // currently disabled headers.
-      let newToggledHeaderIndex = getEnabledCount(targetedHeaders);
-      if (targetedHeaders[currentToggledHeaderIndex].enabled) {
-        newToggledHeaderIndex--;
-      }
-      const newHeaders = moveHeader(
-        currentToggledHeaderIndex,
-        newToggledHeaderIndex,
-        targetedHeaders
-      );
-
-      newHeaders[newToggledHeaderIndex] = {
-        ...newHeaders[newToggledHeaderIndex],
-        enabled: !newHeaders[newToggledHeaderIndex].enabled,
-      };
+      const newHeaders = targetedHeaders.map((header) => {
+        const newHeader = {...header};
+        if (header.name === toggledHeader.name) {
+          newHeader.enabled = !newHeader.enabled;
+        }
+        return newHeader;
+      });
 
       if (rangeEnabled) {
         return {
           ...state,
           rangeSelectionHeaders: newHeaders,
         };
       }
-
       return {
         ...state,
         singleSelectionHeaders: newHeaders,
@@ -1583,30 +1560,3 @@ function buildTagToRuns(runTagInfo: {[run: string]: string[]}) {
   }
   return tagToRuns;
 }
-
-/**
- * Returns a copy of the headers array with item at sourceIndex moved to
- * destinationIndex.
- */
-function moveHeader(
-  sourceIndex: number,
-  destinationIndex: number,
-  headers: ColumnHeader[]
-) {
-  const newHeaders = [...headers];
-  // Delete from original location
-  newHeaders.splice(sourceIndex, 1);
-  // Insert at destinationIndex.
-  newHeaders.splice(destinationIndex, 0, headers[sourceIndex]);
-  return newHeaders;
-}
-
-function getEnabledCount(headers: ColumnHeader[]) {
-  let count = 0;
-  headers.forEach((header) => {
-    if (header.enabled) {
-      count++;
-    }
-  });
-  return count;
-}