search

sialcasa / mvvmFX

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

Clear DefaultNotificationCenter #585

Closed rguillens closed 5 years ago

rguillens commented 5 years ago

Is there any functionality to delete all registered notifications once a session is closed? Something like this would be quite useful for applications that handle sessions in the context of logout:

public void logout() {
    MvvmFX.getNotificationCenter().clear();
}

Of course, I could (1) implement my own NotificationCenter, (2) set a new one on logout using NotificationCenterFactory.setNotificationCenter(new DefaultNotificationCenter()); or (3) change the fields via Reflection API.

  1. I would be duplicating all the code, because I can't even extend current DefaultNotificationCenter implementation due to private fields containing the collections of observers;

  2. This is what I'm using actually, I call NotificationCenterFactory.setNotificationCenter(new DefaultNotificationCenter()); when application starts and on logout, to prevent populating the 'private static final' field defaultNotificationCenter that can't be cleared afterwards.

  3. No code duplication, but reliying on internal field names that could change in any version.

I think it would be nice to have this functionality integrated into the framework to prevent all this tricks.

manuel-mauky commented 5 years ago

There is no such functionality yet. Replacing the instance in the NotificationCenterFactory was also my first idea but it has a serious drawback: It might introduce memory-leaks: The NotificationCenter keeps references to the subscribers. If one of the subscribers cannot be garbage-collected for some reason it may result in the old NotificationCenter instance being also not garbage-collected. And then all other subscribers wouldn't be garbage-collected too. In the early days of mvvmFX we had similar problems with memory-leaks in some of our projects and introduced WeakNotificationObserver to solve some of them but I don't know if it also works if you completely replace the notification center instance. You should at least keep an eye on memory-leaks as long as you use this solution.

I think in the end you are right: There should be a explicit API for this. However, I don't know when I will find the time for this. If you could provide a Pull-Request for this feature I would be happy to merge it.