Maluen / Backbone-Debugger

Chrome Developer Tools extension for debugging Backbone.js applications
Mozilla Public License 2.0
704 stars 44 forks source link

Backbone Layoutmanager #43

Open arfa opened 9 years ago

arfa commented 9 years ago

When I use Backbone Layoutmanager to manage my views, I come to issue where the remove method does not work as expected.

In fact when I remove a layout, the subviews are not deleted and become zombie views.

I'm talking based on what Backbone-Debugger shows: this is what it shows: zombies

and this is what it must show: Deleted

In those screenshots view 3 is the layout and the others are subviews.

I think this issue is mentionned in the section "Known Limitations". I open an issue because I think Backbone Layoutmanager is one of the most importants extensions as it helps to manage views and prevent Backbone views to grow up to be Zombies.

Maluen commented 9 years ago

I'm gonna do some debugging with an app using Layoutmanager and will report.

Maluen commented 9 years ago

Views are removed, but aren't displayed as such in Backbone-Debugger.

Backbone-Debugger considers a view removed when its "remove" method has been called, as per Backbone.View interface.

Backbone Layoutmanager however, removes the subviews by using the LayoutManager._removeView static method.

I can't see any procedural way of detecting if a View has been removed, other than when the remove method is called.

A more ambitious way to solve this is by adding some plugin layer to Backbone-Debugger, so that specific plugins such as Backbone Layoutmanager can be fully supported. But this is quite an ambitious task.

I'm open to all kinds of suggestions.

Maluen commented 9 years ago

Note: when removing a non-layout view directly via its "removeView" method, then the subviews are removed correctly by using the remove method:

  // Use this to remove Views, internally uses `getViews` so you can pass the
  // same argument here as you would to that method.
  removeView: function(fn) {
    var views;

    // Allow an optional selector or function to find the right model and
    // remove nested Views based off the results of the selector or filter.
    views = this.getViews(fn).each(function(nestedView) {
      nestedView.remove();
    });

    // call value incase the chain is evaluated lazily to ensure the views get
    // removed
    views.value();

    return views;
  },
arfa commented 9 years ago

Thank you for your quick response. I myself made some debugging and I concluded the same thing. thanks for this confirmation.

Indeed in the second screenshot, I used removeView to remove the subviews and when the empty event is called I remove the layout.

mylayout.on("empty", function(view) {
    // Do something to view, after all nested Views are removed.
    this.remove()
});

mylayout.removeView();
Maluen commented 9 years ago

So what if a subview has a custom "remove" method? Won't it be called when doing "mylayout.remove()"?

Probably I'm missing something, otherwise an issue could be opened on the LayoutManager repo, so to always call the remove method.

arfa commented 9 years ago

Good point of vue,

In fact when removing a layout, the instance call its own remove function, then it calls the original remove function:

remove: function() {
    // Force remove itself from its parent.
    LayoutManager._removeView(this, true);

    // Call the original remove function.
    return this._remove.apply(this, arguments);
}

The subviews are deleted when calling LayoutManager._removeView(this, true); but without calling the remove function of the subview.

// Remove a single nested View.
_removeView: function(view, force) {
...
    // Clean out the events.
    LayoutManager.cleanViews(view);

    // Since we are removing this view, force subviews to remove
    view._removeViews(true);

    // Remove the View completely.
    view.$el.remove();
...
}

This snippet of code clean out the events when calling LayoutManager.cleanViews(view); and then remove the view from the DOM view.$el.remove();

So if I am not wrong, they can use the original remove function without affecting the current logic. Simply replace view.$el.remove(); by calling the original function view.ORIGINAL_REMOVE();.

Maybe view._remove.apply(view, null); because view.remove(); is a custom remove now.

They can keep stopListening inside the function cleanViews so it continues to do what it is supposed to do.

All that said, if a developper want to implement his custom remove function, his function must call the layout's remove function, otherwise this will break all the logic. (especially if he sets the property manage to true to treat Views as Layouts)

Maluen commented 9 years ago

@arfa Nice suggestions, any chance you could open an issue on the LayoutManager repository for adding the explicit remove call to subviews when removing a Layout?