Merge pull request #1775 from floccusaddon/test/moving-stability

Move stability with same-titled folders

src/lib/Scanner.ts

@@ -17,15 +17,17 @@ export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation>
   private mergeable: (i1: TItem<TItemLocation>, i2: TItem<TItemLocation>) => boolean
   private preserveOrder: boolean
   private checkHashes: boolean
+  private hasCache: boolean
 
   private result: ScanResult<L2, L1>
 
-  constructor(oldTree:TItem<L1>, newTree:TItem<L2>, mergeable:(i1:TItem<TItemLocation>, i2:TItem<TItemLocation>)=>boolean, preserveOrder:boolean, checkHashes = true) {
+  constructor(oldTree:TItem<L1>, newTree:TItem<L2>, mergeable:(i1:TItem<TItemLocation>, i2:TItem<TItemLocation>)=>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<L1 extends TItemLocation, L2 extends TItemLocation>
           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<L1 extends TItemLocation, L2 extends TItemLocation>
           // 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<L1 extends TItemLocation, L2 extends TItemLocation>
               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)

src/lib/Tree.ts

@@ -74,6 +74,10 @@ export class Bookmark<L extends TItemLocation> {
     return false
   }
 
+  childrenSimilarity<L2 extends TItemLocation>(otherItem: TItem<L2>): number {
+    return 0
+  }
+
   async hash():Promise<string> {
     if (!this.hashValue) {
       this.hashValue = await Crypto.sha256(
@@ -107,7 +111,7 @@ export class Bookmark<L extends TItemLocation> {
   }
 
   // TODO: Make this return the correct type based on the type param
-  findItemFilter(type:TItemType, fn:(Item)=>boolean):TItem<L>|null {
+  findItemFilter(type:TItemType, fn:(item:TItem<L>)=>boolean, prefer:(item: TItem<L>)=>number = () => 1):TItem<L>|null {
     if (type === ItemType.BOOKMARK && fn(this)) {
       return this
     }
@@ -185,11 +189,13 @@ export class Folder<L extends TItemLocation> {
   }
 
   // eslint-disable-next-line no-use-before-define
-  findItemFilter(type:TItemType, fn:(Item)=>boolean):TItem<L>|null {
+  findItemFilter(type:TItemType, fn:(Item)=>boolean, prefer:(Item)=>number = () => 1):TItem<L>|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<L> {
@@ -256,6 +262,17 @@ export class Folder<L extends TItemLocation> {
     return false
   }
 
+  childrenSimilarity<L2 extends TItemLocation>(otherItem: TItem<L2>): 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<string> {
     if (this.hashValue && this.hashValue[String(preserveOrder)]) {
       return this.hashValue[String(preserveOrder)]

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()

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()

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',