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 support for suppressing notifications using closure-based write/transaction methods #6252

Merged
merged 5 commits into from
Sep 23, 2019
Merged

Add support for suppressing notifications using closure-based write/transaction methods #6252

merged 5 commits into from
Sep 23, 2019

Conversation

liamnichols
Copy link
Contributor

@liamnichols liamnichols commented Sep 18, 2019

Motivation

We currently use the closure-based Realm.write(_:) method in our current projects and have recently started needing to utilise the notification suppression functionality that exists on Realm.commitWrite(withoutNotifying:).

Since the closure-based APIs don't currently support this, we've added an extension internally to work around this however after digging through the original implementation PR, I noticed that it was mentioned that the exact same extension I was introducing had already been considered but not yet implemented - #4253 (comment)

I thought it might be worth giving it a go here since it would mean one less extension for us and also provide some use for other people as well in the longer run 🙂

Changes

I've done a few things in this PR, hopefully none of them are too significant. This change aims to bring the same functionality to RLMRealm as well as Realm.

The first commit in this PR slightly cleans up the previous implementation of -[RLMRealm commitWriteTransaction:] by calling through to -[RLMRealm commitWriteTransactionWithoutNotifying:error:] directly instead of just duplicating part of the implementation. Since this method is so similar to the one I'm about to introduce, I thought it would be worthwhile making the two methods behave in a consistent way.

The second commit then introduces the Objective C support by adding a new method:

- (BOOL)transactionWithBlock:(__attribute__((noescape)) void(^)(void))block withoutNotifying:(NSArray<RLMNotificationToken *> *)tokens error:(NSError **)error;

The signature here is a tad lengthy so please let me know if there is a way that I can improve it. It's been a while since I've touched Objective C so I'm not really up to date with recommended API design guidelines.

Anyway, this method does exactly what the existing -[RLMRealm transactionWithBlock:error:] was doing but instead it calls through to -[RLMRealm commitWriteTransactionWithoutNotifying:error:] when committing the write and the previous method will pass an empty array (@[]) for convenience.

I've also added a simple test to validate the behaviour.

The last commit then does similar to Swift's Realm object by updating the existing method to introduce a new argument at the beginning. I've also added another unit test here as well to verify the functionality.

API Compatibility

All changes here are additive and shouldn't break when users update to the latest version however there is a slight downside to this since to achieve this, I'd have to leave the block argument without a label which isn't that much of a deal, but it doesn't really follow API design guidelines in Swift as it's now the secondary argument.

I could change it from write(withoutNotifying:_:) to write(withoutNotifying:block:) however it would cause a compile error in the event that the block was declared within the braces such as the following:

try realm.write { ... } // Compiles
try realm.write({ ... }) // Error: Missing argument for parameter 'block' in call

I'd prefer to re-introduce the block label as I think its better matches api design guidelines but I might be wrong.. It seems like it did exist at one time and then was removed in #4078 ... I've left it unlabelled for now but something to think about, I'm not sure what the thoughts are on introducing breaking changes.

I guess sone other option could be to deprecate the old method signature like so:

@available(*, deprecated, message: "Renamed, use write(withoutNotifying:block:) instead")
public func write(_ block: (() throws -> Void)) throws {
    try write(block: block)
}

Documentation

The header documentation added in this PR is mostly explanations that existed on other methods but I think it covered everything... I also think it might be good to update the code snippet on https://realm.io/docs/swift/latest#interface-driven-writes however I can't see that in the repo so I'm guessing it's managed elsewhere

…gh to '-[RLMRealm commitWriteTransactionWithoutNotifying:error:]' with empty array ('@[]') in order to prevent duplicate implementations
…fying:error:]' method to pass ignored notification tokens and add unit test
@liamnichols liamnichols changed the title [Realm] Add support for suppressing notifications using closure-based write/transaction methods Add support for suppressing notifications using closure-based write/transaction methods Sep 18, 2019
@bmunkholm
Copy link
Contributor

@liamnichols Thanks a lot for the PR! Could I ask you t sign the CLA in case you haven't done that already?
Thanks!!

@liamnichols
Copy link
Contributor Author

No worries @bmunkholm! I think I already signed the CLA but have done it again just in case 👍

Btw I'm not sure how to do anything about those 17 issues with the CI checks but let me know if I can do anything to help

@bmunkholm
Copy link
Contributor

Awesome, I'll let @tgoyne review this.

@tgoyne
Copy link
Member

tgoyne commented Sep 19, 2019

The Cocoapods CI jobs don't work for PRs from a fork, so those are just spurious failures.

This mostly looks good. Leaving the block argument to realm.write() unnamed so that it isn't a breaking change is the right choice, and then we'll want to add the label as part of the next major version bump.

In obj-c we want to avoid having anything but an error out-parameter come after a block parameter, so the array of tokens needs to be the first parameter. transactionWithoutNotifying:block:error sounds funny to me, but I don't have better ideas.

@liamnichols
Copy link
Contributor Author

In obj-c we want to avoid having anything but an error out-parameter come after a block parameter, so the array of tokens needs to be the first parameter. transactionWithoutNotifying:block:error sounds funny to me, but I don't have better ideas.

Yeah I thought the same so I added it after, but can change it around then 👍🏼

Thanks for the clarification on everything else

…ifying:block:error:] based on feedback in PR
…g:block:] to omit the error parameter and return value
@liamnichols
Copy link
Contributor Author

@tgoyne:

Updated the argument order in ef0865a 👍

I've also added -[RLMRealm transactionWithoutNotifying:block:] offers a default value for the NSError argument (of nil) and doesn't expose the return value similar to the way that -[RLMRealm transactionWithBlock:] does in def8e68... Let me know what you think of this... I think it might provide some convenience but at the same time it might be a bit over the top...

@tgoyne tgoyne merged commit ce438e3 into realm:master Sep 23, 2019
@liamnichols liamnichols deleted the ln/block-based-write-and-suppress branch September 24, 2019 05:47
@liamnichols
Copy link
Contributor Author

Thanks all for merging these changes and thanks for all the other hard work going into Realm 👏🏼🙇‍♂️

@bmunkholm
Copy link
Contributor

bmunkholm commented Sep 24, 2019

@tgoyne @liamnichols This also requires an update to the changelog.md with due credits. And an update to docs @cbush.

@liamnichols
Copy link
Contributor Author

Not sure how the docs are handled but the Notifications > Interface Driven Writes section could also benefit by updating the insertItem() method body to the following:

func insertItem() throws {
    // Perform an interface-driven write on the main thread, making sure the change notification doesn't apply the change a second time
    try collection.realm!.write(withoutNotifying: [token]) {
        collection.insert(Item(), at: 0)
    }
    // And mirror it instantly in the UI instead
    tableView.insertRows(at: [IndexPath(row: 0, section: 0)], with: .automatic)
}

cc @bmunkholm, @cbush

@cbush
Copy link
Contributor

cbush commented Oct 1, 2019

Thanks @liamnichols. Docs updated.

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants