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

[Woo POS] Make POS aggregate model Observable #15059

Merged
merged 4 commits into from
Feb 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 62 additions & 34 deletions WooCommerce/Classes/POS/Models/PointOfSaleAggregateModel.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Foundation
import Combine
import Observation

import protocol Yosemite.POSOrderableItem
import protocol WooFoundation.Analytics
Expand Down Expand Up @@ -38,24 +39,24 @@ protocol PointOfSaleAggregateModelProtocol {
}

@available(iOS 17.0, *)
class PointOfSaleAggregateModel: ObservableObject, PointOfSaleAggregateModelProtocol {
@Published private(set) var orderStage: PointOfSaleOrderStage = .building

@Published private(set) var cardReaderConnectionStatus: CardPresentPaymentReaderConnectionStatus = .disconnected
@Published private(set) var paymentState: PointOfSalePaymentState
@Published var cardPresentPaymentAlertViewModel: PointOfSaleCardPresentPaymentAlertType?
@Published private(set) var cardPresentPaymentInlineMessage: PointOfSaleCardPresentPaymentMessageType?
@Published var cardPresentPaymentOnboardingViewModel: CardPresentPaymentsOnboardingViewModel?
@Observable final class PointOfSaleAggregateModel: PointOfSaleAggregateModelProtocol {
private(set) var orderStage: PointOfSaleOrderStage = .building

private(set) var cardReaderConnectionStatus: CardPresentPaymentReaderConnectionStatus = .disconnected
private(set) var paymentState: PointOfSalePaymentState
var cardPresentPaymentAlertViewModel: PointOfSaleCardPresentPaymentAlertType?
private(set) var cardPresentPaymentInlineMessage: PointOfSaleCardPresentPaymentMessageType?
var cardPresentPaymentOnboardingViewModel: CardPresentPaymentsOnboardingViewModel?
private var onOnboardingCancellation: (() -> Void)?

@Published private(set) var itemsViewState: ItemsViewState = ItemsViewState(containerState: .loading,
itemsStack: ItemsStackState(root: .loading([]),
itemStates: [:]))
private(set) var itemsViewState: ItemsViewState = ItemsViewState(containerState: .loading,
itemsStack: ItemsStackState(root: .loading([]),
itemStates: [:]))

@Published private(set) var cart: [CartItem] = []
private(set) var cart: [CartItem] = []

@Published private(set) var orderState: PointOfSaleOrderState = .idle
@Published private var internalOrderState: PointOfSaleInternalOrderState = .idle
private(set) var orderState: PointOfSaleOrderState = .idle
private var internalOrderState: PointOfSaleInternalOrderState = .idle

private let itemsController: PointOfSaleItemsControllerProtocol

Expand Down Expand Up @@ -90,7 +91,10 @@ class PointOfSaleAggregateModel: ObservableObject, PointOfSaleAggregateModelProt
@available(iOS 17.0, *)
extension PointOfSaleAggregateModel {
private func publishItemsViewState() {
itemsController.itemsViewStatePublisher.assign(to: &$itemsViewState)
itemsController.itemsViewStatePublisher.sink { [weak self] state in
self?.itemsViewState = state
}
.store(in: &cancellables)
}

@MainActor
Expand Down Expand Up @@ -155,8 +159,11 @@ extension PointOfSaleAggregateModel {
@available(iOS 17.0, *)
extension PointOfSaleAggregateModel {
private func publishCardReaderConnectionStatus() {
// When adopting Observable, we can use `assign(to: on:)` here instead
cardPresentPaymentService.readerConnectionStatusPublisher.assign(to: &$cardReaderConnectionStatus)
cardPresentPaymentService.readerConnectionStatusPublisher
.sink(receiveValue: { [weak self] connectionStatus in
self?.cardReaderConnectionStatus = connectionStatus
})
.store(in: &cancellables)
}

func connectCardReader() {
Expand All @@ -177,7 +184,7 @@ extension PointOfSaleAggregateModel {
/// e.g. when the TotalsView goes offscreen.
private func startPaymentWhenCardReaderConnected() async {
guard case .connected = cardReaderConnectionStatus else {
return startPaymentOnCardReaderConnection = $cardReaderConnectionStatus
return startPaymentOnCardReaderConnection = cardPresentPaymentService.readerConnectionStatusPublisher
.filter { status in
switch status {
case .connected:
Expand Down Expand Up @@ -259,17 +266,19 @@ extension PointOfSaleAggregateModel {
await collectCardPayment()
}

private func setupReaderReconnectionObservation() {
$orderStage.sink(receiveValue: { [weak self] stage in
@Sendable private func setupReaderReconnectionObservation() {
withObservationTracking { [weak self] in
guard let self else { return }
switch stage {
case .building:
cancelCardReaderPreparation()
case .finalizing:
observeReaderReconnection()
switch orderStage {
case .building:
cancelCardReaderPreparation()
case .finalizing:
observeReaderReconnection()
}
})
.store(in: &cancellables)
} onChange: { [weak self] in
guard let self else { return }
DispatchQueue.main.async(execute: setupReaderReconnectionObservation)
}
}

private func cancelCardReaderPreparation() {
Expand All @@ -279,7 +288,7 @@ extension PointOfSaleAggregateModel {
}

private func observeReaderReconnection() {
cardReaderDisconnection = $cardReaderConnectionStatus
cardReaderDisconnection = cardPresentPaymentService.readerConnectionStatusPublisher
.filter({ $0 == .disconnected })
.sink { [weak self] _ in
Task { @MainActor [weak self] in
Expand Down Expand Up @@ -321,13 +330,19 @@ private extension PointOfSaleAggregateModel {
}
return alertType
}
.assign(to: &$cardPresentPaymentAlertViewModel)
.sink(receiveValue: { [weak self] alertType in
self?.cardPresentPaymentAlertViewModel = alertType
})
.store(in: &cancellables)

cardPresentPaymentService.paymentEventPublisher
.map { [weak self] event -> PointOfSaleCardPresentPaymentMessageType? in
self?.mapCardPresentPaymentEventToMessageType(event)
}
.assign(to: &$cardPresentPaymentInlineMessage)
.sink(receiveValue: { [weak self] message in
self?.cardPresentPaymentInlineMessage = message
})
.store(in: &cancellables)

cardPresentPaymentService.paymentEventPublisher
.compactMap { [weak self] paymentEvent -> PointOfSalePaymentState? in
Expand All @@ -338,7 +353,10 @@ private extension PointOfSaleAggregateModel {

return newPaymentState
}
.assign(to: &$paymentState)
.sink(receiveValue: { [weak self] paymentState in
self?.paymentState = paymentState
})
.store(in: &cancellables)

cardPresentPaymentService.paymentEventPublisher
.map { [weak self] event -> CardPresentPaymentsOnboardingViewModel? in
Expand All @@ -349,7 +367,10 @@ private extension PointOfSaleAggregateModel {
onOnboardingCancellation = onCancel
return viewModel
}
.assign(to: &$cardPresentPaymentOnboardingViewModel)
.sink(receiveValue: { [weak self] onboardingViewModel in
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think some of the memory leak issues could get resolved this way? I treated assign as a syntaxic sugar to sink but some of our memory debugger observations are pointing to these assignments 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly... but probably not. assign(to: on:) keeps a strong reference though, perhaps it's worth looking for that deeper in the codebase?

If you instead implemented MyModel with assign(to: lastUpdated, on: self), storing the returned AnyCancellable instance could cause a reference cycle, because the Subscribers.Assign subscriber would hold a strong reference to self. Using assign(to:) solves this problem.
assign(to:) docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staskus Replacing the publishers and associated sink calls with simple properties might help (see my other comment) but it probably won't be practical to break it for the Payments code, because that's used right down at the StripeCardReaderService level.

self?.cardPresentPaymentOnboardingViewModel = onboardingViewModel
})
.store(in: &cancellables)
}

/// Maps PaymentEvent to POSMessageType and annonates additional information if necessary
Expand Down Expand Up @@ -413,9 +434,16 @@ extension PointOfSaleAggregateModel {
func publishOrderState() {
orderController.orderStatePublisher
.map { $0.externalState }
.assign(to: &$orderState)
.sink(receiveValue: { [weak self] orderState in
self?.orderState = orderState
})
.store(in: &cancellables)

orderController.orderStatePublisher.assign(to: &$internalOrderState)
orderController.orderStatePublisher
.sink(receiveValue: { [weak self] internalOrderState in
self?.internalOrderState = internalOrderState
})
.store(in: &cancellables)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import SwiftUI
@available(iOS 17.0, *)
struct CardReaderConnectionStatusView: View {
@Environment(\.posBackgroundAppearance) var backgroundAppearance
@EnvironmentObject var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
@ScaledMetric private var scale: CGFloat = 1.0
@Environment(\.isEnabled) var isEnabled

Expand Down
4 changes: 2 additions & 2 deletions WooCommerce/Classes/POS/Presentation/CartView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import SwiftUI

@available(iOS 17.0, *)
struct CartView: View {
@EnvironmentObject private var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
private let viewHelper = CartViewHelper()

@Environment(\.floatingControlAreaSize) var floatingControlAreaSize: CGSize
Expand Down Expand Up @@ -311,6 +311,6 @@ private extension CartView {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
return CartView()
.environmentObject(posModel)
.environment(posModel)
}
#endif
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Yosemite
struct ChildItemList: View {
private let parentItem: POSItem
private let title: String
@EnvironmentObject private var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
@Environment(\.dismiss) private var dismiss

private var state: ItemListState {
Expand Down Expand Up @@ -167,7 +167,7 @@ private extension ChildItemList {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
return ChildItemList(parentItem: parentItem, title: parentProduct.name)
.environmentObject(posModel)
.environment(posModel)
}

@available(iOS 17.0, *)
Expand All @@ -191,7 +191,7 @@ private extension ChildItemList {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
return ChildItemList(parentItem: parentItem, title: parentProduct.name)
.environmentObject(posModel)
.environment(posModel)
}

#endif
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import struct Yosemite.POSVariableParentProduct
@available(iOS 17.0, *)
struct ItemList<HeaderView: View>: View {
@Environment(\.floatingControlAreaSize) private var floatingControlAreaSize: CGSize
@EnvironmentObject var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
@StateObject private var infiniteScrollTriggerDeterminer = ThresholdInfiniteScrollTriggerDeterminer()

let state: ItemListState
Expand Down Expand Up @@ -80,7 +80,7 @@ private enum Constants {
private struct ItemListRow: View {
let item: POSItem
let analytics: Analytics = ServiceLocator.analytics
@EnvironmentObject var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel

var body: some View {
switch item {
Expand Down Expand Up @@ -156,7 +156,7 @@ private extension ItemListRow {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
ItemList(state: .loading([]))
.environmentObject(posModel)
.environment(posModel)
}

#endif
6 changes: 3 additions & 3 deletions WooCommerce/Classes/POS/Presentation/ItemListView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import protocol Yosemite.POSOrderableItem
struct ItemListView: View {
@Environment(\.dynamicTypeSize) private var dynamicTypeSize

@EnvironmentObject var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel

@State private var showSimpleProductsModal: Bool = false
private var itemListState: ItemListState {
Expand Down Expand Up @@ -323,7 +323,7 @@ private extension ItemListView {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
return ItemListView()
.environmentObject(posModel)
.environment(posModel)
}

@available(iOS 17.0, *)
Expand All @@ -333,7 +333,7 @@ private extension ItemListView {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
return ItemListView()
.environmentObject(posModel)
.environment(posModel)
}

#endif
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import SwiftUI
@available(iOS 17.0, *)
struct POSFloatingControlView: View {
@Environment(\.posBackgroundAppearance) var backgroundAppearance
@EnvironmentObject private var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
@Environment(\.colorScheme) var colorScheme
@Environment(\.horizontalSizeClass) private var horizontalSizeClass
@Binding private var showExitPOSModal: Bool
Expand Down
4 changes: 2 additions & 2 deletions WooCommerce/Classes/POS/Presentation/PaymentButtons.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import SwiftUI

@available(iOS 17.0, *)
struct PaymentsActionButtons: View {
@EnvironmentObject private var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
@Binding var isShowingSendReceiptView: Bool
@Binding private(set) var isShowingReceiptNotEligibleBanner: Bool

Expand Down Expand Up @@ -112,6 +112,6 @@ private extension PaymentsActionButtons {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
PaymentsActionButtons(isShowingSendReceiptView: .constant(false), isShowingReceiptNotEligibleBanner: .constant(true))
.environmentObject(posModel)
.environment(posModel)
}
#endif
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import SwiftUI
struct PointOfSaleCollectCashView: View {
@Environment(\.colorScheme) var colorScheme
@Environment(\.dynamicTypeSize) var dynamicTypeSize
@EnvironmentObject private var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
@FocusState private var isTextFieldFocused: Bool

private let viewHelper = CollectCashViewHelper()
Expand Down Expand Up @@ -230,6 +230,6 @@ private extension PointOfSaleCollectCashView {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
PointOfSaleCollectCashView(orderTotal: "$1.23")
.environmentObject(posModel)
.environment(posModel)
}
#endif
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import SwiftUI

@available(iOS 17.0, *)
struct PointOfSaleDashboardView: View {
@EnvironmentObject private var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
@Environment(\.horizontalSizeClass) private var horizontalSizeClass

@State private var showExitPOSModal: Bool = false
Expand All @@ -11,6 +11,7 @@ struct PointOfSaleDashboardView: View {
@State private var floatingSize: CGSize = .zero

var body: some View {
@Bindable var posModel = posModel
ZStack(alignment: .bottomLeading) {
if case .regular = horizontalSizeClass {
switch posModel.itemsViewState.containerState {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import SwiftUI

@available(iOS 17.0, *)
struct PointOfSaleEntryPointView: View {
@StateObject private var posModel: PointOfSaleAggregateModel
@State private var posModel: PointOfSaleAggregateModel
@StateObject private var posModalManager = POSModalManager()
@Environment(\.horizontalSizeClass) private var horizontalSizeClass

Expand All @@ -19,13 +19,13 @@ struct PointOfSaleEntryPointView: View {
cardPresentPaymentService: cardPresentPaymentService,
orderController: orderController)

self._posModel = StateObject(wrappedValue: posModel)
self._posModel = State(wrappedValue: posModel)
}

var body: some View {
PointOfSaleDashboardView()
.environmentObject(posModalManager)
.environmentObject(posModel)
.environment(posModel)
.onAppear {
onPointOfSaleModeActiveStateChange(true)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import class WordPressShared.EmailFormatValidator

@available(iOS 17.0, *)
struct POSSendReceiptView: View {
@EnvironmentObject private var posModel: PointOfSaleAggregateModel
@Environment(PointOfSaleAggregateModel.self) private var posModel
@Environment(\.dynamicTypeSize) var dynamicTypeSize
@State private var textFieldInput: String = ""
@State private var isLoading: Bool = false
Expand Down Expand Up @@ -170,6 +170,6 @@ private extension POSSendReceiptView {
cardPresentPaymentService: CardPresentPaymentPreviewService(),
orderController: PointOfSalePreviewOrderController())
POSSendReceiptView(isShowingSendReceiptView: .constant(true))
.environmentObject(posModel)
.environment(posModel)
}
#endif
Loading