Skip to content

Commit c1012fb

Browse files
ptzieglerazoitl
ptziegler
authored and
azoitl
committed
Make DirectEditManager more robust when brought down multiple times #757
When bringDown() is called while processing a "direct edit" command, an IllegalArgumentException is thrown because the edit-part listener is removed twice. This is because the listener is null in the second round, which is not a valid parameter to pass to the underlying EventListenerList of the edit part. An explicit null-check has been added to the DirectEditManager to avoid this issue.
1 parent 5568b07 commit c1012fb

File tree

5 files changed

+149
-143
lines changed

5 files changed

+149
-143
lines changed

‎org.eclipse.gef.tests/src/org/eclipse/gef/test/DirectEditManagerTest.java‎

Lines changed: 93 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@
1111

1212
import static org.junit.jupiter.api.Assertions.assertTrue;
1313

14+
import java.util.ArrayList;
1415
import java.util.Collection;
16+
import java.util.List;
1517
import java.util.concurrent.atomic.AtomicBoolean;
1618

1719
import org.eclipse.swt.widgets.Composite;
@@ -22,45 +24,84 @@
2224
import org.eclipse.jface.viewers.ComboBoxCellEditor;
2325
import org.eclipse.ui.PlatformUI;
2426

25-
import org.eclipse.draw2d.Figure;
2627
import org.eclipse.draw2d.IFigure;
2728
import org.eclipse.draw2d.geometry.Point;
2829

2930
import org.eclipse.gef.EditDomain;
3031
import org.eclipse.gef.EditPart;
32+
import org.eclipse.gef.EditPartViewer;
3133
import org.eclipse.gef.GraphicalEditPart;
3234
import org.eclipse.gef.Request;
3335
import org.eclipse.gef.commands.Command;
34-
import org.eclipse.gef.editparts.AbstractGraphicalEditPart;
36+
import org.eclipse.gef.test.utils.TestGraphicalEditPart;
3537
import org.eclipse.gef.tools.DirectEditManager;
3638
import org.eclipse.gef.ui.parts.AbstractEditPartViewer;
3739

40+
import org.junit.jupiter.api.BeforeEach;
3841
import org.junit.jupiter.api.Test;
3942

4043
public class DirectEditManagerTest {
44+
private Shell shell;
45+
private EditDomain editDomain;
46+
private TestDirectEditManager editManager;
47+
private List<Throwable> throwables;
48+
49+
@BeforeEach
50+
public void setUp() {
51+
shell = PlatformUI.getWorkbench().getDisplay()
52+
.syncCall(() -> PlatformUI.getWorkbench().getDisplay().getActiveShell());
53+
editDomain = new EditDomain();
54+
throwables = new ArrayList<>();
55+
editManager = null;
56+
}
4157

42-
@SuppressWarnings("static-method")
4358
@Test
4459
public void testComboCellEditorChangeProcessed() {
45-
Shell shell = PlatformUI.getWorkbench().getDisplay()
46-
.syncCall(() -> PlatformUI.getWorkbench().getDisplay().getActiveShell());
47-
EditDomain editDomain = new EditDomain();
48-
TestDirectEditManager editManager = new TestDirectEditManager(createDummyEditPart(shell, editDomain));
4960
AtomicBoolean comboChangeCommandSubmitted = new AtomicBoolean();
5061
editDomain.getCommandStack().addCommandStackEventListener(event -> comboChangeCommandSubmitted.set(true));
5162

63+
testWith(new DirectGraphicalEditPart() {
64+
@Override
65+
public Command getCommand(Request request) {
66+
return new Command() {
67+
};
68+
}
69+
});
70+
71+
assertTrue(comboChangeCommandSubmitted.get());
72+
assertTrue(throwables.isEmpty(), () -> "Internal exception occurred: " + throwables); //$NON-NLS-1$
73+
}
74+
75+
@Test
76+
public void testComboCellEditorDisposedTwice() {
77+
testWith(new DirectGraphicalEditPart() {
78+
@Override
79+
public Command getCommand(Request request) {
80+
return new Command() {
81+
@Override
82+
public void execute() {
83+
editManager.bringDown();
84+
}
85+
};
86+
}
87+
});
88+
89+
assertTrue(throwables.isEmpty(), () -> "Internal exception occurred: " + throwables); //$NON-NLS-1$
90+
}
91+
92+
private void testWith(GraphicalEditPart editPart) {
93+
editManager = new TestDirectEditManager(editPart);
94+
5295
PlatformUI.getWorkbench().getDisplay().syncExec(() -> {
5396
editManager.show();
5497
// Change value of combo cell editor
5598
editManager.getCellEditor().setValue(0);
5699
// Make combo cell editor lose focus to apply its value
57100
shell.setFocus();
58101
});
59-
60-
assertTrue(comboChangeCommandSubmitted.get());
61102
}
62103

63-
private static class TestDirectEditManager extends DirectEditManager {
104+
private class TestDirectEditManager extends DirectEditManager {
64105
public TestDirectEditManager(GraphicalEditPart source) {
65106
super(source, ComboBoxCellEditor.class, cellEditor -> {
66107
});
@@ -73,7 +114,22 @@ protected CellEditor createCellEditorOn(Composite composite) {
73114

74115
@Override
75116
public CellEditor getCellEditor() {
76-
return super.getCellEditor();
117+
try {
118+
return super.getCellEditor();
119+
} catch (Throwable exception) {
120+
throwables.add(exception);
121+
throw exception;
122+
}
123+
}
124+
125+
@Override
126+
public void bringDown() {
127+
try {
128+
super.bringDown();
129+
} catch (Throwable exception) {
130+
throwables.add(exception);
131+
throw exception;
132+
}
77133
}
78134

79135
@Override
@@ -85,49 +141,32 @@ protected void initCellEditor() {
85141
}
86142
}
87143

88-
private static GraphicalEditPart createDummyEditPart(Control control, EditDomain editDomain) {
89-
return new AbstractGraphicalEditPart() {
90-
@Override
91-
protected void createEditPolicies() {
92-
}
93-
94-
@Override
95-
public org.eclipse.gef.EditPartViewer getViewer() {
96-
return new AbstractEditPartViewer() {
97-
@Override
98-
public EditPart findObjectAtExcluding(Point location, Collection<IFigure> exclusionSet,
99-
Conditional conditional) {
100-
return null;
101-
}
102-
103-
@Override
104-
public Control createControl(Composite parent) {
105-
return parent;
106-
}
107-
108-
@Override
109-
public Control getControl() {
110-
return control;
111-
}
112-
113-
@Override
114-
public org.eclipse.gef.EditDomain getEditDomain() {
115-
return editDomain;
116-
}
117-
};
118-
}
119-
120-
@Override
121-
public Command getCommand(Request request) {
122-
return new Command() {
123-
};
124-
}
125-
126-
@Override
127-
protected IFigure createFigure() {
128-
return new Figure();
129-
}
130-
};
144+
private class DirectGraphicalEditPart extends TestGraphicalEditPart {
145+
@Override
146+
public EditPartViewer getViewer() {
147+
return new AbstractEditPartViewer() {
148+
@Override
149+
public EditPart findObjectAtExcluding(Point location, Collection<IFigure> exclusionSet,
150+
Conditional conditional) {
151+
return null;
152+
}
153+
154+
@Override
155+
public Control createControl(Composite parent) {
156+
return parent;
157+
}
158+
159+
@Override
160+
public Control getControl() {
161+
return shell;
162+
}
163+
164+
@Override
165+
public EditDomain getEditDomain() {
166+
return editDomain;
167+
}
168+
};
169+
}
131170
}
132171

133172
}

‎org.eclipse.gef.tests/src/org/eclipse/gef/test/DragEditPartsTrackerTest.java‎

Lines changed: 2 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2006, 2024 IBM Corporation and others.
2+
* Copyright (c) 2006, 2025 IBM Corporation and others.
33
*
44
* This program and the accompanying materials are made available under the
55
* terms of the Eclipse Public License 2.0 which is available at
@@ -27,51 +27,15 @@
2727
import org.eclipse.ui.IWorkbenchPartSite;
2828
import org.eclipse.ui.PartInitException;
2929

30-
import org.eclipse.draw2d.Figure;
31-
import org.eclipse.draw2d.IFigure;
32-
3330
import org.eclipse.gef.DefaultEditDomain;
3431
import org.eclipse.gef.EditPart;
35-
import org.eclipse.gef.editparts.AbstractGraphicalEditPart;
32+
import org.eclipse.gef.test.utils.TestGraphicalEditPart;
3633
import org.eclipse.gef.tools.DragEditPartsTracker;
3734

3835
import org.junit.jupiter.api.Test;
3936

4037
public class DragEditPartsTrackerTest {
4138

42-
private static class TestGraphicalEditPart extends AbstractGraphicalEditPart {
43-
44-
/*
45-
* (non-Javadoc)
46-
*
47-
* @see org.eclipse.gef.editparts.AbstractEditPart#register()
48-
*/
49-
@Override
50-
protected void register() {
51-
// do nothing
52-
}
53-
54-
/*
55-
* (non-Javadoc)
56-
*
57-
* @see org.eclipse.gef.editparts.AbstractGraphicalEditPart#createFigure()
58-
*/
59-
@Override
60-
protected IFigure createFigure() {
61-
return new Figure();
62-
}
63-
64-
/*
65-
* (non-Javadoc)
66-
*
67-
* @see org.eclipse.gef.editparts.AbstractEditPart#createEditPolicies()
68-
*/
69-
@Override
70-
protected void createEditPolicies() {
71-
// do nothing
72-
}
73-
}
74-
7539
private class DummyEditorPart implements org.eclipse.ui.IEditorPart {
7640

7741
@Override

‎org.eclipse.gef.tests/src/org/eclipse/gef/test/ToolUtilitiesTest.java‎

Lines changed: 2 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*******************************************************************************
2-
* Copyright (c) 2006, 2024 IBM Corporation and others.
2+
* Copyright (c) 2006, 2025 IBM Corporation and others.
33
*
44
* This program and the accompanying materials are made available under the
55
* terms of the Eclipse Public License 2.0 which is available at
@@ -15,59 +15,14 @@
1515

1616
import static org.junit.jupiter.api.Assertions.assertTrue;
1717

18-
import org.eclipse.draw2d.Figure;
19-
import org.eclipse.draw2d.IFigure;
20-
2118
import org.eclipse.gef.EditPart;
22-
import org.eclipse.gef.editparts.AbstractGraphicalEditPart;
19+
import org.eclipse.gef.test.utils.TestGraphicalEditPart;
2320
import org.eclipse.gef.tools.ToolUtilities;
2421

2522
import org.junit.jupiter.api.Test;
2623

2724
public class ToolUtilitiesTest {
2825

29-
private static class TestGraphicalEditPart extends AbstractGraphicalEditPart {
30-
31-
/*
32-
* (non-Javadoc)
33-
*
34-
* @see org.eclipse.gef.editparts.AbstractGraphicalEditPart#activate()
35-
*/
36-
public void addChild(EditPart ep) {
37-
addChild(ep, 0);
38-
}
39-
40-
/*
41-
* (non-Javadoc)
42-
*
43-
* @see org.eclipse.gef.editparts.AbstractEditPart#register()
44-
*/
45-
@Override
46-
protected void register() {
47-
// do nothing
48-
}
49-
50-
/*
51-
* (non-Javadoc)
52-
*
53-
* @see org.eclipse.gef.editparts.AbstractGraphicalEditPart#createFigure()
54-
*/
55-
@Override
56-
protected IFigure createFigure() {
57-
return new Figure();
58-
}
59-
60-
/*
61-
* (non-Javadoc)
62-
*
63-
* @see org.eclipse.gef.editparts.AbstractEditPart#createEditPolicies()
64-
*/
65-
@Override
66-
protected void createEditPolicies() {
67-
// do nothing
68-
}
69-
}
70-
7126
@SuppressWarnings("static-method")
7227
@Test
7328
public void testFindCommonAncestorHappypath() {

‎org.eclipse.gef.tests/src/org/eclipse/gef/test/utils/TestGraphicalEditPart.java‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2006, 2025 IBM Corporation and others.
3+
*
4+
* This program and the accompanying materials are made available under the
5+
* terms of the Eclipse Public License 2.0 which is available at
6+
* http://www.eclipse.org/legal/epl-2.0.
7+
*
8+
* SPDX-License-Identifier: EPL-2.0
9+
*
10+
* Contributors:
11+
* IBM Corporation - initial API and implementation
12+
*******************************************************************************/
13+
package org.eclipse.gef.test.utils;
14+
15+
import org.eclipse.draw2d.Figure;
16+
import org.eclipse.draw2d.IFigure;
17+
18+
import org.eclipse.gef.EditPart;
19+
import org.eclipse.gef.editparts.AbstractGraphicalEditPart;
20+
21+
public class TestGraphicalEditPart extends AbstractGraphicalEditPart {
22+
23+
/**
24+
* @see org.eclipse.gef.editparts.AbstractGraphicalEditPart#activate()
25+
*/
26+
public void addChild(EditPart ep) {
27+
addChild(ep, 0);
28+
}
29+
30+
@Override
31+
protected void register() {
32+
// do nothing
33+
}
34+
35+
@Override
36+
protected IFigure createFigure() {
37+
return new Figure();
38+
}
39+
40+
@Override
41+
protected void createEditPolicies() {
42+
// do nothing
43+
}
44+
}

‎org.eclipse.gef/src/org/eclipse/gef/tools/DirectEditManager.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -440,10 +440,14 @@ public void showFeedback() {
440440
* Unhooks listeners. Called from {@link #bringDown()}.
441441
*/
442442
protected void unhookListeners() {
443-
getEditPart().getFigure().removeAncestorListener(ancestorListener);
444-
getEditPart().removeEditPartListener(editPartListener);
445-
ancestorListener = null;
446-
editPartListener = null;
443+
if (ancestorListener != null) {
444+
getEditPart().getFigure().removeAncestorListener(ancestorListener);
445+
ancestorListener = null;
446+
}
447+
if (editPartListener != null) {
448+
getEditPart().removeEditPartListener(editPartListener);
449+
editPartListener = null;
450+
}
447451

448452
if (getCellEditor() == null) {
449453
return;

0 commit comments

Comments
 (0)