Skip to content

Conversation

@danesfeder
Copy link
Contributor

Closes #920

We were previously waiting to pass the NavigationViewEventDispatcher from the NavigationView to the NavigationViewModel when navigation is initialized. When can do this much sooner though, right when the NavigationView is inflated. This also removes the need to set it to null in the view model to prevent a leak. 🎉

@danesfeder danesfeder added bug Defect to be fixed. navigation-ui labels May 15, 2018
@danesfeder danesfeder added this to the 0.14.0 milestone May 15, 2018
@danesfeder danesfeder self-assigned this May 15, 2018
@danesfeder danesfeder force-pushed the dan-npe-dispatcher branch from ae2b8ce to 4caac9f Compare May 15, 2018 15:47
Copy link
Contributor

@devotaaabel devotaaabel left a comment

Choose a reason for hiding this comment

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

How does this prevent the need for setting it to null to avoid leaks?

@danesfeder
Copy link
Contributor Author

@devotaaabel the NavigationViewModel survives rotation, but the NavigationView itself is destroyed and recreated, so setting the dispatcher to null originally prevented a leak because the view model would no longer hold onto the old reference of the dispatcher. With this PR, the viewmodel dispatcher reference is updated immediately, as soon as the NavigationView is created, so no more leaks

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Minor comment not blocking the PR though.

inflate(getContext(), R.layout.navigation_view_layout, this);
bind();
initViewModels();
initNavigationViewModel();
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, could we rename init methods to initialize? Saw that we're using initialize nomenclature in navigationViewModel for example.

@danesfeder danesfeder force-pushed the dan-npe-dispatcher branch 2 times, most recently from 20ca972 to 472cef7 Compare May 17, 2018 14:18
@danesfeder danesfeder force-pushed the dan-npe-dispatcher branch from 472cef7 to 85f5a3e Compare May 17, 2018 14:28
@danesfeder danesfeder merged commit 9051b98 into master May 17, 2018
@danesfeder danesfeder deleted the dan-npe-dispatcher branch May 17, 2018 14:46
This was referenced May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Defect to be fixed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants