SiliconStudio / xenko

Old repo for Xenko Game Engine. Please use https://github.com/xenko3d/xenko instead.
http://xenko.com
1.54k stars 346 forks source link

TrackingCollectionChangedEventArgs Generics #664

Open stefnotch opened 6 years ago

stefnotch commented 6 years ago

Is there a reason why TrackingCollectionChangedEventArgs isn't a generic class? TrackingHashSet<T> and other classes are generic, so, it seems like it would make sense to make the events generic as well. As in:

public class TrackingCollectionChangedEventArgs<T> : EventArgs
{
    public TrackingCollectionChangedEventArgs(NotifyCollectionChangedAction action, T item, T oldItem, int index, bool collectionChanged);
    public TrackingCollectionChangedEventArgs(NotifyCollectionChangedAction action, object key, T item, T oldItem, bool collectionChanged);
    public NotifyCollectionChangedAction Action { get; }
    public T Item { get; }
    public T OldItem { get; }
    public object Key { get; }
    public int Index { get; }
    public bool CollectionChanged { get; }
}
Kryptos-FR commented 6 years ago

How would you register a handler for any kind of collection change? If it was generic you could only register to a specific type. So you would need to know in advance which types will be used in collections and have one handler per type, hardly a workable solution.

stefnotch commented 6 years ago

Here are a bunch of cases where TrackingCollectionChangedEventArgs gets used: https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/common/core/SiliconStudio.Core/Collections/TrackingCollection.cs#L21 ^Not an issue, because TrackingCollection is a generic class. . https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/samples/Templates/TopDownRPG/TopDownRPG/TopDownRPG.Game/Gameplay/EnableDynamicNavigation.cs#L30 ^Not an issue either, because the programmer clearly knows which types he is expecting. . https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/common/core/SiliconStudio.Core/Collections/TrackingHashSet.cs#L25 ^Not an issue, since TrackingHashSet is a generic class. . https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/common/core/SiliconStudio.Core/Collections/TrackingDictionary.cs#L27 ^TrackingDictionary is generic. And this is probably not an issue either. . https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/engine/SiliconStudio.Xenko.Input/VirtualButton/VirtualButtonConfig.cs#L23 ^VirtualButtonConfig : TrackingCollection<VirtualButtonBinding> The type is known . https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/engine/SiliconStudio.Xenko.Engine/Engine/Scene.cs#L115 ^The type is also known here . https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/engine/SiliconStudio.Xenko.UI/Panels/Panel.cs#L117 ^The type is known . https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/common/core/SiliconStudio.Core/Collections/ITrackingCollectionChanged.cs#L7 ^The interface ITrackingCollectionChanged can easily be turned into a generic one . .

https://github.com/SiliconStudio/xenko/blob/6fca455a2d67f6f53fc1c7ad8c3722938b3b9a56/sources/scripts/ScriptShader/Effects/Deprecated/ParticleProcessor.cs#L69 ^This might be an issue. However, if I'm not mistaken, all of those cases can be worked around by having some code that ensures backwards compatibility:

class TrackingCollectionChangedEventArgs<T> 
{
    public static implicit operator TrackingCollectionChangedEventArgs(TrackingCollectionChangedEventArgs<T> stuff)
    {
        return (X<object>)stuff;
    }
}

class TrackingCollectionChangedEventArgs : TrackingCollectionChangedEventArgs<object>
{

}

Let me know if you find a case that wouldn't work. Or if I made a mistake somewhere.