-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Jetcaster Material Expressive update #1565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ns and removal of episode from queue
Change-Id: I00ee8f7dca86aa381c33e160eb451a7d6647bebd
Change-Id: I7bc3ddf1225154a4ff4f23ed47861be3f18ddf7e
Change-Id: I09331ecf38aa02d04ef24656ce597bce963b2fbd
Change-Id: I6d3518d8190a859ebb8b9d2c1d3bbd21d4f819f5
Change-Id: I82530fb841b93a1c6b9fdbd865b9576cb502dcda
Change-Id: I1faa1772f88f3f3903aa8d5f6736dc313d3a85cb
Change-Id: Ice09cba04437882c94632c3161a38e1b4f75cbb1
Change-Id: Id839934e6471b6e7abbc652ceecbdd00b3319c01
Change-Id: Id81a3b727e44caea76b266e8d5cf5ad6fc65d5cb
Change-Id: Id0fd4d05efa02acf7e3037071abc2397dca06135
Change-Id: I844490d2d96a7351f652d7a3dee00d247ec72161
Change-Id: Ied425db3bad0c4fbb48e9730ac9740c9a31f2c77
Change-Id: I737451dad6a4be3154406e8603f4e7acf1829f7a
Change-Id: I2da7dcb902c0e09bae000a73373096b3e55d4020
Change-Id: I3c65b2f5c0feca806f9df6a6b8487f212bcab72c
Change-Id: I6ad4d0ad97796205192e851d9c633793bacf0b98
Change-Id: I6aad30ec69137b902465b8d409fb77782bbdc7df
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @MagicalMeghan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team! Gemini here, providing a summary of this pull request titled "Jetcaster Material Expressive update".
The primary goal of this PR, as indicated by the title and description, is to integrate Material Expressive design principles and components into the Jetcaster application. This involves updating the visual theme (colors, typography) and replacing standard Compose Material 3 components with their Material Expressive counterparts where appropriate. Additionally, the PR merges code from a Gerrit source, which includes adding functionality for handling multiple media URLs for episodes and implementing episode deletion.
Key areas impacted include the core data layer (adding media URLs and delete functionality), the design system (colors, typography, custom components), the mobile UI (Home, Player, Podcast Details screens adopting Material Expressive components and transitions), the TV player (integrating Media3), and the Glance/Wear modules (minor updates).
Highlights
- Material Expressive Adoption: The application's theme is updated to use Material Expressive colors and typography. Several standard Material 3 components are replaced with Material Expressive versions, such as carousels, buttons (ToggleButton, ButtonGroup), and swipe-to-dismiss.
- Episode Media URLs: The
Episode
data model is extended to support a list of media URLs, and the RSS fetching logic is updated to parse these from enclosures. A Room TypeConverter is added for persistence. - Episode Deletion: Functionality to delete episodes is added to the data layer (
EpisodeStore
) and exposed through the Home and Podcast Details ViewModels. Swipe-to-dismiss is implemented in the episode list items to trigger deletion. - Shared Element Transitions: Experimental Shared Element Transitions are introduced in the mobile UI, specifically between the Home screen's episode list items and the Player screen's episode image.
- Media3 Integration (TV): The TV module is updated to integrate the Media3 library for audio playback, replacing the previous custom player logic.
Changelog
Click here to see the changelog
- Jetcaster/core/data-testing/src/main/java/com/example/jetcaster/core/data/testing/repository/TestEpisodeStore.kt
- Added
deleteEpisode
function to the test implementation (lines 78-81).
- Added
- Jetcaster/core/data/src/main/java/com/example/jetcaster/core/data/database/model/Episode.kt
- Added
TypeConverter
andTypeConverters
imports (lines 25-26). - Annotated
Episode
data class with@TypeConverters(ListOfStringConverter::class)
(line 46). - Added
mediaUrls: List<String>
field toEpisode
data class (line 56). - Added
ListOfStringConverter
class to handle List persistence with Room (lines 59-68).
- Added
- Jetcaster/core/data/src/main/java/com/example/jetcaster/core/data/network/PodcastFetcher.kt
- Added
SyndEnclosure
import (line 27). - Modified
toPodcastResponse
to pass episode enclosures totoEpisode
(line 128). - Modified
toEpisode
function signature to acceptenclosures: List<SyndEnclosure>
(lines 150-152). - Mapped enclosure URLs to the
mediaUrls
field in theEpisode
data class (line 164).
- Added
- Jetcaster/core/data/src/main/java/com/example/jetcaster/core/data/repository/EpisodeStore.kt
- Added
deleteEpisode
function declaration to theEpisodeStore
interface (lines 60-63). - Added a blank line in
LocalEpisodeStore
(line 94). - Added
deleteEpisode
implementation toLocalEpisodeStore
(lines 113-118).
- Added
- Jetcaster/core/designsystem/src/main/java/com/example/jetcaster/designsystem/component/PodcastImage.kt
- Added
imageModifier
parameter toPodcastImage
(line 47). - Applied the main
modifier
to the placeholderBox
(line 85). - Applied the combined
modifier.then(imageModifier)
to theAsyncImage
(line 97).
- Added
- Jetcaster/core/designsystem/src/main/java/com/example/jetcaster/designsystem/theme/Color.kt
- Replaced the entire color palette definition with new Material Expressive colors (lines 20-234).
- Jetcaster/core/designsystem/src/main/java/com/example/jetcaster/designsystem/theme/Type.kt
- Added
TextAlign
import (line 21). - Modified
displayLarge
style to useRobotoFlex
, update font size/line height, add fontWeight and textAlign (lines 26-31). - Modified
displayMedium
style to useRobotoFlex
(line 33).
- Added
- Jetcaster/core/designsystem/src/main/java/com/example/jetcaster/designsystem/theme/Typography.kt
- Added
RobotoFlex
font family definition (lines 31-33).
- Added
- Jetcaster/core/domain-testing/build.gradle.kts
- Updated
compileSdk
andminSdk
parsing style (lines 8-11, 14-17). - Added trailing comma to
proguardFiles
list (line 28).
- Updated
- Jetcaster/core/domain/build.gradle.kts
- Updated
compileSdk
andminSdk
parsing style (lines 9-12, 16-19). - Added trailing comma to
proguardFiles
list (line 30).
- Updated
- Jetcaster/core/domain/src/main/java/com/example/jetcaster/core/model/EpisodeInfo.kt
- Added
podcastUri: String
andmediaUrls: List<String>
fields toEpisodeInfo
data class (lines 28, 35). - Updated
asExternalModel
to mappodcastUri
andmediaUrls
(lines 41, 48). - Added
asDaoModel
extension function to convertEpisodeInfo
back toEpisode
(lines 51-62).
- Added
- Jetcaster/core/player/model/PlayerEpisode.kt
- Added
mediaUrls:List<String>
field toPlayerEpisode
data class (line 38). - Updated primary constructor usage to include
mediaUrls
(line 50). - Updated
toPlayerEpisode
extension function to mapmediaUrls
(line 65).
- Added
- Jetcaster/glancewidget/build.gradle.kts
- Updated
compileSdk
andminSdk
parsing style (lines 9-12, 15-18).
- Updated
- Jetcaster/glancewidget/src/main/java/com/example/jetcaster/glancewidget/JetcasterAppWidget.kt
- Updated copyright year to 2025 (line 2).
- Removed blank line before
testState
initialization (line 120). - Added trailing comma to
Row
modifier parameters (lines 180-181). - Simplified
Widget
function signature and removed comment about title bar (lines 191-193). - Simplified
AlbumArt
function signature (lines 213-214). - Removed trailing commas from
maxLines
parameters inPodcastText
(lines 230, 235, 246). - Added trailing comma to
PlayPauseButton
parameters (line 256). - Added trailing comma to
WidgetAsyncImage
parameters (line 283).
- Jetcaster/glancewidget/src/main/java/com/example/jetcaster/glancewidget/JetcasterAppWidgetPreview.kt
- Updated copyright year to 2025 (line 2).
- Removed blank line before
if
statement (line 59). - Removed trailing comma from
compose
parameters (line 70). - Removed blank line before
provideContent
(line 84). - Removed blank line before
Scaffold
(line 94).
- Jetcaster/gradle/libs.versions.toml
- Added
media3
version definition (line 47). - Changed
androidx-compose-bom
module fromcompose-bom
tocompose-bom-alpha
(line 70). - Added
androidx-media3-ui-compose
dependency (line 107). - Added
androidx-media3-session
andandroidx-media3-exoplayer
dependencies (lines 163-164).
- Added
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/JetcasterApp.kt
- Updated copyright year to 2025 (line 2).
- Added
@file:OptIn(ExperimentalSharedTransitionApi::class)
(line 17). - Added imports for
AnimatedVisibilityScope
,ExperimentalSharedTransitionApi
,SharedTransitionLayout
,SharedTransitionScope
,CompositionLocalProvider
, andcompositionLocalOf
(lines 21-26, 32-33). - Removed
@OptIn(ExperimentalMaterial3AdaptiveApi::class)
(line 30). - Wrapped
NavHost
inSharedTransitionLayout
and providedLocalSharedTransitionScope
andLocalAnimatedVisibilityScope
(lines 50-84). - Defined
LocalAnimatedVisibilityScope
andLocalSharedTransitionScope
composition locals (lines 105-106).
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/MainActivity.kt
- Updated copyright year to 2025 (line 2).
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/home/Home.kt
- Removed
@file:OptIn(ExperimentalFoundationApi::class)
(line 17). - Removed
ExperimentalFoundationApi
,HorizontalPager
,PageSize
,PagerState
,RoundedCornerShape
,HorizontalDivider
, andwidth
imports (lines 22, 40, 44-46, 48, 54). - Added
LibraryMusic
icon,ButtonColors
,ExperimentalMaterial3ExpressiveApi
,FloatingToolbarColors
,HorizontalFloatingToolbar
,HorizontalMultiBrowseCarousel
,rememberCarouselState
,Brush
, andpainterResource
imports (lines 43, 46-50, 75-76, 89-91). - Added trailing comma to
BackHandler
launch block (lines 262-264). - Removed
showHomeCategoryTabs
,homeCategories
, andpagerState
parameters fromHomeScreen
(lines 436-438). - Added
PillToolbar
composable usingHorizontalFloatingToolbar
for category selection (lines 454-553). - Removed
showHomeCategoryTabs
,pagerState
, andhomeCategories
parameters fromHomeContent
(lines 577-579). - Removed
showHomeCategoryTabs
,pagerState
, andhomeCategories
parameters fromHomeContentGrid
(lines 591-593). - Removed
HorizontalPager
and replaced it withHorizontalMultiBrowseCarousel
inHomeContentGrid
(lines 605-618). - Added
removeFromQueue
parameter tolibraryItems
call (line 625). - Added
removeFromQueue
parameter topodcastCategory
call (line 640). - Removed
pagerState
parameter fromFollowedPodcastItem
(line 649). - Removed
pagerState
parameter fromFollowedPodcasts
(line 658). - Removed
HomeCategoryTabs
andHomeCategoryTabIndicator
composables (lines 598-657). - Removed
FEATURED_PODCAST_IMAGE_SIZE_DP
constant (line 659). - Replaced
HorizontalPager
withHorizontalMultiBrowseCarousel
inFollowedPodcasts
(lines 685-690). - Added
maskClip(MaterialTheme.shapes.large)
modifier toFollowedPodcastCarouselItem
clickable (line 699). - Modified
FollowedPodcastCarouselItem
layout and styling to use a gradient overlay and reposition the unfollow button and date text (lines 716-746). - Removed
@OptIn(ExperimentalMaterial3Api::class)
fromHomeAppBarPreview
(line 763).
- Removed
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/home/HomeViewModel.kt
- Added
EpisodeInfo
andasDaoModel
imports (lines 29, 34). - Added handling for
HomeAction.RemoveEpisode
inonHomeAction
(line 166). - Added
deleteEpisode
function to delete an episode usingepisodeStore
(lines 199-202). - Added
RemoveEpisode
data class toHomeAction
sealed interface (line 223).
- Added
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/home/category/PodcastCategory.kt
- Added
@file:OptIn(ExperimentalSharedTransitionApi::class, ExperimentalSharedTransitionApi::class)
(line 17). - Removed
Arrangement
,Column
,PaddingValues
,LazyRow
, anditems
(from lazy) imports (lines 20, 22-23, 29, 32). - Added
ExperimentalSharedTransitionApi
,background
,height
,HorizontalUncontainedCarousel
,rememberCarouselState
,Brush
, andColor
imports (lines 21-22, 28, 36-37, 42-43). - Added
LocalAnimatedVisibilityScope
andLocalSharedTransitionScope
imports (lines 54-55). - Added
removeFromQueue
parameter topodcastCategory
(line 66). - Added usage of
LocalSharedTransitionScope
andLocalAnimatedVisibilityScope
for shared element transitions onEpisodeListItem
(lines 80-101). - Removed
LazyRow
and replaced it withHorizontalUncontainedCarousel
inCategoryPodcastRow
(lines 128-133). - Added
maskClip(MaterialTheme.shapes.large)
modifier toTopPodcastRowItem
clickable (line 145). - Modified
TopPodcastRowItem
layout and styling to use a gradient overlay, reposition the follow button, and change text padding (lines 158-192). - Updated
FilterChip
shape toMaterialTheme.shapes.large
inChoiceChipContent
(line 149).
- Added
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/home/discover/Discover.kt
- Added
removeFromQueue
parameter todiscoverItems
(line 56). - Added
removeFromQueue
parameter topodcastCategory
call (line 84). - Added trailing comma to
PodcastCategoryTabs
parameters (line 92). - Added trailing comma to
ChoiceChipContent
parameters (line 122). - Updated
FilterChip
shape toMaterialTheme.shapes.large
inChoiceChipContent
(line 149).
- Added
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/home/library/Library.kt
- Added
removeFromQueue
parameter tolibraryItems
(line 40). - Changed headline style from
headlineLarge
toheadlineMedium
(line 49). - Added
animateItem
andremoveFromQueue
parameters toEpisodeListItem
call (lines 64-65).
- Added
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/player/PlayerScreen.kt
- Added
@file:OptIn(ExperimentalSharedTransitionApi::class)
(line 17). - Removed
ExperimentalFoundationApi
,Image
,background
,clickable
,CircleShape
,Dp
,alpha
,ColorFilter
,Role
,role
, andsemantics
imports (lines 19-21, 23, 46, 88, 77, 79, 82-84). - Added
ExperimentalSharedTransitionApi
,Spring
,spring
,fadeIn
,fadeOut
,width
,RoundedCornerShape
,ButtonGroup
,ButtonShapes
,ExperimentalMaterial3ExpressiveApi
,IconButtonColors
,ToggleButton
,ToggleButtonColors
imports (lines 21-25, 46, 50, 62-68, 76-77). - Added
LocalAnimatedVisibilityScope
andLocalSharedTransitionScope
imports (lines 105-106). - Added trailing comma to
PlayerScreen
parameters (line 162). - Added trailing comma to
PlayerContentWithBackground
parameters (line 221). - Added trailing comma to
PlayerContent
parameters (line 263). - Added
LocalSharedTransitionScope
andLocalAnimatedVisibilityScope
locals and used them for shared element transition on the player image inPlayerContentRegular
(lines 360-402). - Added trailing comma to
PlayerContentTableTopTop
parameters (line 441). - Added trailing comma to
PlayerContentTableTopBottom
parameters (line 474). - Removed
titleTextStyle
parameter fromPodcastDescription
call inPlayerContentTableTopBottom
(line 496). - Removed
playerButtonSize
parameter fromPlayerButtons
call inPlayerContentTableTopBottom
(line 507). - Added trailing comma to
PlayerContentBookStart
parameters (line 529). - Added trailing comma to
PlayerContentBookEnd
parameters (line 557). - Added
imageModifier
parameter toPlayerImage
(line 626). - Applied
imageModifier
to thePodcastImage
call inPlayerImage
(line 636). - Removed
titleTextStyle
parameter fromPodcastDescription
(line 643). - Updated
PodcastDescription
typography styles todisplayLarge
andbodyLarge
(lines 647, 654). - Updated
PodcastInformation
titleTextStyle
default value toheadlineLarge
(line 666). - Added
@OptIn(ExperimentalMaterial3ExpressiveApi::class)
toPlayerButtons
(line 737). - Removed
playerButtonSize
andsideButtonSize
parameters fromPlayerButtons
(line 748). - Replaced
Row
withColumn
andButtonGroup
inPlayerButtons
(lines 750, 775). - Replaced
Image
withToggleButton
andIconButton
using Material Expressive components and shapes inPlayerButtons
(lines 751-856).
- Added
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/podcast/PodcastDetailsScreen.kt
- Removed
BoxWithConstraints
,Row
,Spacer
, andmin
imports (lines 25, 27-28, 68). - Added
height
,width
,RoundedCornerShape
,NotificationsActive
,NotificationsNone
,ButtonGroup
,ButtonShapes
,ExperimentalMaterial3ExpressiveApi
,ToggleButton
, andToggleButtonColors
imports (lines 28-32, 40-43, 45-46, 53-54). - Added
removeFromQueue
parameter toPodcastDetailsScreen
(line 136). - Added
removeFromQueue
parameter toPodcastDetailsContent
(line 176). - Added
animateItem
andremoveFromQueue
parameters toEpisodeListItem
call (lines 202, 198). - Replaced
BoxWithConstraints
withBox
inPodcastDetailsHeaderItem
(line 216). - Modified
PodcastDetailsHeaderItem
layout to center the image and place the title below it (lines 220-235). - Removed comment about gradient effect in
PodcastDetailsDescription
(line 281). - Added
@OptIn(ExperimentalMaterial3ExpressiveApi::class)
toPodcastDetailsHeaderItemButtons
(line 297). - Replaced
Row
withButtonGroup
inPodcastDetailsHeaderItemButtons
(line 305). - Replaced
Button
andIconButton
withToggleButton
using Material Expressive components and shapes inPodcastDetailsHeaderItemButtons
, including a new notification toggle (lines 306-359).
- Removed
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/shared/EpisodeListItem.kt
- Added
fillMaxSize
,fillMaxWidth
,Delete
,SwipeToDismissBox
,SwipeToDismissBoxValue
, andrememberSwipeToDismissBoxState
imports (lines 26-27, 32, 39-42). - Added
removeFromQueue
parameter toEpisodeListItem
(line 75). - Added
imageModifier
parameter toEpisodeListItem
(line 78). - Wrapped the content in
SwipeToDismissBox
to enable swipe-to-dismiss functionality (lines 83-144). - Implemented background content for swipe-to-dismiss with a delete icon (lines 87-98).
- Handled
SwipeToDismissBoxValue.EndToStart
to callremoveFromQueue
(line 135). - Added
imageModifier
parameter toEpisodeListItemHeader
(line 232). - Added
imageModifier
parameter toEpisodeListItemImage
(line 286). - Applied
imageModifier
to thePodcastImage
call inEpisodeListItemImage
(line 292).
- Added
- Jetcaster/mobile/src/main/java/com/example/jetcaster/ui/theme/Theme.kt
- Added
ExperimentalMaterial3ExpressiveApi
,MaterialExpressiveTheme
, andMotionScheme
imports (lines 21-23). - Added
@OptIn(ExperimentalMaterial3ExpressiveApi::class)
toJetcasterTheme
(line 486). - Changed
MaterialTheme
toMaterialExpressiveTheme
and setmotionScheme = MotionScheme.expressive()
(lines 503-505).
- Added
- Jetcaster/mobile/src/main/java/com/example/jetcaster/util/Buttons.kt
- Removed
animateColorAsState
,animateDpAsState
,background
,padding
,shadow
,onClick
(semantics), andsemantics
imports (lines 19-22, 32, 34-35). - Added
RoundedCornerShape
,ExperimentalMaterial3ExpressiveApi
,IconButtonShapes
,IconToggleButton
, andIconToggleButtonColors
imports (lines 20, 24, 26-28). - Added
@OptIn(ExperimentalMaterial3ExpressiveApi::class)
(line 37). - Replaced
IconButton
withIconToggleButton
using Material Expressive colors and shapes (lines 44-60). - Removed animation and modifier chain related to background, shadow, and padding (lines 62-82).
- Removed
- Jetcaster/mobile/src/main/res/drawable/genres.xml
- Added new drawable resource for a 'genres' icon (lines 1-10).
- Jetcaster/mobile/src/main/res/values/strings.xml
- Added new strings for Discover/Library toolbar labels and content descriptions (lines 30-33).
- Jetcaster/tv/build.gradle.kts
- Updated
compileSdk
,minSdk
, andtargetSdk
parsing style (lines 27-30, 34-37, 38-41). - Added trailing comma to
proguardFiles
list (line 68). - Removed blank line (line 88).
- Added Media3 dependencies (
exoplayer
,session
,ui-compose
) (lines 118-120).
- Updated
- Jetcaster/tv/src/main/java/com/example/jetcaster/tv/ui/player/PlayerScreen.kt
- Added
Context
,Uri
, andDisposableEffect
imports (lines 19-20, 38). - Added Media3 related imports (
C
,MediaItem
,Player.REPEAT_MODE_ALL
,UnstableApi
,DefaultDataSource
,ExoPlayer
,ProgressiveMediaSource
,MediaSession
,PlayerSurface
,resizeWithContentScale
) (lines 64-73). - Integrated Media3
ExoPlayer
andMediaSession
into thePlayer
composable (lines 154-167). - Added
PlayerSurface
to display the audio player (lines 171-177). - Updated player control actions (
play
,pause
,skip
,rewind
) to interact with theExoPlayer
(lines 183-200). - Added
rememberPlayer
composable to create and remember theExoPlayer
instance (lines 535-546).
- Added
- Jetcaster/wear/src/main/java/com/example/jetcaster/MainActivity.kt
- Updated copyright year to 2025 (line 2).
- Jetcaster/wear/src/main/java/com/example/jetcaster/WearApp.kt
- Updated copyright year to 2025 (line 2).
- Removed trailing comma from
NavHost
state parameter (line 70). - Removed trailing comma from
composable
deepLinks parameter (line 75). - Removed trailing comma from
LibraryScreen
onUpNextClick parameter (line 97). - Removed trailing comma from
ScreenScaffold
backStack parameter (line 100). - Removed trailing comma from
composable
deepLinks parameter (line 107).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Compose views with ease,
Material flows like a breeze,
Expressive and bright,
Code reviews take flight,
Merging changes, if you please.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request brings a substantial Material Expressive theme update to Jetcaster, enhancing the visual appeal and user experience with new components and shared element transitions. The data handling for episodes has been improved with support for multiple media URLs and deletion capabilities. The TV player's direct ExoPlayer integration is also a notable enhancement.
The code is generally well-organized, and the UI changes are consistent with the Material Expressive guidelines. I have a few suggestions, primarily around data robustness and minor maintainability points.
As I am an AI, I am not authorized to approve pull requests. Please ensure that this PR is reviewed and approved by other maintainers before merging.
Summary of Findings
- Data Robustness: Comma in Media URLs: The
ListOfStringConverter
inEpisode.kt
uses a comma delimiter. If a media URL contains a comma, parsing will be incorrect. Consider JSON serialization for more robustness if this is a possibility. - Data Robustness: Empty Media URLs in TV Player: The TV player's ExoPlayer integration directly accesses
mediaUrls[0]
. This could cause a crash ifmediaUrls
is empty. A check should be added. - Maintainability: Repetitive UI Logic: The
PillToolbar
inHome.kt
andPlayerButtons
inPlayerScreen.kt
(mobile) have some repetitive styling logic for selected/unselected states and button configurations. Consider refactoring if these patterns are reused. - Dependency: Compose BOM Alpha: The
androidx-compose-bom
has been updated toandroidx-compose-bom-alpha
inlibs.versions.toml
. This implies using alpha versions of Jetpack Compose, which should be an intentional decision, as alpha versions might have API instability or bugs. (Not commented directly due to review settings) - Code Style: Minor Formatting: Several files (
JetcasterAppWidget.kt
,JetcasterAppWidgetPreview.kt
, Gradle files) have minor formatting changes (blank lines, trailing commas). These are generally fine and likely from an auto-formatter. (Not commented directly due to review settings)
Merge Readiness
This pull request introduces significant and positive changes, particularly with the Material Expressive update and shared element transitions. The codebase is generally in good shape.
However, I recommend addressing the high
severity issue regarding potential crashes in the TV player due to empty mediaUrls
before merging. The medium
severity items related to the ListOfStringConverter
robustness and potential refactoring of UI styling logic should also be considered for improvement, though they are not strictly blockers.
As an AI, I am not authorized to approve pull requests. Please ensure that this PR is reviewed and approved by other maintainers, especially after addressing the highlighted concerns.
val exoPlayer = rememberPlayer(context) | ||
|
||
DisposableEffect(exoPlayer, playEpisode) { | ||
exoPlayer.setMediaItem(MediaItem.fromUri(Uri.parse(currentEpisode.mediaUrls[0]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code exoPlayer.setMediaItem(MediaItem.fromUri(Uri.parse(currentEpisode.mediaUrls[0])))
assumes that currentEpisode.mediaUrls
is not empty and that the first URL is always the one to play.
What is the expected behavior if mediaUrls
is empty? This could lead to an IndexOutOfBoundsException
. It might be prudent to add a check for mediaUrls.isNotEmpty()
before attempting to access the first element, and perhaps handle the case where no media URL is available (e.g., show an error, disable playback).
if (currentEpisode.mediaUrls.isNotEmpty()) {
exoPlayer.setMediaItem(MediaItem.fromUri(Uri.parse(currentEpisode.mediaUrls[0])))
} else {
// Handle case where mediaUrls is empty, e.g., log an error or show a message
}
fun fromString(value: String): List<String> { | ||
return value.split(",") | ||
} | ||
|
||
@TypeConverter | ||
fun fromList(list: List<String>): String { | ||
return list.joinToString(",") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ListOfStringConverter
uses a simple comma (,
) as a delimiter for storing a list of media URLs as a single string. While this works for many cases, could there be a scenario where a URL itself might contain a comma? If so, this would lead to incorrect parsing.
Consider if a more robust serialization method, like storing the list as a JSON array string (e.g., using kotlinx.serialization
), might be safer for future-proofing, or if it's confirmed that media URLs will never contain commas.
HorizontalFloatingToolbar( | ||
modifier = modifier, | ||
colors = FloatingToolbarColors( | ||
toolbarContainerColor = MaterialTheme.colorScheme.surfaceContainerHighest, | ||
toolbarContentColor = MaterialTheme.colorScheme.onSurfaceVariant, | ||
fabContainerColor = MaterialTheme.colorScheme.tertiary, | ||
fabContentColor = MaterialTheme.colorScheme.onTertiary | ||
), | ||
expanded = true, | ||
content = { | ||
val libraryContainerColor = | ||
if (selectedHomeCategory.name == HomeCategory.Library.name) { | ||
MaterialTheme.colorScheme.secondary | ||
} else { | ||
MaterialTheme.colorScheme.surfaceContainerHighest | ||
} | ||
|
||
val libraryContentColor = | ||
if (selectedHomeCategory.name == HomeCategory.Library.name) { | ||
MaterialTheme.colorScheme.surfaceContainerHighest | ||
} else { | ||
MaterialTheme.colorScheme.onSurfaceVariant | ||
} | ||
|
||
Button( | ||
onClick = { onHomeAction(HomeAction.HomeCategorySelected(HomeCategory.Library)) }, | ||
colors = ButtonColors( | ||
containerColor = libraryContainerColor, | ||
contentColor = libraryContentColor, | ||
disabledContainerColor = MaterialTheme.colorScheme.surfaceContainerHighest, | ||
disabledContentColor = MaterialTheme.colorScheme.onSurfaceVariant | ||
) | ||
) { | ||
Row(Modifier) { | ||
Icon( | ||
Icons.Filled.LibraryMusic, | ||
modifier = Modifier.padding(end = 8.dp), | ||
contentDescription = stringResource( | ||
R.string.library_toolbar_content_description | ||
) | ||
) | ||
Text(stringResource(R.string.library_toolbar)) | ||
} | ||
} | ||
|
||
val discoverContainerColor = | ||
if (selectedHomeCategory.name == HomeCategory.Library.name) { | ||
MaterialTheme.colorScheme.surfaceContainerHighest | ||
} else { | ||
MaterialTheme.colorScheme.secondary | ||
} | ||
|
||
val discoverContentColor = | ||
if (selectedHomeCategory.name == HomeCategory.Library.name) { | ||
MaterialTheme.colorScheme.onSurfaceVariant | ||
} else { | ||
MaterialTheme.colorScheme.surfaceContainerHighest | ||
} | ||
|
||
Button( | ||
onClick = { onHomeAction(HomeAction.HomeCategorySelected(HomeCategory.Discover)) }, | ||
colors = ButtonColors( | ||
containerColor = discoverContainerColor, | ||
contentColor = discoverContentColor, | ||
disabledContainerColor = MaterialTheme.colorScheme.secondary, | ||
disabledContentColor = MaterialTheme.colorScheme.surfaceContainerHighest | ||
) | ||
) { | ||
Row { | ||
Icon( | ||
painterResource(R.drawable.genres), | ||
modifier = Modifier.padding(end = 8.dp), | ||
contentDescription = stringResource( | ||
R.string.discover_toolbar_content_description | ||
) | ||
) | ||
Text(stringResource(R.string.discover_toolbar)) | ||
} | ||
} | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for determining libraryContainerColor
, libraryContentColor
, discoverContainerColor
, and discoverContentColor
within the PillToolbar
is a bit repetitive for the two buttons. While it's clear for two items, if this pattern of conditional coloring for selected/unselected states were to expand or be reused, would it be beneficial to encapsulate this logic into a helper function or a data structure to reduce duplication and improve maintainability?
.clickable { | ||
onPlayPress() | ||
} | ||
) | ||
val rewindFastForwardButtonsModifier = Modifier | ||
.size(68.dp) | ||
|
||
IconButton( | ||
onClick = onPrevious, | ||
modifier = skipButtonsModifier, | ||
shape = RoundedCornerShape(50.dp), | ||
colors = IconButtonColors( | ||
containerColor = MaterialTheme.colorScheme.surfaceContainerHighest, | ||
contentColor = MaterialTheme.colorScheme.onSurface, | ||
disabledContainerColor = MaterialTheme.colorScheme.surfaceContainerHighest, | ||
disabledContentColor = MaterialTheme.colorScheme.onPrimary | ||
), | ||
enabled = isPlaying, | ||
) { | ||
Icon( | ||
imageVector = Icons.Filled.SkipPrevious, | ||
contentDescription = null | ||
) | ||
} | ||
|
||
IconButton( | ||
onClick = { onRewindBy(Duration.ofSeconds(10)) } , | ||
modifier = rewindFastForwardButtonsModifier, | ||
shape = RoundedCornerShape(15.dp), | ||
colors = IconButtonColors( | ||
containerColor = MaterialTheme.colorScheme.secondary, | ||
contentColor = MaterialTheme.colorScheme.onSecondary, | ||
disabledContainerColor = MaterialTheme.colorScheme.secondary, | ||
disabledContentColor = MaterialTheme.colorScheme.onSecondary | ||
), | ||
enabled = isPlaying, | ||
) { | ||
Icon( | ||
imageVector = Icons.Filled.Replay10, | ||
contentDescription = null | ||
) | ||
} | ||
|
||
IconButton( | ||
onClick = { onAdvanceBy(Duration.ofSeconds(10)) }, | ||
modifier = rewindFastForwardButtonsModifier, | ||
shape = RoundedCornerShape(15.dp), | ||
colors = IconButtonColors( | ||
containerColor = MaterialTheme.colorScheme.secondary, | ||
contentColor = MaterialTheme.colorScheme.onSecondary, | ||
disabledContainerColor = MaterialTheme.colorScheme.secondary, | ||
disabledContentColor = MaterialTheme.colorScheme.onSecondary | ||
), | ||
enabled = isPlaying, | ||
) { | ||
Icon( | ||
imageVector = Icons.Filled.Forward10, | ||
contentDescription = null | ||
) | ||
} | ||
|
||
IconButton( | ||
onClick = onNext, | ||
modifier = skipButtonsModifier, | ||
shape = RoundedCornerShape(50.dp), | ||
colors = IconButtonColors( | ||
containerColor = MaterialTheme.colorScheme.surfaceContainerHighest, | ||
contentColor = MaterialTheme.colorScheme.onSurface, | ||
disabledContainerColor = MaterialTheme.colorScheme.surfaceContainerHighest, | ||
disabledContentColor = MaterialTheme.colorScheme.onSurface | ||
), | ||
enabled = hasNext, | ||
) { | ||
Icon( | ||
imageVector = Icons.Filled.SkipNext, | ||
contentDescription = null | ||
) | ||
} | ||
} | ||
Image( | ||
imageVector = Icons.Filled.Forward10, | ||
contentDescription = stringResource(R.string.cd_forward10), | ||
contentScale = ContentScale.Inside, | ||
colorFilter = ColorFilter.tint(MaterialTheme.colorScheme.onSurface), | ||
modifier = sideButtonsModifier | ||
.clickable { | ||
onAdvanceBy(Duration.ofSeconds(10)) | ||
} | ||
) | ||
Image( | ||
imageVector = Icons.Filled.SkipNext, | ||
contentDescription = stringResource(R.string.cd_skip_next), | ||
contentScale = ContentScale.Inside, | ||
colorFilter = ColorFilter.tint(MaterialTheme.colorScheme.onSurfaceVariant), | ||
modifier = sideButtonsModifier | ||
.clickable(enabled = hasNext, onClick = onNext) | ||
.alpha(if (hasNext) 1f else 0.25f) | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The styling for the ToggleButton
(play/pause) and the IconButton
s within the ButtonGroup
(skip, rewind, forward) involves detailed ToggleButtonColors
, ButtonShapes
, and IconButtonColors
definitions. This provides a nice Material Expressive look.
If these specific color and shape configurations are intended to be standard for these types of controls throughout the app, would it make sense to define these custom color/shape sets as reusable constants or perhaps as part of an extended theme to promote consistency and simplify their application elsewhere?
Adding Material Expressive to Jetcaster and merging gerrit code