Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
## Fixed
- Moving back and forth between channel pages no longer builds up saved state that can crash Flow when you leave the app (#1338)
- Opening a channel, artist or album that is already open further back returns to it instead of opening it again
2 changes: 1 addition & 1 deletion app/src/main/java/io/github/aedev/flow/ui/AppHooks.kt
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ fun HandlePendingRoute(
val tab = flowTabForRoute(route)
when {
tab != null -> navController.navigateToTab(tab, startRoute)
navController.currentBackStackEntry?.destination?.route != route -> navController.navigate(route)
else -> navController.openPage(route)
}
onConsumed()
}
Expand Down
15 changes: 1 addition & 14 deletions app/src/main/java/io/github/aedev/flow/ui/ChannelNavigation.kt
Original file line number Diff line number Diff line change
@@ -1,20 +1,7 @@
package io.github.aedev.flow.ui

import androidx.navigation.NavHostController
import java.net.URLDecoder

internal fun NavHostController.navigateToYoutubeChannel(channelIdOrHandle: String) {
val targetUrl = youtubeChannelUrl(channelIdOrHandle) ?: return
val currentUrl = currentBackStackEntry
?.takeIf { it.destination.route == "channel?url={channelUrl}" }
?.arguments
?.getString("channelUrl")
?.let { encodedUrl ->
runCatching { URLDecoder.decode(encodedUrl, Charsets.UTF_8.name()) }
.getOrDefault(encodedUrl)
}
?.let(::youtubeChannelUrl)

if (currentUrl == targetUrl) return
youtubeChannelRoute(targetUrl)?.let(::navigate)
youtubeChannelRoute(channelIdOrHandle)?.let(::openPage)
}
55 changes: 33 additions & 22 deletions app/src/main/java/io/github/aedev/flow/ui/MediaNavigation.kt
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
package io.github.aedev.flow.ui

import android.net.Uri
import androidx.navigation.NavController
import androidx.navigation.NavHostController
import io.github.aedev.flow.data.shorts.queue.ShortsQueueSource
import io.github.aedev.flow.ui.components.layout.navigation.MediaNavigator
import io.github.aedev.flow.utils.YouTubeLink

internal const val MUSIC_ARTIST_ROUTE_PATTERN = "artist/{channelId}"
internal const val MUSIC_ARTIST_ROUTE_ARG = "channelId"
internal const val MUSIC_PLAYLIST_ROUTE_PATTERN = "musicPlaylist/{playlistId}"
internal const val MUSIC_PLAYLIST_ROUTE_ARG = "playlistId"
internal const val MUSIC_PLAYER_ROUTE_PATTERN = "musicPlayer/{videoId}?list={list}"
internal const val MUSIC_PLAYER_ROUTE_ARG = "videoId"
internal const val MUSIC_PLAYER_LIST_ARG = "list"
Expand All @@ -30,13 +29,33 @@ internal fun musicPlayerRoute(
internal fun musicCollectionRoute(collectionId: String): String? =
collectionId.trim().takeIf(String::isNotEmpty)?.let { "musicPlaylist/${Uri.encode(it)}" }

/** True when the page on top is already [pattern] for [id], so opening it again would stack a duplicate. */
internal fun isOpenMediaPage(
currentPattern: String?,
currentId: String?,
pattern: String,
id: String,
): Boolean = currentPattern == pattern && currentId == id.trim()
internal enum class PageOpen { PUSH, STAY, RETURN }

/** A page already on the back stack is returned to, never stacked again; the one on top stays. */
internal fun <T> pageOpen(
existing: T?,
current: T?,
): PageOpen =
when {
existing == null -> PageOpen.PUSH
existing == current -> PageOpen.STAY
else -> PageOpen.RETURN
}

/**
* Opens [route], or returns to it when it is already on the back stack. Each stacked page keeps its
* state in the activity's saved bundle, and a long loop between two channels grew that bundle past
* what Android can hand over when the app stops (#1338).
*/
internal fun NavController.openPage(route: String) {
// The public lookup for the exact, argument-filled match that popBackStack(route) pops to.
val existing = runCatching { getBackStackEntry(route) }.getOrNull()
when (pageOpen(existing, currentBackStackEntry)) {
PageOpen.PUSH -> navigate(route)
PageOpen.STAY -> Unit
PageOpen.RETURN -> popBackStack(route, inclusive = false)
}
}

/**
* The shell's [MediaNavigator]. [beforeNavigate] moves an expanded player out of the way, so a page
Expand All @@ -54,8 +73,7 @@ internal class FlowMediaNavigator(
navController.navigateToYoutubeChannel(channelId)
}

override fun openArtist(artistId: String) =
open(MUSIC_ARTIST_ROUTE_PATTERN, MUSIC_ARTIST_ROUTE_ARG, artistId, musicArtistRoute(artistId))
override fun openArtist(artistId: String) = open(musicArtistRoute(artistId))

override fun openAlbum(albumId: String) = openCollection(albumId)

Expand Down Expand Up @@ -89,7 +107,7 @@ internal class FlowMediaNavigator(

destination is LinkDestination.Page -> {
beforeNavigate()
navController.navigate(destination.route)
navController.openPage(destination.route)
}
}
return true
Expand All @@ -113,18 +131,11 @@ internal class FlowMediaNavigator(

private fun isShortsOnTop() = navController.currentBackStackEntry?.destination?.route == SHORTS_ROUTE_PATTERN

private fun openCollection(id: String) = open(MUSIC_PLAYLIST_ROUTE_PATTERN, MUSIC_PLAYLIST_ROUTE_ARG, id, musicCollectionRoute(id))
private fun openCollection(id: String) = open(musicCollectionRoute(id))

private fun open(
pattern: String,
argName: String,
id: String,
route: String?,
) {
private fun open(route: String?) {
if (route == null) return
beforeNavigate()
val current = navController.currentBackStackEntry
if (isOpenMediaPage(current?.destination?.route, current?.arguments?.getString(argName), pattern, id)) return
navController.navigate(route)
navController.openPage(route)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ private const val UNKNOWN_STEPS = "Not known. Flow crashed during normal use; th
private const val EXPECTED = "Flow keeps running."
private const val TOP_FRAMES = 6
private val ReportTime: DateTimeFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSS")
private val WrappedCause = Regex("""([A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)+(?:Exception|Error))(?::.*)?""", RegexOption.DOT_MATCHES_ALL)

/** The facts a person needs from a crash report, read back from the text the crash handler saved. */
data class CrashSummary(
Expand All @@ -23,7 +24,20 @@ data class CrashSummary(
val device: String?,
val time: LocalDateTime?,
val topFrames: List<String>,
)
) {
/**
* The class a title names. Android rethrows some failures as `RuntimeException(cause)`, whose
* message is the cause, and the cause is what tells two such crashes apart.
*/
val headline: String
get() =
message
?.let(WrappedCause::matchEntire)
?.groupValues
?.get(1)
?.substringAfterLast('.')
?: exception
}

fun parseCrashSummary(report: String): CrashSummary {
val lines = report.lines().map { it.trim() }
Expand Down Expand Up @@ -61,7 +75,7 @@ fun parseCrashSummary(report: String): CrashSummary {
* happened can still send it in one tap and edit only what they know (#1189).
*/
fun CrashSummary.issueUrl(): String {
val title = "[Bug]: $exception" + (location?.let { " in $it" } ?: "")
val title = "[Bug]: $headline" + (location?.let { " in $it" } ?: "")
val error = exception + (message?.let { ": $it" } ?: "")
val log =
buildString {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ package io.github.aedev.flow.ui
import android.app.Application
import androidx.test.ext.junit.runners.AndroidJUnit4
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.annotation.Config
Expand All @@ -20,12 +18,4 @@ class MediaNavigationTest {
assertNull(musicArtistRoute(" "))
assertNull(musicCollectionRoute(""))
}

@Test
fun theSamePageIsNotOpenedTwice() {
assertTrue(isOpenMediaPage(MUSIC_ARTIST_ROUTE_PATTERN, "UCabc", MUSIC_ARTIST_ROUTE_PATTERN, " UCabc"))
assertFalse(isOpenMediaPage(MUSIC_ARTIST_ROUTE_PATTERN, "UCabc", MUSIC_ARTIST_ROUTE_PATTERN, "UCdef"))
assertFalse(isOpenMediaPage(MUSIC_PLAYLIST_ROUTE_PATTERN, "UCabc", MUSIC_ARTIST_ROUTE_PATTERN, "UCabc"))
assertFalse(isOpenMediaPage(null, null, MUSIC_PLAYLIST_ROUTE_PATTERN, "VLabc"))
}
}
21 changes: 21 additions & 0 deletions app/src/test/java/io/github/aedev/flow/ui/PageOpenTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package io.github.aedev.flow.ui

import com.google.common.truth.Truth.assertThat
import org.junit.Test

class PageOpenTest {
@Test
fun `a page not on the back stack is pushed`() {
assertThat(pageOpen(existing = null, current = "home")).isEqualTo(PageOpen.PUSH)
}

@Test
fun `the page on top stays where it is`() {
assertThat(pageOpen(existing = "channel A", current = "channel A")).isEqualTo(PageOpen.STAY)
}

@Test
fun `a page deeper in the stack is returned to instead of stacked again (1338)`() {
assertThat(pageOpen(existing = "channel A", current = "channel B")).isEqualTo(PageOpen.RETURN)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,38 @@ class CrashSummaryTest {
assertThat(summary.exception).isEqualTo("Crash")
assertThat(summary.location).isNull()
}

@Test
fun `a wrapped cause names the title, as with a saved state too large to send (1338)`() {
val wrapped =
FlowCrashReportFormatter.build(
FlowCrashReportSnapshot(
timestampMs = 0,
threadName = "main",
threadId = 2,
exceptionClass = "java.lang.RuntimeException",
exceptionMessage = "android.os.TransactionTooLargeException: data parcel size 1052744 bytes",
stackTrace =
"""
java.lang.RuntimeException: android.os.TransactionTooLargeException: data parcel size 1052744 bytes
at android.app.servertransaction.PendingTransactionActions${'$'}StopInfo.run(PendingTransactionActions.java:146)
""".trimIndent(),
deviceInfo = " Model: Pixel 8\n Manufacturer: Google",
memoryInfo = "",
breadcrumbs = emptyList(),
),
)
val summary = parseCrashSummary(wrapped)

assertThat(summary.exception).isEqualTo("RuntimeException")
assertThat(summary.headline).isEqualTo("TransactionTooLargeException")
assertThat(
summary.issueUrl(),
).contains("title=%5BBug%5D%3A%20TransactionTooLargeException%20in%20PendingTransactionActions.java%3A146")
}

@Test
fun `a plain message keeps the exception's own name`() {
assertThat(parseCrashSummary(report).headline).isEqualTo("IllegalArgumentException")
}
}
Loading