From f2f92ab4045802da83d2efeae9548a4fd544becb Mon Sep 17 00:00:00 2001 From: Christian Walther Date: Mon, 10 Dec 2018 11:42:44 +0100 Subject: [PATCH] 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 --- .../model/ExternalSettingsProviderTests.java | 80 ++++++++++++++++++- .../model/TestExtSettingsProvider.java | 16 +++- .../CExternalSettingsDeltaProcessor.java | 28 ++++--- .../CExternalSettinsDeltaCalculator.java | 15 ++-- 4 files changed, 113 insertions(+), 26 deletions(-) diff --git a/core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/ExternalSettingsProviderTests.java b/core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/ExternalSettingsProviderTests.java index 2a352d69877..dd9e1fc7a95 100644 --- a/core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/ExternalSettingsProviderTests.java +++ b/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 languageSettingsById = new HashMap<>(); + for (ICLanguageSetting s : root.getLanguageSettings()) { + languageSettingsById.put(s.getLanguageId(), s); + } + + ICLanguageSetting ls; + ICLanguageSettingEntry[] entries; + Set expectedEntries1 = new HashSet<>( + Arrays.asList(new CMacroEntry("MACRO_1", "Value1", 0))); + Set expectedEntries12 = new HashSet<>( + Arrays.asList(new CMacroEntry("MACRO_1", "Value1", 0), new CMacroEntry("MACRO_2", "Value2", 0))); + Set 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 diff --git a/core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/TestExtSettingsProvider.java b/core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/TestExtSettingsProvider.java index 7fa6afa9d09..a09b539efbd 100644 --- a/core/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/TestExtSettingsProvider.java +++ b/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; diff --git a/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettingsDeltaProcessor.java b/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettingsDeltaProcessor.java index 62a77b63470..7420d51faa9 100644 --- a/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettingsDeltaProcessor.java +++ b/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; - - boolean changed = false; - for (ExtSettingsDelta delta : deltas) { - if (isSettingCompatible(setting, delta.fSetting)) { - if (applyDelta(setting, delta, kindMask)) - changed = true; - } - } - return changed; + return applyDelta(setting, deltas, kindMask); } 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 list = calculateUpdatedEntries(entries, diff[0], diff[1]); + List list = calculateUpdatedEntries(entries, additions ? diff[0] : null, + removals ? diff[1] : null); if (list != null) { setting.setSettingEntries(kind, list); diff --git a/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettinsDeltaCalculator.java b/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettinsDeltaCalculator.java index ef4681a7ec7..9f297793f45 100644 --- a/core/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettinsDeltaCalculator.java +++ b/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 deltaList = new LinkedList<>(); + List deltaList = new ArrayList<>(); Map newMap = toSettingsKeyMap(newSettings); Map 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, - // 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)); + deltaList.add(new ExtSettingsDelta((CExternalSetting) oldSettng, false)); } if (deltaList.size() == 0)