Move up null check in GEF viewer when the selected editpart is updated.

The selection inside a GEF viewer is assumed to always be non-null. But
because the (implicit) null check is done after the editpart is added to
the internal list, it leaves the viewer in an inconsistent state.

To avoid this, the null check is done earlier, so that the exception is
thrown before the internal model is modified.

Resolves #433

org.eclipse.gef.tests/src/org/eclipse/gef/test/GEFTestSuite.java

@@ -26,7 +26,8 @@
 	ToolUtilitiesTest.class,
 	DragEditPartsTrackerTest.class,
 	CommandStackTest.class,
-	RulerLayoutTests.class
+	RulerLayoutTests.class,
+	GraphicalViewerTest.class
 })
 public class GEFTestSuite {
 }

org.eclipse.gef.tests/src/org/eclipse/gef/test/GraphicalViewerTest.java

@@ -0,0 +1,56 @@
+/*******************************************************************************
+ * Copyright (c) 2024 Patrick Ziegler and others.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ * Contributors:
+ *     Patrick Ziegler - initial API and implementation
+ *******************************************************************************/
+
+package org.eclipse.gef.test;
+
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+import org.eclipse.jface.viewers.IStructuredSelection;
+import org.eclipse.jface.viewers.StructuredSelection;
+
+import org.eclipse.gef.GraphicalViewer;
+import org.eclipse.gef.ui.parts.GraphicalViewerImpl;
+
+import org.junit.Before;
+import org.junit.Test;
+
+public class GraphicalViewerTest {
+	private GraphicalViewer viewer;
+
+	@Before
+	public void setUp() {
+		viewer = new GraphicalViewerImpl();
+	}
+
+	/**
+	 * Appending a {@code null} edit part shouldn't leave the viewer in an
+	 * inconsistent state.
+	 */
+	@Test
+	public void testAppendNullSelection() {
+		assertThrows(NullPointerException.class, () -> viewer.appendSelection(null));
+		assertTrue(viewer.getSelectedEditParts().isEmpty());
+	}
+
+	/**
+	 * Setting a {@code null} edit part shouldn't leave the viewer in an
+	 * inconsistent state.
+	 */
+	@Test
+	public void testSetNullSelection() {
+		IStructuredSelection selection = new StructuredSelection(new Object[] { null });
+		assertThrows(NullPointerException.class, () -> viewer.setSelection(selection));
+		assertTrue(viewer.getSelectedEditParts().isEmpty());
+	}
+}

org.eclipse.gef/src/org/eclipse/gef/SelectionManager.java

@@ -14,9 +14,9 @@
 package org.eclipse.gef;
 
 import java.lang.reflect.Field;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 
 import org.eclipse.swt.widgets.Control;
@@ -72,6 +72,7 @@ public static SelectionManager createDefault() {
 	 * @since 3.2
 	 */
 	public void appendSelection(EditPart editpart) {
+		Objects.requireNonNull(editpart, "The selected edit part must not be null."); //$NON-NLS-1$
 		if (editpart != getFocus()) {
 			// Fix for 458416: adjust the focus through the viewer only (to give
 			// AbstractEditPartViewer a change to update its focusPart field).
@@ -275,16 +276,16 @@ public void setSelection(ISelection newSelection) {
 
 		@SuppressWarnings("unchecked")
 		List<EditPart> orderedSelection = structuredSelection.toList();
-		// Convert to HashSet to optimize performance.
-		Set<EditPart> hashset = new HashSet<>(orderedSelection);
+		// Convert to set to optimize performance. (Implicit null check)
+		Set<EditPart> set = Set.copyOf(orderedSelection);
 
 		// Fix for 458416: adjust the focus through the viewer only (to give
 		// AbstractEditPartViewer a change to update its focusPart field).
 		// AbstractEditPartViewer#setFocus() should call back setFocus(null)
 		// here, so both focus part values should stay in sync.
 		viewer.setFocus(null);
 		for (EditPart part : selection) {
-			if (!hashset.contains(part)) {
+			if (!set.contains(part)) {
 				part.setSelected(EditPart.SELECTED_NONE);
 			}
 		}