Let devs intercept onChanged events and control when to update their adapter #488

SUPERCILEX · 2017-01-04T00:15:36Z

Would address issues like #482 and see #15 (comment).

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

…-filtering+predicates

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

…-filtering+predicates

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

SUPERCILEX · 2017-01-04T00:16:25Z

app/src/main/java/com/firebase/uidemo/database/ChatActivity.java

        mManager = new LinearLayoutManager(this);
-        mManager.setReverseLayout(false);
-
-        mMessages.setHasFixedSize(false);


Unnecessary, these are defaults.

There are valid reasons for having this code here, which we could debate here. But these reasons have nothing to do with letting devs intercept change events. In future PRs please keep functional changes separate from code cleanup.

I removed this change, but I am curious, why are we setting default values and adding seemingly unnecessary code?

SUPERCILEX · 2017-01-04T00:16:56Z

app/src/main/java/com/firebase/uidemo/database/ChatActivity.java

-                            Toast.makeText(ChatActivity.this, "Signed In",
-                                    Toast.LENGTH_SHORT).show();
-                            attachRecyclerViewAdapter();
+                            Toast.makeText(getApplicationContext(),


Show toast and continue logging in even if screen was rotated.

Why did we remove the call to attachRecyclerViewAdapter() here? Also if we scope the onComplete listener to the Activity context then we can avoid dead activity issues.

@samtstern I've extracted the toast logic into a static class to prevent activity leaks. As for the attachRecyclerViewAdapter, I moved that up to an onSuccess listener that is tied to the activity.

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

…o enable-filtering+predicates

SUPERCILEX · 2017-01-04T00:39:27Z

@puf PTAL, these changes shouldn't be anything major and can be merged in this branch since I'm targeting the dev branch. I'll merge your other PRs into this branch and resolve conflicts since I think there will be style issues causing the Travis build to fail. (FirebaseUI now has static analysis checks!)

samtstern

This change LGTM.

samtstern · 2017-01-05T17:09:22Z

app/src/main/java/com/firebase/uidemo/database/ChatActivity.java

-                            Toast.makeText(ChatActivity.this, "Signed In",
-                                    Toast.LENGTH_SHORT).show();
-                            attachRecyclerViewAdapter();
+                            Toast.makeText(getApplicationContext(),


Why did we remove the call to attachRecyclerViewAdapter() here? Also if we scope the onComplete listener to the Activity context then we can avoid dead activity issues.

samtstern · 2017-01-05T17:13:38Z

@puf could you take a look? Basically this does two things:

Move the OnChangedListener interface out of FirebaseArray and make it first class.
Make a protected onChanged() function in the adapters so that some developers can change the behavior.

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

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

SUPERCILEX · 2017-01-05T19:31:06Z

@puf Since OnChangedListener is now public would you be ok with me renaming things?

OnChangedListener -> ChangeEventListener to be more consistent with Firebase apis
onChanged -> onChildChanged as discussed in This change allows easy detection of the list being empty *and* respo… #487 (comment)

@puf @samtstern What do you guys think?

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

puf

Overall looks like a useful change, but I have a few comments

puf · 2017-01-07T19:38:41Z

app/src/main/java/com/firebase/uidemo/database/ChatActivity.java

        mManager = new LinearLayoutManager(this);
-        mManager.setReverseLayout(false);
-
-        mMessages.setHasFixedSize(false);


There are valid reasons for having this code here, which we could debate here. But these reasons have nothing to do with letting devs intercept change events. In future PRs please keep functional changes separate from code cleanup.

puf · 2017-01-07T19:39:36Z

app/src/main/java/com/firebase/uidemo/database/ChatActivity.java

            attachRecyclerViewAdapter();
+        } else {


Same as before: this change seems unrelated to the topic of this PR and just expresses a personal preference. While that preference might well be better than the existing code, it should not pollute this PR.

@puf I write my PRs with the philosophy that small improvements as we go along are better than constantly submitting side PRs for this kind of change. However, I see where you are coming from and removed these changes from this PR.

puf · 2017-01-07T19:42:53Z

app/src/main/java/com/firebase/uidemo/database/ChatActivity.java

@@ -286,4 +276,21 @@ public void setText(String text) {
            mTextField.setText(text);
        }
    }
+
+    private static class SignInResultNotifier implements OnCompleteListener<AuthResult> {


Nice addition! I'd appreciate some documentation on this class though.

puf · 2017-01-07T19:46:20Z

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

+import com.google.firebase.database.DataSnapshot;
+import com.google.firebase.database.DatabaseError;
+
+public interface OnChangedListener {


I'm fine pulling this up. But wouldn't it be better as an interface in FirebaseArray (which I see moving towards becoming an abstract base-class or interface itself)? Having this inside FirebaseArray makes it more explicitly scoped as "these operations apply to this FirebaseArray (or subclass) object".

@puf, I would like that too, but you've chosen to keep FirebaseArray package private so people will get an error when overriding onChanged saying they can't access FirebaseArray.OnChangedListener.EventType. To put OnChangedListener in FirebaseArray we would have to go public and I don't think FirebaseArray is quite ready for that.

But then isn't this essentially doing the same: making a part of the internal workings of FirebaseArray public?

…-filtering+predicates

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

SUPERCILEX

@puf, I addressed your review comments, but I would like your thoughts on #488 (comment) before merging.

SUPERCILEX · 2017-01-07T20:11:38Z

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

+import com.google.firebase.database.DataSnapshot;
+import com.google.firebase.database.DatabaseError;
+
+public interface OnChangedListener {


@puf, I would like that too, but you've chosen to keep FirebaseArray package private so people will get an error when overriding onChanged saying they can't access FirebaseArray.OnChangedListener.EventType. To put OnChangedListener in FirebaseArray we would have to go public and I don't think FirebaseArray is quite ready for that.

puf · 2017-01-08T03:05:32Z

@SUPERCILEX: Not sure why we had these layout properties in there, but I know for a fact I referred developers specifically to this part of the example multiple times. It's the same as explicitly initializing a variable to its default: you make it clear that it's not mistake and can more easily refer to it to discuss it.

…

On Sat, Jan 7, 2017 at 12:20 PM Alex Saveau ***@***.***> wrote: ***@***.**** commented on this pull request. In app/src/main/java/com/firebase/uidemo/database/ChatActivity.java <#488>: > mManager = new LinearLayoutManager(this); - mManager.setReverseLayout(false); - - mMessages.setHasFixedSize(false); I removed this change, but I am curious, why are we setting default values and adding seemingly unnecessary code? ------------------------------ In app/src/main/java/com/firebase/uidemo/database/ChatActivity.java <#488>: > attachRecyclerViewAdapter(); + } else { @puf <https://github.com/puf> I write my PRs with the philosophy that small improvements as we go along are better than constantly submitting side PRs for this kind of change. However, I see where you are coming from and removed these changes from this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#488>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3w3_-qib3ZtH5b9b76JuMLk4XptPt8ks5rP_OHgaJpZM4LaKv4> .

SUPERCILEX · 2017-01-08T03:49:42Z

I see, it makes sense now. @puf Thanks for letting me know! On Sat, Jan 7, 2017 at 7:05 PM Frank van Puffelen <[email protected]> wrote:

…

@SUPERCILEX: Not sure why we had these layout properties in there, but I know for a fact I referred developers specifically to this part of the example multiple times. It's the same as explicitly initializing a variable to its default: you make it clear that it's not mistake and can more easily refer to it to discuss it. On Sat, Jan 7, 2017 at 12:20 PM Alex Saveau ***@***.***> wrote: > ***@***.**** commented on this pull request. > > In app/src/main/java/com/firebase/uidemo/database/ChatActivity.java > <#488>: > > > mManager = new LinearLayoutManager(this); > - mManager.setReverseLayout(false); > - > - mMessages.setHasFixedSize(false); > > I removed this change, but I am curious, why are we setting default values > and adding seemingly unnecessary code? > ------------------------------ > > In app/src/main/java/com/firebase/uidemo/database/ChatActivity.java > <#488>: > > > attachRecyclerViewAdapter(); > + } else { > > @puf <https://github.com/puf> I write my PRs with the philosophy that > small improvements as we go along are better than constantly submitting > side PRs for this kind of change. However, I see where you are coming from > and removed these changes from this PR. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#488>, or mute the > thread > < https://github.com/notifications/unsubscribe-auth/AA3w3_-qib3ZtH5b9b76JuMLk4XptPt8ks5rP_OHgaJpZM4LaKv4 > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#488 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJDRJOUU4rweI5mp7tOitEf9j0a8HVILks5rQFKDgaJpZM4LaKv4> .

…o `onChildChanged` Signed-off-by: Alex Saveau <[email protected]>

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

SUPERCILEX · 2017-01-09T19:38:02Z

@puf since you pushed 21d5d44, I went ahead with the rename.

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

…predicates # Conflicts: # database/src/androidTest/java/com/firebase/ui/database/TestUtils.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-01-13T01:11:31Z

@puf, any chance you could take another look at this? (I just merged everything)

puf · 2017-01-16T22:54:06Z

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

@@ -80,21 +69,21 @@ public void onChildAdded(DataSnapshot snapshot, String previousChildKey) {
            index = getIndexForKey(previousChildKey) + 1;
        }
        mSnapshots.add(index, snapshot);
-        notifyChangedListeners(OnChangedListener.EventType.ADDED, index);
+        notifyChangedListeners(ChangeEventListener.EventType.ADDED, index);


Definitely a better name! +1

puf · 2017-01-16T22:55:01Z

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

@@ -51,7 +51,7 @@
 *            contained in the children of the given Firebase location
 */
 public abstract class FirebaseListAdapter<T> extends BaseAdapter {
-    private static final String TAG = FirebaseListAdapter.class.getSimpleName();
+    private static final String TAG = "FirebaseListAdapter";


Can you explain why this change is needed/useful?

@puf This isn't so much needed in FirebaseListAdapter, but this is a massive no-no in the FirebaseRecyclerAdapter where the name is 23 characters long. I made this tag in one of my first PRs and I really regret it: if someone were to create a tag using getSimpleName() in FirebaseIndexRecyclerAdapter, Log.anything will throw an exception because the tag name exceeds 24 characters. You should NEVER use dynamic tag names for that reason.

@puf, since this is higher priority than #488, I've created #525.

puf · 2017-01-16T22:59:48Z

@SUPERCILEX I left a few comments. Most are very minor. The only real concern is that we'll end up exposing internal details of the FirebaseArray when those aren't sufficiently hardened yet. But I'm very willing to take a vote there, as the recent changes feel like a move in the right direction.

Of course we'll need the same ability to listen for changes in FirebaseUI-iOS, before we can merge. @morganchen12 did you already have a look at this?

SUPERCILEX · 2017-01-16T23:13:02Z

@puf SGTM! I think you'll like my upcoming PRs for FirebaseArray... 😄

morganchen12 · 2017-01-17T18:21:54Z

I think this feature is added with the transforms PR on iOS. @puf

…predicates

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

…o enable-filtering+predicates

SUPERCILEX · 2017-01-18T00:26:58Z

@puf @morganchen12 @samtstern This PR doesn't provide explicit support for joins because @puf doesn't want that, but it does allow devs to do their own filtering as seen in my crappy example. So, @morganchen12 @puf do you guys want to merge this PR and firebase/FirebaseUI-iOS#186 and call it good because @puf doesn't want to support on device filerting? Or should we do something else?

I personally agree with @puf about not doing filtering on device, but there's the feature parity thing. I think this PR is good enough though because it lets devs choose what they want to do. If need be, we could provider a better example than my own that demos filtering and sorting on device with this PR.

morganchen12 · 2017-01-18T19:30:45Z

iOS doesn't add on-device filtering either, so no feature parity issues there.

SUPERCILEX · 2017-01-19T06:35:49Z

@samtstern @puf @morganchen12 So are we good to merge?

samtstern · 2017-01-19T19:02:15Z

Still fine with me, will let @puf pull the trigger. I have just pushed a new version-1.2.0-dev branch off of master where we should put this feature.

morganchen12 · 2017-01-19T19:33:56Z

Fine with me, pending @puf's decision as well

…-filtering+predicates

SUPERCILEX · 2017-01-24T00:13:47Z

@puf, could you please take another look at this? It's blocking progress on my other PRs... Thanks!

samtstern · 2017-01-24T21:50:31Z

Ok after some team discussion it's time to merge this. Squashing and merging into the 1.2.0 branch.

SUPERCILEX · 2017-01-24T22:15:48Z

@samtstern Really glad to have this merged!

I started writing a post demoing some use cases so just in case anyone wants to see it, here you go: #482 (comment).

SUPERCILEX added 9 commits December 7, 2016 19:12

Update api to let users do their own filtering + sorting

5d5635a

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

Cleanup

0e3ebbc

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

Cleanup

9d462dc

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

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

276a5ba

…-filtering+predicates

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

0b01f48

…-filtering+predicates

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

9ea660e

…-filtering+predicates

Add documentation

0f12fe3

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

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

0fa222d

…-filtering+predicates

Cleanup

2899205

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

SUPERCILEX commented Jan 4, 2017

View reviewed changes

SUPERCILEX added 2 commits January 3, 2017 16:22

Cleanup

0826541

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

Merge remote-tracking branch 'origin/enable-filtering+predicates' int…

bc43a01

…o enable-filtering+predicates

Merge branch 'version-1.1.0-dev' into enable-filtering+predicates

e8a9905

samtstern approved these changes Jan 5, 2017

View reviewed changes

samtstern requested a review from puf January 5, 2017 17:12

SUPERCILEX added 2 commits January 5, 2017 11:09

Extract onComplete listener into static inner class to prevent activi…

ab3743e

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

Cleanup

08159bd

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

Temporarily ignore robolectric updates

b066b54

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

puf reviewed Jan 7, 2017

View reviewed changes

SUPERCILEX added 2 commits January 7, 2017 11:59

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

3e5b71d

…-filtering+predicates

Address review comments

1c1dfb7

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

SUPERCILEX commented Jan 7, 2017

View reviewed changes

SUPERCILEX added 2 commits January 9, 2017 11:32

Rename OnChangedListener to ChangeEventListener and onChanged t…

a9bad96

…o `onChildChanged` Signed-off-by: Alex Saveau <[email protected]>

Rename onChildChanged in adapters

f875b48

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

SUPERCILEX added 4 commits January 10, 2017 20:41

Fix merge mistakes

c85ca14

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

Fix merge mistakes

50d3d37

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

Fix more merge mistakes

17a136d

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

SUPERCILEX mentioned this pull request Jan 14, 2017

Add getItems method to adapters #501

Closed

puf reviewed Jan 16, 2017

View reviewed changes

SUPERCILEX mentioned this pull request Jan 16, 2017

NEVER use dynamic tag names #525

Merged

Merge remote-tracking branch 'firebase/master' into enable-filtering+…

4b74478

…predicates

SUPERCILEX changed the base branch from version-1.1.0-dev to master January 17, 2017 19:00

SUPERCILEX added 3 commits January 17, 2017 11:03

Cleanup

358d50d

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

Cleanup

4496baf

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

Merge remote-tracking branch 'origin/enable-filtering+predicates' int…

dcf4b91

…o enable-filtering+predicates

SUPERCILEX changed the base branch from master to version-1.2.0-dev January 21, 2017 01:30

Merge remote-tracking branch 'firebase/version-1.2.0-dev' into enable…

499d448

…-filtering+predicates

samtstern merged commit d08eaa0 into firebase:version-1.2.0-dev Jan 24, 2017

SUPERCILEX mentioned this pull request Jan 24, 2017

Blinking On Child Updates #482

Closed

SUPERCILEX deleted the enable-filtering+predicates branch January 24, 2017 22:14

Let devs intercept onChanged events and control when to update their adapter #488

Let devs intercept onChanged events and control when to update their adapter #488

Uh oh!

Conversation

SUPERCILEX commented Jan 4, 2017

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 Jan 4, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Jan 4, 2017

Uh oh!

samtstern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Jan 5, 2017

Uh oh!

SUPERCILEX commented Jan 5, 2017

Uh oh!

puf left a comment

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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puf commented Jan 8, 2017 via email

Uh oh!

SUPERCILEX commented Jan 8, 2017 via email

Uh oh!

SUPERCILEX commented Jan 9, 2017

Uh oh!

SUPERCILEX commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Jan 16, 2017 • 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 Jan 16, 2017

Uh oh!

SUPERCILEX commented Jan 16, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

morganchen12 commented Jan 18, 2017

SUPERCILEX Jan 4, 2017 •

edited

Loading

SUPERCILEX Jan 16, 2017 •

edited

Loading

morganchen12 commented Jan 17, 2017 •

edited

Loading

SUPERCILEX commented Jan 18, 2017 •

edited

Loading