Closed Phillipus closed 2 months ago
Phillipus
commented
2 months ago Phillipus
commented
2 months ago Here's a screen recording. I have Eclipse on the left and our app on the right. I detach the editor, hover over the select tool to see its tooltip, re-attach the editor, and then hover over the same tool again. You can see the exception thrown in Eclipse's log on the left.
https://github.com/eclipse/gef-classic/assets/600504/a46b3c93-c083-4aa9-a559-917de67d448a
ptziegler
commented
2 months ago This exception only seems to be thrown when the tooltip has not been shown, prior to detaching the edit part.
My guess is that this is due to the lazy initialization of the tooltip helper. Depending on when the class is instantiated, it might either attach itself to either the shell containing the IDE or the shell containing detached edit part.
When the latter is closed, the tooltip helper is not updated and still referencing the -now- disposed shell.
ptziegler
commented
2 months ago This is the problem here. control.getShell() might be one of the shells mentioned above. When the shell is disposed, the FigureCanvas must create a new ToolTipHelper instance.
ptziegler
commented
2 months ago @Phillipus
Can you check what happens when you replace getShell() in PopUpHelper with this?
protected Shell getShell() {
if (shell == null || shell.isDisposed()) {
shell = createShell();
hookShellListeners();
}
return shell;
}Can you check what happens when you replace getShell() in PopUpHelper with this?
The same exception. It doesn't even get to that point in the code when the exception is thrown.
Phillipus
commented
2 months ago I traced it and the canvas in this line is what is disposed:
Phillipus
commented
2 months ago It's the Shell in PopUpHelper that's getting disposed, as you suggested. I think this Shell is still referenced in the corresponding Tool Entry Edit Part.
ptziegler
commented
2 months ago The same exception. It doesn't even get to that point in the code when the exception is thrown.
The PopUpHelper is creating its own LightweightSystem. When the shell is disposed, this is what's finally causing the error.
So my previous suggestion was not sufficient. When the old shell is disposed, one needs to create a new shell and a new LWS.
Phillipus
commented
2 months ago When the old shell is disposed, one needs to create a new shell and a new LWS.
Ugh. That sounds like it won't be easy.
Phillipus
commented
2 months ago Actually it is easy. Apply the same check for shell disposed to getLightweightSystem and it works! Don't know if there's any overhead in doing this.
ptziegler
commented
2 months ago My current idea is to simply add two methods addDisposeListener() and removeDisposeListener() to the ToolTipHelper class (or perhaps directly to PopUpHelper). Those listeners are notified, when the underlying shell is disposed.
From the SWTEventDispatcher, I can then a listener, which sets the local instance of the ToolTipHelper to null, forcing a new instance to be created, the next time a tooltip needs to be shown.
Phillipus
commented
2 months ago Is it too naive to simply add a Shell dispose listener to PopUpHelper and simply set the LightweightSystem and Shell to null so they are re-created?
ptziegler
commented
2 months ago Perhaps it is. I haven't come up with a good solution yet and am just playing around with different ideas.
Phillipus
commented
2 months ago For now, I've done this in PopUpHelper for our app:
protected Shell createShell() {
Shell newShell = new Shell(control.getShell(), shellStyle);
newShell.addDisposeListener(e -> {
shell = null;
lws = null;
});
return newShell;
}Not sure if there are any side effects, but it must be better than a SWTException.
Phillipus
commented
2 months ago I still don't understand why the Shell of the LightweightSystem in PopUpHelper is being disposed by dragging the editor window back to the host? Is anything else being disposed at this point?
Phillipus
commented
2 months ago For now, I've done this in
PopUpHelperfor our app:protected Shell createShell() { Shell newShell = new Shell(control.getShell(), shellStyle); newShell.addDisposeListener(e -> { shell = null; lws = null; }); return newShell; }Not sure if there are any side effects, but it must be better than a
SWTException.
The problem with this approach is that when the editor is closed later and PopUpHelper#dispose is called, because the Shell has been set to null it is re-created in PopUpHelper#getShell()
Phillipus
commented
2 months ago The problem with this approach is that when the editor is closed later and PopUpHelper#dispose is called, because the Shell has been set to null it is re-created in PopUpHelper#getShell()
This can be fixed by changing ToolTipHelper#dispose to simply call super.dispose() (which is effectively the same)
Phillipus
commented
2 months ago Actually, the simplest solution so far that I found is changing this in SWTEventDispatcher:
protected ToolTipHelper getToolTipHelper() {
if(toolTipHelper == null || (toolTipHelper != null && toolTipHelper.getShell().isDisposed())) {
toolTipHelper = new ToolTipHelper(control);
}
return toolTipHelper;
}In fact, the exception is thrown on any Figure that has a tooltip and uses a LightweightSystem and SWTEventDispatcher, not just the palette.
So the steps to reproduce this:
The important thing is not to show a tooltip before detaching the Editor as mentioned here.
Phillipus
commented
2 months ago Another possible fix is to not set a parent Shell when creating the Shell in PopUpHelper, but rather the parent Display. So this:
protected Shell createShell() {
return new Shell(control.getShell(), shellStyle);
}is changed to this:
protected Shell createShell() {
return new Shell(control.getDisplay(), shellStyle);
}Another possible fix is to not set a parent
Shellwhen creating the Shell inPopUpHelper, but rather the parent Display. So this:protected Shell createShell() { return new Shell(control.getShell(), shellStyle); }is changed to this:
protected Shell createShell() { return new Shell(control.getDisplay(), shellStyle); }
I like that one the most. Do you want to create a PR? It'd feel wrong if I were to just take your suggestion and commit it myself.
Phillipus
commented
2 months ago I like that one the most.
So do I, but I'm not sure if there's any drawback to the Shell not having a parent Shell. In the case of a tooltip it floats above the parent Shell and is so transient that it might not matter. I tested it and it all seems to work OK.
Do you want to create a PR?
ptziegler
commented
2 months ago @Phillipus Thank you for looking at all the possible ways to solve this. The last one is a really clean fix which hopefully handles such use-cases for good.
Phillipus
commented
2 months ago @Phillipus Thank you for looking at all the possible ways to solve this. The last one is a really clean fix which hopefully handles such use-cases for good.
Also, thanks for your help in pin-pointing where the problem occurs.
In other cases of Tooltips the parent of the newly created Shell is the Control's Shell (see here for an example) but in our case we have to choose between checking for a disposed Shell or using the Display when accessing the tooltip's Shell. If the latter has no drawbacks that seems the better choice.