Bug 283731 - [target] Remove target state dependence on preferences
Use target for feature loading, started removing preferences

Change-Id: I2d73275c8c223b03f6bb035ffd972e7f3b072b1f
diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/core/target/LoadTargetDefinitionJob.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/core/target/LoadTargetDefinitionJob.java
index 1f844f6..f8a2d3c 100644
--- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/core/target/LoadTargetDefinitionJob.java
+++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/core/target/LoadTargetDefinitionJob.java
@@ -297,16 +297,6 @@
 			buffer.setLength(buffer.length() - 1);

 		pref.setValue(ICoreConstants.ADDITIONAL_LOCATIONS, buffer.toString());

 

-		String newValue = currentPath;

-		for (int i = 0; i < 4; i++) {

-			String value = pref.getString(ICoreConstants.SAVED_PLATFORM + i);

-			pref.setValue(ICoreConstants.SAVED_PLATFORM + i, newValue);

-			if (!value.equals(currentPath))

-				newValue = value;

-			else

-				break;

-		}

-

 		handleReload(path, additional, pref, new SubProgressMonitor(monitor, 85));

 		monitor.done();

 	}

diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ExternalFeatureModelManager.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ExternalFeatureModelManager.java
index 7302123..d4cc66b 100644
--- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ExternalFeatureModelManager.java
+++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ExternalFeatureModelManager.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2000, 2012 IBM Corporation and others.
+ * Copyright (c) 2000, 2013 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -17,12 +17,11 @@
 import org.eclipse.osgi.util.NLS;
 import org.eclipse.pde.core.IModelProviderEvent;
 import org.eclipse.pde.core.IModelProviderListener;
+import org.eclipse.pde.core.target.ITargetDefinition;
 import org.eclipse.pde.core.target.TargetFeature;
 import org.eclipse.pde.internal.core.feature.ExternalFeatureModel;
-import org.eclipse.pde.internal.core.ifeature.IFeature;
 import org.eclipse.pde.internal.core.ifeature.IFeatureModel;
 import org.eclipse.pde.internal.core.target.Messages;
-import org.osgi.framework.Version;
 
 /**
  * Manages the features known to the PDE state that come from the target platform.
@@ -60,68 +59,10 @@
 		}
 	}
 
-	public static IFeatureModel[] createModels(String platformHome, ArrayList<String> additionalLocations, IProgressMonitor monitor) {
-		if (platformHome != null && platformHome.length() > 0) {
-			URL[] featureURLs = PluginPathFinder.getFeaturePaths(platformHome);
-
-			if (additionalLocations.size() == 0)
-				return createModels(featureURLs, monitor);
-
-			File[] dirs = new File[additionalLocations.size()];
-			for (int i = 0; i < dirs.length; i++) {
-				String directory = additionalLocations.get(i).toString();
-				File dir = new File(directory, "features"); //$NON-NLS-1$
-				if (!dir.exists())
-					dir = new File(directory);
-				dirs[i] = dir;
-			}
-
-			URL[] newUrls = PluginPathFinder.scanLocations(dirs);
-
-			URL[] result = new URL[featureURLs.length + newUrls.length];
-			System.arraycopy(featureURLs, 0, result, 0, featureURLs.length);
-			System.arraycopy(newUrls, 0, result, featureURLs.length, newUrls.length);
-			return createModels(result, monitor);
-		}
-		return new IFeatureModel[0];
-	}
-
-	private static IFeatureModel[] createModels(URL[] featurePaths, IProgressMonitor monitor) {
-		if (monitor == null)
-			monitor = new NullProgressMonitor();
-		monitor.beginTask("", featurePaths.length); //$NON-NLS-1$
-		Map<String, IFeatureModel> uniqueFeatures = new HashMap<String, IFeatureModel>();
-		for (int i = 0; i < featurePaths.length; i++) {
-			File manifest = new File(featurePaths[i].getFile(), ICoreConstants.FEATURE_FILENAME_DESCRIPTOR);
-			if (!manifest.exists() || !manifest.isFile()) {
-				monitor.worked(1);
-				continue;
-			}
-			try {
-				IFeatureModel model = createModel(manifest);
-				if (model != null && model.isLoaded()) {
-					IFeature feature = model.getFeature();
-					uniqueFeatures.put(feature.getId() + "_" + feature.getVersion(), model); //$NON-NLS-1$
-				}
-			} catch (CoreException e) {
-				PDECore.log(e);
-			}
-			monitor.worked(1);
-		}
-		Collection<IFeatureModel> models = uniqueFeatures.values();
-		return models.toArray(new IFeatureModel[models.size()]);
-	}
-
 	private ListenerList fListeners = new ListenerList();
 
 	private IFeatureModel[] fModels;
 
-	private PDEPreferencesManager fPref;
-
-	public ExternalFeatureModelManager() {
-		fPref = PDECore.getDefault().getPreferencesManager();
-	}
-
 	public void addModelProviderListener(IModelProviderListener listener) {
 		fListeners.add(listener);
 	}
@@ -138,80 +79,67 @@
 	 * Loads new feature models from preferences and notifies listeners.
 	 */
 	public void initialize() {
-		// Load all features from the platform path and addditional locations then filter by the list of external features, if available
-		String platformHome = fPref.getString(ICoreConstants.PLATFORM_PATH);
-		String additionalLocations = fPref.getString(ICoreConstants.ADDITIONAL_LOCATIONS);
-		String externalFeaturesString = fPref.getString(ICoreConstants.EXTERNAL_FEATURES);
-
-		IFeatureModel[] oldModels = null;
-		IFeatureModel[] newModels = null;
 		// Do the model loading in a synch block in case other changes cause the models to load
+		IFeatureModel[] oldModels = null;
 		synchronized (this) {
 			oldModels = fModels != null ? fModels : new IFeatureModel[0];
-			IFeatureModel[] allModels = createModels(platformHome, parseAdditionalLocations(additionalLocations), null);
-			if (externalFeaturesString == null || externalFeaturesString.trim().length() == 0) {
-				fModels = allModels;
-			} else {
-				// To allow multiple versions of features, create a map of feature ids to a list of models
-				Map<String, List<IFeatureModel>> modelMap = new HashMap<String, List<IFeatureModel>>();
-				for (int i = 0; i < allModels.length; i++) {
-					String id = allModels[i].getFeature().getId();
-					if (modelMap.containsKey(id)) {
-						List<IFeatureModel> list = modelMap.get(id);
-						list.add(allModels[i]);
-					} else {
-						List<IFeatureModel> list = new ArrayList<IFeatureModel>();
-						list.add(allModels[i]);
-						modelMap.put(id, list);
-					}
-				}
-
-				// Loop through the filter list, finding an exact match in the available models or highest version match
-				Set<IFeatureModel> filteredModels = new HashSet<IFeatureModel>();
-				String[] entries = externalFeaturesString.split(","); //$NON-NLS-1$
-				for (int i = 0; i < entries.length; i++) {
-					String[] parts = entries[i].split("@"); //$NON-NLS-1$
-					if (parts.length > 0) {
-						String id = parts[0];
-						List<?> possibilities = modelMap.get(id);
-						if (possibilities != null) {
-							IFeatureModel candidate = null;
-							for (Iterator<?> iterator = possibilities.iterator(); iterator.hasNext();) {
-								IFeatureModel current = (IFeatureModel) iterator.next();
-								if (candidate == null) {
-									candidate = current;
-								} else if (parts.length > 1 && parts[1].equals(current.getFeature().getVersion())) {
-									candidate = current;
-								} else {
-									Version currentVersion = Version.parseVersion(current.getFeature().getVersion());
-									Version candidateVersion = Version.parseVersion(candidate.getFeature().getVersion());
-									if (currentVersion.compareTo(candidateVersion) == 1) {
-										candidate = current;
-									}
-								}
-							}
-							if (candidate != null) {
-								filteredModels.add(candidate);
-							}
-						}
-					}
-				}
-				fModels = filteredModels.toArray(new IFeatureModel[filteredModels.size()]);
+			long startTime = System.currentTimeMillis();
+			fModels = getExternalModels();
+			if (PDECore.DEBUG_MODEL) {
+				System.out.println("Time to load features from target platform (ms): " + (System.currentTimeMillis() - startTime)); //$NON-NLS-1$
+				System.out.println("External features loaded: " + fModels.length); //$NON-NLS-1$
 			}
-			newModels = new IFeatureModel[fModels.length];
-			System.arraycopy(fModels, 0, newModels, 0, fModels.length);
 		}
 		// Release lock when notifying listeners. See bug 270891.
-		notifyListeners(oldModels, newModels);
+		notifyListeners(oldModels, fModels);
 	}
 
-	private ArrayList<String> parseAdditionalLocations(String additionalLocations) {
-		ArrayList<String> result = new ArrayList<String>();
-		StringTokenizer tokenizer = new StringTokenizer(additionalLocations, ","); //$NON-NLS-1$
-		while (tokenizer.hasMoreTokens()) {
-			result.add(tokenizer.nextToken().trim());
+	/**
+	 * Returns a list of feature models from the target platform or an empty
+	 * array if there is a problem.
+	 * 
+	 * @return list of external feature models, possibly empty
+	 */
+	private IFeatureModel[] getExternalModels() {
+		ITargetDefinition target = null;
+		try {
+			target = TargetPlatformHelper.getWorkspaceTargetResolved(null);
+		} catch (CoreException e) {
+			PDECore.log(e);
+			return new IFeatureModel[0];
 		}
-		return result;
+
+		// TODO resolution cancelled, can we reschedule?
+		if (target == null) {
+			return new IFeatureModel[0];
+		}
+
+		// TODO Log any known issues with the target platform to warn user, already done in PluginModelManager?
+//		if (target.getStatus().getSeverity() == IStatus.ERROR) {
+//			PDECore.log(new Status(IStatus.ERROR, PDECore.PLUGIN_ID, "The current target platform contains errors, open Window > Preferences > Plug-in Development > Target Platform for details.", new CoreException(target.getStatus())));
+//		}
+
+		List<IFeatureModel> result = new ArrayList<IFeatureModel>();
+		TargetFeature[] features = target.getAllFeatures();
+		if (features != null) {
+			for (int i = 0; i < features.length; i++) {
+				// TODO We assume the target will have removed duplicates
+				String location = features[i].getLocation();
+				File manifest = new File(location, ICoreConstants.FEATURE_FILENAME_DESCRIPTOR);
+				if (!manifest.exists() || !manifest.isFile()) {
+					continue;
+				}
+				try {
+					IFeatureModel model = createModel(manifest);
+					if (model != null && model.isLoaded()) {
+						result.add(model);
+					}
+				} catch (CoreException e) {
+					PDECore.log(e);
+				}
+			}
+		}
+		return result.toArray(new IFeatureModel[result.size()]);
 	}
 
 	private void notifyListeners(IFeatureModel[] oldModels, IFeatureModel[] newFeatureModels) {
diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ICoreConstants.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ICoreConstants.java
index 5b7a3f3..66bbcc9 100644
--- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ICoreConstants.java
+++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/ICoreConstants.java
@@ -14,6 +14,7 @@
 import java.util.Locale;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.Path;
+import org.eclipse.pde.core.plugin.TargetPlatform;
 import org.eclipse.pde.core.project.IBundleProjectDescription;
 import org.eclipse.pde.core.target.ITargetPlatformService;
 import org.osgi.framework.Constants;
@@ -21,8 +22,13 @@
 public interface ICoreConstants {
 
 	// Target Platform
+	/**
+	 * Preference key that stores the string path to the root location of the 
+	 * target platform.  Should be accessed through {@link TargetPlatform#getLocation()}.
+	 */
 	String PLATFORM_PATH = "platform_path"; //$NON-NLS-1$
-	String SAVED_PLATFORM = "saved_platform"; //$NON-NLS-1$
+
+	// TODO The following will all be deprecated and only used when loading a target from preferences
 	String TARGET_MODE = "target_mode"; //$NON-NLS-1$
 	String VALUE_USE_THIS = "useThis"; //$NON-NLS-1$
 	String VALUE_USE_OTHER = "useOther"; //$NON-NLS-1$
@@ -30,8 +36,6 @@
 	String VALUE_SAVED_NONE = "[savedNone]"; //$NON-NLS-1$
 	String VALUE_SAVED_ALL = "[savedAll]"; //$NON-NLS-1$
 	String VALUE_SAVED_SOME = "savedSome"; //$NON-NLS-1$
-	String P_SOURCE_LOCATIONS = "source_locations"; //$NON-NLS-1$
-	String P_EXT_LOCATIONS = "ext_locations"; //$NON-NLS-1$
 	String PROGRAM_ARGS = "program_args"; //$NON-NLS-1$
 	/**
 	 * Preferences key that stores a String containing additional arguments to
@@ -39,12 +43,11 @@
 	 * @deprecated Since the 4.4 Luna release, target platform information is no longer stored in preferences. Instead use {@link ITargetPlatformService}.
 	 */
 	String VM_ARGS = "vm_args"; //$NON-NLS-1$
+	// Not sure about this one:
 	String VM_LAUNCHER_INI = "vm_launcher_ini"; //$NON-NLS-1$
 	String IMPLICIT_DEPENDENCIES = "implicit_dependencies"; //$NON-NLS-1$
-	String GROUP_PLUGINS_VIEW = "group_plugins"; //$NON-NLS-1$
 	String ADDITIONAL_LOCATIONS = "additional_locations"; //$NON-NLS-1$
 	String TARGET_PLATFORM_REALIZATION = "target_platform_realization"; //$NON-NLS-1$
-
 	/**
 	 * This preference was only used during 3.5, it has been replaced in 3.6
 	 * with POOLED_URLS.
@@ -259,6 +262,13 @@
 	 */
 	public final static String TARGET_VERSION_LATEST = TARGET38;
 
+	/**
+	 * Preference key that stores a list of user specified source locations.
+	 * No longer supported in the UI.
+	 * @deprecated Not supported in the UI.
+	 */
+	String P_SOURCE_LOCATIONS = "source_locations"; //$NON-NLS-1$
+
 	public final static String EQUINOX = "Equinox"; //$NON-NLS-1$
 
 	// project preferences
diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/PluginModelManager.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/PluginModelManager.java
index acebb51..0fce003 100644
--- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/PluginModelManager.java
+++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/PluginModelManager.java
@@ -26,7 +26,6 @@
 import org.eclipse.pde.core.build.IBuildEntry;
 import org.eclipse.pde.core.plugin.*;
 import org.eclipse.pde.core.target.*;
-import org.eclipse.pde.internal.core.target.IUBundleContainer;
 
 public class PluginModelManager implements IModelProviderListener {
 
@@ -664,53 +663,17 @@
 	 * @return array of URLs for external bundles
 	 */
 	private URL[] getExternalBundles() {
-		final ITargetDefinition target;
+		ITargetDefinition target = null;
 		try {
-			ITargetPlatformService service = (ITargetPlatformService) PDECore.getDefault().acquireService(ITargetPlatformService.class.getName());
-			if (service == null) {
-				throw new CoreException(new Status(IStatus.ERROR, PDECore.PLUGIN_ID, "Could not acquire target platform service"));
-			}
-			target = service.getWorkspaceTargetDefinition();
+			target = TargetPlatformHelper.getWorkspaceTargetResolved(null);
 		} catch (CoreException e) {
 			PDECore.log(e);
 			return new URL[0];
 		}
 
-		// Don't resolve again if we don't have to
-		if (!target.isResolved()) {
-
-			// TODO Performance hack, avoid p2 pinging remote sites or downloading at this time
-			ITargetLocation[] locations = target.getTargetLocations();
-			for (int i = 0; i < locations.length; i++) {
-				if (locations[i] instanceof IUBundleContainer) {
-					((IUBundleContainer) locations[i]).setRemoteFetch(false);
-				}
-			}
-
-			// Resolve the target definition in a separate job to allow cancellation
-			Job job = new Job("Loading external bundles from target platform") {
-				@Override
-				protected IStatus run(IProgressMonitor monitor) {
-					return target.resolve(monitor);
-				}
-			};
-			job.schedule();
-			try {
-				job.join();
-			} catch (InterruptedException e1) {
-			}
-
-			// TODO Performance hack, avoid p2 pinging remote sites or downloading at this time
-			for (int i = 0; i < locations.length; i++) {
-				if (locations[i] instanceof IUBundleContainer) {
-					((IUBundleContainer) locations[i]).setRemoteFetch(true);
-				}
-			}
-
-			if (job.getResult().getSeverity() == IStatus.CANCEL) {
-				// TODO Do we want to schedule a job for later to complete?
-				return new URL[0];
-			}
+		// TODO resolution cancelled, can we reschedule?
+		if (target == null) {
+			return new URL[0];
 		}
 
 		// Log any known issues with the target platform to warn user
diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/SourceLocationManager.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/SourceLocationManager.java
index c238af3..2d5336e 100644
--- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/SourceLocationManager.java
+++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/SourceLocationManager.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- *  Copyright (c) 2000, 2012 IBM Corporation and others.
+ *  Copyright (c) 2000, 2013 IBM Corporation and others.
  *  All rights reserved. This program and the accompanying materials
  *  are made available under the terms of the Eclipse Public License v1.0
  *  which accompanies this distribution, and is available at
@@ -177,8 +177,12 @@
 	}
 
 	/**
+	 * While there is no UI to access this feature, we still support additional
+	 * source locations being provided via preference.
+	 * 
 	 * @return array of source locations that have been specified by the user
 	 */
+	@SuppressWarnings("deprecation")
 	public List<SourceLocation> getUserLocations() {
 		List<SourceLocation> userLocations = new ArrayList<SourceLocation>();
 		String pref = PDECore.getDefault().getPreferencesManager().getString(P_SOURCE_LOCATIONS);
diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TargetPlatformHelper.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TargetPlatformHelper.java
index 0657fe1..969e952 100644
--- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TargetPlatformHelper.java
+++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TargetPlatformHelper.java
@@ -15,12 +15,15 @@
 import java.io.*;
 import java.util.*;
 import org.eclipse.core.runtime.*;
+import org.eclipse.core.runtime.jobs.Job;
 import org.eclipse.jdt.launching.JavaRuntime;
 import org.eclipse.jdt.launching.environments.IExecutionEnvironment;
 import org.eclipse.osgi.service.resolver.BundleDescription;
 import org.eclipse.osgi.service.resolver.State;
 import org.eclipse.osgi.util.ManifestElement;
 import org.eclipse.pde.core.plugin.*;
+import org.eclipse.pde.core.target.ITargetDefinition;
+import org.eclipse.pde.core.target.ITargetPlatformService;
 import org.eclipse.pde.internal.build.IPDEBuildConstants;
 import org.eclipse.pde.internal.core.ifeature.IFeatureModel;
 import org.eclipse.pde.internal.core.util.CoreUtility;
@@ -422,6 +425,71 @@
 		return getPDEState().getState();
 	}
 
+	/**
+	 * Utility method to get the workspace active target platform and ensure it
+	 * has been resolved.  This is potentially a long running operation. If a 
+	 * monitor is provided, progress is reported to it.  If a monitor is not 
+	 * provided, the resolution will run in a named {@link Job}, but this
+	 * thread will join to run synchronously.
+	 * 
+	 * @param monitor optional progress monitor to report progress to
+	 * @return a resolved target definition or <code>null</code> if the resolution was cancelled
+	 * @throws CoreException if there is a problem accessing the workspace target definition
+	 */
+	public static ITargetDefinition getWorkspaceTargetResolved(IProgressMonitor monitor) throws CoreException {
+		ITargetPlatformService service = (ITargetPlatformService) PDECore.getDefault().acquireService(ITargetPlatformService.class.getName());
+		if (service == null) {
+			throw new CoreException(new Status(IStatus.ERROR, PDECore.PLUGIN_ID, "Could not acquire target platform service"));
+		}
+		final ITargetDefinition target = service.getWorkspaceTargetDefinition();
+
+		// Don't resolve again if we don't have to
+		if (!target.isResolved()) {
+
+			// TODO Performance hack, avoid p2 pinging remote sites or downloading at this time
+//			ITargetLocation[] locations = target.getTargetLocations();
+//			for (int i = 0; i < locations.length; i++) {
+//				if (locations[i] instanceof IUBundleContainer) {
+//					((IUBundleContainer) locations[i]).setRemoteFetch(false);
+//				}
+//			}
+
+			if (monitor == null) {
+				// Resolve the target definition in a separate job to allow cancellation
+				Job job = new Job("Loading target platform") {
+					@Override
+					protected IStatus run(IProgressMonitor monitor) {
+						return target.resolve(monitor);
+					}
+				};
+				job.schedule();
+				try {
+					job.join();
+				} catch (InterruptedException e1) {
+				}
+				if (job.getResult().getSeverity() == IStatus.CANCEL) {
+					// TODO Do we want to schedule a job for later to complete?  Is returning null the best option
+					return null;
+				}
+
+			} else {
+				target.resolve(monitor);
+				if (monitor.isCanceled()) {
+					return null;
+				}
+			}
+
+			// TODO Performance hack, avoid p2 pinging remote sites or downloading at this time
+//			for (int i = 0; i < locations.length; i++) {
+//				if (locations[i] instanceof IUBundleContainer) {
+//					((IUBundleContainer) locations[i]).setRemoteFetch(true);
+//				}
+//			}
+
+		}
+		return target;
+	}
+
 	public static Map<Long, String> getPatchMap(PDEState state) {
 		HashMap<Long, String> properties = new HashMap<Long, String>();
 		IPluginModelBase[] models = PluginRegistry.getActiveModels();
diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TargetPreferenceModifyListener.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TargetPreferenceModifyListener.java
index 5212dff..c5392e1 100644
--- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TargetPreferenceModifyListener.java
+++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/TargetPreferenceModifyListener.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2009, 2010 IBM Corporation and others.
+ * Copyright (c) 2009, 2013 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -40,12 +40,6 @@
 				node.remove(ICoreConstants.POOLED_URLS);
 				node.remove(ICoreConstants.PROGRAM_ARGS);
 				node.remove(ICoreConstants.OS);
-				for (int i = 0; i < 4; i++) {
-					StringBuffer key = new StringBuffer();
-					key.append(ICoreConstants.SAVED_PLATFORM);
-					key.append(i);
-					node.remove(key.toString());
-				}
 				node.remove(ICoreConstants.TARGET_MODE);
 				node.remove(ICoreConstants.TARGET_PLATFORM_REALIZATION);
 				node.remove(ICoreConstants.TARGET_PROFILE);
diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java
index 20e5b8a..bc53ffb 100644
--- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java
+++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetPlatformService.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2008, 2012 IBM Corporation and others.
+ * Copyright (c) 2008, 2013 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -264,11 +264,12 @@
 
 		// TODO Go through all uses of this and update to getWorkspaceTargetDefinition
 
+		// TODO Removing this represents a binary compatible API change
 		// If the plug-in registry has not been initialized we may not have a target set, getting the start forces the init
-		PluginModelManager manager = PDECore.getDefault().getModelManager();
-		if (!manager.isInitialized()) {
-			manager.getExternalModelManager();
-		}
+//		PluginModelManager manager = PDECore.getDefault().getModelManager();
+//		if (!manager.isInitialized()) {
+//			manager.getExternalModelManager();
+//		}
 
 		PDEPreferencesManager preferences = PDECore.getDefault().getPreferencesManager();
 		String memento = preferences.getString(ICoreConstants.WORKSPACE_TARGET_HANDLE);
@@ -282,7 +283,8 @@
 	 * @see org.eclipse.pde.core.target.ITargetPlatformService#getWorkspaceTargetDefinition()
 	 */
 	public synchronized ITargetDefinition getWorkspaceTargetDefinition() throws CoreException {
-		if (fWorkspaceTarget != null) {
+		// TODO Check if the preference has changed, might be better to listen for preference changes
+		if (fWorkspaceTarget != null && fWorkspaceTarget.getHandle().equals(getWorkspaceTargetHandle())) {
 			return fWorkspaceTarget;
 		}
 
@@ -312,13 +314,13 @@
 	 * active target. If there are no targets that correspond to workspace settings
 	 * a new definition is created. 
 	 */
-	private synchronized void initDefaultTargetPlatformDefinition() {
+	private void initDefaultTargetPlatformDefinition() {
 		String memento = PDECore.getDefault().getPreferencesManager().getString(ICoreConstants.WORKSPACE_TARGET_HANDLE);
 		if (memento == null || memento.equals("")) { //$NON-NLS-1$
 			if (PDECore.DEBUG_MODEL) {
 				System.out.println("No target memento, loading target info from old preferences.");
 			}
-			// no workspace target handle set, check if any targets are equivalent to current settings
+			// no workspace target handle set, check if any known target definitions are equivalent to current settings
 			ITargetHandle[] targets = getTargets(null);
 			// create target platform from current workspace settings
 			TargetDefinition curr = (TargetDefinition) newTarget();