The Big Kahuna of firebase-ui-database #544

SUPERCILEX · 2017-01-26T01:08:22Z

Enables #539 (I think)
#337
@TheCraftKid I think this one will make you happy! (and your library obsolete 😄)

@samtstern Sorry about this one, the amount of changes is high up there on my list of biggest PRs, but I believe all of this is necessary for us to be able to commit to maintaining firebase-ui-database in the long term.

Decide what to do about equals and hashCode.
Thoroughly test stuff
Figure out what to do about the performance issues with FirebaseArrayOfObjects
Explain what's going on in FirebaseArrayOfObjectsOptimized (Fixed in fda5417)

Signed-off-by: Alex Saveau <[email protected]>

…lback

Signed-off-by: Alex Saveau <[email protected]>

…lback

Signed-off-by: Alex Saveau <[email protected]>

…lback

Signed-off-by: Alex Saveau <[email protected]>

…lback

Signed-off-by: Alex Saveau <[email protected]>

…lback # Conflicts: # app/src/main/java/com/firebase/uidemo/database/ChatActivity.java # 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

Signed-off-by: Alex Saveau <[email protected]>

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

# Conflicts: # database/src/androidTest/java/com/firebase/ui/database/TestUtils.java

…aseUI-Android into db-callback

…lback # Conflicts: # app/src/main/java/com/firebase/uidemo/database/ChatActivity.java # database/src/androidTest/java/com/firebase/ui/database/TestUtils.java # database/src/main/java/com/firebase/ui/database/FirebaseArray.java # database/src/main/java/com/firebase/ui/database/FirebaseIndexArray.java # database/src/main/java/com/firebase/ui/database/FirebaseListAdapter.java # database/src/main/java/com/firebase/ui/database/FirebaseRecyclerAdapter.java

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2017-02-25T02:58:52Z

@samtstern Hmmm, your branch looks really interesting! The gist I got from it was to make the model class a first class citizen instead of DataSnapshot which makes way more sense. I don't really like that you have a getObject instead of getSnapshot method thus moving the model class a little further back. What do you think of rewriting this PR with your changes, but implementing List<E> in ObservableSnapshotArray instead of List<DataSnapshot>? (We could call it ObservableModelArray<T> or something better.)

puf · 2017-02-25T03:04:07Z

Agreed with Alex that this looks promising. I only looked at `FirebaseArray` for now, so still need to see how the extensions (such as the indexed array or a re-sorted array) would work. I'd prefer to not expose too many lists though. If it exposes a `List<E>`, there should also be a `List<String>` with the keys. And when you have both of those, they're really just syntactic sugar for `List<DataSnapshot>`. Nothing wrong with syntactic sugar, but three lists feels like a bit much.

…

On Fri, Feb 24, 2017 at 6:58 PM Alex Saveau ***@***.***> wrote: @samtstern <https://github.com/samtstern> Hmmm, your branch looks really interesting! The gist I got from it was to make the model class a first class citizen instead of DataSnapshot which makes way more sense. I don't really like that you have a getObject instead of getSnapshot method thus moving the model class a little further back. What do you think of rewriting this PR with your changes, but implementing List<E> in ObservableSnapshotArray instead of List<DataSnapshot>? (We could call it ObservableModelArray<T> or something better.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#544 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3w39hrT8tOWWgEXvxFQ8viYkMOLSWhks5rf5jtgaJpZM4LuMcz> .

Change-Id: I831fd0bae52970ff7db6f4ab1ae2fdb7aa04200a

samtstern · 2017-02-27T20:05:35Z

Opened https://github.com/SUPERCILEX/FirebaseUI-Android/pull/24

… object, change type name to `E` in ObservableSnapshotArray to unhide type T in toArray method, remove getSnapshots(int) method from FirebaseArray since there's already get(int), use `index` instead of `newIndex` in onKeyMove in FirebaseIndexArray Signed-off-by: Alex Saveau <[email protected]>

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2017-03-01T21:47:59Z

@samtstern The index adapters were completely broken because we forgot to pass in a parser. Since I feel like this could be a recurring theme, I just removed the possibility of a null parser in 7601ded. If devs really don't want to provide a model class for some reason, they can just pass in Object.class and it will return an object:

var1.equals(Object.class)?var0:(var1.isEnum()?zzd(var0, var1):zze(var0, var1));

I also had to write a hacky constructor that really bothers me, but I couldn't think of a better way of doing it that would still let devs extend the adapters while passing in their own FirebaseArray. What do you think of 7601ded?

samtstern · 2017-03-01T22:16:01Z

@SUPERCILEX

I am fine with removing the null parser option, like you did.
Can you explain a little more what is enabled by adding the empty constructors for the adapters? I think I am missing something.

SUPERCILEX · 2017-03-02T17:20:18Z

@samtstern

Awesome!
Java forces you to either have a default constructor (the option I opted for) or to call super in the constructor you're extending.

public class First {
    public First(String foo) {}
}

private class Second extends First {
    public Second() {} // Doesn't compile

    public Second(String bar) { super(bar); } // Compiles
}

Since we also can't pass this into a super call, we're stuck again. Hope that helps clear things up!

…seArray

samtstern · 2017-03-02T21:11:07Z

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

-        super(activity, modelClass, modelLayout, new FirebaseIndexArray(keyRef, dataRef));
+                                    Query keyQuery,
+                                    DatabaseReference dataRef) {
+        init(activity, new FirebaseIndexArray<>(keyQuery, dataRef, this), modelClass, modelLayout);


Hmm now I see what happened here. Having init be protected and called by the super class definitely "smells" and makes me think we got something wrong.

Seems like the problem is the circular reference between the adapter and the array, even though it goes through the SnapshotParser interface.

Any thoughts on how we can make this better?

@samtstern Hmmm, I found out that you can't reference non-static fields or methods either in a constructor call so that means no forwarding calls or anything like that. At this point, the only solution I can think of is using a setter for the parser, but I feel like that's way more hacky and makes the intentions less clear. Also, that beats the point of initializing everything for people because they will have to call setParser. @puf Have you encountered this kind of issue before or have any thoughts on this?

Here's a possible answer. Since we're breaking parseSnapshot anyway, what if we removed it from the adapter entirely and instead had the adapters take an optional SnapshotParser in their constructor. They would pass it through to the FirebaseArray if provided.

This seems to be a better representation of the hierarchy anyway,

@samtstern As long as you're fine with making more breaking api changes, that solution sounds awesome to me!

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2017-03-06T01:24:29Z

@samtstern I'm getting this really weird crash because ChildEventListeners are firing even though we removed the listener. I checked and there is only one pair of add/removing being called so that's not the issue. At this point, I think it's a Firebase problem, not this PR.

Here are some screenshots showing that even though removeEventListener was called, onChildMoved still gets called:

samtstern · 2017-03-06T03:11:37Z

@SUPERCILEX looks like maybe it's how we're casting? We're calling removeEventListener(this) twice but maybe we need removeEventListener((ChildEventListener) this); removeEventListener((ValueEventListener) this);

SUPERCILEX · 2017-03-06T03:16:57Z

@samtstern I was confused by that too, but I think it's just a decompiling bug (java doesn't let you do what's in the screenshot anyway--I checked). If you look at our real code, we are casting things correctly: https://github.com/firebase/FirebaseUI-Android/pull/544/files#diff-fd5c505ad432166321255bc4541624ddR89 so I'm pretty sure this is an issue with the RTDB.

samtstern · 2017-03-06T03:20:42Z

@SUPERCILEX yep you're right, it's not the casting issue.

I just took a look at this and (I can't believe I'm saying it) but I'm ready to merge this beast! Could you file a new issue about the possible listener removal race condition thing? Definitely worth tracking but it does not seem to be a direct result of this PR.

Do you have any oppositions to my doing a 'squash and merge' on this PR?

samtstern · 2017-03-06T03:21:40Z

SUPERCILEX · 2017-03-06T03:25:26Z

@samtstern AWESOME!!!!

Yeah, I already filed a bug for the slow listener removal.

No objections at all, please don't do anything but a squash. There are way too many commits in this PR!

SUPERCILEX · 2017-03-06T03:26:01Z

Haha, nice gif!

SUPERCILEX added 30 commits December 8, 2016 19:48

Add a callback in FirebaseIndexArray when a Key doesn't exist in a ref

28c366c

Signed-off-by: Alex Saveau <[email protected]>

Add documentation

641297c

Signed-off-by: Alex Saveau <[email protected]>

Rename listeners

a62a025

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into db-cal…

703f85d

…lback

Add JoinResolver.java interface

f1e702b

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

af39a1e

Signed-off-by: Alex Saveau <[email protected]>

Rename ChangeListener to ChangeEventListener

25fc4df

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

5486e8c

Signed-off-by: Alex Saveau <[email protected]>

Make JoinResolver.java public

23806d7

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into db-cal…

06fb7d0

…lback

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into db-cal…

0b56e99

…lback

Rename

331b4bf

Signed-off-by: Alex Saveau <[email protected]>

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into db-cal…

f396fec

…lback

Fix compile error

cfdb305

Signed-off-by: Alex Saveau <[email protected]>

Merge branch 'version-1.1.0-dev' into db-callback

cc3729c

Merge remote-tracking branch 'firebase/version-1.1.0-dev' into db-cal…

f5d06b2

…lback

Make FirebaseArray.java fully decked out and implementing List

2b65f11

Signed-off-by: Alex Saveau <[email protected]>

Make stuff public

961f9d7

Signed-off-by: Alex Saveau <[email protected]>

Fix merge mistakes

97d9cb0

Signed-off-by: Alex Saveau <[email protected]>

Merge branch 'db-callback' into FirebaseArray

0f11146

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

Merge remote-tracking branch 'firebase/master' into FirebaseArray

460b733

# Conflicts: # database/src/androidTest/java/com/firebase/ui/database/TestUtils.java

Merge remote-tracking branch 'firebase/master' into db-callback

e7297d4

# Conflicts: # database/src/androidTest/java/com/firebase/ui/database/TestUtils.java

Merge remote-tracking branch 'firebase/master' into db-callback

9a03342

Merge branch 'version-1.2.0-dev' of https://github.com/firebase/Fireb…

8f87492

…aseUI-Android into db-callback

Fix a ton of merge mistakes

48ea75a

Signed-off-by: Alex Saveau <[email protected]>

Provide default join resolver

4a5a1be

Signed-off-by: Alex Saveau <[email protected]>

Fix tests and bugs

dbbf0dc

Signed-off-by: Alex Saveau <[email protected]>

Cleanup

b37c8bd

Signed-off-by: Alex Saveau <[email protected]>

samtstern added 2 commits February 27, 2017 10:56

Tests pass, TODOs resolved, code made DRY

a38c1b2

Change-Id: I831fd0bae52970ff7db6f4ab1ae2fdb7aa04200a

Fix build and docs

a87a0d9

samtstern and others added 5 commits March 1, 2017 09:51

Respond to review comments

51b05e6

Make FirebaseAdapter protected

be09c80

Merge branch 'FirebaseArray' into firebase-array-explore

a4d7ad1

Remove null parser possibility and fix broken FirebaseIndexAdapters

7601ded

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX mentioned this pull request Mar 2, 2017

Add a callback in FirebaseIndexArray when a Key doesn't exist in a ref #448

Closed

1 task

Merge remote-tracking branch 'firebase/version-2.0.0-dev' into Fireba…

fa6265e

…seArray

samtstern reviewed Mar 2, 2017

View reviewed changes

SUPERCILEX added 2 commits March 3, 2017 19:07

Fix circular SnapshotParser dependency in adapters

42c639a

Signed-off-by: Alex Saveau <[email protected]>

Remove getItemId method - hashcode isn't stable

cfef2c5

Signed-off-by: Alex Saveau <[email protected]>

samtstern merged commit ac5c849 into firebase:version-2.0.0-dev Mar 6, 2017

samtstern added this to the 2.0.0 milestone Mar 6, 2017

SUPERCILEX deleted the FirebaseArray branch March 6, 2017 03:26

samtstern mentioned this pull request Mar 6, 2017

FirebaseArray's Limited Access Modifiers #337

Closed

samtstern pushed a commit that referenced this pull request Mar 6, 2017

Update documentation and sample for #544 (#567)

cf144b4

The Big Kahuna of firebase-ui-database #544

The Big Kahuna of firebase-ui-database #544

Uh oh!

Conversation

SUPERCILEX commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SUPERCILEX commented Feb 25, 2017

Uh oh!

puf commented Feb 25, 2017 via email

Uh oh!

samtstern commented Feb 27, 2017

Uh oh!

SUPERCILEX commented Mar 1, 2017

Uh oh!

samtstern commented Mar 1, 2017

Uh oh!

SUPERCILEX commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samtstern Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

samtstern Mar 3, 2017

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Mar 6, 2017

Uh oh!

samtstern commented Mar 6, 2017

Uh oh!

SUPERCILEX commented Mar 6, 2017

Uh oh!

samtstern commented Mar 6, 2017

Uh oh!

samtstern commented Mar 6, 2017

Uh oh!

SUPERCILEX commented Mar 6, 2017

Uh oh!

SUPERCILEX commented Mar 6, 2017

Uh oh!

Uh oh!

SUPERCILEX commented Jan 26, 2017 •

edited

Loading

SUPERCILEX commented Mar 2, 2017 •

edited

Loading