Ms 720 add audio notification for finger removal after scanning#913
Conversation
meladRaouf
force-pushed
the
MS-720-Add-Audio-Notification-for-Finger-Removal-After-Scanning
branch
from
a997e82 to
a80d3b6
Compare
meladRaouf
requested review from
a team,
BurningAXE,
TristramN,
alex-vt,
alexandr-simprints,
luhmirin-s and
ybourgery
and removed request for
a team
| private fun isAudioEnabled() = | ||
| PreferenceManager | ||
| .getDefaultSharedPreferences(requireContext()) | ||
| .getBoolean(AUDIO_PREFERENCE_KEY, true) |
There was a problem hiding this comment.
I haven't yet reached an agreement with Jon regarding the default value of the preference.
There was a problem hiding this comment.
The final agreement is to make it false
| } | ||
|
|
||
| private fun playBeep() { | ||
| val mediaPlayer = MediaPlayer.create(context, R.raw.beep) |
There was a problem hiding this comment.
Can the MediaPlayer instance be created on the Fragment properties level so that we don't have to re-initialize it every time? If memory serves me right, the MediaPlayer creation might be resource-intensive due to codec and decoders initialization etc.
There was a problem hiding this comment.
If you decide that creating a media player instance makes sense, then please also make sure that it is re-created correctly once the device is rotated
There was a problem hiding this comment.
I would also suggest moving it together with the PreferenceManager to some kind of injectable (ideally singleton) manager class.
There was a problem hiding this comment.
It doesn't seem to me that the LiveData object is the way to go in a tracker that might be used outside of the lifecycle-aware classes.
Using SharedFlow seems more appropriate
class FingerprintScanningStatusTracker @Inject constructor() {
private val _scanCompleted = MutableSharedFlow<Unit>()
val scanCompleted = _scanCompleted.asSharedFlow()
suspend fun notifyScanCompleted() {
_scanCompleted.emit(Unit)
}
}
| } | ||
|
|
||
| private fun playBeep() { | ||
| val mediaPlayer = MediaPlayer.create(context, R.raw.beep) |
There was a problem hiding this comment.
I would also suggest moving it together with the PreferenceManager to some kind of injectable (ideally singleton) manager class.
| distributionBase=GRADLE_USER_HOME | ||
| distributionPath=wrapper/dists | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.4-bin.zip | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.10-bin.zip |
There was a problem hiding this comment.
This should be a part of version bump PR, not a feature PR.
There was a problem hiding this comment.
I was testing the KSP rollback
|
|
||
| // Android X | ||
| implementation(libs.androidX.ui.viewpager2) | ||
| implementation(libs.androidX.ui.preference) |
There was a problem hiding this comment.
Why do you need preferences UI in this module?
There was a problem hiding this comment.
To access the PrefsManger class as it is part of the androidx prefs lib
There was a problem hiding this comment.
Would it make sense to decouple the actual setting from the UI?
luhmirin-s
left a comment
luhmirin-s
left a comment
There was a problem hiding this comment.
Pressed the wrong button :( Was supposed to be just comments.
…dd-Audio-Notification-for-Finger-Removal-After-Scanning
|
| import javax.inject.Singleton | ||
|
|
||
| @Singleton | ||
| class FingerprintScanningStatusTracker @Inject constructor() { |
There was a problem hiding this comment.
This is a bit of a nitpick, but from the name and usage in the code above, I expected this to handle multiple status values. IMO, this class should either handle all the status changes (at least have an easy way to extend to do so) or have a less generic name.
There was a problem hiding this comment.
@luhmirin-s in this story I will introduce the idle success and failure flows
meladRaouf
dismissed
alexandr-simprints’s stale review
The media player is now initialized when first needed and then reused for subsequent scans.
meladRaouf
deleted the
MS-720-Add-Audio-Notification-for-Finger-Removal-After-Scanning
branch
3 participants
No description provided.