-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: 재생화면 뷰 collectionView로 넣기 #92
Conversation
PlaybackViewController를 CollectionView로 변경
|
||
final class PlaybackCell: UICollectionViewCell { | ||
let playbackView: PlaybackView = PlaybackView() | ||
static let reuseIdentifier: String = "PlaybackCellReuseIdentifier" |
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.
저희 UIView extension에 identifier 사용해놔서 선언안해도 괜찮을 것 같습니다.
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.
오마이갓 그랬군요 반영하겠습니다.
iOS/Layover/Layover/Scenes/Playback/PlaybackViewController.swift
Outdated
Show resolved
Hide resolved
guard let duration: CMTime = playerView.player?.currentItem?.duration else { | ||
extension PlaybackViewController: UICollectionViewDelegate { | ||
func scrollViewDidEndDecelerating(_ scrollView: UIScrollView) { | ||
// 추후 무한 스크롤 처리 예정 |
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.
오오....
playbackCollectionView.showsVerticalScrollIndicator = false | ||
playbackCollectionView.isPagingEnabled = true | ||
playbackCollectionView.translatesAutoresizingMaskIntoConstraints = 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.
이 부분은 collectionView 정의 클로저 내부에서 해주면 저희 컨벤션에 더 잘맞을 것 같습니다!
@@ -130,11 +66,10 @@ final class PlaybackViewController: UIViewController, PlaybackDisplayLogic { | |||
super.viewDidLoad() | |||
view.backgroundColor = .black |
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.
setUI에서 설정해주어야 할것 같아요
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.
넵 확인했습니다.
} | ||
|
||
func setPlayerSlider() { | ||
let interval: CMTime = CMTimeMakeWithSeconds(1, preferredTimescale: Int32(NSEC_PER_SEC)) |
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.
Int32(NSEC_PER_SEC)
이건 무슨 값인가요?
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.
1초 단위입니다.
} | ||
|
||
extension PlaybackView { | ||
func setPlayerView() { |
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.
네이밍이 좀 더 구체적이었으면 좋겠어요~~
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.
넵 수정하겠습니당
import UIKit | ||
import AVFoundation | ||
|
||
final class PlaybackView: 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.
View 코드량이 많다 보니까 함수 호출 별로 찾아보기가 어려운데, mark 주석으로 구분해 주실 수 있을까요?
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.
넵 알겠습니다.
|
||
let playerSlider: LOSlider = LOSlider() | ||
|
||
private let playerView: PlayerView = PlayerView() |
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.
LoopingPlayerView를 따로 사용하지 않으신 이유가 궁금합니다!
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.
이전 PR에도 코멘트 달았었는데, LoopingPlayer를 지정하려면 반복 주기 시간을 지정해야 하다 보니 시점이 애매해져서 다 끝나면 처음 시작으로 돌아가게 했습니다.
if prevPlaybackCell == nil { | ||
prevPlaybackCell = playbackCollectionView.cellForItem(at: IndexPath(row: 0, section: 0)) as? PlaybackCell | ||
} else { | ||
prevPlaybackCell?.playbackView.playPlayer() | ||
} |
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.
생명주기 메서드에는 함수 호출만 선언부탁드립니다~
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.
넵 수정하겠습니다~
let cellRegistration = UICollectionView.CellRegistration<PlaybackCell, URL> { (cell, _, url) in | ||
cell.addAVPlayer(url: url) | ||
cell.setPlayerSlider(tabbarHeight: tabbarHeight) | ||
} |
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.
cell이 재사용 되면 계속해서 player가 추가될 것 같은데 그런 재사용 이슈는 발생하지 않나요?
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.
prepareForReuse를 잡아놨었는데, 호출 안돼서 봤더니 reuse등록을 안해놨네요 같이 고칠게요~
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.
이 친구는 동영상 캐싱에 대한 이해가 좀 필요할 것 같습니다. 시간이 좀 걸릴 것 같네요.
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.
prepareForReuse 공식문서에서는 잠재적인 성능 이슈 방지를 위해서 성능과 관련없는 속성(alpha, editing, and selection state)만 초기화 하라고 되어있어서, 재사용 셀 초기화는 dataSource 쪽에서 해주는 게 좋을 것 같은데 어떻게 생각하시나요?
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.
말씀하신건 cell에 값을 넣을 때 초기화 먼저 해주고 해야 한다는 말씀이실까요? 저번에 한 번 들었던 것 같긴 한데 예시가 있으면 링크 주시면 감사하겠습니다
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.
네 말씀하신 방법이 맞습니다! 지금 상황에서는 cell.addAVPlayer
호출 전에 avplayer를 한번 비워주면 될 것 같아요
if descriptionView.checkLabelOverflow() { | ||
descriptionView.descriptionLabel.layer.addSublayer(gradientLayer) | ||
} | ||
extension PlaybackViewController: UICollectionViewDelegateFlowLayout { |
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.
FlowLayout delegate에서 indexPath 별로 다르게 구현한 건 없어보이는데, flowLayout 프로퍼티 설정만으로 해결할 수 있지 않을까요?
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.
Cell의 크기가 CollectionView가 View에 들어간 뒤 잡혀서 시점 상 힘들것 같습니다.
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.
lazy var로 잡을 수 있지 않을까요?
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.
이게 flowlayout도 collectionView의 크기가 필요하고 collectionView도 flowlayout이 필요한 상황이라 쉽지 않을 것 같습니다
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.
아 저는 어차피 collectionView도 view크기랑 동일하게 설정하셨길래 둘 다 view 사이즈로 잡는걸 드린 말씀이었는데 지웅님 편하신대로 하시면 될 것 같습니다~!
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.
엄청난 작업을 하셨네요...! Approve 하겠슴다
/// Home과 Home을 제외한 나머지(맵, 프로필, 태그)의 무한 스크롤 동작이 다름 | ||
/// Home은 내릴 때마다 Video호출 필요, 나머지는 정해진 양이 있음 | ||
/// Home일 경우는 첫번 째 cell일 때 위로 안올라감. | ||
/// 모든 동영상을 다 지나쳐야 첫번째 cell로 이동 |
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.
여기까지만 보면 뷰 마다 동작이 다르기보다는, Home
과 Home이 아닌 것
만 구분하면 되지 않나 싶습니다..!
그래서 ViewType
으로 구분하는 것보다 무한 스크롤 가능 여부
로 구분해도 괜찮을 것 같아요 ㅎㅎ
추후에 무한 스크롤이 가능한 뷰가 더 생기게된다면, View Type
이 추가되고 if문도 변경되게 될 것 같습니다!
코드량이 좀 많아서 미처 못읽은 부분이 있을 것 같은데.. 혹시 추후에 ViewType으로 다른 분기를 처리하려 하셨다면 무시해주세여..
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.
넵 말씀하신 부분 참고해보겠습니다. 이래저래 고민이네요 그냥 뷰컨 새로하나 파는게 편할것 같기도 하고. 감사합니다.
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.
하드한 작업 하시느라 고생하셨어요 지웅냄.. ✨
코멘트 확인만 해주시고 merge 하시면 될 것 같습니당 화이팅..
🧑🚀 PR 요약
📌 변경 사항
collectionView는 화면에 꽉 차게 넣으려고 flowLayout을 사용했습니다.
cell이 움직이면 나타난 cell은 재생하고 그 이전의 cell은 멈추게 하고 싶은데, 좋은 방법이 있을까요? 일단 visible을 사용해볼 계획이긴 합니다.collectionView이동한다고 변경된 코드 양이 많습니다.
📸 ScreenShot
Uploading Simulator Screen Recording - iPhone 13 mini - 2023-11-27 at 01.33.40.mp4…
Linked Issue
close #90