Control collection parent fixes

[eksime]

Discord Discussion

Breaking change: no

Likely fixes #886

This reworks the internals of the ControlCollection<T> class, with the following changes:

• null values are unable to be added to the collection, with short-circuits for methods when item is null
• Cancelling an AddChild or RemoveChild action from a Container should no longer cause the Parent property of the control in question to be in a potentially invalid state.
• Create a copy of the internal array when iterating, as with some BCL concurrent types, as we can't be sure external code will dispose of the enumerator correctly.
• Provide (internal) ReaderWriterLockSlim helper methods that allow basic read and write locks to use using (...) { ... } syntax rather than having to use try/finally everywhere.
• No longer clear Parent on controls when calling Clear - this should be handled by the Container as there's no guarantee that the ControlCollection is being used for parenting/hierarchy purposes.
• Update Container to explicitly set .Parent property on added or removed children.

Notes:

• I'm working on the assumption that the intended behaviour for the control collection is that it never contains nulls and never has duplicates, if either of those assumptions are incorrect then I'll need to rework this change.
• I'm unsure if the indexer setter for ControlCollection<T> should remove the item when attempting to set an indexed position to null or if it should be a noop, currently it removes the item. Additionally, if an index is set to an item that exists elsewhere in the collection, it currently overwrites the index, then removes the duplicate.

[dlamkins]

Gonna do some testing with this build to make sure there are no weird behaviors, but appears to look good and assumptions appear to all be correct.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[eksime]

thinking about it, would it be better to throw ArgumentNullException on operations that involve trying to insert or remove null values?

[dlamkins]

thinking about it, would it be better to throw ArgumentNullException on operations that involve trying to insert or remove null values?

Sorry for the delays on this. Yes, I think you're right. I can't think of a scenario where it would be intended and the short-circuit might mask a module or core bug. Though, like with lists, it may be safe to keep it off of the remove function. Open to your thoughts, though.

The only item that I think we might need to change is:

No longer clear Parent on controls when calling Clear - this should be handled by the Container as there's no guarantee that the ControlCollection is being used for parenting/hierarchy purposes.

This is correct, but it's a breaking change that I can't guarantee won't cause problems within one of the modules. Throwing an exception on inserting/removing null values would also be a breaking change, but seems less likely, and I imagine we would catch it during preview testing.

Open to your thoughts on that as well.