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.

Sign up for GitHub

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

Rewrite BaseItemInfoRow with Compose #3379

Merged

nielsvanvelzen merged 2 commits into jellyfin:master from nielsvanvelzen:compose-base-item-info-row

Feb 20, 2024

Member

nielsvanvelzen commented Feb 17, 2024

This is an old branch of mine that I started about a year ago but never finished... I've finally completed it now lol.

The info row is what is shown below the title while browsing, almost all screens use it and thus it is a bit messy. This pull request aims to rewrite it in Kotlin with Compose, not necessarily to clean it up.

The info row items shown are mostly the same. There are some tweaks to the design, improvements to datetime formatting and in some cases some items are removed/reordered or added.

This also adds a lot of proper plurals to the strings, so stuff like "1 episodes" should not happen in this row anymore.

Changes

Replace most of InfoLayoutHelper with new BaseItemInfoRow

Before

After

Issues

nielsvanvelzen added the enhancement label

nielsvanvelzen added this to the v0.17.0 milestone

github-advanced-security bot found potential problems

View reviewed changes

app/src/main/res/values/strings.xml

+ <item quantity="one">%1$s item</item>
+ <item quantity="other">%1$s items</item>
+ </plurals>
+ <plurals name="movies">

Check warning

Code scanning / Android Lint

Unused resources Warning

The resource R.plurals.movies appears to be unused

app/src/main/res/values/strings.xml

+ <item quantity="one">%1$s movie</item>
+ <item quantity="other">%1$s movies</item>
+ </plurals>
+ <plurals name="tv_series">

Check warning

Code scanning / Android Lint

Unused resources Warning

The resource R.plurals.tv_series appears to be unused

github-advanced-security bot found potential problems

View reviewed changes

github-advanced-security bot left a comment

detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.


Rewrite BaseItemInfoRow with Compose

366b188

nielsvanvelzen force-pushed the compose-base-item-info-row branch from d0ff7ca to 366b188 Compare

February 17, 2024 18:53

nielsvanvelzen marked this pull request as ready for review

February 17, 2024 18:56

github-advanced-security bot found potential problems

View reviewed changes

app/src/main/java/org/jellyfin/androidtv/util/InfoLayoutHelper.java


		public class InfoLayoutHelper {
		public static void addInfoRow(Context context, BaseItemDto item, LinearLayout layout, boolean includeRuntime) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/util/InfoLayoutHelper.java


		public class InfoLayoutHelper {
		public static void addInfoRow(Context context, BaseItemDto item, LinearLayout layout, boolean includeRuntime) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

app/src/main/java/org/jellyfin/androidtv/util/InfoLayoutHelper.java


		public class InfoLayoutHelper {
		public static void addInfoRow(Context context, BaseItemDto item, LinearLayout layout, boolean includeRuntime) {

Check notice

Code scanning / Android Lint

Unknown nullness Note

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations

github-advanced-security bot found potential problems

View reviewed changes

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/BaseItemInfoRow.kt

+}
+@Composable
+fun InfoRowMediaDetails(item: BaseItemDto) {

Check warning

Code scanning / detekt

One method should have one responsibility. Long methods tend to handle many things at once. Prefer smaller methods to make them easier to understand. Warning

The function InfoRowMediaDetails is too long (67). The maximum length is 60.

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/BaseItemInfoRow.kt

+}
+@Composable
+fun BaseItemInfoRow(

Check warning

Code scanning / detekt

One method should have one responsibility. Long methods tend to handle many things at once. Prefer smaller methods to make them easier to understand. Warning

The function BaseItemInfoRow is too long (130). The maximum length is 60.

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/BaseItemInfoRow.kt

+}
+@Composable
+fun InfoRowMediaDetails(item: BaseItemDto) {

Check warning

Code scanning / detekt

Prefer splitting up complex methods into smaller, easier to test methods. Warning

The function InfoRowMediaDetails appears to be too complex based on Cyclomatic Complexity (complexity: 21). Defined complexity threshold for methods is set to '15'

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/BaseItemInfoRow.kt

+}
+@Composable
+fun BaseItemInfoRow(

Check warning

Code scanning / detekt

Prefer splitting up complex methods into smaller, easier to test methods. Warning

The function BaseItemInfoRow appears to be too complex based on Cyclomatic Complexity (complexity: 55). Defined complexity threshold for methods is set to '15'

app/src/main/java/org/jellyfin/androidtv/ui/composable/strings.kt

+@Composable
+@Suppress("MagicNumber")
+fun getResolutionName(width: Int, height: Int, interlaced: Boolean = false): String {

Check warning

Code scanning / detekt

Prefer splitting up complex methods into smaller, easier to test methods. Warning

The function getResolutionName appears to be too complex based on Cyclomatic Complexity (complexity: 15). Defined complexity threshold for methods is set to '15'

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/BaseItemInfoRow.kt

+ verticalAlignment = Alignment.CenterVertically,
+ ) {
+ if (ratingType != RatingType.RATING_HIDDEN) {
+ item.communityRating?.let { InfoRowCommunityRating(it / 10f) }

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. Warning

This expression contains a magic number. Consider defining it to a well named constant.

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/BaseItemInfoRow.kt

+ ) {
+ if (ratingType != RatingType.RATING_HIDDEN) {
+ item.communityRating?.let { InfoRowCommunityRating(it / 10f) }
+ item.criticRating?.let { InfoRowCriticRating(it / 100f) }

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. Warning

This expression contains a magic number. Consider defining it to a well named constant.

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/InfoRowColors.kt

+ * Colors used inm the [BaseItemInfoRow].
+ */
+object InfoRowColors {
+ val Default = Color(0xFF333333)

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. Warning

This expression contains a magic number. Consider defining it to a well named constant.

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/InfoRowColors.kt

+ */
+object InfoRowColors {
+ val Default = Color(0xFF333333)
+ val Green = Color(0xFF30843D)

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. Warning

This expression contains a magic number. Consider defining it to a well named constant.

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/InfoRowColors.kt

+object InfoRowColors {
+ val Default = Color(0xFF333333)
+ val Green = Color(0xFF30843D)
+ val Red = Color(0xFFB20000)

Check warning

Code scanning / detekt

Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. Warning

This expression contains a magic number. Consider defining it to a well named constant.

crobibero approved these changes

View reviewed changes

app/src/main/java/org/jellyfin/androidtv/ui/browsing/composable/inforow/InfoRowColors.kt Outdated Show resolved Hide resolved


Fix typo

cf7e96b

Co-authored-by: Cody Robibero <cody@robibe.ro>

nielsvanvelzen merged commit 004d4e0 into jellyfin:master

6 checks passed

nielsvanvelzen deleted the compose-base-item-info-row branch

February 20, 2024 17:57

nielsvanvelzen mentioned this pull request

Improve info row colors #3448

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment