Skip to content

Commit 98d7d8d

Browse files
marcelklehrmarcelklehr
authored
Merge pull request #1775 from floccusaddon/test/moving-stability
Move stability with same-titled folders
2 parents b5df214 + 36e6aa8 commit 98d7d8d

File tree

5 files changed

+145
-7
lines changed

5 files changed

+145
-7
lines changed

‎src/lib/Scanner.ts‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,17 @@ export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation>
1717
private mergeable: (i1: TItem<TItemLocation>, i2: TItem<TItemLocation>) => boolean
1818
private preserveOrder: boolean
1919
private checkHashes: boolean
20+
private hasCache: boolean
2021

2122
private result: ScanResult<L2, L1>
2223

23-
constructor(oldTree:TItem<L1>, newTree:TItem<L2>, mergeable:(i1:TItem<TItemLocation>, i2:TItem<TItemLocation>)=>boolean, preserveOrder:boolean, checkHashes = true) {
24+
constructor(oldTree:TItem<L1>, newTree:TItem<L2>, mergeable:(i1:TItem<TItemLocation>, i2:TItem<TItemLocation>)=>boolean, preserveOrder:boolean, checkHashes = true, hasCache = true) {
2425
this.oldTree = oldTree
2526
this.newTree = newTree
2627
this.mergeable = mergeable
2728
this.preserveOrder = preserveOrder
2829
this.checkHashes = typeof checkHashes === 'undefined' ? true : checkHashes
30+
this.hasCache = hasCache
2931
this.result = {
3032
CREATE: new Diff(),
3133
UPDATE: new Diff(),
@@ -159,7 +161,9 @@ export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation>
159161
await Promise.resolve()
160162
const removedItem = removeAction.payload
161163

162-
if (this.mergeable(removedItem, createdItem)) {
164+
if (this.mergeable(removedItem, createdItem) &&
165+
(removedItem.type !== 'folder' ||
166+
(!this.hasCache && removedItem.childrenSimilarity(createdItem) > 0.8))) {
163167
this.result.CREATE.retract(createAction)
164168
this.result.REMOVE.retract(removeAction)
165169
this.result.MOVE.commit({
@@ -187,7 +191,11 @@ export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation>
187191
// give the browser time to breathe
188192
await Promise.resolve()
189193
const removedItem = removeAction.payload
190-
const oldItem = removedItem.findItemFilter(createdItem.type, item => this.mergeable(item, createdItem))
194+
const oldItem = removedItem.findItemFilter(
195+
createdItem.type,
196+
item => this.mergeable(item, createdItem),
197+
item => item.childrenSimilarity(createdItem)
198+
)
191199
if (oldItem) {
192200
let oldIndex
193201
this.result.CREATE.retract(createAction)
@@ -215,7 +223,11 @@ export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation>
215223
await this.diffItem(oldItem, createdItem)
216224
}
217225
} else {
218-
const newItem = createdItem.findItemFilter(removedItem.type, item => this.mergeable(removedItem, item))
226+
const newItem = createdItem.findItemFilter(
227+
removedItem.type,
228+
item => this.mergeable(removedItem, item),
229+
item => item.childrenSimilarity(removedItem)
230+
)
219231
let index
220232
if (newItem) {
221233
this.result.REMOVE.retract(removeAction)

‎src/lib/Tree.ts‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ export class Bookmark<L extends TItemLocation> {
7474
return false
7575
}
7676

77+
childrenSimilarity<L2 extends TItemLocation>(otherItem: TItem<L2>): number {
78+
return 0
79+
}
80+
7781
async hash():Promise<string> {
7882
if (!this.hashValue) {
7983
this.hashValue = await Crypto.sha256(
@@ -107,7 +111,7 @@ export class Bookmark<L extends TItemLocation> {
107111
}
108112

109113
// TODO: Make this return the correct type based on the type param
110-
findItemFilter(type:TItemType, fn:(Item)=>boolean):TItem<L>|null {
114+
findItemFilter(type:TItemType, fn:(item:TItem<L>)=>boolean, prefer:(item: TItem<L>)=>number = () => 1):TItem<L>|null {
111115
if (type === ItemType.BOOKMARK && fn(this)) {
112116
return this
113117
}
@@ -185,11 +189,13 @@ export class Folder<L extends TItemLocation> {
185189
}
186190

187191
// eslint-disable-next-line no-use-before-define
188-
findItemFilter(type:TItemType, fn:(Item)=>boolean):TItem<L>|null {
192+
findItemFilter(type:TItemType, fn:(Item)=>boolean, prefer:(Item)=>number = () => 1):TItem<L>|null {
189193
if (!this.index) {
190194
this.createIndex()
191195
}
192-
return Object.values(this.index[type]).find(fn)
196+
const candidates = Object.values(this.index[type]).filter(fn)
197+
// return the preferred match based on a preference measure
198+
return candidates.sort((a,b) => prefer(a) - prefer(b)).pop()
193199
}
194200

195201
findFolder(id:string|number): Folder<L> {
@@ -256,6 +262,17 @@ export class Folder<L extends TItemLocation> {
256262
return false
257263
}
258264

265+
childrenSimilarity<L2 extends TItemLocation>(otherItem: TItem<L2>): number {
266+
if (otherItem instanceof Folder) {
267+
return this.children.reduce(
268+
(count, item) =>
269+
otherItem.children.find(i => i.title === item.title) ? count + 1 : count,
270+
0
271+
) / Math.max(this.children.length, otherItem.children.length)
272+
}
273+
return 0
274+
}
275+
259276
async hash(preserveOrder = false): Promise<string> {
260277
if (this.hashValue && this.hashValue[String(preserveOrder)]) {
261278
return this.hashValue[String(preserveOrder)]

‎src/lib/strategies/Merge.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ export default class MergeSyncProcess extends DefaultSyncProcess {
2323
return false
2424
},
2525
this.preserveOrder,
26+
false,
2627
false
2728
)
2829
const serverScanner = new Scanner(
@@ -36,6 +37,7 @@ export default class MergeSyncProcess extends DefaultSyncProcess {
3637
return false
3738
},
3839
this.preserveOrder,
40+
false,
3941
false
4042
)
4143
const localScanResult = await localScanner.run()
@@ -96,6 +98,7 @@ export default class MergeSyncProcess extends DefaultSyncProcess {
9698
return false
9799
},
98100
this.preserveOrder,
101+
false,
99102
false
100103
)
101104
await subScanner.run()

‎src/lib/strategies/Unidirectional.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ export default class UnidirectionalSyncProcess extends DefaultStrategy {
5151
return false
5252
},
5353
this.preserveOrder,
54+
false,
5455
false
5556
)
5657
const serverScanner = new Scanner(
@@ -64,6 +65,7 @@ export default class UnidirectionalSyncProcess extends DefaultStrategy {
6465
return false
6566
},
6667
this.preserveOrder,
68+
false,
6769
false
6870
)
6971
const localScanResult = await localScanner.run()

‎src/test/test.js‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1893,6 +1893,107 @@ describe('Floccus', function() {
18931893
Boolean(account.server.orderFolder)
18941894
)
18951895
})
1896+
it('should move items without confusing folders', async function() {
1897+
const localRoot = account.getData().localRoot
1898+
1899+
const aFolder = await browser.bookmarks.create({
1900+
title: 'a',
1901+
parentId: localRoot
1902+
})
1903+
const bFolder = await browser.bookmarks.create({
1904+
title: 'b',
1905+
parentId: localRoot
1906+
})
1907+
const dFolder = await browser.bookmarks.create({
1908+
title: 'd',
1909+
parentId: localRoot
1910+
})
1911+
const cFolder1 = await browser.bookmarks.create({
1912+
title: 'c',
1913+
parentId: aFolder.id
1914+
})
1915+
await browser.bookmarks.create({
1916+
title: 'url',
1917+
url: 'http://ur.l/',
1918+
parentId: cFolder1.id
1919+
})
1920+
const cFolder2 = await browser.bookmarks.create({
1921+
title: 'c',
1922+
parentId: bFolder.id
1923+
})
1924+
await browser.bookmarks.create({
1925+
title: 'test',
1926+
url: 'http://urrr.l/',
1927+
parentId: cFolder2.id
1928+
})
1929+
1930+
await account.sync() // propagate to server
1931+
expect(account.getData().error).to.not.be.ok
1932+
1933+
await account.sync() // make sure order is propagated
1934+
expect(account.getData().error).to.not.be.ok
1935+
1936+
await account.init()
1937+
1938+
// move b into a in client
1939+
await browser.bookmarks.move(cFolder1.id, { parentId: localRoot })
1940+
await browser.bookmarks.move(cFolder2.id, { parentId: dFolder.id })
1941+
1942+
await account.sync() // propagate to server
1943+
expect(account.getData().error).to.not.be.ok
1944+
1945+
const tree = await getAllBookmarks(account)
1946+
expectTreeEqual(
1947+
tree,
1948+
new Folder({
1949+
title: tree.title,
1950+
children: [
1951+
new Folder({
1952+
title: 'a',
1953+
children: []
1954+
}),
1955+
new Folder({
1956+
title: 'b',
1957+
children: []
1958+
}),
1959+
new Folder({
1960+
title: 'd',
1961+
children: [
1962+
new Folder({
1963+
title: 'c',
1964+
children: [
1965+
new Bookmark({
1966+
title: 'test',
1967+
url: 'http://urrr.l/',
1968+
})
1969+
]
1970+
})
1971+
]
1972+
}),
1973+
new Folder({
1974+
title: 'c',
1975+
children: [
1976+
new Bookmark({
1977+
title: 'url',
1978+
url: 'http://ur.l/',
1979+
})
1980+
]
1981+
}),
1982+
]
1983+
}),
1984+
false,
1985+
false
1986+
)
1987+
1988+
const localTree = await account.localTree.getBookmarksTree(true)
1989+
localTree.title = tree.title
1990+
expectTreeEqual(
1991+
localTree,
1992+
tree,
1993+
false,
1994+
false
1995+
)
1996+
})
18961997
it('should integrate existing items from both sides', async function() {
18971998
const localRoot = account.getData().localRoot
18981999

@@ -4120,6 +4221,9 @@ describe('Floccus', function() {
41204221
)
41214222
})
41224223
it('should handle complex hierarchy reversals 2', async function() {
4224+
if (ACCOUNT_DATA.type === 'linkwarden') {
4225+
return this.skip()
4226+
}
41234227
const localRoot = account1.getData().localRoot
41244228
const aFolder = await browser.bookmarks.create({
41254229
title: 'a',

0 commit comments

Comments
 (0)