From cc1ca167a4393a08afafab8ab4cca4194cbee56f Mon Sep 17 00:00:00 2001
From: Alex Wilson <yourpalal2@gmail.com>
Date: Fri, 9 Sep 2011 14:34:23 -0600
Subject: [PATCH] Fix a memory leak I introduced with my modification of
 BLayout::RemoveItem() and add comments so that it doesn't happen again. Also
 resolve some TODOs on the appropriate timing of calls to hook methods in
 BLayoutItem/BLayout.

---
 src/kits/interface/Layout.cpp     | 22 +++++++++++-----------
 src/kits/interface/LayoutItem.cpp | 13 ++++++-------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/src/kits/interface/Layout.cpp b/src/kits/interface/Layout.cpp
index cfb7ca377f..afd0ce703f 100644
--- a/src/kits/interface/Layout.cpp
+++ b/src/kits/interface/Layout.cpp
@@ -226,19 +226,17 @@ BLayout::RemoveItem(int32 index)
 		return NULL;
 
 	BLayoutItem* item = (BLayoutItem*)fItems.RemoveItem(index);
-	// If this is the last item in use that refers to a certain BView,
-	// that BView now needs to be removed.
-	BView* view = item->View();
-	if (view && BView::Private(view).CountLayoutItems() == 0)
-		view->_RemoveSelf();
-
-	// TODO: Is this the right place/order to call these hooks?
-	// view->Parent() could be NULL, maybe that's not a problem..
-	
 	ItemRemoved(item, index);
 	item->SetLayout(NULL);
-	InvalidateLayout();
 
+	// If this is the last item in use that refers to its BView,
+	// that BView now needs to be removed. UNLESS fTarget is NULL,
+	// in which case we leave the view as is. (See SetTarget() for more info)
+	BView* view = item->View();
+	if (fTarget && view && BView::Private(view).CountLayoutItems() == 0)
+		view->_RemoveSelf();
+
+	InvalidateLayout();
 	return item;
 }
 
@@ -624,8 +622,10 @@ void
 BLayout::SetTarget(BView* target)
 {
 	if (fTarget != target) {
+		/* With fTarget NULL, RemoveItem() will not remove the views from their
+		 * parent. This ensures that the views are not lost to the void.
+		 */
 		fTarget = NULL;
-			// only remove items, not views
 
 		// remove and delete all items
 		for (int32 i = CountItems() - 1; i >= 0; i--)
diff --git a/src/kits/interface/LayoutItem.cpp b/src/kits/interface/LayoutItem.cpp
index 43bb42ee39..210ad54110 100644
--- a/src/kits/interface/LayoutItem.cpp
+++ b/src/kits/interface/LayoutItem.cpp
@@ -164,17 +164,16 @@ BLayoutItem::SetLayout(BLayout* layout)
 	if (layout == fLayout)
 		return;
 
-	std::swap(fLayout, layout);
-	if (layout)
+	BLayout* oldLayout = fLayout;
+	fLayout = layout;
+
+	if (oldLayout)
 		DetachedFromLayout(layout);
 
-	// TODO: is this the right place to do this?
-	// at this point, this->Layout() will return not exactly truthful
-	// values... this could cause problems in DetachedFromLayout();
 	if (BView* view = View()) {
-		if (layout && !fLayout) {
+		if (oldLayout && !fLayout) {
 			BView::Private(view).DeregisterLayoutItem(this);
-		} else if (fLayout && !layout) {
+		} else if (fLayout && !oldLayout) {
 			BView::Private(view).RegisterLayoutItem(this);
 		}
 	}