| Issue 3102: | De-deprecate HandlerManager.removeHandler | |
| 13 people starred this issue and may be notified of changes. | Back to list |
Sign in to add a comment
|
Found in GWT Release: 1.6 Detailed description: This is a placeholder issue so we can find out if anyone needs the HandlerManager.removeHandler method once listeners are removed from GWT. If you do need it, please add a comment below with why you need it. |
||||||||||
,
Dec 07, 2008
(No comment was entered for this change.)
Labels: Milestone-Planned Category-UI
|
|||||||||||
,
Jan 27, 2009
We've create file upload widget and we use method like
// the default handlers will be used
public void upload() {
panel.submit();
}
// additional handlers should be used once. We add them, execute upload, and
remove handlers as soon as we receive the response
public void upload(final UploadSuccessHandler successHandler, final
UploadFailureHandler failureHandler) {
addUploadSuccessHandler(
new UploadSuccessHandler() {
public void onSuccess(UploadSuccessEvent event) {
successHandler.onSuccess(event);
FileUploadWidget.this.getHandlers().removeHandler(UploadSuccessEvent.getType(), this);
}
}
);
addUploadFailureHandler(
new UploadFailureHandler() {
public void onFailure(UploadFailureEvent event) {
failureHandler.onFailure(event);
FileUploadWidget.this.getHandlers().removeHandler(UploadFailureEvent.getType(), this);
}
}
);
panel.submit();
}
in second case it is convenient to use deprecated removeHandler.
We can also register handlers as private properties of the class, register two
methods that will emulate event and delegates execution to the handlers, but I think
that workaround will be more complex.
|
|||||||||||
,
Feb 02, 2009
It is not possible to call handlerRegistration.removeHandler() from within the
handler itself
This code won't compile :
HandlerRegistration registration = supplierReady.addOpenHandler(new
OpenHandler<SupplierWithDetailsModel>() {
public void onOpen(OpenEvent<SupplierWithDetailsModel> event) {
registration.removeHandler();
// do something
}
});
Therefore you have to use this :
supplierReady.addOpenHandler(new OpenHandler<SupplierWithDetailsModel>() {
public void onOpen(OpenEvent<SupplierWithDetailsModel> event) {
supplierReady.handlers.removeHandler(OpenEvent.getType(), this);
// do something
}
});
|
|||||||||||
,
Feb 02, 2009
of course there are tricks like this :
final HandlerRegistration[] reg = new HandlerRegistration[1];
reg[0] = supplierReady.addOpenHandler(new OpenHandler<SupplierWithDetailsModel>() {
public void onOpen(OpenEvent<SupplierWithDetailsModel> event) {
reg[0].removeHandler();
}
});
|
|||||||||||
,
May 08, 2009
I am migrating to the new version: 1.5 -> 1.6.4 and I am not so sure about this removal... The problem I encounter is better explained through a little example: Suppose you have an class WidgetGroup that is used to add/remove widget and decorate them. For example: single selection of a widget in a list or single focus control of the list. So what I want to do: add a focus listener (or click listener) as soon as a widget is added to WidgetGroup and remove the focus listener when it's removed from WidgetGroup. In 1.5. you could easily do this by creating one FocusListener and simple call the addFocusListener/removeFocusListener to add/remove the focus listener. Because the central removeFocusListener is gone, this is a bit of a problem and needs more coding: maintaining a extra Map of Widget and HandlerRegistration objects that kind of shadows the Map in the HandlerManager to hold the HandlerRegistration instance that are needed to remove the focus listener of a widget .... :(... Euuhhhhhh.... Yep... a lot of code extra to "just" remove a focus listener... So I strongly suggest not removing the HandlerManager.removeHandler(..) method, but improve his usage such that the above situation is at least improved. Why not combine the HandlerRegistration and removeHandler(..) funcationality to get the best of both worlds? (I haven't got a conrete answer right now but I am sure the gwt dev team can come up with a good solution) It's funny as I read that the new event model results in less code, but in my case I doubt that in cases like this... -- Ed |
|||||||||||
,
Jun 03, 2009
I would like this method to stay as well... I ran into several situations where I need a "one time" handler that will be removed after execution, so now instead of calling "remove*listener" from within the listener itself, I have to do the trick mentioned by cbraid... |
|||||||||||
,
Jun 05, 2009
There do appear to be use cases where this is useful. Unless there is a strong rationale for removing it or elegant alternative pattern (which may exist - haven't talked to Ray about this), I think this should be left in.
Owner: rj...@google.com
|
|||||||||||
,
Jun 05, 2009
I am very currious what the alternative is?? |
|||||||||||
,
Jul 24, 2009
I understand the principle of wanting to remove the this method. Since addHandler registers an handler of type <H extends EventHandler>, the removeHandler() method of the returned object is somewhat prettier (and faster) than the conventional removeHandler(type,handler), although is not consistent with the "standard" way of doing things in Java, which addHandler(...) would return a boolean value (making sure that no duplicate handlers for a same type would be added to the manager) and removeHandler would return the removed object (see the Collection package). However, since all the GWT handler interfaces (HasXxxxHandlers) behaves this way, the object responsible to register the event is indeed, then, responsible to remove it and, thus, keeping a convenient object to do so is, indeed, convenient. Therefore, and after considering it, and despite all other comments, it should be removed from the class altogether for the 2.0 release (being a major release, project should stick with 1.x or refactor to follow compliances. Keeping responsibility where it belongs is good practice and I support it. |
|||||||||||
,
Jul 25, 2009
I have to disagree on this last comment. I surely understand your point as I also read "these books". Indeed the theory points to removal, I also supported this at first However, when working with it, I turned around my opinion as I encounter, just like the people above, that you simple need this method. Constructing your code in a different way isn't always the best option as it might create very strange fragil behavior in other places. Just like I have to create very exotic constructions when this method is removed. I don't think this is desirable...... On the other hand, what is the win when removing this method ??.. The only thing is that it can be removed on several places .. that's it... It's not as dangerous method like the call Widget.removeFromParent()...which can lead to memory leaks... I think removal of that method must have a higher priority... (or making it package protected)... (See an other ticket about this)... |
|||||||||||
,
Jul 26, 2009
Looking at a hot topics on Groups, one that I'm closely following is the MVP pattern. Well, regardless of using this pattern or not, I think that, after all, an object registering handlers is responsible to clean itself up, therefore I don't see why you could add an handler and remove it from different classes. This ultimately lead to spaghetti code and hard to maintain resource owners. I believe that's simply a matter of "best practices". The only two downsides that I see by removing this method is that each added handlers must have an associated HandlerRegistration object for removal, and if that object is lost... somehow, there is no way to remove it from the HandlerManager. In conclusion, it all falls back to the "competence" of the programmer (or team) that coded the project. IMHO. |
|||||||||||
,
Aug 04, 2009
Ray, I need some clarification on what this issue tracks. Will I still be able to remove a handler by calling a remove() method on the HandlerRegistration instance that I obtain when I call addHandler()? The reason that I ask is that I noticed that the DefaultHandlerRegistration's internal implementation calls this deprecated method...and if it's removed will this class still be able to implement its removeHandler() method? I'm using a HandlerManager instance as an event bus as you described in your Google I/O session this year (two thumbs up/I laughed, I cried/Better than Cats) and I want to have my components be good citizens of the event bus by cleaning up their handlers whenever they get a PlaceChangeEvent that indicates that they have lost relevance -- as you described in the Q&A at the end. So long as they are still able to perform their cleanup through the HandlerRegistration objects they get back, I do not need the HandlerManager#removeHandler() method itself. |
|||||||||||
,
Aug 04, 2009
Yes, this discussion is entirely about HandlerManager#removeHandler, which is redundant with HandlerRegistration#remove. I'm pretty persuaded by the WidgetGroup scenario, and am inclined do de-deprecate HandlerManager#removeHandler. cc'ing John and Joel, who are most likely to care. Updating the summary to reflect this, and targeting 2.0 ms1
Summary: De-deprecate HandlerManager.removeHandler
Cc: jlaba...@google.com j...@google.com Labels: -Milestone-Planned Milestone-2_0_MS1 |
|||||||||||
,
Aug 04, 2009
>> I'm pretty persuaded by the WidgetGroup scenario hahahah :) |
|||||||||||
,
Aug 04, 2009
+1 I don't see any problem with keeping it, and I agree with the mentioned use cases. Comment 3 about not being able to call removeHandler() from within the handler is especially convincing. |
|||||||||||
,
Aug 31, 2009
Has anyone made an official call on this? It would be good to know if we need to start migrating off the deprecated methods. |
|||||||||||
,
Sep 01, 2009
We will not remove the method, and will remove its @Deprecated tag for milestone 1 |
|||||||||||
,
Nov 01, 2009
Maybe the comment is a little late but I do not think that removing the HandlerManager#removeHandler is a good idea. I agree with Comment 5 that the removing of this method makes things worse! Using HandlerRegistration#removeHandler is useful when involved in small actions but when the complexity increases it does not help much it makes them more complicated. Imagine that you have an event manager that has to manage many widgets and many types of events for each widget! How do you do it in a generic way? (The only solution I see is to make a map that holds widgets linked with another map that holds event type and a HandlerRegistration. A very complex situation !? Even if you have the manager, the widget and the event you have to make a lot of searching to get to the HandlerRegistration) I my opinion I think that using generics together with this HandlerRegistration#removeHandler only creates the situation where you have to make a copy of the backing map the Widget uses for the event management. I would suggest to add an equivalent method for each addXXXHandler something like removeXXXHandler although this may be very redundant. Adding the removeXXXHandler does not complicate things because it uses the same identifiers as the HandlerRegistration#removeHandler - the event type and the handler. |
|||||||||||
,
Nov 02, 2009
It's no longer deprecated and will not be removed, honest. Committed trunk@6581, releases/2.0@6594
Status: FixedNotReleased
Labels: -Milestone-2_0_MS1 Milestone-2_0_RC1 |
|||||||||||
,
Nov 23, 2009
(No comment was entered for this change.)
Status: Fixed
|
|||||||||||
,
Dec 03, 2009
While validating your removeHandler strategy, inside HandlerManager I see:
private <H> void removeHandler(GwtEvent.Type<H> eventKey, H handler) {
ArrayList<H> l = get(eventKey);
boolean result = l.remove(handler);
if (l.size() == 0) {
map.remove(eventKey);
}
assert result : "Tried to remove unknown handler: " + handler + " from "
+ eventKey;
}
}
You are asserting the handler should exist, but letting go a potential NPE (l could
be null l.remove(handler);). Just making sure it's not a bug.
|
|||||||||||
,
Dec 03, 2009
If I may add a little clarification to post nr 12. I am in favor for keeping removeHandler too and after looking at the HandlerManager I think that Type and Handler (required both for the removal) won't necessarily be available everywhere in the code, making removal from different places hard, or intentional. So one less worry abt keeping the removeHandler, IMO ... |
|||||||||||
,
Dec 04 (6 days ago)
andimullaraj - an NPE check was added to removeHandler at r5266: http://code.google.com/p/google-web-toolkit/source/detail?r=5266
Owner: rj...@google.com
|
|||||||||||
|
|
|||||||||||