Skip to content

Fix NavigationViewController crash in storyboard.#2974

Merged
MaximAlien merged 7 commits into
release-v2.0from
maxim/2973-fix-navigation-view-controller-crash-in-storyboard
May 27, 2021
Merged

Fix NavigationViewController crash in storyboard.#2974
MaximAlien merged 7 commits into
release-v2.0from
maxim/2973-fix-navigation-view-controller-crash-in-storyboard

Conversation

@MaximAlien
Copy link
Copy Markdown
Contributor

Description

Fixes #2973.

Implementation

Since it's not possible to call other initializers in NavigationViewController.init(coder:) new public NavigationViewController.initialize(for:routeIndex:routeOptions:navigationOptions:) method was introduced. It's meant to be called right after NavigationViewController instantiation in storyboard. I've also brought back NavigationViewController to Navigation.storyboard to give the ability to create it using this guide.

@MaximAlien MaximAlien added bug Something isn’t working UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. labels May 4, 2021
@MaximAlien MaximAlien added this to the v2.0.0 (GA) milestone May 4, 2021
@MaximAlien MaximAlien requested a review from a team May 4, 2021 09:56
@MaximAlien MaximAlien self-assigned this May 4, 2021
required public init(for route: Route, routeIndex: Int, routeOptions: RouteOptions, navigationOptions: NavigationOptions? = nil) {
super.init(nibName: nil, bundle: nil)

initialize(for: route, routeIndex: routeIndex, routeOptions: routeOptions, navigationOptions: navigationOptions)
Copy link
Copy Markdown
Contributor

@azarovalex azarovalex May 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected the fix to be in required public init?(coder aDecoder: NSCoder), does it work without this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll not be possible to provide needed information in required public init?(coder aDecoder: NSCoder). E.g. route object should already be available before calling that initializer, which doesn't seem to be possible. Seems that the only way is to initialize all needed objects afterwards and only after that present NavigationViewController.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example in this guide shows how new usage can look like.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point now, the problem is that if a user forgets to call initialize method the app will crash. 😿
But I guess it's the best we can do, hope this problem will be noticeable during the development process.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. route object should already be available before calling that initializer, which doesn't seem to be possible.

The separate initialize method is really unfortunate. Its name points to a larger problem: most of the stuff that has been happening in the initializer only needs to take place by the time the view controller is presented, right? Ideally we’d move it all into viewDidLoad(). As it is, developers using NavigationViewController programmatically have to be careful about prematurely initializing the object. Developers don’t expect initializing a view controller or view to have side effects. I guess the tradeoff is that delaying these steps until viewDidLoad() would force us to think about situations where navigationService etc. are nil, so we’d have to revisit a bunch of implicitly unwrapped optionals.

I guess I’m OK with this approach as a stopgap fix, but in that case we should think about a broader lifecycle cleanup as tail work, with an eye towards deprecating the new initialize method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that additional initializer is not an optimal solution. I've changed implementation to use optional parameters for route, routeOptions routeIndex etc. User will be able to change it before NavigationViewController presentation. Example can be found here.

Copy link
Copy Markdown
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changelog should mention that developers can once again segue to Navigation.storyboard or add NavigationViewController to a storyboard, and that prepare(for:sender:) needs to call NavigationViewController.initialize(for:routeIndex:routeOptions:navigationOptions:).

required public init(for route: Route, routeIndex: Int, routeOptions: RouteOptions, navigationOptions: NavigationOptions? = nil) {
super.init(nibName: nil, bundle: nil)

initialize(for: route, routeIndex: routeIndex, routeOptions: routeOptions, navigationOptions: navigationOptions)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. route object should already be available before calling that initializer, which doesn't seem to be possible.

The separate initialize method is really unfortunate. Its name points to a larger problem: most of the stuff that has been happening in the initializer only needs to take place by the time the view controller is presented, right? Ideally we’d move it all into viewDidLoad(). As it is, developers using NavigationViewController programmatically have to be careful about prematurely initializing the object. Developers don’t expect initializing a view controller or view to have side effects. I guess the tradeoff is that delaying these steps until viewDidLoad() would force us to think about situations where navigationService etc. are nil, so we’d have to revisit a bunch of implicitly unwrapped optionals.

I guess I’m OK with this approach as a stopgap fix, but in that case we should think about a broader lifecycle cleanup as tail work, with an eye towards deprecating the new initialize method.


/**
Initializes a navigation view controller that presents the user interface for following a predefined route based on the given options.
Initializes a `NavigationViewController` that presents the user interface for following a predefined route based on the given options.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, it’s OK to refer to the thing the class represents rather than the symbol. There’s no ambiguity because the developer would read this documentation comment in the context of the NavigationViewController class reference already.

@MaximAlien MaximAlien force-pushed the maxim/2973-fix-navigation-view-controller-crash-in-storyboard branch 2 times, most recently from 6c9a4cf to 426df36 Compare May 19, 2021 11:33
@MaximAlien MaximAlien force-pushed the maxim/2973-fix-navigation-view-controller-crash-in-storyboard branch 3 times, most recently from f21efec to 83a5ca8 Compare May 25, 2021 17:02
@MaximAlien MaximAlien force-pushed the maxim/2973-fix-navigation-view-controller-crash-in-storyboard branch from 83a5ca8 to 5311bab Compare May 26, 2021 11:58
@MaximAlien MaximAlien merged commit c1dac1c into release-v2.0 May 27, 2021
@MaximAlien MaximAlien deleted the maxim/2973-fix-navigation-view-controller-crash-in-storyboard branch May 27, 2021 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn’t working UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants