makeNotDraggable unexpected exceptions

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Add any widget to a boundaryPanel with a dragController
2. execute dragController.makeNotDraggable(widget) (note: I never made the 
widget draggable)
3. A runtime exception is thrown

What is the expected output? What do you see instead?
well,.. its already not draggable, expected output should be nothing, done. 
Instead the application 
crashes.

Please use labels and text to provide additional information.

I can't tell exactly where its crashing; my widget does not have a draghandle 
and I added it directly 
to my AbsolutePanel. However there are two points here.

In AbstractDragController makeNotDraggable(Widget draggable) there is no null 
check on

    Widget dragHandle = dragHandles.remove(draggable);
    mouseDragHandler.makeNotDraggable(dragHandle);

maybe my widget was never added or I call it on the wrong widget; replace with

    Widget dragHandle = dragHandles.remove(draggable);
    if(dragHandle != null) {
        mouseDragHandler.makeNotDraggable(dragHandle);
        draggable.removeStyleName(PRIVATE_CSS_DRAGGABLE);
        dragHandle.removeStyleName(PRIVATE_CSS_HANDLE);
    }

Second, in MouseDragHandler makeNotDraggable(Widget dragHandle), don't blow up 
the entire app just 
because a Widget is already not what your trying to change it to. replace

    RegisteredDraggable registeredDraggable = dragHandleMap.remove(dragHandle);
    if(registeredDraggable != null) {
        registeredDraggable.getMouseDownHandlerRegistration().removeHandler();
        registeredDraggable.getMouseUpHandlerRegistration().removeHandler();
        registeredDraggable.getMouseOutHandlerRegistration().removeHandler();
    }

Original issue reported on code.google.com by bbla...@gmail.com on 30 Nov 2009 at 4:18

[GoogleCodeExporter]

Thanks for reporting this.

This is by design though. The application should do it's own bookkeeping. 
Masking a 
double call to makeNotDraggable() could potentially mask bigger bugs in other 
apps.

"Fail early"

Original comment by fredsa@google.com on 1 Dec 2009 at 7:00

• Changed state: WontFix

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

I'll disagree. There is nothing fail fast about this.

    try {
        canvas.makeNotDraggable(widget);
    }
    catch(Exception e) {
        // do what?, nothing, who cares
    }

In any AbstractCollection an object is removed if found. The only case to throw 
an  
exception is if the remove method is not supported by the collection. The only 
difference is draggable is stored as a Map which returns null if the key 
doesn't 
exist (the implementation should be hidden by the API anyways).

I could produce the same exception by calling makeNotDraggable on the same 
widget  
twice. The object would be removed the first call then throw an exception the 
second.  
Why do I have to maintain my own list to make sure I don't incidentally call 
the  
wrong method?

Original comment by bbla...@gmail.com on 2 Dec 2009 at 3:53

[GoogleCodeExporter]

The fail fast is that when you code calls makeNotDraggable on a widget which is 
not in 
fact draggable, an exception is thrown immediately.

The alternative is that your application runs into the inaccurate book keeping 
at some 
later point in your application.

I realize neither case is perfect for everyone. In this case the trade off in 
gwt-dnd is 
made as currently implemented.

Original comment by fredsa@google.com on 2 Dec 2009 at 5:12