Consider merging the events swapOut and swap (Marionette.Region)

[paulovieira]

After reading the documentation (see #1859) and inspecting the code in the show method (Marionette.Region), I don't see the advantage of triggering 2 different events on the region for the act of swapping a view: swap and swapOut.

What if we had only a single swap event?

That is:

before:swap

Triggered before the new view has replaced the old view within the Region. The new view is passed as the first argument. The old view is passed as the second argument.

Note: at the moment this event is triggered, region.currentView is still the old view.

    if (isChangingView) {
        this.triggerMethod('before:swap', view, this.currentView);
    }

swap

Triggered when the new view has replaced the old view within the Region. The new view is passed as the first argument. The old view is passed as the second argument.

Note: at the moment this event is triggered, region.currentView is already the new view.

    this.attachHtml(view);
    var oldView = this.currentView;  // <- new code
    this.currentView = view;

    if (isChangingView) {
        this.triggerMethod('before:swap', view, oldView);
    }

This would make things more simple, no?

[jamesplease]

Thanks for the suggestion, @paulovieira! I think we need 2 events, though, when you consider things from the perspective of the views. As you pointed out, swap is triggered on the view that's coming in, while swapOut is triggered on the view that's going out. I think it's more useful to have two separate events for these cases, rather than trying to parse it out with a single event.

If we adopted your pattern, you'd have to do:

// within myView

this.on('swap', function(newView, oldView) {
  if (this === newView) {
    // I'm being swapped in 
  } else {
    // I'm being swapped out
  }
}

which is no good, I think.

In v3, the names will be swapIn and swapOut.

[jamiebuilds]

To clarify what @jmeas was saying, in the future we would like to be calling these triggerMethods on the view instead of the region where they would be much more useful, and in which case there should be two.

However, I think the triggerMethods on the region could still be useful. In which case it does make sense to have a single triggerMethod. So I'm 👍 to this idea.

[paulovieira]

I wasn't aware that these events should also be triggered on the views.

I agree that swapOut and swapIn are more meaningful on views. But for regions, a single swap makes more sense.

I always see a region as the stage, and the views as blind actors (assume the stage can only have one actor at a given time). The actors don't know who was at the stage before them, nor who will come after them. Only the stage has that knowledge.

So for the swapOut events on views, it doesn't make sense to pass the new view. And the same for swapIn.

To wrap up, my choice would be something like:

• on regions, only 1 event: swap. The callback would receive newView and oldView
• on views, 2 events:
  • swapIn if its the new view (that is, the view in the first argument of region.show). The callback will receive no arguments.
  • swapOut if it's the old view (that is, the view referenced by region.currentView at the time region.show is called). The callback will receive no arguments.

[jamesplease]

I was wrong before; I wasn't aware we were only triggering it on the region. Agree w. you both 👍

[samccone]

This was added the region initially to support custom animations and to let the region be a bit more aware of what is going on inside of it.

I think adding it on the view would be nice also, but there was a use case and rational behind why this was added to regions in the first place.

[samccone]

I am still behind having two events. One to "clear" the stage and one to present a new "actor" on the stage.

Lets look at the code

32116ff#diff-fda89a81760f4fc087909fd080d578c7R146

when a view is being swapped out the region now knows about it, this is really useful if you want to fade/move/do anything to the region or child.

Here is an even better question why should a view be aware of other views being shown where the current view is? should the view even know it is a region? I don't think so and it seems like a leak of knowledge. The region should be aware of its children and what the children are doing. the children should not be aware of what potential new views are going to do...

Some thoughts, again, I do not think these things were blindly added there was a rational.

[jamesplease]

Makes sense, @samccone. This has nothing to do with animations since it's all synchronous, but you're right that there is a pretty big diff. between the two.

I'm cool keeping it separate on the region as well.

[samccone]

it's all synchronous

Indeed with the default region implementation it is. But this is really to help people doing custom regions :)

[paulovieira]

Makes sense then to have two events on the region. We get more fine-grained events. The names would be more meaningful as swapOut/swapIn, but it seems this is scheduled for v3.

So to sum it up, with the current names:

• swapOut is meant to used only with the old view (the view referenced by region.currentView at the time region.show is called)
• swap is meant to be used only the new view (the view referenced by the first argument given to region.show)

The only thing that remains confusing to me is the placement of the swapOut trigger. The proposed documentation (#1859) says:

Triggered when the new view has replaced the old view within the Region.

However swapOut is triggered before the described action has happened. We have to move

this.triggerMethod('swapOut', this.currentView);

to be after

this.attachHtml(view);

[jamesplease]

Agreed @paulovieira!

Because this has strayed from the original post, I'm going to close this issue and open a new one. Also, since this is all v3, I'm going to close everything & add them to #1796.

Thanks for all your awesome work lately @paulovieira :)