mapbox / mapbox-navigation-android

Mapbox Navigation SDK for Android
https://docs.mapbox.com/android/navigation/overview/
Other
622 stars 319 forks source link

Expose API to check if offline regions are expired #4471

Open korshaknn opened 3 years ago

korshaknn commented 3 years ago

TileStore has a method tileRegionContainsDescriptors(...) which can be used by a customer to check single regions expiration. We need provide some extensions to make such checks easier It might be:

fun TileStore.checkAllRegionsTilesExpired(
    descriptorFactory: TilesetDescriptorFactory,
    callback: RegionsTilesCheckExpiredCallback
)

and for a single region

fun TileStore.checkRegionTilesExpired(
    regionId: String,
    descriptorFactory: TilesetDescriptorFactory,
    callback: RegionTilesCheckExpiredCallback
)

but in that case on android we can't cover it with unit tests, TileStore is a c++ class, it's methods can't be mocked without an additional dependency like PowerMockito.

also such methods should be implemented on each platform and we might get additional bugs.

Example (iOS has pretty close logic):

fun TileStore.checkAllRegionsTilesExpired(
    descriptorFactory: TilesetDescriptorFactory,
    callback: RegionsTilesCheckExpiredCallback
) {
    val latestDescriptors = descriptorFactory.getLatest()
    getAllTileRegions {
        if (it.isValue) {
            val regions = it.value!!
            val expiredRegions = mutableListOf<String>()
            val countDownLatch = CountDownLatch(regions.size)
            regions.forEach { region ->
                tileRegionContainsDescriptors(region.id, listOf(latestDescriptors)) { result ->
                    if (result.isValue) {
                        val regionIsExpired = result.value!!.not()
                        if (regionIsExpired) {
                            expiredRegions.add(region.id)
                        }
                    }
                    countDownLatch.countDown()
                }
            }
            countDownLatch.await()

            if (expiredRegions.isEmpty()) {
                callback.onTilesUpToDate()
            } else {
                callback.onTilesExpired(expiredRegions)
            }
        } else {
            callback.onError("type: ${it.error?.type}, message: ${it.error?.message}")
        }
    }
}

interface RegionsTilesCheckExpiredCallback {
    fun onTilesUpToDate()
    fun onTilesExpired(regionIds: List<String>)
    fun onError(message: String)
}

Should we ask common team to expose such API? Common should receive descriptors to make a check, so

it's a little confusing that platforms need to write the same extensions. It means that base API is not enough and it should be improved, doesn't it?

korshaknn commented 3 years ago

cc @Udumft @LukasPaczos @Guardiola31337 @1ec5 @zugaldia @truburt @RingerJK

Udumft commented 3 years ago

If we are suggesting tile expiration API we should mention that it should cover cases when we are checking expiration for tiles, which are not added to the TileStore. Approach above does not understand if there are no Nav tiles at all, and will report 'tiles expired' instead.

Current iOS implementation could be found here.

korshaknn commented 3 years ago

@Udumft yep, android impl was not tested (and can't be tested on android) and might have bugs. As you see there might be some edge cases and we should handle all of them on each platform, or move logic to the common.

Guardiola31337 commented 3 years ago

What about NN? It seems that this logic requires information that NN has. Shouldn't be NN the one handling / implementing this instead of common? What do you think @mskurydin @etl?

cc @kkaefer

kkaefer commented 3 years ago

checkAllRegionsTilesExpired

I recommend against using the word “expired” for this. Those tiles aren’t expired. What you’re checking here is whether there’s a new version available for Navigation tiles in that region.

I also agree that this isn’t logic that should be implemented on the SDK level since it’s identical across platforms.

mskurydin commented 3 years ago

I also agree that it seems more logical to implement these checks in the native code. However, Navigator might be not the best place for it architecturally either. All it needs is the configured tile store (which we get from above on creation).

Guardiola31337 commented 3 years ago

@mskurydin @kkaefer IIRC this was going going to be ultimately implemented in common / NN, could you please clarify what version is going to include the fix? 🙏

korshaknn commented 3 years ago

@kkaefer @mskurydin hey! where are we here? has it been added to common/nn?

kkaefer commented 3 years ago

There’s nothing that moved here.

has it been added to common/nn?

I don’t think we have a good idea of what to add to common.

We need provide some extensions to make such checks easier

Why? It’s not obvious to me that such a method would be good or useful. Before adding more API to Common, we should probably figure out how many helper methods we want to provide in the SDK, and what their intended use case is.

korshaknn commented 3 years ago

@kkaefer it's a common use-case to check if there is a new version of nav tiles (for a list of tile regions or a single one).

and as you said

I also agree that this isn’t logic that should be implemented on the SDK level since it’s identical across platforms.

yes, it's identical, so it would be better to add it to a single place: common/NN.