Skip to content

Commit 5b62380

Browse files
basilevsmickaelistria
basilevs
authored and
mickaelistria
committed
Restore command state support #925
Restore state support for command handlers as documented in IObjectWithState Javadoc. See https://bugs.eclipse.org/bugs/show_bug.cgi?id=549802 Fixes #925 Command state support was lost during migration on E4 as proxies failed to properly support IObjectWithState protocol or ignored is altogether. - HandlerProxy - HandlerServiceHandler - E4HandlerProxy do now support the protocol. Following tests are now reenabled: - StateTest - ToggleStateTest Additional integration test added: - WorkbenchStateTest
1 parent 8c7adc3 commit 5b62380

File tree

13 files changed

+582
-61
lines changed

13 files changed

+582
-61
lines changed

‎bundles/org.eclipse.core.commands/src/org/eclipse/core/commands/AbstractHandlerWithState.java

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,14 @@ public void addState(final String stateId, final State state) {
6868
if (states == null) {
6969
states = new HashMap<>(3);
7070
}
71-
states.put(stateId, state);
71+
State oldState = states.put(stateId, state);
7272
state.addListener(this);
73-
handleStateChange(state, null);
73+
if (oldState != null) {
74+
oldState.removeListener(this);
75+
handleStateChange(state, oldState.getValue());
76+
} else {
77+
handleStateChange(state, null);
78+
}
7479
}
7580

7681
@Override
@@ -126,4 +131,12 @@ public void removeState(final String stateId) {
126131
}
127132
}
128133
}
134+
135+
@Override
136+
public void dispose() {
137+
for (String id : getStateIds()) {
138+
removeState(id);
139+
}
140+
super.dispose();
141+
}
129142
}

‎bundles/org.eclipse.e4.core.commands/src/org/eclipse/e4/core/commands/internal/HandlerServiceHandler.java

Lines changed: 79 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,14 @@
1414

1515
package org.eclipse.e4.core.commands.internal;
1616

17-
import org.eclipse.core.commands.AbstractHandler;
17+
import org.eclipse.core.commands.AbstractHandlerWithState;
1818
import org.eclipse.core.commands.ExecutionEvent;
1919
import org.eclipse.core.commands.ExecutionException;
2020
import org.eclipse.core.commands.HandlerEvent;
2121
import org.eclipse.core.commands.IHandler;
22+
import org.eclipse.core.commands.IObjectWithState;
2223
import org.eclipse.core.commands.NotHandledException;
24+
import org.eclipse.core.commands.State;
2325
import org.eclipse.core.expressions.IEvaluationContext;
2426
import org.eclipse.e4.core.commands.ExpressionContext;
2527
import org.eclipse.e4.core.commands.internal.HandlerServiceImpl.ExecutionContexts;
@@ -33,13 +35,15 @@
3335
/**
3436
* Provide an IHandler to delegate calls to.
3537
*/
36-
public class HandlerServiceHandler extends AbstractHandler {
38+
public class HandlerServiceHandler extends AbstractHandlerWithState {
3739

3840
private static final String FAILED_TO_FIND_HANDLER_DURING_EXECUTION = "Failed to find handler during execution"; //$NON-NLS-1$
3941
private static final String HANDLER_MISSING_EXECUTE_ANNOTATION = " handler is missing @Execute"; //$NON-NLS-1$
4042
private static final Object missingExecute = new Object();
4143

4244
protected final String commandId;
45+
// Remove state from currentStateHandler when it goes out of scope
46+
private IObjectWithState currentStateHandler;
4347

4448
public HandlerServiceHandler(String commandId) {
4549
this.commandId = commandId;
@@ -54,6 +58,7 @@ public boolean isEnabled() {
5458
return super.isEnabled();
5559
}
5660
Object handler = HandlerServiceImpl.lookUpHandler(executionContext, commandId);
61+
switchHandler(handler);
5762
if (handler == null) {
5863
setBaseEnabled(false);
5964
return super.isEnabled();
@@ -73,6 +78,7 @@ public void setEnabled(Object evaluationContext) {
7378
return;
7479
}
7580
Object handler = HandlerServiceImpl.lookUpHandler(executionContext, commandId);
81+
switchHandler(handler);
7682
if (handler == null) {
7783
return;
7884
}
@@ -124,6 +130,7 @@ public boolean isHandled() {
124130
ExecutionContexts contexts = HandlerServiceImpl.peek();
125131
if (contexts != null) {
126132
Object handler = HandlerServiceImpl.lookUpHandler(contexts.context, commandId);
133+
switchHandler(handler);
127134
if (handler instanceof IHandler) {
128135
return ((IHandler) handler).isHandled();
129136
}
@@ -142,6 +149,7 @@ public Object execute(ExecutionEvent event) throws ExecutionException {
142149
}
143150

144151
Object handler = HandlerServiceImpl.lookUpHandler(executionContext, commandId);
152+
switchHandler(handler);
145153
if (handler == null) {
146154
return null;
147155
}
@@ -188,4 +196,73 @@ public void overrideEnabled(boolean b) {
188196
public String toString() {
189197
return this.getClass().getSimpleName() + "(\"" + commandId + "\")"; //$NON-NLS-1$//$NON-NLS-2$
190198
}
199+
200+
@Override
201+
public void addState(String stateId, State state) {
202+
super.addState(stateId, state);
203+
IObjectWithState handler = lookUpHandlerWithState();
204+
if (handler != null) {
205+
handler.addState(stateId, state);
206+
}
207+
}
208+
209+
@Override
210+
public void removeState(String stateId) {
211+
IObjectWithState handler = lookUpHandlerWithState();
212+
if (handler != null) {
213+
handler.removeState(stateId);
214+
}
215+
super.removeState(stateId);
216+
}
217+
218+
@Override
219+
public void handleStateChange(State state, Object oldValue) {
220+
}
221+
222+
@Override
223+
public void dispose() {
224+
switchHandler(null);
225+
super.dispose();
226+
}
227+
228+
private IObjectWithState lookUpHandlerWithState() {
229+
ExecutionContexts contexts = HandlerServiceImpl.peek();
230+
if (contexts == null) {
231+
return null;
232+
}
233+
Object handler = HandlerServiceImpl.lookUpHandler(contexts.context, commandId);
234+
switchHandler(handler);
235+
if (!(handler instanceof IObjectWithState)) {
236+
return null;
237+
}
238+
if (handler instanceof IHandler) {
239+
if (!((IHandler) handler).isHandled()) {
240+
return null;
241+
}
242+
}
243+
return (IObjectWithState) handler;
244+
}
245+
246+
private void switchHandler(Object handler) {
247+
IObjectWithState typed;
248+
if (handler instanceof IObjectWithState) {
249+
typed = (IObjectWithState) handler;
250+
} else {
251+
typed = null;
252+
}
253+
IObjectWithState oldHandler = currentStateHandler;
254+
if (oldHandler == typed) {
255+
return;
256+
}
257+
currentStateHandler = typed;
258+
for (String id : getStateIds()) {
259+
if (oldHandler != null) {
260+
oldHandler.removeState(id);
261+
}
262+
if (typed != null) {
263+
typed.addState(id, getState(id));
264+
}
265+
}
266+
}
267+
191268
}

‎bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/handlers/E4HandlerProxy.java

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@
2424
import org.eclipse.core.commands.IHandler;
2525
import org.eclipse.core.commands.IHandler2;
2626
import org.eclipse.core.commands.IHandlerListener;
27+
import org.eclipse.core.commands.IObjectWithState;
2728
import org.eclipse.core.commands.NotHandledException;
29+
import org.eclipse.core.commands.State;
2830
import org.eclipse.core.expressions.IEvaluationContext;
2931
import org.eclipse.core.runtime.IStatus;
3032
import org.eclipse.core.runtime.Status;
@@ -50,7 +52,7 @@
5052
* @since 3.5
5153
*
5254
*/
53-
public class E4HandlerProxy implements IHandler2, IHandlerListener, IElementUpdater {
55+
public class E4HandlerProxy implements IHandler2, IHandlerListener, IElementUpdater, IObjectWithState {
5456
public HandlerActivation activation;
5557
private final Command command;
5658
private final IHandler handler;
@@ -195,4 +197,34 @@ public String toString() {
195197
return builder.toString();
196198
}
197199

200+
@Override
201+
public void addState(String id, State state) {
202+
if (handler instanceof IObjectWithState) {
203+
((IObjectWithState) handler).addState(id, state);
204+
}
205+
}
206+
207+
@Override
208+
public State getState(String stateId) {
209+
if (handler instanceof IObjectWithState) {
210+
return ((IObjectWithState) handler).getState(stateId);
211+
}
212+
return null;
213+
}
214+
215+
@Override
216+
public String[] getStateIds() {
217+
if (handler instanceof IObjectWithState) {
218+
return ((IObjectWithState) handler).getStateIds();
219+
}
220+
return new String[0];
221+
}
222+
223+
@Override
224+
public void removeState(String stateId) {
225+
if (handler instanceof IObjectWithState) {
226+
((IObjectWithState) handler).removeState(stateId);
227+
}
228+
}
229+
198230
}

‎bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/handlers/HandlerProxy.java

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
import org.eclipse.core.commands.IHandler;
2525
import org.eclipse.core.commands.IHandler2;
2626
import org.eclipse.core.commands.IHandlerListener;
27-
import org.eclipse.core.commands.IStateListener;
27+
import org.eclipse.core.commands.IObjectWithState;
2828
import org.eclipse.core.commands.State;
2929
import org.eclipse.core.expressions.EvaluationResult;
3030
import org.eclipse.core.expressions.Expression;
@@ -341,6 +341,11 @@ private boolean loadHandler() {
341341
if (configurationElement != null) {
342342
handler = (IHandler) configurationElement.createExecutableExtension(handlerAttributeName);
343343
handler.addHandlerListener(getHandlerListener());
344+
if (handler instanceof IObjectWithState) {
345+
for (String id : getStateIds()) {
346+
((IObjectWithState) handler).addState(id, getState(id));
347+
}
348+
}
344349
setEnabled(evaluationService == null ? null : evaluationService.getCurrentState());
345350
refreshElements();
346351
return true;
@@ -453,9 +458,6 @@ public void handleStateChange(State state, Object oldValue) {
453458
radioState = state;
454459
refreshElements();
455460
}
456-
if (handler instanceof IStateListener) {
457-
((IStateListener) handler).handleStateChange(state, oldValue);
458-
}
459461
}
460462

461463
/**
@@ -475,4 +477,20 @@ public String getAttributeName() {
475477
public IHandler getHandler() {
476478
return handler;
477479
}
480+
481+
@Override
482+
public void addState(String stateId, State state) {
483+
super.addState(stateId, state);
484+
if (handler instanceof IObjectWithState) {
485+
((IObjectWithState) handler).addState(stateId, state);
486+
}
487+
}
488+
489+
@Override
490+
public void removeState(String stateId) {
491+
if (handler instanceof IObjectWithState) {
492+
((IObjectWithState) handler).removeState(stateId);
493+
}
494+
super.removeState(stateId);
495+
}
478496
}

‎tests/org.eclipse.ui.genericeditor.tests/src/org/eclipse/ui/genericeditor/tests/HoverTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ public void testProblemHover() throws Exception {
127127
marker.setAttribute(MarkerResolutionGenerator.FIXME, true);
128128
AbstractInformationControlManager manager= triggerCompletionAndRetrieveInformationControlManager();
129129
Object hoverData= getHoverData(manager);
130-
assertTrue(hoverData instanceof Map);
130+
assertTrue(""+hoverData, hoverData instanceof Map);
131131
assertTrue(((Map<?, ?>) hoverData).containsValue(Collections.singletonList(marker)));
132132
assertTrue(((Map<?, ?>) hoverData).containsValue(AlrightyHoverProvider.LABEL));
133133
assertFalse(((Map<?, ?>) hoverData).containsValue(HelloHoverProvider.LABEL));

‎tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/commands/CommandsTestSuite.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
ActionDelegateProxyTest.class,
4444
ToggleStateTest.class,
4545
RadioStateTest.class,
46+
WorkbenchStateTest.class,
4647
E4CommandImageTest.class
4748
})
4849
public final class CommandsTestSuite {

‎tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/commands/RadioStateTest.java

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,23 @@
1414

1515
package org.eclipse.ui.tests.commands;
1616

17+
import static org.junit.Assert.assertEquals;
18+
import static org.junit.Assert.assertTrue;
19+
1720
import org.eclipse.core.commands.Command;
1821
import org.eclipse.core.commands.Parameterization;
1922
import org.eclipse.core.commands.ParameterizedCommand;
2023
import org.eclipse.core.commands.State;
2124
import org.eclipse.jface.resource.ImageDescriptor;
25+
import org.eclipse.ui.IWorkbench;
26+
import org.eclipse.ui.PlatformUI;
2227
import org.eclipse.ui.commands.ICommandService;
2328
import org.eclipse.ui.commands.IElementReference;
2429
import org.eclipse.ui.handlers.IHandlerService;
2530
import org.eclipse.ui.handlers.RadioState;
2631
import org.eclipse.ui.menus.UIElement;
2732
import org.eclipse.ui.services.IServiceLocator;
28-
import org.eclipse.ui.tests.harness.util.UITestCase;
29-
import org.junit.Ignore;
33+
import org.junit.Before;
3034
import org.junit.Test;
3135
import org.junit.runner.RunWith;
3236
import org.junit.runners.JUnit4;
@@ -37,19 +41,15 @@
3741
*
3842
*/
3943
@RunWith(JUnit4.class)
40-
@Ignore("broke during e4 transition and still need adjustments")
41-
public class RadioStateTest extends UITestCase {
44+
public class RadioStateTest {
45+
46+
private final IWorkbench fWorkbench = PlatformUI.getWorkbench();
4247

4348
private ICommandService commandService;
4449
private IHandlerService handlerService;
4550

46-
public RadioStateTest(String testName) {
47-
super(testName);
48-
}
49-
50-
@Override
51-
protected void doSetUp() throws Exception {
52-
super.doSetUp();
51+
@Before
52+
public void doSetUp() throws Exception {
5353
commandService = fWorkbench
5454
.getService(ICommandService.class);
5555
handlerService = fWorkbench

0 commit comments

Comments
 (0)