Avoid unnecessary persistence of activity enablements #1084

The ActivityPersistenceHelper persists all activity enablements upon
every notification about a changed activity enablement it receives.
Persistence is, however, only done and thus necessary for activities
that are not expression-controlled. So the current implementation
performs unnecessary persistence operations.

With this change, the ActivityPersistenceHelper only persists the
enablements when the enablement of a non-expression-controlled activity
has changed. To this end, the received ActivityManagerEvent is extended
by the information about whether such an activity was affected. This
information is either directly passed to the event construction in case
it has already been calculated in the calling context or is calculated
lazily when accessing the value.

Contributes to
#1084.

Author: HeikoKlare

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/activities/ActivityManagerEvent.java

@@ -14,7 +14,10 @@
 
 package org.eclipse.ui.activities;
 
+import static java.util.Collections.emptySet;
+
 import java.util.Set;
+import java.util.stream.Stream;
 import org.eclipse.ui.internal.util.Util;
 
 /**
@@ -39,23 +42,31 @@ public final class ActivityManagerEvent {
 
 	private boolean enabledActivityIdsChanged;
 
+	/**
+	 * Indicates whether enabled IDs of non-expression-controlled activities have
+	 * changed. The value is calculated from changed enabled activity IDs given to
+	 * the constructor if known by the caller or is set to <code>null</null> and
+	 * will be calculated on demand upon first access to the value.
+	 */
+	private Boolean enabledNonExpressionControlledActivityIdsChanged = null;
+
 	/**
 	 * The set of activity identifiers (strings) that were defined before the change
-	 * occurred. If the defined activities did not changed, then this value is
+	 * occurred. If the defined activities did not change, then this value is
 	 * <code>null</code>.
 	 */
 	private final Set<String> previouslyDefinedActivityIds;
 
 	/**
 	 * The set of category identifiers (strings) that were defined before the change
-	 * occurred. If the defined category did not changed, then this value is
+	 * occurred. If the defined category did not change, then this value is
 	 * <code>null</code>.
 	 */
 	private final Set<String> previouslyDefinedCategoryIds;
 
 	/**
 	 * The set of activity identifiers (strings) that were enabled before the change
-	 * occurred. If the enabled activities did not changed, then this value is
+	 * occurred. If the enabled activities did not change, then this value is
 	 * <code>null</code>.
 	 */
 	private final Set<String> previouslyEnabledActivityIds;
@@ -145,6 +156,91 @@ public ActivityManagerEvent(IActivityManager activityManager, boolean definedAct
 		this.enabledActivityIdsChanged = enabledActivityIdsChanged;
 	}
 
+	/**
+	 * Creates a new instance of this class.
+	 *
+	 * @param activityManager              the instance of the interface that
+	 *                                     changed.
+	 * @param definedActivityIdsChanged    <code>true</code>, iff the
+	 *                                     definedActivityIds property changed.
+	 * @param definedCategoryIdsChanged    <code>true</code>, iff the
+	 *                                     definedCategoryIds property changed.
+	 * @param enabledActivityIdsChanged    <code>true</code>, iff the
+	 *                                     enabledActivityIds property changed.
+	 * @param previouslyDefinedActivityIds the set of identifiers to previously
+	 *                                     defined activities. This set may be
+	 *                                     empty. If this set is not empty, it must
+	 *                                     only contain instances of
+	 *                                     <code>String</code> . This set must be
+	 *                                     <code>null</code> if
+	 *                                     definedActivityIdsChanged is
+	 *                                     <code>false</code> and must not be null
+	 *                                     if definedActivityIdsChanged is
+	 *                                     <code>true</code>.
+	 * @param previouslyDefinedCategoryIds the set of identifiers to previously
+	 *                                     defined category. This set may be empty.
+	 *                                     If this set is not empty, it must only
+	 *                                     contain instances of <code>String</code>.
+	 *                                     This set must be <code>null</code> if
+	 *                                     definedCategoryIdsChanged is
+	 *                                     <code>false</code> and must not be null
+	 *                                     if definedCategoryIdsChanged is
+	 *                                     <code>true</code>.
+	 * @param previouslyEnabledActivityIds the set of identifiers to previously
+	 *                                     enabled activities. This set may be
+	 *                                     empty. If this set is not empty, it must
+	 *                                     only contain instances of
+	 *                                     <code>String</code>. This set must be
+	 *                                     <code>null</code> if
+	 *                                     enabledActivityIdsChanged is
+	 *                                     <code>false</code> and must not be null
+	 *                                     if enabledActivityIdsChanged is
+	 *                                     <code>true</code>.
+	 * @param changedEnabledActivityIds    the set of identifiers to activities
+	 *                                     whose enabled state has changed. This set
+	 *                                     may be empty. If this set is not empty,
+	 *                                     it must only contain instances of
+	 *                                     <code>String</code>. This set must be
+	 *                                     <code>null</code> if
+	 *                                     enabledActivityIdsChanged is
+	 *                                     <code>false</code> and must not be null
+	 *                                     if enabledActivityIdsChanged is
+	 *                                     <code>true</code>.
+	 * @since 3.131
+	 */
+	public ActivityManagerEvent(IActivityManager activityManager, boolean definedActivityIdsChanged,
+			boolean definedCategoryIdsChanged, boolean enabledActivityIdsChanged,
+			final Set<String> previouslyDefinedActivityIds, final Set<String> previouslyDefinedCategoryIds,
+			final Set<String> previouslyEnabledActivityIds, Set<String> changedEnabledActivityIds) {
+		this(activityManager, definedActivityIdsChanged, definedCategoryIdsChanged, enabledActivityIdsChanged,
+				previouslyDefinedActivityIds, previouslyDefinedCategoryIds, previouslyEnabledActivityIds);
+		if (enabledActivityIdsChanged) {
+			this.enabledNonExpressionControlledActivityIdsChanged = changedEnabledActivityIds.stream()
+					.anyMatch(this::isIdOfNonExpressionControlledActivity);
+		} else {
+			this.enabledNonExpressionControlledActivityIdsChanged = false;
+		}
+
+	}
+
+	/**
+	 * Creates a copy of this {@link ActivityManagerEvent} containing the given
+	 * {@code newActivityManager}.
+	 *
+	 * @param newActivityManager the new activity manager to be referenced by the
+	 *                           copied event
+	 *
+	 * @return a copy of this event referencing the given activity manager
+	 * @since 3.131
+	 */
+	public ActivityManagerEvent copyFor(IActivityManager newActivityManager) {
+		ActivityManagerEvent copy = new ActivityManagerEvent(newActivityManager, definedActivityIdsChanged,
+				definedCategoryIdsChanged, enabledActivityIdsChanged, previouslyDefinedActivityIds,
+				previouslyDefinedCategoryIds, previouslyEnabledActivityIds);
+		copy.enabledNonExpressionControlledActivityIdsChanged = enabledNonExpressionControlledActivityIdsChanged;
+		return copy;
+	}
+
 	/**
 	 * Returns the instance of the interface that changed.
 	 *
@@ -214,4 +310,35 @@ public boolean haveDefinedCategoryIdsChanged() {
 	public boolean haveEnabledActivityIdsChanged() {
 		return enabledActivityIdsChanged;
 	}
+
+	/**
+	 * Returns whether or not enabledActivityIds property changed and any of the
+	 * changed IDs belongs to a non-expression-controlled activity.
+	 *
+	 * @return <code>true>/code> iff the enabledActivityIds property changed and any
+	 *         of the changed IDs belongs to a non-expression-controlled activity.
+	 * @since 3.131
+	 */
+	public boolean haveEnabledNonExpressionControlledActivityIdsChanged() {
+		if (enabledNonExpressionControlledActivityIdsChanged == null) {
+			enabledNonExpressionControlledActivityIdsChanged = calculateHaveEnabledNonExpressionControlledActivityIdsChanged();
+		}
+		return enabledNonExpressionControlledActivityIdsChanged;
+	}
+
+	private boolean calculateHaveEnabledNonExpressionControlledActivityIdsChanged() {
+		Set<String> previousIds = previouslyEnabledActivityIds == null ? emptySet() : previouslyEnabledActivityIds;
+		Set<String> currentIds = activityManager.getEnabledActivityIds();
+		Stream<String> addedIds = currentIds.stream().filter(id -> !previousIds.contains(id));
+		if (addedIds.anyMatch(this::isIdOfNonExpressionControlledActivity)) {
+			return true;
+		}
+		Stream<String> removedIds = previousIds.stream().filter(id -> !currentIds.contains(id));
+		return removedIds.anyMatch(this::isIdOfNonExpressionControlledActivity);
+	}
+
+	private boolean isIdOfNonExpressionControlledActivity(String id) {
+		return activityManager.getActivity(id).getExpression() == null;
+	}
+
 }

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/ActivityPersistanceHelper.java

@@ -57,7 +57,7 @@ final class ActivityPersistanceHelper {
 			// state
 			loadEnabledStates(activityManagerEvent.getActivityManager().getEnabledActivityIds(), delta);
 		}
-		if (activityManagerEvent.haveEnabledActivityIdsChanged()) {
+		if (activityManagerEvent.haveEnabledNonExpressionControlledActivityIdsChanged()) {
 			saveEnabledStates();
 		}
 	};

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/MutableActivityManager.java

@@ -527,7 +527,8 @@ private void updateListeners(boolean activityManagerChanged, Map<String, Activit
 
 		if (activityManagerChanged) {
 			fireActivityManagerChanged(
-					new ActivityManagerEvent(this, false, false, true, null, null, previouslyEnabledActivityIds));
+					new ActivityManagerEvent(this, false, false, true, null, null,
+							previouslyEnabledActivityIds, deltaActivityIds));
 		}
 	}
 

bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/activities/ProxyActivityManager.java

@@ -32,13 +32,7 @@ public ProxyActivityManager(IActivityManager activityManager) {
 		this.activityManager = activityManager;
 
 		this.activityManager.addActivityManagerListener(activityManagerEvent -> {
-			ActivityManagerEvent proxyActivityManagerEvent = new ActivityManagerEvent(ProxyActivityManager.this,
-					activityManagerEvent.haveDefinedActivityIdsChanged(),
-					activityManagerEvent.haveDefinedCategoryIdsChanged(),
-					activityManagerEvent.haveEnabledActivityIdsChanged(),
-					activityManagerEvent.getPreviouslyDefinedActivityIds(),
-					activityManagerEvent.getPreviouslyDefinedCategoryIds(),
-					activityManagerEvent.getPreviouslyEnabledActivityIds());
+			ActivityManagerEvent proxyActivityManagerEvent = activityManagerEvent.copyFor(ProxyActivityManager.this);
 			fireActivityManagerChanged(proxyActivityManagerEvent);
 		});
 	}