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 change information to collection notifications #3359

Merged
merged 78 commits into from Apr 20, 2016
Merged

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Mar 22, 2016

This adds a third parameter to the notification block on Results/List/RLMResults/RLMArray/etc. that reports the indices in the collection which were deleted, inserted and modified. In addition, it skips sending the notification when the collection did not actually change, rather than notifying on every change to the relevant table(s). It does not include information about which property on the object at the given index changed or supply the old value at that index; for those KVO on each object must be used.

Most of the testing for this is done at the objectstore level, so see the corresponding PR there.

Closes #687. Closes #3435.

Things left to do:

  • Functionality
    • Change information for List
    • Change information for unsorted Results
    • Change information for sorted Results
    • Change information for Results derived from List
    • Finalize API
  • Testing
    • Write tests covering everything that can reasonably be tested in object-store
    • Obj-c integration tests
    • Swift integration tests
    • TSan on object store tests
    • TSan on realm-cocoa tests
    • Fuzz-test unsorted query changesets
    • Fuzz-test sorted query changesets
    • Fuzz-test unsorted linkview changesets
    • Fuzz-test sorted linkview changesets
  • Documentation
    • Write API docs
    • Update the guide
    • Release post
    • Update existing UITableView examples
    • Add an NSOutlineView example
  • Performance
    • Small changesets, Unsorted
    • Small changesets, Sorted
    • Small changesets, List
    • Many changesets, Unsorted
    • Many changesets, Sorted
    • Many changesets, List
    • Large changesets, Unsorted
    • Large changesets, Sorted
    • Large changesets, List

My current goal performance-wise is to have everything be fast enough that no one will need a way to opt-out of the changeset calculations, as that'd make the API unpleasant. Since most of the work is done on background threads and doesn't block the UI this should be easily attainable.

self.tableView.endUpdates()
break
case .Error(let err):
// An error occured while opening the Realm file on the background worker thread
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed this when copying this code for the documentation: 'occurred' needs another 'r' there.

@jpsim
Copy link
Contributor

jpsim commented Apr 12, 2016

Instead of having two separate addNotificationBlock overloads for NSIndexPath/Int block parameters, could we use platform-specific typealiases? e.g. Int for OS X, NSIndexPath for others?

Otherwise, our users will end up fighting Swift's type inference system:

image

vs

image

@tgoyne
Copy link
Member Author

tgoyne commented Apr 15, 2016

After thinking about it some more and looking at some code using the notifications, I'm now thinking that the obj-c NSIndexPath version should be [change insertionsInSection:0] (withSection?), and the Swift one should just be removed entirely.

Only generating paths for section 0 makes it not useful for grouped table views, and generalizing it doesn't really make it worse for the simple case. In Swift, you can just do insertions.map { NSIndexPath(section: 0, index: $0) }, so the whole thing is kinda pointless.

@tgoyne
Copy link
Member Author

tgoyne commented Apr 15, 2016

I tried using fine-grained notifications for the GroupedTableView example, and it turns out that the current design doesn't work for that when a single transaction modifies multiple sections. The problem is that all of the sections need to be updated within a single UITableView batch as it rechecks the sizes of all of the groups when you call endUpdates, but from within the notification block there's no good way to know if there's still more blocks which will be called. Some ideas for how to handle this:

  1. Don't, and just document it as an unsupported use-case
  2. Do something like let tokens = [results1, results2, results].addNotificationBlock { [changes] in ... } to make it possible to get all of the change notifications at once. I'm not really sure what the obj-c API would look like for this.
  3. Delay sending the Realm DidChange notification until after the async queries have been updated so that you can end the batch there
  4. Something less awful?

NotificationToken& operator=(NotificationToken const&) = delete;

private:
util::AtomicSharedPtr<_impl::BackgroundCollection> m_query;
Copy link
Contributor

Choose a reason for hiding this comment

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

m_query doesn't seem like an appropriate name for this member given that the thing it holds is described as a collection.

@bdash
Copy link
Contributor

bdash commented Apr 20, 2016

I love the newly-added comments explaining the move detection code!

👍

@tgoyne tgoyne merged commit d8f07f1 into master Apr 20, 2016
@tgoyne tgoyne removed the S:Review label Apr 20, 2016
@gastonmorixe
Copy link

Thank you @tgoyne and the team!

@jpsim jpsim mentioned this pull request Apr 21, 2016
@@ -201,10 +197,28 @@ void RealmCoordinator::clear_cache()
}
}

void RealmCoordinator::clear_all_caches()
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't appear to be used anywhere. Is it still worth keeping around?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants