Batch updates and empty lists #477
Conversation
…es the touch listener less dependent on the demo app.
There was a problem hiding this comment.
Approving with one nit, but wait for @samtstern's approval before merging.
Also, question: For batching updates, why provide an onReady to mark the end of database changes but not an equivalent method to indicate the start of database changes?
| sendCurrentMessage(); | ||
| } | ||
| }); | ||
| // Allow hitting the Send keyon the soft keyboard to send the message |
There was a problem hiding this comment.
nit: space between 'key' and 'on'
…t it can be moved elsewhere. Renamed ChatTouchListener to RecyclerSwipeListener. Encapsulated all callbacks into an interface, that is now implemented by the ChatActivity. This also removed the need for the swipe listener to have access to the adapter. Correctly reset the ViewHolder subview positions when an item is deleted.
|
Hey @puf, A mcve with plain old |
|
Great find @SUPERCILEX . Agreed on holding off on merging until we figure this one out. Unfortunately this is part of the behavior of the underlying SDK and not something we can change in FirebaseUI. It is not even something we can fix in the SDK, at least not without reworking a large part of the API surface. On the lower level we have no knowledge whether the ValueListener is listening for a single document (where we have a hard guarantee that we will never fire with incomplete results) or it is listening on a collection (where incomplete results are acceptable, as long as each individual child is complete). Can you tell support to ping me on the case? |
|
@puf Yes, I told support to ping you. @puf That's a real bummer for the internal api, but I still have hope because I've been using a hack to get around this problem in my app. In my mvce, if you ask the Here's what I mean: // 1. While online, add a listener to the root ref:
mRef.addValueEventListener(this);
// 2. Go back to querying and it will work offline and online:
mRef.orderByValue().equalTo(1).addValueEventListener(this);@puf Couldn't you guys do some sort of cache hack where if a query is executed offline and returns no results, you guys query the parent ref and use a for loop with |
|
@SUPERCILEX: I'm against workarounds for behavior that is missing in the
Firebase SDK. Those types of workarounds tend to become a maintenance
problem as the SDK evolves.
…On Wed, Jan 4, 2017 at 12:15 PM Alex Saveau ***@***.***> wrote:
@puf <https://github.com/puf> Yes, I told support to ping you.
@puf <https://github.com/puf> That's a real bummer for the internal api,
but I still have hope because I've been using a hack to get around this
problem in my app. In my mvce, if you ask the ValueEventListener for the
parent database instead of the query once while online, that entire ref is
cached and queries will work offline. From what you explained above,
caching the parent ref guarantees complete results which is why it works.
Here's what I mean:
// 1. While online, add a listener to the root ref:
mRef.addValueEventListener(this);// 2. Go back to querying and it will work offline and online:
mRef.orderByValue().equalTo(1).addValueEventListener(this);
@puf <https://github.com/puf> Couldn't you guys do some sort of cache
hack where if a query is executed offline and returns no results, you guys
query the parent ref and use a for loop with getChildren to filter the
results and return the proper query? If that doesn't work for the native
Firebase apis, couldn't we use a similar hack for FirebaseArray?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#477 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA3w36gc9QPy3mQzXcpBl1q5AUEoZ93Cks5rO_3OgaJpZM4LWiMt>
.
|
|
@puf sounds reasonable so we just wait for you guys to fix it on the SDK side. |
|
That is indeed my take. But this one likely won't be fixed soon, given that
it is inherent to the database's model: it's just a JSON tree. On the wire
we have no way to distinguish collections (where partial results are
acceptable and wanted) from "documents" (where we guarantee to never raise
partial results).
…On Sun, Jan 8, 2017 at 10:31 PM Alex Saveau ***@***.***> wrote:
@puf <https://github.com/puf> sounds reasonable so we just wait for you
guys to fix it on the SDK side.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#477 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA3w37QxrO2-3VusZIY97XfRxx5Wd3vuks5rQdRAgaJpZM4LWiMt>
.
|
|
I am going to start doing QA on version |
|
@puf going to close this. Here are the changes in this PR, as I see it:
We've already implemented the former (with your help) and the latter is really cool but not critical to the library. Since this has been open for many months and is currently not mergeable there's not much to do with it. If you ever get some extra time (unlikely, I know) and want to get the app changes merge-able again then I'm happy to make these changes. |
Fixes #458 and #118. Ready for a first review, but don't merge yet.