Skip to content

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