feat(home): veil only when content changed; tell the user when it didn't — #2327
android / Build + lint + test (push) Successful in 4m8s
android / Build + lint + test (push) Successful in 4m8s
The veil raised eagerly: any trigger over a warm cache put it up before knowing whether the refresh would change anything. So every launch cost ~1-2s of opaque panel even when the pull returned exactly what was already cached — which, now that the section swap is atomic and the index flow dedups on ids, produces no visible churn to hide at all. The veil was covering nothing and only delaying first paint. The raise is now reactive: it fires when the content key actually differs from what was already on screen, and never for a no-op refresh. The baseline is the first state that HAS content, not the first state at all — over a warm cache the cached rows paint a moment after the session starts, and counting that first paint as "a change" would veil every launch, which is the thing being fixed. Cost of reacting rather than anticipating: the veil arrives one emission after the change, so a single atomic swap shows through. Everything messier that follows it — tile hydration, then artwork — still lands behind it. That leaves a hole this closes too: a manual pull where nothing changed would now produce no veil, no movement, nothing whatsoever, which reads as broken. So sessions report an outcome — CHANGED / UNCHANGED / FAILED — and Home surfaces it as "Already up to date" or "Couldn't check for updates". Only for refreshes a person actually asked for. "Already up to date" on every launch, every 03:00 rebuild and every reconnect would be worse than silence, so VeilSessionResult carries a userInitiated bit and background sessions stay quiet. The bit is tracked separately from the request token because the request channel is CONFLATED: coalescing drops the older token, and a user's pull must not be swallowed by a background trigger arriving on its heels. The surfaced failure is a deliberate narrowing of the earlier "silent on give up" call, which is now read as being about background refreshes: for a pull the user deliberately triggered, silence looks broken, and staying silent while the success case speaks would be incoherent. Recovery is unaffected either way. Pull-to-refresh now waits for whichever successor actually arrives — the veil, or the snackbar — via finishedSessions, instead of only ever waiting on the veil and timing out for 2s on an unchanged pull. The Error-state Retry goes through the controller as well, so it gets the retries and reports its outcome; over an empty cache there's no content to protect, so no veil appears. HomeViewModel.refresh() is gone, replaced by retry() and refreshFromPull() — the two things that actually exist. Tests: two changed meaning and are rewritten rather than patched. A failed pull writes nothing, so the veil no longer stands over the retries — it goes up when a retry finally lands. And "waits for content to paint" became "cached content painting is not mistaken for a change", which is the baseline subtlety above. Added coverage for UNCHANGED, FAILED, the cold-load CHANGED case, and the conflation of a user request with a background one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -87,6 +87,8 @@ import com.fabledsword.minstrel.playlists.widgets.PlaylistCard
|
||||
import com.fabledsword.minstrel.playlists.widgets.PlaylistPlaceholderCard
|
||||
import com.fabledsword.minstrel.shared.UiState
|
||||
import com.fabledsword.minstrel.shared.UpdateVeilController
|
||||
import com.fabledsword.minstrel.shared.VeilOutcome
|
||||
import com.fabledsword.minstrel.shared.VeilSessionResult
|
||||
import com.fabledsword.minstrel.shared.VeilSettleState
|
||||
import com.fabledsword.minstrel.shared.asCacheFirstStateFlow
|
||||
import com.fabledsword.minstrel.shared.widgets.ArtSettleTracker
|
||||
@@ -100,7 +102,6 @@ import com.fabledsword.minstrel.shared.widgets.SkeletonAlbumTile
|
||||
import com.fabledsword.minstrel.shared.widgets.SkeletonArtistTile
|
||||
import com.fabledsword.minstrel.shared.widgets.SkeletonSectionHeader
|
||||
import dagger.hilt.android.lifecycle.HiltViewModel
|
||||
import kotlinx.coroutines.Job
|
||||
import kotlinx.coroutines.async
|
||||
import kotlinx.coroutines.channels.Channel
|
||||
import kotlinx.coroutines.coroutineScope
|
||||
@@ -128,8 +129,9 @@ private const val BOTTOM_PADDING_FOR_MINIPLAYER_DP = 140
|
||||
private const val RECENTLY_ADDED_GRID_ROWS = 2
|
||||
private const val RECENTLY_ADDED_GRID_HEIGHT_DP = 440
|
||||
|
||||
// "Updating your mixes…" veil. Its lifetime is decided by
|
||||
// UpdateVeilController watching the screen settle — not by a fixed delay,
|
||||
// "Updating your mixes…" veil. UpdateVeilController decides both whether it
|
||||
// appears at all — only when a refresh actually changes something — and how
|
||||
// long it stays, by watching the screen settle rather than by a fixed delay,
|
||||
// which lowered it while tiles and artwork were still landing (#2327).
|
||||
// Near-opaque (VEIL_ALPHA) so the section churn never bleeds through.
|
||||
private const val VEIL_WIPE_MS = 280
|
||||
@@ -138,8 +140,9 @@ private const val VEIL_SPINNER_DP = 22
|
||||
private const val VEIL_SPINNER_STROKE_DP = 2
|
||||
private const val VEIL_LABEL_GAP_DP = 12
|
||||
|
||||
// How long a manual pull keeps its own indicator while waiting for the
|
||||
// veil to take over, so the two don't both vanish for a frame mid-handoff.
|
||||
// Backstop on how long a manual pull keeps its own indicator while waiting
|
||||
// for its successor — the veil, or the "already up to date" snackbar — so the
|
||||
// two never both vanish for a frame mid-handoff.
|
||||
private const val PULL_HANDOFF_TIMEOUT_MS = 2_000L
|
||||
|
||||
// ─── State ───────────────────────────────────────────────────────────
|
||||
@@ -196,10 +199,13 @@ class HomeViewModel @Inject constructor(
|
||||
initialValue = false,
|
||||
)
|
||||
|
||||
private val poolMessages = Channel<String>(Channel.BUFFERED)
|
||||
private val snackbarMessages = Channel<String>(Channel.BUFFERED)
|
||||
|
||||
/** Transient snackbar messages from offline-pool taps. */
|
||||
val transientMessages: Flow<String> = poolMessages.receiveAsFlow()
|
||||
/**
|
||||
* Transient snackbar messages: offline-pool taps, playback failures, and
|
||||
* the outcome of a refresh the user explicitly asked for.
|
||||
*/
|
||||
val transientMessages: Flow<String> = snackbarMessages.receiveAsFlow()
|
||||
|
||||
/**
|
||||
* Copy for the most recent /home/index refresh failure; null once a
|
||||
@@ -228,7 +234,7 @@ class HomeViewModel @Inject constructor(
|
||||
OfflinePoolKind.LIKED -> shuffleSource.liked()
|
||||
}.shuffled()
|
||||
if (tracks.isEmpty()) {
|
||||
poolMessages.trySend("No cached ${kind.label} tracks yet")
|
||||
snackbarMessages.trySend("No cached ${kind.label} tracks yet")
|
||||
} else {
|
||||
player.setQueue(tracks, initialIndex = 0, source = "offline:${kind.name}")
|
||||
}
|
||||
@@ -265,14 +271,14 @@ class HomeViewModel @Inject constructor(
|
||||
try {
|
||||
val detail = libraryRepository.refreshAlbumDetail(albumId)
|
||||
if (detail.tracks.isEmpty()) {
|
||||
poolMessages.trySend("This album has no tracks to play.")
|
||||
snackbarMessages.trySend("This album has no tracks to play.")
|
||||
} else {
|
||||
player.setQueue(detail.tracks, initialIndex = 0, source = "album:$albumId")
|
||||
}
|
||||
} catch (
|
||||
@Suppress("TooGenericExceptionCaught") e: Throwable,
|
||||
) {
|
||||
poolMessages.trySend(
|
||||
snackbarMessages.trySend(
|
||||
"Couldn't start playback: ${ErrorCopy.fromThrowable(e)}",
|
||||
)
|
||||
}
|
||||
@@ -290,14 +296,14 @@ class HomeViewModel @Inject constructor(
|
||||
try {
|
||||
val tracks = libraryRepository.fetchArtistTracks(artistId).shuffled()
|
||||
if (tracks.isEmpty()) {
|
||||
poolMessages.trySend("This artist has no tracks to play.")
|
||||
snackbarMessages.trySend("This artist has no tracks to play.")
|
||||
} else {
|
||||
player.setQueue(tracks, initialIndex = 0, source = "artist:$artistId")
|
||||
}
|
||||
} catch (
|
||||
@Suppress("TooGenericExceptionCaught") e: Throwable,
|
||||
) {
|
||||
poolMessages.trySend(
|
||||
snackbarMessages.trySend(
|
||||
"Couldn't start playback: ${ErrorCopy.fromThrowable(e)}",
|
||||
)
|
||||
}
|
||||
@@ -315,7 +321,7 @@ class HomeViewModel @Inject constructor(
|
||||
suspend fun playPlaylist(playlist: PlaylistRef) {
|
||||
viewModelScope.launch {
|
||||
playPlaylistShuffled(playlist, playlistsRepository, player) {
|
||||
poolMessages.trySend(it)
|
||||
snackbarMessages.trySend(it)
|
||||
}
|
||||
}.join()
|
||||
}
|
||||
@@ -323,9 +329,8 @@ class HomeViewModel @Inject constructor(
|
||||
/**
|
||||
* Pulls /home/index, the playlists list and the system-playlist
|
||||
* status. Returns true when the load-bearing /home/index pull
|
||||
* succeeded — the veil controller retries on false, so this must
|
||||
* report failure rather than swallow it the way the fire-and-forget
|
||||
* [refresh] entry point does.
|
||||
* succeeded — the veil controller retries on false and reports the
|
||||
* outcome, so this must report failure rather than swallow it.
|
||||
*/
|
||||
private suspend fun runRefresh(): Boolean = coroutineScope {
|
||||
// /home/index is the load-bearing pull: its failure drives the
|
||||
@@ -352,19 +357,31 @@ class HomeViewModel @Inject constructor(
|
||||
home.await()
|
||||
}
|
||||
|
||||
/** Unveiled refresh, for the Error state's explicit Retry button. */
|
||||
fun refresh(): Job = viewModelScope.launch { runRefresh() }
|
||||
/**
|
||||
* The Error state's explicit Retry button. User-initiated, so it gets
|
||||
* the controller's retries and reports its outcome; over an empty cache
|
||||
* there's no content to protect, so no veil goes up.
|
||||
*/
|
||||
fun retry() = veil.request(userInitiated = true)
|
||||
|
||||
/**
|
||||
* Manual pull-to-refresh. Goes behind the veil like every other
|
||||
* refresh (operator call, 2026-07-31: the churn a pull causes is
|
||||
* identical to the automatic paths, and a small spinner didn't hide
|
||||
* it). Suspends only long enough to hand the indicator off to the
|
||||
* veil; the refresh itself continues in the controller's session.
|
||||
* Manual pull-to-refresh. Goes behind the veil like every other refresh
|
||||
* (operator call, 2026-07-31: the churn a pull causes is identical to
|
||||
* the automatic paths, and a small spinner didn't hide it).
|
||||
*
|
||||
* Suspends until the veil has taken over OR the session has finished,
|
||||
* so the pull indicator hands off to exactly one successor: the veil if
|
||||
* content changed, the "Already up to date" snackbar if it didn't. The
|
||||
* timeout is only a backstop against a session that outlives it.
|
||||
*/
|
||||
suspend fun refreshFromPull() {
|
||||
veil.request()
|
||||
withTimeoutOrNull(PULL_HANDOFF_TIMEOUT_MS) { veil.visible.first { it } }
|
||||
val before = veil.finishedSessions.value
|
||||
veil.request(userInitiated = true)
|
||||
withTimeoutOrNull(PULL_HANDOFF_TIMEOUT_MS) {
|
||||
combine(veil.visible, veil.finishedSessions) { veiled, finished ->
|
||||
veiled || finished != before
|
||||
}.first { it }
|
||||
}
|
||||
}
|
||||
|
||||
val uiState: StateFlow<UiState<HomeSections>> =
|
||||
@@ -458,14 +475,38 @@ class HomeViewModel @Inject constructor(
|
||||
// right before the cache emits.
|
||||
uiState.value is UiState.Success || homeRepository.hasCachedIndex()
|
||||
},
|
||||
onSessionEnd = ::reportRefreshOutcome,
|
||||
work = ::runRefresh,
|
||||
)
|
||||
|
||||
/**
|
||||
* True while the "Updating your mixes…" veil should be raised. The
|
||||
* controller holds it until Home actually settles — sections swapped,
|
||||
* tiles hydrated, artwork loaded — instead of for a fixed delay after
|
||||
* the network pull returns (issue #2327).
|
||||
* Tells the user how a refresh *they asked for* went, in the one case
|
||||
* the veil can't: when nothing changed there's no veil to see, and a
|
||||
* pull that produces no visible response at all reads as broken.
|
||||
*
|
||||
* Only user-initiated sessions say anything. The same outcome from a
|
||||
* background check — the initial load, the 03:00 rebuild, a reconnect —
|
||||
* is noise, and "Already up to date" on every launch would be worse
|
||||
* than silence (operator's call, 2026-07-31).
|
||||
*/
|
||||
private fun reportRefreshOutcome(result: VeilSessionResult) {
|
||||
if (!result.userInitiated) return
|
||||
when (result.outcome) {
|
||||
// The veil was the feedback.
|
||||
VeilOutcome.CHANGED -> return
|
||||
VeilOutcome.UNCHANGED -> snackbarMessages.trySend("Already up to date")
|
||||
VeilOutcome.FAILED -> snackbarMessages.trySend("Couldn't check for updates")
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* True while the "Updating your mixes…" veil should be raised.
|
||||
*
|
||||
* Raised only when a refresh actually changes what's on screen, and then
|
||||
* held until Home settles — tiles hydrated, artwork loaded — instead of
|
||||
* for a fixed delay after the network pull returns (issue #2327). A
|
||||
* refresh that returns what's already cached shows no veil at all;
|
||||
* [reportRefreshOutcome] tells the user instead, if they asked.
|
||||
*/
|
||||
val isUpdating: StateFlow<Boolean> = veil.visible
|
||||
|
||||
@@ -589,7 +630,7 @@ private fun HomeStateCrossfade(
|
||||
is UiState.Error -> ErrorRetry(
|
||||
title = "Couldn't load home",
|
||||
message = s.message,
|
||||
onRetry = { viewModel.refresh() },
|
||||
onRetry = { viewModel.retry() },
|
||||
)
|
||||
is UiState.Success -> HomeSuccessContent(
|
||||
sections = s.data,
|
||||
|
||||
+136
-34
@@ -12,8 +12,10 @@ import kotlinx.coroutines.flow.asStateFlow
|
||||
import kotlinx.coroutines.flow.debounce
|
||||
import kotlinx.coroutines.flow.distinctUntilChanged
|
||||
import kotlinx.coroutines.flow.first
|
||||
import kotlinx.coroutines.flow.update
|
||||
import kotlinx.coroutines.launch
|
||||
import kotlinx.coroutines.withTimeoutOrNull
|
||||
import java.util.concurrent.atomic.AtomicBoolean
|
||||
|
||||
// Once raised, the veil stays up at least this long. Without a floor a
|
||||
// no-op refresh wipes on and straight back off, which reads as a glitch.
|
||||
@@ -61,9 +63,33 @@ data class VeilSettleState(
|
||||
val quiescent: Boolean,
|
||||
)
|
||||
|
||||
/** What a finished session did, so callers can report it if they want. */
|
||||
enum class VeilOutcome {
|
||||
/** Content changed, and the veil covered the churn. */
|
||||
CHANGED,
|
||||
|
||||
/** The refresh worked, but nothing on screen moved — already current. */
|
||||
UNCHANGED,
|
||||
|
||||
/** Every attempt failed. */
|
||||
FAILED,
|
||||
}
|
||||
|
||||
/**
|
||||
* Drives an "updating" overlay from *observed content settling* rather
|
||||
* than from a fixed delay.
|
||||
* One session's result, plus whether a user explicitly asked for it.
|
||||
*
|
||||
* [userInitiated] is what lets a caller tell feedback from noise: a user
|
||||
* who pulled to refresh is owed an answer even when the answer is "nothing
|
||||
* changed", while the same outcome from a background check is noise.
|
||||
*/
|
||||
data class VeilSessionResult(
|
||||
val outcome: VeilOutcome,
|
||||
val userInitiated: Boolean,
|
||||
)
|
||||
|
||||
/**
|
||||
* Drives an "updating" overlay from *observed content change and settling*
|
||||
* rather than from a fixed delay.
|
||||
*
|
||||
* The problem this replaces: a veil held for `refresh().join() + 500ms`
|
||||
* lowers while the screen is still moving, because finishing the network
|
||||
@@ -73,31 +99,44 @@ data class VeilSettleState(
|
||||
* two overlapping refreshes finishes first, wiping the veil off mid-update
|
||||
* (issue #2327).
|
||||
*
|
||||
* So instead: raise, run [work], then hold until [settleSignal] reports
|
||||
* the screen has stopped changing for [VeilTimings.quietMs] AND is
|
||||
* quiescent — bounded below by [VeilTimings.minHoldMs] so it can never
|
||||
* flash, and above by [VeilTimings.maxHoldMs] so it can never strand.
|
||||
* So instead: run [work], raise only if the content actually changes, then
|
||||
* hold until [settleSignal] reports the screen has stopped changing for
|
||||
* [VeilTimings.quietMs] AND is quiescent — bounded below by
|
||||
* [VeilTimings.minHoldMs] so it can never flash, and above by
|
||||
* [VeilTimings.maxHoldMs] so it can never strand.
|
||||
*
|
||||
* The raise is deliberately *reactive*: a refresh that returns what's
|
||||
* already on screen — the common case on a launch over a warm cache —
|
||||
* raises nothing at all, because a veil over an unchanged screen hides
|
||||
* nothing and only delays first paint. The cost is that the veil arrives
|
||||
* one emission after the change, so a single atomic content swap shows
|
||||
* through; everything messier that follows it (tile hydration, then
|
||||
* artwork) still lands behind the veil.
|
||||
*
|
||||
* Overlapping triggers extend the running session instead of racing it,
|
||||
* so the veil stays up continuously rather than lowering and re-raising.
|
||||
*
|
||||
* Failure is quiet by design: [work] gets [VeilTimings.attempts] tries
|
||||
* behind the veil, and if they all fail the veil simply wipes off over
|
||||
* the cached content with no error surfaced. Giving up here ends only
|
||||
* *this* session — it sets no latch and blocks nothing, so the caller's
|
||||
* own recovery paths (reconnect re-pull, freshness sweeps, the next
|
||||
* event, a manual pull) keep retrying afterwards exactly as before.
|
||||
* Failure is quiet at this layer: [work] gets [VeilTimings.attempts] tries
|
||||
* behind the veil, and if they all fail the veil simply wipes off over the
|
||||
* cached content. Giving up ends only *this* session — it sets no latch and
|
||||
* blocks nothing, so the caller's own recovery paths (reconnect re-pull,
|
||||
* freshness sweeps, the next event, a manual pull) keep retrying afterwards
|
||||
* exactly as before. Callers that want to surface a failure can do it from
|
||||
* [onSessionEnd] instead.
|
||||
*
|
||||
* @param work one refresh attempt; returns true when it succeeded.
|
||||
* @param shouldVeil sampled at session start — "is there cached content
|
||||
* this refresh is about to overwrite?". False means a cold load, where
|
||||
* a skeleton is the right affordance, and the work runs unveiled.
|
||||
* @param onSessionEnd called once per finished session, on the controller's
|
||||
* coroutine. Use it for user-facing feedback the veil itself can't give.
|
||||
*/
|
||||
class UpdateVeilController(
|
||||
private val scope: CoroutineScope,
|
||||
private val settleSignal: Flow<VeilSettleState>,
|
||||
private val shouldVeil: suspend () -> Boolean,
|
||||
private val timings: VeilTimings = VeilTimings(),
|
||||
private val onSessionEnd: (VeilSessionResult) -> Unit = {},
|
||||
private val work: suspend () -> Boolean,
|
||||
) {
|
||||
private val visibleInternal = MutableStateFlow(false)
|
||||
@@ -105,10 +144,25 @@ class UpdateVeilController(
|
||||
/** True while the veil should be drawn over the screen. */
|
||||
val visible: StateFlow<Boolean> = visibleInternal.asStateFlow()
|
||||
|
||||
private val finishedInternal = MutableStateFlow(0)
|
||||
|
||||
/**
|
||||
* Increments as each session ends. Lets a caller wait for "this
|
||||
* refresh is done" without knowing whether a veil ever went up —
|
||||
* a pull-to-refresh indicator needs exactly that, since an unchanged
|
||||
* refresh never raises one.
|
||||
*/
|
||||
val finishedSessions: StateFlow<Int> = finishedInternal.asStateFlow()
|
||||
|
||||
// Conflated: a burst of triggers (reconnect + rebuild event arriving
|
||||
// together) collapses into one follow-up pass, not a queue of them.
|
||||
private val requests = Channel<Unit>(Channel.CONFLATED)
|
||||
|
||||
// Sticky across a conflated burst: conflation drops the older token, so
|
||||
// the "a user asked for this" bit can't ride on it. If ANY coalesced
|
||||
// trigger was the user's, the session still owes them an answer.
|
||||
private val userAsked = AtomicBoolean(false)
|
||||
|
||||
init {
|
||||
// One consumer, so sessions are serialised by construction: two
|
||||
// triggers can never each own a piece of the veil's state.
|
||||
@@ -124,28 +178,31 @@ class UpdateVeilController(
|
||||
* Ask for a refresh. Safe to call from any trigger at any rate —
|
||||
* calls arriving during a session extend it rather than starting a
|
||||
* competing one.
|
||||
*
|
||||
* @param userInitiated true when a person explicitly asked (pull to
|
||||
* refresh, a Retry button), which is what [VeilSessionResult] carries
|
||||
* through to [onSessionEnd].
|
||||
*/
|
||||
fun request() {
|
||||
fun request(userInitiated: Boolean = false) {
|
||||
if (userInitiated) userAsked.set(true)
|
||||
requests.trySend(Unit)
|
||||
}
|
||||
|
||||
private suspend fun runSession() {
|
||||
// Sampled at both ends of the work: a trigger folded in mid-session
|
||||
// (see [drainWork]) may have been the user's, and they're still owed
|
||||
// an answer for it.
|
||||
val askedAtStart = userAsked.getAndSet(false)
|
||||
if (!shouldVeil()) {
|
||||
drainWork()
|
||||
// Cold load: the skeleton is the right affordance, so no veil.
|
||||
// Succeeding here did change the screen — from nothing to
|
||||
// something — so it reports CHANGED, never "already up to date".
|
||||
val ok = drainWork()
|
||||
finish(succeeded = ok, changed = ok, userInitiated = askedAtStart)
|
||||
return
|
||||
}
|
||||
// Raise only once there's content on screen to hide. Over a warm
|
||||
// cache that's within a frame or two of here — long before the
|
||||
// network pull lands — so the churn still gets covered. But on a
|
||||
// genuinely cold load, content appears only *because* this work
|
||||
// produced it, and veiling that would delay first paint to hide
|
||||
// nothing.
|
||||
val raised = CompletableDeferred<Unit>()
|
||||
val raiser = scope.launch {
|
||||
settleSignal.first { it.hasContent }
|
||||
visibleInternal.value = true
|
||||
raised.complete(Unit)
|
||||
}
|
||||
val raiser = scope.launch { raiseWhenContentChanges(raised) }
|
||||
// Floor and ceiling are measured from the raise, not the request,
|
||||
// so a late raise still gets its full no-flash minimum.
|
||||
val floor = scope.launch {
|
||||
@@ -157,12 +214,16 @@ class UpdateVeilController(
|
||||
delay(timings.maxHoldMs)
|
||||
visibleInternal.value = false
|
||||
}
|
||||
var succeeded = false
|
||||
try {
|
||||
drainWork()
|
||||
succeeded = drainWork()
|
||||
// Always wait for the settle, never conditionally on `visible`:
|
||||
// work that finishes without suspending would otherwise reach
|
||||
// here before the raiser has been dispatched, tear the session
|
||||
// down, and leave the churn uncovered.
|
||||
// down, and leave the churn uncovered. This wait is also what
|
||||
// makes `raised.isCompleted` below a trustworthy "did anything
|
||||
// change?" — a change landing just after the pull returns still
|
||||
// gets seen.
|
||||
withTimeoutOrNull(timings.maxHoldMs) { awaitSettled() }
|
||||
// Honour the no-flash minimum before lowering. Deliberately in
|
||||
// the try and not the finally: on cancellation the scope is
|
||||
@@ -175,23 +236,64 @@ class UpdateVeilController(
|
||||
ceiling.cancel()
|
||||
floor.cancel()
|
||||
visibleInternal.value = false
|
||||
finish(
|
||||
succeeded = succeeded,
|
||||
changed = raised.isCompleted,
|
||||
userInitiated = askedAtStart,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
private suspend fun drainWork() {
|
||||
do {
|
||||
runWorkWithRetries()
|
||||
// A trigger that arrived mid-session gets folded into this one.
|
||||
} while (requests.tryReceive().isSuccess)
|
||||
/**
|
||||
* Raises the veil the moment the screen's content differs from what was
|
||||
* already on it — and never, if this refresh turns out to be a no-op.
|
||||
*
|
||||
* The baseline is the first state that HAS content, not simply the first
|
||||
* state: over a warm cache the cached rows paint a moment after the
|
||||
* session starts, and treating that first paint as "a change" would veil
|
||||
* every launch, which is the whole thing this avoids.
|
||||
*/
|
||||
private suspend fun raiseWhenContentChanges(raised: CompletableDeferred<Unit>) {
|
||||
val baseline = settleSignal.first { it.hasContent }
|
||||
settleSignal.first { it.hasContent && it.contentKey != baseline.contentKey }
|
||||
visibleInternal.value = true
|
||||
raised.complete(Unit)
|
||||
}
|
||||
|
||||
private suspend fun runWorkWithRetries() {
|
||||
private fun finish(succeeded: Boolean, changed: Boolean, userInitiated: Boolean) {
|
||||
val outcome = when {
|
||||
!succeeded -> VeilOutcome.FAILED
|
||||
changed -> VeilOutcome.CHANGED
|
||||
else -> VeilOutcome.UNCHANGED
|
||||
}
|
||||
onSessionEnd(
|
||||
VeilSessionResult(
|
||||
outcome = outcome,
|
||||
// Fold in a mid-session request from the user.
|
||||
userInitiated = userInitiated || userAsked.getAndSet(false),
|
||||
),
|
||||
)
|
||||
finishedInternal.update { it + 1 }
|
||||
}
|
||||
|
||||
/** True when the refresh eventually succeeded. */
|
||||
private suspend fun drainWork(): Boolean {
|
||||
var succeeded: Boolean
|
||||
do {
|
||||
succeeded = runWorkWithRetries()
|
||||
// A trigger that arrived mid-session gets folded into this one.
|
||||
} while (requests.tryReceive().isSuccess)
|
||||
return succeeded
|
||||
}
|
||||
|
||||
private suspend fun runWorkWithRetries(): Boolean {
|
||||
repeat(timings.attempts) { attempt ->
|
||||
if (work()) return
|
||||
if (work()) return true
|
||||
if (attempt < timings.attempts - 1) {
|
||||
delay(timings.retryBackoffMs * (attempt + 1))
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
+99
-41
@@ -15,7 +15,6 @@ import kotlin.test.assertFalse
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
private const val WORK_MS = 1_000L
|
||||
private const val SLOW_WORK_MS = 5_000L
|
||||
private const val CHURN_ROUNDS = 5
|
||||
private const val ART_IN_FLIGHT = 3
|
||||
private const val SUCCEED_ON_ATTEMPT = 3
|
||||
@@ -29,9 +28,9 @@ private const val QUIET_WINDOWS_TO_OUTLAST = 3
|
||||
private const val DRAIN_MS = 3_000L
|
||||
|
||||
/**
|
||||
* The veil's job is to stay up until the screen has stopped moving. Each
|
||||
* test pins one of the ways the previous fixed-delay implementation
|
||||
* lowered it too early (issue #2327).
|
||||
* The veil's job is to go up only when content actually changes, and then to
|
||||
* stay up until the screen has stopped moving. Each test pins one of the ways
|
||||
* the original fixed-delay implementation got that wrong (issue #2327).
|
||||
*
|
||||
* The controller is built on `backgroundScope` throughout: its consumer
|
||||
* loop runs forever, so hanging it off the test's own scope would stop
|
||||
@@ -64,7 +63,7 @@ class UpdateVeilControllerTest {
|
||||
VeilSettleState(contentKey = key, hasContent = hasContent, quiescent = art == 0)
|
||||
}
|
||||
|
||||
/** Something visibly changed — re-arms the quiet window. */
|
||||
/** Content visibly changed — what the veil exists to cover. */
|
||||
fun churn() {
|
||||
state.value = state.value.copy(first = state.value.first + 1)
|
||||
}
|
||||
@@ -81,16 +80,18 @@ class UpdateVeilControllerTest {
|
||||
private fun TestScope.controllerOn(
|
||||
screen: FakeScreen,
|
||||
shouldVeil: suspend () -> Boolean = { true },
|
||||
onSessionEnd: (VeilSessionResult) -> Unit = {},
|
||||
work: suspend () -> Boolean,
|
||||
) = UpdateVeilController(
|
||||
scope = backgroundScope,
|
||||
settleSignal = screen.signal,
|
||||
shouldVeil = shouldVeil,
|
||||
timings = timings,
|
||||
onSessionEnd = onSessionEnd,
|
||||
work = work,
|
||||
)
|
||||
|
||||
/** Records every visibility transition, so a blink can't hide. */
|
||||
/** Records every visibility transition, so an extra raise can't hide. */
|
||||
private fun TestScope.recordVisibility(controller: UpdateVeilController): List<Boolean> {
|
||||
val seen = mutableListOf<Boolean>()
|
||||
backgroundScope.launch { controller.visible.collect { seen.add(it) } }
|
||||
@@ -104,6 +105,9 @@ class UpdateVeilControllerTest {
|
||||
|
||||
controller.request()
|
||||
runCurrent()
|
||||
// The pull's write lands: content changed, so the veil goes up.
|
||||
screen.churn()
|
||||
runCurrent()
|
||||
assertTrue(controller.visible.value, "veil is up while the screen is still moving")
|
||||
|
||||
// Tiles hydrating one after another, each inside the quiet window.
|
||||
@@ -126,6 +130,9 @@ class UpdateVeilControllerTest {
|
||||
|
||||
screen.artLoading(ART_IN_FLIGHT)
|
||||
controller.request()
|
||||
runCurrent()
|
||||
screen.churn()
|
||||
runCurrent()
|
||||
// Well past the quiet window and the floor — but art is still in
|
||||
// flight, so lowering now would show the covers popping in.
|
||||
advanceTimeBy(timings.minHoldMs + timings.quietMs * QUIET_WINDOWS_TO_OUTLAST)
|
||||
@@ -137,43 +144,53 @@ class UpdateVeilControllerTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `veil holds through failed attempts and their retries`() = runTest {
|
||||
fun `retries quietly, then veils the churn the successful attempt produces`() = runTest {
|
||||
val screen = FakeScreen()
|
||||
var attempts = 0
|
||||
val controller = controllerOn(screen) {
|
||||
attempts++
|
||||
attempts >= SUCCEED_ON_ATTEMPT // fail twice, succeed on the third
|
||||
val succeeded = attempts >= SUCCEED_ON_ATTEMPT // fail twice
|
||||
// Only a pull that worked writes anything.
|
||||
if (succeeded) screen.churn()
|
||||
succeeded
|
||||
}
|
||||
val seen = recordVisibility(controller)
|
||||
|
||||
controller.request()
|
||||
runCurrent()
|
||||
assertTrue(controller.visible.value)
|
||||
advanceTimeBy(timings.retryBackoffMs + 1)
|
||||
assertTrue(controller.visible.value, "veil stays up across the backoff")
|
||||
// A failed pull changes nothing, so there is nothing to hide yet —
|
||||
// the retries happen with no veil at all.
|
||||
assertFalse(controller.visible.value, "no veil over a pull that changed nothing")
|
||||
|
||||
advanceTimeBy(DRAIN_MS)
|
||||
assertEquals(SUCCEED_ON_ATTEMPT, attempts, "retries until the pull succeeds")
|
||||
assertEquals(listOf(false, true, false), seen, "raised once, lowered once")
|
||||
assertEquals(
|
||||
listOf(false, true, false),
|
||||
seen,
|
||||
"the veil went up once, over the churn the retry finally produced",
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `giving up lowers the veil quietly and does not block later requests`() = runTest {
|
||||
fun `giving up is silent, reports FAILED, and does not block later requests`() = runTest {
|
||||
val screen = FakeScreen()
|
||||
val results = mutableListOf<VeilSessionResult>()
|
||||
var attempts = 0
|
||||
var succeed = false
|
||||
val controller = controllerOn(screen) {
|
||||
val controller = controllerOn(screen, onSessionEnd = { results += it }) {
|
||||
attempts++
|
||||
succeed
|
||||
}
|
||||
|
||||
controller.request()
|
||||
controller.request(userInitiated = true)
|
||||
advanceTimeBy(DRAIN_MS)
|
||||
assertEquals(timings.attempts, attempts, "exhausts its attempts")
|
||||
assertFalse(controller.visible.value, "gives up silently")
|
||||
assertFalse(controller.visible.value, "no veil — a failed pull changed nothing")
|
||||
assertEquals(VeilOutcome.FAILED, results.single().outcome)
|
||||
assertTrue(results.single().userInitiated, "the user asked, so they're owed an answer")
|
||||
|
||||
// The operator's requirement: giving up must not latch anything off
|
||||
// — the reconnect-driven recovery still gets to try again later.
|
||||
// Giving up must not latch anything off — the reconnect-driven
|
||||
// recovery still gets to try again later.
|
||||
succeed = true
|
||||
controller.request()
|
||||
advanceTimeBy(DRAIN_MS)
|
||||
@@ -187,6 +204,7 @@ class UpdateVeilControllerTest {
|
||||
val controller = controllerOn(screen) {
|
||||
started++
|
||||
delay(WORK_MS)
|
||||
screen.churn()
|
||||
true
|
||||
}
|
||||
val seen = recordVisibility(controller)
|
||||
@@ -208,7 +226,10 @@ class UpdateVeilControllerTest {
|
||||
@Test
|
||||
fun `a never-settling screen still releases the veil at the ceiling`() = runTest {
|
||||
val screen = FakeScreen()
|
||||
val controller = controllerOn(screen) { true }
|
||||
val controller = controllerOn(screen) {
|
||||
screen.churn()
|
||||
true
|
||||
}
|
||||
|
||||
screen.artLoading(1) // an image that never completes
|
||||
controller.request()
|
||||
@@ -217,40 +238,77 @@ class UpdateVeilControllerTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a cold load runs unveiled`() = runTest {
|
||||
fun `an unchanged refresh never raises the veil and reports UNCHANGED`() = runTest {
|
||||
val screen = FakeScreen()
|
||||
val results = mutableListOf<VeilSessionResult>()
|
||||
// Succeeds without writing anything — the common case on a launch
|
||||
// over a warm cache, where the server returns what's already cached.
|
||||
val controller = controllerOn(screen, onSessionEnd = { results += it }) { true }
|
||||
val seen = recordVisibility(controller)
|
||||
|
||||
controller.request(userInitiated = true)
|
||||
advanceTimeBy(DRAIN_MS)
|
||||
|
||||
assertEquals(listOf(false), seen, "a veil over an unchanged screen would hide nothing")
|
||||
assertEquals(VeilOutcome.UNCHANGED, results.single().outcome)
|
||||
assertTrue(results.single().userInitiated, "so the caller can say 'already up to date'")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `cached content painting is not mistaken for a change`() = runTest {
|
||||
// Warm cache that hasn't painted yet: the rows arrive a moment after
|
||||
// the session starts. Treating that first paint as churn would veil
|
||||
// every single launch.
|
||||
val screen = FakeScreen(hasContent = false)
|
||||
val controller = controllerOn(screen) { true }
|
||||
|
||||
controller.request()
|
||||
runCurrent()
|
||||
screen.contentAppears()
|
||||
advanceTimeBy(DRAIN_MS)
|
||||
|
||||
assertFalse(controller.visible.value, "first paint is not churn")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a background trigger coalescing with the user's does not swallow their answer`() =
|
||||
runTest {
|
||||
val screen = FakeScreen()
|
||||
val results = mutableListOf<VeilSessionResult>()
|
||||
val controller = controllerOn(screen, onSessionEnd = { results += it }) { true }
|
||||
|
||||
// Conflation drops the older token, so the "a user asked" bit
|
||||
// cannot ride on it — it's tracked separately for exactly this.
|
||||
controller.request(userInitiated = true)
|
||||
controller.request()
|
||||
advanceTimeBy(DRAIN_MS)
|
||||
|
||||
assertTrue(
|
||||
results.first().userInitiated,
|
||||
"the user's request must not be conflated away",
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a cold load runs unveiled and is never reported as already up to date`() = runTest {
|
||||
val screen = FakeScreen()
|
||||
val results = mutableListOf<VeilSessionResult>()
|
||||
var ran = false
|
||||
val controller = controllerOn(
|
||||
screen,
|
||||
shouldVeil = { false }, // empty cache: the skeleton owns this
|
||||
onSessionEnd = { results += it },
|
||||
) {
|
||||
ran = true
|
||||
true
|
||||
}
|
||||
|
||||
controller.request()
|
||||
controller.request(userInitiated = true)
|
||||
advanceTimeBy(DRAIN_MS)
|
||||
|
||||
assertTrue(ran, "the refresh still happens")
|
||||
assertFalse(controller.visible.value, "but no veil over a skeleton")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `veil waits for content to paint before raising`() = runTest {
|
||||
val screen = FakeScreen(hasContent = false)
|
||||
val controller = controllerOn(screen) {
|
||||
delay(SLOW_WORK_MS)
|
||||
true
|
||||
}
|
||||
|
||||
controller.request()
|
||||
advanceTimeBy(WORK_MS)
|
||||
assertFalse(controller.visible.value, "nothing to hide until content is up")
|
||||
|
||||
screen.contentAppears()
|
||||
runCurrent()
|
||||
assertTrue(controller.visible.value, "raises the moment cached content paints")
|
||||
|
||||
advanceTimeBy(SLOW_WORK_MS + DRAIN_MS)
|
||||
assertFalse(controller.visible.value)
|
||||
// It went from nothing to something — that IS a change.
|
||||
assertEquals(VeilOutcome.CHANGED, results.single().outcome)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user