Properly closing views in Marionette

Question

I have an application built on Backbone.Marionette with a CollectionView that instantiates many CompositeViews, which render a tree structure.

I've read through Zombie Views (Bailey on Zombies) and through View and Region documentation. However, if everything looks pretty simple when reading, execution is a different issue altogether.

When I hit any of my routes, my keyboard shortcuts end up being fired multiple times. I found a work around, but this workaround causes other problems on rendering changes in the views.

Here is the actual code that triggers the keyboard shortcuts multiple times. In Snippet A, I have added any way of closing the view that I could think of, despite the fact that normally, closing a view should only require App.contentRegion.currentView.treeRegion.close()

showContentView: (tree) ->
  if @treeView?
    App.contentRegion.currentView.treeRegion.reset()
    App.contentRegion.currentView.treeRegion.close()
    @treeView.close()
    delete @treeView
  @treeView = new App.Note.TreeView(collection: tree)
  App.contentRegion.currentView.treeRegion.show @treeView

Snippet B, below, fixes the keyboard shortcut issue. However, it causes the issue where additionally created models (CompositeView) aren't rendered to the user.

showContentView: (tree) ->
  if @treeView?
    @treeView.collection = tree
    @treeView.render()
  else
    @treeView = new App.Note.TreeView(collection: tree)
    App.contentRegion.currentView.treeRegion.show @treeView

Here is where I initialize the CollectionView, which in turn renders the CompositeViews

initialize: -> # collectionView
  @listenTo @collection, "sort", @render
  @listenTo @collection, "destroy", @addDefaultNote
  Note.eventManager.on 'createNote', @createNote, this
  Note.eventManager.on 'change', @dispatchFunction, this
  @drag = undefined

initialize: -> # compositeView
  @collection = @model.descendants
  @bindKeyboardShortcuts()
  @listenTo @collection, "sort", @render
  Note.eventManager.on "setCursor:#{@model.get('guid')}", @setCursor, @
  Note.eventManager.on "render:#{@model.get('guid')}", @render, @
  Note.eventManager.on "setTitle:#{@model.get('guid')}", @setNoteTitle, @

This is how I bind my keyboard shortcuts in the CompositeViews

bindKeyboardShortcuts: ->
  @.$el.on 'keydown', null, 'ctrl+shift+backspace', @triggerShortcut 'deleteNote'
  @.$el.on 'keydown', null, 'tab', @triggerShortcut 'tabNote'
  @.$el.on 'keydown', null, 'shift+tab', @triggerShortcut 'unTabNote'
  @.$el.on 'keydown', null, 'alt+right', @triggerShortcut 'tabNote'
  @.$el.on 'keydown', null, 'alt+left', @triggerShortcut 'unTabNote'
  @.$el.on 'keydown', null, 'alt+up', @triggerShortcut 'jumpPositionUp'
  @.$el.on 'keydown', null, 'alt+down', @triggerShortcut 'jumpPositionDown'
  @.$el.on 'keydown', null, 'up', @triggerShortcut 'jumpFocusUp'
  @.$el.on 'keydown', null, 'down', @triggerShortcut 'jumpFocusDown'
  @.$el.on 'keydown', null, 'alt+ctrl+left', @triggerShortcut 'zoomOut'
  @.$el.on 'keydown', null, 'alt+ctrl+right', @triggerShortcut 'zoomIn'

And how I trigger them

triggerShortcut: (event) -> (e) =>
  e.preventDefault()
  e.stopPropagation()
  @triggerEvent(event).apply(@, Note.sliceArgs arguments)
triggerEvent: (event) ->
  (e) =>
    @updateNote()
    args = ['change', event, @model].concat(Note.sliceArgs arguments, 0)
    Note.eventManager.trigger.apply(Note.eventManager, args)

Finally, to make sure everything is clean, I unbind every shortcut in onBeforeClose. I also unbind any of eventManager's listeners.

onBeforeClose: ->
  console.log "view being closed", @
  @$el.off()
  Note.eventManager.off "setCursor:#{@model.get('guid')}"
  Note.eventManager.off "render:#{@model.get('guid')}"
  Note.eventManager.off "setTitle:#{@model.get('guid')}"
  Note.eventManager.off "timeoutUpdate:#{@model.get('guid')}"

I know the problem comes from @treeView = new App.Note.TreeView(collection: tree). If I create a new TreeView on each @showContentView (Snippet A), every added model is properly rendered to the view, but the shortcuts get crazy.
On the other hand, if I create one TreeView and swap it's collection (Snippet B), I get the rendering problem in the views, but shortcuts are fine!

I tried to include everything you would need, nothing more (it's already quite some code..) but if you guys need anything else, please ask!
Hope I could get that clear enough..

[Edit] I have tried many different combinations to get rid of the shortcut bug, but if I create a new TreeView on each showContentView, nothing seems to close the view properly. I think this is coming from a deeper memory leak problem. I will probably write an other StackOverflow quesion in that regard, and link to this one for additional information.

Thank you very much!

Answer 1

I figured out what was the issue here.

Using Snippet A and chrome devtool's profiler, I could track down the leak. The onClose method that I provided in my question comes from the CompositeView, where the keyboard shortcut are bound.
The problem was the CollectionView wasn't getting garbage collected because of the use of Note.eventManager.on, which was keeping a reference to the view. So I added a onBeforeClose method to the TreeView (CollectionView)

onBeforeClose: ->
  Note.eventManager.off('createNote', @createNote, this)
  Note.eventManager.off('change', @dispatchFunction, this)
  @drag = undefined

With this onBeforeClose, the view is now being properly closed, which in turn allows the children views to be closed as well and to stop listening to the shortcuts being fired.

I guess that was pretty obvious, once I found out, but I wanted to add this answer so it makes clear any event listener that you set up without @listenTo doesn't get cleared up by Marionette and needs to be properly handled.

[edit]

To follow up on the comments, here would have been a better solution from the start :

Replace

initialize: -> # compositeView
  /* ... */
  Note.eventManager.on "setCursor:#{@model.get('guid')}", @setCursor, @
  Note.eventManager.on "render:#{@model.get('guid')}", @render, @
  Note.eventManager.on "setTitle:#{@model.get('guid')}", @setNoteTitle, @

And

initialize: -> # collectionView
  /* ... */
  Note.eventManager.on 'createNote', @createNote, this
  Note.eventManager.on 'change', @dispatchFunction, this

with

initialize: -> # compositeView
  /* ... */
  @listenTo Note.eventManager, "setCursor:#{@model.get('guid')}", @setCursor
  @listenTo Note.eventManager, "render:#{@model.get('guid')}", @render
  @listenTo Note.eventManager, "setTitle:#{@model.get('guid')}", @setNoteTitle

/* ... */

initialize: -> # collectionView
  /* ... */
  @listenTo Note.eventManager, 'createNote', @createNote, this
  @listenTo Note.eventManager, 'change', @dispatchFunction, this

Using the listenTo syntax as so would have prevented the memory leak in the first place. As so, the onBeforeClose blocks can be completely removed!