-
-
Notifications
You must be signed in to change notification settings - Fork 303
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
Allow PixelEditor to be loaded with a PHAsset #39
Conversation
return loadingView != nil | ||
} | ||
set { | ||
if newValue, self.isLoading == false { |
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 think it would be better to create UI component with separating code to get better management.
Would you mind if I improve this code when I get time?
} | ||
private let asset: PHAsset? | ||
private var loadingView: [UIView]? | ||
private var imageSource: ImageSource? |
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.
If it's possible, we would like to avoid having imageSource here directly.
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.
Done
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.
The concept is okay.
And I would like to improve the code.
(Moving, separating)
@@ -60,10 +60,6 @@ open class ImageScrollView: UIScrollView { | |||
initialize() | |||
} | |||
|
|||
deinit { | |||
NotificationCenter.default.removeObserver(self) |
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.
Should we remove this?
|
||
import UIKit | ||
|
||
class LoadingView: UIView { |
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.
IMO: I usually add the final
attribute when we need to create a subclass.
private func downloadAssetIfNeeded() { | ||
guard let asset = self.asset else { return } | ||
let previewRequestOptions = PHImageRequestOptions() | ||
previewRequestOptions.deliveryMode = .highQualityFormat | ||
previewRequestOptions.isNetworkAccessAllowed = true | ||
previewRequestOptions.version = .current | ||
previewRequestOptions.resizeMode = .fast | ||
let finalImageRequestOptions = PHImageRequestOptions() | ||
finalImageRequestOptions.deliveryMode = .highQualityFormat | ||
finalImageRequestOptions.isNetworkAccessAllowed = true | ||
finalImageRequestOptions.version = .current | ||
finalImageRequestOptions.resizeMode = .none | ||
//TODO cancellation | ||
PHImageManager.default().requestImage(for: asset, targetSize: CGSize(width: 360, height: 360), contentMode: .aspectFit, options: previewRequestOptions) { [weak self] (image, _) in | ||
guard let image = image, let self = self, self.isLoading else { return } | ||
self.replace(imageSource: .init(source: image)) | ||
} | ||
PHImageManager.default().requestImage(for: asset, targetSize: PHImageManagerMaximumSize, contentMode: .aspectFit, options: finalImageRequestOptions) { [weak self] (image, _) in | ||
guard let self = self else { return } | ||
if let image = image { | ||
self.replace(imageSource: .init(source: image)) | ||
self.isLoading = false | ||
} else { | ||
// TODO Error handleing | ||
} | ||
} | ||
} |
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.
It might be good to do this on EditingStack.
I will add a few additional features soon, but I think this can be merged as is as soon as
AssetPicker
is made public