From d8aafd7a43d6a66b685c03d4d41888be57f06123 Mon Sep 17 00:00:00 2001 From: Brian Dubois Date: Mon, 1 Aug 2022 10:37:51 -0400 Subject: [PATCH] Support selecting single run using double click. (#5831) * Motivation for features / changes Users have asked for a way to select one and only one item in the Runs list in the Time Series dashboard. That is, select one run while deselecting all the others. This is supported in Scalars dashboad with a radio button but was not ported to Time Series. We decided to support this differently in Time Series -- with a double click. Double clicking on the checkbox for any run will select that run while deselecting the remaining runs. * Technical description of changes We listen for dblclick events on the mat-checkbox. Events and actions are triggered up to the reducer layer where just the one run is selected while the other runs are deselected. We recognize that this might be a bit difficult to discover and some might consider it an anti-pattern but we think it is the best option for our situation. Some users might even try this out, having learned the pattern from other products. --- .../webapp/runs/actions/runs_actions.ts | 9 ++ .../webapp/runs/store/runs_reducers.ts | 13 +++ .../webapp/runs/store/runs_reducers_test.ts | 100 ++++++++++++++++++ .../runs_table/runs_table_component.ng.html | 6 ++ .../views/runs_table/runs_table_component.ts | 1 + .../views/runs_table/runs_table_container.ts | 22 ++++ .../runs/views/runs_table/runs_table_test.ts | 27 +++++ 7 files changed, 178 insertions(+) diff --git a/tensorboard/webapp/runs/actions/runs_actions.ts b/tensorboard/webapp/runs/actions/runs_actions.ts index f3cd0dd9b7c..8a85a1a4914 100644 --- a/tensorboard/webapp/runs/actions/runs_actions.ts +++ b/tensorboard/webapp/runs/actions/runs_actions.ts @@ -53,6 +53,15 @@ export const runSelectionToggled = createAction( props<{runId: string}>() ); +/** + * An action to indicate a single run being selected while all other runs are to + * be deselected. + */ +export const singleRunSelected = createAction( + '[Runs] Single Run Selected', + props<{runId: string}>() +); + export const runPageSelectionToggled = createAction( '[Runs] Run Page Selection Toggled', props<{runIds: string[]}>() diff --git a/tensorboard/webapp/runs/store/runs_reducers.ts b/tensorboard/webapp/runs/store/runs_reducers.ts index 845d8646047..c4d5472a95a 100644 --- a/tensorboard/webapp/runs/store/runs_reducers.ts +++ b/tensorboard/webapp/runs/store/runs_reducers.ts @@ -380,6 +380,19 @@ const uiReducer: ActionReducer = createReducer( selectionState: nextSelectionState, }; }), + on(runsActions.singleRunSelected, (state, {runId}) => { + const nextSelectionState = new Map(); + + // Select the specified run and deselect the others. + for (const stateRunId of state.selectionState.keys()) { + nextSelectionState.set(stateRunId, runId === stateRunId); + } + + return { + ...state, + selectionState: nextSelectionState, + }; + }), on(runsActions.runPageSelectionToggled, (state, {runIds}) => { const nextSelectionState = new Map(state.selectionState); diff --git a/tensorboard/webapp/runs/store/runs_reducers_test.ts b/tensorboard/webapp/runs/store/runs_reducers_test.ts index 7ae5d1f6636..993dc74b63c 100644 --- a/tensorboard/webapp/runs/store/runs_reducers_test.ts +++ b/tensorboard/webapp/runs/store/runs_reducers_test.ts @@ -479,6 +479,106 @@ describe('runs_reducers', () => { }); }); + describe('singleRunSelected', () => { + it('selects run when it is only run', () => { + const state = buildRunsState( + {}, + { + selectionState: new Map([['run1', false]]), + } + ); + const nextState = runsReducers.reducers( + state, + actions.singleRunSelected({ + runId: 'run1', + }) + ); + expect(nextState.ui.selectionState).toEqual(new Map([['run1', true]])); + }); + + it('selects run and deselects others', () => { + const state = buildRunsState( + {}, + { + selectionState: new Map([ + ['run1', true], + ['run2', true], + ['run3', false], + ['run4', false], + ['run5', true], + ]), + } + ); + const nextState = runsReducers.reducers( + state, + actions.singleRunSelected({ + runId: 'run4', + }) + ); + expect(nextState.ui.selectionState).toEqual( + new Map([ + ['run1', false], + ['run2', false], + ['run3', false], + ['run4', true], + ['run5', false], + ]) + ); + }); + + it('keeps run selected if already selected', () => { + const state = buildRunsState( + {}, + { + selectionState: new Map([ + ['run1', true], + ['run2', true], + ['run3', true], + ]), + } + ); + const nextState = runsReducers.reducers( + state, + actions.singleRunSelected({ + runId: 'run2', + }) + ); + expect(nextState.ui.selectionState).toEqual( + new Map([ + ['run1', false], + ['run2', true], + ['run3', false], + ]) + ); + }); + + it('keeps run selected if only already selected', () => { + const state = buildRunsState( + {}, + { + selectionState: new Map([ + ['run1', false], + ['run2', true], + ['run3', false], + ]), + } + ); + const nextState = runsReducers.reducers( + state, + actions.singleRunSelected({ + runId: 'run2', + }) + ); + expect(nextState.ui.selectionState).toEqual( + new Map([ + ['run1', false], + ['run2', true], + ['run3', false], + ]) + ); + }); + }); + describe('runPageSelectionToggled', () => { it('toggles all items to on when they were all previously off', () => { const state = buildRunsState( diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_component.ng.html b/tensorboard/webapp/runs/views/runs_table/runs_table_component.ng.html index cf62f229183..936c18cf67d 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_component.ng.html +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_component.ng.html @@ -230,9 +230,15 @@ > + (); @Output() onSelectionToggle = new EventEmitter(); + @Output() onSelectionDblClick = new EventEmitter(); @Output() onPageSelectionToggle = new EventEmitter<{items: RunTableItem[]}>(); @Output() onPaginationChange = new EventEmitter<{ diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts index 95a772c4bc3..0b2103d65e7 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts @@ -70,6 +70,7 @@ import { runSelectorRegexFilterChanged, runSelectorSortChanged, runTableShown, + singleRunSelected, } from '../../actions'; import {MAX_NUM_RUNS_TO_ENABLE_BY_DEFAULT} from '../../store/runs_types'; import {SortKey, SortType} from '../../types'; @@ -205,6 +206,7 @@ function matchFilter( [sortOption]="sortOption$ | async" [usePagination]="usePagination" (onSelectionToggle)="onRunSelectionToggle($event)" + (onSelectionDblClick)="onRunSelectionDblClick($event)" (onPageSelectionToggle)="onPageSelectionToggle($event)" (onPaginationChange)="onPaginationChange($event)" (onRegexFilterChange)="onRegexFilterChange($event)" @@ -564,6 +566,26 @@ export class RunsTableContainer implements OnInit, OnDestroy { ); } + onRunSelectionDblClick(item: RunTableItem) { + // Note that a user's double click will trigger both 'change' and 'dblclick' + // events so onRunSelectionToggle() will also be called and we will fire + // two somewhat conflicting actions: runSelectionToggled and + // singleRunSelected. This is ok as long as singleRunSelected is fired last. + // + // We are therefore relying on the mat-checkbox 'change' event consistently + // being fired before the 'dblclick' event. Although we don't have any + // documentation that guarantees this order, we do have documentation that + // states that 'click' is guaranteed to occur before 'dblclick' + // (see https://www.quirksmode.org/dom/events/click.html). We presume, then, + // that we can rely on the 'change' event being fired before the 'dblclick' + // event. + this.store.dispatch( + singleRunSelected({ + runId: item.run.id, + }) + ); + } + // When `usePagination` is false, page selection affects the single page, // containing all items. onPageSelectionToggle(event: {items: RunTableItem[]}) { diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts index 3be341abd97..702581d6b5a 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_test.ts @@ -92,6 +92,7 @@ import { runSelectorRegexFilterChanged, runSelectorSortChanged, runTableShown, + singleRunSelected, } from '../../actions'; import {DomainType} from '../../data_source/runs_data_source_types'; import {MAX_NUM_RUNS_TO_ENABLE_BY_DEFAULT, Run} from '../../store/runs_types'; @@ -1828,6 +1829,32 @@ describe('runs_table', () => { ); }); + it('dispatches singleRunSelected on checkbox double click', async () => { + store.overrideSelector(getRuns, [ + buildRun({id: 'book1', name: "The Philosopher's Stone"}), + buildRun({id: 'book2', name: 'The Chamber Of Secrets'}), + buildRun({id: 'book3', name: 'The Prisoner of Azkaban'}), + ]); + + const fixture = createComponent( + ['rowling'], + [RunsTableColumn.CHECKBOX, RunsTableColumn.RUN_NAME], + true /* usePagination */ + ); + fixture.detectChanges(); + await fixture.whenStable(); + + const rows = fixture.nativeElement.querySelectorAll(Selector.ITEM_ROW); + const book2 = rows[1].querySelector('mat-checkbox'); + book2.dispatchEvent(new MouseEvent('dblclick', {relatedTarget: book2})); + + expect(dispatchSpy).toHaveBeenCalledWith( + singleRunSelected({ + runId: 'book2', + }) + ); + }); + it( 'dispatches runPageSelectionToggled with current page when click on ' + 'header',