Changes reorder column events to use source and destination

tensorboard/webapp/BUILD

@@ -301,6 +301,7 @@ tf_ng_web_test_suite(
         "//tensorboard/webapp/widgets/content_wrapping_input:content_wrapping_input_tests",
         "//tensorboard/webapp/widgets/custom_modal:custom_modal_test",
         "//tensorboard/webapp/widgets/data_table:data_table_test",
+        "//tensorboard/webapp/widgets/data_table:utils_test",
         "//tensorboard/webapp/widgets/dropdown:dropdown_tests",
         "//tensorboard/webapp/widgets/experiment_alias:experiment_alias_test",
         "//tensorboard/webapp/widgets/filter_input:filter_input_test",

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/hparams/_redux/hparams_selectors.ts

@@ -47,8 +47,14 @@ export const getDashboardDefaultHparamFilters = createSelector(
 );
 
 export const getDashboardDisplayedHparamColumns = createSelector(
+  getDashboardHparamsAndMetricsSpecs,
   getHparamsState,
-  (state) => state.dashboardDisplayedHparamColumns
+  ({hparams}, state) => {
+    const hparamSet = new Set(hparams.map((hparam) => hparam.name));
+    return state.dashboardDisplayedHparamColumns.filter((column) =>
+      hparamSet.has(column.name)
+    );
+  }
 );
 
 export const getDashboardHparamFilterMap = createSelector(

tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts

@@ -15,6 +15,7 @@ limitations under the License.
 
 import {ColumnHeaderType} from '../../widgets/data_table/types';
 import {DomainType} from '../types';
+import {State} from './types';
 import * as selectors from './hparams_selectors';
 import {
   buildHparamSpec,
@@ -114,30 +115,63 @@ describe('hparams/_redux/hparams_selectors_test', () => {
   });
 
   describe('#getDashboardDisplayedHparamColumns', () => {
-    it('returns dashboard displayed hparam columns', () => {
-      const fakeColumns = [
-        {
-          type: ColumnHeaderType.HPARAM,
-          name: 'conv_layers',
-          displayName: 'Conv Layers',
-          enabled: true,
-        },
-        {
-          type: ColumnHeaderType.HPARAM,
-          name: 'conv_kernel_size',
-          displayName: 'Conv Kernel Size',
-          enabled: true,
-        },
-      ];
+    it('returns no columns if no hparam specs', () => {
       const state = buildStateFromHparamsState(
         buildHparamsState({
-          dashboardDisplayedHparamColumns: fakeColumns,
+          dashboardSpecs: {
+            hparams: [],
+          },
+          dashboardDisplayedHparamColumns: [
+            {
+              type: ColumnHeaderType.HPARAM,
+              name: 'conv_layers',
+              displayName: 'Conv Layers',
+              enabled: true,
+            },
+            {
+              type: ColumnHeaderType.HPARAM,
+              name: 'conv_kernel_size',
+              displayName: 'Conv Kernel Size',
+              enabled: true,
+            },
+          ],
         })
       );
 
-      expect(selectors.getDashboardDisplayedHparamColumns(state)).toEqual(
-        fakeColumns
+      expect(selectors.getDashboardDisplayedHparamColumns(state)).toEqual([]);
+    });
+
+    it('returns only hparam columns that have specs', () => {
+      const state = buildStateFromHparamsState(
+        buildHparamsState({
+          dashboardSpecs: {
+            hparams: [buildHparamSpec({name: 'conv_layers'})],
+          },
+          dashboardDisplayedHparamColumns: [
+            {
+              type: ColumnHeaderType.HPARAM,
+              name: 'conv_layers',
+              displayName: 'Conv Layers',
+              enabled: true,
+            },
+            {
+              type: ColumnHeaderType.HPARAM,
+              name: 'conv_kernel_size',
+              displayName: 'Conv Kernel Size',
+              enabled: true,
+            },
+          ],
+        })
       );
+
+      expect(selectors.getDashboardDisplayedHparamColumns(state)).toEqual([
+        {
+          type: ColumnHeaderType.HPARAM,
+          name: 'conv_layers',
+          displayName: 'Conv Layers',
+          enabled: true,
+        },
+      ]);
     });
   });
 });

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/BUILD

@@ -18,6 +18,7 @@ tf_ts_library(
         "//tensorboard/webapp/app_routing:types",
         "//tensorboard/webapp/app_routing/actions",
         "//tensorboard/webapp/core/actions",
+        "//tensorboard/webapp/hparams/_redux:hparams_selectors",
         "//tensorboard/webapp/metrics:types",
         "//tensorboard/webapp/metrics:utils",
         "//tensorboard/webapp/metrics/actions",
@@ -33,6 +34,7 @@ tf_ts_library(
         "//tensorboard/webapp/util:types",
         "//tensorboard/webapp/widgets/card_fob:types",
         "//tensorboard/webapp/widgets/data_table:types",
+        "//tensorboard/webapp/widgets/data_table:utils",
         "//tensorboard/webapp/widgets/line_chart_v2/lib:public_types",
         "@npm//@ngrx/store",
     ],
@@ -97,14 +99,18 @@ tf_ts_library(
         "//tensorboard/webapp/app_routing:types",
         "//tensorboard/webapp/app_routing/actions",
         "//tensorboard/webapp/core/actions",
+        "//tensorboard/webapp/hparams:testing",
+        "//tensorboard/webapp/hparams/_redux:types",
         "//tensorboard/webapp/metrics:test_lib",
         "//tensorboard/webapp/metrics:types",
         "//tensorboard/webapp/metrics/actions",
         "//tensorboard/webapp/metrics/data_source",
         "//tensorboard/webapp/persistent_settings",
         "//tensorboard/webapp/routes:testing",
+        "//tensorboard/webapp/testing:utils",
         "//tensorboard/webapp/types",
         "//tensorboard/webapp/util:dom",
+        "//tensorboard/webapp/util:types",
         "//tensorboard/webapp/widgets/card_fob:types",
         "//tensorboard/webapp/widgets/data_table:types",
         "@npm//@types/jasmine",