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

Add write transactions which do not produce local notifications #3439

Closed
wants to merge 147 commits into
base: tg-changeset
from

Conversation

Projects
None yet
@tgoyne
Copy link
Member

tgoyne commented Apr 14, 2016

This is very WIP, but it needs some bikeshedding over the API and what to call it.

This address a problem we discovered with fine-grained notifications when the UI is updated before the data model, rather than the reactive approach of the model being the canonical source of truth that the UI merely displays. The simplest motivating use-case is UITableView reordering; when using the built-in functionality for dragging rows around the delegate is merely notified of changes already displayed in the UI, and re-applying those changes in the collection notification block after the Realm is committed will corrupt the order to the UITV. Because notifications are coalesced, users can't reasonably filter out these redundant updates themselves. and even if they could it would be very easy to get wrong.

This makes it so that when beginning a write transaction, a user can specify that the transaction should not produce notifications on the current thread, as the things they want to update with the notifications are already up-to-date. In addition, this variant of beginWriteTransaction blocks until all async notifications are ready and delivers them, which is needed to avoid having things getting really weird.

An alternative approach would be to instead let the user pass in the tokens for notification blocks which they want to suppress. This is more flexible and makes it work with commits on a different thread from the notified thread, but maybe would make the API even more complex.

@realm-ci

This comment has been minimized.

Copy link

realm-ci commented Apr 14, 2016

Thanks for the PR, @tgoyne! We'll need you to accept our Contributor License Agreement before we can merge your contributions.

Cheers,
The robots and humans at Realm

@astigsen

This comment has been minimized.

Copy link
Contributor

astigsen commented Apr 14, 2016

How does Apple's FetchResultsController deal with this problem (if it does at all)?

@jpsim

This comment has been minimized.

Copy link
Contributor

jpsim commented Apr 14, 2016

An API idea: passing in the notification tokens for which you want the transaction to skip.

func beginWrite(ignoringNotificationTokens: [NotificationToken] = [])
- (void)beginWriteTransactionIgnoringNotificationTokens:(NSArray<RLMNotificationToken *>)tokens;
@tgoyne

This comment has been minimized.

Copy link
Member Author

tgoyne commented Apr 14, 2016

How does Apple's FetchResultsController deal with this problem (if it does at all)?

http://stackoverflow.com/questions/1077568/how-to-implement-re-ordering-of-coredata-records

tl;dr is that it updates synchronously when you call save, so you can manually track whether or not a set of calls on the delegate are from user-driven changes. This doesn't really work with our model of async updates.

@astigsen

This comment has been minimized.

Copy link
Contributor

astigsen commented Apr 15, 2016

tl;dr is that it updates synchronously when you call save,so you can manually track whether or not a set of calls on the delegate are from user-driven changes.

That seem unsafe. What happens if changes happen on other threads while you save (and disable updates)? That could make the core data state update with changes from the other save while you have manually disabled updates.

@tgoyne tgoyne force-pushed the tg/changeset-skip branch from 882b3e1 to cdb7ce8 Apr 15, 2016

@tgoyne tgoyne force-pushed the tg-changeset branch 4 times, most recently from 42af5af to e48f8da Apr 15, 2016

tgoyne and others added some commits Apr 20, 2016

Merge pull request #3359 from realm/tg-changeset
Add change information to collection notifications
Fix the initial ref count for WeakRealmNotifier
Adding the run loop source to the run loop retains it, so the initial refcount
should be 0, not 1.

Fixes #3270.
Merge pull request #3459 from realm/tg/leak
Fix the initial ref count for WeakRealmNotifier
use -[RLMNotificationToken stop] in examples
rather than the deprecated -[RLMRealm removeNotification:]
Merge pull request #3437 from realm/mr-fail-address-space-exhausted
Set error if mmap address space is exhausted

tgoyne and others added some commits Apr 29, 2016

Merge pull request #3503 from realm/tg/timestamp
Update to core 0.100.0
Merge pull request #3529 from segiddins/patch-1
[CHANGELOG] Correct 0.101.0 version
Remove List::get_version_counter()
This was a temporary hack for list notifications which was made
unnecessary by find-grained notifications.
Merge pull request #3530 from realm/tg/list-version
Remove List::get_version_counter()
Fix `BETWEEN` queries that traverse `RLMArray`/`List` properties to
ensure that a single related object satisfies the `BETWEEN` criteria.

We previously allowed different objects in the array to satisfy the
lower and upper bounds, which is not consistent with how Foundation
evaluates predicates.
Only use the subquery codepath for key paths that traverse to-many li…
…nks.

The normal approach gives correct results when only unary links are
involved, and is more efficient than using subqueries.
Merge pull request #3523 from realm/mar/between-over-links
Fix BETWEEN queries that traverse List properties to ensure that a single related object satisfies the BETWEEN criteria

@tgoyne tgoyne force-pushed the tg/changeset-skip branch from cdb7ce8 to da9f93e May 5, 2016

@tgoyne tgoyne added S:On Hold and removed S:In Progress labels May 11, 2016

@beloso

This comment has been minimized.

Copy link

beloso commented May 18, 2016

Any updates on this?

@JadenGeller

This comment has been minimized.

Copy link
Contributor

JadenGeller commented Jul 26, 2016

Closing this since it's being tracked in #3665 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.