Skip to content
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

Nh/refresh send realmchanged #2319

Closed
wants to merge 9 commits into from
Closed

Conversation

nhachicha
Copy link
Collaborator

This PR is the continuation of the wok done in #2233 (delay notification)

This will make refresh send a REALM_CHANGE instead of advancing the Realm. This adds more consistency and make it more easy to reason about async queries/update, as now all the changes are processed via the Looper.

Note: the behaviour of refresh is unchanged for non-Looper thread

@nhachicha
Copy link
Collaborator Author

@realm/java

@Test
public void processLocalListenersAfterRefresh() throws InterruptedException {
public void processRefreshLocalListenersAfterLooperQueueStart() throws Throwable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be simplified immensely using @RunTestInLooperThread?

@Test
@RunTestInLooperThread
public void test() {
// Setup listeners and stuff. 
realm.refresh()
// Verify that listeners haven't been called yet
}

@beeender
Copy link
Contributor

After calling Realm.refresh(), what I expect is the Realm instance wii be updated to the latest version immediately, then I could do some other works following the Realm.refresh() in the same UI loop.

If it just send the REALM_CHANGED to the handler, then it doesn't make too much sense to be called in the looper thread. Because of anyway the REALM_CHANGED will be triggered in the following loops. I don't think it would be a helpful API anymore.

@nhachicha
Copy link
Collaborator Author

I agree the name refresh might suggest an immediate action which could be confusion, maybe renaming it to something like requestRefresh may alleviate the confusion?

@cmelchior
Copy link
Contributor

But I think that raises the question of why even having it then? Because a refresh will only trigger change listeners if something actually changed, and if something changed a REALM_CHANGED event would already be in the queue?

@nhachicha
Copy link
Collaborator Author

It make sense to remove it then, if we have another mechanism for non-Looper thread waitForChanges?

@cmelchior
Copy link
Contributor

Yes, I think it would be the natural consequence if we don't want to have a Realm.refresh(). I am not sure I can come up with a good use case to use it refresh() on the UI thread?

@cmelchior
Copy link
Contributor

@stk1m1
Tasks missing:

  • Revert change to refresh behaviour.
  • Deprecate refresh().
  • Add Realm.waitForChange() which blocks until a change is available, then advance the Realm and continue. It should not trigger RealmChangeListeners I think. See SharedGroup:wait_for_change() and Multiprocess support #1300

We should probably be careful with exactly how a thread is blocked using this method, we should as a minimum abort if the thread is interrupted.

@stk1m1 stk1m1 removed their assignment Mar 3, 2016
@cmelchior
Copy link
Contributor

This PR is replaced by #2386

@cmelchior cmelchior closed this Mar 9, 2016
@Zhuinden Zhuinden mentioned this pull request Jan 17, 2017
24 tasks
@beeender beeender deleted the nh/refresh_send_realmchanged branch January 18, 2017 03:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants