-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fine grained notification for RecyclerViewAdapter #83
Conversation
@@ -87,10 +87,12 @@ public void onChange(BaseRealm results) { | |||
private void addListener(@NonNull OrderedRealmCollection<T> data) { | |||
if (data instanceof RealmResults) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check for instance of RealmCollectionObservable instead? and collapse the two if sections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore this after removing this interface
- Refresh only the needed part in RecyclerViewAdapter acording to the given change set. - Change Timestamp to Counter and show an ordered results in the views. To show a better experience for recycler view. - Add a edit button to randomly delete/add objects in recycler view. - Remove deprecated methods.
1c09ba0
to
7f8e368
Compare
Currently crashes with:
When running |
@cmelchior I could reproduce this with Something wrong in SNAPSHOT build? |
Perhaps. I had to upload the SNAPSHOT from my local machine because CI didn't do it. Can you try running |
|
||
public RealmBaseAdapter(@Nullable OrderedRealmCollection<T> data) { | ||
if (data != null && !data.isManaged()) | ||
throw new IllegalStateException("Only use this adapter with managed list, " + | ||
"for un-managed lists you can just use the BaseAdapter"); | ||
this.adapterData = data; | ||
this.listener = new RealmChangeListener<BaseRealm>() { | ||
this.listener = new RealmChangeListener<Object>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be RealmChangeListener<OrderedRealmCollection<T>>
since addListener
can only accept RealmResults
or RealmList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I think it's OK to use raw type (RealmChangeListener
).
RealmRecyclerViewAdapter
also is using raw type.
@@ -85,12 +79,7 @@ public RealmRecyclerViewAdapter(@Nullable OrderedRealmCollection<T> data, boolea | |||
// Right now don't use generics, since we need maintain two different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to
@@ -85,12 +79,7 @@ public RealmRecyclerViewAdapter(@Nullable OrderedRealmCollection<T> data, boolea | |||
// Right now don't use generics, since we need maintain two different | |||
// types of listeners until RealmList is properly supported. | |||
// See https://github.com/realm/realm-java/issues/989 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the above assumption still valid? realm/realm-java#989 is closed now?
@@ -22,11 +25,19 @@ buildscript { | |||
} | |||
} | |||
|
|||
// Don't cache SNAPSHOT (changing) dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remember to remove when releasing
public class Counter extends RealmObject { | ||
public static final String FIELD_COUNT = "count"; | ||
|
||
private static AtomicInteger integerCounter = new AtomicInteger(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why static? can't we use a member counter with @Ignore
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is in order to have a global counter while the app is running so the list will have ever increasing numbers.
} | ||
|
||
public void setTimeStamp(String timeStamp) { | ||
this.timeStamp = timeStamp; | ||
public void setAndIncrease() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think increment
is probably a more reflective name, as we're not setting any value (method doesn't take args)
realm.executeTransactionAsync(new Realm.Transaction() { | ||
@Override | ||
public void execute(Realm realm) { | ||
Random rand = new Random(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Random
is a bit expensive to init. better create it as a member I guess.
RealmResults<Counter> results = realm.where(Counter.class).findAllSorted(Counter.FIELD_COUNT); | ||
|
||
// Duplicate some existing entries. | ||
int countToDup = results.size() > 3 ? 3 : results.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
countToDup
max is 3 (just double checking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, refactored to named variable for clarity
CHANGELOG.md
Outdated
|
||
* Removed `RealmBaseAdapter(@Nonnull Context context, @Nullable OrderedRealmCollection<T> data)`. | ||
* Removed `RealmRecyclerViewAdapter(@NonNull Context context, @Nullable OrderedRealmCollection<T> data, boolean autoUpdate)`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that removing RealmBaseAdapter#inflater
, RealmBaseAdapter#context
, RealmRecyclerViewAdapter#inflater
and RealmRecyclerViewAdapter#context
are also breaking changes.
@@ -1,3 +1,14 @@ | |||
## 2.0.0 (YYYY-MM-DD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should state supporting Realm-Java version at the beginning of each version entry in Changelog
now 3.x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea.
realmList.realm.handlerController.addChangeListenerAsWeakReference(listener); | ||
RealmList<T> list = (RealmList<T>) data; | ||
//noinspection unchecked | ||
list.addChangeListener((RealmChangeListener)listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need a space after cast
|
||
public RealmBaseAdapter(@Nullable OrderedRealmCollection<T> data) { | ||
if (data != null && !data.isManaged()) | ||
throw new IllegalStateException("Only use this adapter with managed list, " + | ||
"for un-managed lists you can just use the BaseAdapter"); | ||
this.adapterData = data; | ||
this.listener = new RealmChangeListener<BaseRealm>() { | ||
this.listener = new RealmChangeListener<Object>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I think it's OK to use raw type (RealmChangeListener
).
RealmRecyclerViewAdapter
also is using raw type.
// Duplicate some existing entries. | ||
int countToDup = results.size() > 3 ? 3 : results.size(); | ||
for (int i = 0; i < countToDup; i++) { | ||
int id = results.get(rand.nextInt((results.size()))).getCount(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why id
?
This value is shared among multiple items.
It does not seem id
.
@nhachicha @zaki50 PR updated |
No description provided.