Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show playing tab when current chapter is tapped #59

Merged
merged 6 commits into from
Jul 7, 2022

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Jul 6, 2022

Fixes #41

Description

Clicking a current chapter would start the chapter from the beginning. This PR addresses the issue by navigating to the "Now Playing" tab if the chapter clicked is the current one.

show-player-tab-on-current-chapter-click.mp4

P.S. I updated ListData UI state with the showPlayer value based on this principle:

UI actions that originate from the ViewModel—ViewModel events—should always result in a UI state update.

I couldn't find a good example in the existing code for handling ViewModel events. So feel free to point me in the right direction. 😅

Checklist

  • Should this change be included in the release notes? If yes, please add a line in RELEASE-NOTES.md
  • Have you tested in landscape?
  • Have you tested accessibility with TalkBack?
  • Have you tested in different themes?
  • Does the change work with a large display font?
  • Are all the strings localized?
  • Could you have written any new tests?
  • Did you include Compose previews with any components?

@ashiagr ashiagr requested a review from geekygecko as a code owner July 6, 2022 13:32
@ashiagr ashiagr self-assigned this Jul 6, 2022
@ashiagr ashiagr requested a review from mchowning as a code owner July 6, 2022 13:32
}

fun onPlayerShown() {
showPlayerObservable.accept(false)
Copy link
Member

Choose a reason for hiding this comment

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

I haven't used it before but is this where you could use one of the new SharedFlow classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, SharedFlow can be a good candidate here. Thanks for suggesting it. Used it in e058f41.

@@ -186,6 +186,12 @@ open class PlaybackManager @Inject constructor(
return upNextQueue.currentEpisode
}

private suspend fun getCurrentChapter() = getCurrentEpisode()?.let { episode ->
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's possible to get the current chapter with the data already in the PlayerViewModel.

    fun onChapterClick(chapter: Chapter) {
        launch {
            val listData = listDataLive.value
            if (listData?.isSameChapter(chapter) == true) {
                showPlayerObservable.accept(true)
            } else {
                playbackManager.skipToChapter(chapter)
            }
        }
    }
    data class ListData(
        var podcastHeader: PlayerHeader,
        var chaptersExpanded: Boolean,
        var chapters: List<Chapter>,
        var currentChapter: Chapter?,
        var upNextExpanded: Boolean,
        var upNextEpisodes: List<Playable>,
        var upNextSummary: UpNextSummary,
        var showPlayer: Boolean,
    ) {
        fun isSameChapter(chapter: Chapter) = currentChapter?
           .let { it.index == chapter.index } ?: false
    }
        val currentChapter = playbackState.chapters.getChapter(playbackState.positionMs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks better, see in fb81785.

@ashiagr ashiagr requested a review from geekygecko July 7, 2022 04:54
@ashiagr ashiagr force-pushed the issue/41-show-player-tab-on-current-chapter-click branch from d418b37 to 3b358e1 Compare July 7, 2022 05:23
@geekygecko geekygecko merged commit 9cb3cef into main Jul 7, 2022
@geekygecko geekygecko deleted the issue/41-show-player-tab-on-current-chapter-click branch July 7, 2022 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tapping the currently playing chapter on the Chapters player tab, should take you to the playing tab
2 participants