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 Subtitle Color Customization Features #1900

Merged
merged 2 commits into from
Oct 26, 2022
Merged

Add Subtitle Color Customization Features #1900

merged 2 commits into from
Oct 26, 2022

Conversation

sparky3387
Copy link
Contributor

This adds more display and subtitle customization options to the preferences list, needed to see which subtitle color you are selecting, and modifies the subtitle text renderer to display the text with the set options, as this is my first ever Android application and my first Java program in 10 years, I hopefully did not make any mistakes, I have tried to match the source code as much as possible to make the merge easier.

If possible I would request the pull request approver please check the resource attributes I have used to ensure they are correct especially the R.color.button_default_normal_text value

Changes

  • Modifies the RichListDialogFragment/RichListDialogOption preferences to be able to display colors
  • Modifies StrokeTextView.kt and CustomPlaybackOverlayFragment.java to display set subtitle options

@sparky3387 sparky3387 mentioned this pull request Jul 27, 2022
Open
@nielsvanvelzen nielsvanvelzen added the next-release Pull request related to a future release which is not being worked on yet label Jul 28, 2022
@nielsvanvelzen nielsvanvelzen self-assigned this Jul 28, 2022
@nielsvanvelzen
Copy link
Member

I'll review this soon (after 0.14 is released) but you can already start on the linter issues. There's 4 issues that are marked as error. Those need to be fixed to keep compatibility with older platforms (I think you can just use ContextCompat/ResourcesCompat in those places)

@nielsvanvelzen nielsvanvelzen removed their assignment Jul 28, 2022
@nielsvanvelzen nielsvanvelzen self-requested a review July 28, 2022 19:19
@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
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jul 31, 2022
nielsvanvelzen
nielsvanvelzen requested changes Aug 9, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/preference/UserPreferences.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/preference/UserPreferences.kt Outdated Show resolved Hide resolved
app/src/main/res/values/subtitle_colors.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/shared/StrokeTextView.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/shared/StrokeTextView.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/preference_rich_list_option.xml Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/preference/custom/RichListDialogFragment.kt Outdated Show resolved Hide resolved
@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Aug 11, 2022
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Aug 20, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 20, 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 13 potential problems in the proposed changes. Check the Files changed tab for more details.

@nielsvanvelzen
Copy link
Member

There were still some comments unresolved. I've marked them as such again. I'll do a full re-review after they are solved.

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 30, 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 13 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 31, 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 13 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 1, 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 13 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 2, 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 13 potential problems in the proposed changes. Check the Files changed tab for more details.

@nielsvanvelzen nielsvanvelzen added playback Issue related to media playback release-highlight Pull request might be suitable for mentioning in the release blog post labels Sep 3, 2022
nielsvanvelzen
nielsvanvelzen requested changes Sep 3, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/preference/UserPreferences.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/preference/custom/ColorPickerDialogFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/preference/custom/ColorPickerDialogFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/preference/screen/PlaybackPreferencesScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/preference/screen/PlaybackPreferencesScreen.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/shared/StrokeTextView.kt Outdated Show resolved Hide resolved
@nielsvanvelzen nielsvanvelzen removed this from the v0.15.0 milestone Sep 3, 2022
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 12, 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 14 potential problems in the proposed changes. Check the Files changed tab for more details.

nielsvanvelzen
nielsvanvelzen requested changes Oct 1, 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.

Can you squash all commits and rebase on top of master?

app/src/main/java/org/jellyfin/androidtv/preference/UserPreferences.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/preference/custom/ColorListPreference.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/preference/custom/ColorPickerDialogFragment.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/preference/screen/PlaybackPreferencesScreen.kt Outdated Show resolved Hide resolved
@nielsvanvelzen nielsvanvelzen linked an issue Oct 9, 2022 that may be closed by this pull request
Adjust Subtitle Position #2111
Closed
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 11, 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.

Android Lint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@sparky3387
Initial commit
b5fb243 
    pick 13699f6 Initial commit
    squash 7e8af2d Modify RichListDialog to display subtitle colors
    squash 19fcb15 Modify RichListOption to display subtitle colors

Modify RichListDialog to display subtitle colors

Modify RichListOption to display subtitle colors

Add Subtitle options to preferences list

Added dark grey option and match master commit as much as possible

Fix line length

Fix line length

Fix API Level to Support 21 Lint Error 1

Fix API Level to Support 21 Lint Error 1

Fix API Level to Support 21 Lint Error 1

Fix API Level to Support 21 Lint Error 2

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

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/preference/UserPreferences.kt

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/shared/StrokeTextView.kt

Co-authored-by: Niels van Velzen <[email protected]>

Requested resolutions implemented part 1

Update app/src/main/java/org/jellyfin/androidtv/ui/preference/dsl/OptionsColorList.kt

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/shared/StrokeTextView.kt

Co-authored-by: Niels van Velzen <[email protected]>

Requested resolutions implemented part 2, restore RichListDialogFragment.kt and add ColorPickerDialogFragment and ColorListPreference

Requested resolutions implemented part 3, restore distributionkey

Rebased PlaybackPreferencesScreen.kt

Update app/src/main/java/org/jellyfin/androidtv/preference/UserPreferences.kt

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/preference/dsl/OptionsColorList.kt

Co-authored-by: Niels van Velzen <[email protected]>

Resolve line length and magic number issues, using different background for the brighter colors

Update app/src/main/java/org/jellyfin/androidtv/ui/shared/StrokeTextView.kt

Co-authored-by: Niels van Velzen <[email protected]>

Resolve magic numbers attempt

Resolve line length issue

Remove unnecessary antialias

Update app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/preference/screen/PlaybackPreferencesScreen.kt

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java

Co-authored-by: Niels van Velzen <[email protected]>

Move subtitle setting preference to where the rest of the settings are located
Resolve formatting issue in PlaybackPreferenceScreen

Update PlaybackPreferencesScreen.kt

Fixing formatting

Move subtitle setting preference to where the rest of the settings are located
Resolve formatting issue in PlaybackPreferenceScreen
Resolve import of userpreferences in StrokeTextView.kt

Removed unnecessary functions and data attributes from ColorPickerDialogFragment.k

Update app/src/main/java/org/jellyfin/androidtv/ui/playback/CustomPlaybackOverlayFragment.java

Co-authored-by: Niels van Velzen <[email protected]>

Update app/src/main/java/org/jellyfin/androidtv/ui/preference/screen/PlaybackPreferencesScreen.kt

Co-authored-by: Niels van Velzen <[email protected]>

Resolve magic numbers on lighter colors

Resolve positioning of subtitles and importing of preferences

Correct positioning of subtitlesTextColor in UserPreferences.kt
Lower Increment

Resolve preference issue

Resolve UserPreferences import
@nielsvanvelzen nielsvanvelzen added this to the v0.15.0 milestone Oct 26, 2022
@nielsvanvelzen nielsvanvelzen enabled auto-merge (rebase) October 26, 2022 20:11
nielsvanvelzen
nielsvanvelzen approved these changes Oct 26, 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.

Thanks for your contribution and your time! I've made some final adjustments so we can merge the PR.

@nielsvanvelzen nielsvanvelzen merged commit 4db9973 into jellyfin:master Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mueslimak3r mueslimak3r mueslimak3r left review comments

@nielsvanvelzen nielsvanvelzen nielsvanvelzen approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request playback Issue related to media playback 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.

Adjust Subtitle Position
4 participants
@sparky3387 @nielsvanvelzen @mueslimak3r @jellyfin-bot