Merge PR #1057: shift-click to select a range of rows

ui/src/app/app.html

@@ -757,7 +757,7 @@
       <thead>
         <tr>
           <th scope="col" style="width: 1rem;">
-            <app-select-all-checkbox #doneMasterCheckboxRef [id]="'done'" [list]="downloads.done" (changed)="doneSelectionChanged($event)" />
+            <app-select-all-checkbox #doneMasterCheckboxRef [id]="'done'" [list]="downloads.done" [orderedIds]="cachedSortedDoneIds" (changed)="doneSelectionChanged($event)" />
           </th>
           <th scope="col">Video</th>
           <th scope="col">Type</th>

ui/src/app/app.ts

@@ -137,6 +137,9 @@ export class App implements AfterViewInit, OnInit, OnDestroy {
   sortAscending = false;
   expandedErrors: Set<string> = new Set<string>();
   cachedSortedDone: [string, Download][] = [];
+  // The done ids in rendered order, so a shift-click range follows the sort
+  // the user is looking at rather than the map's insertion order.
+  cachedSortedDoneIds: string[] = [];
   lastCopiedErrorId: string | null = null;
   private previousDownloadType = 'video';
   private addRequestSub?: Subscription;
@@ -1532,6 +1535,7 @@ export class App implements AfterViewInit, OnInit, OnDestroy {
       result.reverse();
     }
     this.cachedSortedDone = result;
+    this.cachedSortedDoneIds = result.map(([key]) => key);
   }
 
   toggleErrorDetail(id: string) {

ui/src/app/components/master-checkbox.component.spec.ts

@@ -2,6 +2,38 @@ import { TestBed } from '@angular/core/testing';
 import { SelectAllCheckboxComponent } from './master-checkbox.component';
 import { Checkable } from '../interfaces';
 
+function makeList(ids: string[]): Map<string, Checkable> {
+  const list = new Map<string, Checkable>();
+  for (const id of ids) {
+    list.set(id, { checked: false });
+  }
+  return list;
+}
+
+function makeMaster(list: Map<string, Checkable>, orderedIds: string[] | null = null) {
+  const fixture = TestBed.createComponent(SelectAllCheckboxComponent);
+  fixture.componentRef.setInput('id', 'queue');
+  fixture.componentRef.setInput('list', list);
+  if (orderedIds) {
+    fixture.componentRef.setInput('orderedIds', orderedIds);
+  }
+  fixture.detectChanges();
+  return fixture;
+}
+
+// Simulates what the item checkbox does: ngModel writes the new state, then
+// the change handler reports the click to the master.
+function clickItem(
+  master: SelectAllCheckboxComponent,
+  list: Map<string, Checkable>,
+  id: string,
+  shift = false,
+) {
+  const item = list.get(id)!;
+  item.checked = !item.checked;
+  master.selectionChanged(id, shift);
+}
+
 describe('SelectAllCheckboxComponent', () => {
   beforeEach(async () => {
     await TestBed.configureTestingModule({
@@ -20,4 +52,87 @@ describe('SelectAllCheckboxComponent', () => {
     fixture.componentInstance.clicked();
     expect(list.get('u1')?.checked).toBe(true);
   });
+
+  it('shift-click checks every item between the two clicks', () => {
+    const list = makeList(['u1', 'u2', 'u3', 'u4', 'u5']);
+    const master = makeMaster(list).componentInstance;
+
+    clickItem(master, list, 'u2');
+    clickItem(master, list, 'u4', true);
+
+    expect([...list.values()].map((i) => i.checked)).toEqual([false, true, true, true, false]);
+  });
+
+  it('extends upwards as well as downwards', () => {
+    const list = makeList(['u1', 'u2', 'u3', 'u4']);
+    const master = makeMaster(list).componentInstance;
+
+    clickItem(master, list, 'u4');
+    clickItem(master, list, 'u2', true);
+
+    expect([...list.values()].map((i) => i.checked)).toEqual([false, true, true, true]);
+  });
+
+  it('shift-clicking a checked box clears the range', () => {
+    const list = makeList(['u1', 'u2', 'u3']);
+    list.forEach((item) => (item.checked = true));
+    const master = makeMaster(list).componentInstance;
+
+    clickItem(master, list, 'u1');
+    clickItem(master, list, 'u3', true);
+
+    expect([...list.values()].map((i) => i.checked)).toEqual([false, false, false]);
+  });
+
+  it('follows the rendered order, not the map order', () => {
+    // The done list renders newest-first, so its rendered order is not the
+    // order the entries sit in the map. u2 lies inside the range on screen
+    // and outside it in the map, which is what separates the two.
+    const list = makeList(['u1', 'u2', 'u3', 'u4']);
+    const master = makeMaster(list, ['u4', 'u2', 'u3', 'u1']).componentInstance;
+
+    clickItem(master, list, 'u4');
+    clickItem(master, list, 'u3', true);
+
+    // u1 (rendered last) stays clear; u2 is swept up with the range.
+    expect([...list.values()].map((i) => i.checked)).toEqual([false, true, true, true]);
+  });
+
+  it('a plain click after a range starts a new anchor', () => {
+    const list = makeList(['u1', 'u2', 'u3', 'u4']);
+    const master = makeMaster(list).componentInstance;
+
+    clickItem(master, list, 'u1');
+    clickItem(master, list, 'u2', true);
+    clickItem(master, list, 'u4');
+
+    expect([...list.values()].map((i) => i.checked)).toEqual([true, true, false, true]);
+  });
+
+  it('select-all clears the anchor so the next shift-click is a plain toggle', () => {
+    const list = makeList(['u1', 'u2', 'u3']);
+    const fixture = makeMaster(list);
+    const master = fixture.componentInstance;
+
+    clickItem(master, list, 'u1');
+    master.selected = true;
+    master.clicked();
+    master.selected = false;
+    master.clicked();
+    clickItem(master, list, 'u3', true);
+
+    expect([...list.values()].map((i) => i.checked)).toEqual([false, false, true]);
+  });
+
+  it('ignores a range whose anchor row is gone', () => {
+    const list = makeList(['u1', 'u2', 'u3']);
+    const master = makeMaster(list).componentInstance;
+
+    clickItem(master, list, 'u1');
+    // The anchor finishes downloading and leaves the queue.
+    list.delete('u1');
+    clickItem(master, list, 'u3', true);
+
+    expect([...list.values()].map((i) => i.checked)).toEqual([false, true]);
+  });
 });

ui/src/app/components/master-checkbox.component.ts

@@ -20,17 +20,33 @@ import { FormsModule } from "@angular/forms";
 export class SelectAllCheckboxComponent {
   readonly id = input.required<string>();
   readonly list = input.required<Map<string, Checkable>>();
+  // The ids in the order the rows are rendered. The done list is sorted for
+  // display, so its order is not the map's insertion order, and a range
+  // selection has to follow what the user sees. Left unset, the map order is
+  // the rendered order.
+  readonly orderedIds = input<string[] | null>(null);
   readonly changed = output<number>();
 
   readonly masterCheckbox = viewChild.required<ElementRef>('masterCheckbox');
   selected!: boolean;
 
+  // The item a range extends from: the last one toggled on its own.
+  private anchorId: string | null = null;
+
   clicked() {
     this.list().forEach(item => item.checked = this.selected);
+    // Select-all is not a position, so there is nothing to extend from next.
+    this.anchorId = null;
     this.selectionChanged();
   }
 
-  selectionChanged() {
+  selectionChanged(id?: string, extend = false) {
+    if (id !== undefined) {
+      if (extend && this.anchorId !== null && this.anchorId !== id) {
+        this.applyRange(this.anchorId, id);
+      }
+      this.anchorId = id;
+    }
     const masterCheckbox = this.masterCheckbox();
     if (!masterCheckbox)
       return;
@@ -40,4 +56,27 @@ export class SelectAllCheckboxComponent {
     masterCheckbox.nativeElement.indeterminate = checked > 0 && checked < this.list().size;
     this.changed.emit(checked);
   }
+
+  // Everything between the anchor and the just-clicked row takes the state the
+  // click produced, so shift-clicking a checked box clears the range and
+  // shift-clicking an unchecked one fills it.
+  private applyRange(fromId: string, toId: string) {
+    const ids = this.orderedIds() ?? Array.from(this.list().keys());
+    const from = ids.indexOf(fromId);
+    const to = ids.indexOf(toId);
+    // A row can disappear between two clicks (a download finishing moves it
+    // from the queue to the done list); without both ends there is no range.
+    if (from < 0 || to < 0) {
+      return;
+    }
+    const target = this.list().get(toId)?.checked ?? false;
+    const start = Math.min(from, to);
+    const end = Math.max(from, to);
+    for (let i = start; i <= end; i++) {
+      const item = this.list().get(ids[i]);
+      if (item) {
+        item.checked = target;
+      }
+    }
+  }
 }

ui/src/app/components/slave-checkbox.component.spec.ts

@@ -22,4 +22,33 @@ describe('ItemCheckboxComponent', () => {
     itemFixture.detectChanges();
     expect(itemFixture.componentInstance).toBeTruthy();
   });
+
+  it('reports the shift modifier from the click to the master', () => {
+    const masterFixture = TestBed.createComponent(SelectAllCheckboxComponent);
+    masterFixture.componentRef.setInput('id', 'q');
+    masterFixture.componentRef.setInput('list', new Map());
+    masterFixture.detectChanges();
+    const master = masterFixture.componentInstance;
+    const reported: [string | undefined, boolean | undefined][] = [];
+    master.selectionChanged = (id?: string, extend?: boolean) => {
+      reported.push([id, extend]);
+    };
+
+    const itemFixture = TestBed.createComponent(ItemCheckboxComponent);
+    itemFixture.componentRef.setInput('id', 'row1');
+    itemFixture.componentRef.setInput('master', master);
+    itemFixture.componentRef.setInput('checkable', { checked: false });
+    itemFixture.detectChanges();
+    const item = itemFixture.componentInstance;
+
+    item.clicked(new MouseEvent('click', { shiftKey: true }));
+    item.changed();
+    // The modifier must not stick to the next toggle.
+    item.changed();
+
+    expect(reported).toEqual([
+      ['row1', true],
+      ['row1', false],
+    ]);
+  });
 });

ui/src/app/components/slave-checkbox.component.ts

@@ -7,14 +7,14 @@ import { FormsModule } from '@angular/forms';
     selector: 'app-item-checkbox',
     template: `
   <div class="form-check">
-    <input type="checkbox" class="form-check-input" id="{{master().id()}}-{{id()}}-select" [(ngModel)]="checkable().checked" (change)="master().selectionChanged()" [attr.aria-label]="'Select item ' + id()">
+    <input type="checkbox" class="form-check-input" id="{{master().id()}}-{{id()}}-select" [(ngModel)]="checkable().checked" (click)="clicked($event)" (change)="changed()" [attr.aria-label]="'Select item ' + id()">
     <label class="form-check-label visually-hidden" for="{{master().id()}}-{{id()}}-select">Select item</label>
   </div>
 `,
     // Shared Checkable objects are mutated in place; Eager preserves pre-v22 behavior.
     // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection
     changeDetection: ChangeDetectionStrategy.Eager,
-    imports: [  
+    imports: [
   FormsModule
 ]
 })
@@ -22,4 +22,19 @@ export class ItemCheckboxComponent {
   readonly id = input.required<string>();
   readonly master = input.required<SelectAllCheckboxComponent>();
   readonly checkable = input.required<Checkable>();
+
+  // click fires before change, so the modifier is recorded here and read once
+  // ngModel has written the new state into the checkable. Keyboard activation
+  // fires change without a click, which is a plain toggle.
+  private extend = false;
+
+  clicked(event: MouseEvent) {
+    this.extend = event.shiftKey;
+  }
+
+  changed() {
+    const extend = this.extend;
+    this.extend = false;
+    this.master().selectionChanged(this.id(), extend);
+  }
 }