Repository navigation
fix: feed refresh spinner leak + lyrics glow offset on complex scripts - #317
Merged
Merged
Conversation
…ed content fetchFeed() exits without clearing isRefreshing on two paths: the ensureActive() after loadFeed (fires when a newer session or YT auth emission cancels the job mid-load) and the changed-identity guard's return@launch (connection or username changed while loading). Both left isRefreshing true, so PullToRefreshBox kept its indicator spinning over fully-loaded content indefinitely. A finally now clears the flag on every exit, guarded by feedJob === self so a newer feed job that owns the state is never reset underneath.
The active line's glow was a TextStyle.shadow (50% white, 22px), which the platform positions through its per-glyph shadow pass. For complex-script shaping that pass lands the halo off the shaped glyph positions, so Gurmukhi lines read as a second, offset image behind the text while Devanagari and Latin look fine. LyricGlowText renders the glow as a blurred copy of the very Text drawn on top of it — same text, style, layout direction and width — so the bounds are identical to the glyph run for every script by construction. Wired behind the active line in both the word-sync and line-sync branches; below API 31, where Modifier.blur is a no-op, the original shadow remains as the fallback. LyricsRtlTest gains Indic-script cases (Gurmukhi, Devanagari, Tamil must stay LTR) so a future "non-Latin means RTL" mistake cannot silently mirror a line and offset the whole lyric.
1. FeedRepository: the #315 merge landed ea6a5e8 without its fixup commit a732c68, so the add(it to ...) typo inside the forEachIndexed lambdas is live on main and the app does not compile from it. Restored here: forEachIndexed blocks use add(t to ...), the unnamed forEach blocks below keep add(it to seedScore(...)). 2. ModernLyricsView: Modifier.blur has no clip parameter in this Compose version — its second parameter is edgeTreatment. The glow uses BlurredEdgeTreatment.Unbounded so the halo is not cut at the Text's own bounds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three commits — two feature fixes, one repair found by CI on this PR.
1. Feed: pull-to-refresh spinner spins forever over loaded content —
68665aaFeedViewModel.fetchFeedhas two exit paths that never clearedisRefreshing:ensureActive()afterloadFeed— fires when a newer session/YT-auth emission cancels the job mid-load, andreturn@launch— fires when the connection or username changed while the load was in flight.Both left
isRefreshing = true, soPullToRefreshBoxkept its indicator spinning over fully-loaded content, indefinitely.Fix: a
finallyclears the flag on every exit, guarded byfeedJob === selfso a newer feed job that owns the state is never reset underneath.Note — the other half of the report ("home page takes ages to load") was the discovery-gating bug fixed in #315. This PR fixes the spinner half on top of it.
2. Lyrics: glow offset behind non-Latin lines —
af70b97The active line's glow was a
TextStyle.shadow(50 % white, 22 px). The platform positions that shadow through its per-glyph shadow pass, which lands the halo off the shaped glyph positions for complex-script shaping — Gurmukhi reads as a second, offset image behind the text, while Devanagari and Latin look fine.Fix:
LyricGlowTextdraws the glow as a blurred copy of the veryTextrendered on top of it — same text, style, layout direction, width — so the halo's bounds are identical to the glyph run for every script by construction. Wired behind the active line in both the word-sync and line-sync branches. Below API 31 (Modifier.bluris a no-op) the original shadow stays as the fallback.Tests:
LyricsRtlTestgains Indic-script cases (Gurmukhi, Devanagari, Tamil must stay LTR), so a future "non-Latin means RTL" mistake can't silently mirror a line.3.
844a187— repairs two compile breaks, and main itselfCI on this PR surfaced something bigger than my own bugs:
maincurrently does not compile. The fix: feed first-paint latency, MMAP 44.1 fallback, canvas/static cover handover #315 merge landed the feed commitea6a5e8without its fixupa732c68, so theadd(it to ...)typo inside theforEachIndexedlambdas is live on main — every build from main right now fails atFeedRepository.kt. Restored here (add(t to ...)in the indexed blocks; the unnamedforEachblocks below keepadd(it to seedScore(...))).Modifier.blurhas noclipparameter in this Compose version; the glow now usesedgeTreatment = BlurredEdgeTreatment.Unbounded.If a build of yours failed mysteriously today, this was it.
Status
CI green on
844a187(unit tests + release build). Device pass still worth doing: pull-to-refresh the feed mid-load and confirm the indicator always clears; play a Punjabi track and confirm the glow tracks the glyphs.Not addressed here: "every other page takes ages to load" — the Home/Discover/Playlist/NewReleases loading paths all reset their flags correctly in code, so that half needs a logcat pass on the specific page rather than a guess.