Bugzilla – Full Text Bug Listing
| Summary: | [EditPart] AbstractEditPart.getRoot() should not throw NPE if parent is null | ||
|---|---|---|---|
| Product: | [Tools] GEF | Reporter: | Rick Warren <rick.warren> |
| Component: | GEF-Legacy GEF (MVC) | Assignee: | Alexander Nyßen <nyssen> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | gmf.runtime.diagram-inbox, nyssen |
| Version: | 3.2 | ||
| Target Milestone: | 3.7.1 (Indigo) M3 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
If you fix one NPE, it is just going to pass the NPE along to the caller of that method. So, the next NPE will be in getViewer(). I don't see how that would help any here. There are other issues: There is code in DirectEditManager to bringDown() the cell editor when the editpart is deactivated. If the editpart has no parent, it should have been deactivated already so this is an invalid state (the cell editor still being active). On a separate note, undoing a command should not cause focus lost events. This could have unintended side effects in client code (for example, committing the changes in an unrelated celleditor). Even if it the call to getRoot() respectively getViewer() seems to be illegal in the reported usage scenario, I think it is not harmful to make the framework robust against calls to getRoot() and getViewer() in a situation where no parent is set. I have thus changed the implementations of getRoot() and getViewer() to simply return a null value in case no parent is set. I also changed the JavaDoc of the respective methods in the EditPart interface to state that both methods may return null (I also added a respective statement to getModel() , where it was missing as well). All changes are committed to cvs HEAD (3.7). |
AbstractEditPart.getRoot() is implemented like this: public RootEditPart getRoot() { return getParent().getRoot(); } If the parent is null, getRoot() will throw a NullPointerException. Since I gather that it's not necessarily incorrect for an edit part to have a null parent (depending on where in its lifecycle it is), nor for the root itself to be null, a better implementation would be the following: public RootEditPart getRoot() { if (getParent() != null) { return getParent().getRoot(); } return null; } I encountered this bug while working with GMF (a stack trace follows). In GMF, there's a class called GraphicalEditPart that has exactly the latter implementation of getRoot() (at least in version 1.0). Great! However, GraphicalEditPart extends the superclass AbstractGraphicalEditPart (from GEF), which inherits the implementation from AbstractEditPart. Unfortunately, some (but not all) of the edit parts for my model that GMF generates extend AbstractGraphicalEditPart but not GraphicalEditPart, which means that they inherit the problem implementation rather than the more forgiving one. I'm filing this bug with GEF instead of GMF because it seems like the problem really should be handled at the level of AbstractEditPart itself. However, the GEF maintainers may disagree with me and argue that the current getRoot() implementation is correct. I request the following: (1) GEF: Please decide whether it's appropriate for getRoot() to be called when the parent edit part is null. If it is acceptable: (1a) GEF: Please change the implementation to handle this case as described above. (1b) GMF: The override in GraphicalEditPart will no longer be necessary and can be removed. However, if getRoot() should not be called when the parent is null: (1c) GEF: Please document the contract of getRoot(), and in particular that it will throw if the contract is broken. (2) GMF: Decide whether GMF modifies the GEF contract. (2a) If it does, document that getRoot *may* be called when the parent is null. (2b) If the GEF contract is reasonable, fix the GMF runtime so that getRoot() will not be called at inappropriate times, and remove the getRoot() override from GraphicalEditPart. Details on how to reproduce the NPE: ------------------------------------ I have two nodes in a diagram. I use a connection creation tool to connect them. A new connection is created, and the connection label appears and is highlighted for editing. Immediately, without clicking anywhere, I undo with control-Z from the keyboard. The following stack trace occurs; it seems to have something to do with the connection label edit field: java.lang.NullPointerException at org.eclipse.gef.editparts.AbstractEditPart.getRoot(AbstractEditPart.java:551) at org.eclipse.gef.editparts.AbstractEditPart.getRoot(AbstractEditPart.java:551) at org.eclipse.gmf.runtime.diagram.ui.editparts.GraphicalEditPart.getRoot(GraphicalEditPart.java:1329) at org.eclipse.gef.editparts.AbstractEditPart.getViewer(AbstractEditPart.java:601) at org.eclipse.gef.tools.DirectEditManager.commit(DirectEditManager.java:135) at org.eclipse.gmf.runtime.diagram.ui.tools.TextDirectEditManager.commit(TextDirectEditManager.java:272) at org.eclipse.gef.tools.DirectEditManager$4.applyEditorValue(DirectEditManager.java:270) at org.eclipse.jface.viewers.CellEditor$1.run(CellEditor.java:305) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37) at org.eclipse.core.runtime.Platform.run(Platform.java:843) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:44) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:149) at org.eclipse.jface.viewers.CellEditor.fireApplyEditorValue(CellEditor.java:303) at org.eclipse.jface.viewers.CellEditor.focusLost(CellEditor.java:682) at org.eclipse.jface.viewers.TextCellEditor$5.focusLost(TextCellEditor.java:185) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:109) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:66) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:928) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:952) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:933) at org.eclipse.swt.widgets.Control.sendFocusEvent(Control.java:1970) at org.eclipse.swt.widgets.Widget.wmKillFocus(Widget.java:1671) at org.eclipse.swt.widgets.Control.WM_KILLFOCUS(Control.java:3575) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3277) at org.eclipse.swt.widgets.Text.windowProc(Text.java:1841) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4025) at org.eclipse.swt.internal.win32.OS.BringWindowToTop(Native Method) at org.eclipse.swt.widgets.Decorations.bringToTop(Decorations.java:183) at org.eclipse.swt.widgets.Shell.open(Shell.java:997) at org.eclipse.jface.window.Window.open(Window.java:792) at org.eclipse.jface.dialogs.ProgressMonitorDialog.open(ProgressMonitorDialog.java:640) at org.eclipse.ui.internal.operations.TimeTriggeredProgressMonitorDialog$1.checkTicking(TimeTriggeredProgressMonitorDialog.java:98) at org.eclipse.ui.internal.operations.TimeTriggeredProgressMonitorDialog$1.internalWorked(TimeTriggeredProgressMonitorDialog.java:122) at org.eclipse.core.runtime.ProgressMonitorWrapper.internalWorked(ProgressMonitorWrapper.java:94) at org.eclipse.core.runtime.SubProgressMonitor.internalWorked(SubProgressMonitor.java:151) at org.eclipse.core.runtime.ProgressMonitorWrapper.internalWorked(ProgressMonitorWrapper.java:94) at org.eclipse.core.runtime.SubProgressMonitor.internalWorked(SubProgressMonitor.java:151) at org.eclipse.core.runtime.SubProgressMonitor.worked(SubProgressMonitor.java:177) at org.eclipse.gmf.runtime.common.core.command.CompositeCommand.doUndoWithResult(CompositeCommand.java:659) at org.eclipse.gmf.runtime.common.core.command.AbstractCommand.undo(AbstractCommand.java:202) at org.eclipse.gmf.runtime.common.core.command.CompositeCommand.doUndoWithResult(CompositeCommand.java:627) at org.eclipse.gmf.runtime.common.core.command.AbstractCommand.undo(AbstractCommand.java:202) at org.eclipse.gmf.runtime.common.core.command.CompositeCommand.doUndoWithResult(CompositeCommand.java:627) at org.eclipse.gmf.runtime.common.core.command.AbstractCommand.undo(AbstractCommand.java:202) at org.eclipse.core.commands.operations.DefaultOperationHistory.doUndo(DefaultOperationHistory.java:413) at org.eclipse.core.commands.operations.DefaultOperationHistory.undo(DefaultOperationHistory.java:1244) at org.eclipse.ui.operations.UndoActionHandler.runCommand(UndoActionHandler.java:66) at org.eclipse.ui.operations.OperationHistoryActionHandler$4.run(OperationHistoryActionHandler.java:279) at org.eclipse.jface.operation.ModalContext.runInCurrentThread(ModalContext.java:369) at org.eclipse.jface.operation.ModalContext.run(ModalContext.java:313) at org.eclipse.jface.dialogs.ProgressMonitorDialog.run(ProgressMonitorDialog.java:479) at org.eclipse.ui.internal.operations.TimeTriggeredProgressMonitorDialog.access$6(TimeTriggeredProgressMonitorDialog.java:1) at org.eclipse.ui.internal.operations.TimeTriggeredProgressMonitorDialog$2.run(TimeTriggeredProgressMonitorDialog.java:203) at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67) at org.eclipse.ui.internal.operations.TimeTriggeredProgressMonitorDialog.run(TimeTriggeredProgressMonitorDialog.java:216) at org.eclipse.ui.operations.OperationHistoryActionHandler.run(OperationHistoryActionHandler.java:289) at org.eclipse.gmf.runtime.common.ui.action.actions.global.GlobalUndoAction.doRun(GlobalUndoAction.java:242) at org.eclipse.gmf.runtime.common.ui.action.AbstractActionHandler.run(AbstractActionHandler.java:347) at org.eclipse.gmf.runtime.common.ui.action.ActionManager$1.run(ActionManager.java:225) at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67) at org.eclipse.gmf.runtime.common.ui.action.ActionManager.run(ActionManager.java:223) at org.eclipse.gmf.runtime.common.ui.action.AbstractActionHandler.runWithEvent(AbstractActionHandler.java:365) at org.eclipse.ui.actions.RetargetAction.runWithEvent(RetargetAction.java:229) at org.eclipse.jface.commands.ActionHandler.execute(ActionHandler.java:119) at org.eclipse.core.commands.Command.executeWithChecks(Command.java:461) at org.eclipse.core.commands.ParameterizedCommand.executeWithChecks(ParameterizedCommand.java:424) at org.eclipse.ui.internal.handlers.HandlerService.executeCommand(HandlerService.java:160) at org.eclipse.ui.internal.keys.WorkbenchKeyboard.executeCommand(WorkbenchKeyboard.java:466) at org.eclipse.ui.internal.keys.WorkbenchKeyboard.press(WorkbenchKeyboard.java:799) at org.eclipse.ui.internal.keys.WorkbenchKeyboard.processKeyEvent(WorkbenchKeyboard.java:846) at org.eclipse.ui.internal.keys.WorkbenchKeyboard.filterKeySequenceBindings(WorkbenchKeyboard.java:564) at org.eclipse.ui.internal.keys.WorkbenchKeyboard.access$3(WorkbenchKeyboard.java:506) at org.eclipse.ui.internal.keys.WorkbenchKeyboard$KeyDownFilter.handleEvent(WorkbenchKeyboard.java:122) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:66) at org.eclipse.swt.widgets.Display.filterEvent(Display.java:982) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:927) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:952) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:937) at org.eclipse.swt.widgets.Widget.sendKeyEvent(Widget.java:965) at org.eclipse.swt.widgets.Text.sendKeyEvent(Text.java:1213) at org.eclipse.swt.widgets.Widget.sendKeyEvent(Widget.java:961) at org.eclipse.swt.widgets.Widget.wmChar(Widget.java:1275) at org.eclipse.swt.widgets.Control.WM_CHAR(Control.java:3346) at org.eclipse.swt.widgets.Text.WM_CHAR(Text.java:1846) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3246) at org.eclipse.swt.widgets.Text.windowProc(Text.java:1841) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4025) at org.eclipse.swt.internal.win32.OS.DispatchMessageW(Native Method) at org.eclipse.swt.internal.win32.OS.DispatchMessage(OS.java:1923) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:2966) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:1914) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:1878) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:419) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.IDEApplication.run(IDEApplication.java:95) at org.eclipse.core.internal.runtime.PlatformActivator$1.run(PlatformActivator.java:78) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:92) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:68) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:400) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:177) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.core.launcher.Main.invokeFramework(Main.java:336) at org.eclipse.core.launcher.Main.basicRun(Main.java:280) at org.eclipse.core.launcher.Main.run(Main.java:977) at org.eclipse.core.launcher.Main.main(Main.java:952)