-
Notifications
You must be signed in to change notification settings - Fork 12
Changes need for integration into liveview-client-swiftui #81
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
Changes need for integration into liveview-client-swiftui #81
Conversation
crates/core/liveview-native-core-swift/Sources/LiveViewNativeCore/Support.swift
Outdated
Show resolved
Hide resolved
crates/core/liveview-native-core-swift/Sources/LiveViewNativeCore/Support.swift
Outdated
Show resolved
Hide resolved
crates/core/liveview-native-core-swift/Sources/LiveViewNativeCore/Support.swift
Outdated
Show resolved
Hide resolved
crates/core/liveview-native-core-swift/Sources/LiveViewNativeCore/Support.swift
Outdated
Show resolved
Hide resolved
crates/core/liveview-native-core-swift/Sources/LiveViewNativeCore/Support.swift
Outdated
Show resolved
Hide resolved
crates/core/liveview-native-core-swift/Sources/LiveViewNativeCore/Support.swift
Outdated
Show resolved
Hide resolved
crates/core/liveview-native-core-swift/Sources/LiveViewNativeCore/Support.swift
Outdated
Show resolved
Hide resolved
crates/core/liveview-native-core-swift/Sources/LiveViewNativeCore/Support.swift
Outdated
Show resolved
Hide resolved
if !children.isEmpty { | ||
if let node = children[children.count - 1].next() { | ||
children.append(node.children().makeIterator()) | ||
return node | ||
} else { | ||
children.removeLast() | ||
return self.next() | ||
} | ||
} else { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !children.isEmpty { | |
if let node = children[children.count - 1].next() { | |
children.append(node.children().makeIterator()) | |
return node | |
} else { | |
children.removeLast() | |
return self.next() | |
} | |
} else { | |
return nil | |
} | |
guard !children.isEmpty else { return nil } | |
if let node = children.last?.next() { | |
children.append(node.children().makeIterator()) | |
return node | |
} else { | |
children.removeLast() | |
return self.next() | |
} |
return try self.mergeFragmentJson(payload) | ||
} | ||
|
||
public func on(_ event: EventType, _ callback: @escaping (Document, NodeRef) -> ()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if we could have an AsyncStream
for this API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I refactored this a few times to fix a deadlock. When mergeFragmentJson
is called, it's locking a mutex for the Document on the rust side as it's mutating the document. Today, I pulled in latest main liveview-native/liveview-client-swiftui#1287 and found that recent changes to ObservedElement
caused the deadlock. This is in part because when the closure is called on an incremental edit update, elementChanged
which triggers the sink closure on the ObservedElement
which uses Document.subscript and Document.children which require read locks on the same mutex that's currently locked for editing. I wonder if an AsyncStream
for this API would help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elementChanged
could be refactored to publish the element, instead of just Void
. Then the child caching could be done on read instead of when the change occurs.
Related to: