From 13121143a2074a297d805733579d72a74bd4dc51 Mon Sep 17 00:00:00 2001 From: Paul Falgout Date: Sat, 20 Aug 2016 03:06:25 +0900 Subject: [PATCH 01/14] Allow childViews to be already rendered and attached If you were to override `buildChildView` instantiating the childViews with an `el` then you can create a collectionView from pre-rendered DOM --- src/collection-view.js | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/collection-view.js b/src/collection-view.js index 0dc4c73048..3bd6f6e066 100644 --- a/src/collection-view.js +++ b/src/collection-view.js @@ -489,10 +489,6 @@ const CollectionView = Backbone.View.extend({ // Internal Method. Add the view to children and render it at the given index. _addChildView(view, index) { - // Only trigger attach if already attached and not buffering, - // otherwise _endBuffering() or Region#show() handles this. - const shouldTriggerAttach = !this._isBuffering && this._isAttached; - monitorViewEvents(view); // set up the child view event forwarding @@ -501,23 +497,37 @@ const CollectionView = Backbone.View.extend({ // Store the child view itself so we can properly remove and/or destroy it later this.children.add(view); + this._renderView(view); + + this._attachView(view, index); + }, + + _renderView(view) { + if (view._isRendered) { + return; + } + if (!view.supportsRenderLifecycle) { triggerMethodOn(view, 'before:render', view); } - // Render view view.render(); if (!view.supportsRenderLifecycle) { view._isRendered = true; triggerMethodOn(view, 'render', view); } + }, + + _attachView(view, index) { + // Only trigger attach if already attached and not buffering, + // otherwise _endBuffering() or Region#show() handles this. + const shouldTriggerAttach = !view._isAttached && !this._isBuffering && this._isAttached; if (shouldTriggerAttach) { triggerMethodOn(view, 'before:attach', view); } - // Attach view this.attachHtml(this, view, index); if (shouldTriggerAttach) { From fe6aa199cc89ce653411c60558f42fdbd13965b5 Mon Sep 17 00:00:00 2001 From: Paul Falgout Date: Sat, 20 Aug 2016 04:20:13 +0900 Subject: [PATCH 02/14] Move setElement to the views from the mixin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CollectionView can’t be initially “rendered” because it does its initialization of many things on first render. --- src/collection-view.js | 16 ++++++++++++++++ src/mixins/view.js | 17 ----------------- src/view.js | 17 +++++++++++++++++ 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/collection-view.js b/src/collection-view.js index 3bd6f6e066..7039c4f7b9 100644 --- a/src/collection-view.js +++ b/src/collection-view.js @@ -4,6 +4,7 @@ import _ from 'underscore'; import Backbone from 'backbone'; import destroyBackboneView from './utils/destroy-backbone-view'; +import isNodeAttached from './common/is-node-attached'; import monitorViewEvents from './common/monitor-view-events'; import { triggerMethodOn } from './common/trigger-method'; import ChildViewContainer from './child-view-container'; @@ -132,6 +133,21 @@ const CollectionView = Backbone.View.extend({ this._checkEmpty(); }, + // Overriding Backbone.View's `setElement` to handle + // if an el was previously defined. If so, the view might be + // attached on setElement. + setElement() { + const hasEl = !!this.el; + + Backbone.View.prototype.setElement.apply(this, arguments); + + if (hasEl) { + this._isAttached = isNodeAttached(this.el); + } + + return this; + }, + // Render children views. Override this method to provide your own implementation of a // render function for the collection view. render() { diff --git a/src/mixins/view.js b/src/mixins/view.js index 2a848004ce..b287de44b3 100644 --- a/src/mixins/view.js +++ b/src/mixins/view.js @@ -3,7 +3,6 @@ import Backbone from 'backbone'; import _ from 'underscore'; -import isNodeAttached from '../common/is-node-attached'; import { triggerMethod } from '../common/trigger-method'; import BehaviorsMixin from './behaviors'; import CommonMixin from './common'; @@ -46,22 +45,6 @@ const ViewMixin = { return !!this._isAttached; }, - // Overriding Backbone.View's `setElement` to handle - // if an el was previously defined. If so, the view might be - // rendered or attached on setElement. - setElement() { - const hasEl = !!this.el; - - Backbone.View.prototype.setElement.apply(this, arguments); - - if (hasEl) { - this._isRendered = !!this.$el.length; - this._isAttached = isNodeAttached(this.el); - } - - return this; - }, - // Overriding Backbone.View's `delegateEvents` to handle // `events` and `triggers` delegateEvents(eventsArg) { diff --git a/src/view.js b/src/view.js index 2d0781195a..c0ff5d4ca5 100644 --- a/src/view.js +++ b/src/view.js @@ -3,6 +3,7 @@ import _ from 'underscore'; import Backbone from 'backbone'; +import isNodeAttached from './common/is-node-attached'; import monitorViewEvents from './common/monitor-view-events'; import ViewMixin from './mixins/view'; import RegionsMixin from './mixins/regions'; @@ -82,6 +83,22 @@ const View = Backbone.View.extend({ return this.collection.map(function(model) { return _.clone(model.attributes); }); }, + // Overriding Backbone.View's `setElement` to handle + // if an el was previously defined. If so, the view might be + // rendered or attached on setElement. + setElement() { + const hasEl = !!this.el; + + Backbone.View.prototype.setElement.apply(this, arguments); + + if (hasEl) { + this._isRendered = !!this.$el.length; + this._isAttached = isNodeAttached(this.el); + } + + return this; + }, + // Render the view, defaulting to underscore.js templates. // You can override this in your view definition to provide // a very specific rendering for your view. In general, though, From 5d74ec1ecb4b9665dc310a224d90dc5c9339bf76 Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Sat, 20 Aug 2016 20:48:08 -0700 Subject: [PATCH 03/14] [test] CollectionView when a collection view is DOM * and it's not attached to the document ** should have _isAttached set to false * and it's attached to the document ** should have _isAttached set to true when rendering a childView * should not render childView twice --- test/unit/collection-view.spec.js | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/unit/collection-view.spec.js b/test/unit/collection-view.spec.js index 8bd10ece45..3fcbdc1d0f 100644 --- a/test/unit/collection-view.spec.js +++ b/test/unit/collection-view.spec.js @@ -28,6 +28,34 @@ describe('collection view', function() { // Collection View Specs // --------------------- + describe('when a collection view is DOM', function() { + beforeEach(function() { + this.$fixtureEl = $('
'); + }); + + describe('and it\'s not attached to the document', function() { + beforeEach(function() { + this.collectionView = new this.CollectionView({el: '#fixture-collectionview'}); + }); + + it('should have _isAttached set to false', function() { + expect(this.collectionView).to.have.property('_isAttached', false); + }); + }); + + describe('and it\'s attached to the document', function() { + beforeEach(function() { + this.setFixtures(this.$fixtureEl); + this.collectionView = new this.CollectionView({el: '#fixture-collectionview'}); + }); + + it('should have _isAttached set to true', function() { + expect(this.collectionView).to.have.property('_isAttached', true); + }); + }); + + }); + describe('before rendering a collection view', function() { beforeEach(function() { var CollectionView = this.CollectionView.extend({ @@ -338,6 +366,12 @@ describe('collection view', function() { it('should call "render" on the childView', function() { expect(this.childView.render).to.have.been.calledOnce; }); + + it('should not render childView twice', function() { + this.collectionView._renderView(this.childView); + + expect(this.childView.render).to.not.have.been.calledTwice; + }); }); describe('when sorting a collection', function() { From 5034c325da50864be196efb5a8aec6b3789af02f Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Tue, 23 Aug 2016 20:37:52 -0700 Subject: [PATCH 04/14] [test] CollectionView when a collection view is DOM * and it's not attached to the document ** should have a child view without `_isAttached` * and it's attached to the document ** should have a child view with `_isAttached` set to `true` --- test/unit/collection-view.spec.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/unit/collection-view.spec.js b/test/unit/collection-view.spec.js index 3fcbdc1d0f..f1fe181933 100644 --- a/test/unit/collection-view.spec.js +++ b/test/unit/collection-view.spec.js @@ -30,28 +30,38 @@ describe('collection view', function() { describe('when a collection view is DOM', function() { beforeEach(function() { - this.$fixtureEl = $('
'); + this.$fixtureEl = $('
1
'); }); describe('and it\'s not attached to the document', function() { beforeEach(function() { - this.collectionView = new this.CollectionView({el: '#fixture-collectionview'}); + this.collectionView = new this.CollectionView({el: this.$fixtureEl[0]}); + this.view1 = this.collectionView.addChildView(new Backbone.View({el: this.$fixtureEl.children()[0]}), 0); }); it('should have _isAttached set to false', function() { expect(this.collectionView).to.have.property('_isAttached', false); }); + + it('should have a child view without _isAttached', function() { + expect(this.view1).to.not.have.property('_isAttached'); + }); }); describe('and it\'s attached to the document', function() { beforeEach(function() { this.setFixtures(this.$fixtureEl); this.collectionView = new this.CollectionView({el: '#fixture-collectionview'}); + this.view1 = this.collectionView.addChildView(new Backbone.View({el: '#el1'}), 0); }); it('should have _isAttached set to true', function() { expect(this.collectionView).to.have.property('_isAttached', true); }); + + it('should have a child view with _isAttached set to true', function() { + expect(this.view1).to.have.property('_isAttached', true); + }); }); }); From ba4c9d77c3c3dbaeec7c34507018d67c7083995e Mon Sep 17 00:00:00 2001 From: Paul Falgout Date: Thu, 1 Sep 2016 01:50:01 +0900 Subject: [PATCH 05/14] Make a public detachView This function removes the need for a `preventDestroy` option. Essentially if you want to `preventDestroy` just detach the view first. When you want to preventDestroy you already have to have a copy of the view.. and depending on what is doing the view emptying `preventDestroy` can become a round about API. What you really want is ```js const myView = myRegion.currentView; myRegion.detachView(); myOtherRegion.show(myView); ``` --- src/region.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/region.js b/src/region.js index 42c0652e49..5cd9ef5e98 100644 --- a/src/region.js +++ b/src/region.js @@ -245,6 +245,17 @@ const Region = MarionetteObject.extend({ } }, + detachView() { + const view = this.currentView; + + delete this.currentView; + + this._detachView(view); + delete view._parent; + + return this; + }, + _detachView(view) { const shouldTriggerDetach = !!view._isAttached; if (shouldTriggerDetach) { From 61d20b69b9f6c86bdadaf84a5709e9f162b72f48 Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Thu, 1 Sep 2016 01:50:53 -0700 Subject: [PATCH 06/14] Optimize triggerMethod, mergeOptions and hot handles (#3144) * mergeOptions src/common/merge-options.js * iterate through `keys` and merge instead of getting the `keys` from options, then merging monitor-view-events src/common/monitor-view-events.js * Define once: triggerDOMRefresh handleBeforeAttach handleAttach handleBeforeDetach handleDetach handleRender triggerMethodOn src/common/trigger-method.js * Distinct if `context` has triggerMethod, use and return result with fallback to helper function and return result CollectionView src/collection-view.js * private childViewEventHandle for optimization * _proxyChildEvents using childViewEventHandle src/common/trigger-method.js * will remember the last `event` name transform src/mixins/behaviors.js takes arraylike value only src/mixins/view.js triggerMethod only passes along `arguments` other functions * Make trigger on behavior follow same patterns as parent * Make childViewEventHandler a common module --- src/collection-view.js | 27 +------------ src/common/child-view-event-handler.js | 27 +++++++++++++ src/common/merge-options.js | 8 +++- src/common/monitor-view-events.js | 52 +++++++++++++------------- src/common/trigger-method.js | 15 ++++++-- src/mixins/behaviors.js | 4 +- src/mixins/view.js | 32 ++++------------ 7 files changed, 82 insertions(+), 83 deletions(-) create mode 100644 src/common/child-view-event-handler.js diff --git a/src/collection-view.js b/src/collection-view.js index 7039c4f7b9..7a3a3a41ce 100644 --- a/src/collection-view.js +++ b/src/collection-view.js @@ -5,6 +5,7 @@ import _ from 'underscore'; import Backbone from 'backbone'; import destroyBackboneView from './utils/destroy-backbone-view'; import isNodeAttached from './common/is-node-attached'; +import childViewEventHandler from './common/child-view-event-handler'; import monitorViewEvents from './common/monitor-view-events'; import { triggerMethodOn } from './common/trigger-method'; import ChildViewContainer from './child-view-container'; @@ -697,31 +698,7 @@ const CollectionView = Backbone.View.extend({ // Set up the child view event forwarding. Uses a "childview:" prefix in front of all forwarded events. _proxyChildEvents(view) { - const prefix = _.result(this, 'childViewEventPrefix'); - - // Forward all child view events through the parent, - // prepending "childview:" to the event name - this.listenTo(view, 'all', (eventName, ...args) => { - - const childEventName = prefix + ':' + eventName; - - const childViewEvents = this.normalizeMethods(this._childViewEvents); - - // call collectionView childViewEvent if defined - if (typeof childViewEvents !== 'undefined' && _.isFunction(childViewEvents[eventName])) { - childViewEvents[eventName].apply(this, args); - } - - // use the parent view's proxyEvent handlers - const childViewTriggers = this._childViewTriggers; - - // Call the event with the proxy name on the parent layout - if (childViewTriggers && _.isString(childViewTriggers[eventName])) { - this.triggerMethod(childViewTriggers[eventName], ...args); - } - - this.triggerMethod(childEventName, ...args); - }); + this.listenTo(view, 'all', childViewEventHandler); } }); diff --git a/src/common/child-view-event-handler.js b/src/common/child-view-event-handler.js new file mode 100644 index 0000000000..d77a14230b --- /dev/null +++ b/src/common/child-view-event-handler.js @@ -0,0 +1,27 @@ +import _ from 'underscore'; + +// Called with the context of the parent view +const childViewEventHandler = function(eventName, ...args) { + const prefix = _.result(this, 'childViewEventPrefix'); + + const childEventName = prefix + ':' + eventName; + + const childViewEvents = this.normalizeMethods(this._childViewEvents); + + // call collectionView childViewEvent if defined + if (typeof childViewEvents !== 'undefined' && _.isFunction(childViewEvents[eventName])) { + childViewEvents[eventName].apply(this, args); + } + + // use the parent view's proxyEvent handlers + const childViewTriggers = this._childViewTriggers; + + // Call the event with the proxy name on the parent layout + if (childViewTriggers && _.isString(childViewTriggers[eventName])) { + this.triggerMethod(childViewTriggers[eventName], ...args); + } + + this.triggerMethod(childEventName, ...args); +}; + +export default childViewEventHandler; diff --git a/src/common/merge-options.js b/src/common/merge-options.js index abf35217bf..554e6eae88 100644 --- a/src/common/merge-options.js +++ b/src/common/merge-options.js @@ -3,7 +3,13 @@ import _ from 'underscore'; // Merge `keys` from `options` onto `this` const mergeOptions = function(options, keys) { if (!options) { return; } - _.extend(this, _.pick(options, keys)); + + _.each(keys, (key) => { + const option = options[key]; + if (option !== undefined) { + this[key] = option; + } + }); }; export default mergeOptions; diff --git a/src/common/monitor-view-events.js b/src/common/monitor-view-events.js index 893a6717e6..3812d8f2fa 100644 --- a/src/common/monitor-view-events.js +++ b/src/common/monitor-view-events.js @@ -33,39 +33,39 @@ function shouldDetach(view) { return true; } -// Monitor a view's state, propagating attach/detach events to children and firing dom:refresh -// whenever a rendered view is attached or an attached view is rendered. -function monitorViewEvents(view) { - if (view._areViewEventsMonitored) { return; } +function triggerDOMRefresh(view) { + if (view._isAttached && view._isRendered) { + triggerMethodOn(view, 'dom:refresh', view); + } +} - view._areViewEventsMonitored = true; +function handleBeforeAttach() { + triggerMethodChildren(this, 'before:attach', shouldTriggerAttach); +} - function handleBeforeAttach() { - triggerMethodChildren(view, 'before:attach', shouldTriggerAttach); - } +function handleAttach() { + triggerMethodChildren(this, 'attach', shouldAttach); + triggerDOMRefresh(this); +} - function handleAttach() { - triggerMethodChildren(view, 'attach', shouldAttach); - triggerDOMRefresh(); - } +function handleBeforeDetach() { + triggerMethodChildren(this, 'before:detach', shouldTriggerDetach); +} - function handleBeforeDetach() { - triggerMethodChildren(view, 'before:detach', shouldTriggerDetach); - } +function handleDetach() { + triggerMethodChildren(this, 'detach', shouldDetach); +} - function handleDetach() { - triggerMethodChildren(view, 'detach', shouldDetach); - } +function handleRender() { + triggerDOMRefresh(this); +} - function handleRender() { - triggerDOMRefresh(); - } +// Monitor a view's state, propagating attach/detach events to children and firing dom:refresh +// whenever a rendered view is attached or an attached view is rendered. +function monitorViewEvents(view) { + if (view._areViewEventsMonitored) { return; } - function triggerDOMRefresh() { - if (view._isAttached && view._isRendered) { - triggerMethodOn(view, 'dom:refresh', view); - } - } + view._areViewEventsMonitored = true; view.on({ 'before:attach': handleBeforeAttach, diff --git a/src/common/trigger-method.js b/src/common/trigger-method.js index 07a71fe1b9..5b5ed8280c 100644 --- a/src/common/trigger-method.js +++ b/src/common/trigger-method.js @@ -13,6 +13,10 @@ function getEventName(match, prefix, eventName) { return eventName.toUpperCase(); } +const getOnMethodName = _.memoize(function(event) { + return 'on' + event.replace(splitter, getEventName); +}); + // Trigger an event and/or a corresponding method name. Examples: // // `this.triggerMethod("foo")` will trigger the "foo" event and @@ -22,7 +26,7 @@ function getEventName(match, prefix, eventName) { // call the "onFooBar" method. export function triggerMethod(event, ...args) { // get the method name from the event name - const methodName = 'on' + event.replace(splitter, getEventName); + const methodName = getOnMethodName(event); const method = getOption.call(this, methodName); let result; @@ -33,7 +37,7 @@ export function triggerMethod(event, ...args) { } // trigger the event - this.trigger(event, ...args); + this.trigger.apply(this, arguments); return result; } @@ -43,6 +47,9 @@ export function triggerMethod(event, ...args) { // e.g. `Marionette.triggerMethodOn(view, 'show')` // will trigger a "show" event or invoke onShow the view. export function triggerMethodOn(context, ...args) { - const fnc = _.isFunction(context.triggerMethod) ? context.triggerMethod : triggerMethod; - return fnc.apply(context, args); + if (_.isFunction(context.triggerMethod)) { + return context.triggerMethod.apply(context, args); + } + + return triggerMethod.apply(context, args); } diff --git a/src/mixins/behaviors.js b/src/mixins/behaviors.js index 0f70512d55..306a0d0108 100644 --- a/src/mixins/behaviors.js +++ b/src/mixins/behaviors.js @@ -93,11 +93,11 @@ export default { _invoke(this._behaviors, 'unbindUIElements'); }, - _triggerEventOnBehaviors(...args) { + _triggerEventOnBehaviors() { const behaviors = this._behaviors; // Use good ol' for as this is a very hot function for (let i = 0, length = behaviors && behaviors.length; i < length; i++) { - triggerMethod.apply(behaviors[i], args); + triggerMethod.apply(behaviors[i], arguments); } } }; diff --git a/src/mixins/view.js b/src/mixins/view.js index b287de44b3..75bdd46a11 100644 --- a/src/mixins/view.js +++ b/src/mixins/view.js @@ -3,6 +3,7 @@ import Backbone from 'backbone'; import _ from 'underscore'; +import childViewEventHandler from '../common/child-view-event-handler'; import { triggerMethod } from '../common/trigger-method'; import BehaviorsMixin from './behaviors'; import CommonMixin from './common'; @@ -185,11 +186,11 @@ const ViewMixin = { // import the `triggerMethod` to trigger events with corresponding // methods if the method exists - triggerMethod(...args) { - const ret = triggerMethod.apply(this, args); + triggerMethod() { + const ret = triggerMethod.apply(this, arguments); - this._triggerEventOnBehaviors(...args); - this._triggerEventOnParentLayout(...args); + this._triggerEventOnBehaviors.apply(this, arguments); + this._triggerEventOnParentLayout.apply(this, arguments); return ret; }, @@ -200,32 +201,13 @@ const ViewMixin = { this._childViewTriggers = _.result(this, 'childViewTriggers'); }, - _triggerEventOnParentLayout(eventName, ...args) { + _triggerEventOnParentLayout() { const layoutView = this._parentView(); if (!layoutView) { return; } - // invoke triggerMethod on parent view - const eventPrefix = _.result(layoutView, 'childViewEventPrefix'); - const prefixedEventName = eventPrefix + ':' + eventName; - - layoutView.triggerMethod(prefixedEventName, ...args); - - // use the parent view's childViewEvents handler - const childViewEvents = layoutView.normalizeMethods(layoutView._childViewEvents); - - if (!!childViewEvents && _.isFunction(childViewEvents[eventName])) { - childViewEvents[eventName].apply(layoutView, args); - } - - // use the parent view's proxyEvent handlers - const childViewTriggers = layoutView._childViewTriggers; - - // Call the event with the proxy name on the parent layout - if (childViewTriggers && _.isString(childViewTriggers[eventName])) { - layoutView.triggerMethod(childViewTriggers[eventName], ...args); - } + childViewEventHandler.apply(layoutView, arguments); }, // Walk the _parent tree until we find a view (if one exists). From ecd894b157da33875cf7d9f7e0ef198f92cce23c Mon Sep 17 00:00:00 2001 From: Carl Ogren Date: Fri, 2 Sep 2016 07:03:26 +0200 Subject: [PATCH 07/14] Fix behavior to be compatible with lodash v4 (#3159) --- src/behavior.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/behavior.js b/src/behavior.js index 39a7c9a1cb..3bf958df50 100644 --- a/src/behavior.js +++ b/src/behavior.js @@ -105,7 +105,7 @@ const Behavior = MarionetteObject.extend({ const behaviorEvents = this.normalizeUIKeys(_.result(this, 'events')); // binds the handler to the behavior and builds a unique eventName - return _.reduce(behaviorEvents, function(events, behaviorHandler, key) { + return _.reduce(behaviorEvents, (events, behaviorHandler, key) => { if (!_.isFunction(behaviorHandler)) { behaviorHandler = this[behaviorHandler]; } @@ -113,7 +113,7 @@ const Behavior = MarionetteObject.extend({ key = getUniqueEventName(key); events[key] = _.bind(behaviorHandler, this); return events; - } , {}, this); + }, {}); }, // Internal method to build all trigger handlers for a given behavior From 4396132c31866e690066330de20e9822470f5a0e Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Fri, 2 Sep 2016 00:48:35 -0700 Subject: [PATCH 08/14] CollectionView and CompositeView removing views performance update. (#3105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * CollectionView# and CompositeView# _initialEvents * Listen to `this.collection~event:update` instead. _onCollectionUpdate * pass `options.changed.removed` to `this._removeChildViews` * updated test to reflect additions _removeChildViews * use array of models to determine what views to remove if not already destroyed. * Support `checkEmpty` for preventing empty collection checking _destroyChildren * checks to see if there are any children to destroy before calling `_removeChildViews` removeChildView * using _destroyChildView * added test for trying to remove falsy and valid values _getRemovedViews _removeChildView - ~~~_onCollectionRemove~~~ * test and references removed _findGreatestIndexedView * finds the view with the greatest `_index` and returns it. _updateIndices * accepts array of views for _findGreatestIndexedView [test] when removing the sorted view, the `view._index` should update. * Attempts to always use models instead of modelOrViews This may not be helpful.. just wanted to give it one last show. I don’t think there are perf issues. particularly since `findByModel` is cheap. In fact I think the complexity of reorder went down.. though I didn’t test it.. --- src/collection-view.js | 159 ++++++++++++++++------- src/composite-view.js | 2 +- test/unit/collection-view.filter.spec.js | 9 +- test/unit/collection-view.spec.js | 97 ++++++++++---- test/unit/sorted-views.spec.js | 10 ++ 5 files changed, 203 insertions(+), 74 deletions(-) diff --git a/src/collection-view.js b/src/collection-view.js index 7a3a3a41ce..6222db4b95 100644 --- a/src/collection-view.js +++ b/src/collection-view.js @@ -101,7 +101,7 @@ const CollectionView = Backbone.View.extend({ _initialEvents() { if (this.collection) { this.listenTo(this.collection, 'add', this._onCollectionAdd); - this.listenTo(this.collection, 'remove', this._onCollectionRemove); + this.listenTo(this.collection, 'update', this._onCollectionUpdate); this.listenTo(this.collection, 'reset', this.render); if (this.sort) { @@ -127,11 +127,83 @@ const CollectionView = Backbone.View.extend({ } }, - // get the child view by model it holds, and remove it - _onCollectionRemove(model) { - const view = this.children.findByModel(model); - this.removeChildView(view); - this._checkEmpty(); + // Handle collection update model removals + _onCollectionUpdate(collection, options) { + const changes = options.changes; + this._removeChildModels(changes.removed); + }, + + // Remove the child views and destroy them. + // This function also updates the indices of later views + // in the collection in order to keep the children in sync with the collection. + // "models" is an array of models and the corresponding views + // will be removed and destroyed from the CollectionView + _removeChildModels(models, {checkEmpty} = {}) { + const shouldCheckEmpty = checkEmpty !== false; + + // Used to determine where to update the remaining + // sibling view indices after these views are removed. + const removedViews = this._getRemovedViews(models); + + if (!removedViews.length) { + return; + } + + this.children._updateLength(); + + // decrement the index of views after this one + this._updateIndices(removedViews, false); + + if (shouldCheckEmpty) { + this._checkEmpty(); + } + }, + + // Returns the views that will be used for re-indexing + // through CollectionView#_updateIndices. + _getRemovedViews(models) { + + // Returning a view means something was removed. + return _.reduce(models, (removingViews, model) => { + const view = this.children.findByModel(model); + + if (!view || view._isDestroyed) { + return removingViews; + } + + this._removeChildView(view); + + removingViews.push(view); + + return removingViews; + }, []); + }, + + _findGreatestIndexedView(views) { + + return _.reduce(views, (greatestIndexedView, view) => { + // Even if the index is `undefined`, a view will get returned. + if (!greatestIndexedView || greatestIndexedView._index < view._index) { + return view; + } + + return greatestIndexedView; + }, undefined); + }, + + _removeChildView(view) { + this.triggerMethod('before:remove:child', this, view); + + this.children._remove(view); + if (view.destroy) { + view.destroy(); + } else { + destroyBackboneView(view); + } + + delete view._parent; + this.stopListening(view); + this.triggerMethod('remove:child', this, view); }, // Overriding Backbone.View's `setElement` to handle @@ -194,12 +266,12 @@ const CollectionView = Backbone.View.extend({ } currentIds[model.cid] = true; }); - _.each(previousModels, (prevModel) => { - const removedChildExists = !currentIds[prevModel.cid] && this.children.findByModel(prevModel); - if (removedChildExists) { - this._onCollectionRemove(prevModel); - } + + const removeModels = _.filter(previousModels, (prevModel) => { + return !currentIds[prevModel.cid] && this.children.findByModel(prevModel); }); + + this._removeChildModels(removeModels); }, // Reorder DOM after sorting. When your element's rendering do not use their index, @@ -220,17 +292,25 @@ const CollectionView = Backbone.View.extend({ if (anyModelsAdded) { this.render(); } else { - // Get the DOM nodes in the same order as the models. - const elsToReorder = _.map(models, function(model, index) { - const view = children.findByModel(model); + + const filteredOutModels = []; + + // Get the DOM nodes in the same order as the models and + // find the model that were children before but aren't in this new order. + const elsToReorder = children.reduce(function(viewEls, view) { + const index = _.indexOf(models, view.model); + + if (index === -1) { + filteredOutModels.push(view.model); + return viewEls; + } + view._index = index; - return view.el; - }); - // Find the views that were children before but aren't in this new ordering. - const filteredOutViews = children.filter(function(view) { - return !_.contains(elsToReorder, view.el); - }); + viewEls[index] = view.el; + + return viewEls; + }, new Array(models.length)); this.triggerMethod('before:reorder', this); @@ -239,8 +319,7 @@ const CollectionView = Backbone.View.extend({ this._appendReorderedChildren(elsToReorder); // remove any views that have been filtered out - _.each(filteredOutViews, _.bind(this.removeChildView, this)); - this._checkEmpty(); + this._removeChildModels(filteredOutModels); this.triggerMethod('reorder', this); } @@ -486,11 +565,13 @@ const CollectionView = Backbone.View.extend({ // Internal method. This decrements or increments the indices of views after the added/removed // view to keep in sync with the collection. - _updateIndices(view, increment, index) { + _updateIndices(views, increment, index) { if (!this.sort) { return; } + const view = _.isArray(views) ? this._findGreatestIndexedView(views) : views; + if (increment) { // assign the index to the view view._index = index; @@ -566,22 +647,10 @@ const CollectionView = Backbone.View.extend({ return view; } - this.triggerMethod('before:remove:child', this, view); - - if (view.destroy) { - view.destroy(); - } else { - destroyBackboneView(view); - } - - delete view._parent; - this.stopListening(view); - this.children.remove(view); - this.triggerMethod('remove:child', this, view); - + this._removeChildView(view); + this.children._updateLength(); // decrement the index of views after this one this._updateIndices(view, false); - return view; }, @@ -671,19 +740,15 @@ const CollectionView = Backbone.View.extend({ }, // Destroy the child views that this collection view is holding on to, if any - _destroyChildren({checkEmpty} = {}) { - this.triggerMethod('before:destroy:children', this); - const shouldCheckEmpty = checkEmpty !== false; - const childViews = this.children.map(_.identity); - - this.children.each(_.bind(this.removeChildView, this)); - - if (shouldCheckEmpty) { - this._checkEmpty(); + _destroyChildren(options) { + if (!this.children.length) { + return; } + this.triggerMethod('before:destroy:children', this); + const childModels = this.children.map('model'); + this._removeChildModels(childModels, options); this.triggerMethod('destroy:children', this); - return childViews; }, // Return true if the given child should be shown. Return false otherwise. diff --git a/src/composite-view.js b/src/composite-view.js index 6c4563c024..62c2b9fbbb 100644 --- a/src/composite-view.js +++ b/src/composite-view.js @@ -41,7 +41,7 @@ const CompositeView = CollectionView.extend({ if (this.collection) { this.listenTo(this.collection, 'add', this._onCollectionAdd); - this.listenTo(this.collection, 'remove', this._onCollectionRemove); + this.listenTo(this.collection, 'update', this._onCollectionUpdate); this.listenTo(this.collection, 'reset', this.renderChildren); if (this.sort) { diff --git a/test/unit/collection-view.filter.spec.js b/test/unit/collection-view.filter.spec.js index 6fa4f05192..280db30980 100644 --- a/test/unit/collection-view.filter.spec.js +++ b/test/unit/collection-view.filter.spec.js @@ -71,7 +71,7 @@ describe('collection view - filter', function() { this.collection.add(this.passModel); this.collection.add(this.failModel); this.collectionView = new this.CollectionView(); - this.sinon.spy(this.collectionView, 'removeChildView'); + this.sinon.spy(this.collectionView, '_removeChildModels'); this.collectionView.render(); }); @@ -118,14 +118,15 @@ describe('collection view - filter', function() { describe('when all models passing the filter are removed from the collection', function() { beforeEach(function() { - this.passView = this.collectionView.children.first(); + this.removedViews = this.collectionView.children.first(); + this.removedModels = [this.passModel]; this.collection.remove(this.passModel); }); it('should remove the child view', function() { - expect(this.collectionView.removeChildView).to.have.been.calledOnce + expect(this.collectionView._removeChildModels).to.have.been.calledOnce .and.calledOn(this.collectionView) - .and.calledWith(this.passView); + .and.calledWith(this.removedModels); }); it('should show the EmptyView', function() { diff --git a/test/unit/collection-view.spec.js b/test/unit/collection-view.spec.js index f1fe181933..84cfa681d1 100644 --- a/test/unit/collection-view.spec.js +++ b/test/unit/collection-view.spec.js @@ -659,11 +659,12 @@ describe('collection view', function() { this.beforeRenderSpy = this.sinon.spy(this.collectionView, 'onBeforeRenderEmpty'); this.renderSpy = this.sinon.spy(this.collectionView, 'onRenderEmpty'); + this._removeChildModelsSpy = this.sinon.spy(this.collectionView, '_removeChildModels'); this.sinon.spy(this.childView, 'destroy'); this.sinon.spy(this.EmptyView.prototype, 'render'); - this.collectionView._onCollectionRemove(this.model); + this.collectionView._removeChildModels([this.model]); }); it('should destroy the models view', function() { @@ -681,6 +682,11 @@ describe('collection view', function() { it('should call "onRenderEmpty"', function() { expect(this.renderSpy).to.have.been.called; }); + + it('should call "_removeChildModels"', function() { + expect(this._removeChildModelsSpy).to.have.been.called + .and.to.have.been.calledWith([this.model]); + }); }); describe('when a model is removed from the collection', function() { @@ -772,7 +778,6 @@ describe('collection view', function() { this.collectionView.listenTo(this.collectionView, 'item:foo', this.collectionView.someViewCallback); this.sinon.spy(this.childView, 'destroy'); - this.sinon.spy(this.collectionView, '_onCollectionRemove'); this.sinon.spy(this.collectionView, 'stopListening'); this.sinon.spy(this.collectionView, '_removeElement'); this.sinon.spy(this.collectionView, 'someCallback'); @@ -946,26 +951,28 @@ describe('collection view', function() { expect(this.childView1.remove).to.have.been.called; }); - it('should return the child views', function() { - expect(this.collectionView._destroyChildren).to.have.returned(this.childrenViews); - }); - it('should call checkEmpty', function() { expect(this.collectionView._checkEmpty).to.have.been.calledOnce; }); describe('with the checkEmpty flag set as false', function() { - it('should not call checkEmpty', function() { + it('should not call _checkEmpty', function() { this.collectionView._destroyChildren({checkEmpty: false}); expect(this.collectionView._checkEmpty).to.have.been.calledOnce; }); }); describe('with the checkEmpty flag set as true', function() { - it('should call checkEmpty', function() { + it('should call _checkEmpty', function() { + this.collectionView.collection.add({id: 3}); this.collectionView._destroyChildren({checkEmpty: true}); expect(this.collectionView._checkEmpty).to.have.been.calledTwice; }); + + it('should not call _checkEmpty when collection is empty', function() { + this.collectionView._destroyChildren({checkEmpty: true}); + expect(this.collectionView._checkEmpty).to.have.been.calledOnce; + }); }); }); @@ -1116,27 +1123,73 @@ describe('collection view', function() { }); this.collectionView.render(); - - this.childView = this.collectionView.children[this.model.cid]; - this.collection.remove(this.model); }); - it('should not retain any bindings to this view', function() { - var suite = this; - var bindings = this.collectionView.bindings || {}; - expect(_.some(bindings, function(binding) { - return binding.obj === suite.childView; - })).to.be.false; - }); + describe('by model', function() { + beforeEach(function() { + this.collection.remove(this.model); + this.childView = this.collectionView.children[this.model.cid]; + }); - it('should not retain any references to this view', function() { - expect(_.size(this.collectionView.children)).to.equal(0); + it('should not retain any bindings to this view', function() { + var suite = this; + var bindings = this.collectionView.bindings || {}; + expect(_.some(bindings, function(binding) { + return binding.obj === suite.childView; + })).to.be.false; + }); + + it('should not retain any references to this view', function() { + expect(_.size(this.collectionView.children)).to.equal(0); + }); + + it('childView should be undefined', function() { + expect(this.childView).to.be.undefined; + }); }); - it('childView should be undefined', function() { - expect(this.childView).to.be.undefined; + describe('by view', function() { + beforeEach(function() { + this.childView = this.collectionView.children.findByModel(this.model); + this.removedChildView = this.collectionView.removeChildView(this.childView); + this.afterRemoveChildView = this.collectionView.children.findByModel(this.model); + }); + + it('should not retain any bindings to this view', function() { + var suite = this; + var bindings = this.collectionView.bindings || {}; + expect(_.some(bindings, function(binding) { + return binding.obj === suite.childView; + })).to.be.false; + }); + + it('should not retain any references to this view', function() { + expect(this.collectionView.children.length).to.equal(0); + }); + + it('childView should be undefined', function() { + expect(this.afterRemoveChildView).to.be.undefined; + }); + + it('should be removed and returned', function() { + expect(this.childView).to.equal(this.removedChildView); + }); }); + describe('and trying to remove a view that isn\'t a view', function() { + beforeEach(function() { + this.sinon.spy(this.collectionView, '_removeChildView'); + this.removedChildView = this.collectionView.removeChildView(null); + }); + + it('should return null', function() { + expect(this.removedChildView).is.null; + }); + + it('should not call internal function', function() { + expect(this.collectionView._removeChildView).is.not.called; + }); + }); }); describe('when the collection of a collection view is reset', function() { diff --git a/test/unit/sorted-views.spec.js b/test/unit/sorted-views.spec.js index 606b9e4164..3d025e6aa8 100644 --- a/test/unit/sorted-views.spec.js +++ b/test/unit/sorted-views.spec.js @@ -133,6 +133,11 @@ describe('collection/composite view sorting', function() { expect(this.compositeView.$el).to.have.$text('13'); }); + it('should have children view with correct _index', function() { + expect(this.collectionView.children.findByModel(this.collection.at(0))).to.have.property('_index', 0); + expect(this.collectionView.children.findByModel(this.collection.at(1))).to.have.property('_index', 1); + }); + describe('and then adding another', function() { beforeEach(function() { this.model = new Backbone.Model({foo: '4'}); @@ -273,6 +278,11 @@ describe('collection/composite view sorting', function() { expect(this.compositeView.$el).to.have.$text('31'); }); + it('should have children view with correct _index', function() { + expect(this.collectionView.children.findByModel(this.collection.at(0))).to.have.property('_index', 1); + expect(this.collectionView.children.findByModel(this.collection.at(1))).to.have.property('_index', 0); + }); + describe('and then adding another', function() { beforeEach(function() { this.model = new Backbone.Model({foo: '4', bar: '1'}); From 57eab1f931ee2c9626c07bb2d16c5acaa472eff9 Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Sat, 3 Sep 2016 11:10:55 -0700 Subject: [PATCH 09/14] regions mixin detachView * falsy check Regions detachChildView --- src/mixins/regions.js | 4 ++++ src/region.js | 8 +++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/mixins/regions.js b/src/mixins/regions.js index 5342e06d46..e000234362 100644 --- a/src/mixins/regions.js +++ b/src/mixins/regions.js @@ -180,6 +180,10 @@ export default { return region.show(view, ...args); }, + detachChildView(name) { + return this.getRegion(name).detachView(); + }, + getChildView(name) { return this.getRegion(name).currentView; } diff --git a/src/region.js b/src/region.js index 5cd9ef5e98..45e1b847ec 100644 --- a/src/region.js +++ b/src/region.js @@ -248,12 +248,14 @@ const Region = MarionetteObject.extend({ detachView() { const view = this.currentView; - delete this.currentView; + if (!view) { + return; + } + delete this.currentView; this._detachView(view); delete view._parent; - - return this; + return view; }, _detachView(view) { From 59124d31136d0af937d14521f47408e04e87bd07 Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Sun, 4 Sep 2016 09:33:09 -0700 Subject: [PATCH 10/14] [test] View detachChildView --- test/unit/view.child-views.spec.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/unit/view.child-views.spec.js b/test/unit/view.child-views.spec.js index 88ff7beaf5..e7bdf68aa2 100644 --- a/test/unit/view.child-views.spec.js +++ b/test/unit/view.child-views.spec.js @@ -315,6 +315,21 @@ describe('layoutView', function() { it('childViewEvents are triggered', function() { expect(this.childEventsHandler).to.have.been.calledOnce; }); + + describe('and the view is detached', function() { + beforeEach(function() { + this.detachedView = this.layoutView.detachChildView('regionOne'); + this.noDetachedView = this.layoutView.detachChildView('regionOne'); + }); + + it('should return the childView it was given', function() { + expect(this.detachedView).to.equal(this.childView); + }); + + it('should not return a childView if it was already detached', function() { + expect(this.noDetachedView).to.be.undefined; + }); + }); }); describe('when showing a layoutView via a region', function() { From 84ebc1d07e15a5ab854d16a03eb356484d2392a8 Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Sun, 4 Sep 2016 11:32:24 -0700 Subject: [PATCH 11/14] [test] Region detachView --- test/unit/region.spec.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/unit/region.spec.js b/test/unit/region.spec.js index 8898ff9083..b8cb519aec 100644 --- a/test/unit/region.spec.js +++ b/test/unit/region.spec.js @@ -449,6 +449,20 @@ describe('region', function() { }); }); + describe('and the view is detached', function() { + beforeEach(function() { + this.detachedView = this.region.detachView(); + this.noDetachedView = this.region.detachView(); + }); + + it('should return the childView it was given', function() { + expect(this.detachedView).to.equal(this.view); + }); + + it('should not return a childView if it was already detached', function() { + expect(this.noDetachedView).to.be.undefined; + }); + }); }); describe('when showing nested views', function() { From 53a1611cfb8bba1c05e3d710a5e9df1e82e9faef Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Sun, 4 Sep 2016 16:15:48 -0700 Subject: [PATCH 12/14] [documentation] Region#detachView [documentation] View#detachChildView --- docs/marionette.region.md | 18 ++++++++++++++++++ docs/marionette.view.md | 29 +++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) diff --git a/docs/marionette.region.md b/docs/marionette.region.md index 5f9389d645..e803888eda 100644 --- a/docs/marionette.region.md +++ b/docs/marionette.region.md @@ -18,6 +18,7 @@ managing regions throughout your application. * [Showing a View](#showing-a-view) * [Hiding a View](#hiding-a-view) * [Preserving Existing Views](#preserving-existing-views) + * [Detaching Existing Views](#detaching-existing-views) * [Checking whether a region is showing a view](#checking-whether-a-region-is-showing-a-view) * [`reset` A Region](#reset-a-region) @@ -219,6 +220,23 @@ mainRegion.empty({preventDestroy: true}); **NOTE** When using `preventDestroy: true` you must be careful to cleanup your old views manually to prevent memory leaks. +### Detaching Existing Views +If you want to detach an existing view from a region, use `detachView`. + +```javascript +var myView = new MyView(); + +var myOtherView = new MyView(); + +var childView = new MyChildView(); + +// render and display the view +myView.showChildView('main', childView); + +// ... somewhere down the line +myOtherView.showChildView('main', myView.getRegion('main').detachView()); +``` + ### Checking whether a region is showing a view If you wish to check whether a region has a view, you can use the `hasView` diff --git a/docs/marionette.view.md b/docs/marionette.view.md index 27815add19..e3628989a8 100644 --- a/docs/marionette.view.md +++ b/docs/marionette.view.md @@ -20,6 +20,7 @@ multiple views through the `regions` attribute. * [Managing Sub-views](#managing-sub-views) * [Showing a view](#showing-a-view) * [Accessing a child view](#accessing-a-child-view) + * [Detaching a child view](#detaching-a-child-view) * [Organizing your View](#organizing-your-view) * [Events](#events) * [onEvent Listeners](#onevent-listeners) @@ -198,6 +199,34 @@ var MyView = Mn.View.extend({ If no view is available, `getChildView` returns `null`. +#### Detaching a child view +You can detach a child view from a region through `detachChildView(region)` + +```javascript + +var Mn = require('backbone.marionette'); +var SubView = require('./subview'); + +var MyView = Mn.View.extend({ + template: '#tpl-view-with-regions', + + regions: { + firstRegion: '#first-region', + secondRegion: '#second-region' + }, + + onRender: function() { + this.showChildView('firstRegion', new SubView()); + }, + + onMoveView: function() { + var view = this.detachChildView('firstRegion'); + this.showChildView('secondRegion', view); + } +}); +``` +This is a proxy for [region.detachView()](./marionette.region.md#detaching-existing-views) + ### Region availability Any defined regions within a `View` will be available to the `View` or any calling code immediately after instantiating the `View`. This allows a View to From cffeb86bcfd090280d8c0ae1929a8779d17b9930 Mon Sep 17 00:00:00 2001 From: Rafael De Leon Date: Wed, 7 Sep 2016 00:24:16 -0700 Subject: [PATCH 13/14] CollectionView and CompositeView rendering views performance increase (#3113) * _createView * creates view from model and does event monitor and proxyChild events _renderChildView * render views and triggers render events _setupChildView * sets child view properties: parent, _index (conditionally), monitorViewEvents, and _proxyChildEvents. * CollectionView addChildView * using `_setupChildView` * triggers `event~before:add:child` * triggers `event~add:child` * when buffering, will add view to list of children but will not update children length for performance increase * using `_renderView` ~~_addChildView~~ _updateIndices * removed need to pass index _addChild * removed passing ChildView param * using `_createView` _onCollectionAdd * using `_addChild` _showCollection * using `_addChild` * calls `children._updateLength` _showEmptyView * removed unnecessary `view._parent` setting --- src/collection-view.js | 78 +++++++++++++++---------------- test/unit/collection-view.spec.js | 26 ++++++----- 2 files changed, 53 insertions(+), 51 deletions(-) diff --git a/src/collection-view.js b/src/collection-view.js index 6222db4b95..8e3eb6faf7 100644 --- a/src/collection-view.js +++ b/src/collection-view.js @@ -122,8 +122,7 @@ const CollectionView = Backbone.View.extend({ if (this._shouldAddChild(child, index)) { this._destroyEmptyView(); - const ChildView = this._getChildView(child); - this._addChild(child, ChildView, index); + this._addChild(child, index) } }, @@ -382,12 +381,30 @@ const CollectionView = Backbone.View.extend({ } }, + _createView(model, index) { + const ChildView = this._getChildView(model); + const childViewOptions = this._getChildViewOptions(model, index); + const view = this.buildChildView(model, ChildView, childViewOptions); + return view; + }, + + _setupChildView(view, index) { + view._parent = this; + + monitorViewEvents(view); + + // set up the child view event forwarding + this._proxyChildEvents(view); + + if (this.sort) { + view._index = index; + } + }, + // Internal method to loop through collection and show each child view. _showCollection(models) { - _.each(models, (child, index) => { - const ChildView = this._getChildView(child); - this._addChild(child, ChildView, index); - }); + _.each(models, _.bind(this._addChild, this)); + this.children._updateLength(); }, // Allow the collection to be sorted by a custom view comparator @@ -461,10 +478,8 @@ const CollectionView = Backbone.View.extend({ const view = this.buildChildView(model, EmptyView, emptyViewOptions); this.triggerMethod('before:render:empty', this, view); - this._addChildView(view, 0); + this.addChildView(view, 0); this.triggerMethod('render:empty', this, view); - - view._parent = this; } }, @@ -527,11 +542,8 @@ const CollectionView = Backbone.View.extend({ }, // Internal method for building and adding a child view - _addChild(child, ChildView, index) { - const childViewOptions = this._getChildViewOptions(child, index); - - const view = this.buildChildView(child, ChildView, childViewOptions); - + _addChild(child, index) { + const view = this._createView(child, index); this.addChildView(view, index); return view; @@ -550,13 +562,21 @@ const CollectionView = Backbone.View.extend({ // children in sync with the collection. addChildView(view, index) { this.triggerMethod('before:add:child', this, view); + this._setupChildView(view, index); - // increment indices of views after this one - this._updateIndices(view, true, index); + // Store the child view itself so we can properly remove and/or destroy it later + if (this._isBuffering) { + // Add to children, but don't update children's length. + this.children._add(view); + } else { + // increment indices of views after this one + this._updateIndices(view, true); + this.children.add(view); + } - view._parent = this; + this._renderView(view); - this._addChildView(view, index); + this._attachView(view, index); this.triggerMethod('add:child', this, view); @@ -565,18 +585,13 @@ const CollectionView = Backbone.View.extend({ // Internal method. This decrements or increments the indices of views after the added/removed // view to keep in sync with the collection. - _updateIndices(views, increment, index) { + _updateIndices(views, increment) { if (!this.sort) { return; } const view = _.isArray(views) ? this._findGreatestIndexedView(views) : views; - if (increment) { - // assign the index to the view - view._index = index; - } - // update the indexes of views after this one this.children.each((laterView) => { if (laterView._index >= view._index) { @@ -585,21 +600,6 @@ const CollectionView = Backbone.View.extend({ }); }, - // Internal Method. Add the view to children and render it at the given index. - _addChildView(view, index) { - monitorViewEvents(view); - - // set up the child view event forwarding - this._proxyChildEvents(view); - - // Store the child view itself so we can properly remove and/or destroy it later - this.children.add(view); - - this._renderView(view); - - this._attachView(view, index); - }, - _renderView(view) { if (view._isRendered) { return; diff --git a/test/unit/collection-view.spec.js b/test/unit/collection-view.spec.js index 84cfa681d1..458eeca6cf 100644 --- a/test/unit/collection-view.spec.js +++ b/test/unit/collection-view.spec.js @@ -149,7 +149,7 @@ describe('collection view', function() { this.sinon.spy(this.collectionView.$el, 'append'); this.sinon.spy(this.collectionView, '_startBuffering'); this.sinon.spy(this.collectionView, '_endBuffering'); - this.sinon.spy(this.collectionView, '_addChild'); + this.sinon.spy(this.collectionView, 'addChildView'); this.collectionView.render(); }); @@ -254,10 +254,12 @@ describe('collection view', function() { expect(this.collectionView.onChildViewRender.callCount).to.equal(2); }); - it('should call `addChild` for each item in the collection', function() { - expect(this.collectionView._addChild).to.have.been.calledTwice. - and.calledWith(this.collection.models[0]). - and.calledWith(this.collection.models[1]); + it('should call `addChildView` for each item in the collection', function() { + var v1 = this.collectionView.children.findByIndex(0); + var v2 = this.collectionView.children.findByIndex(1); + expect(this.collectionView.addChildView).to.have.been.calledTwice. + and.calledWith(v1). + and.calledWith(v2); }); it('should be marked rendered', function() { @@ -370,7 +372,7 @@ describe('collection view', function() { collection: this.collection }); this.collectionView.render(); - this.childView = this.collectionView._addChild(this.model, ChildView, 0); + this.childView = this.collectionView._addChild(this.model, 0); }); it('should call "render" on the childView', function() { @@ -648,7 +650,7 @@ describe('collection view', function() { onRenderEmpty: function() {}, render: function() { - this._addChild(suite.model, this.childView, 0); + this._addChild(suite.model, 0); } }); @@ -1277,7 +1279,7 @@ describe('collection view', function() { describe('when a child view is added to a collection view, after the collection view has been shown', function() { beforeEach(function() { this.sinon.spy(this.collectionView, 'attachBuffer'); - this.sinon.spy(this.collectionView, '_addChild'); + this.sinon.spy(this.collectionView, 'addChildView'); this.model3 = new Backbone.Model({foo: 3}); this.collection.add(this.model3); this.childView3 = this.collectionView.children.findByIndex(2); @@ -1287,8 +1289,8 @@ describe('collection view', function() { expect(this.collectionView.attachBuffer).not.to.have.been.called; }); - it('should call addChild with the new model', function() { - expect(this.collectionView._addChild).to.have.been.calledWith(this.model3); + it('should call addChildView with the new view', function() { + expect(this.collectionView.addChildView).to.have.been.calledWith(this.childView3); }); it('should trigger events on the added child view in proper order', function() { @@ -1304,7 +1306,7 @@ describe('collection view', function() { this.childViewAtIndex0 = this.collectionView.children.findByIndex(0); this.beforeModel = new Backbone.Model({foo: 0}); - this.collectionView._addChild(this.beforeModel, this.ChildView, 0); + this.collectionView._addChild(this.beforeModel, 0); }); it('should increment the later childView indexes', function() { @@ -1558,7 +1560,7 @@ describe('collection view', function() { beforeEach(function() { this.model = new Backbone.Model({foo: 'bar'}); this.collectionView = new this.CollectionView(); - this.childView = this.collectionView._addChild(this.model, this.ChildView, 0); + this.childView = this.collectionView._addChild(this.model, 0); }); it('should return the child view for the model', function() { From b129a02fef572c4ca7f53cb2de602708cb530bee Mon Sep 17 00:00:00 2001 From: "Allan L. Bazinet" Date: Tue, 13 Sep 2016 07:41:04 -0700 Subject: [PATCH 14/14] Expose normalizeUIString Expose normalizeUIString, to parallel normalizeUIKeys and normalizeUIValues, issue #3174. --- src/mixins/ui.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/mixins/ui.js b/src/mixins/ui.js index a1e2d34442..0af74f42f3 100644 --- a/src/mixins/ui.js +++ b/src/mixins/ui.js @@ -49,6 +49,13 @@ export default { return normalizeUIKeys(hash, uiBindings); }, + // normalize the passed string with the views `ui` selectors. + // `"@ui.bar"` + normalizeUIString(uiString) { + const uiBindings = this._getUIBindings(); + return normalizeUIString(uiString, uiBindings); + }, + // normalize the values of passed hash with the views `ui` selectors. // `{foo: "@ui.bar"}` normalizeUIValues(hash, properties) {