Bugzilla – Full Text Bug Listing
| Summary: | [Workbench] EventBroker#unsubscribe is not working when same eventhandler registered for more than one topic | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Thomas Schindl <tom.schindl> |
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> |
| Status: | CLOSED WONTFIX | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | bsd, Lars.Vogel, ob1.eclipse, pwebster, remy.suen, sumitsingh_1234 |
| Version: | 4.0 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Mac OS X - Carbon (unsup.) | ||
| Whiteboard: | stalebug | ||
Wrong, wrong, ...
> I think Solution 2 is the way to go here!
Solution 1 is the way to go!!!!
Just to clear up why I marked this as major is that this has very likely an impact on the API which we need to address quite soon because we'll break client code if we go with solution 1 > broker.subscribe("/Blo/*",h);
That's the "old" way of doing it; the new way is:
@Inject @Optional
public void event(@EventTopic(TOPIC) Object data) {
.....
}
I know this one but you can't avoid that people get access to the IEventBroker :-) (In reply to comment #3) > @Inject @Optional > public void event(@EventTopic(TOPIC) Object data) { Right now, most of our code looks like: broker.subscribe(, additionHandler); So now what should it look like? class X { @Inject @Optional public void event(@EventTopic(UIEvents.buildTopic(UIEvents.BindingTableContainer.TOPIC, UIEvents.BindingTableContainer.BINDINGTABLES)) Object data) { // whatever } } ContextInjectionFactory.make(X.class, e4Context); PW (In reply to comment #5) > Right now, most of our code looks like: > broker.subscribe(, additionHandler); That should be: broker.subscribe(UIEvents.buildTopic(UIEvents.BindingTableContainer.TOPIC, UIEvents.BindingTableContainer.BINDINGTABLES), additionHandler); PW (In reply to comment #5) > So now what should it look like? > ... Yes; for UI events we probably need to update the sender part to make sure it wraps event data in a proper way (like EventUtils.send()). I planed to do that and update event receivers, but it is always pushed back by some other things. (The receiving data type can be set to whatever is needed; does not have to be an Object.) (In reply to comment #4) > I know this one but you can't avoid that people get access to the IEventBroker > :-) Right, we need to cleanup event processing code - the way we want to use it evolved, but existing code was not updated. Do you think my proposed solution 1 is the way to go? (In reply to comment #9) > Do you think my proposed solution 1 is the way to go? For e4 SDK code I hope to switch all code to event injection, thus removing the need for EventBroker in the e4 SDK itself. I am not sure at this point if EventBroker should be still available for cases where people don't want to use injection, but do want to use whiteboard event processing. We should have a better idea about this once we change the current SDK code to use event injection. I've opened Bug 331690 to start converting some of our usages of IEventBroker. PW (In reply to comment #10) > (In reply to comment #9) > > Do you think my proposed solution 1 is the way to go? > > For e4 SDK code I hope to switch all code to event injection, thus removing > the need for EventBroker in the e4 SDK itself. We need IEventBroker as API to send out events, so removing this one should not be an option. Just a note here: the EventBroker still needs to remember subscriptions as it's responsible for removing the subscriptions when disposed. A couple of issue here I think: 1) You cannot 'unsubscribe' event handlers using the DI mechanism (are we *sure* that they get correctly unregistered on 'uninject' / disposal ? If so how would the current DI mechanism deal with the scenario ?) 2) The original problem; registrations of the same listener against multiple topics. This appears to be a failing of the IEventBroker API for not providing a specific remove based on the topic. Is there already a defect for this in OSGI ? Yes they are unsubscribed — on context dispose. See EventBrokerTest#testUnsubscribeOnDispose(). The IEventBroker is an E4 API, not OSGi EventAdmin. The EventAdmin handles event listeners using the OSGi service mechanism. This problem is a failing of our IEventBroker API. Basically we need a new method to unsubscribe a handler for a particular topic. Hi *, I got the same problem in my RCP. Till this bug will fix, apart from creating different eventhandler for each topic is there is any other workaround ..? The workaround for now is to use different instances of handlers for different topics, or use @EventTopic/@UIEventTopic injection PW There's no leak here, it looks like unsubscribe(eventHandler) removes all subscribed topics for that event handler. So that unsubscribe will remove the eventHandler from all topics it was subscribed to. Is this something we still want to tackle? PW This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |
I come to this conclusion simply by looking at the code. Suppose someone does this: @Inject EventBroker broker; EventHandler h; public MyView() { h = new EventHandlerImpl(); broker.subscribe("/Bla/*",h); broker.subscribe("/Blo/*",h); } @preDestroy void cleanUp() { h.unsubscribe(h); // This will only remove the "/Blo/*" } The problem is that our internal Map only uses the EventHandler as the key. My proposal would be: Solution 1: ----------- a) Remove the unsubscribe method b) Return not true but a Subscription-Type like public interface Subscribe { public void unsubscribe(); } => This way we don't need the Map any more Solution 2: ----------- a) Add 2 new methods: unsubscribe(String,String,EventHandler,boolean) unsubscribe(String,EventHandler) b) Create an internal Type which holds topic,filter,headless,handler I think Solution 2 is the way to go here!