Use history recording and don't do anything permanent (#915)

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])

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

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

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"