-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add call to onDataChanged when adding first listener #736
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
Conversation
@SUPERCILEX I agree with this one but just curious, what was the bug you ran into? |
@samtstern Lol, as I was writing about my bug, I realized this PR introduced another bug. For my bug, it was just something in my app. Basically, I'm trying to cleanup my "no content hint" logic and I wasn't getting the first As for this PR, I realized that |
|
||
// Reset mHasDataChanged if there are no more listeners | ||
if (!isListening()) { | ||
mHasDataChanged = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is a little surprising to me. Wouldn't you want the first listener attached to still fire immediately if the FirebaseArray contains data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samtstern Yeah, that still happens. This just ensures that adding the first listener is the same whether or not this class was first created or is being reused. The contract for remove
ChangeEventListener
is that the list must be cleared when there are no more listeners, as if the class was first created. Here's the FirebaseArray
implementation:
// Clear data when all listeners are removed
if (!isListening()) {
mQuery.removeEventListener((ValueEventListener) this);
mQuery.removeEventListener((ChildEventListener) this);
clearData();
}
@samtstern Would you like some more clarification on what's going on? Here are the cases I'm trying to solve:
|
Thanks for explaining! SGTM. |
@samtstern This one should really go in before 2.0 is released.
Basically, I got a weird bug because of this and we should be doing it anyway for consistency's sake.