feat: add toggle for trailer banner audio - #110
Conversation
Himanth-reddy
left a comment
Himanth-reddy
left a comment
There was a problem hiding this comment.
Great feature! The trailer audio toggle logic is clean and well-integrated. Two quick fixes needed before merge:
1. Focus Index Collision (Critical)
The "Trailer Audio" toggle at focusedIndex == 9 collides with "Match Frame Rate" (also == 9), making the audio toggle unreachable via D-pad.
- Update "Match Frame Rate" to
focusedIndex == 10 - Increment all subsequent items: Card Layout → 11, UI Mode → 12, Skip Profile Selection → 13, DNS Provider → 14
- Update max index comment:
0 -> 13 // General: 14 items→0 -> 14 // General: 15 items(two locations in SettingsScreen.kt)
2. Trailer Aspect Ratio
Change TrailerPlayer.kt resizeMode from RESIZE_MODE_ZOOM to RESIZE_MODE_FILL so the trailer crops to fill the entire TV screen without letterboxing.
Summary
Everything else looks solid—audio state propagation is correct, DataStore persistence is safe, and the default (muted) behavior is user-friendly.
|
@Himanth-reddy done |
|
Thanks for the update. I took another pass through the changes and wanted to leave a full review here instead of the shorter earlier note. There was some confusion on my side around the trailer sizing behavior. So there is no issue there and no resize-mode change is needed in this PR. So Change the resize mode back to zoom The remaining issues I’m seeing are: 1. General settings navigation still stops too earlyThe General section now includes the new 2. Trailer audio preference is not scoped to the active profileThe new setting is stored as a profile-scoped preference, but the Home screen loads it by checking whether any saved preference key ends with 3. Trailer audio is not included in cloud sync / restore
ScopeI’m also noticing that this branch contains additional settings-related changes beyond the trailer audio feature itself. Since the core trailer-audio feature is small and the extra settings work makes the diff harder to review, I’d recommend keeping this PR focused on the trailer audio toggle and moving the unrelated settings changes into a new pull request. But I still checked the trailer cropping, it still does not fill the entire screen. If you can, make a new pull request with this update. Once the navigation limit, active-profile loading, and sync coverage are fixed, this should be in much better shape. |
0dd5974 to
b0ab5eb
Compare
|
Thanks for this feature — the trailer audio toggle logic itself is clean and the default-muted behavior is a good choice. This PR is still in 1. D-pad navigation limit still stops too early 2. Trailer audio preference is not correctly scoped to the active profile 3. Not included in cloud sync / restore 4. Scope creep
The trailer cropping / Once 1–4 are addressed, this is ready to merge. |
The movie budget line on the home hero banner makes the metadata row noisy, particularly on small screens where it pushes release date, runtime, and rating onto a second line. This adds a new Settings toggle (General > Show Budget on Home, default ON) so users who don't care about movie budgets can hide the field without losing the rest of the hero metadata. Scope of this feature was one of the multi-part requests in #72. Changes: - HomeUiState: new `showBudget: Boolean = true` field. - HomeViewModel.init: loads the new `_show_budget_on_home` key from DataStore alongside the existing trailer_auto_play load, with a default of true so existing users see no change until they explicitly disable it. - HomeScreen: wraps the existing "Budget $budgetText" Text (and its preceding `|` separator) in `if (uiState.showBudget && ...)`. No other visual changes. - SettingsUiState: new `showBudget: Boolean = true` field. - SettingsViewModel: new `showBudgetKey()` helper, loaded into UI state, and `setShowBudget(enabled)` mutator that persists + triggers cloud sync (matches the trailerAutoPlay pattern exactly). - SettingsScreen: new `SettingsToggleRow` for "Show Budget on Home" inserted at focusedIndex == 13 (immediately after "Skip Profile Selection" and before the Network section). DNS Provider shifted from focusedIndex == 13 to focusedIndex == 14, and the max-index clamp in BOTH the auto-scroll LaunchedEffect and the D-pad-down handler bumped from 13 to 14 so DNS Provider remains reachable via remote navigation. The Enter-handler switch now maps 13 -> setShowBudget and 14 -> openDnsProviderPicker. This is the recurring "settings row focus index" footgun that burned PRs #110 and #112 previously \u2014 I've updated both the dynamic max-index AND the scroll auto-scroll max AND the per-index action switch in the same commit. - CloudSyncRepository: adds `showBudget` to the `CloudProfileSettings` data class, `showBudgetKeyFor(profileId)` helper, and push/pull wiring so the setting syncs across devices via the existing account_sync_state snapshot path (the same plumbing that handles trailer_auto_play). Closes #72 (Show Budget part). The other two parts of #72 (auto-hide top bar, native debrid manager) are scoped separately as larger features and are not in this PR.
* feat: Show Budget on Home toggle in Settings (#72) The movie budget line on the home hero banner makes the metadata row noisy, particularly on small screens where it pushes release date, runtime, and rating onto a second line. This adds a new Settings toggle (General > Show Budget on Home, default ON) so users who don't care about movie budgets can hide the field without losing the rest of the hero metadata. Scope of this feature was one of the multi-part requests in #72. Changes: - HomeUiState: new `showBudget: Boolean = true` field. - HomeViewModel.init: loads the new `_show_budget_on_home` key from DataStore alongside the existing trailer_auto_play load, with a default of true so existing users see no change until they explicitly disable it. - HomeScreen: wraps the existing "Budget $budgetText" Text (and its preceding `|` separator) in `if (uiState.showBudget && ...)`. No other visual changes. - SettingsUiState: new `showBudget: Boolean = true` field. - SettingsViewModel: new `showBudgetKey()` helper, loaded into UI state, and `setShowBudget(enabled)` mutator that persists + triggers cloud sync (matches the trailerAutoPlay pattern exactly). - SettingsScreen: new `SettingsToggleRow` for "Show Budget on Home" inserted at focusedIndex == 13 (immediately after "Skip Profile Selection" and before the Network section). DNS Provider shifted from focusedIndex == 13 to focusedIndex == 14, and the max-index clamp in BOTH the auto-scroll LaunchedEffect and the D-pad-down handler bumped from 13 to 14 so DNS Provider remains reachable via remote navigation. The Enter-handler switch now maps 13 -> setShowBudget and 14 -> openDnsProviderPicker. This is the recurring "settings row focus index" footgun that burned PRs #110 and #112 previously \u2014 I've updated both the dynamic max-index AND the scroll auto-scroll max AND the per-index action switch in the same commit. - CloudSyncRepository: adds `showBudget` to the `CloudProfileSettings` data class, `showBudgetKeyFor(profileId)` helper, and push/pull wiring so the setting syncs across devices via the existing account_sync_state snapshot path (the same plumbing that handles trailer_auto_play). Closes #72 (Show Budget part). The other two parts of #72 (auto-hide top bar, native debrid manager) are scoped separately as larger features and are not in this PR. * fix: pass showBudget through to HeroSection composable (scope fix) * fix: thread showBudget through HomeHeroLayer too (second scope fix) --------- Co-authored-by: Arvin <arvin@arflix.local>
3 participants
This PR introduces a new user preference that allows users to toggle the audio for trailers playing in the Home Screen Hero Banner.
By default, trailers will remain muted to ensure a non-intrusive experience, but users can now choose to enable sound via the settings menu.
Key Changes:
New Preference: Added trailer_audio_enabled to the local DataStore. This setting is profile-specific.
Settings UI: Added a new "Trailer Audio" toggle in the General Settings > Playback section.
Audio Control: Updated the TrailerPlayer component to support an isMuted parameter. When unmuted, the trailer plays at a comfortable 50% volume (0.5f).
State Propagation: Updated HomeViewModel to load the current audio preference and pass it through to the TrailerPlayer on the Home screen.
Files Modified:
SettingsViewModel.kt: Added logic to manage and persist the new setting.
SettingsScreen.kt: Added the toggle UI row and updated the GeneralSettings layout.
HomeViewModel.kt: Updated to load the setting for the home screen UI state.
HomeScreen.kt: Passes the user's preference down to the banner player.
TrailerPlayer.kt: Added support for muting/unmuting the background video.