Bug 335344 - External settings lost after changing language IDs, take 2

The original fix (a733900) only fixed part of the problem: It worked for
the case where a complete CExternalSetting was removed and replaced by a
different one, but not in the case where individual entries from a
CExternalSetting were moved to a different one, but others remained
(and, in both cases, the two CExternalSettings applied to the same
ICLanguageSetting).

This commit
- adds a test for the additional condition, which would previously fail
- reverts the previous fix, which is made redundant by the new one
- fixes both cases by applying removals before additions with
ICSettingEntry granularity per ICLanguageSetting rather than for whole
CExternalSettings.

Change-Id: I1b1ee7443b83189c29e458eef12be9cad6b3965d
Signed-off-by: Christian Walther <walther@indel.ch>

core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/ExternalSettingsProviderTests.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2007, 2015 Intel Corporation and others.
+ * Copyright (c) 2007, 2018 Intel Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -20,6 +20,8 @@ package org.eclipse.cdt.core.settings.model;
 
 import java.util.Arrays;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Set;
 
 import org.eclipse.cdt.core.dom.IPDOMManager;
 import org.eclipse.cdt.core.model.CoreModel;
@@ -407,6 +409,82 @@ public class ExternalSettingsProviderTests extends BaseTestCase {
 		assertTrue(Arrays.equals(expectedEntriesSet, entries));
 	}
 
+	/**
+	 * Test if moving an entry to an external setting with different language IDs works
+	 */
+	public void testChangeLanguageSetMove() throws CoreException {
+		TestExtSettingsProvider.setVariantNum(5);
+
+		CoreModel model = CoreModel.getDefault();
+		ICProjectDescriptionManager mngr = model.getProjectDescriptionManager();
+		IProject project = p5.getProject();
+
+		// add external settings provider
+		ICProjectDescription des = model.getProjectDescription(project);
+		ICConfigurationDescription cfgDes = des.getConfigurations()[0];
+		String[] extPIds = new String[] { TestExtSettingsProvider.TEST_EXTERNAL_PROVIDER_ID };
+		cfgDes.setExternalSettingsProviderIds(extPIds);
+		model.setProjectDescription(project, des);
+
+		// read out the settings it caused
+		des = model.getProjectDescription(project, false);
+		cfgDes = des.getConfigurations()[0];
+		ICFolderDescription root = cfgDes.getRootFolderDescription();
+		HashMap<String, ICLanguageSetting> languageSettingsById = new HashMap<>();
+		for (ICLanguageSetting s : root.getLanguageSettings()) {
+			languageSettingsById.put(s.getLanguageId(), s);
+		}
+
+		ICLanguageSetting ls;
+		ICLanguageSettingEntry[] entries;
+		Set<ICLanguageSettingEntry> expectedEntries1 = new HashSet<>(
+				Arrays.asList(new CMacroEntry("MACRO_1", "Value1", 0)));
+		Set<ICLanguageSettingEntry> expectedEntries12 = new HashSet<>(
+				Arrays.asList(new CMacroEntry("MACRO_1", "Value1", 0), new CMacroEntry("MACRO_2", "Value2", 0)));
+		Set<ICLanguageSettingEntry> expectedEntries123 = new HashSet<>(
+				Arrays.asList(new CMacroEntry("MACRO_1", "Value1", 0), new CMacroEntry("MACRO_2", "Value2", 0),
+						new CMacroEntry("MACRO_3", "Value3", 0)));
+
+		// MACRO_2 should be present for assembly but not for C
+		ls = languageSettingsById.get("org.eclipse.cdt.core.assembly");
+		assertNotNull(ls);
+		entries = ls.getSettingEntries(ICSettingEntry.MACRO);
+		assertEquals(3, entries.length);
+		assertEquals(expectedEntries123, new HashSet<>(Arrays.asList(entries)));
+
+		ls = languageSettingsById.get("org.eclipse.cdt.core.gcc");
+		assertNotNull(ls);
+		entries = ls.getSettingEntries(ICSettingEntry.MACRO);
+		assertEquals(1, entries.length);
+		assertEquals(expectedEntries1, new HashSet<>(Arrays.asList(entries)));
+
+		// update settings provider
+		TestExtSettingsProvider.setVariantNum(6);
+		mngr.updateExternalSettingsProviders(extPIds, null);
+
+		// read out the settings it caused
+		des = model.getProjectDescription(project, false);
+		cfgDes = des.getConfigurations()[0];
+		root = cfgDes.getRootFolderDescription();
+		languageSettingsById = new HashMap<>();
+		for (ICLanguageSetting s : root.getLanguageSettings()) {
+			languageSettingsById.put(s.getLanguageId(), s);
+		}
+
+		// MACRO_2 should be present for both now
+		ls = languageSettingsById.get("org.eclipse.cdt.core.gcc");
+		assertNotNull(ls);
+		entries = ls.getSettingEntries(ICSettingEntry.MACRO);
+		assertEquals(2, entries.length);
+		assertEquals(expectedEntries12, new HashSet<>(Arrays.asList(entries)));
+
+		ls = languageSettingsById.get("org.eclipse.cdt.core.assembly");
+		assertNotNull(ls);
+		entries = ls.getSettingEntries(ICSettingEntry.MACRO);
+		assertEquals(3, entries.length);
+		assertEquals(expectedEntries123, new HashSet<>(Arrays.asList(entries)));
+	}
+
 	/**
 	 * Test if macros with the same name but different values can coexist when
 	 * they belong to different language ids

core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/TestExtSettingsProvider.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2007, 2012 Intel Corporation and others.
+ * Copyright (c) 2007, 2018 Intel Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -59,8 +59,20 @@ public class TestExtSettingsProvider extends CExternalSettingProvider {
 			new CExternalSetting(new String[] { "org.eclipse.cdt.core.assembly" }, null, null,
 					new ICSettingEntry[] { new CMacroEntry("THE_MACRO", "TheValue", 0) }) };
 
+	private static CExternalSetting[] SETTINGS_6 = new CExternalSetting[] {
+			new CExternalSetting(null, null, null, new ICSettingEntry[] { new CMacroEntry("MACRO_1", "Value1", 0) }),
+			new CExternalSetting(new String[] { "org.eclipse.cdt.core.assembly" }, null, null, new ICSettingEntry[] {
+					new CMacroEntry("MACRO_2", "Value2", 0), new CMacroEntry("MACRO_3", "Value3", 0) }) };
+
+	private static CExternalSetting[] SETTINGS_7 = new CExternalSetting[] {
+			new CExternalSetting(null, null, null,
+					new ICSettingEntry[] { new CMacroEntry("MACRO_1", "Value1", 0),
+							new CMacroEntry("MACRO_2", "Value2", 0) }),
+			new CExternalSetting(new String[] { "org.eclipse.cdt.core.assembly" }, null, null,
+					new ICSettingEntry[] { new CMacroEntry("MACRO_3", "Value3", 0) }) };
+
 	public static final CExternalSetting[][] SETTINGS_VARIANTS = new CExternalSetting[][] { SETTINGS_1, SETTINGS_2,
-			SETTINGS_3, SETTINGS_4, SETTINGS_5 };
+			SETTINGS_3, SETTINGS_4, SETTINGS_5, SETTINGS_6, SETTINGS_7 };
 
 	private static int variantNum;
 

core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettingsDeltaProcessor.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2007, 2010 Intel Corporation and others.
+ * Copyright (c) 2007, 2018 Intel Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -10,6 +10,7 @@
  *
  * Contributors:
  * Intel Corporation - Initial API and implementation
+ * Christian Walther (Indel AG) - [335344] changing language IDs
  *******************************************************************************/
 package org.eclipse.cdt.internal.core.settings.model;
 
@@ -147,15 +148,7 @@ public class CExternalSettingsDeltaProcessor {
 		ICLanguageSetting setting = des.getLanguageSetting();
 		if (setting == null)
 			return false;
-
+		return applyDelta(setting, deltas, kindMask);
-		boolean changed = false;
-		for (ExtSettingsDelta delta : deltas) {
-			if (isSettingCompatible(setting, delta.fSetting)) {
-				if (applyDelta(setting, delta, kindMask))
-					changed = true;
-			}
-		}
-		return changed;
 	}
 
 	static boolean applyDelta(ICFolderDescription des, ExtSettingsDelta deltas[], int kindMask) {
@@ -173,16 +166,24 @@ public class CExternalSettingsDeltaProcessor {
 
 	static boolean applyDelta(ICLanguageSetting setting, ExtSettingsDelta[] deltas, int kindMask) {
 		boolean changed = false;
+		// apply removals before additions in case several deltas apply to the same setting
 		for (ExtSettingsDelta delta : deltas) {
 			if (isSettingCompatible(setting, delta.fSetting)) {
-				if (applyDelta(setting, delta, kindMask))
+				if (applyDelta(setting, delta, kindMask, false, true))
+					changed = true;
+			}
+		}
+		for (ExtSettingsDelta delta : deltas) {
+			if (isSettingCompatible(setting, delta.fSetting)) {
+				if (applyDelta(setting, delta, kindMask, true, false))
 					changed = true;
 			}
 		}
 		return changed;
 	}
 
-	static boolean applyDelta(ICLanguageSetting setting, ExtSettingsDelta delta, int kindMask) {
+	static boolean applyDelta(ICLanguageSetting setting, ExtSettingsDelta delta, int kindMask, boolean additions,
+			boolean removals) {
 		int kinds[] = KindBasedStore.getLanguageEntryKinds();
 		ICLanguageSettingEntry entries[];
 		ICSettingEntry diff[][];
@@ -196,7 +197,8 @@ public class CExternalSettingsDeltaProcessor {
 				continue;
 
 			entries = setting.getSettingEntries(kind);
-			List<ICLanguageSettingEntry> list = calculateUpdatedEntries(entries, diff[0], diff[1]);
+			List<ICLanguageSettingEntry> list = calculateUpdatedEntries(entries, additions ? diff[0] : null,
+					removals ? diff[1] : null);
 
 			if (list != null) {
 				setting.setSettingEntries(kind, list);

core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettinsDeltaCalculator.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2007, 2011 Intel Corporation and others.
+ * Copyright (c) 2007, 2018 Intel Corporation and others.
  *
  * This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License 2.0
@@ -19,7 +19,6 @@ import java.util.Arrays;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -235,7 +234,7 @@ class CExternalSettinsDeltaCalculator {
 		if (oldSettings == null || oldSettings.length == 0)
 			return createDeltas(newSettings, true);
 
-		LinkedList<ExtSettingsDelta> deltaList = new LinkedList<>();
+		List<ExtSettingsDelta> deltaList = new ArrayList<>();
 
 		Map<ExtSettingMapKey, ICExternalSetting> newMap = toSettingsKeyMap(newSettings);
 		Map<ExtSettingMapKey, ICExternalSetting> oldMap = toSettingsKeyMap(oldSettings);
@@ -243,20 +242,16 @@ class CExternalSettinsDeltaCalculator {
 			CExternalSetting newSetting = (CExternalSetting) entry.getValue();
 			CExternalSetting oldSetting = (CExternalSetting) oldMap.remove(entry.getKey());
 			if (oldSetting == null) {
-				deltaList.addLast(new ExtSettingsDelta(newSetting, true));
+				deltaList.add(new ExtSettingsDelta(newSetting, true));
 			} else {
 				ExtSettingsDelta delta = createDelta(newSetting, oldSetting);
 				if (delta != null)
-					deltaList.addLast(delta);
+					deltaList.add(delta);
 			}
 		}
 
 		for (ICExternalSetting oldSettng : oldMap.values()) {
-			// removals must be prepended to the list so that they are applied before additions,
+			deltaList.add(new ExtSettingsDelta((CExternalSetting) oldSettng, false));
-			// otherwise a setting that was just added might be immediately removed again in
-			// CExternalSettingsDeltaProcessor.applyDelta(ICLanguageSetting, ExtSettingsDelta[], int)
-			// if the old and new setting only differ in their language sets and these overlap
-			deltaList.addFirst(new ExtSettingsDelta((CExternalSetting) oldSettng, false));
 		}
 
 		if (deltaList.size() == 0)