-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Audio] 페이지에 오디오 플레이어 연결 #122
Conversation
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.
고생하셨습니다 !!!!!
로직 많이 겹쳐서 충돌 많이 났을텐데 고생하셨습니다 !!!!!!
밑에 수정할 부분들 코멘트 남겨뒀습니당
nonisolated(unsafe) var audioPlayer: AVAudioPlayer? | ||
var audioPlayState: AudioPlayState = .pause { | ||
didSet { | ||
switch audioPlayState { | ||
case .play: | ||
startTimer() | ||
case .pause: | ||
stopTimer() | ||
} | ||
} | ||
} | ||
var timer: Timer? | ||
|
||
// MARK: - ViewComponent | ||
let backgroundBorderView: UIView = { | ||
let backgroundBorderView = UIView() | ||
backgroundBorderView.backgroundColor = .baseBackground | ||
backgroundBorderView.layer.borderWidth = 3 | ||
backgroundBorderView.layer.cornerRadius = 25 | ||
backgroundBorderView.layer.borderColor = UIColor.captionPlaceHolder.cgColor | ||
|
||
return backgroundBorderView | ||
}() | ||
let audioProgressView: UIView = { | ||
let backgroundView = UIView() | ||
backgroundView.backgroundColor = .mhPink | ||
// backgroundView.layer.borderWidth = 4 | ||
backgroundView.layer.cornerRadius = 21 | ||
// backgroundView.layer.borderColor = UIColor.baseBackground.cgColor | ||
|
||
return backgroundView | ||
}() | ||
var progressViewWidthConstraint: NSLayoutConstraint? | ||
var progressViewConstraints: [NSLayoutConstraint] = [] | ||
let audioStateButton: UIButton = { | ||
let button = UIButton() | ||
button.setImage(UIImage(systemName: "play.fill"), for: .normal) | ||
return button | ||
}() | ||
let playImage = UIImage(systemName: "play.fill") | ||
let pauseImage = UIImage(systemName: "pause.fill") | ||
let audioStateImageView: UIImageView = { | ||
let imageView = UIImageView() | ||
imageView.image = UIImage(named: "audio_play") | ||
|
||
return imageView | ||
}() | ||
let audioPlayTimeLabel: UILabel = { |
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.
P2 여기 private 키워드만 붙여주시면 감사하겠습니당
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.
GOOD😊
// backgroundView.layer.borderWidth = 4 | ||
backgroundView.layer.cornerRadius = 21 | ||
// backgroundView.layer.borderColor = UIColor.baseBackground.cgColor |
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.
P2 불필요한 주석도 제거하면 좋을 거 같아요
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.
GOOD😊
|
||
final public class MHAudioPlayerView: 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.
P1
public 제거하고, extension에 있는 메소드도 Public 제거 해주시면 감사하겠습니다 !
P3
7번 줄 개행 제거해주시면 감사하겠습니당
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.
GOOD😊
} | ||
|
||
extension MHAudioPlayerView: AVAudioPlayerDelegate { | ||
nonisolated public func audioPlayerDidFinishPlaying(_ player: AVAudioPlayer, successfully flag: Bool) { |
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.
P1 public
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.
GOOD😊
case .image: | ||
MHPolaroidPhotoView() | ||
case .video: | ||
MHPolaroidPhotoView() |
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.
P1
제 똥이긴 한데 여기 MHVideoView()
로 바꿔주실 수 있나요 ??
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.
GOOD😊
attachmentMetaData.forEach { location, description in | ||
attachmentMetaData.forEach { | ||
location, | ||
description in |
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.
GOOD😊
<key>UIFileSharingEnabled</key> | ||
<true/> | ||
<key>LSSupportsOpeningDocumentsInPlace</key> | ||
<true/> |
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.
P1
제거하기로 결정했습니다 !!
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.
GOOD😊
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.
오디오 만들어주시느라 정말 수고하셨습니다 .ᐟ.ᐟ 🙇🏻♀️
궁금한 사항들이랑 수정되면 좋을 것 같은 내용들 코멘트로 조금 남겨두었습니다 .ᐟ.ᐟ
audioRecorder?.isMeteringEnabled = true | ||
print("URL: \(url)") |
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.
P2: print문은 지워주면 좋을 것 같습니당 !
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.
GOOD😊
audioButton.addAction(UIAction { [weak self] _ in | ||
self?.input.send(.audioButtonTapped) | ||
}, for: .touchUpInside) | ||
} | ||
private func addTappedEventToCancelButton() { | ||
cancelButton.addAction( | ||
UIAction { [weak self] _ in | ||
self?.input.send(.recordCancelled) | ||
}, | ||
for: .touchUpInside) | ||
} |
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.
P3: 개행 컨벤션이 일정하지 않은데 통일시켜주면 좋을 것 같습니다 !
AVAudioSession.sharedInstance().requestRecordPermission { granted in | ||
Task { @MainActor in | ||
if !granted { | ||
self.present(alert, animated: true, completion: nil) | ||
} | ||
} | ||
} |
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.
P1: 이쪽 혹시 Test 해보셨을까요?? 아마 requestRecordPermission 후행 클로저에 Sendable
붙이지 않으면 앱이 런타임 시점에 죽을 것 같습니다 ~
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.
AVAudioSession.sharedInstance().requestRecordPermission { @Sendable granted in
Task { @MainActor in
if !granted {
self.present(alert, animated: true, completion: nil)
}
}
}
이게 맞을까요? 테스트는 아직...
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.
16.4시뮬로 테스트해봤었는데 잘 됐었습니다!
func configureSource(with mediaDescription: MediaDescription, data: Data) { | ||
|
||
} | ||
|
||
func configureSource(with mediaDescription: MediaDescription, url: URL) { | ||
MHLogger.debug("configure source \(url)") | ||
audioPlayer = try? AVAudioPlayer(contentsOf: url) | ||
guard let audioPlayer else { return } | ||
audioPlayer.delegate = self | ||
self.setTimeLabel(seconds: Int(audioPlayer.duration.rounded())) | ||
} |
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.
P2: 해당 부분은 사용될 일은 없지만 만약을 위해 하단 url과 동일하게 메서드를 채워줘야할 것 같습니다 ! 약간 required init같은 느낌?
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.
AVAduioPlayer Object 생성자 중 매개변수를 Data로 받는 부분이 없어서 조금 애매한데 Logger라도 찍는게 나을까요?
if audioPlayer.isPlaying { | ||
self?.updatePlayAudioProgress() | ||
self?.setTimeLabel(seconds: Int(audioPlayer.currentTime)) | ||
} else { | ||
|
||
} |
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.
P1: else문엔 어떤 처리가 들어가야하나요?
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.animate(withDuration: 0) { | ||
self.layoutIfNeeded() | ||
} |
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.
P2: 애니메이션이 0초만에 일어나는 것이라면 애니메이션을 넣어야할 필요 있나요? 혹시 어떤 애니메이션때문에 해당 코드를 작성해주신 건가요?
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.
테스트해보니 필요 없을 거 같아요
|
||
private func updatePlayAudioProgress() { | ||
guard let audioPlayer else { return } | ||
let width = ceil(Float(audioPlayer.currentTime) / Float(audioPlayer.duration) * Float(299)) |
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.
P3: 299라는 숫자는 무엇을 의미하는 숫자인가요?
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.
audio media 길이입니다!
if audioPlayer?.play() == false { | ||
MHLogger.error("do not play") | ||
} else { | ||
MHLogger.debug("do play") | ||
} |
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.
P2: 해당 로직이 어떤 로직인지 잘 이해가 가지 않습니다.. 혹시 설명해주실 수 있으신가요?
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.
오디오 플레이 테스트할 때 사용되었던 건데 지웠습니다.
private let output = PassthroughSubject<Output, Never>() | ||
private var cancellables = Set<AnyCancellable>() | ||
private var audioIsRecoding: Bool = false | ||
private let completion: (MediaDescription?) -> Void |
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.
P3: completion 핸들러는 보통 VC에 있었던 것 같은데, ViewModel에 completion이 있게 된 경위?가 궁금합니다 !
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.
VC에서 input(화면 닫히는 이벤트들)을 ViewModel에 보내면 ViewModel이 알아서 수행하도록 하고자 했습니다.
(VC로 옮겨도 상관 무)
private func mediaViewFactory(type: MediaType) -> UIView & MediaAttachable { | ||
switch type { | ||
case .image: | ||
MHPolaroidPhotoView() | ||
case .video: | ||
MHVideoView() | ||
case .audio: | ||
MHAudioPlayerView() | ||
default: | ||
MHPolaroidPhotoView() | ||
} |
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.
P2: 해당 메서드를 생성한건 좋은데 사용되지 않는 것 같습니다 .ᐟ.ᐟ 위 코드들을 해당 메서드를 활용하도록 변경해주면 좋을 것 같네용 !
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.
고생하셨습니다 〰️
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.
너무 고생하셨습니다...ㅠㅠㅠㅠ
#️⃣ 연관된 이슈
⏰ 작업 시간
📝 작업 내용
📸 스크린샷
ScreenRecording_12-04-2024.02-50-43_1.MP4