Skip to content

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

Merged
merged 3 commits into from
Jun 8, 2017
Merged

Add call to onDataChanged when adding first listener #736

merged 3 commits into from
Jun 8, 2017

Conversation

SUPERCILEX
Copy link
Collaborator

@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.

@samtstern
Copy link
Contributor

@SUPERCILEX I agree with this one but just curious, what was the bug you ran into?

@SUPERCILEX
Copy link
Collaborator Author

@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 onDataChanged callback because I was adding my listener to a FirebaseArray that was already live. (So my no content hint wouldn't show up even though there were no items.)

As for this PR, I realized that onDataChanged would always be called right away when adding the first listener regardless of whether or not any data had changed. I then considered checking if we had been listening for changes and only send the callback if it wasn't the first listener, but that doesn't scale to the second, third, or n + 1 listener. So instead, the solution is slightly more messy: we only send an onDataChanged event if one has already occurred.


// Reset mHasDataChanged if there are no more listeners
if (!isListening()) {
mHasDataChanged = false;
Copy link
Contributor

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?

Copy link
Collaborator Author

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 removeChangeEventListener 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();
}

@SUPERCILEX
Copy link
Collaborator Author

@samtstern Would you like some more clarification on what's going on? Here are the cases I'm trying to solve:

  1. First listener is added, shouldn't get any callbacks
  2. n + 1 listener is added, should get the current items to catch up with the other listeners, but not onDataChanged unless the other listeners already received that call
  3. n + 1 listener is added after a full cycle of adding items and calling onDataChanged. In this case, we play catch up and call the items added method and onDataChanged.
  4. All listeners are reset: everything should go back to being in the same state as if the class was first loaded.

@samtstern
Copy link
Contributor

Thanks for explaining! SGTM.

@samtstern samtstern merged commit 6eba74c into firebase:version-2.0.0-dev Jun 8, 2017
@SUPERCILEX SUPERCILEX deleted the patch-1 branch June 8, 2017 16:56
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