diff --git a/src/mock/browser/bookmarks.ts b/src/mock/browser/bookmarks.ts index db0a892..d26c26a 100644 --- a/src/mock/browser/bookmarks.ts +++ b/src/mock/browser/bookmarks.ts @@ -32,6 +32,8 @@ class MockBookmarks implements BM.Static { private readonly root: Folder; private readonly by_id = new Map(); + private readonly new_bm_parent: Folder; + constructor() { this.root = { id: this._freeID(), @@ -42,6 +44,41 @@ class MockBookmarks implements BM.Static { dateAdded: 0, }; this.by_id.set(this.root.id, this.root); + + // Yes, these names are deliberately obtuse, to prevent application code + // from trying to guess at the bookmark structure. :) + this.root.children.push({ + id: this._freeID(), + index: 0, + parentId: this.root.id, + title: "Toolbin", + children: [], + dateAdded: 0, + }); + this.root.children.push({ + id: this._freeID(), + index: 0, + parentId: this.root.id, + title: "Menu Can", + children: [], + dateAdded: 0, + }); + this.root.children.push({ + id: this._freeID(), + index: 0, + parentId: this.root.id, + title: "Compost Box", + children: [], + dateAdded: 0, + }); + for (const c of this.root.children) this.by_id.set(c.id, c); + + this.new_bm_parent = this.root.children[ + Math.floor(Math.random() * 3) + ] as Folder; + + if (!this.new_bm_parent) throw new Error(`bm startup: no new_bm_parent`); + fixup_child_ordering(this.root); } @@ -93,7 +130,7 @@ class MockBookmarks implements BM.Static { throw new Error(`Bookmark type is not supported on Chrome`); } - const parentId = bookmark.parentId ?? this.root.id; + const parentId = bookmark.parentId ?? this.new_bm_parent.id; const parent = this._getFolder(parentId); const index = bookmark.index ?? parent.children.length; diff --git a/src/model/bookmarks.test.ts b/src/model/bookmarks.test.ts index 5cc2759..c3e1171 100644 --- a/src/model/bookmarks.test.ts +++ b/src/model/bookmarks.test.ts @@ -31,23 +31,35 @@ describe("model/bookmarks - creating the stash root", () => { }); it("creates a new stash root when requested", async () => { + expect(model.root).to.be.undefined; + expect(model.stash_root.value).to.be.undefined; + const p = model.ensureStashRoot(); await events.next(browser.bookmarks.onCreated); const stash_root = await p; + expect(stash_root.title).to.equal(STASH_ROOT_NAME); expect(stash_root.isLoaded).to.be.true; expect(stash_root.position?.parent).not.to.be.undefined; }); it("populates model.root and .stash_root after creation", async () => { + expect(model.root).to.be.undefined; + expect(model.stash_root.value).to.be.undefined; + const p = model.ensureStashRoot(); await events.next(browser.bookmarks.onCreated); const stash_root = await p; + expect(model.root).not.to.be.undefined; expect(model.stash_root.value).to.equal(stash_root); }); it("reuses an existing stash root", async () => { + await model.ensureStashRoot(); + expect(model.root).not.to.be.undefined; + expect(model.stash_root.value).not.to.be.undefined; + const p = model.ensureStashRoot(); await events.next(browser.bookmarks.onCreated); const stash_root = await p; @@ -77,6 +89,44 @@ describe("model/bookmarks - creating the stash root", () => { expect(sr1.id).to.equal(sr2.id); expect(sr2.id).to.equal(sr3.id); }); + + it("detects when a new stash root is created", async () => { + expect(model.root).to.be.undefined; + expect(model.stash_root.value).to.be.undefined; + + const btn = await browser.bookmarks.create({title: STASH_ROOT_NAME}); + await events.next(browser.bookmarks.onCreated); + + await shortPoll(() => { + if (!model.root) tryAgain(); + if (!model.stash_root.value) tryAgain(); + }); + expect(model.stash_root.value!.id).to.equal(btn.id); + }); + + it("detects when a folder is renamed to be the stash root", async () => { + const btn = await browser.bookmarks.create({ + title: `not ${STASH_ROOT_NAME}`, + }); + await events.next(browser.bookmarks.onCreated); + + // Re-create the model - we don't want it seeing the creation event + model = await M.Model.from_browser(STASH_ROOT_NAME); + expect(model.root).to.be.undefined; + expect(model.stash_root.value).to.be.undefined; + + // Created bookmark should not be loaded since we're not tracking its parent + expect(model.bookmark(btn.id)).to.be.undefined; + + await browser.bookmarks.update(btn.id, {title: STASH_ROOT_NAME}); + await events.next(browser.bookmarks.onChanged); + + await shortPoll(() => { + if (!model.root) tryAgain(); + if (!model.stash_root.value) tryAgain(); + }); + expect(model.stash_root.value!.id).to.equal(btn.id); + }); }); describe("model/bookmarks", () => { diff --git a/src/model/bookmarks.ts b/src/model/bookmarks.ts index c369ebc..fed6e81 100644 --- a/src/model/bookmarks.ts +++ b/src/model/bookmarks.ts @@ -546,6 +546,8 @@ export class Model { async ensureStashRoot(): Promise { if (this.stash_root.value) return this.stash_root.value; + trace("creating new stash root"); + // NOTE: We don't use this.create() here because it's possible the // newly-created stash root will never be loaded into the model, if its // parent isn't present. The subsequent call(s) to _findRoots() should take @@ -565,6 +567,12 @@ export class Model { let candidates = await this._findRoots(); while (Date.now() - start < delay) { if (candidates.length > 1) { + trace( + "race detected; winner = ", + candidates[0].id, + "loser = ", + candidates[1].id, + ); // If we find MULTIPLE candidates so soon after finding NONE, // there must be multiple threads trying to create the root // folder. Let's try to remove one. We are guaranteed that all @@ -579,6 +587,7 @@ export class Model { } // END GROSS HACK + trace("converged on stash root = ", candidates[0].id); return candidates[0]; } @@ -640,7 +649,14 @@ export class Model { whenBookmarkChanged(id: string, info: Bookmarks.OnChangedChangeInfoType) { trace("whenBookmarkChanged", id, info); const node = this.node(id as NodeID); - if (!node) return; + + if (!node) { + // If we see a node is being renamed to a name that matches the stash + // root, we should check if it's becoming the new stash root. + if (info.title === this.stash_root_name) this._maybeUpdateStashRoot(); + return; + } + this._updateNode(node, info); } @@ -718,6 +734,8 @@ export class Model { await browser.bookmarks.search(this.stash_root_name) ).filter(c => isBrowserBTNFolder(c) && c.title === this.stash_root_name); + trace("_findRoots", searched.length, "folders named", this.stash_root_name); + // Make sure those folders and their parents (recursively) are loaded into // the model. This is so we can keep an eye on changes and re-trigger the // stash-root search if anything changes. @@ -725,6 +743,7 @@ export class Model { let to_upsert = Array.from(searched); while (to_fetch.size > 0) { + trace("_findRoots fetching", to_fetch); const bms = await browser.bookmarks.get(Array.from(to_fetch)); to_fetch = new Set(); @@ -773,21 +792,17 @@ export class Model { if (candidates.length > 0) { if (this.stash_root.value !== candidates[0]) { this.stash_root.value = candidates[0]; - trace( - "_findStashRootCandidates", - "set stash_root to", - candidates[0].id, - ); + trace("_findRoots", "set stash_root to", candidates[0].id); } } else if (this.stash_root.value !== undefined) { - trace("_findStashRootCandidates", "cleared stash_root"); + trace("_findRoots", "cleared stash_root"); this.stash_root.value = undefined; } // But if we have multiple candidates, we need to raise the alarm that // there is an ambiguity the user should resolve. if (candidates.length > 1) { - trace("_findStashRootCandidates", "found multiple candidates"); + trace("_findRoots", "found multiple stash_root candidates"); this.stash_root_warning.value = { text: `You have multiple "${this.stash_root_name}" bookmark ` + @@ -797,7 +812,7 @@ export class Model { help: () => browser.tabs.create({active: true, url: ROOT_FOLDER_HELP}), }; } else if (this.stash_root_warning.value !== undefined) { - trace("_findStashRootCandidates", "found single candidate"); + trace("_findRoots", "found single stash_root candidate"); this.stash_root_warning.value = undefined; }