From d9e8b1bec6d34edc6ba9effeb31367d84a68df26 Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Tue, 18 Oct 2016 16:22:49 +0200 Subject: [PATCH] refactor(nav-controller-base): cleanup some logic NavControllerBase is the core of ionic 2 navigation. It handles all the transitions and it is complicated code to follow. I am refactoring it to allow future developers and contributors to follow it better. !node.parent now becomes node.isRoot() ViewController does not remove itself from the stack, but two new auxiliar function in nav controller: _insertView() and _removeView() are used to add a view to the stack. And so on... All e2e and unit tests passing... --- src/animations/animation.ts | 2 + src/components/tabs/tab.ts | 4 +- src/navigation/nav-controller-base.ts | 294 ++++++++++++----------- src/navigation/view-controller.ts | 20 +- src/transitions/transition-controller.ts | 2 +- src/transitions/transition-registry.ts | 3 +- src/transitions/transition.ts | 6 +- src/util/mock-providers.ts | 4 +- 8 files changed, 173 insertions(+), 162 deletions(-) diff --git a/src/animations/animation.ts b/src/animations/animation.ts index e5988c34d2..afa0e6127e 100644 --- a/src/animations/animation.ts +++ b/src/animations/animation.ts @@ -34,6 +34,7 @@ export class Animation { parent: Animation; opts: AnimationOptions; + hasChildren: boolean = false; isPlaying: boolean = false; hasCompleted: boolean = false; @@ -81,6 +82,7 @@ export class Animation { */ add(childAnimation: Animation): Animation { childAnimation.parent = this; + this.hasChildren = true; this._cL = (this._c = this._c || []).push(childAnimation); return this; } diff --git a/src/components/tabs/tab.ts b/src/components/tabs/tab.ts index d6ac9870aa..6f9fd80457 100644 --- a/src/components/tabs/tab.ts +++ b/src/components/tabs/tab.ts @@ -298,14 +298,14 @@ export class Tab extends NavControllerBase { /** * @private */ - _viewInsert(viewCtrl: ViewController, componentRef: ComponentRef, viewport: ViewContainerRef) { + _viewAttachToDOM(viewCtrl: ViewController, componentRef: ComponentRef, viewport: ViewContainerRef) { const isTabSubPage = (this.parent._subPages && viewCtrl.index > 0); if (isTabSubPage) { viewport = this.parent.portal; } - super._viewInsert(viewCtrl, componentRef, viewport); + super._viewAttachToDOM(viewCtrl, componentRef, viewport); if (isTabSubPage) { // add the .tab-subpage css class to tabs pages that should act like subpages diff --git a/src/navigation/nav-controller-base.ts b/src/navigation/nav-controller-base.ts index 8d8a32c60d..2b374029ed 100644 --- a/src/navigation/nav-controller-base.ts +++ b/src/navigation/nav-controller-base.ts @@ -193,9 +193,11 @@ export class NavControllerBase extends Ion implements NavController { while (trns) { if (trns.enteringView && (trns.enteringView._state !== ViewState.LOADED)) { // destroy the entering views and all of their hopes and dreams - trns.enteringView._destroy(this._renderer); + this._destroyView(trns.enteringView); + } + if (!trns.parent) { + break; } - if (!trns.parent) break; } if (trns) { @@ -253,6 +255,8 @@ export class NavControllerBase extends Ion implements NavController { const leavingView = this.getActive(); const enteringView = this._getEnteringView(ti, leavingView); + assert(leavingView || enteringView, 'Both leavingView and enteringView are null'); + // Initialize enteringView if (enteringView && isBlank(enteringView._state)) { // render the entering view, and all child navs and views @@ -327,17 +331,23 @@ export class NavControllerBase extends Ion implements NavController { const opts = ti.opts || {}; const insertViews = ti.insertViews; const removeStart = ti.removeStart; - + let view; let destroyQueue: ViewController[] = []; + // there are views to remove if (isPresent(removeStart)) { for (var i = 0; i < ti.removeCount; i++) { - destroyQueue.push(this._views[i + removeStart]); + view = this._views[i + removeStart]; + assert(view, 'internal error, view in destroyQueue can not be null'); + if (view && view !== enteringView && view !== leavingView) { + destroyQueue.push(view); + } } // default the direction to "back" opts.direction = opts.direction || DIRECTION_BACK; } + // there are views to insert if (insertViews) { // manually set the new view's id if an id was passed in the options if (isPresent(opts.id)) { @@ -346,25 +356,8 @@ export class NavControllerBase extends Ion implements NavController { // add the views to the for (var i = 0; i < insertViews.length; i++) { - var view = insertViews[i]; - - var existingIndex = this._views.indexOf(view); - if (existingIndex > -1) { - // this view is already in the stack!! - // move it to its new location - this._views.splice(ti.insertStart + i, 0, this._views.splice(existingIndex, 1)[0]); - - } else { - // this is a new view to add to the stack - // create the new entering view - view._setNav(this); - - // give this inserted view an ID - view.id = this.id + '-' + (++this._ids); - - // insert the entering view into the correct index in the stack - this._views.splice(ti.insertStart + i, 0, view); - } + view = insertViews[i]; + this._insertViewAt(view, ti.insertStart + i); } if (ti.enteringRequiresTransition) { @@ -372,27 +365,20 @@ export class NavControllerBase extends Ion implements NavController { opts.direction = opts.direction || DIRECTION_FORWARD; } } - // if the views to be removed are in the beginning or middle // and there is not a view that needs to visually transition out // then just destroy them and don't transition anything - for (var i = 0; i < destroyQueue.length; i++) { - // batch all of lifecycles together - var view = destroyQueue[i]; - if (view && view !== enteringView && view !== leavingView) { - this._willLeave(view); - this._didLeave(view); - this._willUnload(view); - } + // batch all of lifecycles together + for (view of destroyQueue) { + this._willLeave(view); + this._didLeave(view); + this._willUnload(view); } - for (var i = 0; i < destroyQueue.length; i++) { - // batch all of the destroys together - var view = destroyQueue[i]; - if (view && view !== enteringView && view !== leavingView) { - view._destroy(this._renderer); - } + + // once all lifecycle events has been delivered, we can safely detroy the views + for (view of destroyQueue) { + this._destroyView(view); } - destroyQueue.length = 0; if (ti.enteringRequiresTransition || ti.leavingRequiresTransition && enteringView !== leavingView) { // set which animation it should use if it wasn't set yet @@ -434,6 +420,27 @@ export class NavControllerBase extends Ion implements NavController { this._willLoad(enteringView); } + _viewAttachToDOM(view: ViewController, componentRef: ComponentRef, viewport: ViewContainerRef) { + // successfully finished loading the entering view + // fire off the "didLoad" lifecycle events + this._didLoad(view); + + // render the component ref instance to the DOM + // ******** DOM WRITE **************** + viewport.insert(componentRef.hostView, viewport.length); + view._state = ViewState.PRE_RENDERED; + + if (view._cssClass) { + // the ElementRef of the actual ion-page created + var pageElement = componentRef.location.nativeElement; + + // ******** DOM WRITE **************** + this._renderer.setElementClass(pageElement, view._cssClass, true); + } + + componentRef.changeDetectorRef.detectChanges(); + } + _viewTest(enteringView: ViewController, leavingView: ViewController, ti: TransitionInstruction) { const promises: Promise[] = []; const reject = ti.reject; @@ -442,47 +449,38 @@ export class NavControllerBase extends Ion implements NavController { if (leavingView) { const leavingTestResult = leavingView._lifecycleTest('Leave'); - if (isPresent(leavingTestResult) && leavingTestResult !== true) { - if (leavingTestResult instanceof Promise) { - // async promise - promises.push(leavingTestResult); - - } else { - // synchronous reject - reject((leavingTestResult !== false ? leavingTestResult : `ionViewCanLeave rejected`)); - return false; - } + if (leavingTestResult === false) { + // synchronous reject + reject((leavingTestResult !== false ? leavingTestResult : `ionViewCanLeave rejected`)); + return false; + } else if (leavingTestResult instanceof Promise) { + // async promise + promises.push(leavingTestResult); } } if (enteringView) { const enteringTestResult = enteringView._lifecycleTest('Enter'); - if (isPresent(enteringTestResult) && enteringTestResult !== true) { - if (enteringTestResult instanceof Promise) { - // async promise - promises.push(enteringTestResult); - - } else { - // synchronous reject - reject((enteringTestResult !== false ? enteringTestResult : `ionViewCanEnter rejected`)); - return false; - } + if (enteringTestResult === false) { + // synchronous reject + reject((enteringTestResult !== false ? enteringTestResult : `ionViewCanEnter rejected`)); + return false; + } else if (enteringTestResult instanceof Promise) { + // async promise + promises.push(enteringTestResult); } } if (promises.length) { // darn, async promises, gotta wait for them to resolve - Promise.all(promises).then(() => { - // all promises resolved! let's continue - this._postViewInit(enteringView, leavingView, ti, resolve); - - }).catch(reject); - return true; + Promise.all(promises) + .then(() => this._postViewInit(enteringView, leavingView, ti, resolve)) + .catch(reject); + } else { + // synchronous and all tests passed! let's move on already + this._postViewInit(enteringView, leavingView, ti, resolve); } - - // synchronous and all tests passed! let's move on already - this._postViewInit(enteringView, leavingView, ti, resolve); return true; } @@ -508,27 +506,21 @@ export class NavControllerBase extends Ion implements NavController { // create the transition animation from the TransitionController // this will either create the root transition, or add it as a child transition - const trns = this._trnsCtrl.get(this._trnsId, enteringView, leavingView, animationOpts); + const transition = this._trnsCtrl.get(this._trnsId, enteringView, leavingView, animationOpts); // ensure any swipeback transitions are cleared out this._sbTrns && this._sbTrns.destroy(); + this._sbTrns = null; - if (trns.parent) { - // this is important for later to know if there - // are any more child tests to check for - trns.parent.hasChildTrns = true; - - } else { - // this is the root transition - if (opts.progressAnimation) { - this._sbTrns = trns; - } + // swipe to go back root transition + if (transition.isRoot() && opts.progressAnimation) { + this._sbTrns = transition; } - trns.registerStart(() => { - this._trnsStart(trns, enteringView, leavingView, opts, resolve); - if (trns.parent) { - trns.parent.start(); + transition.registerStart(() => { + this._trnsStart(transition, enteringView, leavingView, opts, resolve); + if (transition.parent) { + transition.parent.start(); } }); @@ -537,37 +529,16 @@ export class NavControllerBase extends Ion implements NavController { // this would also render new child navs/views // which may have their very own async canEnter/Leave tests // ******** DOM WRITE **************** - this._viewInsert(enteringView, enteringView._cmp, this._viewport); + this._viewAttachToDOM(enteringView, enteringView._cmp, this._viewport); } - if (!trns.hasChildTrns) { + if (!transition.hasChildren) { // lowest level transition, so kick it off and let it bubble up to start all of them - trns.start(); + transition.start(); } } - _viewInsert(view: ViewController, componentRef: ComponentRef, viewport: ViewContainerRef) { - // successfully finished loading the entering view - // fire off the "didLoad" lifecycle events - this._didLoad(view); - - // render the component ref instance to the DOM - // ******** DOM WRITE **************** - viewport.insert(componentRef.hostView, viewport.length); - view._state = ViewState.PRE_RENDERED; - - if (view._cssClass) { - // the ElementRef of the actual ion-page created - var pageElement = componentRef.location.nativeElement; - - // ******** DOM WRITE **************** - this._renderer.setElementClass(pageElement, view._cssClass, true); - } - - componentRef.changeDetectorRef.detectChanges(); - } - - _trnsStart(trns: Transition, enteringView: ViewController, leavingView: ViewController, opts: NavOptions, resolve: TransitionResolveFn) { + _trnsStart(transition: Transition, enteringView: ViewController, leavingView: ViewController, opts: NavOptions, resolve: TransitionResolveFn) { this._trnsId = null; // set the correct zIndex for the entering and leaving views @@ -578,43 +549,46 @@ export class NavControllerBase extends Ion implements NavController { // ******** DOM WRITE **************** enteringView && enteringView._domShow(true, this._renderer); - if (leavingView) { - // always ensure the leaving view is viewable - // ******** DOM WRITE **************** - leavingView._domShow(true, this._renderer); - } + // always ensure the leaving view is viewable + // ******** DOM WRITE **************** + leavingView && leavingView._domShow(true, this._renderer); // initialize the transition - trns.init(); + transition.init(); - if ((!this._init && this._views.length === 1 && !this._isPortal) || this.config.get('animate') === false) { - // the initial load shouldn't animate, unless it's a portal + // we should animate (duration > 0) if the pushed page is not the first one (startup) + // or if it is a portal (modal, actionsheet, etc.) + let isFirstPage = !this._init && this._views.length === 1; + let shouldNotAnimate = isFirstPage && !this._isPortal; + let canNotAnimate = this.config.get('animate') === false; + if (shouldNotAnimate || canNotAnimate) { opts.animate = false; } + if (opts.animate === false) { // if it was somehow set to not animation, then make the duration zero - trns.duration(0); + transition.duration(0); } // create a callback that needs to run within zone // that will fire off the willEnter/Leave lifecycle events at the right time - trns.beforeAddRead(() => { + transition.beforeAddRead(() => { this._zone.run(this._viewsWillLifecycles.bind(this, enteringView, leavingView)); }); // create a callback for when the animation is done - trns.onFinish(() => { + transition.onFinish(() => { // transition animation has ended - this._zone.run(this._trnsFinish.bind(this, trns, opts, resolve)); + this._zone.run(this._trnsFinish.bind(this, transition, opts, resolve)); }); // get the set duration of this transition - const duration = trns.getDuration(); + const duration = transition.getDuration(); // set that this nav is actively transitioning this.setTransitioning(true, duration); - if (!trns.parent) { + if (transition.isRoot()) { // this is the top most, or only active transition, so disable the app // add XXms to the duration the app is disabled when the keyboard is open @@ -622,19 +596,21 @@ export class NavControllerBase extends Ion implements NavController { // if this transition has a duration and this is the root transition // then set that the app is actively disabled this._app.setEnabled(false, duration + ACTIVE_TRANSITION_OFFSET); + } else { + console.debug('transition is running but app has not been disabled'); } // cool, let's do this, start the transition if (opts.progressAnimation) { // this is a swipe to go back, just get the transition progress ready // kick off the swipe animation start - trns.progressStart(); + transition.progressStart(); } else { // only the top level transition should actually start "play" // kick it off and let it play through // ******** DOM WRITE **************** - trns.play(); + transition.play(); } } } @@ -645,32 +621,30 @@ export class NavControllerBase extends Ion implements NavController { leavingView && this._willLeave(leavingView); } - _trnsFinish(trns: Transition, opts: NavOptions, resolve: TransitionResolveFn) { - const hasCompleted = trns.hasCompleted; - + _trnsFinish(transition: Transition, opts: NavOptions, resolve: TransitionResolveFn) { // mainly for testing let enteringName: string; let leavingName: string; - if (hasCompleted) { + if (transition.hasCompleted) { // transition has completed (went from 0 to 1) - if (trns.enteringView) { - enteringName = trns.enteringView.name; - this._didEnter(trns.enteringView); + if (transition.enteringView) { + enteringName = transition.enteringView.name; + this._didEnter(transition.enteringView); } - if (trns.leavingView) { - leavingName = trns.leavingView.name; - this._didLeave(trns.leavingView); + if (transition.leavingView) { + leavingName = transition.leavingView.name; + this._didLeave(transition.leavingView); } - this._cleanup(trns.enteringView); + this._cleanup(transition.enteringView); } - if (!trns.parent) { + if (transition.isRoot()) { // this is the root transition // it's save to destroy this transition - this._trnsCtrl.destroy(trns.trnsId); + this._trnsCtrl.destroy(transition.trnsId); // it's safe to enable the app again this._app.setEnabled(true); @@ -681,7 +655,7 @@ export class NavControllerBase extends Ion implements NavController { this._linker.navChange(opts.direction); } - if (opts.keyboardClose !== false && this._keyboard.isOpen()) { + if (opts.keyboardClose !== false) { // the keyboard is still open! // no problem, let's just close for them this._keyboard.close(); @@ -689,7 +663,40 @@ export class NavControllerBase extends Ion implements NavController { } // congrats, we did it! - resolve(hasCompleted, true, enteringName, leavingName, opts.direction); + resolve(transition.hasCompleted, true, enteringName, leavingName, opts.direction); + } + + _insertViewAt(view: ViewController, index: number) { + var existingIndex = this._views.indexOf(view); + if (existingIndex > -1) { + // this view is already in the stack!! + // move it to its new location + this._views.splice(index, 0, this._views.splice(existingIndex, 1)[0]); + + } else { + // this is a new view to add to the stack + // create the new entering view + view._setNav(this); + + // give this inserted view an ID + view.id = this.id + '-' + (++this._ids); + + // insert the entering view into the correct index in the stack + this._views.splice(index, 0, view); + } + } + + _removeView(view: ViewController) { + const views = this._views; + const index = views.indexOf(view); + if (index > -1) { + views.splice(index, 1); + } + } + + _destroyView(view: ViewController) { + view._destroy(this._renderer); + this._removeView(view); } /** @@ -707,7 +714,7 @@ export class NavControllerBase extends Ion implements NavController { // this view comes after the active view // let's unload it this._willUnload(view); - view._destroy(this._renderer); + this._destroyView(view); } else if (i < activeViewIndex && !this._isPortal) { // this view comes before the active view @@ -800,9 +807,10 @@ export class NavControllerBase extends Ion implements NavController { } destroy() { - for (var i = this._views.length - 1; i >= 0; i--) { - this._views[i]._willUnload(); - this._views[i]._destroy(this._renderer); + let view; + for (view of this._views) { + view._willUnload(); + view._destroy(this._renderer); } this._views.length = 0; diff --git a/src/navigation/view-controller.ts b/src/navigation/view-controller.ts index 32e245cc3a..6b8613fefc 100644 --- a/src/navigation/view-controller.ts +++ b/src/navigation/view-controller.ts @@ -528,26 +528,25 @@ export class ViewController { this._cmp.destroy(); } - if (this._nav) { - // remove it from the nav - const index = this._nav.indexOf(this); - if (index > -1) { - this._nav._views.splice(index, 1); - } - } - this._nav = this._cmp = this.instance = this._cntDir = this._cntRef = this._hdrDir = this._ftrDir = this._nb = this._onWillDismiss = null; } /** * @private */ - _lifecycleTest(lifecycle: string): boolean | string | Promise { + _lifecycleTest(lifecycle: string): boolean | Promise { let instance = this.instance; let methodName = 'ionViewCan' + lifecycle; if (instance && instance[methodName]) { try { - return instance[methodName](); + let result = instance[methodName](); + if (result === false) { + return false; + } else if (result instanceof Promise) { + return result; + } else { + return true; + } } catch (e) { console.error(`${this.name} ${methodName} error: ${e.message}`); @@ -572,7 +571,6 @@ export class ViewController { } - export function isViewController(viewCtrl: any) { return !!(viewCtrl && (viewCtrl)._didLoad && (viewCtrl)._willUnload); } diff --git a/src/transitions/transition-controller.ts b/src/transitions/transition-controller.ts index d1c150a180..a542fb0fe8 100644 --- a/src/transitions/transition-controller.ts +++ b/src/transitions/transition-controller.ts @@ -34,7 +34,7 @@ export class TransitionController { return this._ids++; } - get(trnsId: number, enteringView: ViewController, leavingView: ViewController, opts: AnimationOptions) { + get(trnsId: number, enteringView: ViewController, leavingView: ViewController, opts: AnimationOptions): Transition { const trns = createTransition(this._config, opts.animation, enteringView, leavingView, opts); trns.trnsId = trnsId; diff --git a/src/transitions/transition-registry.ts b/src/transitions/transition-registry.ts index a692944193..b14fc35eb5 100644 --- a/src/transitions/transition-registry.ts +++ b/src/transitions/transition-registry.ts @@ -1,4 +1,5 @@ import { Config } from '../config/config'; +import { Transition } from './transition'; import { IOSTransition } from './transition-ios'; import { MDTransition } from './transition-md'; import { WPTransition } from './transition-wp'; @@ -62,7 +63,7 @@ export function registerTransitions(config: Config) { } -export function createTransition(config: Config, transitionName: string, enteringView: any, leavingView: any, opts: any) { +export function createTransition(config: Config, transitionName: string, enteringView: any, leavingView: any, opts: any): Transition { let TransitionClass: any = config.getTransition(transitionName); if (!TransitionClass) { // didn't find a transition animation, default to ios-transition diff --git a/src/transitions/transition.ts b/src/transitions/transition.ts index 4ac599e3a5..2d661a7481 100644 --- a/src/transitions/transition.ts +++ b/src/transitions/transition.ts @@ -21,10 +21,8 @@ export class Transition extends Animation { _trnsStart: Function; parent: Transition; - hasChildTrns: boolean = false; trnsId: number; - constructor(public enteringView: ViewController, public leavingView: ViewController, opts: AnimationOptions, raf?: Function) { super(null, opts, raf); } @@ -35,6 +33,10 @@ export class Transition extends Animation { this._trnsStart = trnsStart; } + isRoot(): boolean { + return !this.parent; + } + start() { this._trnsStart && this._trnsStart(); this._trnsStart = null; diff --git a/src/util/mock-providers.ts b/src/util/mock-providers.ts index b11cc62a2e..af0c83d97b 100644 --- a/src/util/mock-providers.ts +++ b/src/util/mock-providers.ts @@ -275,9 +275,9 @@ export const mockNavController = function(): NavControllerBase { enteringView._state = ViewState.INITIALIZED; }; - (nav)._orgViewInsert = nav._viewInsert; + (nav)._orgViewInsert = nav._viewAttachToDOM; - nav._viewInsert = function(view: ViewController, componentRef: ComponentRef, viewport: ViewContainerRef) { + nav._viewAttachToDOM = function(view: ViewController, componentRef: ComponentRef, viewport: ViewContainerRef) { let mockedViewport: any = { insert: () => { } };