From 62f4a1f3c2da7fe9f8927d26b5304ac37a4283d1 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Thu, 30 May 2024 12:28:58 -0700 Subject: [PATCH] Use history recording and don't do anything permanent (#915) --- CHANGELOG.md | 2 + plugin/src/InstanceMap.lua | 7 +++- plugin/src/Reconciler/applyPatch.lua | 22 +++++++++-- plugin/src/Reconciler/applyPatch.spec.lua | 45 +++++++++++++++++------ 4 files changed, 59 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05550ae2..79abf800 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # Rojo Changelog ## Unreleased Changes +* Updated Undo/Redo history to be more robust ([#915]) * Fixed removing trailing newlines ([#903]) * Added Never option to Confirmation ([#893]) * Added popout diff visualizer for table properties like Attributes and Tags ([#834]) @@ -65,6 +66,7 @@ [#893]: https://github.com/rojo-rbx/rojo/pull/893 [#903]: https://github.com/rojo-rbx/rojo/pull/903 [#911]: https://github.com/rojo-rbx/rojo/pull/911 +[#915]: https://github.com/rojo-rbx/rojo/pull/915 ## [7.4.1] - February 20, 2024 * Made the `name` field optional on project files ([#870]) diff --git a/plugin/src/InstanceMap.lua b/plugin/src/InstanceMap.lua index b035d423..76754fef 100644 --- a/plugin/src/InstanceMap.lua +++ b/plugin/src/InstanceMap.lua @@ -112,9 +112,12 @@ end function InstanceMap:destroyInstance(instance) local id = self.fromInstances[instance] - local descendants = instance:GetDescendants() - instance:Destroy() + + -- Because the user might want to Undo this change, we cannot use Destroy + -- since that locks that parent and prevents ChangeHistoryService from + -- ever bringing it back. Instead, we parent to nil. + instance.Parent = nil -- After the instance is successfully destroyed, -- we can remove all the id mappings diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 5ee6ed5e..05e7412a 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -20,6 +20,11 @@ local setProperty = require(script.Parent.setProperty) local function applyPatch(instanceMap, patch) local patchTimestamp = DateTime.now():FormatLocalTime("LTS", "en-us") + local historyRecording = ChangeHistoryService:TryBeginRecording("Rojo: Patch " .. patchTimestamp) + if not historyRecording then + -- There can only be one recording at a time + Log.debug("Failed to begin history recording for " .. patchTimestamp .. ". Another recording is in progress.") + end -- Tracks any portions of the patch that could not be applied to the DOM. local unappliedPatch = PatchSet.newEmpty() @@ -62,6 +67,9 @@ local function applyPatch(instanceMap, patch) if parentInstance == nil then -- This would be peculiar. If you create an instance with no -- parent, were you supposed to create it at all? + if historyRecording then + ChangeHistoryService:FinishRecording(historyRecording, Enum.FinishRecordingOperation.Commit) + end invariant( "Cannot add an instance from a patch that has no parent.\nInstance {} with parent {}.\nState: {:#?}", id, @@ -164,10 +172,14 @@ local function applyPatch(instanceMap, patch) end -- See you later, original instance. - -- + + -- Because the user might want to Undo this change, we cannot use Destroy + -- since that locks that parent and prevents ChangeHistoryService from + -- ever bringing it back. Instead, we parent to nil. + -- TODO: Can this fail? Some kinds of instance may not appreciate - -- being destroyed, like services. - instance:Destroy() + -- being reparented, like services. + instance.Parent = nil -- This completes your rebuilding a plane mid-flight safety -- instruction. Please sit back, relax, and enjoy your flight. @@ -214,7 +226,9 @@ local function applyPatch(instanceMap, patch) end end - ChangeHistoryService:SetWaypoint("Rojo: Patch " .. patchTimestamp) + if historyRecording then + ChangeHistoryService:FinishRecording(historyRecording, Enum.FinishRecordingOperation.Commit) + end return unappliedPatch end diff --git a/plugin/src/Reconciler/applyPatch.spec.lua b/plugin/src/Reconciler/applyPatch.spec.lua index c561db75..b44e3eda 100644 --- a/plugin/src/Reconciler/applyPatch.spec.lua +++ b/plugin/src/Reconciler/applyPatch.spec.lua @@ -4,25 +4,41 @@ return function() local InstanceMap = require(script.Parent.Parent.InstanceMap) local PatchSet = require(script.Parent.Parent.PatchSet) - local dummy = Instance.new("Folder") - local function wasDestroyed(instance) + local container = Instance.new("Folder") + + local tempContainer = Instance.new("Folder") + local function wasRemoved(instance) -- If an instance was destroyed, its parent property is locked. - local ok = pcall(function() + -- If an instance was removed, its parent property is nil. + -- We need to ensure we only remove, so that ChangeHistoryService can still Undo. + + local isParentUnlocked = pcall(function() local oldParent = instance.Parent - instance.Parent = dummy + instance.Parent = tempContainer instance.Parent = oldParent end) - return not ok + return instance.Parent == nil and isParentUnlocked end + beforeEach(function() + container:ClearAllChildren() + end) + + afterAll(function() + container:Destroy() + tempContainer:Destroy() + end) + it("should return an empty patch if given an empty patch", function() local patch = applyPatch(InstanceMap.new(), PatchSet.newEmpty()) assert(PatchSet.isEmpty(patch), "expected remaining patch to be empty") end) - it("should destroy instances listed for remove", function() + it("should remove instances listed for remove", function() local root = Instance.new("Folder") + root.Name = "ROOT" + root.Parent = container local child = Instance.new("Folder") child.Name = "Child" @@ -38,14 +54,16 @@ return function() local unapplied = applyPatch(instanceMap, patch) assert(PatchSet.isEmpty(unapplied), "expected remaining patch to be empty") - assert(not wasDestroyed(root), "expected root to be left alone") - assert(wasDestroyed(child), "expected child to be destroyed") + assert(not wasRemoved(root), "expected root to be left alone") + assert(wasRemoved(child), "expected child to be removed") instanceMap:stop() end) - it("should destroy IDs listed for remove", function() + it("should remove IDs listed for remove", function() local root = Instance.new("Folder") + root.Name = "ROOT" + root.Parent = container local child = Instance.new("Folder") child.Name = "Child" @@ -62,8 +80,8 @@ return function() assert(PatchSet.isEmpty(unapplied), "expected remaining patch to be empty") expect(instanceMap:size()).to.equal(1) - assert(not wasDestroyed(root), "expected root to be left alone") - assert(wasDestroyed(child), "expected child to be destroyed") + assert(not wasRemoved(root), "expected root to be left alone") + assert(wasRemoved(child), "expected child to be removed") instanceMap:stop() end) @@ -73,6 +91,8 @@ return function() -- tests on reify, not here. local root = Instance.new("Folder") + root.Name = "ROOT" + root.Parent = container local instanceMap = InstanceMap.new() instanceMap:insert("ROOT", root) @@ -113,6 +133,8 @@ return function() it("should return unapplied additions when instances cannot be created", function() local root = Instance.new("Folder") + root.Name = "ROOT" + root.Parent = container local instanceMap = InstanceMap.new() instanceMap:insert("ROOT", root) @@ -159,6 +181,7 @@ return function() it("should recreate instances when changedClassName is set, preserving children", function() local root = Instance.new("Folder") root.Name = "Initial Root Name" + root.Parent = container local child = Instance.new("Folder") child.Name = "Child"