Update database access modifiers #297

SUPERCILEX · 2016-09-10T21:35:16Z

As I was going through the adapters for PR #276, I noticed that the access modifiers were all weird. Is there something I'm not getting or is this change ok?

SUPERCILEX · 2016-09-10T21:36:26Z

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

@@ -25,7 +25,7 @@
 * This class implements an array-like collection on top of a Firebase location.
 */
 class FirebaseArray implements ChildEventListener {
-    public interface OnChangedListener {


This one should definitely be changed, I don't know why it's not at least protected.

samtstern · 2016-09-12T14:53:50Z

@puf what do you think?

puf · 2016-09-12T17:45:04Z

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

@@ -51,10 +51,10 @@
 */
 public abstract class FirebaseListAdapter<T> extends BaseAdapter {

+    private Activity mActivity;


I've had some cases in the past where I wanted to use the activity and layout in my concrete implementation. I'm not sure if these still apply, but would prefer to keep these protected for now.

SUPERCILEX · 2016-09-12T18:16:01Z

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

-    protected int mLayout;
-    protected Activity mActivity;
+    Activity mActivity;
+    final Class<T> mModelClass;


Why is this final?

Pull latest changes

SUPERCILEX

I think everything is good now: easy subclassing, developer happiness, the usual. 😄

SUPERCILEX · 2016-10-04T00:46:02Z

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

@@ -24,8 +24,8 @@
 /**
 * This class implements an array-like collection on top of a Firebase location.
 */
-class FirebaseArray implements ChildEventListener {
-    public interface OnChangedListener {
+public class FirebaseArray implements ChildEventListener {


The class is now public so that people can extend it and everything is protected for the same reason.

Oh and I forgot to mention: abstraction from ArrayList<DataSnapshot> mSnapshots is still maintained with this new model.

SUPERCILEX · 2016-10-04T01:11:24Z

@TheCraftKid Thanks for catching that! I thought these methods were no modifier originally. Anyway, they're back to normal again.

MaciejCiemiega · 2016-10-04T08:39:32Z

I would also vote for it. It would allow developers to implement many usable sub classes that are otherwise not possible without copying the code, which is not an elegant solution...

puf

Sorry for not getting back sooner. Let's take the "make FirebaseArray public" topic to an issue and continue discussion of the other changes here (if needed).

puf · 2016-10-07T17:03:07Z

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

@@ -24,7 +24,7 @@
 /**
 * This class implements an array-like collection on top of a Firebase location.
 */
-class FirebaseArray implements ChildEventListener {
+public class FirebaseArray implements ChildEventListener {


I so far consider FirebaseArray to not be a part of the FirebaseUI public API. It's fine to discuss whether my consideration is valid, but that discussion is best had in an issue and not in a PR. See for example #206 and #337 (which I haven't responded to yet).

I reverted to making it no modifier. If this is the case, I think all of FirebaseArray's methods should also be no modifier since FirebaseArray will never be used outside of the package.

puf · 2016-10-07T17:05:18Z

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

-    FirebaseArray mSnapshots;
+    protected final Class<T> mModelClass;
+    protected int mLayout;
+    protected FirebaseArray mSnapshots;


The mSnapshots field should not be modified by subclasses, hence it should not be accessible. All the pertinent info of snapshots is already available via getItem(), getRef() and getRef().

puf · 2016-10-07T17:06:40Z

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

    protected Activity mActivity;
-    FirebaseArray mSnapshots;
+    protected final Class<T> mModelClass;


I'm more inclined to make mLayout private than to make mModelClass protected. The logic is the same: subclasses should not be modifying these fields as it may lead to unexpected behavior.

puf · 2016-10-07T17:07:38Z

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

-    Class<VH> mViewHolderClass;
-    FirebaseArray mSnapshots;
+    protected Class<VH> mViewHolderClass;
+    protected FirebaseArray mSnapshots;


Same as for the other adapter: the fields should not be accessible to subclasses, as those might otherwise modify them. The pertinent information from the fields should already be available through getters.

This does leave me wondering how multiple ViewHolders and models could be used. Maybe I'll mess around with that today.

puf · 2016-10-09T07:16:31Z

I would probably start with a new adapter class for that. The benefit of
reusing code that you get by inheriting from the existing
FirebaseRecyclerAdapter may not outweigh having to work around the
difference in implementation.

On Sat, Oct 8, 2016 at 11:11 PM Willie Chalmers III <
[email protected]> wrote:

@TheCraftKid commented on this pull request.

In
database/src/main/java/com/firebase/ui/database/FirebaseRecyclerAdapter.java
#297:
 protected int mModelLayout;
Class mViewHolderClass;

FirebaseArray mSnapshots;

protected Class mViewHolderClass;

protected FirebaseArray mSnapshots;
This does leave me wondering how multiple ViewHolders and models could be
used. Maybe I'll mess around with that today.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#297, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AA3w36aplKJapcqPAfKWq63IXyCxozmuks5qyAccgaJpZM4J51Xr
.

…-database-access-modifiers # 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

SUPERCILEX · 2016-11-02T00:22:22Z

@puf I've updated this PR since #276 was merged. What do you think?

samtstern · 2016-11-28T17:27:54Z

@puf this is a very old PR with a small footprint, looks like @SUPERCILEX even updated it recently after #276 was merged. Could you two make a yes/no decision on this and either merge or close it?

…ccess-modifiers

puf · 2016-11-29T15:24:26Z

lgtm, but apparently github has a special button for that now. :-)

SUPERCILEX · 2016-11-29T16:48:21Z

Awesome!

Update access modifiers

0deadf8

SUPERCILEX reviewed Sep 10, 2016
View reviewed changes

puf reviewed Sep 12, 2016
View reviewed changes

Update FirebaseListAdapter.java

329a5dc

SUPERCILEX reviewed Sep 12, 2016
View reviewed changes

SUPERCILEX and others added 6 commits September 12, 2016 11:16

Update FirebaseRecyclerAdapter.java

81b7da9

Merge pull request #15 from firebase/master

fa82073

Pull latest changes

Merge branch 'firebase-master' into update-database-access-modifiers

4b6550c

Cleanup

12760c1

Cleanup

ccea6ec

Cleanup

b818cf8

SUPERCILEX commented Oct 4, 2016

View reviewed changes

SUPERCILEX mentioned this pull request Oct 4, 2016

Support for joins #276

Merged

1 task

SUPERCILEX and others added 5 commits October 3, 2016 18:06

Cleanup

7033d77

Update FirebaseArray.java

14d1f8c

Update FirebaseArray.java

d81994b

Update FirebaseArray.java

3b90228

Update FirebaseArray.java

1ead7c3

puf suggested changes Oct 7, 2016

View reviewed changes

Revert

8ff8c06

SUPERCILEX added 2 commits November 1, 2016 17:04

Update access modifiers

f4f031b

SUPERCILEX changed the base branch from master to version-1.0.0-dev November 2, 2016 00:07

SUPERCILEX added 2 commits November 1, 2016 17:11

Cleanup

9c5f92b

More updated access modifiers

00f389e

SUPERCILEX added 3 commits November 2, 2016 08:40

More updated access modifiers

d2cec9d

Cleanup

22baf3d

Cleanup

8aeaa7c

SUPERCILEX changed the base branch from version-1.0.0-dev to master November 5, 2016 05:42

Merge branch 'master' into update-database-access-modifiers

81acc87

Merge remote-tracking branch 'firebase/master' into update-database-a…

e5ca921

…ccess-modifiers

puf approved these changes Nov 29, 2016

View reviewed changes

samtstern merged commit 8667f35 into firebase:master Nov 29, 2016

SUPERCILEX deleted the update-database-access-modifiers branch November 29, 2016 16:48

Update database access modifiers #297

Update database access modifiers #297

Conversation

SUPERCILEX commented Sep 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Sep 12, 2016

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!

SUPERCILEX commented Oct 4, 2016

Uh oh!

MaciejCiemiega commented Oct 4, 2016

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

puf commented Oct 9, 2016

@TheCraftKid commented on this pull request.

Uh oh!

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

Uh oh!

Uh oh!

samtstern commented Nov 28, 2016

Uh oh!

puf commented Nov 29, 2016

Uh oh!

SUPERCILEX commented Nov 29, 2016

Uh oh!

Uh oh!

SUPERCILEX commented Nov 2, 2016 •

edited

Loading