-
Notifications
You must be signed in to change notification settings - Fork 12
[김예란_Android] 10주차 과제 제출 #75
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
base: main
Are you sure you want to change the base?
Conversation
skdud0629
left a comment
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.
수고하셨습니다! 중복된 로직 신경 써주시면 좋을 것 같습니다:)
|
|
||
| override fun onResume() { | ||
| super.onResume() | ||
| if (hasPermission()) showMusicList() |
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.
oncreate에서 해당 로직을 사용하셨는데 onResume에서도 사용하는 이유가 있을까요? 생명주기에 다시 생각해보셨으면 좋겠습니다.
| recyclerView.visibility = View.VISIBLE | ||
| permissionMessage.visibility = View.GONE | ||
| openSettingsButton.visibility = View.GONE | ||
| requestPermissionButton.visibility = View.GONE |
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.
Visibility를 변경하는 로직은 따로 빼주는 게 좋을 것 같네요 중복되는 부분이 많습니다.
| else | ||
| Manifest.permission.READ_EXTERNAL_STORAGE | ||
| permissionLauncher.launch(permission) | ||
| } |
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.
권한 체크하는 로직이 중복되었네요.
| val pendingIntent = PendingIntent.getActivity( | ||
| this, 0, notifIntent, PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE | ||
| ) | ||
| val notification = NotificationCompat.Builder(this, "music_channel") |
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.
채널 id는 상수를 사용해주세요.
| import android.os.IBinder | ||
| import androidx.core.app.NotificationCompat | ||
|
|
||
| class MusicService : Service() { |
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.
| class MusicService : Service() { | |
| class MusicService : BindService() { |
JaeYoung290
left a comment
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.
과제하시느라 수고 많으셨습니다!
| val idIdx = it.getColumnIndex(MediaStore.Audio.Media._ID) | ||
| val titleIdx = it.getColumnIndex(MediaStore.Audio.Media.TITLE) | ||
| val artistIdx = it.getColumnIndex(MediaStore.Audio.Media.ARTIST) | ||
| val durationIdx = it.getColumnIndex(MediaStore.Audio.Media.DURATION) |
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.
getColumnIndex는 해당 column이 없을 경우 -1을 반환하는 함수입니다 이때에 대한 예외 처리 로직도 추가해주시면 좋을 것 같습니다
| recyclerView.visibility = View.GONE | ||
| permissionMessage.visibility = View.GONE | ||
| openSettingsButton.visibility = View.GONE | ||
| requestPermissionButton.visibility = View.GONE |
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.
위의 뷰를 초기화하는 4개 줄은 xml에서 visibility에 초기값을 주는 방식으로 개선해도 괜찮을 것으로 보입니다
| android:padding="12dp"> | ||
|
|
||
| <TextView | ||
| android:id="@+id/textTitle" |
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.
xml의 네이밍 컨벤션은 lower camel case를 권장드립니다
| setContentView(R.layout.activity_main) | ||
|
|
||
| recyclerView = findViewById(R.id.MusicRecyclerView) | ||
| permissionMessage = findViewById(R.id.text1) |
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.
메인 xml layout에서는 text1이 없는 것으로 보이는 데 어떤 텍스트뷰일까요?
| ActivityResultContracts.RequestPermission() | ||
| ) { granted -> | ||
| if (granted) showMusicList() | ||
| else showPermissionUI() |
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.
현재 코드에서는 뷰를 조절하는 것으로 권한 관련 UI를 표시하는 것으로 보입니다 visible로 조절하는 것 보다는 shouldShowRequestPermissionRationale() 등으로 분기처리 해주시는 것을 권장드립니다
| openSettingsButton.visibility = View.GONE | ||
| requestPermissionButton.visibility = View.GONE | ||
| loadMusicList() | ||
| } |
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.
showPermissionUI와 showMusicList에서 UI 관련 코드가 중복되었는데 이럴때는 따로 확장 함수로 구현하시는 것을 추천드립니다
No description provided.