[TV] Add the profile Settings sheet - #5794
Conversation
Generated by 🚫 Danger |
Reviewing PR #5794
|
9313b34 to
b5b9032
Compare
|
Claude finished @sztomek's task in 5m 16s —— View job The structure is solid — the view model composes cleanly off Blocking
Non-blocking
Nits
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
b46fa11 to
bee11b6
Compare
e142cb8 to
5c360df
Compare
|
Claude finished @sztomek's task in 1m 0s —— View job Reviewed only Still open from the last review
Nits
|
bee11b6 to
f4d55dc
Compare
5c360df to
3fac200
Compare
|
Claude finished @sztomek's task in 2m 47s —— View job Reviewed the three commits since my last pass ( Blocking
Non-blocking
Nits
|
| ) | ||
|
|
||
| fun setUseEpisodeArtwork(value: Boolean) { | ||
| tvPreferences.setUsingEpisodeArtwork(value) |
There was a problem hiding this comment.
Moving the toggle off settings.artworkConfiguration onto a TV-local pref makes it inert: nothing in tv/ ever calls TvPreferences.isUsingEpisodeArtwork() except this view model, and every episode row renders podcast art unconditionally (TvEpisodeRow.kt:141 — PodcastImage.getMediumArtworkUrl(podcastUuid); TvArtworkImage takes a plain model). So the user flips "Use episode artwork", the checkmark moves, an analytics event fires, and no artwork anywhere in the app changes.
Either wire the pref into the artwork models (episode art URL when enabled, podcast art otherwise) or drop the row until it does something. As-is it also poisons SettingsAppearanceUseEpisodeArtworkToggledEvent, which on mobile means the shared appearance setting actually changed.
There was a problem hiding this comment.
Fixed by matching Apple TV. On tvOS the same toggle (SettingsMenuView) reads/writes the shared Settings.loadEmbeddedImages and is actually consumed by EpisodeRow (renders EpisodeArtworkView when on). We'd diverged twice: it was a throwaway TV-local pref, and nothing consumed it. Now: (1) the toggle binds to the shared settings.artworkConfiguration (same write as SettingsAppearanceViewModel), so the SettingsAppearanceUseEpisodeArtworkToggledEvent is honest again; (2) TvEpisodeRow loads episode artwork via PocketCastsImageRequestFactory when enabled, driven by a new LocalUseEpisodeArtwork provided from TvScaffold (same CompositionLocal pattern as LocalOpenNowPlaying). Removed the now-dead TvPreferences episode-artwork pref. Visual rendering still to be device-verified.
| } | ||
|
|
||
| private val Subscription.isManagedOnAnotherPlatform: Boolean | ||
| get() = platform != SubscriptionPlatform.Android |
There was a problem hiding this comment.
!= Android now also catches Gift and Unknown, which aren't purchases and have no platform to manage them on. Concretely, a Champion subscriber (isChampion requires platform == Gift, Subscription.kt:24) now sees "Next renewal: Lifetime" immediately followed by "This subscription was made on another platform. Please use that platform to manage the subscription." — self-contradictory, and there is no platform for them to go to.
Restrict this to the platforms the copy is about: platform == SubscriptionPlatform.iOS || platform == SubscriptionPlatform.Web, or at least exclude Gift.
There was a problem hiding this comment.
Fixed — isManagedOnAnotherPlatform now checks platform == iOS || platform == Web instead of != Android, so Gift (Champion) and Unknown no longer trigger it. A Champion subscriber now sees "Next renewal: Lifetime" with no contradictory other-platform note.
| return prefs.getBoolean(USE_EPISODE_ARTWORK_KEY, false) | ||
| } | ||
|
|
||
| fun setUsingEpisodeArtwork(isUsingEpisodeArtwork: Boolean) { |
There was a problem hiding this comment.
(non-blocking) TvSignOutManager.kt:54 calls tvPreferences.clearAll(), so this display preference is wiped on sign-out along with the per-podcast archived state. The setting it replaced was account-scoped, so that was defensible; a device-local artwork preference arguably should survive logging out.
There was a problem hiding this comment.
Resolved as a side effect of the artwork fix: the toggle no longer uses a TV-local pref at all (it writes the shared, synced settings.artworkConfiguration), so there's nothing display-related left in TvPreferences for clearAll() to wipe on sign-out.
f4d55dc to
edf5b84
Compare
3fac200 to
2ec5362
Compare
edf5b84 to
f48f27c
Compare
2ec5362 to
822daef
Compare
|
Claude finished @sztomek's task in 3m 11s —— View job Reviewed Non-blocking
Nits
|
|
Claude finished @sztomek's task in 49s —— View job Reviewed One behaviour change worth naming in the test pass: Back from Settings now returns to the home screen rather than to the profile modal, since the profile modal is dismissed on the way in. Still open from the last review
|
2 participants
Description
Adds the Settings sheet to the Android TV profile modal, bringing it to parity with the Apple TV app (
UI/Profile/SettingsMenuView.swift). The TV app previously had no settings surface of any kind.Opening the profile modal now shows a Settings entry (in both the signed-in and signed-out states, matching tvOS). It presents a settings menu with four items:
TvPreferences(the Kotlin equivalent of iOSSettings.loadEmbeddedImages, which the tvOS app deliberately keeps in localUserDefaults), default off. See the follow-up note below.Reuses existing TV components (
TvModal,TvModalButton, theqrmodule'srememberQrPainter) and the existing legal URL constants (Settings.INFO_PRIVACY_URL/Settings.INFO_TOS_URL).Analytics
Fires the EventHorizon equivalents of the tvOS screen-view events, one-shot when each surface appears (all no-property):
SettingsGeneralShownEvent— settings menu appearsAccountDetailsSubscriptionEvent— subscription info appearsAccountDetailsShowPrivacyPolicyEvent— privacy QR appearsAccountDetailsShowTosEvent— terms QR appearsSettingsAppearanceUseEpisodeArtworkToggledEvent {enabled}— fired on every flip of the Use episode artwork toggle (matches the phone's appearance setter)ProfileShownEvent— fired every time the profile modal opens (both signed-in and signed-out), matching tvOSProfileMenuView.onAppearAll four events already exist in the generated EventHorizon catalog, so no schema PR is required.
Follow-up parity fixes (audit v2)
Four ride-along commits from the v2 parity audit (Part 9):
Settings.artworkConfigurationwithupdateModifiedAt = true, so toggling it on the TV silently changed the user's phone setting. tvOS keeps this device-local. It now reads/writesTvPreferencesinstead. Known gap: no TV rendering path currently consumes this value — every TV episode row/cell renders podcast artwork (PodcastImage.getMediumArtworkUrl) regardless — so the toggle stores the preference but has no visible effect yet. Wiring TV episode-embedded artwork rendering is tracked as a separate follow-up (audit Part 5/8). This commit's purpose is to stop mutating the synced phone setting. (Because it now lives in the sharedtv_preferences, it is also cleared on the TV sign-out data wipe, like the other TV device preferences.)settings_appearance_use_episode_artwork_toggled {enabled}on every flip, matching the phone appearance setter.profile_shown. The profile modal previously tracked nothing. It now firesprofile_shownon every open (signed-in and signed-out), matching tvOSProfileMenuView.onAppearand the phone'sProfileViewModel.Fixes PCDROID-737 https://linear.app/a8c/issue/PCDROID-737/profile-settings-sheet
Testing Instructions
LoggingAnalyticsListenerlogcat, firessettings_appearance_use_episode_artwork_toggledwith the newenabledvalue.profile_showneach time (check logcat), signed-in or signed-out.Screenshots or Screencast
Screen_recording_20260827_143023.mp4
Checklist
./gradlew spotlessApplyto automatically apply formatting/linting)modules/services/localization/src/main/res/values/strings.xml