My favorites | Sign in
Google
                
New issue | Search
for
| Advanced search | Search tips
Issue 3102: De-deprecate HandlerManager.removeHandler
13 people starred this issue and may be notified of changes. Back to list
 
Reported by ecc@google.com, Nov 12, 2008
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.


Comment 1 by rjrjr+pe...@google.com, Dec 07, 2008
(No comment was entered for this change.)
Labels: Milestone-Planned Category-UI
Comment 2 by pronkly, 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.
Comment 3 by cbraid, 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
	}
});



Comment 4 by cbraid, 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();
	}
});


Comment 5 by post2edbras, 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
Comment 7 by ekassif, 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...
Comment 8 by abowers+...@google.com, 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
Comment 9 by post2edbras, Jun 05, 2009
I am very currious what the alternative is??
Comment 11 by yanick.rochon, 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.
Comment 12 by post2edbras, 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)...


Comment 13 by yanick.rochon, 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.
Comment 15 by pohl.longsine, 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. 
Comment 16 by rjrjr+pe...@google.com, 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
Comment 17 by post2edbras, Aug 04, 2009
>> I'm pretty persuaded by the WidgetGroup scenario

hahahah :)
Comment 18 by jlabanca+personal@google.com, 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.
Comment 19 by troy.jezewski, 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.
Comment 20 by rjrjr+pe...@google.com, Sep 01, 2009
We will not remove the method, and will remove its @Deprecated tag for milestone 1
Comment 21 by talianu.robert, 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.
Comment 22 by rjrjr+pe...@google.com, 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
Comment 23 by cramsdale@google.com, Nov 23, 2009
(No comment was entered for this change.)
Status: Fixed
Comment 24 by andimullaraj, 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.
Comment 25 by andimullaraj, 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 ...


Comment 26 by jlaba...@google.com, 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
Sign in to add a comment