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

Prevent NavigationNotification update after unregistered #1118

Merged
merged 1 commit into from
Jul 19, 2018

Conversation

danesfeder
Copy link
Contributor

@danesfeder danesfeder commented Jul 18, 2018

Closes #1116 Regression from #1066

Added a boolean to prevent notification updates after the notification has been shut down.

@Guardiola31337
Copy link
Contributor

@danesfeder this would only fix the first crash reported in #1116 right?
If that's correct, should we create a new ticket for the second crash? Noting because you added Closes #1116 and that would close the issue completely.

@Guardiola31337
Copy link
Contributor

should we create a new ticket for the second crash?

I've just 👀 #1116 (comment) Thanks @danesfeder 🙏

@Guardiola31337
Copy link
Contributor

I've just 👀 #1116 (comment)

Tracking in #1119

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.

Added a minor comment (not blocking the PR).

Thanks @danesfeder

@@ -8,6 +8,7 @@
class NavigationNotificationProvider {

private NavigationNotification navigationNotification;
private boolean isUpdating = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT - what about shouldUpdate instead? It sounds better to me. By default, shouldUpdate? Yes - true After shutting down, shouldUpdate? No - false
What do you think?

@danesfeder danesfeder merged commit 1678aa0 into master Jul 19, 2018
@danesfeder danesfeder deleted the dan-notification-register branch July 19, 2018 15:38
@danesfeder danesfeder mentioned this pull request Jul 20, 2018
11 tasks
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.

2 participants