Support video in the picker #44

amelia2801 · 2019-04-18T08:16:58Z

Provide options to display images only, videos only, or both images and videos in the ImagePickerController

carouselljenkins · 2019-04-18T08:20:18Z

@HungnguyenVN, can you review this pull request?

carouselljenkins · 2019-04-18T08:20:25Z

	1 Error
🚫	Please update CHANGELOG.md.

Generated by 🚫 Danger

daveluong · 2019-04-18T08:24:25Z

Pickle/Classes/ImagePickerConfigurable.swift

+public enum ImagePickerMediaType {
+
+    /// Show images and videos in ImagePickerController
+    case unknown


Should be all ?

PHAsset has audio mediaType as well.
https://developer.apple.com/documentation/photokit/phassetmediatype

any better suggestion? or do u think can use all?

.unknown doesn't make sense I feel. How about removing .unknown and bitmasking it using OptionSet?

https://developer.apple.com/documentation/swift/optionset

Then the API consumer can pass in any combination of mediaType they desire:
mediaType: ImagePickerMediaType = [.video, .image]
mediaType: ImagePickerMediaType = [.video, .audio]
mediaType: ImagePickerMediaType = .video
mediaType: ImagePickerMediaType = .all
...

daveluong · 2019-04-18T08:37:36Z

Pickle/Classes/PhotoGalleryCell.swift

@@ -32,6 +32,8 @@ internal final class PhotoGalleryCell: UICollectionViewCell {
        return imageView
    }()

+    private let videoPropertyView = VideoPropertyView()


Wondering if we should have a different type of cell for video:

We won't need to set isHidden

Won't have redundant and hidden videoPropertyView on top of all the cells

Easy to reason about

It could get messy if we support more types with different UI in the future (live photo, panorama, etc..) if all of them reuse this cell

was thinking of that too, but the only diff is this VideoPropertyView. shall we subclass PhotoGalleryCell?
call it... GalleryVideoCell? and rename PhotoGalleryCell to GalleryPhotoCell.

class GalleryVideoCell: GalleryPhotoCell { }

daveluong · 2019-04-18T08:40:37Z

Pickle/Classes/PhotoGalleryViewController.swift

+        case .video:
+            options.predicate = NSPredicate(format: "mediaType = %d", PHAssetMediaType.video.rawValue)
+        default:
+            break


I dont quite understand this. Shouldn't the fetch results contain both type of assets?

if we don't pass in the predicate, then yes it will fetch both type of assets

It probably can be simplified as one switch:

switch configuration?.mediaType { case .image?: // ... case .video?: // ... case nil: // ... }

daveluong · 2019-04-18T08:41:19Z

Pickle/Classes/VideoPropertyView.swift

+//  See https://github.com/carousell/pickle/graphs/contributors for the list of project authors
+//
+
+internal final class VideoPropertyView: UIView {


internal is default

It's generally a good practice to specify explicit access control levels in a library. It implies that a method should only be made public in the future with careful consideration. The internal keyword is more about the documentation.

bcylin

Looks good overall 👍 Just left some minor suggestions.

bcylin · 2019-04-18T21:22:29Z

Pickle/Classes/PhotoGalleryViewController.swift

+        case .video:
+            options.predicate = NSPredicate(format: "mediaType = %d", PHAssetMediaType.video.rawValue)
+        default:
+            break


It probably can be simplified as one switch:

switch configuration?.mediaType { case .image?: // ... case .video?: // ... case nil: // ... }

bcylin · 2019-04-18T21:23:25Z

Pickle/Classes/VideoPropertyView.swift

+    private let videoIcon: UIImageView = {
+        let icon = UIImageView()
+        icon.tintColor = .white
+        icon.image = UIImage(named: "video-icon")?.withRenderingMode(.alwaysTemplate)


You might need to specify the framework bundle that contains the image.

init?(named name: String, in bundle: Bundle?, compatibleWith traitCollection: UITraitCollection?)

bcylin · 2019-04-18T21:24:36Z

Pickle/Classes/UIColor+Palette.swift

@@ -18,6 +18,7 @@ internal extension UIColor {
        static let gray = UIColor(hex: 0x8F939C)
        static let darkGray = UIColor(hex: 0x8F939C)
        static let magnesium = UIColor(hex: 0xB2B2B2)
+        static let darkGrey = UIColor(hex: 0x4B4D52)


There's already a darkGray, perhaps it needs a different name?

I just noticed that 0x8F939C is duplicated for lightGray, gray, and darkGray 😂
So, I removed darkGray to be replaced with darkGrey.
Also, renamed other "gray" to "grey" to follow UK spelling

bcylin · 2019-04-18T21:31:53Z

Pickle/Classes/VideoPropertyView.swift

+//  See https://github.com/carousell/pickle/graphs/contributors for the list of project authors
+//
+
+internal final class VideoPropertyView: UIView {


It's generally a good practice to specify explicit access control levels in a library. It implies that a method should only be made public in the future with careful consideration. The internal keyword is more about the documentation.

amelia2801 · 2019-04-23T09:11:31Z

@daveluong @bcylin i made some changes based on your comments. Please help review again 🙏
Do I need to update CHANGELOG? And when should we update version?

bcylin · 2019-04-23T19:31:41Z

It should be all right to add a ## Next release section in CHANGELOG.md.

Since the master branch is protected, I usually have another PR to run make bump version=<number> and update README if needed.

bcylin

👍

bcylin · 2019-04-23T19:33:08Z

Pickle/Classes/PhotoGalleryViewController.swift

+        } else {
+            cell = collectionView.dequeueReusableCell(withReuseIdentifier: String(describing: GalleryPhotoCell.self), for: indexPath)
+            (cell as? GalleryPhotoCell)?.configure(with: asset, taggedText: text, configuration: configuration)
+            collectionView.deselectItem(at: indexPath, animated: false)


Is the deselectItem(at:animated:) needed in L187 and L191?

This is already followed by:

if text != nil { collectionView.selectItem(at: indexPath, animated: false, scrollPosition: []) } else { collectionView.deselectItem(at: indexPath, animated: false) }

bcylin · 2019-04-23T19:33:22Z

Pickle/Classes/GalleryVideoCell.swift

+
+    override func prepareForReuse() {
+        super.prepareForReuse()
+    }


The implementation is empty. It's nice to have it removed.

bcylin · 2019-04-23T19:44:06Z

Pickle/Classes/GalleryVideoCell.swift

+            videoPropertyView.setSelected(true)
+        } else {
+            videoPropertyView.setSelected(false)
+        }


videoPropertyView.setSelected(taggedText != nil)

It can be more concise, but it's a choice of coding style.

bcylin · 2019-04-23T19:46:08Z

Pickle/Classes/VideoPropertyView.swift

+    }
+
+    func setSelected(_ value: Bool) {
+        if value {


Suggested change

if value {

if isSelected {

daveluong · 2019-04-24T01:43:18Z

Thank you @bcylin for helping us to review the PRs! Truly appreciate it :)

amelia2801 · 2019-04-24T05:51:20Z

Thank you @bcylin and @daveluong ! 😄

amelia2801 added 5 commits April 15, 2019 14:28

Add media type options support for image and video

b225632

Add video icon and duration

d17c0a8

Show hour unit only if necessary

d78c367

Move icon to the right assets folder

8d010d0

Update header file

97d1b62

amelia2801 requested review from daveluong and bcylin April 18, 2019 08:16

daveluong reviewed Apr 18, 2019

View reviewed changes

bcylin reviewed Apr 18, 2019

View reviewed changes

amelia2801 added 3 commits April 23, 2019 16:28

Update colors

9906e51

Use OptionSet to define ImagePickerMediaType

362075d

Subclass GalleryPhotoCell to support video cell

7f7eeb0

bcylin approved these changes Apr 23, 2019

View reviewed changes

amelia2801 added 2 commits April 24, 2019 13:38

Minor code adjustments

7924ab7

Update CHANGELOG.md

c4ee19a

amelia2801 merged commit f1aa077 into master Apr 24, 2019

amelia2801 deleted the feature/video-support branch April 24, 2019 05:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support video in the picker #44

Support video in the picker #44

amelia2801 commented Apr 18, 2019

carouselljenkins commented Apr 18, 2019

carouselljenkins commented Apr 18, 2019

daveluong Apr 18, 2019

amelia2801 Apr 18, 2019

daveluong Apr 22, 2019 •

edited

Loading

daveluong Apr 18, 2019 •

edited

Loading

amelia2801 Apr 18, 2019

daveluong Apr 18, 2019

amelia2801 Apr 18, 2019

bcylin Apr 18, 2019

daveluong Apr 18, 2019

bcylin Apr 18, 2019

bcylin left a comment

bcylin Apr 18, 2019

bcylin Apr 18, 2019

bcylin Apr 18, 2019

amelia2801 Apr 23, 2019

bcylin Apr 18, 2019

amelia2801 commented Apr 23, 2019

bcylin commented Apr 23, 2019

bcylin left a comment

bcylin Apr 23, 2019

bcylin Apr 23, 2019

bcylin Apr 23, 2019

bcylin Apr 23, 2019

daveluong commented Apr 24, 2019

amelia2801 commented Apr 24, 2019

Support video in the picker #44

Support video in the picker #44

Conversation

amelia2801 commented Apr 18, 2019

carouselljenkins commented Apr 18, 2019

carouselljenkins commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveluong Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

daveluong Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcylin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amelia2801 commented Apr 23, 2019

bcylin commented Apr 23, 2019

bcylin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daveluong commented Apr 24, 2019

amelia2801 commented Apr 24, 2019

daveluong Apr 22, 2019 •

edited

Loading

daveluong Apr 18, 2019 •

edited

Loading