Fixed a couple of bugs in the ConversationVC

• Fixed a bug where a disappearing voice message wouldn't stop playing
• Fixed a bug where the conversation screen was never freed from memory
• Fixed an issue with an earlier change where the openGroupManager cache would access itself while modifying itself causing a crash
pull/976/head
Morgan Pretty 1 month ago
parent 593c3792b2
commit b58a3f23cd

@ -1 +1 @@
Subproject commit d113e77c7df369eaa0fcc5dbeb3e1e249efeade9 Subproject commit b66e54b25805a3edbf5c09fafa2c486b18766383

@ -34,7 +34,7 @@ extension ConversationVC:
openSettingsFromTitleView() openSettingsFromTitleView()
} }
@objc func openSettingsFromTitleView() { @objc func openSettingsFromTitleView() {
switch self.titleView.currentLabelType { switch self.titleView.currentLabelType {
case .userCount: case .userCount:
if self.viewModel.threadData.threadVariant == .group || self.viewModel.threadData.threadVariant == .legacyGroup { if self.viewModel.threadData.threadVariant == .group || self.viewModel.threadData.threadVariant == .legacyGroup {

@ -214,7 +214,9 @@ final class ConversationVC: BaseVC, LibSessionRespondingViewController, Conversa
messageLabelAccessibilityLabel: "Outdated client banner text", messageLabelAccessibilityLabel: "Outdated client banner text",
height: 40 height: 40
) )
let result: InfoBanner = InfoBanner(info: info, dismiss: self.removeOutdatedClientBanner) let result: InfoBanner = InfoBanner(info: info, dismiss: { [weak self] in
self?.removeOutdatedClientBanner()
})
result.accessibilityLabel = "Outdated client banner" result.accessibilityLabel = "Outdated client banner"
result.isAccessibilityElement = true result.isAccessibilityElement = true
@ -1532,8 +1534,8 @@ final class ConversationVC: BaseVC, LibSessionRespondingViewController, Conversa
cell.update( cell.update(
with: cellViewModel, with: cellViewModel,
mediaCache: mediaCache, mediaCache: mediaCache,
playbackInfo: viewModel.playbackInfo(for: cellViewModel) { updatedInfo, error in playbackInfo: viewModel.playbackInfo(for: cellViewModel) { [weak self] updatedInfo, error in
DispatchQueue.main.async { [weak self] in DispatchQueue.main.async {
guard error == nil else { guard error == nil else {
let modal: ConfirmationModal = ConfirmationModal( let modal: ConfirmationModal = ConfirmationModal(
targetView: self?.view, targetView: self?.view,

@ -202,7 +202,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate {
deinit { deinit {
// Stop any audio playing when leaving the screen // Stop any audio playing when leaving the screen
audioPlayer.wrappedValue?.stop() stopAudio()
} }
// MARK: - Thread Data // MARK: - Thread Data
@ -442,6 +442,16 @@ public class ConversationViewModel: OWSAudioPlayerDelegate {
.filter { !$0.cellType.isPostProcessed } // Remove headers and other .filter { !$0.cellType.isPostProcessed } // Remove headers and other
.sorted { lhs, rhs -> Bool in lhs.timestampMs < rhs.timestampMs } .sorted { lhs, rhs -> Bool in lhs.timestampMs < rhs.timestampMs }
// If we are currently playing a voice message we should make sure it hasn't been deleted and, if
// it has, we should stop playing the audio (this is mostly to catch the case where a message is
// deleted due to a disappearing messages setting)
if
let audioPlayingInteractionId: Int64 = currentPlayingInteraction.wrappedValue,
!sortedData.contains(where: { $0.id == audioPlayingInteractionId })
{
self.stopAudio()
}
// We load messages from newest to oldest so having a pageOffset larger than zero means // We load messages from newest to oldest so having a pageOffset larger than zero means
// there are newer pages to load // there are newer pages to load
return [ return [
@ -957,6 +967,12 @@ public class ConversationViewModel: OWSAudioPlayerDelegate {
} }
public func playOrPauseAudio(for viewModel: MessageViewModel) { public func playOrPauseAudio(for viewModel: MessageViewModel) {
/// Ensure the `OWSAudioPlayer` logic is run on the main thread as it calls `MainAppContext.ensureSleepBlocking`
/// must run on the main thread (also there is no guarantee that `AVAudioPlayer` is thread safe so better safe than sorry)
guard Thread.isMainThread else {
return DispatchQueue.main.sync { [weak self] in self?.playOrPauseAudio(for: viewModel) }
}
guard guard
let attachment: Attachment = viewModel.attachments?.first, let attachment: Attachment = viewModel.attachments?.first,
let originalFilePath: String = attachment.originalFilePath, let originalFilePath: String = attachment.originalFilePath,
@ -1010,6 +1026,12 @@ public class ConversationViewModel: OWSAudioPlayerDelegate {
} }
public func speedUpAudio(for viewModel: MessageViewModel) { public func speedUpAudio(for viewModel: MessageViewModel) {
/// Ensure the `OWSAudioPlayer` logic is run on the main thread as it calls `MainAppContext.ensureSleepBlocking`
/// must run on the main thread (also there is no guarantee that `AVAudioPlayer` is thread safe so better safe than sorry)
guard Thread.isMainThread else {
return DispatchQueue.main.sync { [weak self] in self?.speedUpAudio(for: viewModel) }
}
// If we aren't playing the specified item then just start playing it // If we aren't playing the specified item then just start playing it
guard viewModel.id == currentPlayingInteraction.wrappedValue else { guard viewModel.id == currentPlayingInteraction.wrappedValue else {
playOrPauseAudio(for: viewModel) playOrPauseAudio(for: viewModel)
@ -1027,12 +1049,18 @@ public class ConversationViewModel: OWSAudioPlayerDelegate {
} }
public func stopAudioIfNeeded(for viewModel: MessageViewModel) { public func stopAudioIfNeeded(for viewModel: MessageViewModel) {
guard viewModel.id == currentPlayingInteraction.wrappedValue else { return } guard viewModel.id == currentPlayingInteraction.wrappedValue else { return }
audioPlayer.wrappedValue?.stop() stopAudio()
} }
public func stopAudio() { public func stopAudio() {
/// Ensure the `OWSAudioPlayer` logic is run on the main thread as it calls `MainAppContext.ensureSleepBlocking`
/// must run on the main thread (also there is no guarantee that `AVAudioPlayer` is thread safe so better safe than sorry)
guard Thread.isMainThread else {
return DispatchQueue.main.sync { [weak self] in self?.stopAudio() }
}
audioPlayer.wrappedValue?.stop() audioPlayer.wrappedValue?.stop()
currentPlayingInteraction.mutate { $0 = nil } currentPlayingInteraction.mutate { $0 = nil }
@ -1088,13 +1116,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate {
updatedPlaybackInfo?.updateCallback(updatedPlaybackInfo, nil) updatedPlaybackInfo?.updateCallback(updatedPlaybackInfo, nil)
// Clear out the currently playing record // Clear out the currently playing record
currentPlayingInteraction.mutate { $0 = nil } stopAudio()
audioPlayer.mutate {
// Note: We clear the delegate and explicitly set to nil here as when the OWSAudioPlayer
// gets deallocated it triggers state changes which cause UI bugs when auto-playing
$0?.delegate = nil
$0 = nil
}
// If the next interaction is another voice message then autoplay it // If the next interaction is another voice message then autoplay it
guard guard
@ -1121,7 +1143,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate {
playbackRate: 1 playbackRate: 1
) )
currentPlayingInteraction.mutate { $0 = nil } stopAudio()
playbackInfo.mutate { $0[interactionId] = updatedPlaybackInfo } playbackInfo.mutate { $0[interactionId] = updatedPlaybackInfo }
updatedPlaybackInfo?.updateCallback(updatedPlaybackInfo, AttachmentError.invalidData) updatedPlaybackInfo?.updateCallback(updatedPlaybackInfo, AttachmentError.invalidData)
} }

@ -39,16 +39,17 @@ public final class OpenGroupManager {
.defaulting(to: []) .defaulting(to: [])
// Update the cache state and re-create all of the pollers // Update the cache state and re-create all of the pollers
dependencies.caches.mutate(cache: .openGroupManager) { cache in let pollers: [OpenGroupAPI.PollerType] = dependencies.caches.mutate(cache: .openGroupManager) { cache in
cache.isPolling = true cache.isPolling = true
servers return servers.map { server -> OpenGroupAPI.PollerType in cache.getOrCreatePoller(for: server.lowercased()) }
.map { server -> OpenGroupAPI.PollerType in cache.getOrCreatePoller(for: server.lowercased()) } }
.forEach { poller in
// Now that the pollers have been created actually start them // Need to do this outside of the mutate as the poller will attempt to read from the
poller.stop() // Should never be an issue // 'openGroupManager' cache which is not allowed
poller.startIfNeeded(using: dependencies) pollers.forEach { poller in
} poller.stop() // Should never be an issue
poller.startIfNeeded(using: dependencies)
} }
} }
} }
@ -297,6 +298,8 @@ public final class OpenGroupManager {
$0.getOrCreatePoller(for: server.lowercased()) $0.getOrCreatePoller(for: server.lowercased())
} }
// Need to do this outside of the mutate as the poller will attempt to read from the
// 'openGroupManager' cache which is not allowed
poller.stop() poller.stop()
poller.startIfNeeded(using: dependencies) poller.startIfNeeded(using: dependencies)
} }

@ -76,7 +76,7 @@ public class Poller {
// on startup // on startup
let drainBehaviour: Atomic<SwarmDrainBehaviour> = Atomic(pollDrainBehaviour) let drainBehaviour: Atomic<SwarmDrainBehaviour> = Atomic(pollDrainBehaviour)
Threading.pollerQueue.async { [weak self] in pollerQueue.async { [weak self] in
guard self?.isPolling.wrappedValue[publicKey] != true else { return } guard self?.isPolling.wrappedValue[publicKey] != true else { return }
// Might be a race condition that the setUpPolling finishes too soon, // Might be a race condition that the setUpPolling finishes too soon,

@ -1492,14 +1492,13 @@ public final class JobQueue: Hashable {
// If the next job isn't scheduled in the future then just restart the JobRunner immediately // If the next job isn't scheduled in the future then just restart the JobRunner immediately
let secondsUntilNextJob: TimeInterval = (nextJobTimestamp - dependencies.dateNow.timeIntervalSince1970) let secondsUntilNextJob: TimeInterval = (nextJobTimestamp - dependencies.dateNow.timeIntervalSince1970)
let ceilSecondsUntilNextJob: Int = Int(ceil(Swift.abs(secondsUntilNextJob)))
guard secondsUntilNextJob > 0 else { guard secondsUntilNextJob > 0 else {
// Only log that the queue is getting restarted if this queue had actually been about to stop // Only log that the queue is getting restarted if this queue had actually been about to stop
if executionType != .concurrent || currentlyRunningJobIds.wrappedValue.isEmpty { if executionType != .concurrent || currentlyRunningJobIds.wrappedValue.isEmpty {
let timingString: String = (nextJobTimestamp == 0 ? let timingString: String = (nextJobTimestamp == 0 ?
"that should be in the queue" : "that should be in the queue" :
"scheduled \(ceilSecondsUntilNextJob) second\(ceilSecondsUntilNextJob == 1 ? "" : "s") ago" "scheduled \(.seconds(secondsUntilNextJob), unit: .s) ago"
) )
SNLog("[JobRunner] Restarting \(queueContext) immediately for job \(timingString)") SNLog("[JobRunner] Restarting \(queueContext) immediately for job \(timingString)")
} }
@ -1517,7 +1516,7 @@ public final class JobQueue: Hashable {
guard executionType == .concurrent || currentlyRunningJobIds.wrappedValue.isEmpty else { return } guard executionType == .concurrent || currentlyRunningJobIds.wrappedValue.isEmpty else { return }
// Setup a trigger // Setup a trigger
SNLog("[JobRunner] Stopping \(queueContext) until next job in \(ceilSecondsUntilNextJob) second\(ceilSecondsUntilNextJob == 1 ? "" : "s")") SNLog("[JobRunner] Stopping \(queueContext) until next job in \(.seconds(secondsUntilNextJob), unit: .s)")
nextTrigger.mutate { trigger in nextTrigger.mutate { trigger in
trigger?.invalidate() // Need to invalidate the old trigger to prevent a memory leak trigger?.invalidate() // Need to invalidate the old trigger to prevent a memory leak
trigger = Trigger.create(queue: self, timestamp: nextJobTimestamp, using: dependencies) trigger = Trigger.create(queue: self, timestamp: nextJobTimestamp, using: dependencies)

Loading…
Cancel
Save