Java to Kotlin conversion for Domain Models#476
Conversation
* Updated to non deprecated preferences and drawable * Code converted from java to kotlin
Converted the model classes from Java to Kotlin. Fixed up various compilation issues after the conversion and updated the tests.
Add several unit tests for media containers, catching a couple of issues with the code under test.
WalkthroughThis pull request performs a significant Java-to-Kotlin migration of the model layer, including core interfaces (ContentInfo, VideoContentInfo, SeriesContentInfo), base implementation classes (AbstractMediaContainer, AbstractSeriesContentInfo, AbstractVideoContentInfo), and supporting data and container classes. Updates the Spotless plugin version and modifies the UI layer to invoke getter methods instead of accessing properties directly. Adds new unit tests for migrated classes and restructures several layout XML files. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt (1)
71-78: Fix async coroutines not being awaited.The
asyncblocks in theforEachloop are never awaited, creating a fire-and-forget pattern that can lead to:
- Parent coroutine completing before child coroutines finish
- Unhandled exceptions being swallowed
- Potential memory leaks if coroutines don't complete properly
Apply this diff to fix the issue:
- presenterScope.launch(Dispatchers.IO) { - seasons.forEach { season -> - async { - val result = repository.fetchEpisodes(season.key.orEmpty()) - withContext(Dispatchers.Main) { - viewState.updateSeasonEpisodes(season, result) - } - } - } - } + seasons.forEach { season -> + presenterScope.launch(Dispatchers.IO) { + val result = repository.fetchEpisodes(season.key.orEmpty()) + withContext(Dispatchers.Main) { + viewState.updateSeasonEpisodes(season, result) + } + } + }This changes
asynctolaunchand moves the scope launch inside the loop, which is more appropriate for independent concurrent operations that update the UI as they complete.serenity-app/src/test/kotlin/us/nineworlds/serenity/core/imageloader/BackgroundBitmapDisplayerTest.kt (1)
41-74: Reset shared prefs between tests to avoid order-dependent failures.
PreferenceManager.getDefaultSharedPreferences(...)persists the fade-in flag across runs, so once one test enables it, any test that expects the defaultfalsewill start failing if execution order changes.(developer.android.com) Please clear the"animation_background_fadein"key intearDown()(or reset it at the start of each test) so the suite remains deterministic.@After fun tearDown() { clearAllMocks() + PreferenceManager.getDefaultSharedPreferences(ApplicationProvider.getApplicationContext()) + .edit { + remove("animation_background_fadein") + } }
🧹 Nitpick comments (6)
serenity-app/src/main/res/layout-large/leanback_fullwidth_details_overview.xml (1)
57-57: Remove redundantvisibility="visible"attribute.The
android:visibility="visible"attribute on line 57 is the default and does not need to be explicitly set.- android:visibility="visible" android:orientation="vertical"serenity-app/src/main/java/us/nineworlds/serenity/core/model/CategoryInfo.kt (1)
3-11: Consider using immutable properties.The properties are declared as
var, making them mutable. Per the coding guidelines, prefer immutable data structures where possible.If the properties don't need to be mutated after initialization, consider converting to a data class with immutable properties:
-open class CategoryInfo { - var level: Int = 0 - var category: String? = null - var categoryDetail: String? = null +open class CategoryInfo( + val level: Int = 0, + val category: String? = null, + val categoryDetail: String? = null +) {Based on coding guidelines.
serenity-app/src/main/java/us/nineworlds/serenity/core/model/SecondaryCategoryInfo.kt (1)
3-5: Consider using immutable properties.Similar to
CategoryInfo, this class uses a mutable property. For consistency and to align with the coding guidelines preference for immutability, consider using a constructor parameter withval.-class SecondaryCategoryInfo : CategoryInfo() { - var parentCategory: String? = null -} +class SecondaryCategoryInfo( + level: Int = 0, + category: String? = null, + categoryDetail: String? = null, + val parentCategory: String? = null +) : CategoryInfo(level, category, categoryDetail)Based on coding guidelines.
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/Subtitle.kt (1)
5-10: Consider immutability for the data class properties.All properties are declared as
var, making them mutable. IfSubtitleinstances don't need to change after construction, consider usingvalfor immutability, which aligns better with data class semantics and can prevent unintended modifications.Apply this diff if immutability is appropriate:
data class Subtitle( - var key: String? = null, - var format: String? = null, - var description: String? = null, - var languageCode: String? = null + val key: String? = null, + val format: String? = null, + val description: String? = null, + val languageCode: String? = null ) : Serializable {serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeasonsMediaContainer.kt (1)
25-25: Avoid non-null assertion operator onvideoList.Using
!!can throw aNullPointerExceptionifvideoListis null. WhilevideoListis likely initialized in the parent classSeriesMediaContainer, it's safer to use a null-safe call or verify initialization.Apply this diff to use a safe call:
- videoList!!.add(mpi) + videoList?.add(mpi)serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.kt (1)
5-20: Consider naming consistency for id().The interface mixes naming conventions:
id()doesn't follow thegetXxx()pattern used by other getters (getType(),getTitle(), etc.). While this might be intentional for migration compatibility, consider renamingid()togetId()for consistency, or document the rationale for the different naming.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (80)
AGENTS.md(1 hunks)build.gradle(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/imageloader/BackgroundBitmapDisplayer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/imageloader/BackgroundBitmapDisplayer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/ArtistContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/CategoryInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/CategoryInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/DBMetaData.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/SecondaryCategoryInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/SecondaryCategoryInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/SeriesContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/SeriesContentInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/TrackContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/VideoContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/VideoContentInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AudioTrackContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/CategoryMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/CategoryMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodePosterInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodePosterInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MenuMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MenuMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MovieMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MovieMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MoviePosterInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MoviePosterInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MusicAlbumContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MusicArtistContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeasonsMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeasonsMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SecondaryCategoryMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SecondaryCategoryMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeriesMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeriesMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/Subtitle.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/Subtitle.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SubtitleMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SubtitleMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVCategoryMediaContainer.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVCategoryMediaContainer.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVShowSeriesInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVShowSeriesInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/YouTubeVideoContentInfo.java(0 hunks)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/YouTubeVideoContentInfo.kt(1 hunks)serenity-app/src/main/java/us/nineworlds/serenity/fragments/MainMenuVideoContentVerticalGridFragment.kt(2 hunks)serenity-app/src/main/java/us/nineworlds/serenity/ui/leanback/search/CardPresenter.kt(1 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/core/menus/MenuItem.kt(0 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/fragments/mainmenu/MainMenuPresenter.kt(1 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsFragment.kt(4 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt(2 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/CategoryVideoPresenter.kt(1 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/EpisodeVideoPresenter.kt(1 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/MoviePresenterViewHolder.kt(2 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/SeriesPresenterViewHolder.kt(2 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/VideoContentInfoPresenter.kt(1 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt(4 hunks)serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepository.kt(1 hunks)serenity-app/src/main/res/layout-large/activity_login_user.xml(1 hunks)serenity-app/src/main/res/layout-large/leanback_details_summary.xml(1 hunks)serenity-app/src/main/res/layout-large/leanback_fullwidth_details_overview.xml(1 hunks)serenity-app/src/main/res/layout-large/status_overlay_view.xml(1 hunks)serenity-app/src/main/res/layout/movie_custom_actionbar.xml(1 hunks)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/imageloader/BackgroundBitmapDisplayerTest.kt(2 hunks)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractMediaContainerTest.kt(1 hunks)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfoTest.kt(1 hunks)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfoTest.kt(1 hunks)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainerTest.kt(1 hunks)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/MovieMediaContainerTest.kt(1 hunks)serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/leanback/search/CardPresenterTest.kt(1 hunks)serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt(2 hunks)
💤 Files with no reviewable changes (30)
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/CategoryInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVShowSeriesInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SecondaryCategoryMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/VideoContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodePosterInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SubtitleMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeriesMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/ArtistContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/SecondaryCategoryInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MusicAlbumContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeasonsMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/CategoryMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/DBMetaData.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MusicArtistContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/TrackContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MovieMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MoviePosterInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/YouTubeVideoContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/Subtitle.java
- serenity-app/src/main/kotlin/us/nineworlds/serenity/core/menus/MenuItem.kt
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/SeriesContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MenuMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVCategoryMediaContainer.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AudioTrackContentInfo.java
- serenity-app/src/main/java/us/nineworlds/serenity/core/imageloader/BackgroundBitmapDisplayer.java
🧰 Additional context used
🧬 Code graph analysis (21)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SecondaryCategoryMediaContainer.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SecondaryCategoryMediaContainer.java (1)
SecondaryCategoryMediaContainer(32-63)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainerTest.kt (2)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractMediaContainerTest.kt (1)
bind(61-65)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfoTest.kt (1)
bind(159-164)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/SeriesContentInfo.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/SeriesContentInfo.java (1)
SeriesContentInfo(32-109)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVShowSeriesInfo.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVShowSeriesInfo.java (1)
TVShowSeriesInfo(28-32)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt (2)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.kt (5)
setId(17-17)setSummary(15-15)setBackgroundURL(16-16)setImageURL(14-14)setTitle(13-13)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MovieMediaContainer.kt (1)
createVideoDetails(66-71)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeasonsMediaContainer.kt (2)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfo.kt (4)
setId(69-71)setBackgroundURL(65-67)setImageURL(57-59)setTitle(53-55)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeasonsMediaContainer.java (1)
SeasonsMediaContainer(34-79)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfoTest.kt (4)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractMediaContainerTest.kt (1)
bind(61-65)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfoTest.kt (1)
bind(159-164)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainerTest.kt (1)
bind(156-161)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/MovieMediaContainerTest.kt (1)
bind(122-126)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/MovieMediaContainerTest.kt (2)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractMediaContainerTest.kt (1)
bind(61-65)serenity-app/src/test/java/us/nineworlds/serenity/core/model/impl/MovieMediaContainerTest.java (1)
RunWith(49-121)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/CategoryMediaContainer.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/CategoryMediaContainer.java (1)
CategoryMediaContainer(32-78)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/VideoContentInfo.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/VideoContentInfo.java (1)
VideoContentInfo(34-205)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfoTest.kt (4)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractMediaContainerTest.kt (1)
bind(61-65)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfoTest.kt (1)
bind(211-215)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainerTest.kt (1)
bind(156-161)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/MovieMediaContainerTest.kt (1)
bind(122-126)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfo.kt (4)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/SeriesContentInfo.kt (1)
totalShows(11-11)serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.kt (2)
id(5-20)id(6-6)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.kt (1)
id(24-24)serenity-app/src/main/java/us/nineworlds/serenity/core/services/UnWatchVideoAsyncTask.java (1)
UnWatchVideoAsyncTask(36-53)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodePosterInfo.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodePosterInfo.java (1)
EpisodePosterInfo(29-36)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeriesMediaContainer.kt (3)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SeasonsMediaContainer.kt (1)
createSeriesInfo(8-27)serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.kt (6)
setId(17-17)setMediaTagIdentifier(19-19)setSummary(15-15)setBackgroundURL(16-16)setImageURL(14-14)setTitle(13-13)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfo.kt (6)
setId(69-71)setMediaTagIdentifier(75-77)setSummary(61-63)setBackgroundURL(65-67)setImageURL(57-59)setTitle(53-55)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractMediaContainerTest.kt (4)
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfoTest.kt (1)
bind(211-215)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfoTest.kt (1)
bind(159-164)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainerTest.kt (1)
bind(156-161)serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/MovieMediaContainerTest.kt (1)
bind(122-126)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.kt (4)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfo.kt (2)
viewedPercentage(115-121)id(25-25)serenity-app/src/main/java/us/nineworlds/serenity/core/model/VideoContentInfo.kt (1)
viewedPercentage(39-39)serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.kt (2)
id(5-20)id(6-6)serenity-app/src/main/java/us/nineworlds/serenity/core/services/UnWatchVideoAsyncTask.java (1)
UnWatchVideoAsyncTask(36-53)
serenity-app/src/main/java/us/nineworlds/serenity/core/imageloader/BackgroundBitmapDisplayer.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/imageloader/BackgroundBitmapDisplayer.java (1)
BackgroundBitmapDisplayer(29-86)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractMediaContainer.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractMediaContainer.java (1)
AbstractMediaContainer(33-47)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MenuMediaContainer.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MenuMediaContainer.java (1)
MenuMediaContainer(42-122)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MovieMediaContainer.kt (3)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt (2)
createVideoContent(14-26)createVideoDetails(85-87)serenity-app/src/main/java/us/nineworlds/serenity/core/model/ContentInfo.kt (7)
setType(8-8)setMediaTagIdentifier(19-19)setId(17-17)setSummary(15-15)setBackgroundURL(16-16)setImageURL(14-14)setTitle(13-13)serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.kt (7)
setType(28-30)setMediaTagIdentifier(62-64)setId(56-58)setSummary(48-50)setBackgroundURL(52-54)setImageURL(44-46)setTitle(40-42)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SubtitleMediaContainer.kt (1)
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SubtitleMediaContainer.java (1)
SubtitleMediaContainer(35-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (35)
serenity-app/src/main/res/layout-large/activity_login_user.xml (1)
2-8: Formatting change approved.The reformatting of the RelativeLayout opening tag to multi-line attribute layout is standard Android convention and improves readability. This aligns with the Spotless plugin version update mentioned in the PR.
serenity-app/src/main/res/layout/movie_custom_actionbar.xml (1)
2-8: Formatting alignment looks good.The xmlns:android attribute has been appropriately moved to its own line for consistency with Android XML conventions. This is a purely cosmetic change with no impact on runtime behavior or the layout structure.
build.gradle (1)
19-19: Verify Spotless 8.0.0 compatibility with existing configuration.The plugin version has been upgraded from 6.18.0 to 8.0.0—a significant jump spanning multiple minor versions. This could introduce breaking changes to the configuration format or behavior. The existing Spotless config at lines 56–62 targets XML layout files and references an external config file (
spotless.xml.prefs).Please confirm:
- The build passes with Spotless 8.0.0 and the current XML formatting configuration (lines 56–62).
- The formatting behavior for layout files has not changed unexpectedly.
- Any deprecated or removed options from the 6.x → 8.0 migration have been addressed.
If the build is already passing in CI/CD, feel free to note that as confirmation.
serenity-app/src/main/res/layout-large/leanback_fullwidth_details_overview.xml (3)
39-49: Clarify intended behavior forlayout_weightwithwrap_contentheight.Line 49 applies
layout_weight="2"to theNonOverlappingFrameLayout, which haslayout_height="wrap_content". In Android,layout_weighttypically requires the height to be0dp(or0px) when used inside a verticalLinearLayout; using it withwrap_contentmay produce unexpected sizing behavior where the weight is ignored or causes layout issues.Is this weight intended to give the description area 2/3 of available space? If so, consider changing
layout_heightto0dpand adjusting other layout_height values accordingly. Otherwise, removelayout_weight="2"if the description should size naturally to its content.
28-30: Verify whethertools:ignore="UnusedAttribute"is hiding legitimate warnings.Line 30 silences warnings for the
android:elevationattribute ondetails_frame. Ensure this attribute is intentional and needed for the visual design (e.g., shadow/depth effect). If it is, the ignore can remain; if not, remove both the attribute and the ignore directive.
8-77: Layout restructuring appears sound for reorganizing details view.The introduction of nested FrameLayouts to support elevation/z-ordering and the LinearLayout container for vertical content organization is a reasonable refactor. The flattening of the actions area (consolidating
HorizontalGridViewdirectly into the background container) reduces nesting and should improve layout performance.serenity-app/src/main/res/layout-large/leanback_details_summary.xml (1)
1-125: Layout restructuring is well-organized and constraint-compliant (apart from poster issue above).The introduction of explicit vertical guidelines (5%, 95%, 25%) provides a clean, maintainable foundation for aligning the poster, title, summary, and info sections. The constraint chain from title → summary → extra text → info graphic is properly constructed with wrap_content and 0dp heights used appropriately. Once the poster's bottom constraint is resolved, this layout should render correctly and scale responsively across large-screen form factors.
serenity-app/src/main/res/layout-large/status_overlay_view.xml (1)
12-21: Verify posterImageView sizing constraints.The
posterImageViewhasandroid:layout_width="wrap_content"butandroid:layout_height="match_parent". Combined with the ConstraintLayout constraints (Top and Start), ensure this sizing produces the intended layout. The tools preview dimensions are 200x250dp; verify the runtime behavior matches design expectations.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsMVPPresenter.kt (2)
54-54: No issues found with this change.The use of
item.getType()is not a transitional migration pattern—it's the only available method. TheVideoContentInfointerface and itsAbstractVideoContentInfoimplementation define only thegetType(): Types?method; no.typeproperty exists on these types. The mixed usage patterns of.typefound elsewhere in the codebase are on different, unrelated types. The change at line 54 is correct as written.Likely an incorrect or invalid review comment.
73-73: Address null-safety inconsistency with season keys.The
season.key.orEmpty()call indicates a null-safety gap in the data model. While the defensive check is reasonable, a better approach depends on whether season keys should ever be null:
- If keys are always present from the API (current code in
CategoryMediaContainerassumes this by comparingdir.keydirectly), then change the Kotlin property to non-nullable and validate inSeasonsMediaContainer- If null keys are legitimate, filter out seasons without keys earlier instead of passing empty strings to
fetchEpisodesThe current approach risks silent failures when the API returns an empty-key result rather than failing fast with a clear error.
serenity-app/src/main/java/us/nineworlds/serenity/fragments/MainMenuVideoContentVerticalGridFragment.kt (2)
44-44: LGTM! Consistent with getter-based API migration.The change from direct property access to
getType()aligns with the broader Java-to-Kotlin migration pattern established across this PR.
72-72: LGTM! Background URL retrieval updated correctly.The change to
getBackgroundURL()is consistent with the API migration and maintains the existing image loading behavior.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/VideoContentInfoPresenter.kt (1)
45-46: LGTM! Getter-based API usage is consistent.The changes to use
getImageURL()andgetType()align with the broader migration to getter-based access patterns across the codebase.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/CategoryVideoPresenter.kt (1)
46-47: LGTM! Consistent getter-based access.The changes to use
getImageURL()andgetType()are consistent with the API migration pattern applied across other presenter classes in this PR.serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/TVShowSeriesInfo.kt (1)
1-9: LGTM! Clean Java-to-Kotlin conversion.The conversion is straightforward and correct. The
serialVersionUIDin the companion object follows the proper Kotlin pattern forSerializableclasses.serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepositoryTest.kt (1)
66-66: LGTM! Test correctly updated for property-based API.The mocks correctly reflect the API change from
isWatched()method toisWatchedproperty, aligning with Kotlin property conventions.Also applies to: 80-80
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/MoviePosterInfo.kt (1)
1-9: LGTM! Standard Java-to-Kotlin conversion.The class structure is correct, properly extending
AbstractVideoContentInfoand implementingSerializablewith the appropriateserialVersionUID.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/EpisodeVideoPresenter.kt (1)
33-39: LGTM! Consistent use of getter methods.The changes correctly use
getImageURL()andgetTitle()accessor methods instead of direct property access, aligning with the updated API surface across the UI layer.serenity-app/src/test/kotlin/us/nineworlds/serenity/ui/leanback/search/CardPresenterTest.kt (1)
66-66: LGTM! Test correctly updated for getter-based API.Both the mock setup and verification correctly use
getImageURL()instead of direct property access, maintaining consistency with the API changes.Also applies to: 72-72
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/YouTubeVideoContentInfo.kt (1)
1-9: LGTM! Clean conversion.Standard Java-to-Kotlin conversion following the correct pattern for
Serializableclasses withserialVersionUIDin the companion object.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/activity/leanback/details/DetailsFragment.kt (1)
200-200: LGTM! Consistent use of accessor methods.The changes correctly adopt getter methods (
getBackgroundURL(),getTitle()) instead of direct property access, maintaining consistency with the updated API surface. The equality check on line 238 properly uses Kotlin's==operator, which delegates toequals()for content comparison in theDiffCallback.Also applies to: 214-214, 226-226, 238-238
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/SeriesPresenterViewHolder.kt (1)
21-22: LGTM!The migration from direct property access to explicit getter methods (
getTitle(),getSummary(),getMediaTagIdentifier()) is consistent with the broader Java-to-Kotlin conversion pattern across the PR. The logic remains unchanged and the accessor usage is correct.Also applies to: 70-70
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SubtitleMediaContainer.kt (1)
7-25: LGTM!The Kotlin migration maintains the original logic while adding improved null-safety. The chained safe-call navigation at line 8 handles missing data gracefully, and the guard at line 19 ensures only valid subtitles with non-null keys are added to the result.
serenity-app/src/main/java/us/nineworlds/serenity/ui/leanback/search/CardPresenter.kt (1)
64-73: LGTM!The migration to getter methods combined with improved null-safety is well-executed. Wrapping the binding logic in
getImageURL()?.letensures the card is only populated when an image URL exists, and the safe call onmainImageViewat line 103 prevents potential NPEs.Also applies to: 102-106
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/ExoplayerPresenter.kt (1)
71-73: LGTM!The null-safety improvements throughout this file are well-implemented. Using
?.letblocks and.orEmpty()consistently handles nullable video IDs and properties, preventing potential NPEs while maintaining the original logic. The changes align with Kotlin best practices.Also applies to: 94-94, 98-98, 104-104, 162-168, 175-175, 199-199, 202-202
serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractMediaContainerTest.kt (1)
18-66: LGTM! Well-structured test suite.The test coverage is appropriate for a base abstract class, verifying initialization, constructor assignment, and DI injection. The test structure follows established patterns from other test files in the codebase.
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodePosterInfo.kt (1)
1-10: LGTM! Clean Java-to-Kotlin conversion.The conversion is straightforward and correctly places
serialVersionUIDin the companion object. The nullableResources?parameter maintains defensive coding practices.serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/leanback/presenters/MoviePresenterViewHolder.kt (2)
22-25: LGTM! Consistent API access pattern.The switch from direct property access to getter methods aligns with the Java-to-Kotlin migration strategy, ensuring consistent access patterns across the codebase.
102-102: LGTM! Consistent getter usage.Correctly updated to use
getMediaTagIdentifier()to match the pattern established elsewhere in this file.serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/CategoryMediaContainer.kt (2)
9-13: Note: @JvmField annotations for Java interop.The
@JvmFieldannotations on protected fields suggest these are accessed from Java code. This is appropriate for the migration phase but consider removing once all callers are converted to Kotlin.
27-43: LGTM! Good null safety and idiomatic Kotlin.The null-safe access to
mc.directorieswith early return is appropriate, and the use ofapplyfor object initialization is idiomatic Kotlin.serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/SecondaryCategoryMediaContainer.kt (1)
6-31: LGTM! Clean Kotlin conversion with good encapsulation.The conversion follows Kotlin best practices with null-safe access, proper use of scope functions, and better encapsulation (private
categorieslist). The nullableparentCategoryKeyparameter is handled appropriately.serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/EpisodeMediaContainer.kt (2)
63-78: LGTM! Robust media metadata extraction with safe fallbacks.The null-safe navigation and use of
toIntOrNull()with fallback values (line 71-72) provides defensive handling of potentially malformed data.
51-55: No issues found with background URL construction.The URL fields (
backgroundImageKey,mc.art,thumbNailImageKey, etc.) are consistently populated with a leading "/" prefix from the data adaptor (e.g., "/emby/Items/${id}/Images/Backdrop"). ThereplaceFirst("/", "")calls are safe—they remove the "/" if present or perform a no-op if absent, ensuring correct URL concatenation withbaseUrl.The current implementation is functionally correct. While
removePrefix("/")would be slightly more semantic, it provides no practical advantage overreplaceFirst()given the guaranteed "/" prefix from the data source. The code does not require changes.serenity-app/src/test/kotlin/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfoTest.kt (1)
26-165: LGTM! Comprehensive test coverage.Excellent test suite covering:
- Title formatting variations
- Watch state logic (partially watched, watched, unwatched)
- Edge cases (duration = 0)
- Watch status toggle behavior with proper async task verification
The test structure with companion object for constructor mocking and proper setup/teardown is well-organized.
| Do NOT try to call `scope.inject` directly in the test. That is handled by `InjectingTest`. | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove duplicate line.
Lines 180 and 182 contain identical text. This appears to be an unintentional duplication.
Apply this diff to remove the duplicate:
Do NOT try to call `scope.inject` directly in the test. That is handled by `InjectingTest`.
-
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Do NOT try to call `scope.inject` directly in the test. That is handled by `InjectingTest`. | |
| Do NOT try to call `scope.inject` directly in the test. That is handled by `InjectingTest`. |
🤖 Prompt for AI Agents
In AGENTS.md around lines 180 to 182, there is a duplicated sentence "Do NOT try
to call `scope.inject` directly in the test. That is handled by
`InjectingTest`."; remove the redundant line so the sentence appears only once,
keeping spacing and surrounding context intact.
| abstract class AbstractMediaContainer( | ||
| @JvmField | ||
| protected var mc: IMediaContainer, | ||
| ) : BaseInjector() { | ||
|
|
||
| @JvmField | ||
| var videoList: MutableList<in ContentInfo>? = null | ||
|
|
||
| @Inject | ||
| lateinit var factory: SerenityClient |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the type and visibility changes for compatibility.
The conversion introduces several significant changes that could impact existing code:
-
Type change on
videoList(line 16):- Old:
List<VideoContentInfo> - New:
MutableList<in ContentInfo>?
The contravariant type parameter
in ContentInfochanges semantics: while you can addContentInfosubtypes, reading from the list returns the upper bound (Any?), notVideoContentInfo. If consuming code casts items from this list toVideoContentInfo, this will still compile but is less type-safe. - Old:
-
Visibility changes: All fields (
mc,videoList,factory) changed fromprotectedtopublicvia@JvmFieldand default Kotlin visibility. This expands the API surface.
These changes may be intentional for supporting multiple content types across different container implementations, but please verify that existing usages (especially reading from videoList) are compatible with the new type.
Run the following script to check how videoList is used across the codebase:
🏁 Script executed:
#!/bin/bash
# Find all usages of videoList to verify compatibility with new type
# Search for videoList field access and method calls
rg -n -C3 --type=kotlin --type=java 'videoList\.'
# Search for iteration over videoList
rg -n -C3 --type=kotlin --type=java 'for.*videoList'
# Search for videoList assignments and casts
rg -n -C3 --type=kotlin --type=java 'videoList\s*=' Length of output: 2513
I need to gather more information about the type hierarchy to fully assess the compatibility. Let me search for the related type definitions.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find ContentInfo definition and hierarchy
rg -n --type=kotlin --type=java '(interface|class)\s+ContentInfo' -A 3
# Find VideoContentInfo definition
rg -n --type=kotlin --type=java '(interface|class)\s+VideoContentInfo' -A 3
# Find SeriesContentInfo definition
rg -n --type=kotlin --type=java '(interface|class)\s+SeriesContentInfo' -A 3
# Find IVideo definition (likely in emby-lib)
rg -n --type=kotlin --type=java '(interface|class)\s+IVideo' -A 3
Length of output: 2463
Let me verify the class hierarchy for MediaContainer to confirm the scope of the type incompatibility.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if MediaContainer extends AbstractMediaContainer
rg -n --type=java 'class\s+MediaContainer' -A 2
# Check the full context of the setVideos method
rg -n --type=java -B 5 -A 3 'setVideos.*List<IVideo>'
Length of output: 2253
Verify contravariance design and unsafe casts are intentional.
The type change from List<VideoContentInfo> to MutableList<in ContentInfo>? introduces genuine type safety issues:
-
Unsafe casts confirmed at consumption sites:
MovieMediaContainer.kt:13:return videoList as List<VideoContentInfo>SeriesMediaContainer.kt:12:return videoList as List<SeriesContentInfo>
-
Contravariance breaks type safety:
MutableList<in ContentInfo>reads returnAny?, not the concrete type. Code must cast unsafely to recover the original type. -
Nullability risk: The
?makesvideoListnullable, but existing code initializes it asmutableListOf()(non-null). Runtime null-checks will be needed.
If the goal is supporting multiple content types in a shared container, the contravariance approach requires unsafe casts everywhere. Consider instead: keeping the original generic List<VideoContentInfo> or redesigning with proper polymorphic handling that avoids casts.
| val isUnwatched: Boolean | ||
| get() { | ||
| val unwatched = showsUnwatched!!.toInt() | ||
| return unwatched > 0 | ||
| } | ||
|
|
||
| val isWatched: Boolean | ||
| get() { | ||
| val watchedCount = showsWatched!!.toInt() | ||
| return totalShows() == watchedCount | ||
| } | ||
|
|
||
| override fun totalShows(): Int { | ||
| val unwatched = showsUnwatched!!.toInt() | ||
| val watched = showsWatched!!.toInt() | ||
| return unwatched + watched | ||
| } | ||
|
|
||
| fun viewedPercentage(): Float { | ||
| if (totalShows() == 0) { | ||
| return 0f | ||
| } | ||
| val watched = showsWatched!!.toFloat() | ||
| return watched / totalShows() | ||
| } |
There was a problem hiding this comment.
Guard nullable watch counters
showsWatched / showsUnwatched default to null, yet the getters dereference them with !!. Any code path that inspects watch state before the server populates these fields will now crash. Please fall back to zero when the counts are missing.
- val isUnwatched: Boolean
- get() {
- val unwatched = showsUnwatched!!.toInt()
- return unwatched > 0
- }
+ val isUnwatched: Boolean
+ get() {
+ val unwatched = showsUnwatched?.toIntOrNull() ?: 0
+ return unwatched > 0
+ }
val isWatched: Boolean
get() {
- val watchedCount = showsWatched!!.toInt()
+ val watchedCount = showsWatched?.toIntOrNull() ?: 0
return totalShows() == watchedCount
}
override fun totalShows(): Int {
- val unwatched = showsUnwatched!!.toInt()
- val watched = showsWatched!!.toInt()
+ val unwatched = showsUnwatched?.toIntOrNull() ?: 0
+ val watched = showsWatched?.toIntOrNull() ?: 0
return unwatched + watched
}
fun viewedPercentage(): Float {
if (totalShows() == 0) {
return 0f
}
- val watched = showsWatched!!.toFloat()
- return watched / totalShows()
+ val watched = showsWatched?.toFloat() ?: 0f
+ return watched / totalShows()
}🤖 Prompt for AI Agents
In
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractSeriesContentInfo.kt
around lines 97 to 121, the getters and methods dereference nullable
showsWatched/showsUnwatched with !! which can throw NPEs; change them to treat
null as zero (use safe calls and elvis operator or null-coalescing), e.g.
convert showsUnwatched?.toInt() ?: 0 and showsWatched?.toInt() ?: 0 in
isUnwatched, isWatched, totalShows and viewedPercentage, and ensure
viewedPercentage uses the integer total converted to float to avoid integer
division and still guard against total zero.
| override val season: String? | ||
| get() = resources?.getString(R.string.season_) + seasonNumber | ||
| override var seasonNumber: Int = 0 | ||
| override val episode: String? | ||
| get() = resources?.getString(R.string.episode_) + episodeNumber | ||
| override var episodeNumber: Int = 0 |
There was a problem hiding this comment.
Avoid “null0” season/episode labels when resources are absent
resources is optional, yet these getters always concatenate its (possibly null) string with the number, yielding "null0" when the caller supplies null. Please gate the concatenation on a non-null resource string.
- override val season: String?
- get() = resources?.getString(R.string.season_) + seasonNumber
+ override val season: String?
+ get() = resources?.getString(R.string.season_)?.let { "$it$seasonNumber" }
override var seasonNumber: Int = 0
override val episode: String?
- get() = resources?.getString(R.string.episode_) + episodeNumber
+ get() = resources?.getString(R.string.episode_)?.let { "$it$episodeNumber" }🤖 Prompt for AI Agents
In
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/AbstractVideoContentInfo.kt
around lines 92–97, the season/episode getters concatenate a nullable resources
string with the number, producing "null0" when resources is absent; change the
getters to only concatenate when the resource string is non-null (e.g. use
resources?.getString(...)?.let { it + seasonNumber } ?: seasonNumber.toString()
for season, and similarly for episode) so you avoid "null0" while still
returning a sensible fallback when resources are null.
| fun createCatagoriesFilteringAlbums(): List<CategoryInfo> { | ||
| filterAlbums = true | ||
| populateCategories() | ||
| return categories | ||
| } |
There was a problem hiding this comment.
Fix typo in method name.
The method name has a typo: createCatagoriesFilteringAlbums should be createCategoriesFilteringAlbums for consistency with createCategories().
Apply this diff:
- fun createCatagoriesFilteringAlbums(): List<CategoryInfo> {
+ fun createCategoriesFilteringAlbums(): List<CategoryInfo> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun createCatagoriesFilteringAlbums(): List<CategoryInfo> { | |
| filterAlbums = true | |
| populateCategories() | |
| return categories | |
| } | |
| fun createCategoriesFilteringAlbums(): List<CategoryInfo> { | |
| filterAlbums = true | |
| populateCategories() | |
| return categories | |
| } |
🤖 Prompt for AI Agents
In
serenity-app/src/main/java/us/nineworlds/serenity/core/model/impl/CategoryMediaContainer.kt
around lines 21-25, the method name createCatagoriesFilteringAlbums contains a
typo and should be renamed to createCategoriesFilteringAlbums; rename the method
declaration to the correct spelling, update every reference/call site across the
codebase (including tests and any interfaces/overrides) to the new name, run a
build to catch usages, and ensure visibility/signature remains identical so
behavior is unchanged.
| } | ||
| filteredCategories.forEach { category -> | ||
| when (val result = repository.fetchItemsByCategory(category.category, itemId, type)) { | ||
| when (val result = repository.fetchItemsByCategory(category.category.orEmpty(), itemId, type)) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify empty string handling in repository method.
Using .orEmpty() ensures a non-null string is passed to fetchItemsByCategory, but an empty string may not be a valid category identifier. Consider whether categories with null names should be skipped entirely.
Run the following script to check how fetchItemsByCategory handles empty strings:
🏁 Script executed:
#!/bin/bash
# Description: Check fetchItemsByCategory implementation to verify empty string handling
# Find the CategoryRepository interface and implementation
ast-grep --pattern 'fun fetchItemsByCategory($$$)'Length of output: 0
I'll search the codebase for the repository method implementation using broader search techniques.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for fetchItemsByCategory method definition across the codebase
rg -n "fetchItemsByCategory" --type=kotlin -A 5 -B 2
Length of output: 2359
🏁 Script executed:
#!/bin/bash
# Search for CategoryRepository interface/class definition
rg -n "class CategoryRepository|interface CategoryRepository" --type=kotlin -A 10Length of output: 1418
🏁 Script executed:
#!/bin/bash
# Check MainMenuPresenter context around line 73 for more details on category handling
fd "MainMenuPresenter.kt" --type=f --exec cat {} \;Length of output: 3680
🏁 Script executed:
#!/bin/bash
# Search for category.category pattern to understand the data structure
rg -n "category\.category" --type=kotlin -B 3 -A 3Length of output: 6049
🏁 Script executed:
#!/bin/bash
# Find all category-related null checks or validations in the codebase
rg -n "category.*null|category.*empty" --type=kotlin -C 2Length of output: 1869
I need to see the complete implementation of fetchItemsByCategory to verify how it handles empty strings.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full CategoryRepository.kt file to see complete fetchItemsByCategory implementation
cat serenity-app/src/main/kotlin/us/nineworlds/serenity/core/repository/CategoryRepository.kt
Length of output: 2718
🏁 Script executed:
#!/bin/bash
# Search for how the repository uses categoryId parameter in the method
rg -n "fetchItemsByCategory" --type=kotlin -A 20Length of output: 5335
Skip null categories instead of converting to empty string.
The concern is valid. Categories with null category field are being passed as empty strings to fetchItemsByCategory, which likely isn't a valid category identifier. The repository method doesn't validate this and passes the empty string directly downstream to the client.
Instead of using .orEmpty(), filter out null categories before the iteration:
val filteredCategories = categories.filter { category -> category.category != null && category.category != "unwatched" }Then remove .orEmpty() from line 73 and access category.category!! (safe since you've already filtered nulls).
🤖 Prompt for AI Agents
In
serenity-app/src/main/kotlin/us/nineworlds/serenity/fragments/mainmenu/MainMenuPresenter.kt
around line 73, categories with a null category field are being converted to
empty strings via .orEmpty() and passed to fetchItemsByCategory; instead, filter
out null (and "unwatched") categories before iterating (e.g., create
filteredCategories = categories.filter { it.category != null && it.category !=
"unwatched" }) and then remove .orEmpty() on line 73 and call
fetchItemsByCategory using category.category!! (safe after filtering) so only
valid non-null category identifiers are passed downstream.
| val videoId: String = video.id().orEmpty() | ||
| if (video.isWatched) { | ||
| serenityClient.watched(videoId) | ||
| serenityClient.progress(videoId, "0") | ||
| } else { | ||
| serenityClient.progress(videoId, video.getResumeOffset().toString()) | ||
| serenityClient.progress(videoId, video.resumeOffset.toString()) |
There was a problem hiding this comment.
Inconsistent API access pattern.
The code mixes method calls (video.id()) with property access (video.isWatched, video.resumeOffset). This inconsistency can be confusing for maintainers and suggests an incomplete migration.
For consistency, consider using a uniform approach throughout the codebase. Either:
- Use getter methods for all:
getId(),getIsWatched(),getResumeOffset() - Use Kotlin properties for all:
id,isWatched,resumeOffset
Since this is a Kotlin migration, Kotlin properties are generally preferred for simple accessors.
🤖 Prompt for AI Agents
In
serenity-app/src/main/kotlin/us/nineworlds/serenity/ui/video/player/PlaybackRepository.kt
around lines 21 to 26, the code mixes Java-style getter calls (video.id()) with
Kotlin property access (video.isWatched, video.resumeOffset); update the
accessors to be consistent Kotlin properties: replace video.id() with video.id,
and ensure any other usages in this block use the property form
(video.isWatched, video.resumeOffset) so the code uses idiomatic Kotlin property
access throughout.
| <us.nineworlds.serenity.widgets.RoundedImageView | ||
| android:id="@+id/video_poster" | ||
| android:layout_width="0dp" | ||
| android:layout_height="0dp" | ||
| app:layout_constraintStart_toEndOf="@+id/guideline_vertical_details_start" | ||
| app:layout_constraintEnd_toStartOf="@+id/guideline_details_25" | ||
| app:layout_constraintTop_toTopOf="parent" | ||
| app:layout_constraintDimensionRatio="H,2:3" | ||
| android:layout_margin="5dp" | ||
| android:minHeight="@dimen/movie_poster_image_height" | ||
| app:corner_radius="5dp" | ||
| app:mutate_background="true" /> |
There was a problem hiding this comment.
Poster image missing bottom constraint for 0dp height.
The video_poster ImageView declares android:layout_height="0dp" (line 43), which requires both top and bottom constraints to properly determine its size in combination with the aspect ratio. Currently it has app:layout_constraintTop_toTopOf="parent" (line 46) but is missing a corresponding bottom constraint. While the 2:3 aspect ratio may provide sizing, this incomplete constraint configuration is a fragile pattern and may cause unexpected layout behavior or preview issues in the editor.
Add a bottom constraint, such as:
<us.nineworlds.serenity.widgets.RoundedImageView
android:id="@+id/video_poster"
android:layout_width="0dp"
android:layout_height="0dp"
app:layout_constraintStart_toEndOf="@+id/guideline_vertical_details_start"
app:layout_constraintEnd_toStartOf="@+id/guideline_details_25"
app:layout_constraintTop_toTopOf="parent"
+ app:layout_constraintBottom_toBottomOf="@+id/guideline_details_25"
app:layout_constraintDimensionRatio="H,2:3"
android:layout_margin="5dp"
android:minHeight="@dimen/movie_poster_image_height"
app:corner_radius="5dp"
app:mutate_background="true" />(Adjust the bottom constraint target as needed based on your actual layout intent—this assumes constraining to the bottom of the first guideline.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In serenity-app/src/main/res/layout-large/leanback_details_summary.xml around
lines 40 to 51 the RoundedImageView with id video_poster declares
android:layout_height="0dp" but only has a top constraint; add a bottom
constraint so the 0dp height can be resolved (e.g. add
app:layout_constraintBottom_toBottomOf="parent" or constrain to the appropriate
guideline such as
app:layout_constraintBottom_toTopOf="@+id/guideline_details_25" depending on the
intended visual layout) to ensure stable sizing with the 2:3 aspect ratio.
| <TextView | ||
| android:id="@+id/metaOverlay" | ||
| style="@android:style/TextAppearance.Holo.Small" | ||
| android:layout_width="fill_parent" | ||
| android:layout_height="wrap_content" | ||
| app:layout_constraintTop_toBottomOf="@+id/posterOverlayTitle" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| android:background="#80000000" | ||
| android:ellipsize="end" | ||
| android:maxLines="1" | ||
| android:textColor="#F0F0F0" | ||
| android:textStyle="bold" | ||
| android:visibility="invisible" /> |
There was a problem hiding this comment.
Replace deprecated fill_parent with match_parent.
Line 41 uses android:layout_width="fill_parent", which is deprecated since API level 8. Use match_parent instead.
<TextView
android:id="@+id/metaOverlay"
style="@android:style/TextAppearance.Holo.Small"
- android:layout_width="fill_parent"
+ android:layout_width="0dp"
android:layout_height="wrap_content"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <TextView | |
| android:id="@+id/metaOverlay" | |
| style="@android:style/TextAppearance.Holo.Small" | |
| android:layout_width="fill_parent" | |
| android:layout_height="wrap_content" | |
| app:layout_constraintTop_toBottomOf="@+id/posterOverlayTitle" | |
| app:layout_constraintStart_toStartOf="parent" | |
| android:background="#80000000" | |
| android:ellipsize="end" | |
| android:maxLines="1" | |
| android:textColor="#F0F0F0" | |
| android:textStyle="bold" | |
| android:visibility="invisible" /> | |
| <TextView | |
| android:id="@+id/metaOverlay" | |
| style="@android:style/TextAppearance.Holo.Small" | |
| android:layout_width="0dp" | |
| android:layout_height="wrap_content" | |
| app:layout_constraintTop_toBottomOf="@+id/posterOverlayTitle" | |
| app:layout_constraintStart_toStartOf="parent" | |
| app:layout_constraintEnd_toEndOf="parent" | |
| android:background="#80000000" | |
| android:ellipsize="end" | |
| android:maxLines="1" | |
| android:textColor="#F0F0F0" | |
| android:textStyle="bold" | |
| android:visibility="invisible" /> |
🤖 Prompt for AI Agents
In serenity-app/src/main/res/layout-large/status_overlay_view.xml around lines
38 to 50, the TextView uses the deprecated attribute
android:layout_width="fill_parent"; replace it with
android:layout_width="match_parent" to conform to current Android APIs and avoid
deprecation warnings, leaving all other attributes unchanged.
| <LinearLayout | ||
| android:id="@+id/infoGraphicMeta" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| app:layout_constraintBottom_toTopOf="@id/posterInprogressIndicator" | ||
| android:layout_above="@+id/posterInprogressIndicator" | ||
| android:layout_marginEnd="10dp" | ||
| android:background="@color/fity_percent_transparent" | ||
| android:visibility="gone" | ||
| tools:visibility="visible" | ||
| > | ||
|
|
||
| <ImageView | ||
| android:id="@+id/posterWatchedIndicator" | ||
| android:layout_width="33dp" | ||
| android:layout_height="33dp" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| android:scaleType="fitXY" /> | ||
| android:id="@+id/subtitleIndicator" | ||
| android:layout_width="20dp" | ||
| android:layout_height="20dp" | ||
| android:scaleType="fitXY" | ||
| android:src="@drawable/subtitles" | ||
| android:visibility="visible" /> | ||
|
|
||
| <LinearLayout | ||
| android:id="@+id/infoGraphicMeta" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| app:layout_constraintBottom_toTopOf="@id/posterInprogressIndicator" | ||
| android:layout_above="@+id/posterInprogressIndicator" | ||
| android:layout_marginEnd="10dp" | ||
| android:background="@color/fity_percent_transparent" | ||
| android:visibility="gone" | ||
| tools:visibility="visible" | ||
| > | ||
|
|
||
| <ImageView | ||
| android:id="@+id/subtitleIndicator" | ||
| android:layout_width="20dp" | ||
| android:layout_height="20dp" | ||
| android:scaleType="fitXY" | ||
| android:src="@drawable/subtitles" | ||
| android:visibility="visible" /> | ||
|
|
||
| <ImageView | ||
| android:id="@+id/trailerIndicator" | ||
| android:layout_width="20dp" | ||
| android:layout_height="20dp" | ||
| android:scaleType="fitXY" | ||
| android:src="@drawable/yt_social_icon_red_128px" | ||
| android:visibility="visible" /> | ||
| <ImageView | ||
| android:id="@+id/trailerIndicator" | ||
| android:layout_width="20dp" | ||
| android:layout_height="20dp" | ||
| android:scaleType="fitXY" | ||
| android:src="@drawable/yt_social_icon_red_128px" | ||
| android:visibility="visible" /> | ||
|
|
||
| </LinearLayout> | ||
| </LinearLayout> |
There was a problem hiding this comment.
🧩 Analysis chain
Remove RelativeLayout attribute from ConstraintLayout child.
The infoGraphicMeta LinearLayout mixes ConstraintLayout and RelativeLayout attribute systems. Line 67 contains android:layout_above="@+id/posterInprogressIndicator", which is a RelativeLayout attribute and should not be used within a ConstraintLayout hierarchy.
Remove the deprecated attribute:
<LinearLayout
android:id="@+id/infoGraphicMeta"
android:layout_width="0dp"
android:layout_height="wrap_content"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintBottom_toTopOf="@id/posterInprogressIndicator"
- android:layout_above="@+id/posterInprogressIndicator"
android:layout_marginEnd="10dp"
android:background="@color/fity_percent_transparent"
android:visibility="gone"
tools:visibility="visible"
>
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate the XML file
fd "status_overlay_view.xml" --type f
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Display the full content of the layout file to verify context
cat serenity-app/src/main/res/layout-large/status_overlay_view.xmlLength of output: 3893
🏁 Script executed:
#!/bin/bash
# Search for any RelativeLayout attributes in ConstraintLayout children
rg "android:layout_above|android:layout_below|android:layout_toStartOf|android:layout_toEndOf" serenity-app/src/main/res/layout-large/ -C5Length of output: 17340
🏁 Script executed:
#!/bin/bash
# Check if there's a corresponding file in the base layout directory
fd "status_overlay_view.xml" serenity-app/src/main/res/layout/ --type fLength of output: 0
Remove RelativeLayout attribute from ConstraintLayout child.
The infoGraphicMeta LinearLayout contains an incompatible RelativeLayout attribute that conflicts with the ConstraintLayout parent. Line 67 has android:layout_above="@+id/posterInprogressIndicator", which is not recognized by ConstraintLayout. The positioning is already correctly handled by app:layout_constraintBottom_toTopOf on line 66, so the RelativeLayout attribute should be removed:
<LinearLayout
android:id="@+id/infoGraphicMeta"
android:layout_width="0dp"
android:layout_height="wrap_content"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintBottom_toTopOf="@id/posterInprogressIndicator"
- android:layout_above="@+id/posterInprogressIndicator"
android:layout_marginEnd="10dp"
android:background="@color/fity_percent_transparent"
android:visibility="gone"
tools:visibility="visible"
>🤖 Prompt for AI Agents
In serenity-app/src/main/res/layout-large/status_overlay_view.xml around lines
60 to 90, the LinearLayout with id infoGraphicMeta contains a RelativeLayout
attribute android:layout_above="@+id/posterInprogressIndicator" which conflicts
with its ConstraintLayout parent and is redundant because
app:layout_constraintBottom_toTopOf already defines the positioning; remove the
android:layout_above attribute from the LinearLayout so only ConstraintLayout
constraints are used.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #476 +/- ##
==============================================
+ Coverage 0 47.12% +47.12%
- Complexity 0 446 +446
==============================================
Files 0 59 +59
Lines 0 1895 +1895
Branches 0 259 +259
==============================================
+ Hits 0 893 +893
- Misses 0 906 +906
- Partials 0 96 +96
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Set of commits that converts the code from Java to Kotlin. Adding unit tests, and correct access across the code base after the conversion.
Summary by CodeRabbit
Tests
Documentation
Chores