Skip to content

[Woo POS] Avoid initial view model unwanted recreation #12799

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 23, 2024
Merged

[Woo POS] Avoid initial view model unwanted recreation #12799

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9649cc0
DI PointOfSaleDashboardViewModel from HubMenu
iamgabrielma May 21, 2024
90d5619
Present filter modal view on tap
iamgabrielma May 21, 2024
4a5ded6
Make pos vm StateObject
iamgabrielma May 23, 2024
4275f7d
Adjust init access to internal
iamgabrielma May 23, 2024
df0879b
Merge branch 'trunk' into task/pos-compose-viewmodel-outside-view
iamgabrielma May 23, 2024
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
1 change: 1 addition & 0 deletions WooCommerce/Classes/POS/Presentation/FilterView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ struct FilterView: View {
var body: some View {
Button("Filter") {
// TODO: https://github.com/woocommerce/woocommerce-ios/issues/12761
viewModel.showFilters()
}
.frame(maxWidth: .infinity, idealHeight: 120)
.font(.title2)
Expand Down
3 changes: 3 additions & 0 deletions WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ struct PointOfSaleDashboardView: View {
.sheet(isPresented: $viewModel.showsCardReaderSheet, content: {
CardReaderConnectionView(viewModel: viewModel.cardReaderConnectionViewModel)
})
.sheet(isPresented: $viewModel.showsFilterSheet, content: {
FilterView(viewModel: viewModel)
})
}
}

Expand Down
14 changes: 4 additions & 10 deletions WooCommerce/Classes/POS/Presentation/PointOfSaleEntryPointView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,15 @@
import SwiftUI

struct PointOfSaleEntryPointView: View {
// TODO:
// Temporary. DI proper product models once we have a data layer
private let viewModel: PointOfSaleDashboardViewModel = {
let products = POSProductFactory.makeFakeProducts()
let viewModel = PointOfSaleDashboardViewModel(products: products, cardReaderConnectionViewModel: .init(state: .scanningForReader(cancel: {})))

return viewModel
}()
@ObservedObject private var viewModel: PointOfSaleDashboardViewModel
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, would @StateObject here fix the issue? Personally, I feel like owning the view model within the POS boundary (as if an invisible framework) would be preferable, and this seems to be the goal of StateObject in the doc. The dependencies can be DI'ed through the view's initializer, and the view just does a simple initialization of the view model with the depenencies.

@ObservedObject view model works in the HubMenu because it's being held by the UIKit hosting controller which doesn't get recreated like in a SwiftUI view.

If @StateObject doesn't work out and the POS view model needs to be DI'ed to the POS view, maybe having HubMenuViewModel own it would be more consistent with the existing setup like InPersonPaymentsMenuViewModel.

Copy link
Contributor Author

@iamgabrielma iamgabrielma May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just wondering, would @StateObject here fix the issue? Personally, I feel like owning the view model within the POS boundary (as if an invisible framework) would be preferable, and this seems to be the goal of StateObject in the doc. The dependencies can be DI'ed through the view's initializer, and the view just does a simple initialization of the view model with the depenencies.

You mean the View declaring its own dependencies via a @StateObject rather than DI it from outside? So:

Suggested change
@ObservedObject private var viewModel: PointOfSaleDashboardViewModel
@StateObject private var viewModel = PointOfSaleDashboardViewModel(products: POSProductFactory.makeFakeProducts(),
cardReaderConnectionViewModel: .init(state: .connectingToReader))

It does seem to fix the problem as well, I'm guessing because of:

SwiftUI creates a new instance of the model object only once during the lifetime of the container that declares the state object. For example, SwiftUI doesn’t create a new instance if a view’s inputs change, but does create a new instance if the identity of a view changes

I like the idea of setting a POS boundary via using StateObject, and the benefits of dependency injection for testing purposes are not lost in this case since is just the initial POS state. I don't really know if there's any other trade-offs we should be aware of?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup could be just @StateObject private var viewModel: PointOfSaleDashboardViewModel and setting the view model in the view init given the dependencies.

I don't really know if there's any other trade-offs we should be aware of?

Good point, I can look more into this tomorrow. We've been using @StateObject in the code base in views that own a view model throughout the UI's lifetime.

Copy link
Contributor

Choose a reason for hiding this comment

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

After some more reading on the official doc and other posts, StateObject feels to me like the most appropriate place for the view to own the view model that's only created once. With a few notes:

  • The view model's dependencies shouldn't change, which I think is the case for POS - site ID, some data layer service implementation, analytics implementation (singleton in the app), etc.

Explicit state object initialization works well when the external data that the object depends on doesn’t change for a given instance of the object’s container.

If we do want the view model to be recreated again when some dependency changes, it looks like we can achieve this by binding the identity of the view that owns the StateObject view model to one or more parameters.

  • StateObject should be private in all cases, and devs need to be aware not to re-initialize it again after the first initialization because any following inits wouldn't work silently and could result in unexpected behavior

Lemme know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me! Updated on 4a5ded6 . Now the Hub doesn't know anything about the pos viewmodel, but is entirely driven internally by the point of entry.


private let hideAppTabBar: ((Bool) -> Void)

// Necessary to expose the View's entry point to WooCommerce
// Otherwise the current switch on `HubMenu` where this View is created
// will error with "No exact matches in reference to static method 'buildExpression'"
public init(hideAppTabBar: @escaping ((Bool) -> Void)) {
public init(viewModel: PointOfSaleDashboardViewModel, hideAppTabBar: @escaping ((Bool) -> Void)) {
self.viewModel = viewModel
self.hideAppTabBar = hideAppTabBar
}

Expand All @@ -32,6 +26,6 @@ struct PointOfSaleEntryPointView: View {

#if DEBUG
#Preview {
PointOfSaleEntryPointView(hideAppTabBar: { _ in })
PointOfSaleEntryPointView(viewModel: .defaultPreview(), hideAppTabBar: { _ in })
}
#endif
12 changes: 12 additions & 0 deletions WooCommerce/Classes/POS/ViewModels/PointOfSaleDashboardViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ final class PointOfSaleDashboardViewModel: ObservableObject {
@Published var productsInCart: [CartProduct] = []

@Published var showsCardReaderSheet: Bool = false
@Published var showsFilterSheet: Bool = false
@ObservedObject private(set) var cardReaderConnectionViewModel: CardReaderConnectionViewModel

init(products: [POSProduct],
Expand Down Expand Up @@ -70,4 +71,15 @@ final class PointOfSaleDashboardViewModel: ObservableObject {
func showCardReaderConnection() {
showsCardReaderSheet = true
}

func showFilters() {
showsFilterSheet = true
}
}

extension PointOfSaleDashboardViewModel {
// Helper function to populate SwifUI previews
static func defaultPreview() -> PointOfSaleDashboardViewModel {
PointOfSaleDashboardViewModel(products: [], cardReaderConnectionViewModel: .init(state: .connectingToReader))
}
}
14 changes: 8 additions & 6 deletions WooCommerce/Classes/ViewRelated/Hub Menu/HubMenu.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ struct HubMenu: View {
@ObservedObject private var iO = Inject.observer

@ObservedObject private var viewModel: HubMenuViewModel
@ObservedObject private var pointOfSaleViewModel: PointOfSaleDashboardViewModel

init(viewModel: HubMenuViewModel) {
init(viewModel: HubMenuViewModel, pointOfSaleViewModel: PointOfSaleDashboardViewModel) {
self.viewModel = viewModel
self.pointOfSaleViewModel = pointOfSaleViewModel
}

var body: some View {
Expand Down Expand Up @@ -162,7 +164,7 @@ private extension HubMenu {
case HubMenuViewModel.Customers.id:
CustomersListView(viewModel: .init(siteID: viewModel.siteID))
case HubMenuViewModel.PointOfSaleEntryPoint.id:
PointOfSaleEntryPointView(hideAppTabBar: { isHidden in
PointOfSaleEntryPointView(viewModel: pointOfSaleViewModel, hideAppTabBar: { isHidden in
AppDelegate.shared.setShouldHideTabBar(isHidden)
})
default:
Expand Down Expand Up @@ -386,17 +388,17 @@ private extension HubMenu {

struct HubMenu_Previews: PreviewProvider {
static var previews: some View {
HubMenu(viewModel: .init(siteID: 123, tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker()))
HubMenu(viewModel: .init(siteID: 123, tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker()), pointOfSaleViewModel: .defaultPreview())
.environment(\.colorScheme, .light)

HubMenu(viewModel: .init(siteID: 123, tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker()))
HubMenu(viewModel: .init(siteID: 123, tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker()), pointOfSaleViewModel: .defaultPreview())
.environment(\.colorScheme, .dark)

HubMenu(viewModel: .init(siteID: 123, tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker()))
HubMenu(viewModel: .init(siteID: 123, tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker()), pointOfSaleViewModel: .defaultPreview())
.previewLayout(.fixed(width: 312, height: 528))
.environment(\.sizeCategory, .accessibilityExtraExtraExtraLarge)

HubMenu(viewModel: .init(siteID: 123, tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker()))
HubMenu(viewModel: .init(siteID: 123, tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker()), pointOfSaleViewModel: .defaultPreview())
.previewLayout(.fixed(width: 1024, height: 768))
}
}
8 changes: 7 additions & 1 deletion WooCommerce/Classes/ViewRelated/Hub Menu/HubMenuViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Yosemite
/// Displays a grid view of all available menu in the "Menu" tab (eg. View Store, Reviews, Coupons, etc...)
final class HubMenuViewController: UIHostingController<HubMenu> {
private let viewModel: HubMenuViewModel
private let pointOfSaleViewModel: PointOfSaleDashboardViewModel
private let tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker

private var storePickerCoordinator: StorePickerCoordinator?
Expand All @@ -14,8 +15,13 @@ final class HubMenuViewController: UIHostingController<HubMenu> {
tapToPayBadgePromotionChecker: TapToPayBadgePromotionChecker) {
self.viewModel = HubMenuViewModel(siteID: siteID,
tapToPayBadgePromotionChecker: tapToPayBadgePromotionChecker)
// TODO:
// Temporary. DI proper product models once we have a data layer
self.pointOfSaleViewModel = PointOfSaleDashboardViewModel(products: POSProductFactory.makeFakeProducts(),
cardReaderConnectionViewModel: .init(state: .scanningForReader(cancel: {})))

self.tapToPayBadgePromotionChecker = tapToPayBadgePromotionChecker
super.init(rootView: HubMenu(viewModel: viewModel))
super.init(rootView: HubMenu(viewModel: viewModel, pointOfSaleViewModel: pointOfSaleViewModel))
configureTabBarItem()
rootView.switchStoreHandler = { [weak self] in
self?.presentSwitchStore()
Expand Down