GeppettoJS / backbone.geppetto

Bring your Backbone applications to life with an event-driven Command framework.
http://geppettojs.github.com/backbone.geppetto/
MIT License
203 stars 28 forks source link

problem with dispatchGlobally #49

Closed mmikeyy closed 10 years ago

mmikeyy commented 10 years ago

The function iterates over an array containing all defined contexts, triggering each one.

    Geppetto.Context.prototype.dispatchGlobally = function dispatchGlobally(eventName, eventData) {
        _.each(contexts, function(context) {
            context.vent.trigger(eventName, eventData);
        });
    };

The problem is that when an event thus triggered destroys a context that's not yet been processed, the loop fails when it reaches this context (trying to access vent of undefined).

This can occur for example when one triggers an event that closes a view, which destroys its context as a side effect.

The fix is simple:

    Geppetto.Context.prototype.dispatchGlobally = function dispatchGlobally(eventName, eventData) {

        _.each(contexts, function(context) {
            if (!context){
                return true // skip this context
            }
            context.vent.trigger(eventName, eventData);
        });
    };
geekdave commented 10 years ago

Thanks for catching, @mmikeyy ! Would you be able to whip up a PR for this with a test spec that demonstrates the issue? Might be awhile before I can get to this...

mmikeyy commented 10 years ago

ok. I've prepared a small demonstration.

Here's the main:

require.config

  baseUrl: "include"
  paths:
    jquery:     "//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min"
    jqueryui:   '//ajax.googleapis.com/ajax/libs/jqueryui/1.10.4/jquery-ui.min'
    underscore: 'lib/underscore-min'
    backbone:   'lib/backbone-min'
    marionette: 'lib/backbone.marionette.min'
    geppetto:   'lib/backbone.geppetto'

require ['test_events'], (obj)->

# uncomment one of the two following lines to test or to just run program

  obj.run() # uncomment to fiddle with program
  #obj.test() # uncomment to just return and display result

Now here is the coffeescript version of test_event.js to be located in baseUrl:

define [
    'underscore'
    'jquery'
    'jqueryui'
    'backbone'
    'marionette'
    'geppetto'
  ],
(
  _
  $
  $ui
  Backbone
  Marionette
  Geppetto
)->

  # data ot fill collection
  data_names = [
    id: 1, name: 'john'
  ,
    id: 2, name: 'alex'
  ,
    id: 3, name: 'mikey'
  ,
    id: 4, name: 'Theresa'
  ,
    id: 5, name: 'Emilio'
  ]

  class one_name extends Backbone.Model
    initialize: ({@id,@name})->

  class names extends Backbone.Collection
    model: one_name
    comparator:(model)->
      (model.get 'name').toLowerCase()

  cmd =
    reset_collection: class reset_coll
      wiring: ['collection_names']
      execute:->
        @collection_names.reset data_names

    reduce_collection: class reduce_coll
      wiring: ['collection_names']
      execute: ->
        size = @collection_names.size()
        @collection_names.pop() while size-- > 4

  class main_context extends Geppetto.Context
    initialize: ->
      $.extend true, @wiring, values:main:context:@ # don't look at this. it's just a quick way I found to make context accessible from view
    wiring:
      commands:
        reset_collection: cmd.reset_collection
        reduce_collection: cmd.reduce_collection
      singletons:
        collection_names: names
      values:
        main: {}

  class view_one_name extends Marionette.ItemView

    initialize: ({@parentContext})->
      @listenTo @model, 'change', @render
      Geppetto.bindContext
        view: @
        context: Geppetto.Context # empty context; just needed for dispatching from view
        parentContext: @parentContext

    template:(data)->
      _.template """
      <td style="width:36px;text-align:right">
        <span class="edit input" style="display:none">
          <span class="ui-icon ui-icon-check accept"></span>
          <span class="ui-icon ui-icon-close reject"></span>
        </span>
      </td>
      <td  style="width:150px">
        <span class="edit link name" style="cursor: pointer">
          <%- data.name %>
        </span>
        <span class="edit input" style="display:none">
          <input maxlength="10" size="10"/>
        </span>
      </td>

      """, data, variable:'data'
    tagName: 'TR'

    ui:
      edit_spans: 'span.edit'
      accept: 'span.accept'
      reject: 'span.reject'
      input: 'input'
      name: 'span.name'
      edit: 'span.edit.input'

    contextEvents:
      close_local_edit: 'close_edit'

    events:
      'click @ui.reject':     'toggle_edit'
      'click @ui.accept':     'accept'
      'click @ui.name':       'edit'

    onDestroy: ->
      @trigger 'close' # do this because Geppetto uses the old 'close' instead of the newer 'destroy'

    toggle_edit: ->
      @ui.edit_spans.toggle()

    close_edit: ->

      @context.dispatchToParent 'reduce_collection'
      return if @isDestroyed # as the current view may be one of the ones destroyed by previous line
      @ui.name.show()
      @ui.edit.hide()

    edit: ->
      try
        @context.dispatchGlobally 'close_local_edit'
        @toggle_edit()
        @ui.input.val(@model.get 'name').focus()
        @context.dispatchToParent 'show_pass'
      catch err
        @context.dispatchToParent 'show_fail', msg:err
    accept: ->
      @close_edit()
      @model.set name: @ui.input.val()

  class view_names extends Marionette.CollectionView
    initialize: ({@context})->
      @listenTo @collection, 'reset', @render
      @listenTo @collection, 'change:name', =>@collection.sort()
    childView: view_one_name
    childViewOptions: ->
      parentContext: @context

  class layout extends Marionette.LayoutView
    wiring: ['main', 'collection_names']
    initialize: ->
      @$el.appendTo 'body'

      Geppetto.bindContext
        view: @
        context: main_context

      @context = @main.context

      @render()
      @dispatch 'reset_collection'

    ui:
      buttons:  'button'
      b_reset:  'button.reset'
      b_render: 'button.render'
      b_show_count: 'button.show_count'
      pass:     'span.success' # success msg
      fail:     'span.fail' # fail msg
      msg:      'span.msg' # reason for error
      count:    'span.count' # count of names in collection

    contextEvents:
      # display test results
      show_pass:    'show_pass'
      show_fail:    'show_fail'

    events: ->
      'click @ui.b_reset': =>@dispatch 'reset_collection'
      'click @ui.b_render': @render
      'click @ui.b_show_count': 'show_count'

    template: ->"""
        <b>Click on a name to do the following:</b>
        <ul>
          <li>
            bring all lines out of edit mode (to ensure that no other line is left in edit mode)
          </li>
          <li>
            put the clicked line in edit mode (it will be the only line in edit mode)
          </li>
          <li>
             destroy all lines beyond the 4th line
         </li>
        </ul>
        <div style="width:600px">
        ** of course, this program is not doing anything very smart. It's just for test purposes.
        The problem occurs when a globally-dispatched event is in the process of looping through all
        contexts to allow each one to process it, and the processing of the event in a particular
        context causes the destruction of a view (and associated context) in a yet unprocessed context.
        When the looping later reaches the destroyed context, an attempt is made to have it process the event, which
        causes the error.
        <p>Keep on clicking on names in list below after initial error to see what the program does when it works</p>

        </div>
        <hr/>
        <div id="main" style="margin-top:10px;border: solid 1px #aaa; border-radius: 5px;padding:10px;width:auto;display:inline-block">

            <table class="no_borders">
                <tbody class="names"></tbody>
            </table>
            <button class="reset">Reset</button> <button class="render">Render</button> <button class="show_count">show count</button><span class="count"></span>
        <hr/>
        <span style="background-color:green;color:white;padding:4px;display:none" class="success">SUCCESS!</span>
        <span style="background-color:red;color:white;padding:4px;display:none" class="fail">FAIL!<span style="margin-left:10px" class="msg"></span></span>
        </div>

      """

    onRender: ->
      @addRegions
        names: @$('tbody.names')

      @collectionView = new view_names  collection: @collection_names, context: @context
      @names.show @collectionView
      @ui.buttons.button()

    show_count: ->
      @ui.count.stop(true,true).text(@collection_names.size()).show().fadeOut()

    show_pass: ->
      @ui.pass.show()
      @ui.fail.hide()
    show_fail: (data)->
      @ui.pass.hide()
      @ui.fail.show()
      if data?.msg
        @ui.msg.text(data.msg).show()
      else
        @ui.msg.hide()

    test: ->
      try
        @context.dispatchGlobally 'close_local_edit'
        console.log 'success'
        $('body').append $('<div/>').text('success')
        return true
      catch
        console.log 'fail'
        $('body').append $('<div/>', css:color:'red').text('fail')
        return false
      finally
        console.log 'end test'
        @destroy()

  # exports
  run: ->
    new layout

  test: ->
    (new layout).test()

Finally the html:

<!DOCTYPE html>
<html>
<head lang="en">
    <meta charset="UTF-8">
    <title></title>

    <script data-main="include/main.js" src="include/require.js"></script>
    <link rel="stylesheet" type="text/css" href="http://ajax.googleapis.com/ajax/libs/jqueryui/1.10.3/themes/blitzer/jquery-ui.css"/>

    <style>
        body{font:13px/1.231 arial,helvetica,clean,sans-serif;*font-size:small;*font:x-small;}select,input,button,textarea,button{font:99% arial,helvetica,clean,sans-serif;}table{font-size:inherit;font:100%;}pre,code,kbd,samp,tt{font-family:monospace;*font-size:108%;line-height:100%;}
        table.no_borders, table.no_borders td, table.no_borders th {
            border: none;
        }
        a {
            cursor:pointer;
        }
        tr:hover a {
            visibility: visible
        }
        table td span.ui-icon {
            cursor: pointer;
            display: inline-block!important
        }

    </style>

</head>
<body>

</body>

</html>

So apart from the html, the only files required are:

require.js main.js test_events.js

plus of course underscore, backbone, marionnette, geppetto (latest versions...)

geekdave commented 10 years ago

Fixed in https://github.com/ModelN/backbone.geppetto/pull/55