Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support selecting single run using double click. #5831

Merged
merged 4 commits into from
Aug 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions tensorboard/webapp/runs/actions/runs_actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[]}>()
Expand Down
13 changes: 13 additions & 0 deletions tensorboard/webapp/runs/store/runs_reducers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,19 @@ const uiReducer: ActionReducer<RunsUiState, Action> = createReducer(
selectionState: nextSelectionState,
};
}),
on(runsActions.singleRunSelected, (state, {runId}) => {
const nextSelectionState = new Map<string, boolean>();

// 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);

Expand Down
100 changes: 100 additions & 0 deletions tensorboard/webapp/runs/store/runs_reducers_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,15 @@
>
<ng-container [ngSwitch]="columnId">
<span *ngSwitchCase="RunsTableColumn.CHECKBOX">
<!--
Single click toggles the run while double click selects only that
run and deselects the rest. The double click will emit both the
(change) and the (dblclick) events.
-->
<mat-checkbox
[checked]="item.selected"
(change)="onSelectionToggle.emit(item)"
(dblclick)="onSelectionDblClick.emit(item)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a note here that (dblclick) will also fire (change)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

></mat-checkbox>
</span>
<tb-experiment-alias
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ export class RunsTableComponent implements OnChanges {

@Output() onRegexFilterChange = new EventEmitter<string>();
@Output() onSelectionToggle = new EventEmitter<RunTableItem>();
@Output() onSelectionDblClick = new EventEmitter<RunTableItem>();
@Output() onPageSelectionToggle = new EventEmitter<{items: RunTableItem[]}>();
@Output()
onPaginationChange = new EventEmitter<{
Expand Down
22 changes: 22 additions & 0 deletions tensorboard/webapp/runs/views/runs_table/runs_table_container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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)"
Expand Down Expand Up @@ -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[]}) {
Expand Down
27 changes: 27 additions & 0 deletions tensorboard/webapp/runs/views/runs_table/runs_table_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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',
})
);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth to add a test that click and dblclick event are fired in such order and we get the single selection result?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a user or machine has to actually double click on the element in the browser window in order for both events to be fired. Here I am just doing a lower-level operation pretending that some sort of double click has happened.

I will explore writing a webtest for this, though, to see if I can simulate an actual double click (and both the change and dblclick events).


it(
'dispatches runPageSelectionToggled with current page when click on ' +
'header',
Expand Down