431694: [All diagrams] Problem with Surfboard display after Delete ->

Undo
https://bugs.eclipse.org/bugs/show_bug.cgi?id=431694

- Improve the CSS Engine robustness against orphaned views
- Activate the tests

4 files changed, 71 insertions, 38 deletions

diff --git a/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/dom/GMFElementAdapter.java b/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/dom/GMFElementAdapter.java
index 438c74a2882..7a325c3cfef 100644
--- a/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/dom/GMFElementAdapter.java
+++ b/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/dom/GMFElementAdapter.java
@@ -194,6 +194,12 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	 */

 	public GMFElementAdapter(View view, ExtendedCSSEngine engine) {

 		super(view, engine);

+

+		//Bug 431694: Don't instantiate an ElementAdapter for an Orphaned view

+		if(view == null || view.getDiagram() == null) {

+			throw new IllegalArgumentException("Cannot handle orphaned view : " + view);

+		}

+

 		notationElement = view;

 		listenNotationElement();

 	}

@@ -257,6 +263,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	 * Returns the first parent Notation Element representing a different semantic object

 	 * than self.

 	 */

+	@Override

 	public Node getParentNode() {

 		if(parentNode == null) {

 			View gmfElement = notationElement;

@@ -289,6 +296,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public NodeList getChildNodes() {

 		return this;

 	}

@@ -296,6 +304,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public String getNamespaceURI() {

 		if(namespaceURI == null) {

 			namespaceURI = EMFHelper.getQualifiedName(getSemanticElement().eClass().getEPackage(), ".");

@@ -306,6 +315,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public String getCSSId() {

 		return getCSSID(notationElement);

 	}

@@ -313,6 +323,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public String getCSSClass() {

 		return getCSSClass(notationElement);

 	}

@@ -320,6 +331,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public String getCSSStyle() {

 		return getCSSStyle(notationElement);

 	}

@@ -460,6 +472,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public Node item(int index) {

 		return getChildren()[index];

 	}

@@ -467,6 +480,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public int getLength() {

 		return getChildren().length;

 	}

@@ -503,20 +517,20 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 

 		/*

 		 * <--------------------

-		 *

+		 * 

 		 * //Allows both notations Class > Property and Class > Compartment > Property

-		 *

+		 * 

 		 * //FIXME: The Tree is computed through "getParentNode". "getChildren" is barely used. Moreover,

 		 * //there is a mapping between Notation element and DOM element, which makes it impossible to associate the same

 		 * //notation element to different DOM elements.

-		 *

+		 * 

 		 * // for(EObject child : notationElement.eContents()) {

 		 * // if(child instanceof BasicCompartment) {

 		 * // //Add the Compartment's children to this' children

 		 * // childList.addAll(Arrays.asList(computeChildren((View)child, engine)));

 		 * // }

 		 * // }

-		 *

+		 * 

 		 * -------------------->

 		 */

 

@@ -560,6 +574,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 	 * Source: cssStyleListener

 	 */

 	//Change incoming from one of the cssCustomStyles (class, id, local style or diagram stylesheets)

+	@Override

 	public void handleChange(ChangeEvent event) {

 		if(notationElement instanceof CSSDiagram) {

 			//TODO: Use a finer grained event (We should reset only when the

@@ -678,9 +693,10 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 

 	/*

 	 * (non-Javadoc)

-	 *

+	 * 

 	 * @see org.eclipse.papyrus.infra.gmfdiag.css.notation.StatefulView#addStates(java.util.Set)

 	 */

+	@Override

 	public void addStates(Set<String> states) {

 		for(String state : states) {

 			this.pseudoInstances.add(new StringIgnoreCase(state));

@@ -690,9 +706,10 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 

 	/*

 	 * (non-Javadoc)

-	 *

+	 * 

 	 * @see org.eclipse.papyrus.infra.gmfdiag.css.notation.StatefulView#removeStates(java.util.Set)

 	 */

+	@Override

 	public void removeStates(Set<String> states) {

 		for(String state : states) {

 			this.pseudoInstances.remove(new StringIgnoreCase(state));

@@ -700,6 +717,7 @@ public class GMFElementAdapter extends ElementAdapter implements NodeList, IChan
 		getEngine().notifyChange(this);

 	}

 

+	@Override

 	public Set<String> getStates() {

 		Set<String> result = new HashSet<String>();

 		for(StringIgnoreCase element : pseudoInstances) {

diff --git a/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/engine/DiagramCSSEngine.java b/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/engine/DiagramCSSEngine.java
index 4d3e95c30bf..0ce237f7b86 100644
--- a/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/engine/DiagramCSSEngine.java
+++ b/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/engine/DiagramCSSEngine.java
@@ -107,6 +107,7 @@ public class DiagramCSSEngine extends ExtendedCSSEngineImpl implements IChangeLi
 		return (EObject)element; //GMFElement

 	}

 

+	@Override

 	public void handleChange(ChangeEvent event) {

 		resetCache();

 		DiagramHelper.setNeedsRefresh();

@@ -122,6 +123,11 @@ public class DiagramCSSEngine extends ExtendedCSSEngineImpl implements IChangeLi
 		EObject notationElement = getNativeWidget(node);

 		View canonicalNotationElement = CSSDOMSemanticElementHelper.findPrimaryView(notationElement);

 

+		//Orphaned view

+		if(canonicalNotationElement.getDiagram() == null) {

+			return null;

+		}

+

 		//A View and a Compartment associated to the same Semantic Element

 		//must have the same XML Element. They share the same children.

 		//This is required to map the Semantic model (Used by the CSS selectors)

diff --git a/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/engine/ExtendedCSSEngineImpl.java b/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/engine/ExtendedCSSEngineImpl.java
index 73369e90b40..463ddf47f6a 100644
--- a/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/engine/ExtendedCSSEngineImpl.java
+++ b/plugins/infra/gmfdiag/css/org.eclipse.papyrus.infra.gmfdiag.css/src/org/eclipse/papyrus/infra/gmfdiag/css/engine/ExtendedCSSEngineImpl.java
@@ -28,6 +28,7 @@ import org.eclipse.e4.ui.css.core.dom.parsers.CSSParser;
 import org.eclipse.e4.ui.css.core.dom.parsers.CSSParserFactory;

 import org.eclipse.e4.ui.css.core.dom.parsers.ICSSParserFactory;

 import org.eclipse.e4.ui.css.core.dom.properties.converters.ICSSValueConverter;

+import org.eclipse.e4.ui.css.core.impl.dom.CSSStyleDeclarationImpl;

 import org.eclipse.e4.ui.css.core.impl.engine.AbstractCSSEngine;

 import org.eclipse.e4.ui.css.core.impl.sac.CSSConditionFactoryImpl;

 import org.eclipse.e4.ui.css.core.impl.sac.CSSSelectorFactoryImpl;

@@ -133,6 +134,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public CSSValue retrievePropertyValue(Object node, String property) {

 		if(node == null || property == null) {

 			return null;

@@ -149,6 +151,9 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 

 	private CSSStyleDeclaration getStyleDeclaration(Object node, String pseudo) {

 		Element element = getElement(node);

+		if(element == null) {

+			return new CSSStyleDeclarationImpl(null);

+		}

 		if(!declarationsCache.containsKey(element)) {

 			declarationsCache.put(element, getViewCSS().getComputedStyle(element, pseudo));

 		}

@@ -180,6 +185,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public void resetCache() {

 		declarationsCache.clear();

 		availableClasses.clear();

@@ -257,6 +263,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	 * changed. Resets this engine and forwards the event to children

 	 * stylesheets

 	 */

+	@Override

 	public void styleSheetChanged(ExtendedCSSEngine owner) {

 		reset();

 	}

@@ -273,6 +280,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public void addStyleSheetChangeListener(StyleSheetChangeListener listener) {

 		styleSheetListeners.add(listener);

 	}

@@ -280,6 +288,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public void removeStyleSheetChangedListener(StyleSheetChangeListener listener) {

 		styleSheetListeners.remove(listener);

 	}

@@ -287,6 +296,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	/**

 	 * {@inheritDoc}

 	 */

+	@Override

 	public ExtendedStyleSheetList getAllStylesheets() {

 		if(styleSheetsList == null) {

 			parseStyleSheets();

@@ -348,6 +358,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	 *

 	 * Source: GMFElementAdapter

 	 */

+	@Override

 	public void notifyChange(Element elementAdapter) {

 		resetCache(); //TODO: We should only refresh a subset of the cache

 

@@ -356,6 +367,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 		DiagramHelper.setNeedsRefresh();

 		Display.getDefault().asyncExec(new Runnable() {

 

+			@Override

 			public void run() {

 				DiagramHelper.refreshDiagrams(); //TODO: Contextual refresh

 			}

@@ -370,6 +382,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	 *

 	 * Source: GMFElementAdapter

 	 */

+	@Override

 	public void handleDispose(Object nativeWidget) {

 		super.handleWidgetDisposed(nativeWidget);

 	}

@@ -381,6 +394,7 @@ public abstract class ExtendedCSSEngineImpl extends AbstractCSSEngine implements
 	 *

 	 * @see #resetCache()

 	 */

+	@Override

 	public void reapply() {

 		resetCache();

 	}

diff --git a/tests/junit/plugins/uml/org.eclipse.papyrus.diagram.common.tests/test/org/eclipse/papyrus/diagram/common/tests/css/Bug431694_UndoDeleteTest.java b/tests/junit/plugins/uml/org.eclipse.papyrus.diagram.common.tests/test/org/eclipse/papyrus/diagram/common/tests/css/Bug431694_UndoDeleteTest.java
index f5d6867074d..8d756ea2319 100644
--- a/tests/junit/plugins/uml/org.eclipse.papyrus.diagram.common.tests/test/org/eclipse/papyrus/diagram/common/tests/css/Bug431694_UndoDeleteTest.java
+++ b/tests/junit/plugins/uml/org.eclipse.papyrus.diagram.common.tests/test/org/eclipse/papyrus/diagram/common/tests/css/Bug431694_UndoDeleteTest.java
@@ -1,6 +1,6 @@
 /*****************************************************************************

  * Copyright (c) 2014 CEA LIST.

- * 

+ *

  * 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

@@ -28,10 +28,7 @@ import org.eclipse.papyrus.commands.wrappers.GEFtoEMFCommandWrapper;
 import org.eclipse.papyrus.diagram.tests.canonical.AbstractPapyrusTestCase;

 import org.eclipse.papyrus.infra.core.services.ServiceException;

 import org.eclipse.papyrus.infra.gmfdiag.css.helper.CSSHelper;

-import org.eclipse.papyrus.infra.gmfdiag.css.theme.Theme;

-import org.eclipse.papyrus.infra.gmfdiag.css.theme.ThemeManager;

 import org.eclipse.papyrus.junit.utils.DiagramUtils;

-import org.eclipse.papyrus.junit.utils.classification.FailingTest;

 import org.eclipse.papyrus.junit.utils.tests.AbstractEditorTest;

 import org.eclipse.swt.widgets.Display;

 import org.eclipse.uml2.uml.Package;

@@ -58,7 +55,7 @@ public class Bug431694_UndoDeleteTest extends AbstractEditorTest {
 

 	protected boolean operationFailed = false;

 

-	

+

 	@Before

 	public void prepareTest() throws Exception {

 		initModel(PROJECT_NAME, "model", getBundle());

@@ -68,25 +65,24 @@ public class Bug431694_UndoDeleteTest extends AbstractEditorTest {
 	 * Test with a {@link Package} with a css style already applied

 	 */

 	@Test

-	@FailingTest("Bug 431694")

 	public void testDeleteOnPackageWithStyle() throws Exception {

 		// check css on the package P1

 		// get Package 1 view on the open diagram

 		// get the rootModel

 		Assert.assertNotNull("RootModel is null", getRootUMLModel());

-		

+

 		Diagram mainDiagram = DiagramUtils.getNotationDiagram(getModelSet(), DIAGRAM_MAIN_NAME);

 		getPageManager().openPage(mainDiagram);

 		Assert.assertEquals("current opened diagram is not the expected one", mainDiagram.getName(), DIAGRAM_MAIN_NAME);

 

-		// check css is working 

+		// check css is working

 		Assert.assertTrue("CSS is not supported on the given model", CSSHelper.isCSSSupported(getModelSet()));

 		Shape package1View = DiagramUtils.findShape(mainDiagram, PACKAGE1);

 		if(package1View == null) {

 			return;

 		}

 		checkPackage1CSS(package1View);

-		

+

 		// delete P1

 		// get edit part for this view and send a delete request

 		IGraphicalEditPart packageEditPart = DiagramUtils.findEditPartforView(editor, package1View, IGraphicalEditPart.class);

@@ -97,39 +93,38 @@ public class Bug431694_UndoDeleteTest extends AbstractEditorTest {
 		Assert.assertNotEquals("This should not be an unexecutable command", command, UnexecutableCommand.INSTANCE);

 		assertTrue("command should be executable", command.canExecute());

 		execute(command);

-		Assert.assertNull("There should be no shape for this element: "+PACKAGE1, DiagramUtils.findShape(mainDiagram, PACKAGE1));

-				

+		Assert.assertNull("There should be no shape for this element: " + PACKAGE1, DiagramUtils.findShape(mainDiagram, PACKAGE1));

+

 		// undo

 		undo();

 

 		// check css on P1

 		Shape newPackage1View = DiagramUtils.findShape(mainDiagram, PACKAGE1);

-		Assert.assertNotNull("There should be a shape for this element: "+PACKAGE1, newPackage1View);

+		Assert.assertNotNull("There should be a shape for this element: " + PACKAGE1, newPackage1View);

 		checkPackage1CSS(newPackage1View);

-		

+

 	}

-	

+

 	/**

 	 * Test with a {@link Package} with a css style already applied

 	 */

 	@Test

-	@FailingTest("Bug 431694")

 	public void testDeleteOnClassNamedStyleFont() throws Exception {

 		// check css on the class ClassNamedStyleFont

 		Assert.assertNotNull("RootModel is null", getRootUMLModel());

-		

+

 		Diagram mainDiagram = DiagramUtils.getNotationDiagram(getModelSet(), DIAGRAM_MAIN_NAME);

 		getPageManager().openPage(mainDiagram);

 		Assert.assertEquals("current opened diagram is not the expected one", mainDiagram.getName(), DIAGRAM_MAIN_NAME);

 

-		// check css is working 

+		// check css is working

 		Assert.assertTrue("CSS is not supported on the given model", CSSHelper.isCSSSupported(getModelSet()));

 		Shape ClassNamedStyleFontView = DiagramUtils.findShape(mainDiagram, CLASS_NAMED_STYLE_FONT);

 		if(ClassNamedStyleFontView == null) {

 			return;

 		}

 		checkClassNamedStyleFontCSS(ClassNamedStyleFontView);

-		

+

 		// delete ClassNamedStyleFont

 		// get edit part for this view and send a delete request

 		IGraphicalEditPart ClassNamedStyleFontEditPart = DiagramUtils.findEditPartforView(editor, ClassNamedStyleFontView, IGraphicalEditPart.class);

@@ -140,34 +135,34 @@ public class Bug431694_UndoDeleteTest extends AbstractEditorTest {
 		Assert.assertNotEquals("This should not be an unexecutable command", command, UnexecutableCommand.INSTANCE);

 		assertTrue("command should be executable", command.canExecute());

 		execute(command);

-		Assert.assertNull("There should be no shape for this element: "+CLASS_NAMED_STYLE_FONT, DiagramUtils.findShape(mainDiagram, CLASS_NAMED_STYLE_FONT));

-		

+		Assert.assertNull("There should be no shape for this element: " + CLASS_NAMED_STYLE_FONT, DiagramUtils.findShape(mainDiagram, CLASS_NAMED_STYLE_FONT));

+

 		// undo

 		undo();

 

 		// check css on the new view for ClassNamedStyleFont

 		Shape newClassNamedStyleFontView = DiagramUtils.findShape(mainDiagram, CLASS_NAMED_STYLE_FONT);

-		Assert.assertNotNull("There should be a shape for this element: "+CLASS_NAMED_STYLE_FONT, newClassNamedStyleFontView);

+		Assert.assertNotNull("There should be a shape for this element: " + CLASS_NAMED_STYLE_FONT, newClassNamedStyleFontView);

 		checkClassNamedStyleFontCSS(newClassNamedStyleFontView);

-		

+

 	}

 

 	private void checkClassNamedStyleFontCSS(Shape classNamedStyleFontView) {

-		// default style: papyrus theme 

-		Assert.assertEquals("Invalid Fill color (Default): "+DiagramUtils.integerToRGBString(classNamedStyleFontView.getFillColor()), DiagramUtils.rgb(195, 215, 221), classNamedStyleFontView.getFillColor()); // Papyrus Theme = 

-		Assert.assertEquals("Gradient should be default (vertical)", classNamedStyleFontView.getGradient().getGradientStyle(), GradientStyle.VERTICAL);	// Papyrus Theme = 

+		// default style: papyrus theme

+		Assert.assertEquals("Invalid Fill color (Default): " + DiagramUtils.integerToRGBString(classNamedStyleFontView.getFillColor()), DiagramUtils.rgb(195, 215, 221), classNamedStyleFontView.getFillColor()); // Papyrus Theme =

+		Assert.assertEquals("Gradient should be default (vertical)", classNamedStyleFontView.getGradient().getGradientStyle(), GradientStyle.VERTICAL); // Papyrus Theme =

 		Assert.assertEquals("Invalid Gradient Color (Default)", DiagramUtils.rgb(255, 255, 255), classNamedStyleFontView.getGradient().getGradientColor1()); // Papyrus Theme = 

-		

+

 		// named style: font color is white

 		Assert.assertEquals("Invalid Font Color", DiagramUtils.rgb(255, 255, 255), classNamedStyleFontView.getFontColor()); // White font by the named style

 	}

 

 	protected void checkPackage1CSS(Shape packageShape) {

-		// named style: fill red and horizontal green gradient 

+		// named style: fill red and horizontal green gradient

 		Assert.assertEquals("Invalid Fill color", DiagramUtils.rgb(255, 0, 0), packageShape.getFillColor()); //Red = #FF0000

 		Assert.assertEquals("Gradient should be horizontal", packageShape.getGradient().getGradientStyle(), GradientStyle.HORIZONTAL);

 		Assert.assertEquals("Invalid Gradient Color", DiagramUtils.rgb(0, 255, 0), packageShape.getGradient().getGradientColor1()); // GREEN

-		

+

 		// unnamed style: font color is blue

 		Assert.assertEquals("Invalid gradient", DiagramUtils.rgb(0, 0, 255), packageShape.getFontColor()); // Blue font by the named style

 	}

@@ -182,7 +177,7 @@ public class Bug431694_UndoDeleteTest extends AbstractEditorTest {
 	protected Bundle getBundle() {

 		return org.eclipse.papyrus.uml.diagram.common.Activator.getDefault().getBundle();

 	}

-	

+

 	/**

 	 * Call {@link AbstractPapyrusTestCase#execute(Command) execute} on the UI

 	 * thread.

@@ -219,7 +214,7 @@ public class Bug431694_UndoDeleteTest extends AbstractEditorTest {
 	protected void assertLastOperationSuccessful() {

 		Assert.assertFalse("The operation failed. Look at the log, or put a breakpoint on ExecutionException or DefaultOperationHistory#notifyNotOK to find the cause.", this.operationFailed);

 	}

-	

+

 	/**

 	 * Reset the "operation failed" state. Call this before executing each

 	 * operation for which you want to test whether if failed with {@link AbstractPapyrusTestCase#assertLastOperationSuccessful()}.

@@ -227,7 +222,7 @@ public class Bug431694_UndoDeleteTest extends AbstractEditorTest {
 	protected void resetLastOperationFailedState() {

 		this.operationFailed = false;

 	}

-	

+

 	/** Execute the given command in the diagram editor. */

 	protected void execute(final Command command) {

 		resetLastOperationFailedState();

@@ -264,6 +259,6 @@ public class Bug431694_UndoDeleteTest extends AbstractEditorTest {
 		}

 		return null;

 	}

-	

+

 

 }