Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for listener bug #620

Merged
merged 1 commit into from
Jan 3, 2018
Merged

Fix for listener bug #620

merged 1 commit into from
Jan 3, 2018

Conversation

devotaaabel
Copy link
Contributor

One fix to consider for preventing inability to add listeners to dispatcher before it's initialized is initializing it earlier. Whether this fix is adopted or not, I think we need to add more information to the documentation about when the user can safely add listeners and start navigation.

cc. @danesfeder @ericrwolfe

Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

@devotaaabel I think initializing the dispatcher earlier is a good idea as it doesn't require any parameters.

Because we changed the way we add the listeners (packed in NavigationViewOptions), a crash as a result from trying to add a listener to a null dispatcher should be prevented.

But it would probably be a good idea to add some documentation or a runtime exception to ensure NavigationView#startNavigation(NavigationViewOptions options) isn't called until the onNavigationReadyCallback.onNavigationReady() callback fires.

…atcher before it's initialized is initializing it earlier.
@danesfeder danesfeder force-pushed the devota-expose-listeners-bug branch from c04605d to bfe9152 Compare January 3, 2018 17:25
@devotaaabel
Copy link
Contributor Author

@danesfeder Those are probably both good ideas (adding documentation and a runtime exception).

@devotaaabel devotaaabel merged commit 72d32ea into master Jan 3, 2018
@devotaaabel devotaaabel deleted the devota-expose-listeners-bug branch January 3, 2018 19:32
@danesfeder danesfeder mentioned this pull request Jan 23, 2018
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants