This change allows easy detection of the list being empty *and* respo… by puf · Pull Request #487 · firebase/FirebaseUI-Android

puf · 2017-01-03T23:07:09Z

Fixes #458 and #118. Ready for a final review, mostly to see how everyone feels about the name.

Previously just writing repositories makes it confusing whether to add the url under `buildscript` or `allprojects`.

add fabric maven url under allprojects

…nding to multi-child changes. The demo app has been updated to show an "this list is empty" view when there are no chat messages. Separate API changes (and minimal) demo from the bigger demo changes in https://github.com/firebase/FirebaseUI-Android/pull/477/files

puf · 2017-01-03T23:07:36Z

@SUPERCILEX Can you have a look at this PR and weigh in on the name of the new method?

SUPERCILEX · 2017-01-04T00:12:07Z

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

+
+            @Override
+            public void onReady() {
+                FirebaseRecyclerAdapter.this.onDataChanged();


Wouldn't we want to unify these names and pick either onReady or onDataChanged? Other than that, LGTM!

Yeah, I wasn't sure. In the context of FirebaseArray the name onDataChanged() is too close to the existing onChanged() name. But I like the name as a whole.

We could consider renaming onChanged() to onChildChanged() (keep its signature aside from the name-change). Since the interface isn't part of the public API (yet), we could do that in a minor version.

I think we should go for the second option (with onChildChanged letting us use onDataChanged). It would make the api more consistent with the native Firebase apis and fits well in the context of my other PR: https://github.com/firebase/FirebaseUI-Android/pull/488/files#diff-3620a1e9acaa17bea769cfbc3079f39aR18.

Agree, might as well change the signature now while it's still private and get the name you actually want.

SUPERCILEX · 2017-01-04T02:56:07Z

@puf, I found a stopship bug when offline #477 (comment).

samtstern

Approve pending resolution of discussion in comments.

samtstern · 2017-01-04T21:17:51Z

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

+
+            @Override
+            public void onReady() {
+                FirebaseRecyclerAdapter.this.onDataChanged();


Agree, might as well change the signature now while it's still private and get the name you actually want.

…y` to `onDataChanged`.

samtstern · 2017-01-09T19:47:26Z

@puf I am ready to merge this, can you change the target branch to version-1.1.0-dev and then we'll merge?

SUPERCILEX · 2017-01-09T20:09:20Z

@samtstern, I think @puf wanted to hold off on merging because of this. I could be wrong, but I just wanted to make sure.

(#487 and #477 appear to be duplicates.)

samtstern · 2017-01-09T21:22:58Z

I interpreted the end of that discussion to mean "it will be fixed here when it's fixed in the SDK" but will let @puf confirm

puf · 2017-01-10T04:51:57Z

@samtstern @SUPERCILEX there is no ongoing work to fix this in the SDK, but I don't want to implement a workaround in FirebaseUI. If that is the only thing blocking this PR, I suggest we merge and keep #477 (comment) open.

puf · 2017-01-10T04:53:26Z

@SUPERCILEX #487 is a small subset of #477. The latter includes a more elaborate demo, which we're (it's @samtstern for once) not ready to merge yet.

SUPERCILEX · 2017-01-10T04:53:37Z

@puf, SGTM.

googlebot · 2017-01-10T04:54:46Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

SUPERCILEX · 2017-01-10T04:54:57Z

Ah, OK. Thanks for the clarification @puf.

SUPERCILEX · 2017-01-10T05:13:02Z

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

+        mEmptyListView = findViewById(R.id.emptyTextView);
+
+        mRef = FirebaseDatabase.getInstance().getReference();
+        mChatRef = mRef.child("chats");


@puf looks like the merge didn't work (mChatRef got split into a nonexistent mRef for some reason).

Yeah, I split into two refs and apparently forgot to initialize one when resolving the conflict. It was my first time using the web conflict resolution editor and I must've borked something. I'll fix it tomorrow back at my desktop.

Oh cool! I haven't been able to try that out yet. Github just needs to add checks to make sure your code compiles 😄 (oh, and why not add Intellij while we're at it and just write all of your code for you! 😄)

SUPERCILEX

@puf FirebaseUI now has checkstyle and it appears to be mad about spacing... Hopefully, this won't become too much of an annoyance. 😄

SUPERCILEX · 2017-01-10T19:19:49Z

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

+            @Override
+            protected void onDataChanged() {
+                // if there are no chat messages, show a view that invites the user to add a message
+                mEmptyListView.setVisibility(mRecyclerViewAdapter.getItemCount() == 0?View.VISIBLE:View.INVISIBLE);


Space around : and ?.

…picky about that.

SUPERCILEX · 2017-01-10T19:32:46Z

app/src/main/res/layout/activity_chat.xml

+        android:id="@+id/emptyTextView"
+        android:layout_width="match_parent"
+        android:layout_height="wrap_content"
+        android:text="No messages. Start chatting at the bottom!"


@puf Hmmm, this is kind of an experiment so we'll have to see how it goes. I would recommend running ./gradlew check to make sure lint and checkstyle are happy, but in this case it wants you to create and @string resource. You can just ignore it with tools:ignore="HardcodedText" if you want.

…hat.

…achange

samtstern · 2017-01-10T21:15:43Z

Going to squash and merge this, thank you all!

jdsingh and others added 3 commits December 9, 2016 13:56

add fabric maven url under allprojects

83cbc3c

Previously just writing repositories makes it confusing whether to add the url under `buildscript` or `allprojects`.

Merge pull request #449 from jdsingh/patch-1

41cafd6

add fabric maven url under allprojects

puf requested review from morganchen12 and samtstern January 3, 2017 23:07

SUPERCILEX reviewed Jan 4, 2017

View reviewed changes

samtstern approved these changes Jan 4, 2017

View reviewed changes

morganchen12 approved these changes Jan 4, 2017

View reviewed changes

SUPERCILEX mentioned this pull request Jan 5, 2017

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

Merged

Renamed original onChanged to onChildChanged, renamed new `onRead…

21d5d44

…y` to `onDataChanged`.

puf assigned samtstern Jan 9, 2017

puf changed the base branch from master to version-1.1.0-dev January 10, 2017 04:53

Merge branch 'version-1.1.0-dev' into puf-ondatachange

deb25cb

SUPERCILEX reviewed Jan 10, 2017

View reviewed changes

Added missing mRef field.

33253cb

SUPERCILEX reviewed Jan 10, 2017

View reviewed changes

Added whitespace around ternary operator, because the style guide is …

f715502

…picky about that.

SUPERCILEX reviewed Jan 10, 2017

View reviewed changes

SUPERCILEX mentioned this pull request Jan 10, 2017

Add string resource to puf-ondatachange #507

Merged

Add string resource to puf-ondatachange (#507)

e2cc00c

puf added 2 commits January 10, 2017 12:01

Added whitespace after cast, because the style guide is picky about t…

a2cafb9

…hat.

Merge remote-tracking branch 'origin/puf-ondatachange' into puf-ondat…

e35c7d9

…achange

samtstern merged commit dd6dd6a into version-1.1.0-dev Jan 10, 2017

samtstern added this to the 1.1.0 milestone Jan 10, 2017

samtstern deleted the puf-ondatachange branch June 15, 2017 15:37

Conversation

puf commented Jan 3, 2017

Uh oh!

puf commented Jan 3, 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!

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 9, 2017

Uh oh!

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

Uh oh!

Uh oh!

samtstern commented Jan 9, 2017

Uh oh!

puf commented Jan 10, 2017

Uh oh!

puf commented Jan 10, 2017

Uh oh!

SUPERCILEX commented Jan 10, 2017

Uh oh!

googlebot commented Jan 10, 2017

Uh oh!

SUPERCILEX commented Jan 10, 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 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!

samtstern commented Jan 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SUPERCILEX commented Jan 9, 2017 •

edited

Loading