diff --git a/src/lib/Scanner.ts b/src/lib/Scanner.ts index 231c6ba685..9b67328ebe 100644 --- a/src/lib/Scanner.ts +++ b/src/lib/Scanner.ts @@ -17,15 +17,17 @@ export default class Scanner private mergeable: (i1: TItem, i2: TItem) => boolean private preserveOrder: boolean private checkHashes: boolean + private hasCache: boolean private result: ScanResult - constructor(oldTree:TItem, newTree:TItem, mergeable:(i1:TItem, i2:TItem)=>boolean, preserveOrder:boolean, checkHashes = true) { + constructor(oldTree:TItem, newTree:TItem, mergeable:(i1:TItem, i2:TItem)=>boolean, preserveOrder:boolean, checkHashes = true, hasCache = true) { this.oldTree = oldTree this.newTree = newTree this.mergeable = mergeable this.preserveOrder = preserveOrder this.checkHashes = typeof checkHashes === 'undefined' ? true : checkHashes + this.hasCache = hasCache this.result = { CREATE: new Diff(), UPDATE: new Diff(), @@ -159,7 +161,9 @@ export default class Scanner await Promise.resolve() const removedItem = removeAction.payload - if (this.mergeable(removedItem, createdItem)) { + if (this.mergeable(removedItem, createdItem) && + (removedItem.type !== 'folder' || + (!this.hasCache && removedItem.childrenSimilarity(createdItem) > 0.8))) { this.result.CREATE.retract(createAction) this.result.REMOVE.retract(removeAction) this.result.MOVE.commit({ @@ -187,7 +191,11 @@ export default class Scanner // give the browser time to breathe await Promise.resolve() const removedItem = removeAction.payload - const oldItem = removedItem.findItemFilter(createdItem.type, item => this.mergeable(item, createdItem)) + const oldItem = removedItem.findItemFilter( + createdItem.type, + item => this.mergeable(item, createdItem), + item => item.childrenSimilarity(createdItem) + ) if (oldItem) { let oldIndex this.result.CREATE.retract(createAction) @@ -215,7 +223,11 @@ export default class Scanner await this.diffItem(oldItem, createdItem) } } else { - const newItem = createdItem.findItemFilter(removedItem.type, item => this.mergeable(removedItem, item)) + const newItem = createdItem.findItemFilter( + removedItem.type, + item => this.mergeable(removedItem, item), + item => item.childrenSimilarity(removedItem) + ) let index if (newItem) { this.result.REMOVE.retract(removeAction) diff --git a/src/lib/Tree.ts b/src/lib/Tree.ts index 1910142af8..49f8351ec4 100644 --- a/src/lib/Tree.ts +++ b/src/lib/Tree.ts @@ -74,6 +74,10 @@ export class Bookmark { return false } + childrenSimilarity(otherItem: TItem): number { + return 0 + } + async hash():Promise { if (!this.hashValue) { this.hashValue = await Crypto.sha256( @@ -107,7 +111,7 @@ export class Bookmark { } // TODO: Make this return the correct type based on the type param - findItemFilter(type:TItemType, fn:(Item)=>boolean):TItem|null { + findItemFilter(type:TItemType, fn:(item:TItem)=>boolean, prefer:(item: TItem)=>number = () => 1):TItem|null { if (type === ItemType.BOOKMARK && fn(this)) { return this } @@ -185,11 +189,13 @@ export class Folder { } // eslint-disable-next-line no-use-before-define - findItemFilter(type:TItemType, fn:(Item)=>boolean):TItem|null { + findItemFilter(type:TItemType, fn:(Item)=>boolean, prefer:(Item)=>number = () => 1):TItem|null { if (!this.index) { this.createIndex() } - return Object.values(this.index[type]).find(fn) + const candidates = Object.values(this.index[type]).filter(fn) + // return the preferred match based on a preference measure + return candidates.sort((a,b) => prefer(a) - prefer(b)).pop() } findFolder(id:string|number): Folder { @@ -256,6 +262,17 @@ export class Folder { return false } + childrenSimilarity(otherItem: TItem): number { + if (otherItem instanceof Folder) { + return this.children.reduce( + (count, item) => + otherItem.children.find(i => i.title === item.title) ? count + 1 : count, + 0 + ) / Math.max(this.children.length, otherItem.children.length) + } + return 0 + } + async hash(preserveOrder = false): Promise { if (this.hashValue && this.hashValue[String(preserveOrder)]) { return this.hashValue[String(preserveOrder)] diff --git a/src/lib/strategies/Merge.ts b/src/lib/strategies/Merge.ts index e2c4c411a0..c98670ef61 100644 --- a/src/lib/strategies/Merge.ts +++ b/src/lib/strategies/Merge.ts @@ -23,6 +23,7 @@ export default class MergeSyncProcess extends DefaultSyncProcess { return false }, this.preserveOrder, + false, false ) const serverScanner = new Scanner( @@ -36,6 +37,7 @@ export default class MergeSyncProcess extends DefaultSyncProcess { return false }, this.preserveOrder, + false, false ) const localScanResult = await localScanner.run() @@ -96,6 +98,7 @@ export default class MergeSyncProcess extends DefaultSyncProcess { return false }, this.preserveOrder, + false, false ) await subScanner.run() diff --git a/src/lib/strategies/Unidirectional.ts b/src/lib/strategies/Unidirectional.ts index 0b83028a6a..8f4f145889 100644 --- a/src/lib/strategies/Unidirectional.ts +++ b/src/lib/strategies/Unidirectional.ts @@ -51,6 +51,7 @@ export default class UnidirectionalSyncProcess extends DefaultStrategy { return false }, this.preserveOrder, + false, false ) const serverScanner = new Scanner( @@ -64,6 +65,7 @@ export default class UnidirectionalSyncProcess extends DefaultStrategy { return false }, this.preserveOrder, + false, false ) const localScanResult = await localScanner.run() diff --git a/src/test/test.js b/src/test/test.js index ef9f0aacb9..e191ad6f27 100644 --- a/src/test/test.js +++ b/src/test/test.js @@ -1893,6 +1893,107 @@ describe('Floccus', function() { Boolean(account.server.orderFolder) ) }) + it('should move items without confusing folders', async function() { + const localRoot = account.getData().localRoot + + const aFolder = await browser.bookmarks.create({ + title: 'a', + parentId: localRoot + }) + const bFolder = await browser.bookmarks.create({ + title: 'b', + parentId: localRoot + }) + const dFolder = await browser.bookmarks.create({ + title: 'd', + parentId: localRoot + }) + const cFolder1 = await browser.bookmarks.create({ + title: 'c', + parentId: aFolder.id + }) + await browser.bookmarks.create({ + title: 'url', + url: 'http://ur.l/', + parentId: cFolder1.id + }) + const cFolder2 = await browser.bookmarks.create({ + title: 'c', + parentId: bFolder.id + }) + await browser.bookmarks.create({ + title: 'test', + url: 'http://urrr.l/', + parentId: cFolder2.id + }) + + await account.sync() // propagate to server + expect(account.getData().error).to.not.be.ok + + await account.sync() // make sure order is propagated + expect(account.getData().error).to.not.be.ok + + await account.init() + + // move b into a in client + await browser.bookmarks.move(cFolder1.id, { parentId: localRoot }) + await browser.bookmarks.move(cFolder2.id, { parentId: dFolder.id }) + + await account.sync() // propagate to server + expect(account.getData().error).to.not.be.ok + + const tree = await getAllBookmarks(account) + expectTreeEqual( + tree, + new Folder({ + title: tree.title, + children: [ + new Folder({ + title: 'a', + children: [] + }), + new Folder({ + title: 'b', + children: [] + }), + new Folder({ + title: 'd', + children: [ + new Folder({ + title: 'c', + children: [ + new Bookmark({ + title: 'test', + url: 'http://urrr.l/', + }) + ] + }) + ] + }), + new Folder({ + title: 'c', + children: [ + new Bookmark({ + title: 'url', + url: 'http://ur.l/', + }) + ] + }), + ] + }), + false, + false + ) + + const localTree = await account.localTree.getBookmarksTree(true) + localTree.title = tree.title + expectTreeEqual( + localTree, + tree, + false, + false + ) + }) it('should integrate existing items from both sides', async function() { const localRoot = account.getData().localRoot @@ -4120,6 +4221,9 @@ describe('Floccus', function() { ) }) it('should handle complex hierarchy reversals 2', async function() { + if (ACCOUNT_DATA.type === 'linkwarden') { + return this.skip() + } const localRoot = account1.getData().localRoot const aFolder = await browser.bookmarks.create({ title: 'a',