Skip to content

Commit

Permalink
Fix region view destroy bug.
Browse files Browse the repository at this point in the history
  • Loading branch information
samccone committed Jul 18, 2014
1 parent be4cd62 commit 15d95e1
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 8 deletions.
40 changes: 39 additions & 1 deletion spec/javascripts/region.spec.js
Expand Up @@ -99,6 +99,8 @@ describe('region', function() {
this.regionSwapSpy = this.sinon.spy();
this.viewBeforeShowSpy = this.sinon.spy();
this.viewShowSpy = this.sinon.spy();
this.regionEmptySpy = this.sinon.spy();
this.regionBeforeEmptySpy = this.sinon.spy();

this.view = new this.MyView();
this.sinon.spy(this.view, 'render');
Expand All @@ -112,7 +114,8 @@ describe('region', function() {
this.myRegion.on('before:show', this.regionBeforeShowSpy);
this.myRegion.on('before:swap', this.regionBeforeSwapSpy);
this.myRegion.on('swap', this.regionSwapSpy);

this.myRegion.on('empty', this.regionEmptySpy);
this.myRegion.on('before:empty', this.regionBeforeEmptySpy);
this.view.on('before:show', this.viewBeforeShowSpy);
this.view.on('show', this.viewShowSpy);

Expand Down Expand Up @@ -193,6 +196,11 @@ describe('region', function() {
expect(this.regionBeforeSwapSpy).to.have.been.called;
});

it('should trigger empty once', function() {
expect(this.regionEmptySpy).to.have.been.calledOnce;
expect(this.regionBeforeEmptySpy).to.have.been.calledOnce;
});

it('should trigger a swap event for the region', function() {
expect(this.regionSwapSpy).to.have.been.called;
});
Expand Down Expand Up @@ -739,4 +747,34 @@ describe('region', function() {
expect(this.region.empty).to.have.been.called;
});
});

describe('when destroying a view in a region', function() {
beforeEach(function() {
this.setFixtures('<div id="region"></div>');
this.beforeEmptySpy = new sinon.spy();
this.emptySpy = new sinon.spy();

this.region = new Backbone.Marionette.Region({
el: '#region'
});

this.region.on('before:empty', this.beforeEmptySpy);
this.region.on('empty', this.emptySpy);

this.View = Backbone.Marionette.View.extend({
template: _.template('')
});

this.view = new this.View();

this.region.show(this.view);
this.region.currentView.destroy();
});

it('should remove the view from the region after being destroyed', function() {
expect(this.beforeEmptySpy).to.have.been.calledOnce.and.calledWith(this.view);
expect(this.emptySpy).to.have.been.calledOnce.calledWith(this.view);
expect(this.region.currentView).to.be.undefined;
});
});
});
33 changes: 26 additions & 7 deletions src/marionette.region.js
@@ -1,4 +1,4 @@
/* jshint maxcomplexity: 10, maxstatements: 28 */
/* jshint maxcomplexity: 10, maxstatements: 29 */

// Region
// ------
Expand Down Expand Up @@ -154,6 +154,13 @@ _.extend(Marionette.Region.prototype, Backbone.Events, {
var _shouldShowView = isDifferentView || forceShow;

if (_shouldShowView) {

// We need to listen for if a view is destroyed
// in a way other than through the region.
// If this happens we need to remove the reference
// to the currentView since once a view has been destroyed
// we can not reuse it.
view.once('destroy', _.bind(this.empty, this));
view.render();

if (isChangingView) {
Expand Down Expand Up @@ -218,19 +225,31 @@ _.extend(Marionette.Region.prototype, Backbone.Events, {
// current view, it does nothing and returns immediately.
empty: function() {
var view = this.currentView;
if (!view || view.isDestroyed) { return; }

this.triggerMethod('before:empty', view);

// call 'destroy' or 'remove', depending on which is found
if (view.destroy) { view.destroy(); }
else if (view.remove) { view.remove(); }
// If there is no view in the region
// we should not remove anything
if (!view) { return; }

this.triggerMethod('before:empty', view);
this._destroyView();
this.triggerMethod('empty', view);

// Remove region pointer to the currentView
delete this.currentView;
},

// call 'destroy' or 'remove', depending on which is found
// on the view (if showing a raw Backbone view or a Marionette View)
_destroyView: function() {
var view = this.currentView;

if (view.destroy && !view.isDestroyed) {
view.destroy();
} else if (view.remove) {
view.remove();
}
},

// Attach an existing view to the region. This
// will not call `render` or `onShow` for the new view,
// and will not replace the current HTML for the `el`
Expand Down

0 comments on commit 15d95e1

Please sign in to comment.