While browsing the code, I noticed a memory leak due to a events not being properly unregistered. Before PropertyPath was introduced, PropertyBinding subscripted directly to the top level PropertyChanged event on the binding source (and only a single property name was specified). In this case, PropertyBinding had a direct registration, and unregistered in ClearBindings (which is called from OnDestroy). However, that code was not updated when PropertyPath support was added. What should be happening in ClearBindings is to call ClearHandlers on the source and target PropertyPaths. This will ensure all event handlers are unregistered, and those handlers can be GC'd.
While wrapping my head around this code, I also realized it could be simplified a bit. Specifically, PropertyPath.OnPropertyChanged already filters the PropertyChanged events on the PropertyName, so there is no reason for the final handlers of PropertyPath to also check for this. As such, I changed the final handler from a PropertyChangedEventHandler to just an Action (since it will only be called when the property that changed corresponds to the associated property in the PropertyPath). This also makes it a lot easier to add support for other change triggers (beyond just INotifyPropertyChanged.PropertyChanged, which would be needed for things like polling based view-view bindings).
Lastly, I also updated PropertyBindingEditor to use the DropDownMenu helper. This will also make it easier to add support for additional types of change triggers (beyond UnityEventBase events).
ryantrem
commented
5 years ago
While browsing the code, I noticed a memory leak due to a events not being properly unregistered. Before
PropertyPathwas introduced,PropertyBindingsubscripted directly to the top levelPropertyChangedevent on the binding source (and only a single property name was specified). In this case,PropertyBindinghad a direct registration, and unregistered inClearBindings(which is called fromOnDestroy). However, that code was not updated whenPropertyPathsupport was added. What should be happening inClearBindingsis to callClearHandlerson the source and targetPropertyPaths. This will ensure all event handlers are unregistered, and those handlers can be GC'd.While wrapping my head around this code, I also realized it could be simplified a bit. Specifically,
PropertyPath.OnPropertyChangedalready filters thePropertyChangedevents on thePropertyName, so there is no reason for the final handlers ofPropertyPathto also check for this. As such, I changed the final handler from aPropertyChangedEventHandlerto just anAction(since it will only be called when the property that changed corresponds to the associated property in thePropertyPath). This also makes it a lot easier to add support for other change triggers (beyond justINotifyPropertyChanged.PropertyChanged, which would be needed for things like polling based view-view bindings).Lastly, I also updated
PropertyBindingEditorto use theDropDownMenuhelper. This will also make it easier to add support for additional types of change triggers (beyondUnityEventBaseevents).