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

[Product Creation AI v2] Select package image #13193

Merged
merged 17 commits into from
Jul 2, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,17 @@ import SwiftUI
final class AddProductWithAIContainerHostingController: UIHostingController<AddProductWithAIContainerView> {
private let viewModel: AddProductWithAIContainerViewModel
private var addProductFromImageCoordinator: AddProductFromImageCoordinator?
private var selectPackageImageCoordinator: SelectPackageImageCoordinator?

init(viewModel: AddProductWithAIContainerViewModel) {
self.viewModel = viewModel
super.init(rootView: AddProductWithAIContainerView(viewModel: viewModel))
rootView.onUsePackagePhoto = { [weak self] productName in
self?.presentPackageFlow(productName: productName)
}
viewModel.startingInfoViewModel.onPickPackagePhoto = { [weak self] source in
await self?.presentImageSelectionFlow(source: source)
}
Comment on lines +15 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

❓Could you explain why need to use async here? It seems to me that we're mixing the logic of presenting the selection flow with the selected image.

Would it be possible to separate the logic like what we did previously with the old flow? i.e. We keep the presentation of the image picker flow synchronous; and when an image is selected, we trigger a method in startingInfoViewModel with the selected image. Would that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain why need to use async here? It seems to me that we're mixing the logic of presenting the selection flow with the selected image.

We are using async because we will send the image view into a loading state and wait for the image to be received from this method. Code here

Would it be possible to separate the logic like what we did previously with the old flow? i.e. We keep the presentation of the image picker flow synchronous; and when an image is selected, we trigger a method in startingInfoViewModel with the selected image. Would that work?

I actually followed the pattern from AddProductFromImageCoordinator and made this method async. We use the same pattern in BlazeEditAdHostingController as well. Please correct me if I am misunderstanding things.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer a more naive implementation for separation of concerns. I'm also worried about the use of async because it could potentially come with complications when migrating to Swift 6. Please use your own judgement to decide whether to rewrite this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the input, Huong!

After considering this, I prefer to keep the implementation as is.

My thoughts are,

  1. Separation of concerns wise, SelectPackageImageCoordinator takes care of selecting and downloading the image. SelectPackageImageCoordinator also encapsulates the different paths of camera capture, device media library and WP Media library image selection. After initial selection, all those paths require async operations to prepare the MediaPickerImage. i.e. We present the selection flow and wait to get back a MediaPickerImage.
  2. Using await in ProductCreationAIStartingInfoViewModel makes it easy to maintain the previous image state and display a loading indicator until the image is ready.
  3. We can alter this if we find any complications while migrating to Swift 6. SelectPackageImageCoordinator along with BlazeEditAdHostingController (We will remove AddProductFromImageCoordinator while removing package flow code)

Please let me know if you still think we need to change this. I will add a subtask and work on this at the end of the project. 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with keeping this as-is for now, thanks for the explanation!

}

@available(*, unavailable)
Expand Down Expand Up @@ -57,6 +61,26 @@ private extension AddProductWithAIContainerHostingController {
self.addProductFromImageCoordinator = coordinator
coordinator.start()
}

/// Presents the image selection flow to select package photo
///
@MainActor
func presentImageSelectionFlow(source: MediaPickingSource) async -> MediaPickerImage? {
await withCheckedContinuation { continuation in
guard let navigationController else {
continuation.resume(returning: nil)
return
}
let coordinator = SelectPackageImageCoordinator(siteID: viewModel.siteID,
mediaSource: source,
sourceNavigationController: navigationController,
onImageSelected: { image in
continuation.resume(returning: image)
})
selectPackageImageCoordinator = coordinator
coordinator.start()
}
}
}

/// Container view for the product creation with AI flow.
Expand All @@ -81,7 +105,6 @@ struct AddProductWithAIContainerView: View {
case .productName:
if viewModel.featureFlagService.isFeatureFlagEnabled(.productCreationAIv2M1) {
ProductCreationAIStartingInfoView(viewModel: viewModel.startingInfoViewModel,
onUsePackagePhoto: onUsePackagePhoto,
onContinueWithFeatures: { features in
withAnimation {
viewModel.onProductFeaturesAdded(features: features)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
import Photos
import UIKit
import Yosemite
import WooFoundation

/// Controls navigation for the flow to select a package photo
///
final class SelectPackageImageCoordinator: Coordinator {
let navigationController: UINavigationController
private var mediaPickingCoordinator: MediaPickingCoordinator?
private let siteID: Int64
private let mediaSource: MediaPickingSource
private let productImageLoader: ProductUIImageLoader
private let onImageSelected: (MediaPickerImage?) -> Void

init(siteID: Int64,
mediaSource: MediaPickingSource,
sourceNavigationController: UINavigationController,
productImageLoader: ProductUIImageLoader = DefaultProductUIImageLoader(phAssetImageLoaderProvider: { PHImageManager.default() }),
onImageSelected: @escaping (MediaPickerImage?) -> Void) {
self.siteID = siteID
self.mediaSource = mediaSource
self.navigationController = sourceNavigationController
self.productImageLoader = productImageLoader
self.onImageSelected = onImageSelected
}

func start() {
let mediaPickingCoordinator = MediaPickingCoordinator(siteID: siteID,
imagesOnly: true,
allowsMultipleSelections: false,
flow: .readTextFromProductPhoto,
onCameraCaptureCompletion: { [weak self] asset, error in
guard let self else { return }

Task { @MainActor in
let image = await self.onCameraCaptureCompletion(asset: asset, error: error)
self.onImageSelected(image)
}
}, onDeviceMediaLibraryPickerCompletion: { [weak self] assets in
guard let self else { return }

Task { @MainActor in
let image = await self.onDeviceMediaLibraryPickerCompletion(assets: assets)
self.onImageSelected(image)
}
}, onWPMediaPickerCompletion: { [weak self] mediaItems in
guard let self else { return }

Task { @MainActor in
let image = await self.onWPMediaPickerCompletion(mediaItems: mediaItems)
self.onImageSelected(image)
}
})
self.mediaPickingCoordinator = mediaPickingCoordinator
mediaPickingCoordinator.showMediaPicker(source: mediaSource, from: navigationController)
}
}

// MARK: - Action handling for camera capture
//
private extension SelectPackageImageCoordinator {
@MainActor
func onCameraCaptureCompletion(asset: PHAsset?, error: Error?) async -> MediaPickerImage? {
guard let asset else {
displayErrorAlert(error: error)
return nil
}
return await requestImage(from: asset)
}
}

// MARK: Action handling for device media library picker
//
private extension SelectPackageImageCoordinator {
@MainActor
func onDeviceMediaLibraryPickerCompletion(assets: [PHAsset]) async -> MediaPickerImage? {
guard let asset = assets.first else {
return nil
}
return await requestImage(from: asset)
}
}

// MARK: - Action handling for WordPress Media Library
//
private extension SelectPackageImageCoordinator {
@MainActor
func onWPMediaPickerCompletion(mediaItems: [Media]) async -> MediaPickerImage? {
guard let media = mediaItems.first,
let image = try? await productImageLoader.requestImage(productImage: media.toProductImage)else {
return nil
}
return .init(image: image, source: .media(media: media))
}
}

// MARK: Error handling
//
private extension SelectPackageImageCoordinator {
@MainActor
func displayErrorAlert(error: Error?) {
let alertController = UIAlertController(title: Localization.ImageErrorAlert.title,
message: error?.localizedDescription,
preferredStyle: .alert)
let cancel = UIAlertAction(title: Localization.ImageErrorAlert.ok,
style: .cancel,
handler: nil)
alertController.addAction(cancel)
navigationController.present(alertController, animated: true)
}
}

private extension SelectPackageImageCoordinator {
@MainActor
func requestImage(from asset: PHAsset) async -> MediaPickerImage? {
await withCheckedContinuation { continuation in
// PHImageManager.requestImageForAsset can be called more than once.
var hasReceivedImage = false
productImageLoader.requestImage(asset: asset, targetSize: PHImageManagerMaximumSize, skipsDegradedImage: true) { image in
guard hasReceivedImage == false else {
return
}
continuation.resume(returning: .init(image: image, source: .asset(asset: asset)))
hasReceivedImage = true
}
}
}
}

private extension SelectPackageImageCoordinator {
enum Localization {
enum ImageErrorAlert {
static let title = NSLocalizedString(
"selectPackageImageCoordinator.imageErrorAlert.title",
value: "Unable to select image",
comment: "Title of the alert when there is an error selecting image"
)
static let ok = NSLocalizedString(
"selectPackageImageCoordinator.imageErrorAlert.ok",
value: "OK",
comment: "Dismiss button on the alert when there is an error selecting image"
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,11 @@ struct ProductCreationAIStartingInfoView: View {
@ScaledMetric private var scale: CGFloat = 1.0
@FocusState private var editorIsFocused: Bool

private let onUsePackagePhoto: (String?) -> Void
private let onContinueWithFeatures: (String) -> Void

init(viewModel: ProductCreationAIStartingInfoViewModel,
onUsePackagePhoto: @escaping (String?) -> Void,
onContinueWithFeatures: @escaping (String) -> Void) {
self.viewModel = viewModel
self.onUsePackagePhoto = onUsePackagePhoto
self.onContinueWithFeatures = onContinueWithFeatures
}

Expand Down Expand Up @@ -53,8 +50,22 @@ struct ProductCreationAIStartingInfoView: View {
.frame(height: Layout.dividerHeight)
.foregroundColor(Color(.separator))

readTextFromPhotoButton
.padding(insets: Layout.readTextFromPhotoButtonInsets)
switch viewModel.imageState {
case .empty:
readTextFromPhotoButton
.padding(insets: Layout.readTextFromPhotoButtonInsets)
case .loading, .success:
PackagePhotoView(imageState: viewModel.imageState,
onTapViewPhoto: {
viewModel.didTapViewPhoto()
},
onTapReplacePhoto: {
viewModel.didTapReplacePhoto()
},
onTapRemovePhoto: {
viewModel.didTapRemovePhoto()
})
}
}
.overlay(
RoundedRectangle(cornerRadius: Layout.cornerRadius).stroke(editorIsFocused ? Color(.brand) : Color(.separator))
Expand All @@ -72,6 +83,9 @@ struct ProductCreationAIStartingInfoView: View {
}
.background(Color(uiColor: .systemBackground))
}
.mediaSourceActionSheet(showsActionSheet: $viewModel.isShowingMediaPickerSourceSheet, selectMedia: { source in
viewModel.selectImage(from: source)
})
}
}

Expand All @@ -80,7 +94,6 @@ private extension ProductCreationAIStartingInfoView {
HStack {
Button {
viewModel.didTapReadTextFromPhoto()
// TODO: Launch photo selection flow
} label: {
HStack(alignment: .center, spacing: Layout.UsePackagePhoto.spacing) {
Image(systemName: Layout.UsePackagePhoto.cameraSFSymbol)
Expand Down Expand Up @@ -123,6 +136,92 @@ private extension ProductCreationAIStartingInfoView {
.disabled(viewModel.features.isEmpty)
}
}

private extension ProductCreationAIStartingInfoView {
struct PackagePhotoView: View {
@ScaledMetric private var scale: CGFloat = 1.0

let imageState: EditableImageViewState

let onTapViewPhoto: () -> Void
let onTapReplacePhoto: () -> Void
let onTapRemovePhoto: () -> Void

var body: some View {
HStack(alignment: .center, spacing: Layout.spacing) {
EditableImageView(imageState: imageState,
emptyContent: {})
.frame(width: Layout.packagePhotoSize * scale, height: Layout.packagePhotoSize * scale)
.cornerRadius(Layout.cornerRadius)

Text(Localization.photoSelected)
.bodyStyle()

Spacer()

Menu {
Button(Localization.viewPhoto) {
onTapViewPhoto()
}
Button(Localization.replacePhoto) {
onTapReplacePhoto()
}
Button(role: .destructive) {
onTapRemovePhoto()
} label: {
Text(Localization.removePhoto)
}
} label: {
Image(systemName: "ellipsis")
.frame(width: Layout.ellipisButtonSize * scale, height: Layout.ellipisButtonSize * scale)
.bodyStyle()
.foregroundStyle(Color.secondary)
}
}
.padding(Layout.padding)
.background(Color(.systemColor(.systemGray6)))
.clipShape(
.rect(
bottomLeadingRadius: Layout.textFieldOverlayCornerRadius,
bottomTrailingRadius: Layout.textFieldOverlayCornerRadius
)
)
}

enum Layout {
static let spacing: CGFloat = 16
static let cornerRadius: CGFloat = 4
static let padding = EdgeInsets(top: 12, leading: 16, bottom: 12, trailing: 16)
static let textFieldOverlayCornerRadius: CGFloat = 8
static let packagePhotoSize: CGFloat = 48
static let ellipisButtonSize: CGFloat = 24
}

enum Localization {
static let photoSelected = NSLocalizedString(
"productCreationAIStartingInfoView.packagePhotoView.photoSelected",
value: "Photo selected",
comment: "Text to explain that a package photo has been selected in starting info screen."
)
static let viewPhoto = NSLocalizedString(
"productCreationAIStartingInfoView.packagePhotoView.viewPhoto",
value: "View Photo",
comment: "Title of button which opens the selected package photo in starting info screen."
)
static let replacePhoto = NSLocalizedString(
"productCreationAIStartingInfoView.packagePhotoView.replacePhoto",
value: "Replace Photo",
comment: "Title of the button which opens photo selection flow to replace selected package photo in starting info screen."
)
static let removePhoto = NSLocalizedString(
"productCreationAIStartingInfoView.packagePhotoView.removePhoto",
value: "Remove Photo",
comment: "Title of button which removes selected package photo in starting info screen."
)
}
}
}

private extension ProductCreationAIStartingInfoView {
enum Layout {
static let insets: EdgeInsets = .init(top: 24, leading: 16, bottom: 16, trailing: 16)
Expand Down Expand Up @@ -155,7 +254,7 @@ private extension ProductCreationAIStartingInfoView {
static let subtitle = NSLocalizedString(
"productCreationAIStartingInfoView.subtitle",
value: "Tell us about your product, what it is and what makes it unique, then let the AI work its magic.",
comment: "Subtitle on the starting info screen."
comment: "Subtitle on the starting info screen explaining what text to enter in the textfield."
)
static let placeholder = NSLocalizedString(
"productCreationAIStartingInfoView.placeholder",
Expand All @@ -165,7 +264,7 @@ private extension ProductCreationAIStartingInfoView {
static let readTextFromPhoto = NSLocalizedString(
"productCreationAIStartingInfoView.readTextFromPhoto",
value: "Read text from product photo",
comment: "Upload package photo button on the text field"
comment: "Button to upload package photo to read text from the photo"
)
static let continueText = NSLocalizedString(
"productCreationAIStartingInfoView.continueText",
Expand All @@ -177,6 +276,6 @@ private extension ProductCreationAIStartingInfoView {

struct ProductCreationAIStartingInfoView_Previews: PreviewProvider {
static var previews: some View {
ProductCreationAIStartingInfoView(viewModel: .init(siteID: 123), onUsePackagePhoto: { _ in }, onContinueWithFeatures: { _ in })
ProductCreationAIStartingInfoView(viewModel: .init(siteID: 123), onContinueWithFeatures: { _ in })
}
}
Loading