LightweightSystem#setUpdateManager does't dispose old UpdateManager #605
Description
Based on https://bugs.eclipse.org/bugs/show_bug.cgi?id=250255
Steps To Reproduce:
public class EditableFigureCanvasTest extends TestCase {
private static final class TestUpdateManager extends UpdateManager {
@Override
public void addDirtyRegion(IFigure figure, int x, int y, int w, int h) {
}
@Override
public void addInvalidFigure(IFigure figure) {
}
@Override
public void performUpdate() {
}
@Override
public void performUpdate(Rectangle exposed) {
}
@Override
public void setGraphicsSource(GraphicsSource gs) {
}
@Override
public void setRoot(IFigure figure) {
}
@Override
public boolean isDisposed() {
return super.isDisposed();
}
}
@Test
public void test_setUpdateManager() {
//test if old updateManager is disposed
Display.getDefault().syncExec(new Runnable() {
public void run() {
//test if bug still exists
LightweightSystem lightweightSystem = new LightweightSystem();
TestUpdateManager um = new TestUpdateManager();
lightweightSystem.setUpdateManager(um);
lightweightSystem.setUpdateManager(new TestUpdateManager());
assertFalse("bug is fixed, remove workaround",um.isDisposed());
}
});
}
}To Produce a widget is disposed exception setup a canvas:
- create a canvas
- add an update listener to the UpdateManager
- access the canvas inside listener
- dispose canvas after replacing the UpdateManager
More information:
If the widget is disposed after setting the new UpdateManager, the old UpdateManager is not disposed and will execute its pending updates and validations. This will result in a widget is disposed error.
Generally, I don't think disposing the old UpdateManager is a good idea, as we have no idea where else it might be used (perhaps a different LWS?). Instead, I think it's sufficient to simply remove UpdateListener from the old manager to solve this problem.
After a quick glance, I noticed that there is a conceptional problem with replacing the UpdateManager. The UpdateManager is hooked to the FigureCanvas upon creation. But this is only done for the initial manager, meaning that the new UpdateManager won't receive any of the new events and any further updates are lost once the old one is disposed.
gef-classic/org.eclipse.draw2d/src/org/eclipse/draw2d/FigureCanvas.java
Lines 262 to 274 in 9974631
A more prudent approach would be to have the LightweightSystem fire an event, whenever the update manager is replaced. The FigureCanvas listens to this event and then updates the listener accordingly. Any listeners added outside Draw2D have to be removed by the client (perhaps even via the same event).