Skip to content

Commit

Permalink
model: Pass objects instead of IDs around
Browse files Browse the repository at this point in the history 
This should give us a small performance boost, because we can avoid a
lot of unnecessary ID lookups inside the models.

Closes #497.
  • Loading branch information
@josh-berry
josh-berry committed Jul 21, 2024
1 parent 1015910 commit cc50982
Show file tree
Hide file tree
Showing 12 changed files with 244 additions and 264 deletions.
56 changes: 26 additions & 30 deletions src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@ import browser from "webextension-polyfill";

import {copyIf} from "./model/index.js";
import type {ShowWhatOpt, StashWhatOpt} from "./model/options.js";
import type {Tab, TabID, WindowID} from "./model/tabs.js";
import type {Tab} from "./model/tabs.js";
import service_model from "./service-model.js";
import {asyncEvent, backingOff, nonReentrant, urlToOpen} from "./util/index.js";
import {
asyncEvent,
backingOff,
filterMap,
nonReentrant,
urlToOpen,
} from "./util/index.js";
import {logErrorsFrom} from "./util/oops.js";

logErrorsFrom(async () => {
Expand Down Expand Up @@ -38,12 +44,14 @@ logErrorsFrom(async () => {

const tabs = await browser.tabs.query({hidden: true});

for (const t of tabs) {
if (model.bookmarks.isURLStashed(t.url!)) {
// This applies the tag as a side effect
await model.tabs.hide([t.id! as TabID]);
}
}
const stashed_hidden_tabs = tabs.filter(t =>
model.bookmarks.isURLStashed(t.url!),
);

// This applies the tag as a side effect
await model.tabs.hide(
filterMap(stashed_hidden_tabs, t => model.tabs.tab(t.id!)),
);

await model.options.local.set({migrated_tab_markers_applied: true});
});
Expand Down Expand Up @@ -186,7 +194,7 @@ logErrorsFrom(async () => {
if (!tab) return;
await model.putItemsInFolder({
items: [tab],
toFolderId: (await model.bookmarks.createStashFolder()).id,
toFolder: await model.bookmarks.createStashFolder(),
});
},

Expand Down Expand Up @@ -239,16 +247,15 @@ logErrorsFrom(async () => {

switch (options.what) {
case "all":
await model.stashAllTabsInWindow(
options.tab.position.parent.id as WindowID,
{copy: !!options.copy},
);
await model.stashAllTabsInWindow(options.tab.position.parent, {
copy: !!options.copy,
});
break;

case "single":
await model.putItemsInFolder({
items: copyIf(!!options.copy, [options.tab]),
toFolderId: (await model.ensureRecentUnnamedFolder()).id,
toFolder: await model.ensureRecentUnnamedFolder(),
});
break;

Expand Down Expand Up @@ -410,10 +417,6 @@ logErrorsFrom(async () => {
// Garbage-collect hidden tabs by diffing the old and new sets of URLs
// in the tree.
const new_urls = model.bookmarks.urlsInStash();
let windows = await browser.windows.getAll({
windowTypes: ["normal"],
populate: true,
});

// Ugh, why am I open-coding a set-difference operation? This
// should be built-in!
Expand All @@ -422,18 +425,11 @@ logErrorsFrom(async () => {
if (!new_urls.has(url)) removed_urls.add(url);
}

let tids = [];
for (let w of windows) {
// #undef We only asked for 'normal' windows, which have tabs
for (let t of w.tabs!) {
if (!t.hidden) continue;
if (t.id === undefined) continue;
if (!removed_urls.has(urlToOpen(t.url!))) continue;
tids.push(t.id as TabID);
}
}

await model.tabs.remove(tids);
await model.tabs.remove(
model.tabs
.allTabs()
.filter(t => t.hidden && removed_urls.has(urlToOpen(t.url))),
);

managed_urls = new_urls;
}),
Expand Down
30 changes: 23 additions & 7 deletions src/model/bookmarks.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,11 @@ describe("model/bookmarks", () => {
});

it("reorders bookmarks (forward)", async () => {
const p = model.move(bms.alice.id, bms.outside.id, 4);
const p = model.move(
model.bookmark(bms.alice.id)!,
model.folder(bms.outside.id)!,
4,
);
await events.next(browser.bookmarks.onMoved);
await p;

Expand All @@ -221,7 +225,11 @@ describe("model/bookmarks", () => {
});

it("reorders bookmarks (backward)", async () => {
const p = model.move(bms.empty.id, bms.outside.id, 0);
const p = model.move(
model.node(bms.empty.id)!,
model.folder(bms.outside.id)!,
0,
);
await events.next(browser.bookmarks.onMoved);
await p;

Expand All @@ -231,7 +239,11 @@ describe("model/bookmarks", () => {
});

it("moves bookmarks between folders", async () => {
const p = model.move(bms.bob.id, bms.names.id, 2);
const p = model.move(
model.bookmark(bms.bob.id)!,
model.folder(bms.names.id)!,
2,
);
await events.next(browser.bookmarks.onMoved);
await p;

Expand Down Expand Up @@ -402,7 +414,7 @@ describe("model/bookmarks", () => {

describe("cleans up empty folders", () => {
it("when deleting stash bookmarks from an unnamed folder", async () => {
const p = model.remove(bms.undyne.id);
const p = model.remove(model.bookmark(bms.undyne.id)!);
await events.nextN(browser.bookmarks.onRemoved, 2);
await p;

Expand All @@ -413,7 +425,11 @@ describe("model/bookmarks", () => {
});

it("when moving stash bookmarks to another folder", async () => {
const p = model.move(bms.undyne.id, bms.names.id, 5);
const p = model.move(
model.bookmark(bms.undyne.id)!,
model.folder(bms.names.id)!,
5,
);
await events.next(browser.bookmarks.onMoved);
await events.next(browser.bookmarks.onRemoved);
await p;
Expand Down Expand Up @@ -455,7 +471,7 @@ describe("model/bookmarks", () => {
});

it("when deleting bookmarks outside the stash root", async () => {
const p = model.remove(new_child.id);
const p = model.remove(new_child);
await events.nextN(browser.bookmarks.onRemoved, 1);
await p;

Expand All @@ -473,7 +489,7 @@ describe("model/bookmarks", () => {
});

it("when moving bookmarks outside the stash root", async () => {
const p = model.move(new_child.id, bms.names.id, 5);
const p = model.move(new_child, model.folder(bms.names.id)!, 5);
await events.next(browser.bookmarks.onMoved);
await p;

Expand Down
62 changes: 35 additions & 27 deletions src/model/bookmarks.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,19 @@ export class Model {
});
}

/** Creates a bookmark folder and waits for the model to see it. */
createFolder(opts: {
title: string;
parent: Folder;
index?: number;
}): Promise<Folder> {
return this.create({
title: opts.title,
parentId: opts.parent.id,
index: opts.index,
}) as Promise<Folder>;
}

/** Updates a bookmark's title and waits for the model to reflect the
* update. */
async rename(bm: Bookmark | Folder, title: string): Promise<void> {
Expand All @@ -319,32 +332,29 @@ export class Model {
* If the node is part of the stash and belongs to an unnamed folder which
* is now empty, cleanup that folder as well.
*/
async remove(id: NodeID): Promise<void> {
const node = this.node(id);
if (!node) return;

async remove(node: Node): Promise<void> {
const pos = node.position;

await browser.bookmarks.remove(id);
await browser.bookmarks.remove(node.id);

// Wait for the model to catch up
await shortPoll(() => {
// Wait for the model to catch up
if (this.by_id.has(id)) tryAgain();
if (this.by_id.has(node.id)) tryAgain();
});

if (pos) await this.maybeCleanupEmptyFolder(pos.parent);
}

/** Deletes an entire tree of bookmarks and waits for the model to reflect
* the deletion. */
async removeTree(id: NodeID): Promise<void> {
await browser.bookmarks.removeTree(id);
async removeTree(node: Node): Promise<void> {
await browser.bookmarks.removeTree(node.id);

// Wait for the model to catch up
await shortPoll(() => {
// Wait for the model to catch up
if (this.by_id.has(id)) tryAgain();
if (this.by_id.has(node.id)) tryAgain();
});
}

Expand All @@ -354,42 +364,40 @@ export class Model {
*
* Use this instead of `browser.bookmarks.move()`, which behaves differently
* in Chrome and Firefox... */
async move(id: NodeID, toParent: NodeID, toIndex: number): Promise<void> {
async move(node: Node, toParent: Folder, toIndex: number): Promise<void> {
// Firefox's `index` parameter behaves like the bookmark is first
// removed, then re-added. Chrome's/Edge's behaves like the bookmark is
// first added, then removed from its old location, so the index of the
// item after the move will sometimes be toIndex-1 instead of toIndex;
// we account for this below.
const node = expect(this.node(id), () => `No such bookmark node: ${id}`);
const position = expect(
node.position,
() => `Unable to locate node ${id} in its parent`,
() => `Unable to locate node ${node.id} in its parent`,
);

// Clamp the destination index based on the model length, or the poll
// below won't see the index it's expecting. (This isn't 100%
// reliable--we might still get an exception if multiple concurrent
// moves are going on, but even Firefox itself has bugs in this
// situation, soooo... *shrug*)
const toParentFolder = expect(
this.folder(toParent),
() => `Unable to locate destination folder: ${toParent}`,
);
toIndex = Math.min(toParentFolder.children.length, Math.max(0, toIndex));
toIndex = Math.min(toParent.children.length, Math.max(0, toIndex));

/* c8 ignore next -- platform-specific check */
if (!!browser.runtime.getBrowserInfo) {
// We're using Firefox
if (position.parent.id === toParent) {
if (position.parent === toParent) {
if (toIndex > position.index) toIndex--;
}
}
await browser.bookmarks.move(id, {parentId: toParent, index: toIndex});
await browser.bookmarks.move(node.id, {
parentId: toParent.id,
index: toIndex,
});
await shortPoll(() => {
const pos = node.position;
/* c8 ignore next -- race avoidance */
if (!pos) tryAgain();
if (pos.parent.id !== toParent || pos.index !== toIndex) tryAgain();
if (pos.parent !== toParent || pos.index !== toIndex) tryAgain();
});

await this.maybeCleanupEmptyFolder(position.parent);
Expand Down Expand Up @@ -420,7 +428,7 @@ export class Model {
// threads see the same ordering of candidate folders (and thus
// will all choose the same folder to save) because the
// candidate list is sorted deterministically.
await this.remove(candidates[1].id).catch(() => {});
await this.remove(candidates[1]).catch(() => {});
delay += 10;
}
await new Promise(r => setTimeout(r, 5 * Math.random()));
Expand All @@ -436,18 +444,18 @@ export class Model {
* default name will be assigned based on the folder's creation time. */
async createStashFolder(
name?: string,
parent?: NodeID,
parent?: Folder,
position?: "top" | "bottom",
): Promise<Folder> {
const stash_root = await this.ensureStashRoot();
parent ??= stash_root.id;
parent ??= stash_root;
position ??= "top";

const bm = await this.create({
parentId: parent,
parentId: parent.id,
title: name ?? genDefaultFolderName(new Date()),
// !-cast: this.create() will check the existence of the parent for us
index: position === "top" ? 0 : this.folder(parent)!.children.length,
index: position === "top" ? 0 : parent.children.length,
});
return bm as Folder;
}
Expand All @@ -465,7 +473,7 @@ export class Model {
//
// ALSO NOTE: If the folder is suddenly NOT empty due to a race, stale
// model, etc., this will fail, because the browser itself will throw.
await this.remove(folder.id);
await this.remove(folder);
}

//
Expand Down Expand Up @@ -761,7 +769,7 @@ export class Model {
) => {
if (levels > 0) {
for (let i = 0; i < options.folder_count; ++i) {
const f = await this.createStashFolder(undefined, parent.id);
const f = await this.createStashFolder(undefined, parent);
await populate_folder(f, levels - 1, `${path}-${i}`);
}
} else {
Expand Down
Loading

0 comments on commit cc50982

Please sign in to comment.