From 039d92ce78cf0f927508e7619978554797099474 Mon Sep 17 00:00:00 2001 From: Lucien Greathouse Date: Mon, 16 Nov 2020 11:44:56 -0800 Subject: [PATCH] plugin: Support ClassName changes in applyPatch --- plugin/src/Reconciler/applyPatch.lua | 78 +++++++++++++++++++++-- plugin/src/Reconciler/applyPatch.spec.lua | 39 ++++++++++++ 2 files changed, 113 insertions(+), 4 deletions(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index f806013c..352ed48f 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -73,7 +73,7 @@ local function applyPatch(instanceMap, patch) if instance == nil then -- We can't update an instance that doesn't exist. - -- TODO: Should this be an invariant? + table.insert(unappliedPatch.updated, update) continue end @@ -84,10 +84,80 @@ local function applyPatch(instanceMap, patch) } local partiallyApplied = false + -- If the instance's className changed, we have a bumpy ride ahead while + -- we recreate this instance and move all of its children into the new + -- copy atomically...ish. if update.changedClassName ~= nil then - -- TODO: Support changing class name by destroying + recreating. - unappliedUpdate.changedClassName = update.changedClassName - partiallyApplied = true + -- If the instance's name also changed, we'll do it here, since this + -- branch will skip the rest of the loop iteration. + local newName = update.changedName or instance.Name + + -- TODO: When changing between instances that have similar sets of + -- properties, like between an ImageLabel and an ImageButton, we + -- should preserve all of the properties that are shared between the + -- two classes unless they're changed as part of this patch. This is + -- similar to how "class changer" Studio plugins work. + -- + -- For now, we'll only apply properties that are mentioned in this + -- update. Patches with changedClassName set only occur in specific + -- circumstances, usually between Folder and ModuleScript instances. + -- While this may result in some issues, like not preserving the + -- "Archived" property, a robust solution is sufficiently + -- complicated that we're pushing it off for now. + local newProperties = update.changedProperties + + -- If the instance's ClassName changed, we'll kick into reify to + -- create this instance. We'll handle moving all of children between + -- the instances after the new one is created. + local mockVirtualInstance = { + Id = update.id, + Name = newName, + ClassName = update.changedClassName, + Properties = newProperties, + Children = {}, + } + + local mockAdded = { + [update.id] = mockVirtualInstance, + } + + local failedToReify = reify(instanceMap, mockAdded, update.id, instance.Parent) + + local newInstance = instanceMap.fromIds[update.id] + + -- Some parts of reify may have failed, but this is not necessarily + -- critical. If the instance wasn't recreated or has the wrong Name, + -- we'll consider our attempt a failure. + if instance == newInstance or newInstance.Name ~= newName then + table.insert(unappliedPatch.updated, update) + continue + end + + -- Here are the non-critical failures. We know that the instance + -- succeeded in creating and that assigning Name did not fail, but + -- other property assignments might've. + if not PatchSet.isEmpty(failedToReify) then + PatchSet.assign(unappliedPatch, failedToReify) + end + + -- Watch out, this is the scary part! Move all of the children of + -- instance into newInstance. + -- + -- TODO: If this fails part way through, should we move everything + -- back? For now, we assume that moving things will not fail. + for _, child in ipairs(instance:GetChildren()) do + child.Parent = newInstance + end + + -- See you later, original instance. + -- + -- TODO: Can this fail? Some kinds of instance may not appreciate + -- being destroyed, like services. + instance:Destroy() + + -- This completes your rebuilding a plane mid-flight safety + -- instruction. Please sit back, relax, and enjoy your flight. + continue end if update.changedName ~= nil then diff --git a/plugin/src/Reconciler/applyPatch.spec.lua b/plugin/src/Reconciler/applyPatch.spec.lua index 5308079d..28e2a41e 100644 --- a/plugin/src/Reconciler/applyPatch.spec.lua +++ b/plugin/src/Reconciler/applyPatch.spec.lua @@ -156,4 +156,43 @@ return function() assert(PatchSet.isEmpty(unapplied), "expected remaining patch to be empty") expect(value.Value).to.equal("WORLD") end) + + it("should recreate instances when changedClassName is set, preserving children", function() + local root = Instance.new("Folder") + root.Name = "Initial Root Name" + + local child = Instance.new("Folder") + child.Name = "Child" + child.Parent = root + + local instanceMap = InstanceMap.new() + instanceMap:insert("ROOT", root) + instanceMap:insert("CHILD", child) + + local patch = PatchSet.newEmpty() + table.insert(patch.updated, { + id = "ROOT", + changedName = "Updated Root Name", + changedClassName = "StringValue", + changedProperties = { + Value = { + Type = "String", + Value = "I am Root", + }, + }, + }) + + local unapplied = applyPatch(instanceMap, patch) + assert(PatchSet.isEmpty(unapplied), "expected remaining patch to be empty") + + local newRoot = instanceMap.fromIds["ROOT"] + assert(newRoot ~= root, "expected instance to be recreated") + expect(newRoot.ClassName).to.equal("StringValue") + expect(newRoot.Name).to.equal("Updated Root Name") + expect(newRoot.Value).to.equal("I am Root") + + local newChild = newRoot:FindFirstChild("Child") + assert(newChild ~= nil, "expected child to be present") + assert(newChild == child, "expected child to be preserved") + end) end \ No newline at end of file