NAVAND-552: predownload voice instructions #6547
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| package com.mapbox.navigation.core.internal | ||
|
|
||
| import androidx.annotation.UiThread | ||
| import androidx.annotation.VisibleForTesting | ||
| import com.mapbox.navigation.core.MapboxNavigation | ||
| import java.util.concurrent.CopyOnWriteArraySet | ||
|
|
||
| fun interface MapboxNavigationCreateObserver { | ||
|
|
||
| fun onCreated(mapboxNavigation: MapboxNavigation) | ||
| } | ||
|
|
||
| @Deprecated("Used to keep track of MapboxNavigation instances created via deprecated methods") | ||
| @UiThread | ||
| object LegacyMapboxNavigationInstanceHolder { | ||
kmadsen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| @Volatile | ||
| private var mapboxNavigation: MapboxNavigation? = null | ||
|
|
||
| private val createObservers = CopyOnWriteArraySet<MapboxNavigationCreateObserver>() | ||
|
|
||
| fun onCreated(instance: MapboxNavigation) { | ||
| mapboxNavigation = instance | ||
| createObservers.forEach { it.onCreated(instance) } | ||
| } | ||
|
|
||
| fun onDestroyed() { | ||
| mapboxNavigation = null | ||
| } | ||
|
|
||
| fun peek(): MapboxNavigation? = mapboxNavigation | ||
|
|
||
| fun registerCreateObserver(observer: MapboxNavigationCreateObserver) { | ||
| createObservers.add(observer) | ||
| mapboxNavigation?.let { observer.onCreated(it) } | ||
| } | ||
|
|
||
| fun unregisterCreateObserver(observer: MapboxNavigationCreateObserver) { | ||
| createObservers.remove(observer) | ||
| } | ||
|
|
||
| @VisibleForTesting(otherwise = VisibleForTesting.NONE) | ||
| fun unregisterAllCreateObservers() { | ||
| createObservers.clear() | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| package com.mapbox.navigation.core | ||
|
|
||
| import com.mapbox.navigation.core.internal.LegacyMapboxNavigationInstanceHolder | ||
| import com.mapbox.navigation.core.internal.MapboxNavigationCreateObserver | ||
| import com.mapbox.navigation.testing.LoggingFrontendTestRule | ||
| import io.mockk.clearMocks | ||
| import io.mockk.mockk | ||
| import io.mockk.verify | ||
| import org.junit.After | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertNull | ||
| import org.junit.Before | ||
| import org.junit.Rule | ||
| import org.junit.Test | ||
| import org.junit.runner.RunWith | ||
| import org.robolectric.RobolectricTestRunner | ||
|
|
||
| @RunWith(RobolectricTestRunner::class) | ||
| class LegacyMapboxNavigationInstanceHolderTest { | ||
|
|
||
| @get:Rule | ||
| val loggerRule = LoggingFrontendTestRule() | ||
| private val observer = mockk<MapboxNavigationCreateObserver>(relaxed = true) | ||
| private val mapboxNavigation = mockk<MapboxNavigation>(relaxed = true) | ||
|
|
||
| @Before | ||
| fun setUp() { | ||
| LegacyMapboxNavigationInstanceHolder.unregisterAllCreateObservers() | ||
| LegacyMapboxNavigationInstanceHolder.onDestroyed() | ||
| } | ||
|
|
||
| @After | ||
| fun tearDown() { | ||
| LegacyMapboxNavigationInstanceHolder.unregisterAllCreateObservers() | ||
| LegacyMapboxNavigationInstanceHolder.onDestroyed() | ||
| } | ||
|
|
||
| @Test | ||
| fun `observer is invoked on registration if has mapboxNavigation`() { | ||
| LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) | ||
|
|
||
| LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer) | ||
|
|
||
| verify(exactly = 1) { observer.onCreated(mapboxNavigation) } | ||
| } | ||
|
|
||
| @Test | ||
| fun `observer is not invoked on registration if has no mapboxNavigation`() { | ||
| LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer) | ||
|
|
||
| verify(exactly = 0) { observer.onCreated(mapboxNavigation) } | ||
| } | ||
|
|
||
| @Test | ||
| fun `observer is not invoked on registration if has destroyed mapboxNavigation`() { | ||
| LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) | ||
| LegacyMapboxNavigationInstanceHolder.onDestroyed() | ||
|
|
||
| LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer) | ||
|
|
||
| verify(exactly = 0) { observer.onCreated(mapboxNavigation) } | ||
| } | ||
|
|
||
| @Test | ||
| fun `observer is invoked when mapboxNavigation is created`() { | ||
| LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer) | ||
|
|
||
| LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) | ||
|
|
||
| verify(exactly = 1) { observer.onCreated(mapboxNavigation) } | ||
| } | ||
|
|
||
| @Test | ||
| fun `observer is invoked when new mapboxNavigation is created`() { | ||
| LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer) | ||
| LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) | ||
| clearMocks(observer, answers = false) | ||
|
|
||
| val newNavigation = mockk<MapboxNavigation>(relaxed = true) | ||
| LegacyMapboxNavigationInstanceHolder.onCreated(newNavigation) | ||
|
|
||
| verify(exactly = 1) { observer.onCreated(newNavigation) } | ||
| } | ||
|
|
||
| @Test | ||
| fun `removed observer is not invoked when mapboxNavigation is created`() { | ||
| LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer) | ||
| LegacyMapboxNavigationInstanceHolder.unregisterCreateObserver(observer) | ||
|
|
||
| LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) | ||
|
|
||
| verify(exactly = 0) { observer.onCreated(any()) } | ||
| } | ||
|
|
||
| @Test | ||
| fun `multiple observers are invoked when new mapboxNavigation is created`() { | ||
| val secondObserver = mockk<MapboxNavigationCreateObserver>(relaxed = true) | ||
| LegacyMapboxNavigationInstanceHolder.registerCreateObserver(observer) | ||
| LegacyMapboxNavigationInstanceHolder.registerCreateObserver(secondObserver) | ||
|
|
||
| LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) | ||
|
|
||
| verify(exactly = 1) { | ||
| observer.onCreated(mapboxNavigation) | ||
| secondObserver.onCreated(mapboxNavigation) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `peek with no instance`() { | ||
| assertNull(LegacyMapboxNavigationInstanceHolder.peek()) | ||
| } | ||
|
|
||
| @Test | ||
| fun `peek with active instance`() { | ||
| LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) | ||
|
|
||
| assertEquals(mapboxNavigation, LegacyMapboxNavigationInstanceHolder.peek()) | ||
| } | ||
|
|
||
| @Test | ||
| fun `peek with destroyed instance`() { | ||
| LegacyMapboxNavigationInstanceHolder.onCreated(mapboxNavigation) | ||
| LegacyMapboxNavigationInstanceHolder.onDestroyed() | ||
|
|
||
| assertNull(LegacyMapboxNavigationInstanceHolder.peek()) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is weird, why are we adding a new
@Deprecatedobject? 🤔Uh oh!
There was an error while loading. Please reload this page.
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.
I have to register RoutesObserver and RouteProgressObserver. And there are multiple ways of creating
MapboxNavigation:MapboxNavigationProvider(deprecated)MapboxNavigationAppThe "correct" way of observing
MapboxNavigationis usingMapboxNavigationAppbut the other ways are widely used and I want the feature to work for all the users. SoLegacyMapboxNavigationInstanceHoldertracks allMapboxNavigationinstances. Not sure if the naming is very good but I wanted to emphasize by this that theMapboxNavigationAppis the appropriate way to do it andLegacyMapboxNavigationInstanceHolderis just for us to support other creation methods.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.
LegacyMapboxNavigationInstanceHolderis an internal object. User aren't supposed to use it anyway, are they?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.
They aren't. It's just a note for us. I can just add a comment or sth instead of deprecating it.
But actually I'm thinking that it should not be deprecated at all. We still expose a valid way of creating
MapboxNavigationvia constructor and some customers use it. So it's not really "legacy", it's more like "everything including legacy". I just don't like it that we haveMapboxNavigationAppand this new way of observing. But since the new one is internal, it's not that big a deal.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.
LegacyMapboxNavigationInstanceHolderis definitely not an improvement toMapboxNavigationAppthough, in my opinion.MapboxNavigationCreateObserveris also redundant.. It is the same asMapboxNavigationObserver.onAttached.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.
I's not, see #6547 (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.
This is not a reason to require an automatic link. It can be done after MapboxNavigation is constructed