From edf17b48337ff67c522de1a8ecb0172d29925e3d Mon Sep 17 00:00:00 2001 From: Edrilan Berisha Date: Tue, 9 Jun 2026 01:14:25 +0200 Subject: [PATCH] Fix toolbar items enabled state not updating automatically (#3953) When an AbstractSourceProvider fires fireSourceChanged(), the EvaluationService contextUpdater listener was updating the legacy expression context variable but never sending REQUEST_ENABLEMENT_UPDATE_TOPIC on the event broker. As a result, ToolBarManagerRenderer (which listens for that topic) was never notified, so toolbar items relying on custom source providers with enabledWhen expressions stayed frozen until a focus change accidentally triggered an update. Why ratUpdater alone is insufficient: ratUpdater sends REQUEST_ENABLEMENT_UPDATE_TOPIC on every re-run, but it only re-runs when one of the variables in ratVariables changes. A variable enters ratVariables only when some handler or evaluation expression registered via addEvaluationListener references it. Toolbar items that use a custom source variable in enabledWhen but have no corresponding handler do not cause the variable to be tracked, so ratUpdater is never triggered by their source changes. Fix: send REQUEST_ENABLEMENT_UPDATE_TOPIC in contextUpdater after each source change originating from a registered ISourceProvider (both single-variable and multi-variable variants). This covers the gap left by ratUpdater and is consistent with requestEvaluation(), which already sends the event explicitly. Tests: rewrote the regression tests in EvaluationServiceTest to use a source provider with a variable intentionally absent from ratVariables. The previous tests used the "username" variable, which a registered handler's activeWhen expression already adds to ratVariables, so ratUpdater sent the event regardless of the fix and the tests passed without it. The new tests isolate the code path in contextUpdater that the fix introduced: - testUntrackedSourceProviderFiresEnablementUpdateEvent: single-variable form - testUntrackedSourceProviderMultiVarFiresEnablementUpdateEvent: Map-based form Fixes: https://github.com/eclipse-platform/eclipse.platform.ui/issues/3953 --- .../internal/services/EvaluationService.java | 2 + .../tests/services/EvaluationServiceTest.java | 137 ++++++++++++++++++ 2 files changed, 139 insertions(+) diff --git a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/services/EvaluationService.java b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/services/EvaluationService.java index 48248f13bcc..0830c4d38d2 100644 --- a/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/services/EvaluationService.java +++ b/bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/internal/services/EvaluationService.java @@ -113,6 +113,7 @@ public Object compute(IEclipseContext context, String contextKey) { @Override public void sourceChanged(int sourcePriority, String sourceName, Object sourceValue) { changeVariable(sourceName, sourceValue); + getEventBroker().send(UIEvents.REQUEST_ENABLEMENT_UPDATE_TOPIC, UIEvents.ALL_ELEMENT_ID); } @Override @@ -122,6 +123,7 @@ public void sourceChanged(int sourcePriority, Map sourceValuesByName) { final Map.Entry entry = (Entry) i.next(); changeVariable((String) entry.getKey(), entry.getValue()); } + getEventBroker().send(UIEvents.REQUEST_ENABLEMENT_UPDATE_TOPIC, UIEvents.ALL_ELEMENT_ID); } }; variableFilter.addAll(Arrays.asList(ISources.ACTIVE_WORKBENCH_WINDOW_NAME, ISources.ACTIVE_WORKBENCH_WINDOW_SHELL_NAME, ISources.ACTIVE_EDITOR_ID_NAME, ISources.ACTIVE_EDITOR_INPUT_NAME, ISources.SHOW_IN_INPUT, ISources.SHOW_IN_SELECTION, ISources.ACTIVE_PART_NAME, ISources.ACTIVE_PART_ID_NAME, ISources.ACTIVE_SITE_NAME, ISources.ACTIVE_CONTEXT_NAME, ISources.ACTIVE_CURRENT_SELECTION_NAME)); diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/services/EvaluationServiceTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/services/EvaluationServiceTest.java index c5a47f28b9f..76140ea0c9f 100644 --- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/services/EvaluationServiceTest.java +++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/services/EvaluationServiceTest.java @@ -30,6 +30,8 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.core.expressions.EvaluationResult; @@ -50,6 +52,7 @@ import org.eclipse.jface.viewers.StructuredSelection; import org.eclipse.jface.viewers.TreePath; import org.eclipse.jface.viewers.TreeSelection; +import org.eclipse.ui.AbstractSourceProvider; import org.eclipse.ui.IPerspectiveDescriptor; import org.eclipse.ui.IPerspectiveRegistry; import org.eclipse.ui.ISources; @@ -62,6 +65,8 @@ import org.eclipse.ui.handlers.IHandlerService; import org.eclipse.ui.internal.WorkbenchWindow; import org.eclipse.ui.internal.handlers.HandlerPersistence; +import org.eclipse.e4.core.services.events.IEventBroker; +import org.eclipse.e4.ui.workbench.UIEvents; import org.eclipse.ui.services.IEvaluationReference; import org.eclipse.ui.services.IEvaluationService; import org.eclipse.ui.services.ISourceProviderService; @@ -711,6 +716,98 @@ public void testWorkbenchProvider() throws Exception { } } + /** + * Regression test for bug #3953 — toolbar items enabled state stays frozen + * when a custom source provider whose variable is not tracked by any evaluation + * expression fires a source change. + *

+ * When a variable is absent from {@code ratVariables} (because no handler or + * evaluation listener references it), {@code ratUpdater} is not triggered by + * that variable changing. The fix in {@code contextUpdater} sends + * {@code REQUEST_ENABLEMENT_UPDATE_TOPIC} unconditionally so that + * {@code ToolBarManagerRenderer} can re-evaluate toolbar-item expressions. + *

+ * The variable name used here is intentionally unique and not referenced by any + * handler or expression in this test bundle. This isolates the path in + * {@code contextUpdater.sourceChanged(int, String, Object)} that the fix + * introduced, which {@code ratUpdater} alone cannot cover. + */ + @Test + public void testUntrackedSourceProviderFiresEnablementUpdateEvent() throws Exception { + IWorkbenchWindow window = openTestWindow(); + IEvaluationService service = window.getService(IEvaluationService.class); + assertNotNull(service); + + IEventBroker eventBroker = window.getWorkbench().getService(IEventBroker.class); + assertNotNull("IEventBroker service must be available", eventBroker); + + // Variable name guaranteed absent from ratVariables: no plugin.xml handler + // or addEvaluationListener call in this bundle references it. + TestSourceProvider provider = new TestSourceProvider("test.untracked.singleVar", "initial"); + service.addSourceProvider(provider); + + final int[] count = { 0 }; + org.osgi.service.event.EventHandler eventHandler = e -> count[0]++; + boolean subscribed = eventBroker.subscribe(UIEvents.REQUEST_ENABLEMENT_UPDATE_TOPIC, eventHandler); + assertTrue("Should subscribe to REQUEST_ENABLEMENT_UPDATE_TOPIC", subscribed); + + try { + processEvents(); + int before = count[0]; + + provider.fireValue("changed"); + processEvents(); + + assertTrue( + "fireSourceChanged() with a variable absent from ratVariables must send " + + "REQUEST_ENABLEMENT_UPDATE_TOPIC so toolbar items re-evaluate (bug #3953)", + count[0] > before); + } finally { + eventBroker.unsubscribe(eventHandler); + service.removeSourceProvider(provider); + } + } + + /** + * Verifies that the Map-based {@code sourceChanged} overload also sends + * {@code REQUEST_ENABLEMENT_UPDATE_TOPIC} for variables absent from + * {@code ratVariables}. + */ + @Test + public void testUntrackedSourceProviderMultiVarFiresEnablementUpdateEvent() throws Exception { + IWorkbenchWindow window = openTestWindow(); + IEvaluationService service = window.getService(IEvaluationService.class); + assertNotNull(service); + + IEventBroker eventBroker = window.getWorkbench().getService(IEventBroker.class); + assertNotNull(eventBroker); + + TestSourceProvider provider = new TestSourceProvider("test.untracked.multiVar", "initial"); + service.addSourceProvider(provider); + + final int[] count = { 0 }; + org.osgi.service.event.EventHandler eventHandler = e -> count[0]++; + eventBroker.subscribe(UIEvents.REQUEST_ENABLEMENT_UPDATE_TOPIC, eventHandler); + + try { + processEvents(); + int before = count[0]; + + Map changes = new HashMap<>(); + changes.put("test.untracked.multiVar", "changed"); + provider.fireValues(changes); + processEvents(); + + assertTrue( + "Map-based fireSourceChanged() with an untracked variable must send " + + "REQUEST_ENABLEMENT_UPDATE_TOPIC (bug #3953)", + count[0] > before); + } finally { + eventBroker.unsubscribe(eventHandler); + service.removeSourceProvider(provider); + } + } + private void assertSelection(final ArrayList selection, int callIdx, Class clazz, String viewId) { assertEquals(callIdx + 1, selection.size()); assertEquals(clazz, getSelection(selection, callIdx) @@ -725,4 +822,44 @@ private ISelection getSelection(final ArrayList selection, int id private IWorkbenchPart getPart(final ArrayList selection, int idx) { return selection.get(idx).part; } + + /** Source provider with a configurable variable name for use in isolated tests. */ + private static class TestSourceProvider extends AbstractSourceProvider { + private final String varName; + private String value; + + TestSourceProvider(String varName, String initial) { + this.varName = varName; + this.value = initial; + } + + void fireValue(String newValue) { + value = newValue; + fireSourceChanged(ISources.ACTIVE_CONTEXT << 1, varName, newValue); + } + + @SuppressWarnings("rawtypes") + void fireValues(Map map) { + map.forEach((k, v) -> { + if (k.equals(varName)) value = (String) v; + }); + fireSourceChanged(ISources.ACTIVE_CONTEXT << 1, (Map) map); + } + + @Override + public Map getCurrentState() { + Map m = new HashMap<>(); + m.put(varName, value); + return m; + } + + @Override + public String[] getProvidedSourceNames() { + return new String[] { varName }; + } + + @Override + public void dispose() { + } + } }