Support for joins #276

SUPERCILEX · 2016-09-01T02:13:35Z

If this has been discussed in an issue, make sure to mention the issue number here. If not, go file an issue about this to make sure this is a desirable change.
- Issues Allow retrieving data from a reference of keys in FirebaseArray #243 and Support for joins #12
If this is a new feature please co-ordinate with someone on FirebaseUI-iOS to make sure that we can implement this on both platforms and maintain feature parity.
- I don't know objective C or swift. Anyone want to do this?

TODO:

tests

# Conflicts: # database/src/main/java/com/firebase/ui/database/FirebaseArray.java # database/src/main/java/com/firebase/ui/database/FirebaseListAdapter.java # database/src/main/java/com/firebase/ui/database/FirebaseRecyclerAdapter.java

giautm · 2016-09-04T15:46:18Z

I'm waiting for this deloy. 👍

…se/master

test

puf · 2016-09-07T17:26:38Z

Hey Alex. Thanks for the PR. Looks great, definitely a very clean implementation of attaching and detaching the listeners. I also like that it uses the data model from the Firebase documentation as its basis.

I don't think we should add this code to the basic FirebaseXxxAdapters though. Scanning your code structure I see many if (condition) oldBehavior() else newBehavior(), which would be cleaner to catch in a subclass. That would also keep the current classes as simple as we intentionally kept them and allows us to later add support for other join structures without breaking backwards compatibility of the basic adapters.

Can you see if you can isolate the behavior in separate sub-adapters?

Pull latest changes

SUPERCILEX · 2016-09-10T05:33:52Z

database/src/main/java/com/firebase/ui/database/FirebaseArray.java

    public DataSnapshot getItem(int index) {
        return mSnapshots.get(index);
    }

+    protected DataSnapshot internalGetItem(int index) {


This is completely gross IMO, but I don't know how to get rid of it. Any feedback is welcome!

Agreed. I'll brood on a better way to do this.

I found a bug here. When you remove the last element of the list, remove trigger events before and list have no element. So it would make an exception:

java.lang.IndexOutOfBoundsException: Invalid index 0, size is 0 at java.util.ArrayList.throwIndexOutOfBoundsException(ArrayList.java:255) at java.util.ArrayList.get(ArrayList.java:308) at com.firebase.ui.database.FirebaseArray.internalGetItem(FirebaseArray.java:61) at com.firebase.ui.database.FirebaseIndexArray.notifyChangedListeners(FirebaseIndexArray.java:119) at com.firebase.ui.database.FirebaseArray.notifyChangedListeners(FirebaseArray.java:119) at com.firebase.ui.database.FirebaseArray.onChildRemoved(FirebaseArray.java:100) at com.firebase.ui.database.FirebaseIndexArray.onChildRemoved(FirebaseIndexArray.java:29) at com.google.android.gms.internal.zzahe.zza(Unknown Source) at com.google.android.gms.internal.zzaje.zzcta(Unknown Source) at com.google.android.gms.internal.zzajh$1.run(Unknown Source) at android.os.Handler.handleCallback(Handler.java:739) at android.os.Handler.dispatchMessage(Handler.java:95) at android.os.Looper.loop(Looper.java:155) at android.app.ActivityThread.main(ActivityThread.java:5727) at java.lang.reflect.Method.invoke(Native Method) at java.lang.reflect.Method.invoke(Method.java:372) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1029) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:824)

because mSnapshots.remove(index) was call before notifyChangedListeners

@Override public void onChildRemoved(DataSnapshot snapshot) { int index = getIndexForKey(snapshot.getKey()); mSnapshots.remove(index); notifyChangedListeners(OnChangedListener.EventType.Removed, index); }

More seriously, your code is removed mistakenly listener. :(

Thanks for catching that!!! I completely messed up removing an item: mSnapshots.remove(index) wasn't even getting called! Check it out now: https://github.com/firebase/FirebaseUI-Android/pull/276/files#diff-c7d9ac7c32439f20359555e3db253268R119

SUPERCILEX · 2016-09-10T05:36:44Z

Thanks a ton for the feedback!!! I'm new to the object oriented thing and to programming in general so this was a huge help (and a bit of a struggle). I just figured how to use interfaces to get rid of instanceof two weeks ago, pretty sad. :)

Anyway, thanks again. I think I have a nice implementation now. Also, I left a question/comment if you have any thoughts about it.

SUPERCILEX · 2016-09-10T05:38:08Z

Should I update the app example to index the data? I don't think it's necessary, but it would put people on the right track for scalability.

puf · 2016-09-10T16:17:17Z

database/src/main/java/com/firebase/ui/database/FirebaseArray.java

            }
+            index++;


Revert this change please.

SUPERCILEX · 2016-10-19T21:59:50Z

database/src/main/java/com/firebase/ui/database/FirebaseIndexArray.java

+    public void onCancelled(DatabaseError error) {
+        Log.e(TAG, "A fatal error occur retrieving the necessary keys to poplulate your adapter");
+        super.onCancelled(error);
+    }


@morganchen12

Thanks!

I should add that invalidating the whole object feels kind of gross, since the dev who encounters this error basically has the options of giving up or creating another indexed array and hoping that one doesn't error. It's like a failable initializer, but in this case the failure is asynchronous and therefore extremely nasty.

If you have ideas for better handling this error, please do share them!

Devs can override this method in which they could reinitialize their adapter with a new ref or replace their view with an "Oops, an unexpected error occurred." message. I think this gives devs nice options. What do you think of having a separate error method to override for the key failure vs the ref failure?

Yeah, I think we've done what we can to avoid badness here. How you handle other errors I'll leave to you since I don't know the platform specifics well, but imo they should be passed up to the API consumer with enough info for them to make a good decision.

SUPERCILEX · 2016-10-21T00:19:29Z

@morganchen12 @samtstern
While working on tests, I discovered that you cannot change the priority of the dataRefs (well you can, but it won't do anything). Should I add in the documentation that the order of the list items is determined by the order they are received in from the keyRef? If you think about it, that's kinda obvious because you're going to have all the other entries in dataRef and having a bunch of haphazard priorities in dataRef would be weird... should I add it anyway?

SUPERCILEX · 2016-10-21T00:36:10Z

We've got a problem. The database tests aren't running at all and haven't ever been. Is this on purpose?

puf · 2016-10-21T00:40:00Z

I've run the database test locally, manually when we just started the project. They've never been run automatically afaik.

SUPERCILEX · 2016-10-21T00:51:33Z

@puf So as long as the tests ran fine for me, we're good?

I got all greens so unless you guys have a few final comments, this PR is good to merge!

puf · 2016-10-25T18:32:31Z

I wouldn't mind them running in CI. We just never had CI when we started.

On Thu, Oct 20, 2016 at 5:51 PM Alex Saveau [email protected]
wrote:

@puf https://github.com/puf So as long as the tests ran fine for me,
we're good?

I got all greens so unless you guys have a few final comments, this PR is
good to merge!

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#276 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA3w33poOIuq3GezTTYXkRHQ-T5iTxQ6ks5q2AyXgaJpZM4JyPx9
.

SUPERCILEX · 2016-10-25T19:33:36Z

I'm pretty sure we'd need an emulator with the current state of the tests which would significantly slow Travis down.

puf · 2016-10-31T17:42:39Z

database/src/main/java/com/firebase/ui/database/FirebaseArray.java

    public void onChildMoved(DataSnapshot snapshot, String previousChildKey) {
        int oldIndex = getIndexForKey(snapshot.getKey());
        mSnapshots.remove(oldIndex);
-        int newIndex = previousChildKey == null ? 0 : (getIndexForKey(previousChildKey) + 1);
+        int newIndex = previousChildKey == null ? 0 : getIndexForKey(previousChildKey) + 1;


Please put back the parentheses around (getIndexForKey(previousChildKey) + 1). The behavior is the same, but it makes the intent more explicit.

puf · 2016-10-31T17:56:02Z

database/src/main/java/com/firebase/ui/database/FirebaseIndexArray.java

+        mListener = listener;
+    }
+
+    private class DataRefSnapshot implements ValueEventListener {


We should name this class to indicate that it's a listener.

I renamed it DataRefListener, what do you think?

puf · 2016-10-31T18:02:04Z

I had a few nits, aside from that this looks good to me. Thanks for all the hard work on this!

SUPERCILEX · 2016-11-01T23:22:58Z

@puf @samtstern firebase/FirebaseUI-iOS#164 was just merged, any idea when this PR will be merged?

morganchen12 · 2016-11-01T23:27:40Z

@SUPERCILEX can you edit this PR to target the 1.0 branch? Once that's done and tests pass/conflicts are resolved I'll merge

SUPERCILEX · 2016-11-01T23:40:50Z

No conflicts!!! @morganchen12 This is awesome!!! I finally get to delete my tmpfirebasedatabase package!

samtstern · 2016-11-02T04:07:45Z

@SUPERCILEX two months later it's in! Thank you for your patience and all the other contributions you made while this one was pending. Version 1.0.0 is getting very close, excited to see this live.

SUPERCILEX · 2016-11-02T15:16:38Z

@puf @samstern thank you guys for your patience guiding me towards writing better code. I really appreciate it! I've learned a ton working on FirebaseUI and I hope to keep it up. Thanks!

I'm pretty excited for 1.0 too–there are so many new features!

SUPERCILEX added 17 commits August 31, 2016 18:41

Update FirebaseArray to support joined data

8b7b4f4

Update FirebaseRecyclerAdapter to support joined data

adaebb9

Refactor setOnChangedListener

a176ee5

Update FirebaseListAdapter to support joined data

01ab8d5

Refactor setOnChangedListener

620e8bf

Update README.md

c5f8d94

Handle key not found at dataRef

34482a3

Handle onCancelled

00e488f

Handle onCancelled

ac217dc

Handle onCancelled

15b8b9b

Update FirebaseArray.java

355d845

Update FirebaseArray.java

723dab2

Update FirebaseListAdapter.java

964ca72

Update FirebaseRecyclerAdapter.java

eca10e9

Update FirebaseArray.java

871aab2

Update FirebaseListAdapter.java

d97bb60

Log a warning instead of exception if key is missing

7e2be3a

SUPERCILEX mentioned this pull request Sep 6, 2016

Flattening nested data #280

Closed

SUPERCILEX added 2 commits September 6, 2016 14:39

Merge remote-tracking branch 'refs/remotes/origin/master' into fireba…

8389204

…se/master

Merge pull request #1 from firebase/master

b002725

test

SUPERCILEX added 3 commits September 8, 2016 18:56

Merge pull request #4 from firebase/master

315428d

Pull latest changes

Extract indexed array into separate class

b8f3002

Merge remote-tracking branch 'origin/master' into indexed-database

3cbadb9

SUPERCILEX reviewed Sep 10, 2016
View reviewed changes

puf reviewed Sep 10, 2016
View reviewed changes

database/src/main/java/com/firebase/ui/database/FirebaseArray.java

}

index++;

Copy link

Contributor

puf Sep 10, 2016

Choose a reason for hiding this comment

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

Revert this change please.

Even clearer

e269eb4

SUPERCILEX commented Oct 19, 2016

View reviewed changes

SUPERCILEX added 3 commits October 19, 2016 19:24

Relax query requirements and remove unnecessary constructors.

4fddf27

Merge remote-tracking branch 'origin/master'

d6fed13

fix typo

8ce38f6

SUPERCILEX added 2 commits October 20, 2016 17:20

tests part 1

20ebddd

test travis should fail

645dc05

tests part 2

5769975

SUPERCILEX added 2 commits October 27, 2016 20:31

Remove unnecessary constructor

9d8f7f6

Fix typo

eecc8cd

puf reviewed Oct 31, 2016

View reviewed changes

Fix nits

9cdb9ea

Merge remote-tracking branch 'firebase/version-1.0.0-dev'

c6ca19d

SUPERCILEX changed the base branch from master to version-1.0.0-dev November 1, 2016 23:39

morganchen12 merged commit 5a4b661 into firebase:version-1.0.0-dev Nov 1, 2016

samtstern mentioned this pull request Nov 28, 2016

Support for joins #12

Closed

SUPERCILEX mentioned this pull request Feb 18, 2017

The Big Kahuna of firebase-ui-database #544

Merged

4 tasks

Support for joins #276

Support for joins #276

Uh oh!

Conversation

SUPERCILEX commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giautm commented Sep 4, 2016

Uh oh!

puf commented Sep 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giautm Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Sep 10, 2016

Uh oh!

SUPERCILEX commented Sep 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Oct 21, 2016

Uh oh!

SUPERCILEX commented Oct 21, 2016

Uh oh!

puf commented Oct 21, 2016

Uh oh!

SUPERCILEX commented Oct 21, 2016

Uh oh!

puf commented Oct 25, 2016

Uh oh!

SUPERCILEX commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puf commented Oct 31, 2016

Uh oh!

SUPERCILEX commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morganchen12 commented Nov 1, 2016

Uh oh!

SUPERCILEX commented Nov 1, 2016

Uh oh!

samtstern commented Nov 2, 2016

Uh oh!

SUPERCILEX commented Nov 2, 2016

Uh oh!

Uh oh!

SUPERCILEX commented Sep 1, 2016 •

edited

Loading

giautm Sep 11, 2016 •

edited

Loading

SUPERCILEX Sep 11, 2016 •

edited

Loading

SUPERCILEX Oct 31, 2016 •

edited

Loading

SUPERCILEX commented Nov 1, 2016 •

edited

Loading