Fix small mistakes in FirebaseIndexArray #412
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SUPERCILEX
commented
Nov 21, 2016
| index++; | ||
| } | ||
| } | ||
| return index; | ||
| } | ||
|
|
||
| private boolean isMatch(int index, String key) { | ||
| return index >= 0 && index < getCount() && getItem(index).getKey().equals(key); | ||
| return index >= 0 && index < getCount() && mDataSnapshots.get(index).getKey().equals(key); |
There was a problem hiding this comment.
We should have never used getItem() here because it's public api and anyone overriding that method will break our implementation.
| if (isMatch(index, key)) { | ||
| mDataSnapshots.remove(index); | ||
| notifyChangedListeners(OnChangedListener.EventType.REMOVED, index); | ||
| } else { | ||
| Log.w(TAG, "Key not found at ref: " + snapshot.getRef()); |
There was a problem hiding this comment.
Only log the warning if the data wasn't in our list. Example: if people delete the data ref before the index ref, the warning would have been shown which is kinda pointless.
SUPERCILEX
commented
Nov 21, 2016
| @@ -41,7 +42,7 @@ | |||
| */ | |||
| public FirebaseIndexListAdapter(Activity activity, | |||
| Class<T> modelClass, | |||
| int modelLayout, | |||
| @LayoutRes int modelLayout, | |||
There was a problem hiding this comment.
I also just noticed that we never had an @Res annotation on our layouts so I added that too.
|
This LGTM, thanks! |
|
Awesome! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @samtstern, while working on #411, I noticed a few things that slipped past me so this is a super small PR to fix them.