Fix deleting active combo

[OverHash]

Closes #88

Took me a while to understand the way Iris renders the combo, but I'm fairly happy with this implementation. It solves the original issue and doesn't seem to cause problems for other combos.

I tested it with this script:

local ReplicatedStorage = game:GetService("ReplicatedStorage")

local Iris = require(ReplicatedStorage.Iris)

local Iris = Iris.Init()
Iris:Connect(function()
	local hasSelectedYet = Iris.State(nil)

	Iris.Window() do
		if not hasSelectedYet:get() then
			Iris.Combo({"My nice combo"}) do
				Iris.Selectable({"Select me", true}, { index = hasSelectedYet })
			end

			Iris.End()
		end
		
		Iris.ComboArray({"This one always shows"}, {}, {"first", "second", "third"}) do
			
		end

		Iris.End()
	end
end)

and the behavior is correct:

2025-01-04.13-59-27.mp4

my main concern is that the ComboContainer instance still exists under PlayerGui.PopupScreenGui. It looks like Iris actually creates one of these per Combo. Ideally this instance gets destroyed -- we currently just make it invisible. I'm not sure what the consequences are of deleting it -- this is something I think should be discussed before merging.

[OverHash]

This is the ComboContainer before I stop rendering the active combo. We can see the Iris_Selectable is the child component.
Once I stop rendering the Combo, the Iris_Selectable (the Selectable widget) has the Discard triggered, and the ComboContainer is now set to Visible = false (with this PR). But I think it should be destroyed.

I looked in the source code and couldn't see anywhere where thisWidget.ChildContainer is destroyed -- is this an oversight and potential memory leak issue?

My main concern with destroying the thisWidget.ChildContainer was that it would break the Discard function of other widgets.

[OverHash]

I just checked and it looks like Internal._DiscardWidget is doing it in the correct order: discarding the children first, then the parent.

Is that behavior guaranteed? If so, we should just be able to delete the thisWidget.ChildContainer inside the Combo widget Discard function.
I went looking and found this in the source:

Iris/lib/Internal.lua

Lines 217 to 223 in 46b26d4

	for _, widget: Types.Widget in Internal._lastVDOM do
	if widget.lastCycleTick ~= Internal._cycleTick and (widget.lastCycleTick ~= -1) then
	-- a widget which used to be rendered was not called last frame, so we discard it.
	-- if the cycle tick is -1 we have already discarded it.
	Internal._DiscardWidget(widget)
	end
	end

which looks to be triggering the discard function. Unfortunately, we insert into the lastVdom via

Iris/lib/Internal.lua

Line 508 in 46b26d4

Internal._VDOM[ID] = thisWidget

which makes it a k/v table, and so I believe we default to the generalized table iteration iteration semantics:

First, the traversal goes over numeric keys in range 1..k up until reaching the first k such that t[k] == nil
Then, the traversal goes over the remaining keys (with non-nil values), numeric and otherwise, in unspecified order.

unfortunately I think this means we don't have a good way (right now) to delete the ChildContainer instance, because sometimes we might delete the parent before we destroy the child. I think we need some way to run some code that will discard the widget after all children have been discarded. The alternative would be to have some code run in both Discard and ChildDiscarded that destroys the ChildContainer once there is zero children remaining.

[SirMallard]

I see the issue. I discovered something similar before, where it would attempt to destroy a widget twice. That's why I added the widget.lastCycleTick ~= -1 part, to ensure that we don't delete it twice. However, this doesn't fix the issue here.

I think you are right in stating that it was an oversight and a memory leak to not delete the ChildContainer, meaning we need to be more careful with deleteing it.

[SirMallard]

In terms of fixing this, the Discard function does not clean up events because all events are tied to the Instances. Therefore, destroying a parent happily cleans up any connections. And apart from Root, Window and Table widgets, the Discard function only destroys the instances and releases any state objects.

Therefore, I think it is safe to continue deleting widgets in any order and update every Discard function to check the Instance and ChildContainer still exist (haven't been destroyed by a parent) before calling :Destroy().

[SirMallard]

I've added some extra checks and updated all of the ChildContainers, or differently-parented widgets, to properly destroy them.

One thing that is bothering me is that we've never had to check before that a widget's instance still exists before deleting it. Logically, if we delete a parent and its instances, and then attempt to delete the child instance, it will error. Or is that just because it still has a reference to the Instance and Roblox allows us to call :Destroy() as many times as we like on an Instance?

[SirMallard]

14c0fa3

I've reverted the need to check for the Instance and ChildContainer, since they will still exist in memory and we don't modify them directly.

But I've noticed a larger issue about discarding certain widgets which share a state. Currently, if a Menu or Tab is discarded, it won't update any others, even thought they work together. I've added extra checks which should properly update any others. However, I believe this introduces errors, since if we discard a parent and then destroy each child, the cleanup functions will assume that any siblings still exist, and therefore will attempt to use their instances, which have been destroyed. This is due to the state object updating any other widgets.

Best way to solve this is to check within the state that the widget has not been destroyed.