From 655d29dfc16dfa104205ab1cdcc2d110223dc6cc Mon Sep 17 00:00:00 2001 From: boatbomber Date: Mon, 13 May 2024 19:48:35 -0700 Subject: [PATCH 1/5] Use history recording --- plugin/src/Reconciler/applyPatch.lua | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 5ee6ed5e4..e026a16da 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -20,6 +20,12 @@ 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.") + return + end -- Tracks any portions of the patch that could not be applied to the DOM. local unappliedPatch = PatchSet.newEmpty() @@ -214,7 +220,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 From bfc3870e7d6854d1c15246defeb22c0f62f26a2b Mon Sep 17 00:00:00 2001 From: boatbomber Date: Mon, 13 May 2024 19:52:45 -0700 Subject: [PATCH 2/5] Update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05550ae26..79abf800d 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]) From 020fe79d41c75b1c3107504c379133e2bb343f74 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Mon, 13 May 2024 20:00:45 -0700 Subject: [PATCH 3/5] Fix permanent destruction during patch application --- plugin/src/Reconciler/applyPatch.lua | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index e026a16da..26e660c49 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -170,10 +170,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 use Remove. + -- TODO: Can this fail? Some kinds of instance may not appreciate - -- being destroyed, like services. - instance:Destroy() + -- being Removed, like services. + instance:Remove() -- This completes your rebuilding a plane mid-flight safety -- instruction. Please sit back, relax, and enjoy your flight. From d018986f1f136182e8ced4f13701c7629703b3b5 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Tue, 14 May 2024 07:41:23 -0700 Subject: [PATCH 4/5] Ensure the recording is stopped even when we error out --- plugin/src/Reconciler/applyPatch.lua | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 26e660c49..9f213737d 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -24,7 +24,6 @@ local function applyPatch(instanceMap, patch) 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.") - return end -- Tracks any portions of the patch that could not be applied to the DOM. @@ -68,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, From 3b7d796cf8c446f4953104264292ee5e8442a316 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Wed, 15 May 2024 23:57:13 -0700 Subject: [PATCH 5/5] Use Parent = nil and update tests to match --- plugin/src/InstanceMap.lua | 7 +++- plugin/src/Reconciler/applyPatch.lua | 6 +-- plugin/src/Reconciler/applyPatch.spec.lua | 45 +++++++++++++++++------ 3 files changed, 42 insertions(+), 16 deletions(-) diff --git a/plugin/src/InstanceMap.lua b/plugin/src/InstanceMap.lua index b035d423c..76754fef5 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 26e660c49..85ba8564e 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -173,11 +173,11 @@ local function applyPatch(instanceMap, patch) -- 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 use Remove. + -- ever bringing it back. Instead, we parent to nil. -- TODO: Can this fail? Some kinds of instance may not appreciate - -- being Removed, like services. - instance:Remove() + -- 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. diff --git a/plugin/src/Reconciler/applyPatch.spec.lua b/plugin/src/Reconciler/applyPatch.spec.lua index c561db751..b44e3eda6 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"