Skip to content

Commit

Permalink
Fixed ViewController cleanup issue reported w/Sencha Touch
Browse files Browse the repository at this point in the history
Deft.mvc.ViewController::onViewDestroyed() is never called on Sencha Touch (due to the differing destruction lifecycle path). Consequently, the cleanup logic implemented there is not executed on that platform.
Moved the clean-up logic to the destroy() method. This will require any subclasses to call `this.callParent()`, but that should be fairly well understood as a standard practice when overriding destroy() method.
Added associated unit tests to help prevent future regression.

Fixes #45
  • Loading branch information
John Yanarella committed Aug 10, 2012
1 parent a29fb9a commit a5cd776
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 48 deletions.
23 changes: 7 additions & 16 deletions build/deft-debug.js
Expand Up @@ -881,6 +881,13 @@ Ext.define('Deft.mvc.ViewController', {
*/ */


destroy: function() { destroy: function() {
var id, selector;
for (id in this.registeredComponentReferences) {
this.removeComponentReference(id);
}
for (selector in this.registeredComponentSelectors) {
this.removeComponentSelector(selector);
}
return true; return true;
}, },
/** /**
Expand All @@ -891,9 +898,6 @@ Ext.define('Deft.mvc.ViewController', {
var config, id, listeners, live, originalViewDestroyFunction, selector, self, _ref; var config, id, listeners, live, originalViewDestroyFunction, selector, self, _ref;
if (Ext.getVersion('extjs') != null) { if (Ext.getVersion('extjs') != null) {
this.getView().on('beforedestroy', this.onViewBeforeDestroy, this); this.getView().on('beforedestroy', this.onViewBeforeDestroy, this);
this.getView().on('destroy', this.onViewDestroy, this, {
single: true
});
} else { } else {
self = this; self = this;
originalViewDestroyFunction = this.getView().destroy; originalViewDestroyFunction = this.getView().destroy;
Expand Down Expand Up @@ -941,19 +945,6 @@ Ext.define('Deft.mvc.ViewController', {
} }
return false; return false;
}, },
/**
@private
*/

onViewDestroy: function() {
var id, selector;
for (id in this.registeredComponentReferences) {
this.removeComponentReference(id);
}
for (selector in this.registeredComponentSelectors) {
this.removeComponentSelector(selector);
}
},
/** /**
Add a component accessor method the ViewController for the specified view-relative selector. Add a component accessor method the ViewController for the specified view-relative selector.
*/ */
Expand Down
2 changes: 1 addition & 1 deletion build/deft.js

Large diffs are not rendered by default.

16 changes: 14 additions & 2 deletions spec/coffee/Deft/mvc/ViewController.coffee
Expand Up @@ -1538,7 +1538,7 @@ describe( 'Deft.mvc.ViewController', ->
return return
) )


it( 'should remove event listeners it attached to a view component referenced implicitly by item id when the associated view (and view controller) is destroyed', -> it( 'should remove event listeners it attached to a view component referenced implicitly by itemId when the associated view (and view controller) is destroyed', ->
Ext.define( 'ExampleViewController', Ext.define( 'ExampleViewController',
extend: 'Deft.mvc.ViewController' extend: 'Deft.mvc.ViewController'


Expand All @@ -1560,6 +1560,8 @@ describe( 'Deft.mvc.ViewController', ->


component = view.query( '#example' )[ 0 ] component = view.query( '#example' )[ 0 ]


expect( viewController.getExample ).not.toBe( null )

expect( hasListener( component, 'exampleevent' ) ).toBe( true ) expect( hasListener( component, 'exampleevent' ) ).toBe( true )


spyOn( viewController, 'destroy' ).andCallThrough() spyOn( viewController, 'destroy' ).andCallThrough()
Expand All @@ -1569,6 +1571,7 @@ describe( 'Deft.mvc.ViewController', ->
view.destroy() view.destroy()


expect( viewController.destroy ).toHaveBeenCalled() expect( viewController.destroy ).toHaveBeenCalled()
expect( viewController.getExample ).toBe( null )
expect( isViewDestroyed ).toBe( true ) expect( isViewDestroyed ).toBe( true )


expect( hasListener( component, 'exampleevent' ) ).toBe( false ) expect( hasListener( component, 'exampleevent' ) ).toBe( false )
Expand Down Expand Up @@ -1610,6 +1613,8 @@ describe( 'Deft.mvc.ViewController', ->


components = view.query( 'example' ) components = view.query( 'example' )


expect( viewController.getExample ).not.toBe( null )

for component in components for component in components
expect( hasListener( component, 'exampleevent' ) ).toBe( true ) expect( hasListener( component, 'exampleevent' ) ).toBe( true )


Expand All @@ -1620,6 +1625,7 @@ describe( 'Deft.mvc.ViewController', ->
view.destroy() view.destroy()


expect( viewController.destroy ).toHaveBeenCalled() expect( viewController.destroy ).toHaveBeenCalled()
expect( viewController.getExample ).toBe( null )
expect( isViewDestroyed ).toBe( true ) expect( isViewDestroyed ).toBe( true )


for component in components for component in components
Expand All @@ -1628,7 +1634,7 @@ describe( 'Deft.mvc.ViewController', ->
return return
) )


it( 'should remove event listeners it attached to a dynamic view component referenced by a live selector implicitly by item id when the associated view (and view controller) is destroyed', -> it( 'should remove event listeners it attached to a dynamic view component referenced by a live selector implicitly by itemId when the associated view (and view controller) is destroyed', ->
Ext.define( 'ExampleViewController', Ext.define( 'ExampleViewController',
extend: 'Deft.mvc.ViewController' extend: 'Deft.mvc.ViewController'


Expand Down Expand Up @@ -1656,6 +1662,8 @@ describe( 'Deft.mvc.ViewController', ->
} }
) )


expect( viewController.getDynamicExample ).not.toBe( null )

expect( hasListener( component, 'exampleevent' ) ).toBe( true ) expect( hasListener( component, 'exampleevent' ) ).toBe( true )


spyOn( viewController, 'destroy' ).andCallThrough() spyOn( viewController, 'destroy' ).andCallThrough()
Expand All @@ -1665,6 +1673,7 @@ describe( 'Deft.mvc.ViewController', ->
view.destroy() view.destroy()


expect( viewController.destroy ).toHaveBeenCalled() expect( viewController.destroy ).toHaveBeenCalled()
expect( viewController.getDynamicExample ).toBe( null )
expect( isViewDestroyed ).toBe( true ) expect( isViewDestroyed ).toBe( true )


expect( hasListener( component, 'exampleevent' ) ).toBe( false ) expect( hasListener( component, 'exampleevent' ) ).toBe( false )
Expand Down Expand Up @@ -1708,6 +1717,8 @@ describe( 'Deft.mvc.ViewController', ->
] ]
) )
components = view.query( 'example' ) components = view.query( 'example' )

expect( viewController.getDynamicExample ).not.toBe( null )
expect( viewController.getDynamicExample() ).toEqual( components ) expect( viewController.getDynamicExample() ).toEqual( components )
expect( viewController.getDynamicExample().length ).toEqual( 4 ) expect( viewController.getDynamicExample().length ).toEqual( 4 )


Expand All @@ -1721,6 +1732,7 @@ describe( 'Deft.mvc.ViewController', ->
view.destroy() view.destroy()


expect( viewController.destroy ).toHaveBeenCalled() expect( viewController.destroy ).toHaveBeenCalled()
expect( viewController.getDynamicExample ).toBe( null )
expect( isViewDestroyed ).toBe( true ) expect( isViewDestroyed ).toBe( true )


for component in components for component in components
Expand Down
12 changes: 10 additions & 2 deletions spec/js/Deft/mvc/ViewController.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 4 additions & 11 deletions src/coffee/Deft/mvc/ViewController.coffee
Expand Up @@ -61,6 +61,10 @@ Ext.define( 'Deft.mvc.ViewController',
Destroy the ViewController Destroy the ViewController
### ###
destroy: -> destroy: ->
for id of @registeredComponentReferences
@removeComponentReference( id )
for selector of @registeredComponentSelectors
@removeComponentSelector( selector )
return true return true


###* ###*
Expand All @@ -70,7 +74,6 @@ Ext.define( 'Deft.mvc.ViewController',
if Ext.getVersion( 'extjs' )? if Ext.getVersion( 'extjs' )?
# Ext JS # Ext JS
@getView().on( 'beforedestroy', @onViewBeforeDestroy, @ ) @getView().on( 'beforedestroy', @onViewBeforeDestroy, @ )
@getView().on( 'destroy', @onViewDestroy, @, single: true )
else else
# Sencha Touch # Sencha Touch
self = this self = this
Expand Down Expand Up @@ -110,16 +113,6 @@ Ext.define( 'Deft.mvc.ViewController',
return true return true
return false return false


###*
@private
###
onViewDestroy: ->
for id of @registeredComponentReferences
@removeComponentReference( id )
for selector of @registeredComponentSelectors
@removeComponentSelector( selector )
return

###* ###*
Add a component accessor method the ViewController for the specified view-relative selector. Add a component accessor method the ViewController for the specified view-relative selector.
### ###
Expand Down
23 changes: 7 additions & 16 deletions src/js/Deft/mvc/ViewController.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit a5cd776

Please sign in to comment.