Bugzilla – Full Text Bug Listing
| Summary: | NPE when moving an SWT component on a read-only file | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Sébastien Gandon <sgandon> | ||||||
| Component: | GEF-Legacy GEF (MVC) | Assignee: | Steven R. Shaw <steveshaw> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | critical | ||||||||
| Priority: | P3 | CC: | richkulp | ||||||
| Version: | 3.2 | ||||||||
| Target Milestone: | 3.2.0 (Callisto) RC1 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows 2000 | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Sébastien Gandon
This works fine in the VE 1.2 development drivers we have. It does fail for us with VE 1.2M5, but it now works. We will be releasing a VE 1.2 Integration build for I20060405 shortly. You can give it a try and see if it fails for you. Found the problem. It is actually a GEF bug. I will be rerouting to them. What was different is that I didn't start dragging immediately. And the java editor automatically brings up a dialog to request do you want to change it to read/write. The problem was that when dragging immediately it goes through a slightly different path and gets into a state that GEF is confused with. If you select and not start dragging but instead wait for the dialog things will be fine. Created attachment 37807 [details]
The stack trace of the NPE.
GEF: Here is the problem. When you select something in the java visual editor and start dragging in the case of a read-only file the SelectionTool.setDragTracker is called with a new drag tracker. It is this piece of code causing the problem:
public void setDragTracker(DragTracker newDragTracker) {
if (newDragTracker == dragTracker)
return;
if (dragTracker != null)
dragTracker.deactivate();
dragTracker = newDragTracker;
refreshCursor();
// if (!getCurrentInput().isMouseButtonDown(3))
// setMouseCapture(dragTracker != null);
if (newDragTracker != null) {
newDragTracker.setEditDomain(getDomain());
newDragTracker.activate();
newDragTracker.setViewer(getCurrentViewer());
}
}
The problem is in the refreshCursor() call. When it is called the new drag tracker has been placed into the selection tool but it has not yet set the new drag tracker up (the code at the end of the method does this). We overrode the refreshCursor to do the tests to make sure the file is writable. The standard Eclipse to do that pops up a dialog asking if you want to make the file read/write. But this causes a lose of focus and so it goes to try to deactivate the drag tracker. But the drag tracker is not yet set up, there is no editDomain set into it yet, so the deactivate throws an NPE when it tries to access the editdomain.
This stack trace shows the path. The two lines marked with "--->" are the start and end of the interesting section of the stack trace.
For seb.fr: There is a persistent problem in SWT VE. In that case click on the java beans tree first instead of the graphical area. I was testing with Swing VE and didn't have the problem unless I started dragging immediately. But it happened right away on SWT. So use the tree first. GEF could add protection code to prevent the NPE, but this would leave a listener still installed on the command stack. Perhaps there's another place the command stack listener should be removed as well? Could you instead just move the refresh cursor call to be AFTER the activation of the drag tracker? Is there any reason the refreshCursor needs to be before activation of the drag tracker? That way the drag tracker is truly active at the time of the focus being stolen. And it is acurate in that since the focus is stolen you really want to deactivate. You don't want it to become active when the dialog that is popped up is disposed because at that point in time the mouse is no longer where it was at the time of drag tracker activation and so the tracker could be a little confused. You could give me a patched jar and I could give that a try before you actually release the changes to see if that causes any other problems. Created attachment 38156 [details]
Deployable jar with fix
See attached for deployable jar of GEF for test fix.
I can't see any reason why refreshCursor needs to be before the drag tracker is initialized. I did some simple testing in the logic diagram example and couldn't find any problems.
Let me know if this fix works for you.
Thanks, that worked well. I had no problems with it either. Committed change. Moved refreshCursor call to be after the drag tracker initialization. Committed change. Moved refreshCursor call to be after the drag tracker initialization. Rich, refreshCursor() sounds like a strange place to query whether editing is possible, and an even stranger place to be opening a dialog. You might consider doing this in the state transition to DRAG_IN_PROGRESS.
A safer fix at this late stage might be to change:
if (newDragTracker != null) {
newDragTracker.setEditDomain(getDomain());
newDragTracker.activate();
newDragTracker.setViewer(getCurrentViewer());
}
to refer to the real field instead of the parameter, since the real field will be NULL:
if (dragTracker != null) {
newDragTracker.setEditDomain(getDomain());
newDragTracker.activate();
newDragTracker.setViewer(getCurrentViewer());
}
I haven't followed the discussion to closely, but would this also solve the problem?
|