Dangerous RemoveGadget/RemoveWidget usage

[saurtron]

Description

As far as I can see, RemoveGadget, RemoveWidget shouldn't be used unless the callin list is being looped in reverse. check this comment. But this isn't generally done since sometimes we want to loop forward depending on how we want layer priority to behave (I believe). As far as I can see, doing it that way will mean the next element can be skipped.

Currently most of the traversing (90%) is done with ipairs, so it will skip elements.

I checked and some gadgets actually remove themselves in forward looping lists. For example, game_initial_spawn.lua is removing itself inside GameFrame, which is looped forwards with ipairs.

Luckily, most gadgets do this at Initialize, and that's ok, others may be doing it in rare situations, like when gl4 initialization fails. In most cases when this happens the skip of the next element will not be noticed since it can be something minor. Still, I think this should be fixed if I'm right and this is being done incorrectly.

In addition to this, all reordering methods suffer from the same issues, like RaiseGadget, LowerGadget... they can reorder the lists while they are traversed.

• reference: ArrayRemove

Expected Behaviour

Shouldn't skip callins when RemoveGadget is called.

Actual Behaviour

I think it skips elements.

Reproduction steps

This snippet should demonstrate the problem:

arr = {1, 2, 3}
for k, v in ipairs(arr) do
    Spring.Echo("elmt", k, v)
    if k == 1 then
        table.remove(arr, k)
    end
end

it will print:

[t=00:01:25.446803][f=0001991] elmt, 1, 1
[t=00:01:25.446814][f=0001991] elmt, 2, 3

Other

A possible solution is queue the removals until the callin list has been processed, this might have side effects in rare situations, but I think it should be ok.

Another posibility is reviewing everything and make sure no widgets delete themselves in at the "wrong moment".

Other possible solution is removing from tables inside ArrayRemove with t[k] = nil instead of table.remove(t, k), but this only works when iterating with pairs, not with ipairs as is the case here (ipairs is more efficient so I don't think it's a good idea to switch to pairs).

If you have other ideas pls post here :D

[sprunk]

RemoveGadget shouldn't be used unless the callin list is being looped in reverse

Yes, though keep in mind that conversely, looping in reverse prevents InsertGadget in turn.

See also beyond-all-reason/spring#835

[saurtron]

In that case I think adding widgets to a queue and having them removed after the list looping is finished would be the best. Since some lists are looped in reverse, the same should be done for InsertWidget.

[saurtron]

I think RaiseGadget/Widget and LowerGadget/Widget can also cause problems when looping the list so a solution should also take those into account.

[saurtron]

This was already fixed by #3941