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

Fix issue with ObservedResults and searchable #8114

Merged
merged 2 commits into from
Feb 15, 2023
Merged

Conversation

leemaguire
Copy link
Contributor

@leemaguire leemaguire commented Jan 27, 2023

Addresses issue where observation subscription is cancelled on initial view refresh.
This PR fixes up the SwiftUITests for iOS 16 too.
Fixes: #8096
Fixes: #8132

@leemaguire leemaguire self-assigned this Jan 27, 2023
@cla-bot cla-bot bot added the cla: yes label Jan 27, 2023
ActionType = "Xcode.IDEStandardExecutionActionsCore.ExecutionActionType.ShellScriptAction">
<ActionContent
title = "Run Script"
scriptText = "killall Simulator&#10;defaults write com.apple.iphonesimulator ConnectHardwareKeyboard -bool false&#10;">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the hardware keyboard option enabled will give a high chance of characters being missed when tests are writing to text fields.

@@ -237,6 +242,7 @@ private class ObservableStorage<ObservedType>: ObservableObject where ObservedTy
willSet {
if newValue != value {
objectWillChange.send()
objectWillChange.update(value: newValue)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When ObservedResults is setup the publisher is passed RLMResults<ResultType>.emptyDetached(). When the actual Results to be observed is ready we need to update the publisher to hold a reference to it. Otherwise receive() will never setup the ObservationSubscription again once the original token is invalidated.

Copy link
Collaborator

@dianaafanador3 dianaafanador3 left a comment

Choose a reason for hiding this comment

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

It would be nice to have a test which really test this particular use case. If I run the SwiftUI tests removing the update function, all the tests are completing successfully (running on ios16), so I cannot reproduce the original issue. The way the issue seems to be happenignwhen we update the realm without using the property wrapper ($observedResults.append(:)), or maybe I'm wrong.

@leemaguire leemaguire merged commit 21a11b1 into master Feb 15, 2023
@leemaguire leemaguire deleted the lm/swiftui_fixes branch February 15, 2023 18:56
@@ -966,7 +966,9 @@ public class UserPublisher: Publisher {
/// :nodoc:
public func receive<S>(subscriber: S) where S: Subscriber, S.Failure == Never, Output == S.Input {
let token = user.subscribe { _ in
_ = subscriber.receive(self.user)
DispatchQueue.main.async {
Copy link
Member

Choose a reason for hiding this comment

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

This is not a valid thing to do, and it's producing (correct) sendability warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert this. Is the correct approach to have the end user bounce to the main thread instead when calling something which mutates theUser?

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