Skip to content

Commit

Permalink
Giant Model Refactoring
Browse files Browse the repository at this point in the history 
Store direct references between nodes in the bookmark and tabs trees,
which should hopefully improve performance by avoiding lookups when
accessing child nodes (among other things).

This required a bunch of changes throughout the codebase, particularly
with respect to generic trees, and some slight changes to the typings of
specific trees.

As a side effect, this forced me to change how DnD works, because we
can't serialize model items into JSON anymore (they have circular
references now). So we make DnD a bit more efficient (hopefully) by
serializing just the IDs. And we make changing DnD less error-prone, by
moving the "on-the-wire" protocol into its own file.
  • Loading branch information
@josh-berry
josh-berry committed Jun 3, 2023
1 parent 9f75f4a commit 45b7956
Show file tree
Hide file tree
Showing 24 changed files with 1,249 additions and 784 deletions.
9 changes: 5 additions & 4 deletions src/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,14 @@ logErrorsFrom(async () => {
copy?: boolean;
tab?: Tab;
}) {
if (!options.tab || options.tab.windowId === undefined) return;
if (!options.tab || options.tab.position === undefined) return;

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

case "single":
Expand Down
152 changes: 63 additions & 89 deletions src/model/bookmarks.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ describe("model/bookmarks", () => {

const parent = model.folder(template.parentId!)!;
expect(parent).to.have.property("children");
expect(parent.children[template.index!]).to.equal(bm!.id);
expect(parent.children[template.index!]).to.equal(bm);
}
});

Expand All @@ -48,7 +48,9 @@ describe("model/bookmarks", () => {

const bm = model.folder(template.id)!;
expect(bm.id).to.equal(template.id);
expect(bm.children).to.deep.equal(template.children.map(c => c.id));
expect(bm.children.map(bm => bm.id)).to.deep.equal(
template.children.map(c => c.id),
);
}
});

Expand Down Expand Up @@ -94,14 +96,20 @@ describe("model/bookmarks", () => {
delete new_bm.type;
await events.next(browser.bookmarks.onCreated);

expect(model.node(new_bm.id as M.NodeID)).to.deep.equal({
...new_bm,
const n = model.node(new_bm.id as M.NodeID)!;
expect(n).to.deep.include({
id: new_bm.id as M.NodeID,
title: "New",
url: "/new",
$selected: false,
});
expect(model.bookmarksWithURL("/new")).to.deep.equal(
new Set([{...new_bm, $selected: false}]),
);
expect(model.folder(bms.root.id)!.children).to.deep.equal([
expect(n.position).to.deep.equal({
parent: model.folder(bms.root.id)!,
index: 2,
});

expect(model.bookmarksWithURL("/new")).to.deep.equal(new Set([n]));
expect(model.folder(bms.root.id)!.children.map(bm => bm.id)).to.deep.equal([
bms.doug_1.id,
bms.francis.id,
new_bm.id,
Expand All @@ -125,17 +133,18 @@ describe("model/bookmarks", () => {
delete new_a.type;
delete new_a.index;

expect(model.node(bms.alice.id)).to.deep.include(new_a);
expect(model.node(bms.alice.id)).to.deep.include({
id: bms.alice.id,
title: "The New A",
url: "/new_a",
});
expect(model.bookmarksWithURL(`${B}#alice`)).to.deep.equal(new Set([]));
expect(model.bookmarksWithURL("/new_a")).to.deep.equal(
new Set([model.node(bms.alice.id)]),
);
expect(model.folder(bms.outside.id)!.children).to.deep.equal([
bms.alice.id,
bms.separator.id,
bms.bob.id,
bms.empty.id,
]);
expect(
model.folder(bms.outside.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.alice.id, bms.separator.id, bms.bob.id, bms.empty.id]);
});

it("updates bookmarks", async () => {
Expand All @@ -153,12 +162,9 @@ describe("model/bookmarks", () => {
expect(model.bookmarksWithURL("/new_a")).to.deep.equal(
new Set([model.node(bms.alice.id)]),
);
expect(model.folder(bms.outside.id)!.children).to.deep.equal([
bms.alice.id,
bms.separator.id,
bms.bob.id,
bms.empty.id,
]);
expect(
model.folder(bms.outside.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.alice.id, bms.separator.id, bms.bob.id, bms.empty.id]);
});

it("updates folder titles", async () => {
Expand All @@ -176,11 +182,9 @@ describe("model/bookmarks", () => {

expect(model.node(bms.bob.id)).to.be.undefined;
expect(model.bookmarksWithURL(`${B}#bob`)).to.deep.equal(new Set([]));
expect(model.folder(bms.outside.id)!.children).to.deep.equal([
bms.alice.id,
bms.separator.id,
bms.empty.id,
]);
expect(
model.folder(bms.outside.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.alice.id, bms.separator.id, bms.empty.id]);
});

it("removes folders idempotently", async () => {
Expand All @@ -203,79 +207,45 @@ describe("model/bookmarks", () => {
);

expect(model.node(bms.names.id)).to.be.undefined;
expect(model.folder(bms.stash_root.id)!.children).to.deep.equal([
bms.unnamed.id,
bms.big_stash.id,
bms.nested.id,
]);
expect(
model.folder(bms.stash_root.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.unnamed.id, bms.big_stash.id, bms.nested.id]);
});

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

expect(model.folder(bms.outside.id)!.children).to.deep.equal([
bms.separator.id,
bms.bob.id,
bms.empty.id,
bms.alice.id,
]);
expect(
model.folder(bms.outside.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.separator.id, bms.bob.id, bms.empty.id, bms.alice.id]);
});

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

expect(model.folder(bms.outside.id)!.children).to.deep.equal([
bms.empty.id,
bms.alice.id,
bms.separator.id,
bms.bob.id,
]);
expect(
model.folder(bms.outside.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.empty.id, bms.alice.id, bms.separator.id, bms.bob.id]);
});

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

expect(model.folder(bms.outside.id)!.children).to.deep.equal([
bms.alice.id,
bms.separator.id,
bms.empty.id,
]);
expect(model.folder(bms.names.id)!.children).to.deep.equal([
bms.doug_2.id,
bms.helen.id,
bms.bob.id,
bms.patricia.id,
bms.nate.id,
]);
expect(
model.folder(bms.outside.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.alice.id, bms.separator.id, bms.empty.id]);
expect(model.folder(bms.names.id)!.children.map(bm => bm.id)).to.deep.equal(
[bms.doug_2.id, bms.helen.id, bms.bob.id, bms.patricia.id, bms.nate.id],
);
});

describe("reports info about bookmarks", () => {
it("bookmark is in a folder", () => {
const undyne = model.node(bms.undyne.id)!;
expect(model.isNodeInFolder(undyne, bms.stash_root.id)).to.be.true;
});

it("bookmark is NOT in a folder", () => {
const alice = model.node(bms.alice.id)!;
expect(model.isNodeInFolder(alice, bms.stash_root.id)).to.be.false;
});

it("path to a bookmark", async () => {
const helen = model.node(bms.helen.id)!;
expect(model.pathTo(helen)).to.deep.equal([
{parent: model.node(model.root_id!), index: 0},
{parent: model.node(bms.root.id), index: 3},
{parent: model.node(bms.stash_root.id), index: 0},
{parent: model.node(bms.names.id), index: 1},
]);
});

describe("folders in the stash containing a URL", () => {
function test(
name: string,
Expand Down Expand Up @@ -558,11 +528,9 @@ describe("model/bookmarks", () => {
await p;

expect(model.node(bms.unnamed.id)).to.be.undefined;
expect(model.folder(bms.stash_root.id)!.children).to.deep.equal([
bms.names.id,
bms.big_stash.id,
bms.nested.id,
]);
expect(
model.folder(bms.stash_root.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.names.id, bms.big_stash.id, bms.nested.id]);
});

it("when moving stash bookmarks to another folder", async () => {
Expand All @@ -572,12 +540,12 @@ describe("model/bookmarks", () => {
await p;

expect(model.node(bms.unnamed.id)).to.be.undefined;
expect(model.folder(bms.stash_root.id)!.children).to.deep.equal([
bms.names.id,
bms.big_stash.id,
bms.nested.id,
]);
expect(model.folder(bms.names.id)!.children).to.deep.equal([
expect(
model.folder(bms.stash_root.id)!.children.map(bm => bm.id),
).to.deep.equal([bms.names.id, bms.big_stash.id, bms.nested.id]);
expect(
model.folder(bms.names.id)!.children.map(bm => bm.id),
).to.deep.equal([
bms.doug_2.id,
bms.helen.id,
bms.patricia.id,
Expand Down Expand Up @@ -614,7 +582,9 @@ describe("model/bookmarks", () => {

expect(model.node(new_child.id)).to.be.undefined;
expect(model.folder(new_unnamed.id)!.children).to.deep.equal([]);
expect(model.folder(bms.outside.id)!.children).to.deep.equal([
expect(
model.folder(bms.outside.id)!.children.map(bm => bm.id),
).to.deep.equal([
bms.alice.id,
bms.separator.id,
bms.bob.id,
Expand All @@ -629,14 +599,18 @@ describe("model/bookmarks", () => {
await p;

expect(model.folder(new_unnamed.id)!.children).to.deep.equal([]);
expect(model.folder(bms.outside.id)!.children).to.deep.equal([
expect(
model.folder(bms.outside.id)!.children.map(bm => bm.id),
).to.deep.equal([
bms.alice.id,
bms.separator.id,
bms.bob.id,
bms.empty.id,
new_unnamed.id,
]);
expect(model.folder(bms.names.id)!.children).to.deep.equal([
expect(
model.folder(bms.names.id)!.children.map(bm => bm.id),
).to.deep.equal([
bms.doug_2.id,
bms.helen.id,
bms.patricia.id,
Expand Down
Loading

0 comments on commit 45b7956

Please sign in to comment.