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

[Feature]#72 홈 화면의 복사한 링크 저장하기 기능 구현 #75

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

l5x5l
Copy link
Contributor

@l5x5l l5x5l commented Sep 28, 2024

Key Changes

PR 유형

어떤 변경 사항이 있나요?

  • 새로운 기능 추가
  • 버그 수정
  • CSS 등 사용자 UI 디자인 변경
  • 코드에 영향을 주지 않는 변경사항(오타 수정, 탭 사이즈 변경, 변수명 변경)
  • 코드 리팩토링
  • 주석 추가 및 수정
  • 문서 수정
  • 테스트 추가, 테스트 리팩토링
  • 빌드 부분 혹은 패키지 매니저 수정
  • 파일 혹은 폴더명 수정
  • 파일 혹은 폴더 삭제

To Reviewers

  • 복사한 링크에 대해 Toast UI를 표시하는 대략적인 과정은 아래와 같습니다
  1. MainActivity의 onWindowFocusChanged에서 hasFocus가 true일시 clipbaordManager로부터 복사된 링크를 가져온다.
  2. 해당 링크가 url 형식에 맞다면, feature:home 모듈 내 새로 정의한 ClipboardLinkManager(싱글톤)의 setClipboardLink를 호출한다.
  3. ClipboardLinkManager의 setClipboardLink은 인자로 전달받은 링크 문자열을 MutableEventFlow (SharedFlow 변형)에 emit한다.
  4. 이 eventFlow(ClipboardLinkManager.clipboardLinkUrl)은 PokitViewModel 내에서 collect하고 있으며, collect시 HomeScreen에서 이를 감지하여 Toast UI를 표시한다.

코드 분석하면서 호출 과정이 헷갈리거나 조금 더 개선할 수 있는 방향 있다면 알려줘!

PR Checklist

PR이 다음 요구 사항을 충족하는지 확인하세요.

  • 커밋 메시지 컨벤션에 맞게 작성했습니다.
  • 정해진 코딩 컨벤션에 맞게 작성했습니다.
  • 변경 사항에 대한 테스트를 했습니다.(버그 수정/기능에 대한 테스트)

Etc.

로그인 화면에서 자동 로그인 성공시 홈 화면이 2번 호출되는 현상이 있어서 해당 부분 수정했어!

@l5x5l l5x5l linked an issue Sep 28, 2024 that may be closed by this pull request
2 tasks
Copy link
Member

@jiwon2724 jiwon2724 left a comment

Choose a reason for hiding this comment

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

고생하셨씁니다~~!

클립보드 로직 읽기 쉬워서 좋았슴미다bbb 짱짱


ClipboardLinkManager.setClipboardLink(clipboardTextData)
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
clipboardManager.clearPrimaryClip()
Copy link
Member

Choose a reason for hiding this comment

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

여기서 현재 클립된 데이터를 지우는 이유가 있나영?.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

클립보드 복사 -> 홈 화면 진입 -> 링크 추가 화면 진입 및 링크 추가 -> 다시 홈 화면 복귀 -> 홈 버튼 누른 후 앱 재진입
위 케이스에서 이미 저장한 클립 데이터가 남아있어서 다시 toast 메세지가 표시되는 현상이 있었어!

그래서 복사된 링크를 한번 설정하면 해당 링크로는 다시 toast 메세지가 발생하지 않게끔 하려는게 주 목적이었어

Copy link
Member

Choose a reason for hiding this comment

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

아하 요건 그러면 파이 이상에서만 발생하는 건가?.?

Copy link
Contributor Author

@l5x5l l5x5l Sep 30, 2024

Choose a reason for hiding this comment

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

어 그러네 파이 미만부분이 아예 처리가 안되어있었구나
수정완!

Comment on lines 22 to 32
fun checkUrlIsValid(url: String): Boolean {
val urlPattern = (
"^(http://|https://)?[a-z0-9]+([\\-\\.]{1}[a-z0-9]+)*\\.[a-z]{2,5}" +
"(:[0-9]{1,5})?(/.*)?$"
)

val pattern = Pattern.compile(urlPattern)
val matcher = pattern.matcher(url.lowercase(Locale.getDefault()))

return matcher.matches()
}
Copy link
Member

Choose a reason for hiding this comment

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

url 확인정도면 정규식 말구 startWith("http") or startWith("https")로 대체 가능할 것 같은뎁?
의견 궁금함미다 꼭 해당 정규식을 사용해야하는 케이스가 있을까?.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분은 그냥 url이니까 정규식 사용해야지~ 하고 크게 고민없이 작성했는데
생각해보니 형 말대로 더 간단하게 대체할 수 있겠다

"http_asdfjklasjdklfjsalkdf" 이런 문자 복사하고 들어오면 이것도 링크로 인식할 수는 있겠지만,
사용자가 이런 문자를 복사하고 이 앱에 진입하는 경우는 거의 없을 것 같아!


object ClipboardLinkManager {
private val _clipboardLinkUrl: MutableEventFlow<String> = MutableEventFlow()
val clipboardLinkUrl: EventFlow<String> = _clipboardLinkUrl.asEventFlow()
Copy link
Member

Choose a reason for hiding this comment

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

EventFlow가 뭔가 전역적으로 처리하는 그런 기능인가여?

Copy link
Contributor Author

@l5x5l l5x5l Sep 29, 2024

Choose a reason for hiding this comment

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

내부적으로 SharedFlow를 가지고 있는 Flow 구현체인데,
한번 collect된 데이터를 중복해서 collect하지 않도록 살짝 수정한 클래스로 생각하면 될듯?
아래 링크에서 6번째 항목 부분 참고하면 될 것 같아
MVVM의 ViewModel에서 이벤트를 처리하는 방법 6가지

SharedFlow 대신 EventFlow를 쓴 이유는 복사한 링크 이벤트에 대해서 막연하게 중복으로 이벤트가 collect되면 안되다는 생각이었는데, 다시 살펴보니까 이 부분에서는 EventFlow 말고 SharedFlow 사용해도 문제 없을 둣!

Copy link
Member

Choose a reason for hiding this comment

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

음 그러면 StateFlow도 사용해도 될 것 같은데, Flow마다 처리 방법을 좀 더 봐야겠담

- url링크 유효성 검증시 조건 단순화
- SDK 27 이하일 때 클립보드 초기화 누락 수정
@l5x5l l5x5l requested a review from jiwon2724 October 1, 2024 08:06
Copy link
Member

@jiwon2724 jiwon2724 left a comment

Choose a reason for hiding this comment

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

세환 고생했어~~~!!

@l5x5l l5x5l merged commit 63630e9 into develop Oct 1, 2024
1 check passed
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.

[FEATURE] 앱 진입시 링크가 복사된 경우 해당 링크 표시 구현
2 participants