Bug 348069 Closing an additional window of an Eclipse 4 applicationv20120210-2010I20120210-1700

doesn't unrender it

When a window was being closed, only its widgets were destroyed
and its context structure was not. This leads to memory leaks.

If a top-level window gets closed, its entire structure and
context will be destroyed and the window itself will be removed
from the application's list of child windows if it is not the
last window in the list. If it is the last window, then the
window's structure and contexts will be destroyed but it will
not be removed from the application's list of child windows for
restart purposes.

For detached windows, detached windows will have always have
their structures and contexts destroyed but it will only remove
itself from its parent list of child windows if the detached
window being destroyed does not contain any parts.

4 files changed, 363 insertions, 12 deletions

diff --git a/bundles/org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt/WBWRenderer.java b/bundles/org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt/WBWRenderer.java
index faf7bb04f80..c46bef1155f 100644
--- a/bundles/org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt/WBWRenderer.java
+++ b/bundles/org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt/WBWRenderer.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2008, 2011 IBM Corporation and others.
+ * Copyright (c) 2008, 2012 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
@@ -30,6 +30,7 @@ import org.eclipse.e4.ui.model.application.MApplication;
 import org.eclipse.e4.ui.model.application.ui.MContext;
 import org.eclipse.e4.ui.model.application.ui.MElementContainer;
 import org.eclipse.e4.ui.model.application.ui.MUIElement;
+import org.eclipse.e4.ui.model.application.ui.advanced.MPerspective;
 import org.eclipse.e4.ui.model.application.ui.advanced.MPlaceholder;
 import org.eclipse.e4.ui.model.application.ui.basic.MPart;
 import org.eclipse.e4.ui.model.application.ui.basic.MPartStack;
@@ -110,6 +111,9 @@ public class WBWRenderer extends SWTPartRenderer {
 	@Inject
 	private IEventBroker eventBroker;
 
+	@Inject
+	private IPresentationEngine engine;
+
 	private EventHandler shellUpdater;
 	private EventHandler visibilityHandler;
 	private EventHandler sizeHandler;
@@ -170,7 +174,15 @@ public class WBWRenderer extends SWTPartRenderer {
 			setCSSInfo(stack, stack.getWidget());
 	}
 
-	private void closeDetachedWindow(MWindow window) {
+	/**
+	 * Closes the provided detached window.
+	 * 
+	 * @param window
+	 *            the detached window to close
+	 * @return <code>true</code> if the window should be closed,
+	 *         <code>false</code> otherwise
+	 */
+	private boolean closeDetachedWindow(MWindow window) {
 		EPartService partService = (EPartService) window.getContext().get(
 				EPartService.class.getName());
 		List<MPart> parts = modelService.findElements(window, null,
@@ -179,7 +191,8 @@ public class WBWRenderer extends SWTPartRenderer {
 		// at all
 		for (MPart part : parts) {
 			if (!partService.savePart(part, true)) {
-				return;
+				// user cancelled the operation, return false
+				return false;
 			}
 		}
 
@@ -187,9 +200,7 @@ public class WBWRenderer extends SWTPartRenderer {
 		for (MPart part : parts) {
 			partService.hidePart(part);
 		}
-
-		// finally unrender the window itself
-		window.setToBeRendered(false);
+		return true;
 	}
 
 	@PostConstruct
@@ -483,8 +494,7 @@ public class WBWRenderer extends SWTPartRenderer {
 			context.set(IWindowCloseHandler.class.getName(),
 					new IWindowCloseHandler() {
 						public boolean close(MWindow window) {
-							closeDetachedWindow(window);
-							return false;
+							return closeDetachedWindow(window);
 						}
 					});
 		} else {
@@ -541,12 +551,16 @@ public class WBWRenderer extends SWTPartRenderer {
 
 			shell.addShellListener(new ShellAdapter() {
 				public void shellClosed(ShellEvent e) {
+					// override the shell close event
+					e.doit = false;
 					MWindow window = (MWindow) e.widget.getData(OWNING_ME);
 					IWindowCloseHandler closeHandler = (IWindowCloseHandler) window
 							.getContext().get(
 									IWindowCloseHandler.class.getName());
-					if (closeHandler != null) {
-						e.doit = closeHandler.close(window);
+					// if there's no handler or the handler permits the close
+					// request, clean-up as necessary
+					if (closeHandler == null || closeHandler.close(window)) {
+						cleanUp(window);
 					}
 				}
 			});
@@ -568,6 +582,34 @@ public class WBWRenderer extends SWTPartRenderer {
 		}
 	}
 
+	private void cleanUp(MWindow window) {
+		Object parent = ((EObject) window).eContainer();
+		if (parent instanceof MApplication) {
+			MApplication application = (MApplication) parent;
+			List<MWindow> children = application.getChildren();
+			if (children.size() > 1) {
+				// not the last window, destroy and remove
+				window.setToBeRendered(false);
+				children.remove(window);
+			} else {
+				// last window, just destroy without changing the model
+				engine.removeGui(window);
+			}
+		} else if (parent != null) {
+			window.setToBeRendered(false);
+			// this is a detached window, check for parts
+			if (modelService.findElements(window, null, MPart.class, null)
+					.isEmpty()) {
+				// if no parts, remove it
+				if (parent instanceof MWindow) {
+					((MWindow) parent).getWindows().remove(window);
+				} else if (parent instanceof MPerspective) {
+					((MPerspective) parent).getWindows().remove(window);
+				}
+			}
+		}
+	}
+
 	/*
 	 * Processing the contents of a Workbench window has to take into account
 	 * that there may be trim elements contained in its child list. Since the
diff --git a/bundles/org.eclipse.e4.ui.workbench.swt/src/org/eclipse/e4/ui/internal/workbench/swt/PartRenderingEngine.java b/bundles/org.eclipse.e4.ui.workbench.swt/src/org/eclipse/e4/ui/internal/workbench/swt/PartRenderingEngine.java
index 42c4bd2dc4c..98417fbf4d7 100644
--- a/bundles/org.eclipse.e4.ui.workbench.swt/src/org/eclipse/e4/ui/internal/workbench/swt/PartRenderingEngine.java
+++ b/bundles/org.eclipse.e4.ui.workbench.swt/src/org/eclipse/e4/ui/internal/workbench/swt/PartRenderingEngine.java
@@ -1048,7 +1048,7 @@ public class PartRenderingEngine implements IPresentationEngine {
 		if (theApp != null) {
 			for (MWindow window : theApp.getChildren()) {
 				if (window.getWidget() != null) {
-					((Shell) window.getWidget()).close();
+					removeGui(window);
 				}
 			}
 		} else if (testShell != null && !testShell.isDisposed()) {
diff --git a/examples/org.eclipse.e4.demo.contacts/src/org/eclipse/e4/demo/contacts/views/DetailsView.java b/examples/org.eclipse.e4.demo.contacts/src/org/eclipse/e4/demo/contacts/views/DetailsView.java
index 1a7417d6e16..7394a2fe504 100644
--- a/examples/org.eclipse.e4.demo.contacts/src/org/eclipse/e4/demo/contacts/views/DetailsView.java
+++ b/examples/org.eclipse.e4.demo.contacts/src/org/eclipse/e4/demo/contacts/views/DetailsView.java
@@ -224,6 +224,8 @@ public class DetailsView {
 			uiItem.setLabel("Details");
 		}
 		dirtyable.setDirty(false);
-		detailComposite.update(contact);
+		if (!detailComposite.isDisposed()) {
+			detailComposite.update(contact);
+		}
 	}
 }
diff --git a/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java b/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java
index 28a3bb5493d..3a0b33ad101 100644
--- a/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java
+++ b/tests/org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/workbench/PartRenderingEngineTests.java
@@ -2885,6 +2885,313 @@ public class PartRenderingEngineTests extends TestCase {
 		assertNotNull(partB.getWidget());
 	}
 
+	public void testBug348069_01() {
+		MApplication application = ApplicationFactoryImpl.eINSTANCE
+				.createApplication();
+
+		MWindow window = BasicFactoryImpl.eINSTANCE.createWindow();
+		application.getChildren().add(window);
+		application.setSelectedElement(window);
+
+		MPart part = BasicFactoryImpl.eINSTANCE.createPart();
+		part.setContributionURI("platform:/plugin/org.eclipse.e4.ui.tests/org.eclipse.e4.ui.tests.workbench.SampleView");
+		window.getChildren().add(part);
+		window.setSelectedElement(part);
+
+		application.setContext(appContext);
+		appContext.set(MApplication.class.getName(), application);
+
+		wb = new E4Workbench(application, appContext);
+		wb.createAndRunUI(window);
+
+		SampleView view = (SampleView) part.getObject();
+		assertFalse(view.isDestroyed());
+
+		((Shell) window.getWidget()).close();
+		assertTrue(view.isDestroyed());
+		assertTrue(application.getChildren().contains(window));
+	}
+
+	public void testBug348069_02() {
+		MApplication application = ApplicationFactoryImpl.eINSTANCE
+				.createApplication();
+
+		MWindow windowA = BasicFactoryImpl.eINSTANCE.createWindow();
+		application.getChildren().add(windowA);
+		application.setSelectedElement(windowA);
+
+		MPart partA = BasicFactoryImpl.eINSTANCE.createPart();
+		partA.setContributionURI("platform:/plugin/org.eclipse.e4.ui.tests/org.eclipse.e4.ui.tests.workbench.SampleView");
+		windowA.getChildren().add(partA);
+		windowA.setSelectedElement(partA);
+
+		MWindow windowB = BasicFactoryImpl.eINSTANCE.createWindow();
+		application.getChildren().add(windowB);
+
+		MPart partB = BasicFactoryImpl.eINSTANCE.createPart();
+		partB.setContributionURI("platform:/plugin/org.eclipse.e4.ui.tests/org.eclipse.e4.ui.tests.workbench.SampleView");
+		windowB.getChildren().add(partB);
+		windowB.setSelectedElement(partB);
+
+		application.setContext(appContext);
+		appContext.set(MApplication.class.getName(), application);
+
+		wb = new E4Workbench(application, appContext);
+		wb.createAndRunUI(windowA);
+		wb.createAndRunUI(windowB);
+
+		SampleView viewA = (SampleView) partA.getObject();
+		assertFalse(viewA.isDestroyed());
+		SampleView viewB = (SampleView) partB.getObject();
+		assertFalse(viewB.isDestroyed());
+
+		((Shell) windowB.getWidget()).close();
+		assertTrue(viewB.isDestroyed());
+		assertFalse(windowB.isToBeRendered());
+		assertFalse(application.getChildren().contains(windowB));
+
+		((Shell) windowA.getWidget()).close();
+		assertTrue(viewA.isDestroyed());
+		assertTrue(windowA.isToBeRendered());
+		assertTrue(application.getChildren().contains(windowA));
+	}
+
+	public void testBug348069_DetachedWindow_01() {
+		MApplication application = ApplicationFactoryImpl.eINSTANCE
+				.createApplication();
+
+		MWindow window = BasicFactoryImpl.eINSTANCE.createWindow();
+		application.getChildren().add(window);
+		application.setSelectedElement(window);
+
+		MWindow detachedWindow = BasicFactoryImpl.eINSTANCE.createWindow();
+		window.getWindows().add(detachedWindow);
+
+		MPart part = BasicFactoryImpl.eINSTANCE.createPart();
+		part.setContributionURI("platform:/plugin/org.eclipse.e4.ui.tests/org.eclipse.e4.ui.tests.workbench.SampleView");
+		detachedWindow.getChildren().add(part);
+		detachedWindow.setSelectedElement(part);
+
+		application.setContext(appContext);
+		appContext.set(MApplication.class.getName(), application);
+
+		wb = new E4Workbench(application, appContext);
+		wb.createAndRunUI(window);
+
+		SampleView view = (SampleView) part.getObject();
+		assertFalse(view.isDestroyed());
+
+		((Shell) detachedWindow.getWidget()).close();
+		assertTrue(view.isDestroyed());
+		assertTrue(window.getWindows().contains(detachedWindow));
+	}
+
+	public void testBug348069_DetachedWindow_02() {
+		MApplication application = ApplicationFactoryImpl.eINSTANCE
+				.createApplication();
+
+		MWindow window = BasicFactoryImpl.eINSTANCE.createWindow();
+		application.getChildren().add(window);
+		application.setSelectedElement(window);
+
+		MWindow detachedWindow = BasicFactoryImpl.eINSTANCE.createWindow();
+		window.getWindows().add(detachedWindow);
+
+		MPart part = BasicFactoryImpl.eINSTANCE.createPart();
+		part.setContributionURI("platform:/plugin/org.eclipse.e4.ui.tests/org.eclipse.e4.ui.tests.workbench.SampleView");
+		detachedWindow.getChildren().add(part);
+		detachedWindow.setSelectedElement(part);
+
+		application.setContext(appContext);
+		appContext.set(MApplication.class.getName(), application);
+
+		wb = new E4Workbench(application, appContext);
+		wb.createAndRunUI(window);
+
+		SampleView view = (SampleView) part.getObject();
+		assertFalse(view.isDestroyed());
+
+		detachedWindow.setSelectedElement(null);
+		detachedWindow.getChildren().remove(part);
+		window.getChildren().add(part);
+
+		((Shell) detachedWindow.getWidget()).close();
+		assertFalse(view.isDestroyed());
+		assertFalse(window.getWindows().contains(detachedWindow));
+	}
+
+	public void testBug348069_DetachedWindow_03() {
+		MApplication application = ApplicationFactoryImpl.eINSTANCE
+				.createApplication();
+
+		MWindow window = BasicFactoryImpl.eINSTANCE.createWindow();
+		application.getChildren().add(window);
+		application.setSelectedElement(window);
+
+		MWindow detachedWindow = BasicFactoryImpl.eINSTANCE.createWindow();
+		window.getWindows().add(detachedWindow);
+
+		MPart part = BasicFactoryImpl.eINSTANCE.createPart();
+		part.setContributionURI("platform:/plugin/org.eclipse.e4.ui.tests/org.eclipse.e4.ui.tests.workbench.SampleView");
+		detachedWindow.getChildren().add(part);
+		detachedWindow.setSelectedElement(part);
+
+		application.setContext(appContext);
+		appContext.set(MApplication.class.getName(), application);
+
+		wb = new E4Workbench(application, appContext);
+		wb.createAndRunUI(window);
+
+		SampleView view = (SampleView) part.getObject();
+		assertFalse(view.isDestroyed());
+
+		((Shell) detachedWindow.getWidget()).close();
+		assertTrue(view.isDestroyed());
+		assertTrue(window.getWindows().contains(detachedWindow));
+	}
+
+	private void testBug348069_DetachedPerspectiveWindow_01(
+			boolean createPlaceholder) {
+		MApplication application = ApplicationFactoryImpl.eINSTANCE
+				.createApplication();
+
+		MWindow window = BasicFactoryImpl.eINSTANCE.createWindow();
+		application.getChildren().add(window);
+		application.setSelectedElement(window);
+
+		MPerspectiveStack perspectiveStack = AdvancedFactoryImpl.eINSTANCE
+				.createPerspectiveStack();
+		window.getChildren().add(perspectiveStack);
+		window.setSelectedElement(perspectiveStack);
+
+		MPerspective perspective = AdvancedFactoryImpl.eINSTANCE
+				.createPerspective();
+		perspectiveStack.getChildren().add(perspective);
+		perspectiveStack.setSelectedElement(perspective);
+
+		MWindow detachedWindow = BasicFactoryImpl.eINSTANCE.createWindow();
+		perspective.getWindows().add(detachedWindow);
+
+		MPartStack partStack = BasicFactoryImpl.eINSTANCE.createPartStack();
+		detachedWindow.getChildren().add(partStack);
+		detachedWindow.setSelectedElement(partStack);
+
+		MPart part = BasicFactoryImpl.eINSTANCE.createPart();
+		part.setContributionURI("platform:/plugin/org.eclipse.e4.ui.tests/org.eclipse.e4.ui.tests.workbench.SampleView");
+
+		if (createPlaceholder) {
+			window.getSharedElements().add(part);
+
+			MPlaceholder placeholder = AdvancedFactoryImpl.eINSTANCE
+					.createPlaceholder();
+			placeholder.setRef(part);
+			part.setCurSharedRef(placeholder);
+
+			partStack.getChildren().add(placeholder);
+			partStack.setSelectedElement(placeholder);
+		} else {
+			partStack.getChildren().add(part);
+			partStack.setSelectedElement(part);
+		}
+
+		application.setContext(appContext);
+		appContext.set(MApplication.class.getName(), application);
+
+		wb = new E4Workbench(application, appContext);
+		wb.createAndRunUI(window);
+
+		SampleView view = (SampleView) part.getObject();
+		assertFalse(view.isDestroyed());
+
+		((Shell) detachedWindow.getWidget()).close();
+		assertTrue(view.isDestroyed());
+		assertTrue(perspective.getWindows().contains(detachedWindow));
+	}
+
+	public void testBug348069_DetachedPerspectiveWindow_01_TRUE() {
+		testBug348069_DetachedPerspectiveWindow_01(true);
+	}
+
+	public void testBug348069_DetachedPerspectiveWindow_01_FALSE() {
+		testBug348069_DetachedPerspectiveWindow_01(false);
+	}
+
+	private void testBug348069_DetachedPerspectiveWindow_02(
+			boolean createPlaceholder) {
+		MApplication application = ApplicationFactoryImpl.eINSTANCE
+				.createApplication();
+
+		MWindow window = BasicFactoryImpl.eINSTANCE.createWindow();
+		application.getChildren().add(window);
+		application.setSelectedElement(window);
+
+		MPerspectiveStack perspectiveStack = AdvancedFactoryImpl.eINSTANCE
+				.createPerspectiveStack();
+		window.getChildren().add(perspectiveStack);
+		window.setSelectedElement(perspectiveStack);
+
+		MPerspective perspective = AdvancedFactoryImpl.eINSTANCE
+				.createPerspective();
+		perspectiveStack.getChildren().add(perspective);
+		perspectiveStack.setSelectedElement(perspective);
+
+		MWindow detachedWindow = BasicFactoryImpl.eINSTANCE.createWindow();
+		perspective.getWindows().add(detachedWindow);
+
+		MPartStack partStack = BasicFactoryImpl.eINSTANCE.createPartStack();
+		detachedWindow.getChildren().add(partStack);
+		detachedWindow.setSelectedElement(partStack);
+
+		MPlaceholder placeholder = null;
+		MPart part = BasicFactoryImpl.eINSTANCE.createPart();
+		part.setContributionURI("platform:/plugin/org.eclipse.e4.ui.tests/org.eclipse.e4.ui.tests.workbench.SampleView");
+
+		if (createPlaceholder) {
+			window.getSharedElements().add(part);
+
+			placeholder = AdvancedFactoryImpl.eINSTANCE.createPlaceholder();
+			placeholder.setRef(part);
+			part.setCurSharedRef(placeholder);
+
+			partStack.getChildren().add(placeholder);
+			partStack.setSelectedElement(placeholder);
+		} else {
+			partStack.getChildren().add(part);
+			partStack.setSelectedElement(part);
+		}
+
+		application.setContext(appContext);
+		appContext.set(MApplication.class.getName(), application);
+
+		wb = new E4Workbench(application, appContext);
+		wb.createAndRunUI(window);
+
+		SampleView view = (SampleView) part.getObject();
+		assertFalse(view.isDestroyed());
+
+		if (createPlaceholder) {
+			partStack.setSelectedElement(null);
+			partStack.getChildren().remove(placeholder);
+			perspective.getChildren().add(placeholder);
+		} else {
+			partStack.setSelectedElement(null);
+			partStack.getChildren().remove(part);
+			perspective.getChildren().add(part);
+		}
+
+		((Shell) detachedWindow.getWidget()).close();
+		assertFalse(view.isDestroyed());
+		assertFalse(perspective.getWindows().contains(detachedWindow));
+	}
+
+	public void testBug348069_DetachedPerspectiveWindow_02_TRUE() {
+		testBug348069_DetachedPerspectiveWindow_02(true);
+	}
+
+	public void testBug348069_DetachedPerspectiveWindow_02_FALSE() {
+		testBug348069_DetachedPerspectiveWindow_02(false);
+	}
+
 	private MWindow createWindowWithOneView(String partName) {
 		final MWindow window = BasicFactoryImpl.eINSTANCE.createWindow();
 		window.setHeight(300);