Fix selection of open tabs

The `selectedCount` computed property wasn't firing, and therefore the
rest of the UI didn't notice that tabs were being selected.  This
happened because the targetWindow was set before the window was actually
loaded in the model, but fetching the window from the model was not,
itself, reactive.  We hack around this by making targetWindow always
refer to the window itself, and assigning window objects to things
targetWindow depends on when those windows are actually populated.

src/model/index.test.ts

@@ -1576,7 +1576,9 @@ describe("model", () => {
 
   describe("restores tabs", () => {
     beforeEach(() => {
-      expect(model.tabs.initialWindow.value).to.equal(windows.real.id);
+      expect(model.tabs.initialWindow.value).to.equal(
+        model.tabs.window(windows.real.id),
+      );
       expect(model.tabs.activeTab()!.url).to.equal(B);
     });
 

src/model/index.ts

@@ -427,21 +427,17 @@ export class Model {
     items: StashLeaf[],
     options: {background?: boolean},
   ): Promise<Tabs.Tab[]> {
-    const toWindowId = this.tabs.targetWindow.value;
-    if (toWindowId === undefined) {
+    const toWindow = this.tabs.targetWindow.value;
+    if (toWindow === undefined) {
       throw new Error(`No target window; not sure where to restore tabs`);
     }
-    const cur_win = expect(
-      this.tabs.window(toWindowId),
-      () => `Target window ${toWindowId} is unknown to the model`,
-    );
 
     // As a special case, if we are restoring just a single tab, first check
     // if we already have the tab open and just switch to it.  (No need to
     // disturb the ordering of tabs in the browser window.)
     if (!options.background && items.length === 1 && items[0].url) {
       const t = Array.from(this.tabs.tabsWithURL(items[0].url)).find(
-        t => !t.hidden && t.position?.parent.id === toWindowId,
+        t => !t.hidden && t.position?.parent === toWindow,
       );
       if (t) {
         await browser.tabs.update(t.id, {active: true});
@@ -451,15 +447,15 @@ export class Model {
 
     // We want to know what tabs are currently open in the window, so we can
     // avoid opening duplicates.
-    const win_tabs = cur_win.children;
+    const win_tabs = toWindow.children;
 
     // We want to know which tab the user is currently looking at so we can
     // close it if it's just the new-tab page.
     const active_tab = win_tabs.filter(t => t.active)[0];
 
     const tabs = await this.putItemsInWindow({
       items: this.copying(items),
-      toWindowId,
+      toWindowId: toWindow.id,
     });
 
     if (!options.background) {
@@ -680,7 +676,7 @@ export class Model {
     toIndex?: number;
     task?: TaskMonitor;
   }): Promise<Tabs.Tab[]> {
-    const to_win_id = options.toWindowId ?? this.tabs.targetWindow.value;
+    const to_win_id = options.toWindowId ?? this.tabs.targetWindow.value?.id;
     if (to_win_id === undefined) {
       throw new Error(`No target window available: ${to_win_id}`);
     }

src/model/tabs.ts

@@ -1,4 +1,4 @@
-import {reactive, ref, watch, type Ref} from "vue";
+import {computed, reactive, ref, watch, type Ref} from "vue";
 import type {Tabs, Windows} from "webextension-polyfill";
 import browser from "webextension-polyfill";
 
@@ -67,19 +67,25 @@ export class Model {
   private readonly tabs_by_url = new Map<OpenableURL, Set<Tab>>();
 
   /** The initial window that this model was opened with (if it still exists). */
-  readonly initialWindow: Ref<WindowID | undefined> = ref();
+  readonly initialWindow: Ref<Window | WindowID | undefined> = ref();
 
   /** The window that currently has the focus (if any). */
-  readonly focusedWindow: Ref<WindowID | undefined> = ref();
+  readonly focusedWindow: Ref<Window | WindowID | undefined> = ref();
 
   /** The "target" window that this model should be for.  Controls for things
    * like which window is shown in the UI, tab selection, etc.  This isn't
    * precisely the same as `focusedWindow`, because it accounts for the fact
    * that the user could tear off the Tab Stash tab into a new window, and yet
    * still want to manage the window that the tab was originally opened in. */
-  readonly targetWindow = computedLazyEq(
-    () => this.initialWindow.value ?? this.focusedWindow.value,
-  );
+  readonly targetWindow = computed(() => {
+    if (typeof this.initialWindow.value === "object") {
+      return this.initialWindow.value;
+    }
+    if (typeof this.focusedWindow.value === "object") {
+      return this.focusedWindow.value;
+    }
+    return undefined;
+  });
 
   /** The number of selected tabs. */
   readonly selectedCount = computedLazyEq(() => {
@@ -203,7 +209,7 @@ export class Model {
   /** Returns the active tab in the specified window (or in
    * `this.current_window`, if no window is specified). */
   activeTab(windowId?: WindowID): Tab | undefined {
-    if (windowId === undefined) windowId = this.targetWindow.value;
+    if (windowId === undefined) windowId = this.targetWindow.value?.id;
     if (windowId === undefined) return undefined;
 
     const window = this.window(windowId);
@@ -405,25 +411,42 @@ export class Model {
     if (win.tabs !== undefined) {
       for (const t of win.tabs) this.whenTabCreated(t);
     }
+
+    // HACK: If we know the window ID, but not the window object, for the
+    // initial window, we must update here, otherwise reactive effects that
+    // depend on the contents of the initialWindow will never happen.  Same goes
+    // for focusedWindow.
+    if (window.id === this.initialWindow.value) {
+      this.initialWindow.value = window;
+    }
+    if (window.id === this.focusedWindow.value) {
+      this.focusedWindow.value = window;
+    }
+
     return window;
   }
 
   whenWindowFocusChanged(winId: number) {
-    this.focusedWindow.value =
-      winId !== browser.windows.WINDOW_ID_NONE
-        ? (winId as WindowID)
-        : undefined;
+    if (winId === browser.windows.WINDOW_ID_NONE) {
+      this.focusedWindow.value = undefined;
+      return;
+    }
+
+    const win = this.window(winId);
+    this.focusedWindow.value = win ?? (winId as WindowID);
   }
 
   whenWindowRemoved(winId: number) {
     const win = this.windows.get(winId as WindowID);
     trace("event windowRemoved", winId, win);
     if (!win) return;
 
-    if (this.initialWindow.value === winId)
+    if (this.initialWindow.value === winId) {
       this.initialWindow.value = undefined;
-    if (this.focusedWindow.value === winId)
+    }
+    if (this.focusedWindow.value === winId) {
       this.focusedWindow.value = undefined;
+    }
 
     // We clone the array to avoid disturbances while iterating
     for (const t of Array.from(win.children)) this.whenTabRemoved(t.id);
@@ -667,26 +690,20 @@ export class Model {
     // We only allow tabs in the current window to be selected
     // istanbul ignore if -- shouldn't occur in tests
     if (this.targetWindow.value === undefined) return;
-
-    const win = this.window(this.targetWindow.value);
-    if (!win) return;
-
-    for (const t of win.children) if (t.$selected) yield t;
+    for (const t of this.targetWindow.value.children) if (t.$selected) yield t;
   }
 
   itemsInRange(start: Tab, end: Tab): Tab[] | null {
     // istanbul ignore if -- shouldn't occur in tests
     if (this.targetWindow.value === undefined) return null;
 
+    const win = this.targetWindow.value;
     let startPos = start.position;
     let endPos = end.position;
     if (!startPos || !endPos) return null;
 
-    if (startPos.parent.id !== this.targetWindow.value) return null;
-    if (endPos.parent.id !== this.targetWindow.value) return null;
-
-    const win = this.window(this.targetWindow.value);
-    if (!win) return null;
+    if (startPos.parent !== this.targetWindow.value) return null;
+    if (endPos.parent !== this.targetWindow.value) return null;
 
     if (endPos.index < startPos.index) {
       const tmp = endPos;

src/stash-list/folder.vue

@@ -244,7 +244,7 @@ import {
   type Node,
 } from "../model/bookmarks";
 import type {FilteredItem, FilteredParent} from "../model/filtered-tree";
-import type {Tab} from "../model/tabs";
+import type {Tab, Window} from "../model/tabs";
 import {pathTo} from "../model/tree";
 
 import AsyncTextInput from "../components/async-text-input.vue";
@@ -300,7 +300,7 @@ export default defineComponent({
       return this.model().bookmark_metadata.get(this.folder.unfiltered.id);
     },
 
-    targetWindow(): number | undefined {
+    targetWindow(): Window | undefined {
       return this.model().tabs.targetWindow.value;
     },
 
@@ -359,10 +359,8 @@ export default defineComponent({
       const model = this.model();
       const target_win = model.tabs.targetWindow.value;
       if (!target_win) return [];
-      const win = model.tabs.window(target_win);
-      if (!win) return [];
 
-      return win.children
+      return target_win.children
         .filter(
           t =>
             !t.pinned &&
@@ -500,13 +498,13 @@ export default defineComponent({
 
     stash(ev: MouseEvent | KeyboardEvent) {
       const model = this.model();
-      const win_id = model.tabs.targetWindow.value;
-      if (!win_id) return;
+      const win = model.tabs.targetWindow.value;
+      if (!win) return;
 
       model.attempt(
         async () =>
           await model.putItemsInFolder({
-            items: model.copyIf(ev.altKey, model.stashableTabsInWindow(win_id)),
+            items: model.copyIf(ev.altKey, model.stashableTabsInWindow(win.id)),
             toFolderId: this.folder.unfiltered.id,
           }),
       );
@@ -659,7 +657,7 @@ export default defineComponent({
       this.attempt(async () => {
         const model = this.model();
         if (model.tabs.targetWindow.value === undefined) return;
-        await model.stashAllTabsInWindow(model.tabs.targetWindow.value, {
+        await model.stashAllTabsInWindow(model.tabs.targetWindow.value.id, {
           copy: ev.altKey,
           parent: this.folder.unfiltered.id,
           position: "bottom",

src/stash-list/index.vue

@@ -245,14 +245,12 @@ export default defineComponent({
     targetWindow() {
       const m = this.model().tabs;
       if (!m.targetWindow.value) return undefined;
-      return this.filteredTabs.wrappedParent(m.window(m.targetWindow.value)!);
+      return this.filteredTabs.wrappedParent(m.targetWindow.value);
     },
     tabs(): readonly Tab[] {
       const m = this.model().tabs;
       if (m.targetWindow.value === undefined) return [];
-      const win = m.window(m.targetWindow.value);
-      if (!win) return [];
-      return win.children;
+      return m.targetWindow.value.children;
     },
     stashRoot(): FilteredParent<Folder, Node> | undefined {
       const root = this.model().bookmarks.stash_root.value;

src/stash-list/tab.vue

@@ -87,7 +87,7 @@ export default defineComponent({
   computed: {
     altKey: altKeyName,
     bgKey: bgKeyName,
-    targetWindow(): number | undefined {
+    targetWindow(): Window | undefined {
       return this.model().tabs.targetWindow.value;
     },
     favIcon(): string {
@@ -111,7 +111,7 @@ export default defineComponent({
     isActive(): boolean {
       return (
         this.tab.unfiltered.active &&
-        this.tab.unfiltered.position?.parent.id === this.targetWindow
+        this.tab.unfiltered.position?.parent === this.targetWindow
       );
     },
     stashedIn(): string[] {

src/util/index.ts

@@ -104,12 +104,15 @@ export function cmpVersions(a: string, b: string): number {
  * This is generally more efficient than using computed() for primitive
  * values--using it with objects/arrays is not recommended; it is likely to
  * perform strictly worse than computed(). */
-export function computedLazyEq<T>(f: () => T): Vue.Ref<T> {
+export function computedLazyEq<T>(
+  f: () => T,
+  options?: Vue.WatchOptionsBase,
+): Vue.Ref<T> {
   const ref: Vue.Ref<T> = Vue.ref(undefined!);
   Vue.watchEffect(() => {
     const newValue = f();
     if (ref.value !== newValue) ref.value = newValue;
-  });
+  }, options);
   return ref;
 }