-
Notifications
You must be signed in to change notification settings - Fork 118
[Woo POS][Local Catalog] Add incremental sync triggers #16264
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
base: trunk
Are you sure you want to change the base?
Changes from 14 commits
052a5e2
a2d89cb
2e2560a
2bb944d
3cfabc2
5d88b5a
557db95
8d6c664
2e810f4
81af5ec
a49da86
97b2109
964662a
d7585bb
381fb22
bb7d5f6
ce0cf16
b182b62
a31e679
83944b5
c17e0bb
b9d209a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,19 +6,24 @@ import protocol Yosemite.POSObservableDataSourceProtocol | |||||||||
| import struct Yosemite.POSVariableParentProduct | ||||||||||
| import class Yosemite.GRDBObservableDataSource | ||||||||||
| import protocol Storage.GRDBManagerProtocol | ||||||||||
| import protocol Yosemite.POSCatalogSyncCoordinatorProtocol | ||||||||||
| import enum Yosemite.POSCatalogSyncError | ||||||||||
|
|
||||||||||
| /// Controller that wraps an observable data source for POS items | ||||||||||
| /// Uses computed state based on data source observations for automatic UI updates | ||||||||||
| @Observable | ||||||||||
| final class PointOfSaleObservableItemsController: PointOfSaleItemsControllerProtocol { | ||||||||||
| private let dataSource: POSObservableDataSourceProtocol | ||||||||||
| private let catalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol | ||||||||||
| private let siteID: Int64 | ||||||||||
|
|
||||||||||
| // Track which items have been loaded at least once | ||||||||||
| private var hasLoadedProducts = false | ||||||||||
| private var hasLoadedVariationsForCurrentParent = false | ||||||||||
|
|
||||||||||
| // Track current parent for variation state mapping | ||||||||||
| private var currentParentItem: POSItem? | ||||||||||
| private var refreshState: RefreshState = .idle | ||||||||||
|
|
||||||||||
| var itemsViewState: ItemsViewState { | ||||||||||
| ItemsViewState( | ||||||||||
|
|
@@ -32,22 +37,41 @@ final class PointOfSaleObservableItemsController: PointOfSaleItemsControllerProt | |||||||||
|
|
||||||||||
| init(siteID: Int64, | ||||||||||
| grdbManager: GRDBManagerProtocol, | ||||||||||
| currencySettings: CurrencySettings) { | ||||||||||
| currencySettings: CurrencySettings, | ||||||||||
| catalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol) { | ||||||||||
| self.siteID = siteID | ||||||||||
| self.dataSource = GRDBObservableDataSource( | ||||||||||
| siteID: siteID, | ||||||||||
| grdbManager: grdbManager, | ||||||||||
| currencySettings: currencySettings | ||||||||||
| ) | ||||||||||
| self.catalogSyncCoordinator = catalogSyncCoordinator | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // periphery:ignore - used by tests | ||||||||||
| init(dataSource: POSObservableDataSourceProtocol) { | ||||||||||
| init(siteID: Int64, | ||||||||||
| dataSource: POSObservableDataSourceProtocol, | ||||||||||
| catalogSyncCoordinator: POSCatalogSyncCoordinatorProtocol) { | ||||||||||
| self.siteID = siteID | ||||||||||
| self.dataSource = dataSource | ||||||||||
| self.catalogSyncCoordinator = catalogSyncCoordinator | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func loadItems(base: ItemListBaseItem) async { | ||||||||||
| switch base { | ||||||||||
| case .root: | ||||||||||
| // Refresh if there's an error or if items are empty after initial load | ||||||||||
| let shouldRefresh = { | ||||||||||
| if case .error = refreshState { | ||||||||||
| return true | ||||||||||
| } | ||||||||||
| return hasLoadedProducts && dataSource.productItems.isEmpty | ||||||||||
| }() | ||||||||||
|
|
||||||||||
| if shouldRefresh { | ||||||||||
| await refreshItems(base: base) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| dataSource.loadProducts() | ||||||||||
| hasLoadedProducts = true | ||||||||||
| case .parent(let parent): | ||||||||||
|
|
@@ -62,21 +86,38 @@ final class PointOfSaleObservableItemsController: PointOfSaleItemsControllerProt | |||||||||
| hasLoadedVariationsForCurrentParent = false | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Refresh if there's an error or if variations are empty after initial load | ||||||||||
| let shouldRefresh = { | ||||||||||
| if case .error = refreshState { | ||||||||||
| return true | ||||||||||
| } | ||||||||||
| return hasLoadedVariationsForCurrentParent && dataSource.variationItems.isEmpty | ||||||||||
| }() | ||||||||||
|
|
||||||||||
| if shouldRefresh { | ||||||||||
| await refreshItems(base: base) | ||||||||||
| } | ||||||||||
|
|
||||||||||
| dataSource.loadVariations(for: parentProduct) | ||||||||||
| hasLoadedVariationsForCurrentParent = true | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| func refreshItems(base: ItemListBaseItem) async { | ||||||||||
| switch base { | ||||||||||
| case .root: | ||||||||||
| dataSource.refresh() | ||||||||||
| case .parent(let parent): | ||||||||||
| guard case .variableParentProduct(let parentProduct) = parent else { | ||||||||||
| assertionFailure("Unsupported parent type for refreshing items: \(parent)") | ||||||||||
| return | ||||||||||
| refreshState = .loading | ||||||||||
|
|
||||||||||
| do { | ||||||||||
| try await catalogSyncCoordinator.performIncrementalSync(for: siteID) | ||||||||||
| refreshState = .idle | ||||||||||
| } catch let error as POSCatalogSyncError { | ||||||||||
| switch error { | ||||||||||
| case .syncAlreadyInProgress: | ||||||||||
| refreshState = .idle | ||||||||||
| default: | ||||||||||
| refreshState = .error(error) | ||||||||||
| } | ||||||||||
| dataSource.loadVariations(for: parentProduct) | ||||||||||
| } catch { | ||||||||||
| refreshState = .error(error) | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
@@ -108,12 +149,21 @@ private extension PointOfSaleObservableItemsController { | |||||||||
| return .initial | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Loading state - preserve existing items | ||||||||||
| if dataSource.isLoadingProducts { | ||||||||||
| // Loading state - preserve existing items (both for data loading and refresh) | ||||||||||
| if dataSource.isLoadingProducts || refreshState == .loading { | ||||||||||
|
||||||||||
| // Loading state - preserve existing items (both for data loading and refresh) | |
| if dataSource.isLoadingProducts || refreshState == .loading { | |
| // Loading state - preserve existing items (only use for data loading, refresh has a PTR spinner) | |
| if dataSource.isLoadingProducts && refreshState != .loading { |
We shouldn't show the loading cell if we're refreshing – the PTR indicator is sufficient, and changing the state breaks the refresh task anyway.
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.
I implemented it to support the error "retry" case, but we could live without it. It's a shame there's no way to manually show a PTR loading animation. I remember trying to replicate it without success.
Outdated
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.
| // Loading state - preserve existing items (both for data loading and refresh) | |
| if dataSource.isLoadingVariations || refreshState == .loading { | |
| // Loading state - preserve existing items (only use for data loading, refresh has a PTR spinner) | |
| if dataSource.isLoadingProducts && refreshState != .loading { |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import Foundation | ||
| import Observation | ||
|
|
||
| /// A helper method for continuously tracking observations on a value. | ||
| /// | ||
| func withObservationTracking<T: Sendable>(of value: @Sendable @escaping @autoclosure () -> T, execute: @Sendable @escaping (T) -> Void) { | ||
| Observation.withObservationTracking { | ||
| execute(value()) | ||
| } onChange: { | ||
| DispatchQueue.main.async { | ||
| withObservationTracking(of: value(), execute: execute) | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,7 +78,9 @@ private extension ChildItemList { | |
| .transition(.opacity) | ||
| .refreshable { | ||
| analyticsTracker.trackRefresh() | ||
| await itemsController.refreshItems(base: node) | ||
| await Task { | ||
| await itemsController.refreshItems(base: node) | ||
| }.value | ||
|
||
| } | ||
| } | ||
| } | ||
|
|
||
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.
What do you think about this being shared between root and variations? It feels like an opportunity to leak state between views by accident...
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 made it shared since the incremental sync itself is shared. There's no separate sync for specific parent views. How do you think I should structure it?
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.
Ah yes, I see. Makes sense.
The one niggle I have is that this can be out of sync with the view state, because Apple's code's in control of which PTR indicator is shown. If you PTR on a variation screen, then go back to a parent screen and scroll down to load more items, we'd be loading and refreshing at the same time. Which is kind of OK... but if we use the refresh state to decide to show a loading indicator, we could finish one load without finishing the incremental sync, and still show a loading indicator for the refresh.
If we remove the loading cell for refresh tasks it's less of an issue... and we deliberately don't show that for PTR in the existing implementation, as the spinner is enough.