Skip to content

Conversation

@ekrud99
Copy link
Collaborator

@ekrud99 ekrud99 commented Nov 18, 2024

🌁 Background

유저가 메인 화면에서 browsing 시작하면 각 화이트보드 cell에 참여자 수와 아이콘을 표시하기 위한 PR입니다.

📱 Screenshot

로직이라 작동 영상은 없습니다

👩‍💻 Contents

NearbyNetworkService 내부 startPublishing 메서드 오버로딩해서 discoveryInfo에 참여자 정보 전달
수정된 NearbyNetworkService에 맞게 repository, usecase, viewmodel 수정

✅ Testing

discoveryInfo를 통해 advertising 상황에서 상호 정보 교환 상황은 슬랙에 있는 .zip 파일로 테스트 했습니다. 파일

📝 Review Note

  • advertising 실패 시 에러 처리
  • 현재 advertising stop 후 바로 start하면 예상하는 대로 동작하지 않는 이슈가 있어 3초의 시간 텀을 강제로 주고있습니다. 추후에 개선이 필요해보입니다.

@ekrud99 ekrud99 self-assigned this Nov 18, 2024
@ekrud99 ekrud99 changed the title [Feature] [Feature] Whiteboard를 advertising 할 때 참여자 정보 담아서 보내는 로직 구현 Nov 18, 2024
/// 화이트보드를 주변에 알립니다.
func startPublishing()
/// 주변에 내 기기를 정보와 함께 화이트보드를 알립니다.
func startPublishing(with info: [String: String])
Copy link
Collaborator

Choose a reason for hiding this comment

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

어떤 정보를 어떻게 담아야 할지 주석으로 추가해주면 좋을 것 같습니다!
현재 메서드의 파라미터나, 주석만 봤을때는 Key값으로 어떤 정보를 넣어줘야하고, value 값으로 어떤 정보를 넣어줘야 하는지 알기 쉽지 않은 것 같습니다.
만약 정보가 참여하고 있는 사람들만 전해준다면, reposiroty에는 [String: String] 형태의 딕셔너리가 아닌, 참여자 배열[Profile]을 전달하는 형식으로 설계해도 좋을 것 같구요!

Copy link
Collaborator Author

@ekrud99 ekrud99 Nov 18, 2024

Choose a reason for hiding this comment

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

제가 코드를 작성하지 않았다고 생각하고 다시 봤는데 확실히 info에 뭘 넣어야 할지 감이 안오네요.. ㅠ
주석은 더 상세하게 수정해보도록 하겠습니다.

일단 info는 ["participants": "😃,😥,😄"] 과 같이 전달되는 것으로 조이와 합의를 보았습니다.
아쉽게도 advertiser가 사용하는 discoveryInfo의 타입이 [String: String]이라 참여자 배열을 전달하는 방식은 쉽지않아보입니다.. ㅠ
이와 관련해 더 좋은 방법이 있다면 언제든 알려주시면 감사하겠습니다..!

Copy link
Collaborator

Choose a reason for hiding this comment

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

도메인 쪽에서 참여자 엔티티 배열을 전달해주고, resitory가 해당 정보를 가공해서 적절한 네트워크 서비스의 api를 호출하는 방법은 어떤가요? 좀 더 유연하게 확장에 대처할 수 있지 않을까 합니다.
현재는 근거리 통신 모듈의 api만 사용하고 있고, 해당 api에서는 [String: String]을 discoveryInfo로 사용하고 있습니다.
나중에다른 인터넷을 이용한 통신 모듈이 추가된다면, 해당 모듈의 api에서는 사용자의 프로필 정보를 받아 처리하는 모듈도 추가될 수 있다 생각합니다.
호옥시 구조적으로 어려움을 느끼고 있는 부분이 있다면 어떤 부분인지 설명해주시면 감사하겠습니다!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하.. 조이의 설명과 같이 보니 이해가 가네요..!
무슨 말씀이신지 이해했습니다 적용해볼게요!

public init(repository: WhiteboardRepositoryInterface) {
public init(repository: WhiteboardRepositoryInterface, profile: Profile) {
self.repository = repository
participantsInfo["participants"] = profile.profileIcon.emoji
Copy link
Collaborator

Choose a reason for hiding this comment

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

participantsInfo에는 어떤 정보가 들어가나요?
"participants" 라는 키 값 이외에 어떤 키 값을 사용가능한 지 궁금합니다!!

Copy link
Collaborator Author

@ekrud99 ekrud99 Nov 18, 2024

Choose a reason for hiding this comment

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

participantsInfo는 현재 ["participants": "😃,😥,😄"] 형태로 설계되어있으며, invitation 요청 후 성공할 때 마다 뒤에 참여자의 아이콘을 추가하는 방향으로 구현되지않을까 생각합니다.
이후 advertising 할 때 participantsInfo를 넘기고, browsing 하는 유저가 whiteboard cell의 참여자 정보를 표시할 때 사용합니다.
현재 해당 키값 외의 사용처는 없습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

그렇다면, array나 set을 고려하는건 어떤가요? 하나의 키 값으로 value에 아이콘들을 하나씩 붙여 추가하는 것은 조금은 어색하게 느껴지는 것 같습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반영해보겠습니다!

case startDrawing(startAt: CGPoint)
case addDrawingPoint(point: CGPoint)
case finishUsingTool
case publishWithInfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

따로 View쪽에서 info를 가지고 publish 할지, 그냥 publish 할지를 알아야 하는게 아니라면 publish라는 네이밍은 어떨까요?
View는 최대한 멍청한게 좋다고 생각하기 때문입니다!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

추가로, 어떤 경우에 View에서 해당 Input을 전달해주는지 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

따로 View쪽에서 info를 가지고 publish 할지, 그냥 publish 할지를 알아야 하는게 아니라면 publish라는 네이밍은 어떨까요? View는 최대한 멍청한게 좋다고 생각하기 때문입니다!!

동의합니다! 네이밍 수정해보겠습니다.

추가로, 어떤 경우에 View에서 해당 Input을 전달해주는지 궁금합니다!

input을 전달해주는 경우는 아마 해당 화이트보드에 새로운 참여자가 등장해서 호스트가 참여자 정보를 갱신한 뒤 advertising 할 때 라고 예상합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

동의합니다!! 다만, view쪽에서도 내가 호스트이고, 새로운 참여자가 들어왔다!! 라는 사실을 알고 있어야 할지가 궁금합니다.!.!
호스트가 버튼을 눌러 물리적으로 방 새로고침하는 경우가 아니라면, view에서 input을 통해 전해주지 않아도 괜찮다고 생각합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

생각해보니까 이건 input으로 처리할 필요가 없군요..
감사합니다!

Copy link
Collaborator

@choijungp choijungp left a comment

Choose a reason for hiding this comment

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

PoC까지 직접하고 .. 고생 많았습니다우니 ~~ 👍🏻
그래서 안됐던 이유가 무엇이었고 어떻게 수정했더니 동작했는지 설명해주실 수 있을까요 ?? 🥺

public func startPublishing() {
nearbyNetwork.startPublishing()
public func startPublishing(with info: [String: String]) {
nearbyNetwork.startPublishing(with: info)
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 ! 띵똥의 코멘트를 보니 여기서 nearbyNetwork의 메서드를 실행할 때 딕셔너리로 바꿔주는 것도 좋은 방법 같다고 생각합니다. !!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

확인했습니다~~

PoC 중 advertising을 stop하고, discoveryInfo를 바꾼 뒤 바로 advertising을 start하는 과정에서 예상대로 동작하지 않는 이슈가 있었습니다.
때문에 현재 작성된 로직과 같이 3초의 텀을 두는 식으로 해결했는데.. 더 좋은 방법이 있다면 알려주세요!! 😭

Copy link
Member

@eemdeeks eemdeeks left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다..!!!

고민을 많이 하셨을 것 같아요ㅜㅜ할게 아직 많네요.. 화이팅입니다..

private var participantsInfo: [String] = []

public init(repository: WhiteboardRepositoryInterface) {
public init(repository: WhiteboardRepositoryInterface, profile: Profile) {
Copy link
Member

Choose a reason for hiding this comment

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

첫 생성이라 본인의 프로필만 넣어주는 것으로 이해가 되네요!

startPublishingWhiteboard()
와 같이 누군가가 들어왔을 때, participantsInfo를 변경시켜주는 로직도 필요해 보입니다ㅜㅜ

Copy link
Collaborator

@taipaise taipaise left a comment

Choose a reason for hiding this comment

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

poc부터 설계 구현까지!!! 고생하셨습니다!!
코드에 다우니의 고민이 잘 느껴지는 것 같습니다!

@ekrud99 ekrud99 merged commit 6f7b802 into develop Nov 19, 2024
2 checks passed
@ekrud99 ekrud99 deleted the feature/AdvertisingWhiteboard branch November 20, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants