NPE in PaletteViewer

[Phillipus]

Hi,

I don't know whether this is a problem in my fork of GEF or not. But the affected classes are the same in my fork.

java.lang.NullPointerException: Cannot invoke "org.eclipse.draw2d.EventDispatcher.requestFocus(org.eclipse.draw2d.IFigure)" because the return value of "org.eclipse.draw2d.IFigure.internalGetEventDispatcher()" is null
	at org.eclipse.gef.ui.palette.PaletteViewer.handleFocusGained(PaletteViewer.java:262)
	at org.eclipse.gef.ui.parts.GraphicalViewerImpl$2.focusGained(GraphicalViewerImpl.java:291)

Click here for the full stack trace.

java.lang.NullPointerException: Cannot invoke "org.eclipse.draw2d.EventDispatcher.requestFocus(org.eclipse.draw2d.IFigure)" because the return value of "org.eclipse.draw2d.IFigure.internalGetEventDispatcher()" is null
	at org.eclipse.gef.ui.palette.PaletteViewer.handleFocusGained(PaletteViewer.java:262)
	at org.eclipse.gef.ui.parts.GraphicalViewerImpl$2.focusGained(GraphicalViewerImpl.java:291)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:181)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4364)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1217)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1241)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1222)
	at org.eclipse.swt.widgets.Control.sendFocusEvent(Control.java:2956)
	at org.eclipse.swt.widgets.Widget.wmSetFocus(Widget.java:2474)
	at org.eclipse.swt.widgets.Control.WM_SETFOCUS(Control.java:5542)
	at org.eclipse.swt.widgets.Canvas.WM_SETFOCUS(Canvas.java:433)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4931)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:336)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5132)
	at org.eclipse.swt.internal.win32.OS.SetFocus(Native Method)
	at org.eclipse.swt.widgets.Control.forceFocus(Control.java:1069)
	at org.eclipse.swt.widgets.Control.setFocus(Control.java:3459)
	at org.eclipse.swt.widgets.Composite.setFocus(Composite.java:1079)
	at org.eclipse.swt.widgets.Decorations.restoreFocus(Decorations.java:738)
	at org.eclipse.swt.widgets.Decorations.WM_ACTIVATE(Decorations.java:1544)
	at org.eclipse.swt.widgets.Shell.WM_ACTIVATE(Shell.java:2422)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4856)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:336)
	at org.eclipse.swt.widgets.Decorations.windowProc(Decorations.java:1499)
	at org.eclipse.swt.widgets.Shell.windowProc(Shell.java:2395)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5132)
	at org.eclipse.swt.internal.win32.OS.DestroyWindow(Native Method)
	at org.eclipse.swt.widgets.Control.destroyWidget(Control.java:755)
	at org.eclipse.swt.widgets.Shell.destroyWidget(Shell.java:745)
	at org.eclipse.swt.widgets.Widget.release(Widget.java:916)
	at org.eclipse.swt.widgets.Widget.dispose(Widget.java:488)
	at org.eclipse.swt.widgets.Decorations.dispose(Decorations.java:371)
	at org.eclipse.jface.window.Window.close(Window.java:335)
	at org.eclipse.jface.dialogs.Dialog.close(Dialog.java:985)
	at org.eclipse.jface.dialogs.MessageDialog.buttonPressed(MessageDialog.java:274)
	at org.eclipse.jface.dialogs.Dialog.lambda$0(Dialog.java:617)
	at org.eclipse.swt.events.SelectionListener$1.widgetSelected(SelectionListener.java:83)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:290)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4364)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1217)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4162)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3750)
	at org.eclipse.jface.window.Window.runEventLoop(Window.java:824)
	at org.eclipse.jface.window.Window.open(Window.java:804)
	at org.eclipse.jface.dialogs.MessageDialog.open(MessageDialog.java:608)
	at com.archimatetool.editor.model.impl.EditorModelManager.askSaveModel(EditorModelManager.java:474)
	at com.archimatetool.editor.model.impl.EditorModelManager$1.preShutdown(EditorModelManager.java:106)
	at org.eclipse.ui.internal.Workbench$4.run(Workbench.java:997)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:47)
	at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:169)
	at org.eclipse.ui.internal.Workbench.firePreShutdown(Workbench.java:994)
	at org.eclipse.ui.internal.Workbench.busyClose(Workbench.java:1123)
	at org.eclipse.ui.internal.Workbench.lambda$5(Workbench.java:1474)
	at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67)
	at org.eclipse.ui.internal.Workbench.close(Workbench.java:1474)
	at org.eclipse.ui.internal.Workbench.close(Workbench.java:1445)
	at org.eclipse.ui.internal.WorkbenchWindow.busyClose(WorkbenchWindow.java:1717)
	at org.eclipse.ui.internal.WorkbenchWindow.lambda$5(WorkbenchWindow.java:1744)
	at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:67)
	at org.eclipse.ui.internal.WorkbenchWindow.close(WorkbenchWindow.java:1744)
	at org.eclipse.ui.internal.WorkbenchWindow.close(WorkbenchWindow.java:1753)
	at org.eclipse.ui.internal.WorkbenchWindow.lambda$2(WorkbenchWindow.java:505)
	at org.eclipse.e4.ui.workbench.renderers.swt.WBWRenderer.lambda$4(WBWRenderer.java:566)
	at org.eclipse.swt.events.ShellListener$2.shellClosed(ShellListener.java:101)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:140)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4364)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1217)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1241)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1226)
	at org.eclipse.swt.widgets.Decorations.closeWidget(Decorations.java:268)
	at org.eclipse.swt.widgets.Decorations.WM_CLOSE(Decorations.java:1571)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4861)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:336)
	at org.eclipse.swt.widgets.Decorations.windowProc(Decorations.java:1499)
	at org.eclipse.swt.widgets.Shell.windowProc(Shell.java:2395)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5132)
	at org.eclipse.swt.internal.win32.OS.DefWindowProc(Native Method)
	at org.eclipse.swt.widgets.Shell.callWindowProc(Shell.java:509)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4961)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:336)
	at org.eclipse.swt.widgets.Decorations.windowProc(Decorations.java:1499)
	at org.eclipse.swt.widgets.Shell.windowProc(Shell.java:2395)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5132)
	at org.eclipse.swt.internal.win32.OS.DefWindowProc(Native Method)
	at org.eclipse.swt.widgets.Shell.callWindowProc(Shell.java:509)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:4961)
	at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:336)
	at org.eclipse.swt.widgets.Decorations.windowProc(Decorations.java:1499)
	at org.eclipse.swt.widgets.Shell.windowProc(Shell.java:2395)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5132)
	at org.eclipse.swt.internal.win32.OS.DispatchMessage(Native Method)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3748)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1147)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1038)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:153)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:677)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:583)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
	at com.archimatetool.editor.Application.start(Application.java:65)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:219)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:149)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:115)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:467)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:298)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:615)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:563)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1415)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1387)

I was experimenting with a PaletteStack basically like the code below, clicking on the stack, pinning it. dragging palette objects onto the canvas, unpinning it and then, when the stack was closed, I clicked on it and got the above NPE.

PaletteGroup group = new PaletteGroup("Group");
PaletteStack stack = new PaletteStack("Stack", "", "");
group.add(stack);
ToolEntry entry1 = new CombinedTemplateCreationEntry("1", "", factory, null, null);
stack.add(entry1 );
ToolEntry entry2 = new CombinedTemplateCreationEntry("2", "", factory, null, null);
stack.add(entry2);

[Phillipus]

Steps to reproduce:

1. Open the Palette Stack (by clicking on the reveal triangle)
2. Pin Open
3. Select an entry in the lower part of the Stack and add it to the Canvas
4. Close the stack with the little reveal triangle
5. Click on the Canvas so it takes the focus
6. Click on the top-level Stack entry

The thing that triggers it is having the Stack pinned open.

[azoitl]

I can reproduce it with the logic editor. What I see is that getFocusEditPart() is providing the editpart of the tool inside of the collapsed pined stack. As the stack is closed in step 4 the figure that is provided by this editpart is not in the figure tree anymore therefore internalGetEventDispatcher is not able to find the RootFigure.

I thought a workaround would be to use the root editpart's figure for getting the event dispatcher but then the focus would be given to the not shown figure. I don't think this is the right way.

Most probably we need to change the selection when a pined stack is closed as done in step 4. Similar to what is happening when the stack is autoclosing when it is not pinned.

[ptziegler]

I already found this logic fishy, the last time I looked at it. And Bug 63297 - Focus problems in the palette, which is supposed to be fixed by this is not something I can reproduce.

gef-classic/org.eclipse.gef/src/org/eclipse/gef/ui/palette/PaletteViewer.java

Lines 268 to 280 in 16d9b02

	/*
	* Fix for Bug# 63297 When the palette gets focus, the LWS will take care of
	* setting focus on the correct figure. However, when the user clicks on an
	* entry, the focus is "thrown away." In that case, the LWS would set focus on
	* the first focusable figure. We override that here and set focus on the
	* correct/selected figure. Invoking getFocusEditPart().setFocus(true) does not
	* work because that editpart thinks it already has focus (it's not aware of
	* focus being thrown away) and does nothing.
	*/
	if (focusPart == null) {
	IFigure fig = ((GraphicalEditPart) getFocusEditPart()).getFigure();
	fig.internalGetEventDispatcher().requestFocus(fig);
	}

Perhaps we should check whether this is still needed?

[Phillipus]

Perhaps we should check whether this is still needed?

Would it be safer to do a null check for this one case?

gef-classic/org.eclipse.draw2d/src/org/eclipse/draw2d/Figure.java

Lines 1089 to 1094 in 16d9b02

	public EventDispatcher internalGetEventDispatcher() {
	if (getParent() != null) {
	return getParent().internalGetEventDispatcher();
	}
	return null;
	}

[ptziegler]

Would it be safer to do a null check for this one case?

I don't think that really solves the issue. Even if the exception is avoided here, you still have the broken selection. I agree with @azoitl that the selection itself needs to be corrected.

Most probably we need to change the selection when a pined stack is closed as done in step 4. Similar to what is happening when the stack is autoclosing when it is not pinned.

The problem itself is caused by this section, where the expandablePane contains the figure of the selected EditPart that will later throw the NPE:

gef-classic/org.eclipse.gef/src/org/eclipse/gef/internal/ui/palette/editparts/PinnablePaletteStackFigure.java

Lines 530 to 533 in 16d9b02

	if (expandablePane.getParent() == this) {
	remove(expandablePane);
	pinFigure.getParent().remove(pinFigure);
	}

As as side note: This error doesn't necessarily depend on the pinned state. You can select the stack entry with the arrow keys and then close the stack.

I haven't seen anything similar for the "normal" stack, so I don't think the exception can be thrown there. Personally, i would expect the stack to behave similar to a tree viewer: If you close the stack while one of its entries is selected, the stack itself gets selected.

[azoitl]

I removed the code that was added for Bug 63297 in PaletteViewer::handleFocusedGained() and played a bit with the the LogicEditor. There I could not find any issues. So it could be the case that it is not needed anymore. I even tried the described way, by opening the palette view and switching between views with keyboard shortcuts (CTRL+F7). I tested on GTK.

So maybe we can really remove that piece of code by now.

[Phillipus]

So maybe we can really remove that piece of code by now.

That's probably preferable to a NPE. ;-)