-
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
책 수정 화면 로직 구현 #113
책 수정 화면 로직 구현 #113
Conversation
@@ -78,3 +79,12 @@ final class MHPolaroidPhotoView: UIView { | |||
creationDateLabel.text = creationDate | |||
} | |||
} | |||
|
|||
extension MHPolaroidPhotoView: @preconcurrency MediaAttachable { |
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.
이거는 Xcode가 붙이래서 붙인 preconcurrency..
input.send(.pageWillDisappear) | ||
cancellables.forEach { $0.cancel() } | ||
cancellables = [] | ||
viewModel = nil | ||
textView.text = "" |
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 reuse되기전에 청소했습니다.. 구독을 꼭 끊어야 합니다.. (deinit이 안돼서)
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.
정현님 고생하셨습니다 !!
짱짱짱
public struct LocalMediaRepository: MediaRepository, Sendable { | ||
private let storage: FileStorage | ||
private let temporaryPath = "temp" | ||
private let snapshotFileName = ".snapshot" |
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.
temporaryPath는 언제 사용되나요?
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.
이제 사용이 안되서 없앨 예정입니당,,
public func createSnapshot(for media: [MediaDescription], in bookID: UUID) async -> Result<Void, MHDataError> { | ||
let path = bookID.uuidString | ||
let mediaList = media.map { fileName(of: $0) } | ||
guard let snapshot = try? JSONEncoder().encode(mediaList) | ||
else { return .failure(.snapshotEncodingFailure) } | ||
|
||
return await storage.create(at: path, fileName: snapshotFileName, data: snapshot) | ||
} |
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.
편집뷰에서 수정완려/저장 버튼을 눌렀을 때 호출됩니다!
} | ||
} | ||
|
||
public struct DefaultPersistentlyStoreMediaUseCase: PersistentlyStoreMediaUseCase { |
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.
Temporary가 있었던 거 같은데 대비해서 Persistently 좋은 거 같습니다.
근데 부사가 쓰이는지 가물가물해서 Persistent도 좋은 거 같아요
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.
정현님.. 정말.. 저희 팀의 "기둥" 이십니다.. 🥹 경배하라 갓.정.현 ✨
궁금했던 부분들과 수정하면 좋을 것 같은 제 조그만 의견 남겨두었습니다 !
추가로 뭔가 FileManager부터 URL과 Data 로직은 똑같은데 매개변수때문에 함수가 2개로 늘어나는 느낌이 드네용.. url과 data를 옵셔널로 둘 다 한 번에 받는 방식으로 처리하는 것에 대해선 어떻게 생각하시나욥??
그리고 코멘트에도 달아뒀지만, protocol이나 class들은 짧더라도 소스파일 단위로 분리해주면 이후에 해당 객체의 위치를 찾기 더 수월할 것 같다는 생각입니당 ~
@@ -125,6 +136,11 @@ final class SceneDelegate: UIResponder, UIWindowSceneDelegate { | |||
BookRepository.self, | |||
object: LocalBookRepository(storage: bookStorage) | |||
) | |||
guard let fileManager = try? DIContainer.shared.resolve(MHFileManager.self) else { return } |
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: 다른곳과 마찬가지로 해당 함수 자체가 throws라서 그냥 아래와 같이 사용해도 될 것 같습니당 !
guard let fileManager = try? DIContainer.shared.resolve(MHFileManager.self) else { return } | |
let fileManager = try DIContainer.shared.resolve(MHFileManager.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.
맞다 바꾸는 것을 까먹었네요,, 수정하겠습니다!
DIContainer.shared.register( | ||
MHFileManager.self, | ||
object: MHFileManager(directoryType: .documentDirectory) | ||
) |
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: 정말 별 것 아니긴한데, 보기에 뭔가 CoreData 끼리 먼저 register를 해주고 FileManager와 UserDefaults는 가장 아래 위치시키면 가독성측면에서 더 좋을 것 같습니당
@@ -22,13 +22,13 @@ extension MHFileManager: FileStorage { | |||
|
|||
do { | |||
try fileManager.createDirectory(at: directory, withIntermediateDirectories: true) | |||
try data.write(to: dataPath) | |||
try data.write(to: dataPath, options: .atomic) |
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: 해당 option은 어떤걸 뜻하는 것인가욥??
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.
저장할 때 임시파일 만들었다가 저장이 완료되면 실제 파일로 바뀌는 옵션입니다!
더 많은 옵션 보기
} | ||
} | ||
|
||
public struct DefaultPersistentlyStoreMediaUseCase: PersistentlyStoreMediaUseCase { |
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.
해당 UseCase가 하는 일이 수정된 미디어의 삭제를 하는 것 같은데 DefaultDeleteModifiedMedia
는 어떨까요?? 아니면.. 임시 미디어 삭제라는 의미로 DefaultDeleteTemporaryMedia
??
guard let originDirectory = fileManager.urls( | ||
for: directoryType, | ||
in: .userDomainMask | ||
).first?.appending(path: path) |
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.
해당 메서드는 어떤뜻인지 궁금합니다 ! userDomainMask는 어떤 옵션인지 궁금해요!
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.
urls(for:in:)메스드에서
for:은 저희가 찾고자 하는 디렉토리(폴더)입니다.
in:은 해당 디렉토리를 찾고자 하는 영역입니다.
맥을 기준으로 설명드리면 ~/Document라든지 ~/Downloads 등이 있는데, ~가 in:에 해당하고, Downloads, Document가 for:에 해당합니다., 즉, in은 영역(사용자 영역, 로컬 영역-가끔 "앱을 전체 사용자가 사용할 수 있게 하겠습니까?" 그런것 있죠?, 네트워크 영역-공유 폴더 등)을 나타내고, for:는 실제 디렉토리를 의미합니다.
저희는 ~/Document를 사용한다고 코딩해놓은 것입니다!
문서가 더 잘 설명되어있으니 한번 보셔도 좋을 것같아요!
final class MediaAttachment: NSTextAttachment { | ||
// MARK: - Property | ||
private let view: (UIView & MediaAttachable) | ||
var cachedViewProvider: MediaAttachmentViewProvider? |
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: 캐싱도 .. 구현하신건가요..?
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.
이름이 cached여서 그렇지 그냥 reuse 그런 의미입니다,,,핳
private func addEmptyPage() { | ||
let page = Page(id: UUID(), metadata: [:], text: "") | ||
let editPageViewModel = EditPageViewModel( | ||
fetchMediaUseCase: fetchMediaUseCase, | ||
deleteMediaUseCase: deleteMediaUseCase, | ||
bookID: bookID, | ||
page: page | ||
) | ||
editPageViewModel.delegate = self | ||
editPageViewModels.append(editPageViewModel) | ||
output.send(.updateViewController(title: title)) | ||
} |
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: addPage를 보니 생각났는데 deletePage는 아직 구현이 안..된거겠죠..??
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 snapshotImage() -> UIImage? { | ||
let renderer = UIGraphicsImageRenderer(bounds: bounds) | ||
return renderer.image { context in | ||
layer.render(in: context.cgContext) | ||
} | ||
} |
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: 해당 image snapshot은 어디서 사용되는건가요?
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.
P2: 해당 소스파일 내부에 여러 protocol들과 class들이 함께 섞여있는데, 파일별로 분리해야 나중에 해당 class 혹은 protocol을 찾기가 수월해질 것 같습니당 !
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.
DataSource빼고는 전부 분리했습니다!
} | ||
} | ||
|
||
class MediaAttachmentViewProvider: NSTextAttachmentViewProvider { |
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: 해당 class가 다른곳에 또 상속될 일이 있나용?? 없다면 final 키워드를 붙여줘도 좋을 것 같습니당
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.
ㄹㅇㄹㅇ 우리 팀의 기둥
감사합니다 고생하셨어요 !!!
struct MHFileManager { | ||
private let fileManager = FileManager.default | ||
public struct MHFileManager: Sendable { | ||
private var fileManager: FileManager { FileManager.default } |
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 fileManager = FileManager.default
가 더 괜찮지 않은지 질문 드립니다 !!
만약 테스트를 할 거면 생성자 시점에서 주입 받아도 괜찮구요
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.
좋습니당 ~~ 수고많으셨어요 👍
#️⃣ 연관된 이슈
⏰ 작업 시간
📝 작업 내용
📸 스크린샷
📒 리뷰 노트
따라서 기존의 temp로 저장하는 것은 슬슬... 보내주어야..
변한게 좀 많군요.... 하하... 죄송합니다..
⚽️ 트러블 슈팅
너무 길어서? 그냥 노션링크로 올릴꼐요... 하하... 죄송합니다..