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

Support video in the picker #44

Merged
merged 10 commits into from
Apr 24, 2019
Merged
Show file tree
Hide file tree
Changes from 5 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
3 changes: 3 additions & 0 deletions Example/CarousellImagePickerController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -108,4 +108,7 @@ private struct CarousellTheme: ImagePickerConfigurable {

let hintTextMargin: UIEdgeInsets? = .zero

// MARK: Media Types

var mediaType: ImagePickerMediaType? = .unknown
}
6 changes: 6 additions & 0 deletions Example/Images.xcassets/Contents.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"info" : {
"version" : 1,
"author" : "xcode"
}
}
4 changes: 4 additions & 0 deletions Pickle.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
607FACD81AFB9204008FA782 /* ViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 607FACD71AFB9204008FA782 /* ViewController.swift */; };
607FACDD1AFB9204008FA782 /* Images.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 607FACDC1AFB9204008FA782 /* Images.xcassets */; };
607FACE01AFB9204008FA782 /* LaunchScreen.xib in Resources */ = {isa = PBXBuildFile; fileRef = 607FACDE1AFB9204008FA782 /* LaunchScreen.xib */; };
94F4BF192264609C00E7A672 /* VideoPropertyView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 94F4BF182264609C00E7A672 /* VideoPropertyView.swift */; };
B55833221EC08E180061E182 /* CarousellImagePickerController.swift in Sources */ = {isa = PBXBuildFile; fileRef = B55833211EC08E180061E182 /* CarousellImagePickerController.swift */; };
B563515E1F06278E00B5A46D /* UITests.swift in Sources */ = {isa = PBXBuildFile; fileRef = B563515D1F06278E00B5A46D /* UITests.swift */; };
B57BEA34216AAF6300A2C776 /* Pickle.h in Headers */ = {isa = PBXBuildFile; fileRef = B57BEA32216AAF6300A2C776 /* Pickle.h */; settings = {ATTRIBUTES = (Public, ); }; };
Expand Down Expand Up @@ -92,6 +93,7 @@
607FACDC1AFB9204008FA782 /* Images.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = Images.xcassets; sourceTree = "<group>"; };
607FACDF1AFB9204008FA782 /* Base */ = {isa = PBXFileReference; lastKnownFileType = file.xib; name = Base; path = Base.lproj/LaunchScreen.xib; sourceTree = "<group>"; };
7D176E52E185B871CF8D97C4 /* Pods_PickleUITests.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_PickleUITests.framework; sourceTree = BUILT_PRODUCTS_DIR; };
94F4BF182264609C00E7A672 /* VideoPropertyView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = VideoPropertyView.swift; sourceTree = "<group>"; };
B55833211EC08E180061E182 /* CarousellImagePickerController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CarousellImagePickerController.swift; sourceTree = "<group>"; };
B563515B1F06278E00B5A46D /* PickleUITests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = PickleUITests.xctest; sourceTree = BUILT_PRODUCTS_DIR; };
B563515D1F06278E00B5A46D /* UITests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UITests.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -289,6 +291,7 @@
B57BEA61216AAFD700A2C776 /* UIColor+Palette.swift */,
B57BEA4B216AAFD700A2C776 /* UIFont+Style.swift */,
B57BEA4A216AAFD700A2C776 /* UINavigationController+ImagePickerConfigurable.swift */,
94F4BF182264609C00E7A672 /* VideoPropertyView.swift */,
);
path = Classes;
sourceTree = "<group>";
Expand Down Expand Up @@ -538,6 +541,7 @@
B57BEA6F216AAFD700A2C776 /* PhotoDetailViewController.swift in Sources */,
B57BEA70216AAFD700A2C776 /* PhotoGalleryCameraCell.swift in Sources */,
B57BEA7A216AAFD700A2C776 /* PhotoGalleryCameraIconView.swift in Sources */,
94F4BF192264609C00E7A672 /* VideoPropertyView.swift in Sources */,
B57BEA73216AAFD700A2C776 /* PhotoGalleryCell.swift in Sources */,
B57BEA6B216AAFD700A2C776 /* PhotoGalleryHintLabel.swift in Sources */,
20FC48032256FAC3004CCD1D /* CameraSessionHandler.swift in Sources */,
Expand Down
12 changes: 12 additions & 0 deletions Pickle/Assets/Images.xcassets/video-icon.imageset/Contents.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"images" : [
{
"idiom" : "universal",
"filename" : "video-icon.pdf"
}
],
"info" : {
"version" : 1,
"author" : "xcode"
}
}
Binary file not shown.
17 changes: 17 additions & 0 deletions Pickle/Classes/ImagePickerConfigurable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,25 @@ public protocol ImagePickerConfigurable {

/// Specifies whether the camera button shows a live preview.
var isLiveCameraViewEnabled: Bool? { get }

// MARK: - Media Types

/// Specifies the supported media types
var mediaType: ImagePickerMediaType? { get }
}

/// An enum that represents media type selections in ImagePickerController
public enum ImagePickerMediaType {

/// Show images and videos in ImagePickerController
case unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be all ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@daveluong daveluong Apr 22, 2019

Choose a reason for hiding this comment

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

.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
...


/// Only show images in ImagePickerController
case image

/// Only show videos in ImagePickerController
case video
}

/// An enum that represents photo selections allowed in ImagePickerController.
public enum ImagePickerSelection {
Expand Down
4 changes: 4 additions & 0 deletions Pickle/Classes/Parameters.swift
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,8 @@ public struct Parameters: ImagePickerConfigurable {
/// Specifies whether the camera button shows a live preview.
public var isLiveCameraViewEnabled: Bool?

// MARK: - Media Types

/// Specifies the supported media types
public var mediaType: ImagePickerMediaType?
}
15 changes: 15 additions & 0 deletions Pickle/Classes/PhotoGalleryCell.swift
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ internal final class PhotoGalleryCell: UICollectionViewCell {
return imageView
}()

private let videoPropertyView = VideoPropertyView()
Copy link
Contributor

@daveluong daveluong Apr 18, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 { }


private let overlayView = UIView()
private let tagLabel = PhotoGalleryTagLabel()

Expand All @@ -41,9 +43,11 @@ internal final class PhotoGalleryCell: UICollectionViewCell {
overlayView.isHidden = false
tagLabel.text = text
accessibilityIdentifier = text
videoPropertyView.setSelected(true)
} else {
overlayView.isHidden = true
accessibilityIdentifier = nil
videoPropertyView.setSelected(false)
}
}
}
Expand All @@ -58,6 +62,7 @@ internal final class PhotoGalleryCell: UICollectionViewCell {
imageRequestID = nil
taggedText = nil
imageView.image = nil
videoPropertyView.isHidden = true
}

// MARK: -
Expand All @@ -83,6 +88,9 @@ internal final class PhotoGalleryCell: UICollectionViewCell {
}
}

videoPropertyView.isHidden = asset.mediaType != .video
videoPropertyView.configure(duration: asset.duration)

if let color = configuration?.selectedImageOverlayColor {
overlayView.backgroundColor = color
}
Expand Down Expand Up @@ -113,6 +121,13 @@ internal final class PhotoGalleryCell: UICollectionViewCell {
tagLabel.widthAnchor.constraint(equalTo: tagLabel.heightAnchor).isActive = true
tagLabel.topAnchor.constraint(equalTo: overlayView.topAnchor, constant: 10).isActive = true
tagLabel.trailingAnchor.constraint(equalTo: overlayView.trailingAnchor, constant: -10).isActive = true

contentView.addSubview(videoPropertyView)
videoPropertyView.translatesAutoresizingMaskIntoConstraints = false
videoPropertyView.bottomAnchor.constraint(equalTo: contentView.bottomAnchor).isActive = true
videoPropertyView.leadingAnchor.constraint(equalTo: contentView.leadingAnchor).isActive = true
videoPropertyView.trailingAnchor.constraint(equalTo: contentView.trailingAnchor).isActive = true
videoPropertyView.heightAnchor.constraint(equalToConstant: 24).isActive = true
}

}
18 changes: 17 additions & 1 deletion Pickle/Classes/PhotoGalleryViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,23 @@ internal final class PhotoGalleryViewController: UIViewController,
internal private(set) lazy var fetchResult: PHFetchResult<PHAsset> = {
let options = PHFetchOptions()
options.sortDescriptors = [NSSortDescriptor(key: "creationDate", ascending: false)]
options.predicate = NSPredicate(format: "mediaType = %d", PHAssetMediaType.image.rawValue)

guard let configuration = configuration,
let mediaType = configuration.mediaType
else {
options.predicate = NSPredicate(format: "mediaType = %d", PHAssetMediaType.image.rawValue)
return PHAsset.fetchAssets(in: self.album, options: options)
}

switch mediaType {
case .image:
options.predicate = NSPredicate(format: "mediaType = %d", PHAssetMediaType.image.rawValue)
case .video:
options.predicate = NSPredicate(format: "mediaType = %d", PHAssetMediaType.video.rawValue)
default:
break
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

It probably can be simplified as one switch:

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

}

return PHAsset.fetchAssets(in: self.album, options: options)
}()

Expand Down
1 change: 1 addition & 0 deletions Pickle/Classes/UIColor+Palette.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

internal convenience init(hex: Int) {
Expand Down
73 changes: 73 additions & 0 deletions Pickle/Classes/VideoPropertyView.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
//
// This source file is part of the carousell/pickle open source project
//
// Copyright © 2017 Carousell and the project authors
// Licensed under Apache License v2.0
//
// See https://github.com/carousell/pickle/blob/master/LICENSE for license information
// See https://github.com/carousell/pickle/graphs/contributors for the list of project authors
//

internal final class VideoPropertyView: UIView {
Copy link
Contributor

Choose a reason for hiding this comment

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

internal is default

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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

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

return icon
}()

private let durationLabel: UILabel = {
let label = UILabel()
label.font = UIFont.boldSystemFont(ofSize: 12)
label.textAlignment = .right
label.textColor = .white
return label
}()

func configure(duration: TimeInterval) {
let formatter = DateComponentsFormatter()
formatter.zeroFormattingBehavior = .pad

if duration >= 3600 {
formatter.allowedUnits = [.hour, .minute, .second]
} else {
formatter.allowedUnits = [.minute, .second]
}

durationLabel.text = formatter.string(from: duration)
}

func setSelected(_ value: Bool) {
if value {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if value {
if isSelected {

backgroundColor = UIColor.Palette.blue
} else {
backgroundColor = UIColor.Palette.darkGray.withAlphaComponent(0.2)
}
}

override init(frame: CGRect) {
super.init(frame: frame)
setupViews()
}

required init?(coder aDecoder: NSCoder) {
fatalError("init(coder:) has not been implemented")
}

private func setupViews() {
backgroundColor = UIColor.Palette.darkGray.withAlphaComponent(0.2)

addSubview(videoIcon)
videoIcon.translatesAutoresizingMaskIntoConstraints = false
videoIcon.heightAnchor.constraint(equalToConstant: 16).isActive = true
videoIcon.widthAnchor.constraint(equalToConstant: 16).isActive = true
videoIcon.leadingAnchor.constraint(equalTo: leadingAnchor, constant: 4).isActive = true
videoIcon.centerYAnchor.constraint(equalTo: centerYAnchor).isActive = true

addSubview(durationLabel)
durationLabel.translatesAutoresizingMaskIntoConstraints = false
durationLabel.trailingAnchor.constraint(equalTo: trailingAnchor, constant: -4).isActive = true
durationLabel.centerYAnchor.constraint(equalTo: centerYAnchor).isActive = true
}
}