From b3434db2a32a8c229738e98e160a0900537707b0 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Tue, 5 Sep 2023 16:58:12 +0200 Subject: [PATCH] Dispose activity management during shutdown completely and early #1084 The WorkbenchActivitySupport is currently disabled quite late during workbench shutdown. In particular, it is performed after the service locator disposal, which produces several events for the disablement of expression-controlled activities that are processed by the activity management. Since the activity management is shut down afterwards anyway, the processing of these events is unnecessary. This change ensures that activities enablement changes are not unnecessarily processed during workbench shutdown. It consists of two parts: 1. It performs the disposal of activity management and its persistence handler early in the workbench shutdown process (in particular before the service locator disposal). 2. It adds proper dispose functionality for the IActivityManager, such that disposing the workbench activity support disposes the activity manager, which then removes all its listeners to not react to further events. Contributes to https://github.com/eclipse-platform/eclipse.platform.ui/issues/1084. --- .../ui/activities/IActivityManager.java | 9 +++ .../org/eclipse/ui/internal/Workbench.java | 17 ++++-- .../activities/AbstractActivityManager.java | 13 +++++ .../activities/MutableActivityManager.java | 19 ++++++- .../activities/ProxyActivityManager.java | 1 + .../ws/WorkbenchActivitySupport.java | 4 +- .../eclipse/ui/tests/rcp/RcpTestSuite.java | 2 +- .../eclipse/ui/tests/rcp/WorkbenchTest.java | 56 +++++++++++++++++++ .../META-INF/MANIFEST.MF | 1 + 9 files changed, 113 insertions(+), 9 deletions(-) create mode 100644 tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/WorkbenchTest.java diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/activities/IActivityManager.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/activities/IActivityManager.java index 1cf46d696e8..47e5403d03b 100644 --- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/activities/IActivityManager.java +++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/activities/IActivityManager.java @@ -114,4 +114,13 @@ public interface IActivityManager { * performed. */ void removeActivityManagerListener(IActivityManagerListener activityManagerListener); + + /** + * Disposes this activity manager. Removes all listeners. The behavior of all + * other methods after disposing is undefined. + * + * @since 3.131 + */ + void dispose(); + } diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java index 82975d14599..69b3e6bd8db 100644 --- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java +++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/Workbench.java @@ -3019,18 +3019,27 @@ private void shutdown() { workbenchListeners.clear(); cancelEarlyStartup(); - if (workbenchService != null) + if (workbenchService != null) { workbenchService.unregister(); + } workbenchService = null; - if (e4WorkbenchService != null) + if (e4WorkbenchService != null) { e4WorkbenchService.unregister(); + } e4WorkbenchService = null; // for dynamic UI registry.removeRegistryChangeListener(extensionEventHandler); registry.removeRegistryChangeListener(startupRegistryListener); + // shut down activity helper before disposing workbench activity support; + // dispose activity support before disposing service locator to avoid + // unnecessary activity disablement processing + activityHelper.shutdown(); + workbenchActivitySupport.dispose(); + WorkbenchHelpSystem.disposeIfNecessary(); + // Bring down all of the services. serviceLocator.dispose(); application.getCommands().removeAll(commandsToRemove); @@ -3038,11 +3047,7 @@ private void shutdown() { getDisplay().removeFilter(SWT.MouseDown, backForwardListener); backForwardListener = null; - workbenchActivitySupport.dispose(); - WorkbenchHelpSystem.disposeIfNecessary(); - // shutdown the rest of the workbench - activityHelper.shutdown(); uninitializeImages(); if (WorkbenchPlugin.getDefault() != null) { WorkbenchPlugin.getDefault().reset(); diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/AbstractActivityManager.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/AbstractActivityManager.java index 6ac7f011aec..dbe1192c63e 100644 --- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/AbstractActivityManager.java +++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/AbstractActivityManager.java @@ -22,6 +22,8 @@ public abstract class AbstractActivityManager implements IActivityManager { private ListenerList activityManagerListeners; + private boolean disposed; + protected AbstractActivityManager() { } @@ -60,4 +62,15 @@ public void removeActivityManagerListener(IActivityManagerListener activityManag activityManagerListeners.remove(activityManagerListener); } } + + @Override + public void dispose() { + activityManagerListeners.clear(); + disposed = true; + } + + protected boolean isDisposed() { + return disposed; + } + } diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/MutableActivityManager.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/MutableActivityManager.java index b24d348165b..33912033976 100644 --- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/MutableActivityManager.java +++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/MutableActivityManager.java @@ -133,6 +133,23 @@ public MutableActivityManager(ITriggerPointAdvisor triggerPointAdvisor, IActivit readRegistry(true); } + @Override + public void dispose() { + super.dispose(); + clearExpressions(); + activityRegistry.removeActivityRegistryListener(activityRegistryListener); + activitiesById.clear(); + activityRequirementBindingsByActivityId.clear(); + activityPatternBindingsByActivityId.clear(); + categoriesById.clear(); + categoryActivityBindingsByCategoryId.clear(); + categoryDefinitionsById.clear(); + definedActivityIds.clear(); + definedCategoryIds.clear(); + enabledActivityIds.clear(); + identifiersById.clear(); + } + @Override synchronized public IActivity getActivity(String activityId) { if (activityId == null) { @@ -596,7 +613,7 @@ private Map updateActivities(Collection activityI } private IPropertyChangeListener enabledWhenListener = event -> { - if (addingEvaluationListener) { + if (addingEvaluationListener || isDisposed()) { return; } diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ProxyActivityManager.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ProxyActivityManager.java index dae3408a5b8..fe0de34f1d3 100644 --- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ProxyActivityManager.java +++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ProxyActivityManager.java @@ -66,4 +66,5 @@ public Set getEnabledActivityIds() { public IIdentifier getIdentifier(String identifierId) { return activityManager.getIdentifier(identifierId); } + } diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ws/WorkbenchActivitySupport.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ws/WorkbenchActivitySupport.java index db28a387b78..47b62414745 100644 --- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ws/WorkbenchActivitySupport.java +++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ws/WorkbenchActivitySupport.java @@ -315,11 +315,13 @@ private ImageBindingRegistry getCategoryImageBindingRegistry() { } /** - * Dispose of the image registries. + * Dispose of the image registries and activity manager. * * @since 3.1 */ public void dispose() { + proxyActivityManager.dispose(); + mutableActivityManager.dispose(); if (activityImageBindingRegistry != null) { activityImageBindingRegistry.dispose(); PlatformUI.getWorkbench().getExtensionTracker().unregisterHandler(activityImageBindingRegistry); diff --git a/tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/RcpTestSuite.java b/tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/RcpTestSuite.java index f5cff186b15..bf5c9ba393e 100644 --- a/tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/RcpTestSuite.java +++ b/tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/RcpTestSuite.java @@ -25,7 +25,7 @@ @RunWith(Suite.class) @Suite.SuiteClasses({ PlatformUITest.class, WorkbenchAdvisorTest.class, WorkbenchConfigurerTest.class, WorkbenchWindowConfigurerTest.class, ActionBarConfigurerTest.class, IWorkbenchPageTest.class, - WorkbenchSaveRestoreStateTest.class, WorkbenchListenerTest.class }) + WorkbenchSaveRestoreStateTest.class, WorkbenchListenerTest.class, WorkbenchTest.class }) public class RcpTestSuite { diff --git a/tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/WorkbenchTest.java b/tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/WorkbenchTest.java new file mode 100644 index 00000000000..0d8cf28d7d9 --- /dev/null +++ b/tests/org.eclipse.ui.tests.rcp/Eclipse RCP Tests/org/eclipse/ui/tests/rcp/WorkbenchTest.java @@ -0,0 +1,56 @@ +/******************************************************************************* + * Copyright (c) 2023 Vector Informatik GmbH and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + *******************************************************************************/ +package org.eclipse.ui.tests.rcp; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +import org.eclipse.swt.widgets.Display; +import org.eclipse.ui.IWorkbench; +import org.eclipse.ui.PlatformUI; +import org.eclipse.ui.activities.IActivityManager; +import org.eclipse.ui.activities.IActivityManagerListener; +import org.eclipse.ui.application.WorkbenchAdvisor; +import org.eclipse.ui.tests.harness.util.RCPTestWorkbenchAdvisor; +import org.junit.Test; + +public class WorkbenchTest { + /** + * Tests activity manager behavior during workbench shutdown. + * + * See https://github.com/eclipse-platform/eclipse.platform.ui/issues/1084 + */ + @Test + public void testWorkbenchShutdownProducesNoActivityManagerEvents() { + IActivityManagerListener activityListener = mock(IActivityManagerListener.class); + WorkbenchAdvisor closeAfterStartupAdvisor = new RCPTestWorkbenchAdvisor() { + @Override + public void postStartup() { + IWorkbench workbench = getWorkbenchConfigurer().getWorkbench(); + IActivityManager activityManager = workbench.getActivitySupport().getActivityManager(); + activityManager.addActivityManagerListener(activityListener); + workbench.close(); + }; + }; + + runWorkbench(closeAfterStartupAdvisor); + + verify(activityListener, never()).activityManagerChanged(any()); + } + + private void runWorkbench(WorkbenchAdvisor workbenchAdvisor) { + Display display = PlatformUI.createDisplay(); + PlatformUI.createAndRunWorkbench(display, workbenchAdvisor); + } + +} diff --git a/tests/org.eclipse.ui.tests.rcp/META-INF/MANIFEST.MF b/tests/org.eclipse.ui.tests.rcp/META-INF/MANIFEST.MF index 3e4fdebbdd8..5db851bf9ee 100644 --- a/tests/org.eclipse.ui.tests.rcp/META-INF/MANIFEST.MF +++ b/tests/org.eclipse.ui.tests.rcp/META-INF/MANIFEST.MF @@ -7,6 +7,7 @@ Bundle-Vendor: %providerName Require-Bundle: org.eclipse.core.runtime, org.eclipse.ui, org.junit, + org.mockito.mockito-core, org.eclipse.test.performance, org.eclipse.ui.tests.harness Bundle-ActivationPolicy: lazy