search

sialcasa / mvvmFX

an Application Framework for implementing the MVVM Pattern with JavaFX
Apache License 2.0
494 stars 104 forks source link

[notifications] NotificationCenter internally uses hard references to observers #286

Closed mthiele closed 8 years ago

mthiele commented 9 years ago

When registering an observer to the NotificationCenter, this instance will never be garbage collected, since the NotificationCenter holds a hard reference on the observer even though it might not be referenced anywhere else. The solution is to manually keep track of when which observer is destroyed and the unregister it from the NotificationCenter by hand which is clumsy or live with possible memory leaks. It would also be helpful to have a method to unsubscribe all observers.

manuel-mauky commented 9 years ago

Unsubscribe all observers for a given message key isn't a good idea in my opinion as it leads to a tighter coupling between the sender and the observers. You don't know if there are other observers for a message key that may be still interested in getting messages for a key.

But I've changed the DefaultNotificationCenter to use WeakReferences to hold the observers. I hope that there are no unexpected side effects resulting from this change. Could you please review the code to see if I've missed something? I have the feeling that this change needs a lot of manual testing to be sure no existing apps will break.

manuel-mauky commented 9 years ago

I've reverted the commit as it leads to unexpected behaviour like listeners that aren't notified anymore. Instead of using WeakReferences we will stay with hard references and make it easier for the user to manage subscriptions and make individual unsubscribes. For example RxJava returns a Subscription object that has a unsubscribe method. The user can store the Subscriptions in a Collection and unsubscribe when the component is removed. From mvvmFX perspective we should try to provide livecycle methods that the user can use to react on removal of a View/ViewModel.

Related issue: #251

We will address this issue when we implement the typesafe notificationcenter, see #250

sialcasa commented 8 years ago

Add the possibility to do a weakSubscribe on the NotificationCenter

manuel-mauky commented 8 years ago

With PR #390 we introduces the possibility to use weak observers.