From 2855d2a402642d22eef323f402cfd3192d011354 Mon Sep 17 00:00:00 2001 From: Scott Erickson Date: Thu, 3 Jul 2014 17:41:34 -0700 Subject: [PATCH] Made the deltas lib conflict identifying library able to handle many-to-many conflicts. Made conflict finding a bit more liberal, in that any messing with arrays (adding, removing or moving things inside) will conflict with any other such change. --- app/lib/deltas.coffee | 64 ++++++++++++++--------------- app/models/CocoModel.coffee | 4 +- app/views/editor/level/edit.coffee | 2 +- app/views/editor/patch_modal.coffee | 1 + test/app/lib/deltas.spec.coffee | 18 ++++++++ 5 files changed, 53 insertions(+), 36 deletions(-) create mode 100644 test/app/lib/deltas.spec.coffee diff --git a/app/lib/deltas.coffee b/app/lib/deltas.coffee index 4ec639532..8ff4d5dd2 100644 --- a/app/lib/deltas.coffee +++ b/app/lib/deltas.coffee @@ -93,7 +93,7 @@ module.exports.makeJSONDiffer = -> jsondiffpatch.create({objectHash:hasher}) module.exports.getConflicts = (headDeltas, pendingDeltas) -> - # headDeltas and pendingDeltas should be lists of deltas returned by interpretDelta + # headDeltas and pendingDeltas should be lists of deltas returned by expandDelta # Returns a list of conflict objects with properties: # headDelta # pendingDelta @@ -105,20 +105,33 @@ module.exports.getConflicts = (headDeltas, pendingDeltas) -> # Here's my thinking: conflicts happen when one delta path is a substring of another delta path # So, sort paths from both deltas together, which will naturally make conflicts adjacent, - # and if one is identified, one path is from the headDeltas, the other is from pendingDeltas + # and if one is identified AND one path is from the headDeltas AND the other is from pendingDeltas # This is all to avoid an O(nm) brute force search. conflicts = [] paths.sort() for path, i in paths - continue if i + 1 is paths.length - nextPath = paths[i+1] - if nextPath.startsWith path - headDelta = (headPathMap[path] or headPathMap[nextPath])[0].delta - pendingDelta = (pendingPathMap[path] or pendingPathMap[nextPath])[0].delta - conflicts.push({headDelta:headDelta, pendingDelta:pendingDelta}) - pendingDelta.conflict = headDelta - headDelta.conflict = pendingDelta + offset = 1 + while i + offset < paths.length + # Look at the neighbor + nextPath = paths[i+offset] + offset += 1 + + # these stop being substrings of each other? Then conflict DNE + if not (nextPath.startsWith path) then break + + # check if these two are from the same group, but we still need to check for more beyond + unless headPathMap[path] or headPathMap[nextPath] then continue + unless pendingPathMap[path] or pendingPathMap[nextPath] then continue + + # Okay, we found two deltas from different groups which conflict + for headMetaDelta in (headPathMap[path] or headPathMap[nextPath]) + headDelta = headMetaDelta.delta + for pendingMetaDelta in (pendingPathMap[path] or pendingPathMap[nextPath]) + pendingDelta = pendingMetaDelta.delta + conflicts.push({headDelta:headDelta, pendingDelta:pendingDelta}) + pendingDelta.conflict = headDelta + headDelta.conflict = pendingDelta return conflicts if conflicts.length @@ -126,12 +139,13 @@ groupDeltasByAffectingPaths = (deltas) -> metaDeltas = [] for delta in deltas conflictPaths = [] + # We're being fairly liberal with what's a conflict, because the alternative is worse if delta.action is 'moved-index' - # every other action affects just the data path, but moved indexes affect a swath - indices = [delta.originalIndex, delta.destinationIndex] - indices.sort() - for index in _.range(indices[0], indices[1]+1) - conflictPaths.push delta.dataPath.slice(0, delta.dataPath.length-1).concat(index) + # If you moved items around in an array, mark the whole array as a gonner + conflictPaths.push delta.dataPath.slice(0, delta.dataPath.length-1) + else if delta.action in ['deleted', 'added'] and _.isNumber(delta.dataPath[delta.dataPath.length-1]) + # If you remove or add items in an array, mark the whole thing as a gonner + conflictPaths.push delta.dataPath.slice(0, delta.dataPath.length-1) else conflictPaths.push delta.dataPath for path in conflictPaths @@ -141,25 +155,7 @@ groupDeltasByAffectingPaths = (deltas) -> } map = _.groupBy metaDeltas, 'path' - - # Turns out there are cases where a single delta can include paths - # that 'conflict' with each other, ie one is a substring of the other - # because of moved indices. To handle this case, go through and prune - # out all deeper paths that conflict with more shallow paths, so - # getConflicts path checking works properly. - - paths = _.keys(map) - return map unless paths.length - paths.sort() - prunedMap = {} - previousPath = paths[0] - for path, i in paths - continue if i is 0 - continue if path.startsWith previousPath - prunedMap[path] = map[path] - previousPath = path - - prunedMap + return map module.exports.pruneConflictsFromDelta = (delta, conflicts) -> expandedDeltas = (conflict.pendingDelta for conflict in conflicts) diff --git a/app/models/CocoModel.coffee b/app/models/CocoModel.coffee index 7c4a2f1a1..e28e8f1ca 100644 --- a/app/models/CocoModel.coffee +++ b/app/models/CocoModel.coffee @@ -58,7 +58,9 @@ class CocoModel extends Backbone.Model @set(existing, {silent:true}) CocoModel.backedUp[@id] = @ - saveBackup: -> + saveBackup: -> @saveBackupNow() + + saveBackupNow: -> storage.save(@id, @attributes) CocoModel.backedUp[@id] = @ diff --git a/app/views/editor/level/edit.coffee b/app/views/editor/level/edit.coffee index 1b19e68b2..ab63f26b4 100644 --- a/app/views/editor/level/edit.coffee +++ b/app/views/editor/level/edit.coffee @@ -73,7 +73,7 @@ module.exports = class EditorLevelView extends View Backbone.Mediator.publish 'level-loaded', level: @level @showReadOnly() if me.get('anonymous') @patchesView = @insertSubView(new PatchesView(@level), @$el.find('.patches-view')) - @listenTo @patchesView, 'accepted-patch', -> setTimeout "location.reload()", 400 + @listenTo @patchesView, 'accepted-patch', -> location.reload() @$el.find('#level-watch-button').find('> span').toggleClass('secret') if @level.watching() onPlayLevel: (e) -> diff --git a/app/views/editor/patch_modal.coffee b/app/views/editor/patch_modal.coffee index debd20231..c1dd6395e 100644 --- a/app/views/editor/patch_modal.coffee +++ b/app/views/editor/patch_modal.coffee @@ -57,6 +57,7 @@ module.exports = class PatchModal extends ModalView acceptPatch: -> delta = @deltaView.getApplicableDelta() @targetModel.applyDelta(delta) + @targetModel.saveBackupNow() @patch.setStatus('accepted') @trigger 'accepted-patch' @hide() diff --git a/test/app/lib/deltas.spec.coffee b/test/app/lib/deltas.spec.coffee new file mode 100644 index 000000000..e658f3869 --- /dev/null +++ b/test/app/lib/deltas.spec.coffee @@ -0,0 +1,18 @@ +deltas = require 'lib/deltas' + +describe 'deltas lib', -> + + describe 'getConflicts', -> + + it 'handles conflicts where one change conflicts with several changes', -> + originalData = {list:[1,2,3]} + forkA = {list:['1', 2, '3']} + forkB = {noList: '...'} + differ = deltas.makeJSONDiffer() + + expandedDeltaA = deltas.expandDelta(differ.diff originalData, forkA) + expandedDeltaB = deltas.expandDelta(differ.diff originalData, forkB) + deltas.getConflicts(expandedDeltaA, expandedDeltaB) + for delta in expandedDeltaA + expect(delta.conflict).toBeDefined() + \ No newline at end of file