Skip to content
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

Jump to bottom

Add RTL Support #1842

Merged
merged 2 commits into from
Aug 19, 2022
Merged

Add RTL Support #1842

merged 2 commits into from
Aug 19, 2022

Conversation

hadicharara
Copy link
Contributor

Changes
I added RTL support for this app. Most of the work was clicking the refactor button in Android Studio. Other parts included properly formatting numbers (to support Eastern Arabic Numbers, for example), rewriting certain layouts (like legacy card image layout) and adding more constraints to others.

Issues
None

@hadicharara
Copy link
Contributor Author

I am open to any feedback!

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 13, 2022
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

@nielsvanvelzen nielsvanvelzen added the next-release Pull request related to a future release which is not being worked on yet label Jul 18, 2022
@jellyfin-bot jellyfin-bot added merge conflict Conflicts prevent merging and removed merge conflict Conflicts prevent merging labels Jul 20, 2022
@nielsvanvelzen
Copy link
Member

Sorry for the wait. I'm currently busy preparing the 0.14 release and this PR might break some things so it will be added for 0.15 to give us some more time to test it. From a quick glance it looks fine but I'll properly review it in the coming weeks.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 22, 2022
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

@hadicharara
Copy link
Contributor Author

No worries mate. Take your time.

@jellyfin-bot jellyfin-bot added merge conflict Conflicts prevent merging and removed merge conflict Conflicts prevent merging labels Jul 23, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 24, 2022
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

@nielsvanvelzen nielsvanvelzen added this to the v0.15.0 milestone Jul 29, 2022
@nielsvanvelzen nielsvanvelzen removed the next-release Pull request related to a future release which is not being worked on yet label Jul 29, 2022
@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Aug 9, 2022
nielsvanvelzen
nielsvanvelzen requested changes Aug 9, 2022
View reviewed changes
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look fine for the most part but the changes in the view_card_legacy_image layout cause the card shadows to enable in the browsing view, which shouldn't happen:

image

@hadicharara
Copy link
Contributor Author

I'm looking into it.

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 9, 2022
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

@hadicharara
Copy link
Contributor Author

It's weird as I don't get the same result:
image
Did you do anything in particular to see those rectangular opaque shadows?

@nielsvanvelzen
Copy link
Member

It only showed in the browsing screen (so when in a library)

@hadicharara
Copy link
Contributor Author

I couldn't reproduce it. I am guessing maybe it has to do with your non-stock theme. Should I just eliminate shadows going down?

@hadicharara
Copy link
Contributor Author

Also are we sure these are shadows?

@nielsvanvelzen
Copy link
Member

I am guessing maybe it has to do with your non-stock theme.

It's the default theme

Also are we sure these are shadows?

Partially background, partially shadow

@hadicharara
Copy link
Contributor Author

I meant your OS theme. The title progress bar and the tick don't look the same on my machine. Honestly, I couldn't reproduce it so I don't know what to do.

@nielsvanvelzen
Copy link
Member

Testing was done with the Android emulator (Android 12) with nothing changed.

@hadicharara
Copy link
Contributor Author

hadicharara commented Aug 13, 2022 •
edited
Loading

I tried both Android 12 TV emulator images available in Android Studio. Neither look like your screenshot. They looked more like mine. There was a light blue circle tick on the cards and no shadow. I'm sorry, I couldn't reproduce the issue. I don't know what to do.

@nielsvanvelzen
Copy link
Member

Did some more testing and apparently this only happens when the library is set to align items vertically, weirdly enough.

@hadicharara
Copy link
Contributor Author

Thank you! I managed to reproduce it! I'll investigate the root cause.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Aug 13, 2022
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Aug 18, 2022
@hadicharara
Copy link
Contributor Author

It should be fixed now.

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 19, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java
@@ -452,7 +452,7 @@
if (item.getBaseItemType() == BaseItemType.Series) {
firstRow = new InfoItem(
getString(R.string.lbl_seasons),
Utils.getSafeValue(item.getChildCount(), 0).toString());
String.format("%d", Utils.getSafeValue(item.getChildCount(), 0)));

Check notice

Code scanning

Implied default locale in case conversion

Implicitly using the default locale is a common source of bugs: Use String.format(Locale, ...) instead
app/src/main/java/org/jellyfin/androidtv/ui/itemdetail/FullDetailsActivity.java
@@ -816,7 +816,7 @@

private String getRunTime() {
Long runtime = Utils.getSafeValue(mBaseItem.getRunTimeTicks(), mBaseItem.getOriginalRunTimeTicks());
return runtime != null && runtime > 0 ? (int) Math.ceil((double) runtime / 600000000) + getString(R.string.lbl_min) : "";
return runtime != null && runtime > 0 ? String.format("%d%s", (int) Math.ceil((double) runtime / 600000000), getString(R.string.lbl_min)) : "";

Check notice

Code scanning

Implied default locale in case conversion

Implicitly using the default locale is a common source of bugs: Use String.format(Locale, ...) instead
app/src/main/AndroidManifest.xml
@@ -50,7 +50,8 @@
android:largeHeap="true"
android:roundIcon="@drawable/app_icon_round"
android:theme="@style/Theme.Jellyfin"
android:usesCleartextTraffic="true">
android:usesCleartextTraffic="true"

Check notice

Code scanning

Attribute unused on older versions

Attribute usesCleartextTraffic is only used in API level 23 and higher (current min is 21)
app/src/main/java/org/jellyfin/androidtv/util/InfoLayoutHelper.java
@@ -234,7 +237,7 @@

TextView amt = new TextView(context);
amt.setTextSize(textSize);
amt.setText(item.getCommunityRating().toString()+" ");
amt.setText(nf.format(item.getCommunityRating()) + " ");

Check notice

Code scanning

TextView Internationalization

Do not concatenate text displayed with setText. Use resource string with placeholders.
app/src/main/java/org/jellyfin/androidtv/util/InfoLayoutHelper.java
@@ -252,7 +255,7 @@
layout.addView(tomato);
TextView amt = new TextView(context);
amt.setTextSize(textSize);
amt.setText(item.getCriticRating().toString() + "% ");
amt.setText(nfp.format(item.getCriticRating()/100) + " ");

Check notice

Code scanning

TextView Internationalization

Do not concatenate text displayed with setText. Use resource string with placeholders.
nielsvanvelzen
nielsvanvelzen approved these changes Aug 19, 2022
View reviewed changes
Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work now! Thanks!

@nielsvanvelzen nielsvanvelzen merged commit 7e2bbbc into jellyfin:master Aug 19, 2022
@nielsvanvelzen nielsvanvelzen added the release-highlight Pull request might be suitable for mentioning in the release blog post label Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsvanvelzen nielsvanvelzen nielsvanvelzen approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request release-highlight Pull request might be suitable for mentioning in the release blog post
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.
3 participants
@hadicharara @nielsvanvelzen @jellyfin-bot