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

adds "shuffle with" button if "always ask" is selected as preferred player. #2459

Merged
merged 2 commits into from
Feb 1, 2023
Merged

adds "shuffle with" button if "always ask" is selected as preferred player. #2459

merged 2 commits into from
Feb 1, 2023

Conversation

tim-vk
Copy link
Contributor

@tim-vk tim-vk commented Jan 31, 2023 •
edited
Loading

Changes

Shows the "shuffle" button when video player is set to "always ask". Selecting the shuffle button prompts the player types to be selected.

Issues

Implements #2321

Some questions for the reviewers

  • The button is now labelled: "Shuffle" not "shuffle with". is that desired?
  • I'm not used to java: I've created a seperate function: playWithItemMenu which is used within shuffleMenu and PlayMenu. Is this the correct way to implement such a feature?
  • There are some other functions that are also hidden if "always ask" is selected. Would you like me to implement those too? If yes: should that be in this PR or seperate PR's per "feature"?
  • What is the conversion policy for when/how to convert to Kotlin?

@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Jan 31, 2023
@nielsvanvelzen
Copy link
Member

The button is now labelled: "Shuffle" not "shuffle with". is that desired?

I think that's fine.

I'm not used to java: I've created a seperate function: playWithItemMenu which is used within shuffleMenu and PlayMenu. Is this the correct way to implement such a feature?

In this case, good enough. This file is a bit messy anyway so I'm not that picky reviewing it.

There are some other functions that are also hidden if "always ask" is selected. Would you like me to implement those too? If yes: should that be in this PR or seperate PR's per "feature"?

They could probably be in the same PR but tbh it makes sense to hide those options if the user chooses to not have a default player.

What is the conversion policy for when/how to convert to Kotlin?

New files are Kotlin, existing files may be converted but the left-over Java files is mostly stuff that's slowly getting rewritten so I wouldn't bother with it if I were you.

@tim-vk tim-vk force-pushed the feature/shuffle_with branch from bc35048 to ae4d7d4 Compare January 31, 2023 15:29
@tim-vk
Copy link
Contributor Author

tim-vk commented Jan 31, 2023 •
edited
Loading

I'm not used to java: I've created a seperate function: playWithItemMenu which is used within shuffleMenu and PlayMenu. Is this the correct way to implement such a feature?

In this case, good enough. This file is a bit messy anyway so I'm not that picky reviewing it.

Luckily it's not just me who finds this messy. Are there any running project/plans to refactor this file? Reason I ask is because I also wanted to take a look at language settings (for subtitles e.g.) before you press play: i think that also touches this file quite a lot.

There are some other functions that are also hidden if "always ask" is selected. Would you like me to implement those too? If yes: should that be in this PR or seperate PR's per "feature"?

They could probably be in the same PR but tbh it makes sense to hide those options if the user chooses to not have a default player.

Check! I'll leave this PR as-is. So If you are happy with this PR you can merge it. I won't be adding features.

@tim-vk tim-vk requested a review from nielsvanvelzen January 31, 2023 18:09
@nielsvanvelzen
Copy link
Member

Luckily it's not just me who finds this messy. Are there any running project/plans to refactor this file? Reason I ask is because I also wanted to take a look at language settings (for subtitles e.g.) before you press play: i think that also touches this file quite a lot.

I'm currently working on rewriting all playback code (#1057) and after that I want to rewrite the UI with Jetpack Compose when it's ready for Android TV.

@tim-vk tim-vk force-pushed the feature/shuffle_with branch from ae4d7d4 to c93c2a4 Compare February 1, 2023 20:36
nielsvanvelzen
nielsvanvelzen approved these changes Feb 1, 2023
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 the PR!

@nielsvanvelzen nielsvanvelzen added this to the v0.16.0 milestone Feb 1, 2023
@nielsvanvelzen nielsvanvelzen merged commit ed0540a into jellyfin:master Feb 1, 2023
@tim-vk tim-vk deleted the feature/shuffle_with branch February 2, 2023 08:08
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
Projects
None yet
Milestone
v0.16.0
Development

Successfully merging this pull request may close these issues.
2 participants
@tim-vk @nielsvanvelzen