Skip to content

Commit 6c0df45

Browse files
marcelklehrmarcelklehr
committed
refactor(Scanner): Move mapping creation from mergeable fn to diffItem
With this change, mappings will only be created if both childrenSimilarity and mergeable give a go. Before, mergeable would sometimes create mappings prematurely. Signed-off-by: Marcel Klehr <mklehr@gmx.net>
1 parent f2b78ad commit 6c0df45

5 files changed

Lines changed: 105 additions & 73 deletions

File tree

‎src/lib/Mappings.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ export default class Mappings {
4848
}
4949

5050
async addFolder({ localId, remoteId }: { localId?:string|number, remoteId?:string|number }):Promise<void> {
51+
Mappings.remove(this.folders, { localId, remoteId })
5152
Mappings.add(this.folders, { localId, remoteId })
5253
}
5354

@@ -56,6 +57,7 @@ export default class Mappings {
5657
}
5758

5859
async addBookmark({ localId, remoteId }: { localId?:string|number, remoteId?:string|number }):Promise<void> {
60+
Mappings.remove(this.bookmarks, { localId, remoteId })
5961
Mappings.add(this.bookmarks, { localId, remoteId })
6062
}
6163

‎src/lib/Scanner.ts‎

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import { Bookmark, Folder, ItemLocation, ItemType, TItem, TItemLocation } from '
44
import Logger from './Logger'
55
import { IHashSettings } from './interfaces/Resource'
66
import { yieldToEventLoop } from './yieldToEventLoop'
7+
import Mappings from './Mappings'
8+
import optionDownloadLogs from '../ui/components/OptionDownloadLogs.vue'
79

810
export interface ScanResult<L1 extends TItemLocation, L2 extends TItemLocation> {
911
CREATE: Diff<L1, L2, CreateAction<L1, L2>>
@@ -14,23 +16,36 @@ export interface ScanResult<L1 extends TItemLocation, L2 extends TItemLocation>
1416
}
1517

1618
export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation> {
19+
private mappings: Mappings
1720
private oldTree: TItem<L1>
1821
private newTree: TItem<L2>
1922
private mergeable: (i1: TItem<TItemLocation>, i2: TItem<TItemLocation>) => boolean
2023
private hashSettings: IHashSettings
2124
private checkHashes: boolean
2225
private hasCache: boolean
26+
private createMappings: boolean
2327
private iterations = 0
2428

2529
private result: ScanResult<L2, L1>
2630

27-
constructor(oldTree:TItem<L1>, newTree:TItem<L2>, mergeable:(i1:TItem<TItemLocation>, i2:TItem<TItemLocation>)=>boolean, hashSettings: IHashSettings, checkHashes = true, hasCache = true) {
31+
constructor(
32+
mappings: Mappings,
33+
oldTree:TItem<L1>,
34+
newTree:TItem<L2>,
35+
mergeable:(i1:TItem<TItemLocation>, i2:TItem<TItemLocation>)=>boolean,
36+
hashSettings: IHashSettings,
37+
checkHashes = true,
38+
hasCache = true,
39+
createMappings = true,
40+
) {
41+
this.mappings = mappings
2842
this.oldTree = oldTree
2943
this.newTree = newTree
3044
this.mergeable = mergeable
3145
this.hashSettings = hashSettings
3246
this.checkHashes = typeof checkHashes === 'undefined' ? true : checkHashes
3347
this.hasCache = hasCache
48+
this.createMappings = createMappings
3449
this.result = {
3550
CREATE: new Diff(),
3651
UPDATE: new Diff(),
@@ -46,15 +61,22 @@ export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation>
4661

4762
async run():Promise<ScanResult<L2, L1>> {
4863
await this.diffItem(this.oldTree, this.newTree)
64+
Logger.log('Diff before findMoves', this.result)
4965
await this.findMoves()
5066
await this.addReorders()
5167
return this.result
5268
}
5369

5470
async diffItem(oldItem:TItem<L1>, newItem:TItem<L2>):Promise<void> {
5571
if (oldItem.type === 'folder' && newItem.type === 'folder') {
72+
if (this.createMappings) {
73+
await this.addMapping(oldItem, newItem)
74+
}
5675
return this.diffFolder(oldItem, newItem)
5776
} else if (oldItem.type === 'bookmark' && newItem.type === 'bookmark') {
77+
if (this.createMappings) {
78+
await this.addMapping(oldItem, newItem)
79+
}
5880
return this.diffBookmark(oldItem, newItem)
5981
} else {
6082
throw new Error('Mismatched diff items: ' + oldItem.type + ', ' + newItem.type)
@@ -182,7 +204,7 @@ export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation>
182204
this.mergeable(removedItem, createdItem) &&
183205
(removedItem.type !== 'folder' ||
184206
(!this.hasCache &&
185-
removedItem.childrenSimilarity(createdItem) >= 0.5))
207+
removedItem.childrenSimilarity(createdItem) >= 0.8))
186208
) {
187209
this.result.CREATE.retract(createAction)
188210
this.result.REMOVE.retract(removeAction)
@@ -292,6 +314,30 @@ export default class Scanner<L1 extends TItemLocation, L2 extends TItemLocation>
292314
})
293315
}
294316

317+
async addMapping(oldItem: TItem<L1>, newItem: TItem<L2>): Promise<void> {
318+
let localId, remoteId
319+
if (
320+
newItem.location === ItemLocation.SERVER &&
321+
oldItem.location === ItemLocation.LOCAL
322+
) {
323+
localId = oldItem.id
324+
remoteId = newItem.id
325+
} else if (
326+
oldItem.location === ItemLocation.SERVER &&
327+
newItem.location === ItemLocation.LOCAL
328+
) {
329+
localId = newItem.id
330+
remoteId = oldItem.id
331+
} else {
332+
return
333+
}
334+
if (newItem.type === 'folder') {
335+
await this.mappings.addFolder({ localId, remoteId })
336+
} else {
337+
await this.mappings.addBookmark({ localId, remoteId })
338+
}
339+
}
340+
295341
async addReorders(): Promise<void> {
296342
Logger.log('Scanner: Generate reorders')
297343
const targets = {}

‎src/lib/strategies/Default.ts‎

Lines changed: 35 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -626,12 +626,11 @@ export default class SyncProcess {
626626
async getDiffs():Promise<{localScanResult:ScanResult<typeof ItemLocation.LOCAL, TItemLocation>, serverScanResult:ScanResult<typeof ItemLocation.SERVER, TItemLocation>}> {
627627
const mappingsSnapshot = this.mappings.getSnapshot()
628628

629-
const newMappings = []
630-
631629
const isUsingTabs = await this.localTree.isUsingBrowserTabs?.()
632630

633631
// Since we have the cache available, Diff cache and both trees..
634632
const localScanner = new Scanner(
633+
this.mappings,
635634
this.cacheTreeRoot,
636635
this.localTreeRoot,
637636
(oldItem, newItem) => {
@@ -656,8 +655,11 @@ export default class SyncProcess {
656655
return false
657656
},
658657
this.hashSettings,
658+
true,
659+
false,
659660
)
660661
const serverScanner = new Scanner(
662+
this.mappings,
661663
this.cacheTreeRoot,
662664
this.serverTreeRoot,
663665
(oldItem, newItem) => {
@@ -671,30 +673,28 @@ export default class SyncProcess {
671673

672674
// The two are mappable, no-brainer...
673675
if (Mappings.mappable(mappingsSnapshot, oldItem, newItem)) {
674-
newMappings.push([oldItem, newItem])
675676
return true
676677
}
677678

678679
// We also allow canMergeWith here for bookmarks, because e.g. for NextcloudFolders the id of moved bookmarks changes (because their id is "<bookmarkID>;<folderId>")
679680
if (oldItem.type === 'bookmark' && newItem.type === 'bookmark' && oldItem.canMergeWith(newItem)) {
680-
newMappings.push([oldItem, newItem])
681681
return true
682682
}
683683

684684
// We also allow canMergeWith here for folders, if we're dealing with tabs, because Window IDs are not stable
685685
if (isUsingTabs && oldItem.type === 'folder' && newItem.type === 'folder' && oldItem.canMergeWith(newItem)) {
686-
newMappings.push([oldItem, newItem])
687686
return true
688687
}
689688

690689
return false
691690
},
692691
this.hashSettings,
692+
true,
693+
true,
693694
)
694695
Logger.log('Calculating diffs for local and server trees relative to cache tree')
695696
const localScanResult = await localScanner.run()
696697
const serverScanResult = await serverScanner.run()
697-
await Parallel.map(newMappings, ([localItem, serverItem]) => this.addMapping(this.server, localItem, serverItem.id), 1)
698698
return {localScanResult, serverScanResult}
699699
}
700700

@@ -765,26 +765,32 @@ export default class SyncProcess {
765765
))
766766
if (concurrentCreation) {
767767
// created on both the target and sourcely, try to reconcile
768-
const newMappings = []
769768
const subScanner = new Scanner(
769+
this.mappings,
770770
concurrentCreation.payload, // target tree
771771
action.payload, // source tree
772772
(oldItem, newItem) => {
773-
if (oldItem.type === newItem.type && oldItem.canMergeWith(newItem)) {
774-
// if two items can be merged, we'll add mappings here directly
775-
newMappings.push([oldItem, newItem.id])
773+
if (
774+
oldItem.type === newItem.type &&
775+
oldItem.canMergeWith(newItem)
776+
) {
776777
return true
777778
}
778779
return false
779780
},
780781
this.hashSettings,
781-
false
782+
false,
783+
true,
784+
true,
782785
)
783786
const scanResult = await subScanner.run()
784-
newMappings.push([concurrentCreation.payload, action.payload.id])
785-
await Parallel.each(newMappings, async([oldItem, newId]) => {
786-
await this.addMapping(action.payload.location === ItemLocation.LOCAL ? this.localTree : this.server, oldItem, newId)
787-
},1)
787+
await this.addMapping(
788+
action.payload.location === ItemLocation.LOCAL
789+
? this.localTree
790+
: this.server,
791+
concurrentCreation.payload,
792+
action.payload.id
793+
)
788794

789795
// SubScanner may reveal residual CREATE/REMOVE actions that we add to the plan here
790796
// We do not act on REMOVEs, only on CREATEs as we merge the two sides
@@ -1098,25 +1104,25 @@ export default class SyncProcess {
10981104
// Try bulk import with sub folders
10991105
const imported = await resource.bulkImportFolder(id, action.oldItem.copyWithLocation(false, action.payload.location)) as Folder<typeof targetLocation>
11001106
await done()
1101-
const newMappings = []
11021107
const subScanner = new Scanner(
1108+
this.mappings,
11031109
action.oldItem,
11041110
imported,
11051111
(oldItem, newItem) => {
1106-
if (oldItem.type === newItem.type && oldItem.canMergeWith(newItem)) {
1107-
// if two items can be merged, we'll add mappings here directly
1108-
newMappings.push([oldItem, newItem.id])
1112+
if (
1113+
oldItem.type === newItem.type &&
1114+
oldItem.canMergeWith(newItem)
1115+
) {
11091116
return true
11101117
}
11111118
return false
11121119
},
11131120
this.hashSettings,
11141121
false,
1122+
true,
1123+
true,
11151124
)
11161125
await subScanner.run()
1117-
await Parallel.each(newMappings, async([oldItem, newId]: [TItem<TItemLocation>, string|number]) => {
1118-
await this.addMapping(resource, oldItem, newId)
1119-
}, 10)
11201126

11211127
if ('orderFolder' in resource) {
11221128
const mappingsSnapshot = this.mappings.getSnapshot()
@@ -1157,25 +1163,26 @@ export default class SyncProcess {
11571163
Logger.log('Attempting chunked bulk import')
11581164
tempItem.children = bookmarks.splice(0, 70)
11591165
const imported = await resource.bulkImportFolder(action.payload.id, tempItem)
1160-
const newMappings = []
11611166
const subScanner = new Scanner(
1167+
this.mappings,
11621168
tempItem,
11631169
imported,
11641170
(oldItem, newItem) => {
1165-
if (oldItem.type === newItem.type && oldItem.canMergeWith(newItem)) {
1171+
if (
1172+
oldItem.type === newItem.type &&
1173+
oldItem.canMergeWith(newItem)
1174+
) {
11661175
// if two items can be merged, we'll add mappings here directly
1167-
newMappings.push([oldItem, newItem.id])
11681176
return true
11691177
}
11701178
return false
11711179
},
11721180
this.hashSettings,
11731181
false,
1182+
true,
1183+
true,
11741184
)
11751185
await subScanner.run()
1176-
await Parallel.each(newMappings, async([oldItem, newId]: [TItem<TItemLocation>, string|number]) => {
1177-
await this.addMapping(resource, oldItem, newId)
1178-
}, 10)
11791186
}
11801187

11811188
// create sub plan for the folders

0 commit comments

Comments
 (0)