Skip to content

Conversation

@zeac
Copy link
Contributor

@zeac zeac commented Dec 19, 2022

Description

This PR fixes the possible crash. The reproduce steps are unknown, I guess there is a race between calls to clear and focus callback calls.

@zeac zeac added bug Defect to be fixed. needs backporting Requires cherry-picking to a currently running release branch v2.x Related to version 2.x. labels Dec 19, 2022
@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #6760 (3c2fa0c) into main (0077240) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #6760   +/-   ##
=========================================
  Coverage     72.51%   72.51%           
- Complexity     5540     5541    +1     
=========================================
  Files           777      777           
  Lines         30038    30039    +1     
  Branches       3546     3547    +1     
=========================================
+ Hits          21782    21783    +1     
  Misses         6832     6832           
  Partials       1424     1424           
Impacted Files Coverage Δ
...tion/ui/voice/api/MapboxVoiceInstructionsPlayer.kt 79.12% <100.00%> (+0.23%) ⬆️

val currentAnnouncement = currentPlayCallback.announcement
val currentClientCallback = currentPlayCallback.consumer
currentClientCallback.accept(currentAnnouncement)
if (currentPlayCallback != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the following test to MapboxVoiceInstructionsPlayerTest:

    @Test
    fun `play is done after clear`() {
        val anyLanguage = Locale.US.language
        val mockedAnnouncement: SpeechAnnouncement = mockk(relaxed = true)
        val mapboxVoiceInstructionsPlayer =
            MapboxVoiceInstructionsPlayer(
                aMockedContext,
                anyLanguage,
                mockedPlayerOptions
            )
        val mockedPlay: SpeechAnnouncement = mockedAnnouncement
        val voiceInstructionsPlayerConsumer =
            mockk<MapboxNavigationConsumer<SpeechAnnouncement>>(relaxed = true)

        val requestSlotCallback = slot<AudioFocusRequestCallback>()
        every {
            mockedAudioFocusDelegate.requestFocus(any(), capture(requestSlotCallback))
        } just Runs
        mapboxVoiceInstructionsPlayer.play(mockedPlay, voiceInstructionsPlayerConsumer)
        mapboxVoiceInstructionsPlayer.clear()

        requestSlotCallback.captured.invoke(true)

        verify(exactly = 0) { voiceInstructionsPlayerConsumer.accept(any()) }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'd wait for Tomasz though, he's the one who's been working with voice instructions lately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, we's out until after the holidays, let's merge.

val currentAnnouncement = currentPlayCallback.announcement
val currentClientCallback = currentPlayCallback.consumer
currentClientCallback.accept(currentAnnouncement)
if (currentPlayCallback != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a changelog entry. Sth like:

- Fixed a rare `java.lang.NullPointerException: Attempt to read from field 'en.b dn.m.a' on a null object reference` crash in `PlayCallback.getAnnouncement`. [#6760](https://github.com/mapbox/mapbox-navigation-android/pull/6760)

to mapbox-navigation-android/CHANGELOG.md.

@Guardiola31337
Copy link
Contributor

@zeac noting that Ktlint failed 👀

> Task :libnavui-voice:ktlint
/root/code/libnavui-voice/src/test/java/com/******/navigation/ui/voice/api/MapboxVoiceInstructionsPlayerTest.kt:662:1: Needless blank line(s) (no-consecutive-blank-lines)

> Task :libnavui-voice:ktlint FAILED

@Guardiola31337
Copy link
Contributor

@zeac it seems we're good merging here 🚀

cc @dzinad

@Guardiola31337
Copy link
Contributor

@zeac

it seems we're good merging here 🚀

I'd wait for Tomasz though, he's the one who's been working with voice instructions lately.

Oh well, maybe not #6760 (comment) wdyt @dzinad?

cc @tomaszrybakiewicz

@zeac zeac force-pushed the NAVAND-1025-fix branch 2 times, most recently from 732dff4 to fe32c8b Compare December 21, 2022 14:35
@zeac zeac enabled auto-merge (squash) December 21, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Defect to be fixed. needs backporting Requires cherry-picking to a currently running release branch v2.x Related to version 2.x.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants