CollectionBinder.managerForModel does not use internal keys? #131

Closed
katowulf opened this Issue Apr 25, 2013 · 4 comments

Comments

Projects
None yet
2 participants
Collaborator

katowulf commented Apr 25, 2013

Hello again, apparently it's CollectionBinder today over here at my dev box : ) (actually, it's bug fix day, so there's lots of libs getting explored that I don't normally delve too deeply into)

I'm trying to utilize CollectionBinder to retrieve a view it has created, so that I can interact with it. In doing so, I ran across this interesting tidbit:

      getManagerForModel: function(model){
         var i, elManager, elManagers = _.values(this._elManagers);

         for(i = 0; i < elManagers.length; i++){
            elManager = elManagers[i];

            if(elManager.getModel() === model){
               return elManager;
            }
         }

         return undefined;
      },

      _onCollectionAdd: function(model){
         this._elManagers[model.cid] = this._elManagerFactory.makeElManager(model);
         this._elManagers[model.cid].createEl();

         if(this._options['autoSort']){
            this.sortRootEls();
         }
      },

Note that we have all the elManagers stored by model.cid, but then getManagerForModel does not utilize it to retrieve them, instead iterating the entire list.

Is this intentional? Have I missed something obvious?

Owner

theironcook commented Apr 25, 2013

@katowulf

oops :)

Yeah, it appears like the getManagerForModel could be rewrittwen to be more efficient. Good catch.
I don't have a computer for another 10 days :( I am in between jobs. You are welcome to make the change / do a pull request.

Collaborator

katowulf commented Apr 25, 2013

Okay, once you've looked at the attr/prop changes, and approved/denied that, I'll put in a fix for this. Otherwise, we'll end up with a host of commits under that one and it should really stand on its own because of the number of files touched.

Owner

theironcook commented Apr 28, 2013

@katowulf
Hi,
I just merged your previous pull request - thank you for that change.

Collaborator

katowulf commented Apr 28, 2013

I tested this fix in Safari, Firefox, IE 9, and Chrome (latest builds all), against all the CollectionBinder example pages and also ran the specs in all browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment