From b786bcc7746136c270fddb2c04ab47704231b9af Mon Sep 17 00:00:00 2001
From: Kevin Sawicki <kevin@github.com>
Date: Tue, 2 Jul 2013 09:41:46 -0700
Subject: [PATCH] Store views that don't implement setModel() by their item

Previously viewForItem() would create a new view each time it was called
with an item whose view did not implement setModel() even if a view for that
item already existed in the pane.

Now a WeakMap is used to map items to their view so they can be reused
and cleaned up even when the view does not implement setModel().
---
 spec/app/pane-spec.coffee |  9 +++++++++
 src/app/pane.coffee       | 25 ++++++++++++++++++-------
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/spec/app/pane-spec.coffee b/spec/app/pane-spec.coffee
index 56ccc1b22..c8ee47b0b 100644
--- a/spec/app/pane-spec.coffee
+++ b/spec/app/pane-spec.coffee
@@ -123,6 +123,15 @@ describe "Pane", ->
             pane.showItem(model2)
             expect(pane.itemViews.find('.test-view').length).toBe initialViewCount + 2
 
+            pane.showPreviousItem()
+            expect(pane.itemViews.find('.test-view').length).toBe initialViewCount + 2
+
+            pane.removeItem(model2)
+            expect(pane.itemViews.find('.test-view').length).toBe initialViewCount + 1
+
+            pane.removeItem(model1)
+            expect(pane.itemViews.find('.test-view').length).toBe initialViewCount
+
     describe "when showing a view item", ->
       it "appends it to the itemViews div if it hasn't already been appended and shows it", ->
         expect(pane.itemViews.find('#view-2')).not.toExist()
diff --git a/src/app/pane.coffee b/src/app/pane.coffee
index 75f968918..2d6fb6e83 100644
--- a/src/app/pane.coffee
+++ b/src/app/pane.coffee
@@ -22,6 +22,8 @@ class Pane extends View
 
   activeItem: null
   items: null
+  viewsByClassName: null
+  viewsByItem: null
 
   initialize: (args...) ->
     if args[0] instanceof telepath.Document
@@ -45,6 +47,7 @@ class Pane extends View
       @showItemForUri(newValue) if key is 'activeItemUri'
 
     @viewsByClassName = {}
+    @viewsByItem = new WeakMap()
     if activeItemUri = @state.get('activeItemUri')
       @showItemForUri(activeItemUri)
     else
@@ -269,12 +272,15 @@ class Pane extends View
     if item instanceof $
       viewToRemove = item
     else
-      viewClass = item.getViewClass()
-      otherItemsForView = @items.filter (i) -> i.getViewClass?() is viewClass
-      unless otherItemsForView.length
-        viewToRemove = @viewsByClassName[viewClass.name]
-        viewToRemove?.setModel(null)
-        delete @viewsByClassName[viewClass.name]
+      if viewToRemove = @viewsByItem.get(item)
+        @viewsByItem.delete(item)
+      else
+        viewClass = item.getViewClass()
+        otherItemsForView = @items.filter (i) -> i.getViewClass?() is viewClass
+        unless otherItemsForView.length
+          viewToRemove = @viewsByClassName[viewClass.name]
+          viewToRemove?.setModel(null)
+          delete @viewsByClassName[viewClass.name]
 
     if @items.length > 0
       if @isMovingItem and item is viewToRemove
@@ -288,13 +294,18 @@ class Pane extends View
   viewForItem: (item) ->
     if item instanceof $
       item
+    else if view = @viewsByItem.get(item)
+      view
     else
       viewClass = item.getViewClass()
       if view = @viewsByClassName[viewClass.name]
         view.setModel(item)
       else
         view = new viewClass(item)
-        @viewsByClassName[viewClass.name] = view if _.isFunction(view.setModel)
+        if _.isFunction(view.setModel)
+          @viewsByClassName[viewClass.name] = view
+        else
+          @viewsByItem.set(item, view)
       view
 
   viewForActiveItem: ->