Fix for bug 73156 -- [Managed Build] No validation of data in Manage Config Dialog.

The fix has two parts. When the user adds a config from the dialog, it doesn't get added to the MBS until the hit OK. The user could delete and restore the new config several times befores that. The dialog now holds on to these deleted "new" configs in a separate data structure. The UI has also been updated to check for identity in a case-insensitive way and to make sure no invalid characters are in the name. The error dialog has been removed and a status line isused for the message.

2 files changed, 129 insertions, 72 deletions

diff --git a/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/ManageConfigDialog.java b/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/ManageConfigDialog.java
index f2ab6057607..08a4b0a4c3d 100644
--- a/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/ManageConfigDialog.java
+++ b/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/ManageConfigDialog.java
@@ -17,7 +17,6 @@ import java.util.TreeMap;
 
 import org.eclipse.cdt.managedbuilder.core.IConfiguration;
 import org.eclipse.cdt.managedbuilder.core.ITarget;
-import org.eclipse.cdt.managedbuilder.core.ManagedBuildManager;
 import org.eclipse.cdt.managedbuilder.internal.ui.ManagedBuilderUIMessages;
 import org.eclipse.core.resources.IProject;
 import org.eclipse.jface.dialogs.Dialog;
@@ -70,10 +69,12 @@ public class ManageConfigDialog extends Dialog {
 	private String makeCommand;
 	// The target the configs belong to
 	private ITarget managedTarget;
-	// Map of new configurations chosen by the user
-	private SortedMap newConfigs;
+	/** All new configs added by the user but not yet part of target */
+	private SortedMap newAddedConfigs;
+	/** All new configs removed by the user but not yet part of target */
+	private SortedMap removedNewConfigs;
 	// The title of the dialog.
-	private String title = ""; //$NON-NLS-1$
+	final private String title;
 	// State of the check box on exit
 	private boolean useDefaultMake;
 	
@@ -127,6 +128,7 @@ public class ManageConfigDialog extends Dialog {
 		} else {
 			useDefaultMake = true;
 			artifactName = managedTarget.getArtifactName();
+			artifactExt = managedTarget.getArtifactExtension();
 		}
 		super.buttonPressed(buttonId);
 	}
@@ -421,6 +423,7 @@ public class ManageConfigDialog extends Dialog {
 	 * @return
 	 */
 	public ArrayList getDeletedConfigIds() {
+		// Iterate over the list of deleted configurations
 		return new ArrayList(getDeletedConfigs().values());
 	}
 
@@ -449,12 +452,34 @@ public class ManageConfigDialog extends Dialog {
 	 * @return Map   
 	 */
 	public SortedMap getNewConfigs() {
-		if (newConfigs == null) {
-			newConfigs = new TreeMap();
+		if (newAddedConfigs == null) {
+			newAddedConfigs = new TreeMap();
 		}
-		return newConfigs;
+		return newAddedConfigs;
 	}
 
+	// Answers a list of new configuration names that have been added-- 
+	// or added and removed--by the user, but that have not yet been added 
+	// to the target
+	private ArrayList getNewConfigNames() {
+		ArrayList names = new ArrayList();
+		names.addAll(getNewConfigs().keySet());
+		names.addAll(getRemovedNewConfigs().keySet());
+		return names;
+	}
+	
+	
+	// This data structure hangs on to a new configuration that is added
+	// by the user, then removed before it is added to the target. This is 
+	// a required bookeeping step because the user may change their minds and 
+	// restore the deleted configuration. 
+	private SortedMap getRemovedNewConfigs() {
+		if (removedNewConfigs == null) {
+			removedNewConfigs = new TreeMap();
+		}
+		return removedNewConfigs;
+	}
+	
 	/*
 	 * @return the <code>IProject</code> associated with the target
 	 */
@@ -466,18 +491,10 @@ public class ManageConfigDialog extends Dialog {
 	 * Event handler for the add button
 	 */
 	protected void handleNewPressed() {
-		// Find the defined target
-		ITarget parentTarget = null;
-		ITarget [] targets = ManagedBuildManager.getDefinedTargets(getProject());
-		for (int i = 0; i < targets.length; i++) {
-			ITarget target = targets[i];
-			if (target.getId().equals(managedTarget.getParent().getId())) {
-				parentTarget = target;
-				break;
-			}
-		}
+		// Pop-up a dialog to properly handle the request
 		NewConfigurationDialog dialog = new NewConfigurationDialog(getShell(), 
-																   managedTarget, 
+																   managedTarget,
+																   getNewConfigNames(),
 																   ManagedBuilderUIMessages.getResourceString(CONF_DLG));
 		if (dialog.open() == NewConfigurationDialog.OK) {
 			// Get the new name and configuration to base the new config on
@@ -499,18 +516,18 @@ public class ManageConfigDialog extends Dialog {
 		int selectionIndex = currentConfigList.getSelectionIndex();
 		if (selectionIndex != -1){
 			String selectedConfigName = currentConfigList.getItem(selectionIndex);
-			String selectedConfigId = null;
 			
-			// If this is a newly added config, remove it from that map
+			// If this is a newly added config, remove it from the new map 
+			// and add it to a special map to support the restore use case
 			if (getNewConfigs().containsKey(selectedConfigName)) {
 				IConfiguration selectedConfig = (IConfiguration) getNewConfigs().get(selectedConfigName); 
-				selectedConfigId = selectedConfig.getId();
+				getRemovedNewConfigs().put(selectedConfigName, selectedConfig);
 				getNewConfigs().remove(selectedConfigName);
 			} else {
 				// If it is not a new item, the ID is in the existing list
-				selectedConfigId = (String) getExistingConfigs().get(selectedConfigName);
+				String selectedConfigId = (String) getExistingConfigs().get(selectedConfigName);
+				getDeletedConfigs().put(selectedConfigName, selectedConfigId);
 			}
-			getDeletedConfigs().put(selectedConfigName, selectedConfigId);
 
 			// Clean up the UI lists
 			currentConfigList.remove(selectionIndex);
@@ -527,21 +544,21 @@ public class ManageConfigDialog extends Dialog {
 	protected void handleRestorePressed() {
 		// Determine which configuration was selected
 		int selectionIndex = deletedConfigList.getSelectionIndex();
-		// Move the selected element from the deleted list to the current list
-		if (selectionIndex != -1){
+		// Move the selected element from the correct deleted list to the current list
+		if (selectionIndex != -1){			
 			// Get the name of the item to delete
 			String selectedConfigName = deletedConfigList.getItem(selectionIndex);
-			String selectedConfigId = (String) getDeletedConfigs().get(selectedConfigName);
 			
-			// If this was a new config (it won't be in the existing list) then add it back there
-			if (!getExistingConfigs().containsKey(selectedConfigName)) {
-				IConfiguration restoredConfig = managedTarget.getConfiguration(selectedConfigId);
+			// The deleted config may be one of the existing configs or one of the
+			// new configs that have not been added to the target yet
+			if (getRemovedNewConfigs().containsKey(selectedConfigName)) {
+				IConfiguration restoredConfig = (IConfiguration) getRemovedNewConfigs().get(selectedConfigName);
 				getNewConfigs().put(selectedConfigName, restoredConfig);
+				getRemovedNewConfigs().remove(selectedConfigName);
+			} else {
+				getDeletedConfigs().remove(selectedConfigName);
 			}
 			
-			// Remove it from the deleted map
-			getDeletedConfigs().remove(selectedConfigName);
-
 			// Clean up the UI
 			deletedConfigList.remove(selectionIndex);
 			deletedConfigList.setSelection(selectionIndex - 1);
diff --git a/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/NewConfigurationDialog.java b/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/NewConfigurationDialog.java
index 082e15c71ea..ce433f2fe49 100644
--- a/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/NewConfigurationDialog.java
+++ b/build/org.eclipse.cdt.managedbuilder.ui/src/org/eclipse/cdt/managedbuilder/ui/properties/NewConfigurationDialog.java
@@ -11,12 +11,14 @@ package org.eclipse.cdt.managedbuilder.ui.properties;
  * IBM Rational Software - Initial API and implementation
 ***********************************************************************/
 
+import java.util.ArrayList;
+
+import org.eclipse.cdt.internal.ui.dialogs.StatusDialog;
+import org.eclipse.cdt.internal.ui.dialogs.StatusInfo;
 import org.eclipse.cdt.managedbuilder.core.IConfiguration;
 import org.eclipse.cdt.managedbuilder.core.ITarget;
 import org.eclipse.cdt.managedbuilder.internal.ui.ManagedBuilderUIMessages;
-import org.eclipse.jface.dialogs.Dialog;
 import org.eclipse.jface.dialogs.IDialogConstants;
-import org.eclipse.jface.dialogs.MessageDialog;
 import org.eclipse.swt.SWT;
 import org.eclipse.swt.events.ModifyEvent;
 import org.eclipse.swt.events.ModifyListener;
@@ -34,7 +36,7 @@ import org.eclipse.swt.widgets.Label;
 import org.eclipse.swt.widgets.Shell;
 import org.eclipse.swt.widgets.Text;
 
-public class NewConfigurationDialog extends Dialog {
+public class NewConfigurationDialog extends StatusDialog {
 	// String constants
 	private static final String PREFIX = "NewConfiguration";	//$NON-NLS-1$
 	private static final String LABEL = PREFIX + ".label";	//$NON-NLS-1$
@@ -43,37 +45,43 @@ public class NewConfigurationDialog extends Dialog {
 	private static final String GROUP = LABEL + ".group";	//$NON-NLS-1$
 	private static final String COPY = LABEL + ".copy";	//$NON-NLS-1$
 	private static final String CLONE = LABEL + ".clone";	//$NON-NLS-1$
-	private static final String TITLE = ERROR + ".title";	//$NON-NLS-1$	
 	private static final String DUPLICATE = ERROR + ".duplicateName";	//$NON-NLS-1$	
 
 	// Widgets
 	private Button btnClone;
 	private Button btnCopy;
-	private Button btnOk;
 	private Text configName;
 	private Combo copyConfigSelector;
 	private Combo cloneConfigSelector;
 		
-	// Bookeeping
+	// Bookeeping variable
 	private boolean clone;
+	/** Default configurations defined in the toolchain description */
 	private IConfiguration[] defaultConfigs;
+	/** Configurations defined in the target */
 	private IConfiguration[] definedConfigs;
 	private IConfiguration parentConfig;
 	private ITarget target;
 	private String newName;
-	private String title = ""; //$NON-NLS-1$
+	/** A list containing config names that have been defined but not added to the target */
+	final private ArrayList reservedNames;
+	final private String title;
 
 	
 	/**
 	 * @param parentShell
+	 * @param managedTarget
+	 * @param nameList A list of names that have been added by the user but have not yet been added to the target 
+	 * @param title The title of the dialog
 	 */
-	protected NewConfigurationDialog(Shell parentShell, ITarget managedTarget, String title) {
+	protected NewConfigurationDialog(Shell parentShell, ITarget managedTarget, ArrayList nameList, String title) {
 		super(parentShell);
 		this.title = title;
 		setShellStyle(getShellStyle()|SWT.RESIZE);
 		newName = new String();
 		parentConfig = null;
 		this.target = managedTarget;
+		reservedNames = nameList;
 		
 		// The default behaviour is to clone the settings
 		clone = true;
@@ -133,13 +141,12 @@ public class NewConfigurationDialog extends Dialog {
 	 * @see org.eclipse.jface.dialogs.Dialog#createButtonsForButtonBar(org.eclipse.swt.widgets.Composite)
 	 */
 	protected void createButtonsForButtonBar(Composite parent) {
-		btnOk = createButton(parent, IDialogConstants.OK_ID, IDialogConstants.OK_LABEL, true);
-		createButton(parent, IDialogConstants.CANCEL_ID, IDialogConstants.CANCEL_LABEL, false);
+		super.createButtonsForButtonBar(parent);
 		configName.setFocus();
 		if (configName != null) {
 			configName.setText(newName);
 		}
-		updateButtonState();
+		validateState();
 	}
 
 	protected Control createDialogArea(Composite parent) {
@@ -163,7 +170,7 @@ public class NewConfigurationDialog extends Dialog {
 		configName.setLayoutData(gd);
 		configName.addModifyListener(new ModifyListener() {
 			public void modifyText(ModifyEvent e) {
-				updateButtonState();
+				validateState();
 			}
 		});
 		
@@ -201,7 +208,7 @@ public class NewConfigurationDialog extends Dialog {
 		copyConfigSelector.setLayoutData(gd);
 		copyConfigSelector.addSelectionListener(new SelectionAdapter() {
 			public void widgetSelected(SelectionEvent e) {
-				updateButtonState();		
+				validateState();		
 			}
 		});	
 		copyConfigSelector.setEnabled(false);
@@ -225,14 +232,13 @@ public class NewConfigurationDialog extends Dialog {
 		cloneConfigSelector.setLayoutData(gd);
 		cloneConfigSelector.addSelectionListener(new SelectionAdapter() {
 			public void widgetSelected(SelectionEvent e) {
-				updateButtonState();		
+				validateState();		
 			}
 		});	
 
 		return composite;
 	}
 
-
 	/**
 	 * @return the <code>IConfiguration</code> the user selected as 
 	 * the parent of the new configuration.
@@ -286,28 +292,19 @@ public class NewConfigurationDialog extends Dialog {
 	 */
 	protected boolean isDuplicateName(String newName) {
 		// Return true if there is already a config of that name defined on the target
-		IConfiguration [] configs = target.getConfigurations();
-		for (int index = 0; index < configs.length; index++) {
-			IConfiguration configuration = configs[index];
-			if (configuration.getName() == newName) {
+		for (int index = 0; index < definedConfigs.length; index++) {
+			IConfiguration configuration = definedConfigs[index];
+			if (configuration.getName().equalsIgnoreCase(newName)) {
 				return true;
 			}
 		}
+		if (reservedNames.contains(newName)) {
+			return true;
+		}
 		return false;
 	}
 	
 	/* (non-Javadoc)
-	 * Enable the OK button if there is a valid name in the text widget
-	 * and there is a valid selection in the base configuration combo 
-	 */
-	private void updateButtonState() {
-		if (btnOk != null) {
-			int selectionIndex = copyConfigSelector.getSelectionIndex();			
-			btnOk.setEnabled(validateName() && selectionIndex != -1);
-		}
-	}
-
-	/* (non-Javadoc)
 	 * Radio button selection event handler calls this helper method to 
 	 * enable or disable the radio buttons.
 	 */
@@ -316,17 +313,60 @@ public class NewConfigurationDialog extends Dialog {
 		copyConfigSelector.setEnabled(!clone);
 	}
 
-	private boolean validateName() {
-		String currentName = configName.getText().trim(); 
+	/* (non-Javadoc)
+	 * Checks the argument fro whitespaces, and invalid directory name characters. 
+	 * @param name
+	 * @return <I>true</i> is the name is a valid directory name with no whitespaces
+	 */
+	private boolean validateName(String name) {
+		// Iterate over the name checking for bad characters
+		char[] chars = name.toCharArray();
+		for (int index = 0; index < chars.length; ++index) {
+			// For now no whitespaces
+			if (Character.isWhitespace(chars[index])) {
+				return false;
+			}
+			// Config name must be a valid dir name too, so we ban "\ / : * ? " < >" in the names
+			if (!Character.isLetterOrDigit(chars[index])) {
+				switch (chars[index]) {
+				case '/':
+				case '\\':
+				case ':':
+				case '*':
+				case '?':
+				case '\"':
+				case '<':
+				case '>':
+					return false;
+				default:
+					break;
+				}
+			}
+		}
+		return true;
+	}
+
+	/* (non-Javadoc)
+	 * Update the status message and button state based on the input selected
+	 * by the user
+	 * 
+	 */
+	private void validateState() {
+		StatusInfo status= new StatusInfo();
+		String currentName = configName.getText(); 
 		int nameLength = currentName.length();
 		// Make sure the name is not a duplicate
-		if (isDuplicateName(currentName)) {
-			MessageDialog.openError(getShell(), 
-					ManagedBuilderUIMessages.getResourceString(TITLE), 
-					ManagedBuilderUIMessages.getFormattedString(DUPLICATE, currentName)); //$NON-NLS-1$
-			return false;
-		}
-		// TODO make sure there are no invalid chars in name
-		return (nameLength > 0);
+		if (nameLength == 0) {
+			// TODO Create a decent I18N string to describe this problem
+			status.setError("");	//$NON-NLS-1$
+		} else if (isDuplicateName(currentName)) {
+			status.setError(ManagedBuilderUIMessages.getFormattedString(DUPLICATE, currentName));
+		} else if (!validateName(currentName)) {
+			// TODO Create a decent I18N string to describe this problem
+			status.setError("");	//$NON-NLS-1$
+		} 
+		
+		updateStatus(status);
+		return;
 	}
 }