Allow card step selection to be slightly independent of global step s…

…election in redux (tensorflow#6230)

## Motivation for features / changes
tensorflow#6172 began the effort of moving the step selector to redux, however,
cards are now able to operate with some amount of independence from the
global state. To resolve this while also keeping things global, I have
added a handful of additional settings which can be used to determine
the correct time and range selection.

## Technical description of changes
Three state enums allow for a cleaner description of the state than
would be possible with booleans (I did try both). This also allows
clicking the checkboxes in the settings panel to ensure the correct
overrides are set.

### `stepSelection`
This detaches the existence of a cards step selection in redux state
from the need to display that step selection. This also allows step
selection to be disabled for individual cards while remaining globally
enabled.
* When a cards step selection is modified, its `stepSelection` setting
is set to `ENABLED`
* When all steps of a card are removed, the cards `stepSelection`
setting is set to `DISABLED`

### `rangeSelection`
Similarly to `stepSelection` this enables a card to have an end value
without needing to display it.
* When a cards time selection is modified, if an end step is being set,
`rangeSelection` is set to `ENABLED`, otherwise, `DISABLED`.

## Screenshots of UI changes
No changes

## Detailed steps to verify changes work correctly (as executed by you)
* I wrote a lot of tests
* I also forked this PR and made changes to the scalar card container to
allow me to play around with it.
This branch on my fork has the necessary changes
https://github.com/rileyajones/tensorboard/tree/step-selector-redux-4

## Alternate designs / implementations considered
While the card specific settings should take precedent over the global
settings, the global controls (the settings checkboxes) should override
those settings. This means that it is necessary to remove overrides when
they are clicked.

tensorboard/webapp/metrics/actions/index.ts

@@ -244,6 +244,13 @@ export const stepSelectorToggled = createAction(
     // Affordance for internal analytics purpose. When no affordance is specified or is
     // undefined we do not want to log an analytics event.
     affordance?: TimeSelectionToggleAffordance;
+    // This action can be triggered by two different events:
+    //   1) Clicking the checkbox in the settings panel
+    //   2) Removing the last fob from a scalar card
+    //
+    // Setting the cardId results in stepSelection being toggled for a specific card.
+    // Without the cardId being set this action only effects the global stepSeletion.
+    cardId?: CardId;
   }>()
 );
 export const rangeSelectionToggled = createAction(

tensorboard/webapp/metrics/store/metrics_reducers.ts

@@ -66,7 +66,9 @@ import {
   getTimeSeriesLoadable,
 } from './metrics_store_internal_utils';
 import {
+  CardFeatureOverride,
   CardMetadataMap,
+  CardStateMap,
   CardStepIndexMap,
   MetricsNamespacedState,
   MetricsNonNamespacedState,
@@ -1050,6 +1052,25 @@ const reducer = createReducer(
     let nextStepSelectorEnabled = state.stepSelectorEnabled;
     let linkedTimeSelection = state.linkedTimeSelection;
 
+    const nextCardStateMap = Object.entries(state.cardStateMap).reduce(
+      (cardStateMap, [cardId, cardState]) => {
+        // Range selection is tiered, it can be turned on/off globally and
+        // then overridden for an individual card.
+        //
+        // Since range selection was last toggled on/off, some cards were
+        // individually turned off/on respectively. Those cards differed
+        // from the "global" step selection enablement state. Now that
+        // range selection is being turned back on or off, all cards once
+        // again have the "global" state.
+        cardStateMap[cardId] = {
+          ...cardState,
+          rangeSelectionOverride: CardFeatureOverride.NONE,
+        };
+        return cardStateMap;
+      },
+      {} as CardStateMap
+    );
+
     if (nextRangeSelectionEnabled) {
       nextStepSelectorEnabled = nextRangeSelectionEnabled;
       if (!linkedTimeSelection) {
@@ -1077,6 +1098,7 @@ const reducer = createReducer(
       stepSelectorEnabled: nextStepSelectorEnabled,
       rangeSelectionEnabled: nextRangeSelectionEnabled,
       linkedTimeSelection,
+      cardStateMap: nextCardStateMap,
     };
   }),
   on(actions.timeSelectionChanged, (state, change) => {
@@ -1113,6 +1135,11 @@ const reducer = createReducer(
       nextCardStateMap[cardId] = {
         ...nextCardStateMap[cardId],
         timeSelection: nextTimeSelection,
+        stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
+        rangeSelectionOverride:
+          nextTimeSelection.end?.step === undefined
+            ? CardFeatureOverride.OVERRIDE_AS_DISABLED
+            : CardFeatureOverride.OVERRIDE_AS_ENABLED,
       };
     }
 
@@ -1136,14 +1163,42 @@ const reducer = createReducer(
       cardStateMap: nextCardStateMap,
     };
   }),
-  on(actions.stepSelectorToggled, (state, {affordance}) => {
+  on(actions.stepSelectorToggled, (state, {affordance, cardId}) => {
+    const nextCardStateMap = {...state.cardStateMap};
+    if (cardId) {
+      // cardId is only included when the event is generated from a scalar card
+      // The only time that the scalar card dispatches a step selection toggled
+      // event is when the last fob is being removed, therefore this should
+      // always result in stepSelection being disabled.
+      const {timeSelection, ...cardState} = nextCardStateMap[cardId] || {};
+      nextCardStateMap[cardId] = {
+        ...cardState,
+        stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED,
+      };
+    } else {
+      // Step selection is tiered, it can be turned on/off global and then
+      // overridden for an individual card.
+      //
+      // When no cardId is provided, the global status is being changed and
+      // thus all cards should be made to adhere to the new state.
+      Object.keys(nextCardStateMap).forEach((cardId) => {
+        nextCardStateMap[cardId] = {
+          ...nextCardStateMap[cardId],
+          stepSelectionOverride: CardFeatureOverride.NONE,
+        };
+      });
+    }
+
     if (
       !state.linkedTimeEnabled &&
       affordance !== TimeSelectionToggleAffordance.CHECK_BOX
     ) {
       // In plain step selection mode (without linked time), we do not allow
       // interactions with fobs to modify global step selection state.
-      return {...state};
+      return {
+        ...state,
+        cardStateMap: nextCardStateMap,
+      };
     }
 
     const nextStepSelectorEnabled = !state.stepSelectorEnabled;
@@ -1157,6 +1212,7 @@ const reducer = createReducer(
       linkedTimeEnabled: nextLinkedTimeEnabled,
       stepSelectorEnabled: nextStepSelectorEnabled,
       rangeSelectionEnabled: nextRangeSelectionEnabled,
+      cardStateMap: nextCardStateMap,
     };
   }),
   on(actions.timeSelectionCleared, (state) => {

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -56,6 +56,7 @@ import {
 import {reducers} from './metrics_reducers';
 import {getCardId, getPinnedCardId} from './metrics_store_internal_utils';
 import {
+  CardFeatureOverride,
   CardMetadataMap,
   MetricsState,
   RunToLoadState,
@@ -3068,7 +3069,7 @@ describe('metrics reducers', () => {
             cardId: 'card2',
             timeSelection: {
               start: {step: 1},
-              end: {step: 5},
+              end: null,
             },
           })
         );
@@ -3078,8 +3079,10 @@ describe('metrics reducers', () => {
           card2: {
             timeSelection: {
               start: {step: 1},
-              end: {step: 5},
+              end: null,
             },
+            stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
+            rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED,
           },
         });
       });
@@ -3121,6 +3124,38 @@ describe('metrics reducers', () => {
               start: {step: 1},
               end: {step: 5},
             },
+            stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
+            rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
+          },
+        });
+      });
+
+      it('enables card specific range selection if an end value is provided', () => {
+        const state1 = buildMetricsState({
+          cardStateMap: {
+            card1: {},
+          },
+        });
+        const state2 = reducers(
+          state1,
+          actions.timeSelectionChanged({
+            cardId: 'card2',
+            timeSelection: {
+              start: {step: 1},
+              end: {step: 5},
+            },
+          })
+        );
+
+        expect(state2.cardStateMap).toEqual({
+          card1: {},
+          card2: {
+            timeSelection: {
+              start: {step: 1},
+              end: {step: 5},
+            },
+            stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
+            rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
           },
         });
       });
@@ -3233,6 +3268,37 @@ describe('metrics reducers', () => {
           end: null,
         });
       });
+
+      it('sets all card specific overrides to default', () => {
+        const state1 = buildMetricsState({
+          linkedTimeSelection: {
+            start: {step: 100},
+            end: {step: 1000},
+          },
+          rangeSelectionEnabled: false,
+          cardStateMap: {
+            card1: {
+              rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
+            },
+            card2: {
+              rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED,
+            },
+            card3: {},
+          },
+        });
+        const state2 = reducers(state1, actions.rangeSelectionToggled({}));
+        expect(state2.cardStateMap).toEqual({
+          card1: {
+            rangeSelectionOverride: CardFeatureOverride.NONE,
+          },
+          card2: {
+            rangeSelectionOverride: CardFeatureOverride.NONE,
+          },
+          card3: {
+            rangeSelectionOverride: CardFeatureOverride.NONE,
+          },
+        });
+      });
     });
 
     describe('#cardMinMaxChanged', () => {
@@ -3549,6 +3615,43 @@ describe('metrics reducers', () => {
       expect(state2.linkedTimeEnabled).toBe(false);
       expect(state2.rangeSelectionEnabled).toBe(false);
     });
+
+    it('disables card specific step selection when cardId is provided', () => {
+      const prevState = buildMetricsState();
+      const nextState = reducers(
+        prevState,
+        actions.stepSelectorToggled({cardId: 'card1'})
+      );
+      expect(nextState.cardStateMap['card1'].stepSelectionOverride).toEqual(
+        CardFeatureOverride.OVERRIDE_AS_DISABLED
+      );
+    });
+
+    it('removes all card specific overrides when no card id is provided', () => {
+      const prevState = buildMetricsState({
+        cardStateMap: {
+          card1: {
+            stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
+          },
+          card2: {
+            stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED,
+          },
+          card3: {},
+        },
+      });
+      const nextState = reducers(prevState, actions.stepSelectorToggled({}));
+      expect(nextState.cardStateMap).toEqual({
+        card1: {
+          stepSelectionOverride: CardFeatureOverride.NONE,
+        },
+        card2: {
+          stepSelectionOverride: CardFeatureOverride.NONE,
+        },
+        card3: {
+          stepSelectionOverride: CardFeatureOverride.NONE,
+        },
+      });
+    });
   });
 
   describe('plugin filtering feature', () => {

tensorboard/webapp/metrics/store/metrics_selectors.ts

@@ -34,7 +34,10 @@ import {
   MinMaxStep,
 } from '../views/card_renderer/scalar_card_types';
 import * as storeUtils from './metrics_store_internal_utils';
-import {getMinMaxStepFromCardState} from './metrics_store_internal_utils';
+import {
+  getCardSelectionStateToBoolean,
+  getMinMaxStepFromCardState,
+} from './metrics_store_internal_utils';
 import {
   CardMetadataMap,
   CardStateMap,
@@ -391,6 +394,22 @@ export const getMetricsRangeSelectionEnabled = createSelector(
   }
 );
 
+export const getMetricsCardRangeSelectionEnabled = createSelector(
+  getMetricsRangeSelectionEnabled,
+  getCardStateMap,
+  (
+    globalRangeSelectionEnabled: boolean,
+    cardStateMap: CardStateMap,
+    cardId: CardId
+  ) => {
+    const cardState = cardStateMap[cardId];
+    return getCardSelectionStateToBoolean(
+      cardState?.rangeSelectionOverride,
+      globalRangeSelectionEnabled
+    );
+  }
+);
+
 export const getMetricsStepMinMax = createSelector(
   selectMetricsState,
   (state: MetricsState): {min: number; max: number} => {
@@ -487,7 +506,6 @@ export const getMetricsCardMinMax = createSelector(
   getCardStateMap,
   (cardStateMap: CardStateMap, cardId: CardId): MinMaxStep | undefined => {
     if (!cardStateMap[cardId]) return;
-
     return getMinMaxStepFromCardState(cardStateMap[cardId]);
   }
 );
@@ -497,29 +515,47 @@ export const getMetricsCardMinMax = createSelector(
  */
 export const getMetricsCardTimeSelection = createSelector(
   getCardStateMap,
+  getMetricsStepSelectorEnabled,
   getMetricsLinkedTimeEnabled,
   getMetricsLinkedTimeSelection,
   (
     cardStateMap: CardStateMap,
+    globalStepSelectionEnabled: boolean,
     linkedTimeEnabled: boolean,
     linkedTimeSelection: TimeSelection | null,
     cardId: CardId
   ): TimeSelection | undefined => {
+    // Handling Linked Time
     if (linkedTimeEnabled && linkedTimeSelection) {
       return linkedTimeSelection;
     }
 
-    if (cardStateMap[cardId]?.timeSelection) {
-      return cardStateMap[cardId]?.timeSelection;
+    const cardState = cardStateMap[cardId];
+    if (!cardState) {
+      return;
     }
-    const minMaxStep = getMinMaxStepFromCardState(cardStateMap[cardId]);
+    const minMaxStep = getMinMaxStepFromCardState(cardState);
     if (!minMaxStep) {
       return;
     }
 
+    // If the user has disabled step selection, nothing should be returned.
+    if (
+      !getCardSelectionStateToBoolean(
+        cardState.stepSelectionOverride,
+        globalStepSelectionEnabled
+      )
+    ) {
+      return;
+    }
+
+    const startStep = cardState.timeSelection?.start.step ?? minMaxStep.minStep;
+    const endStep = cardState.timeSelection?.end?.step ?? minMaxStep.maxStep;
+
+    // The default time selection
     return {
-      start: {step: minMaxStep.minStep},
-      end: {step: minMaxStep.maxStep},
+      start: {step: startStep},
+      end: {step: endStep},
     };
   }
 );