Skip to content

Commit 81088f4

Browse files
dzinaddzinad
committed
NAVAND-552: meet code review
1 parent 26508a1 commit 81088f4

File tree

13 files changed

+102
-139
lines changed

13 files changed

+102
-139
lines changed

‎examples/src/main/java/com/mapbox/navigation/examples/core/MapboxVoiceActivity.kt‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ import java.util.Locale
8181
* attention to its usage. Long press anywhere on the map to set a destination and trigger
8282
* navigation.
8383
*/
84+
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
8485
class MapboxVoiceActivity : AppCompatActivity(), OnMapLongClickListener {
8586

8687
private var isMuted: Boolean = false
@@ -203,7 +204,6 @@ class MapboxVoiceActivity : AppCompatActivity(), OnMapLongClickListener {
203204
}
204205
}
205206

206-
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
207207
private val voiceInstructionsPrefetcher by lazy {
208208
VoiceInstructionsPrefetcher(speechApi)
209209
}
@@ -389,43 +389,39 @@ class MapboxVoiceActivity : AppCompatActivity(), OnMapLongClickListener {
389389
.build()
390390
)
391391
init()
392+
voiceInstructionsPrefetcher.onAttached(mapboxNavigation)
392393
}
393394

394-
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
395395
override fun onStart() {
396396
super.onStart()
397397
if (::mapboxNavigation.isInitialized) {
398398
mapboxNavigation.registerRoutesObserver(routesObserver)
399399
mapboxNavigation.registerLocationObserver(locationObserver)
400-
voiceInstructionsPrefetcher.onAttached(mapboxNavigation)
401400
mapboxNavigation.registerRouteProgressObserver(routeProgressObserver)
402401
mapboxNavigation.registerRouteProgressObserver(replayProgressObserver)
403402
mapboxNavigation.registerVoiceInstructionsObserver(voiceInstructionsObserver)
404403
}
405404
ResourceLoaderFactory.getInstance().registerObserver(resourceLoadObserver)
406405
}
407406

408-
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
409407
override fun onStop() {
410408
super.onStop()
411409
ResourceLoaderFactory.getInstance().unregisterObserver(resourceLoadObserver)
412410
mapboxNavigation.unregisterRoutesObserver(routesObserver)
413-
voiceInstructionsPrefetcher.onDetached(mapboxNavigation)
414411
mapboxNavigation.unregisterLocationObserver(locationObserver)
415412
mapboxNavigation.unregisterRouteProgressObserver(routeProgressObserver)
416413
mapboxNavigation.unregisterRouteProgressObserver(replayProgressObserver)
417414
mapboxNavigation.unregisterVoiceInstructionsObserver(voiceInstructionsObserver)
418415
}
419416

420-
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
421417
override fun onDestroy() {
422418
super.onDestroy()
423419
routeLineApi.cancel()
424420
routeLineView.cancel()
425421
mapboxReplayer.finish()
426422
mapboxNavigation.onDestroy()
427423
speechApi.cancel()
428-
voiceInstructionsPrefetcher.destroy()
424+
voiceInstructionsPrefetcher.onDetached(mapboxNavigation)
429425
voiceInstructionsPlayer.shutdown()
430426
}
431427

‎libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2020,15 +2020,6 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
20202020
}
20212021
}
20222022

2023-
private class TestVoiceInstructionsTriggerObserver : RoutesObserver, RouteProgressObserver {
2024-
2025-
override fun onRouteProgressChanged(routeProgress: RouteProgress) {
2026-
}
2027-
2028-
override fun onRoutesChanged(result: RoutesUpdatedResult) {
2029-
}
2030-
}
2031-
20322023
private fun alternativeWithId(mockId: String): RouteAlternative {
20332024
val mockedRoute = mockk<RouteInterface> {
20342025
every { routeId } returns mockId

‎libnavui-voice/api/current.txt‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ package com.mapbox.navigation.ui.voice.api {
6464
method public void cancel();
6565
method public void clean(com.mapbox.navigation.ui.voice.model.SpeechAnnouncement announcement);
6666
method public void generate(com.mapbox.api.directions.v5.models.VoiceInstructions voiceInstruction, com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer<com.mapbox.bindgen.Expected<com.mapbox.navigation.ui.voice.model.SpeechError,com.mapbox.navigation.ui.voice.model.SpeechValue>> consumer);
67-
method public void generatePredownloaded(com.mapbox.api.directions.v5.models.VoiceInstructions voiceInstruction, com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer<com.mapbox.bindgen.Expected<com.mapbox.navigation.ui.voice.model.SpeechError,com.mapbox.navigation.ui.voice.model.SpeechValue>> consumer);
67+
method @com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI public void generatePredownloaded(com.mapbox.api.directions.v5.models.VoiceInstructions voiceInstruction, com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer<com.mapbox.bindgen.Expected<com.mapbox.navigation.ui.voice.model.SpeechError,com.mapbox.navigation.ui.voice.model.SpeechValue>> consumer);
6868
}
6969

7070
@UiThread public final class MapboxVoiceInstructionsPlayer {
@@ -95,8 +95,6 @@ package com.mapbox.navigation.ui.voice.api {
9595

9696
@com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI public final class VoiceInstructionsPrefetcher implements com.mapbox.navigation.core.lifecycle.MapboxNavigationObserver {
9797
ctor public VoiceInstructionsPrefetcher(com.mapbox.navigation.ui.voice.api.MapboxSpeechApi speechApi);
98-
ctor public VoiceInstructionsPrefetcher(int observableTime, double timePercentageToTriggerAfter, com.mapbox.navigation.ui.voice.api.MapboxSpeechApi speechApi);
99-
method public void destroy();
10098
method public void onAttached(com.mapbox.navigation.core.MapboxNavigation mapboxNavigation);
10199
method public void onDetached(com.mapbox.navigation.core.MapboxNavigation mapboxNavigation);
102100
field public static final com.mapbox.navigation.ui.voice.api.VoiceInstructionsPrefetcher.Companion Companion;

‎libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxAudioGuidance.kt‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ internal constructor(
4242

4343
private var dataStoreOwner: NavigationDataStoreOwner? = null
4444
private var configOwner: NavigationConfigOwner? = null
45-
private var audioGuidanceVoice: MapboxAudioGuidanceVoice? = null
45+
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
46+
private var trigger: VoiceInstructionsPrefetcher? = null
4647
private var mutedStateFlow = MutableStateFlow(false)
4748
private val internalStateFlow = MutableStateFlow(MapboxAudioGuidanceState())
4849
private val scope = CoroutineScope(SupervisorJob() + dispatcher)
@@ -72,9 +73,10 @@ internal constructor(
7273
/**
7374
* @see [MapboxNavigationApp]
7475
*/
76+
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
7577
override fun onDetached(mapboxNavigation: MapboxNavigation) {
7678
mapboxVoiceInstructions.unregisterObservers(mapboxNavigation)
77-
audioGuidanceVoice?.destroy()
79+
trigger?.onDetached(mapboxNavigation)
7880
job?.cancel()
7981
job = null
8082
}
@@ -165,17 +167,14 @@ internal constructor(
165167

166168
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
167169
private fun MapboxNavigation.audioGuidanceVoice(): Flow<MapboxAudioGuidanceVoice> {
168-
var trigger: VoiceInstructionsPrefetcher? = null
169170
return combine(
170171
mapboxVoiceInstructions.voiceLanguage(),
171172
configOwner!!.language(),
172173
) { voiceLanguage, deviceLanguage -> voiceLanguage ?: deviceLanguage }
173174
.distinctUntilChanged()
174175
.map { language ->
175-
audioGuidanceVoice?.destroy()
176176
trigger?.onDetached(this)
177177
audioGuidanceServices.mapboxAudioGuidanceVoice(this, language).also {
178-
audioGuidanceVoice = it
179178
trigger = VoiceInstructionsPrefetcher(it.mapboxSpeechApi).also { trigger ->
180179
trigger.onAttached(this)
181180
}

‎libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/MapboxSpeechApi.kt‎

Lines changed: 25 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package com.mapbox.navigation.ui.voice.api
22

33
import android.content.Context
4+
import androidx.annotation.UiThread
45
import com.mapbox.api.directions.v5.models.VoiceInstructions
56
import com.mapbox.bindgen.Expected
67
import com.mapbox.bindgen.ExpectedFactory
8+
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
79
import com.mapbox.navigation.core.trip.session.VoiceInstructionsObserver
810
import com.mapbox.navigation.ui.base.util.MapboxNavigationConsumer
911
import com.mapbox.navigation.ui.voice.model.SpeechAnnouncement
@@ -13,6 +15,7 @@ import com.mapbox.navigation.ui.voice.model.TypeAndAnnouncement
1315
import com.mapbox.navigation.ui.voice.model.VoiceState
1416
import com.mapbox.navigation.ui.voice.options.MapboxSpeechApiOptions
1517
import com.mapbox.navigation.utils.internal.InternalJobControlFactory
18+
import kotlinx.coroutines.Dispatchers
1619
import kotlinx.coroutines.cancel
1720
import kotlinx.coroutines.launch
1821
import java.util.Locale
@@ -31,11 +34,10 @@ class MapboxSpeechApi @JvmOverloads constructor(
3134
private val options: MapboxSpeechApiOptions = MapboxSpeechApiOptions.Builder().build()
3235
) {
3336

34-
private val cachedFilesLock = Any()
3537
private val cachedFiles = mutableMapOf<TypeAndAnnouncement, SpeechValue>()
3638
private val mainJobController by lazy { InternalJobControlFactory.createMainScopeJobControl() }
37-
private val predownloadScope by lazy {
38-
InternalJobControlFactory.createDefaultScopeJobControl().scope
39+
private val predownloadJobController by lazy {
40+
InternalJobControlFactory.createDefaultScopeJobControl()
3941
}
4042
private val voiceAPI = VoiceApiProvider.retrieveMapboxVoiceApi(
4143
context,
@@ -80,6 +82,7 @@ class MapboxSpeechApi @JvmOverloads constructor(
8082
* with the raw announcement (without file) that can be played with a text-to-speech engine.
8183
* @see [cancel]
8284
*/
85+
@ExperimentalPreviewMapboxNavigationAPI
8386
fun generatePredownloaded(
8487
voiceInstruction: VoiceInstructions,
8588
consumer: MapboxNavigationConsumer<Expected<SpeechError, SpeechValue>>
@@ -119,29 +122,26 @@ class MapboxSpeechApi @JvmOverloads constructor(
119122
fun clean(announcement: SpeechAnnouncement) {
120123
voiceAPI.clean(announcement)
121124
VoiceInstructionsParser.parse(announcement).onValue {
122-
synchronized(cachedFilesLock) {
123-
val value = cachedFiles[it]
124-
// when we clear fallback announcement, there is a chance we will remove the key
125-
// from map and not remove the file itself
126-
// since for fallback SpeechAnnouncement file is null
127-
if (value?.announcement == announcement) {
128-
cachedFiles.remove(it)
129-
}
125+
val value = cachedFiles[it]
126+
// when we clear fallback announcement, there is a chance we will remove the key
127+
// from map and not remove the file itself
128+
// since for fallback SpeechAnnouncement file is null
129+
if (value?.announcement == announcement) {
130+
cachedFiles.remove(it)
130131
}
131132
}
132133
}
133134

135+
@UiThread
134136
internal fun predownload(instructions: List<VoiceInstructions>) {
135-
mainJobController.scope.launch {
136-
instructions.forEach { instruction ->
137-
val typeAndAnnouncement = VoiceInstructionsParser.parse(instruction).value
138-
if (typeAndAnnouncement != null && !hasTypeAndAnnouncement(typeAndAnnouncement)) {
139-
predownloadScope.launch {
140-
retrieveVoiceFile(instruction) {
137+
instructions.forEach { instruction ->
138+
val typeAndAnnouncement = VoiceInstructionsParser.parse(instruction).value
139+
if (typeAndAnnouncement != null && !hasTypeAndAnnouncement(typeAndAnnouncement)) {
140+
predownloadJobController.scope.launch {
141+
retrieveVoiceFile(instruction) {
142+
mainJobController.scope.launch(Dispatchers.Main.immediate) {
141143
it.onValue { speechValue ->
142-
synchronized(cachedFilesLock) {
143-
cachedFiles[typeAndAnnouncement] = speechValue
144-
}
144+
cachedFiles[typeAndAnnouncement] = speechValue
145145
}
146146
}
147147
}
@@ -151,11 +151,9 @@ class MapboxSpeechApi @JvmOverloads constructor(
151151
}
152152

153153
internal fun destroy() {
154-
predownloadScope.cancel()
155-
synchronized(cachedFilesLock) {
156-
val announcements = cachedFiles.map { it.value.announcement }
157-
announcements.forEach { clean(it) }
158-
}
154+
predownloadJobController.job.children.forEach { it.cancel() }
155+
val announcements = cachedFiles.map { it.value.announcement }
156+
announcements.forEach { clean(it) }
159157
}
160158

161159
@Throws(IllegalStateException::class)
@@ -202,15 +200,11 @@ class MapboxSpeechApi @JvmOverloads constructor(
202200
}
203201

204202
private fun hasTypeAndAnnouncement(typeAndAnnouncement: TypeAndAnnouncement): Boolean {
205-
synchronized(cachedFilesLock) {
206-
return typeAndAnnouncement in cachedFiles
207-
}
203+
return typeAndAnnouncement in cachedFiles
208204
}
209205

210206
private fun getFromCache(voiceInstruction: VoiceInstructions): SpeechValue? {
211207
val key = VoiceInstructionsParser.parse(voiceInstruction).value
212-
synchronized(cachedFilesLock) {
213-
return key?.let { cachedFiles[it] }
214-
}
208+
return key?.let { cachedFiles[it] }
215209
}
216210
}

‎libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/api/VoiceInstructionsPrefetcher.kt‎

Lines changed: 8 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -21,46 +21,23 @@ import com.mapbox.navigation.utils.internal.ifNonNull
2121
*/
2222
@ExperimentalPreviewMapboxNavigationAPI
2323
class VoiceInstructionsPrefetcher internal constructor(
24+
private val speechApi: MapboxSpeechApi,
2425
private val observableTime: Int,
2526
private val timePercentageToTriggerAfter: Double,
26-
private val speechApi: MapboxSpeechApi,
27-
private val nextVoiceInstructionsProvider: NextVoiceInstructionsProvider,
28-
private val timeProvider: Time,
27+
private val nextVoiceInstructionsProvider: NextVoiceInstructionsProvider =
28+
TimeBasedNextVoiceInstructionsProvider(observableTime),
29+
private val timeProvider: Time = Time.SystemImpl,
2930
) : MapboxNavigationObserver {
3031

3132
/**
32-
* Creates [VoiceInstructionsPrefetcher] with default
33-
* observableTime and timePercentageToTriggerAfter.
34-
* See [DEFAULT_OBSERVABLE_TIME_SECONDS] and [DEFAULT_TIME_PERCENTAGE_TO_TRIGGER_AFTER].
33+
* Creates [VoiceInstructionsPrefetcher.
3534
*
3635
* @param speechApi [MapboxSpeechApi] instances that's used to generate instructions
3736
*/
3837
constructor(speechApi: MapboxSpeechApi) : this(
39-
DEFAULT_OBSERVABLE_TIME_SECONDS,
40-
DEFAULT_TIME_PERCENTAGE_TO_TRIGGER_AFTER,
41-
speechApi
42-
)
43-
44-
/**
45-
* Creates [VoiceInstructionsPrefetcher] with custom
46-
* observableTime and timePercentageToTriggerAfter.
47-
*
48-
* @param observableTime voice instructions will be predownloaded for `observableTime` seconds
49-
* of route ahead
50-
* @param timePercentageToTriggerAfter voice instructions predownloading will not be triggered
51-
* if `timePercentageToTriggerAfter` seconds did not pass since last predownloading session
52-
* @param speechApi [MapboxSpeechApi] instances that's used to generate instructions
53-
*/
54-
constructor(
55-
observableTime: Int,
56-
timePercentageToTriggerAfter: Double,
57-
speechApi: MapboxSpeechApi,
58-
) : this(
59-
observableTime,
60-
timePercentageToTriggerAfter,
6138
speechApi,
62-
TimeBasedNextVoiceInstructionsProvider(observableTime),
63-
Time.SystemImpl
39+
DEFAULT_OBSERVABLE_TIME_SECONDS,
40+
DEFAULT_TIME_PERCENTAGE_TO_TRIGGER_AFTER
6441
)
6542

6643
private val routesObserver = RoutesObserver { onRoutesChanged(it) }
@@ -88,6 +65,7 @@ class VoiceInstructionsPrefetcher internal constructor(
8865
override fun onDetached(mapboxNavigation: MapboxNavigation) {
8966
mapboxNavigation.unregisterRoutesObserver(routesObserver)
9067
mapboxNavigation.unregisterRouteProgressObserver(routeProgressObserver)
68+
speechApi.destroy()
9169
}
9270

9371
private fun onRoutesChanged(result: RoutesUpdatedResult) {
@@ -134,14 +112,6 @@ class VoiceInstructionsPrefetcher internal constructor(
134112
}
135113
}
136114

137-
/**
138-
* The method stops all work related to pre-downloading voice instructions and unregisters
139-
* all related callbacks. It should be invoked from `Activity#onDestroy`.
140-
*/
141-
fun destroy() {
142-
speechApi.destroy()
143-
}
144-
145115
private fun shouldDownloadBasedOnTime(): Boolean {
146116
return timeProvider.seconds() >=
147117
lastDownloadTime + observableTime * timePercentageToTriggerAfter

‎libnavui-voice/src/main/java/com/mapbox/navigation/ui/voice/internal/MapboxAudioGuidanceVoice.kt‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.mapbox.navigation.ui.voice.internal
22

33
import com.mapbox.api.directions.v5.models.VoiceInstructions
4+
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
45
import com.mapbox.navigation.ui.voice.api.MapboxSpeechApi
56
import com.mapbox.navigation.ui.voice.api.MapboxVoiceInstructionsPlayer
67
import com.mapbox.navigation.ui.voice.model.SpeechAnnouncement
@@ -41,10 +42,7 @@ class MapboxAudioGuidanceVoice(
4142
}
4243
}
4344

44-
fun destroy() {
45-
mapboxSpeechApi.destroy()
46-
}
47-
45+
@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
4846
private suspend fun MapboxSpeechApi.generate(
4947
instructions: VoiceInstructions
5048
): SpeechAnnouncement = suspendCancellableCoroutine { cont ->

‎libnavui-voice/src/test/java/com/mapbox/navigation/ui/voice/TestMapboxAudioGuidanceServices.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ class TestMapboxAudioGuidanceServices(
3636

3737
private val mapboxSpeechApi = mockk<MapboxSpeechApi>(relaxed = true)
3838

39-
val mapboxAudioGuidanceVoice = mockk<MapboxAudioGuidanceVoice> {
39+
val mapboxAudioGuidanceVoice = mockk<MapboxAudioGuidanceVoice>(relaxed = true) {
4040
coEvery { speak(any()) } coAnswers {
4141
val voiceInstructions = firstArg<VoiceInstructions?>()
4242
val speechAnnouncement: SpeechAnnouncement? = voiceInstructions?.let {

0 commit comments

Comments
 (0)