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.

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

Create Episode Next Up Behavior and OSD Remaining Time #1987

Merged
merged 9 commits into from
Oct 23, 2024

Conversation

Renbo2024
Copy link
Contributor

@Renbo2024 Renbo2024 commented Oct 11, 2024

Changes

Added 2 new features:

  1. New setting Episode Next Up Behavior which controls the behavior when a Next Up show episode is clicked from the home screen. It may either navigate to a description page of the episode, or navigate to the season that episode is in, and select the episode.
  2. New setting OSD Remaining Time which controls what information is shown in the video player bottom OSD (when visible) on the right side. It may either show Remaining Time (current default behavior), completion Time of Day, or both.

This adds a setting with controls the type of data displayed for remaining time: remaining time, completion time of day, or both.
@Renbo2024 Renbo2024 marked this pull request as ready for review October 11, 2024 20:23
@Renbo2024 Renbo2024 requested a review from a team as a code owner October 11, 2024 20:23
@cewert
Copy link
Member

cewert commented Oct 11, 2024

New setting Episode Next Up Behavior... It may either play the episode immediately (current default behavior), navigate to a description page of the episode, or navigate to the season that episode is in, and select the episode.

IMO we should leave the "play the episode" behavior to the play button and just pick one of the two remaining options as the default behavior for everyone. That way if the user wants default audio tracks, subtitle tracks etc they hit the play button. If they want to read description, select audio track, see what time the episode ends etc. they hit the OK button.

The episode description page is just cramming an episode into the movie detail view (which itself still needs lots of work). I vote for episode list as default behavior for the OK button. Once we actually have a proper episode detail page we could revisit this and possibly change default behavior to use that instead.

edit: on second thought the episode page is better than I remember with features the episode list doesnt have. I could go either way. I still vote we pick one as default for eveything and if there's a desire make a user setting to allow either episode page or episode list

@Renbo2024
Copy link
Contributor Author

New setting Episode Next Up Behavior... It may either play the episode immediately (current default behavior), navigate to a description page of the episode, or navigate to the season that episode is in, and select the episode.

IMO we should leave the "play the episode" behavior to the play button and just pick one of the two remaining options as the default behavior for everyone. That way if the user wants default audio tracks, subtitle tracks etc they hit the play button. If they want to read description, select audio track, see what time the episode ends etc. they hit the OK button.

So then your suggestion is to differentiate the behavior of the Play and OK button? If that is the suggestion, that is what this feature does, only it allows for a customization of what to do on OK.

The episode description page is just cramming an episode into the movie detail view (which itself still needs lots of work). I vote for episode list as default behavior for the OK button. Once we actually have a proper episode detail page we could revisit this and possibly change default behavior to use that instead.

OK, so looking like your suggestion is to differentiate the OK and Play button, with OK to season episode list, and leave out the setting for default behavior entirely?

edit: on second thought the episode page is better than I remember with features the episode list doesnt have. I could go either way. I still vote we pick one as default for eveything and if there's a desire make a user setting to allow either episode page or episode list

OK, so just remove the "Play" option in the settings?

Side note: Be aware this is my VERY FIRST ever attempt at working in Brightscript. I tested as best I can, and do not see any exceptions, but if anyone has comments on how I might improve code, let me know.

@Renbo2024
Copy link
Contributor Author

Just pinging again.

If I remove the "Play" option from the setting, does this seem like an acceptable modification for approval?

@jellyfin-bot
Copy link
Contributor

This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks!

@jellyfin-bot jellyfin-bot added the merge-conflict This PR has a merge conflict label Oct 17, 2024
@jellyfin-bot jellyfin-bot removed the merge-conflict This PR has a merge conflict label Oct 18, 2024
@Renbo2024
Copy link
Contributor Author

I have removed "Play" from the OK button options for home screen next up show episodes. I have also resolved conflict. Please let me know if there's anything else needed.

@Renbo2024
Copy link
Contributor Author

Just pinging. I do not know if I missed a step, or perhaps it's just taking a while. Please let me know if there's anything else I need to do.

@jimdogx
Copy link
Contributor

jimdogx commented Oct 22, 2024

Just pinging. I do not know if I missed a step, or perhaps it's just taking a while. Please let me know if there's anything else I need to do.

I like the idea here. However, while testing it, I noticed that the Episode Details page still has the "extra info" popup available (where you'd normally see the "Cast" and "More like this"), except it's empty. Also when hitting "Back" from this popup, it takes you to the Home Screen (instead of back to the Episode).

I would recommend we hide that popup altogether if we don't have anything to show. I will test the OSD stuff next.

@jimdogx
Copy link
Contributor

jimdogx commented Oct 22, 2024

Love the OSD updates.

Regarding the Episode Detail page again... I'm wondering how people will feel about having to click twice to get back to the Home Screen from the middle of an episode now (the first back takes you to the Episode Details page). Same when an episode finishes. It takes you back to the Episode details page of the episode you just watched.

If I'm not mistaken originally there was a third option in your new settings to just "Play"? If so, I would add that back. In other words, let people choose to leave it as it currently works if they want.

locale/en_US/translations.ts Outdated Show resolved Hide resolved
@Renbo2024
Copy link
Contributor Author

Love the OSD updates.

Regarding the Episode Detail page again... I'm wondering how people will feel about having to click twice to get back to the Home Screen from the middle of an episode now (the first back takes you to the Episode Details page). Same when an episode finishes. It takes you back to the Episode details page of the episode you just watched.

It's going back up the navigation history/stack. I am not sure how to override this. Do you know of examples elsewhere that sidestep the stack?

If I'm not mistaken originally there was a third option in your new settings to just "Play"? If so, I would add that back. In other words, let people choose to leave it as it currently works if they want.

I removed this in response to an earlier PR comment, suggesting the Play button on the roku remote already handles the play-immediately behavior.

@cewert
Copy link
Member

cewert commented Oct 22, 2024

This should really be two separate PRs. That would make it easier for us to discuss, review, and test.

We have a user setting that hides all the clocks and references to time for people with insomnia (right next to your new setting in settings.json). If possible, your OSD setting should respect it as well and hide it as necessary.

@jimdogx
Copy link
Contributor

jimdogx commented Oct 22, 2024

I removed this in response to an earlier PR comment, suggesting the Play button on the roku remote already handles the play-immediately behavior.

I didn't think about that. Since the Play button handles the "default", customizing just the OK button makes sense. Sorry for the confusion.

@Renbo2024
Copy link
Contributor Author

We have a user setting that hides all the clocks and references to time for people with insomnia (right next to your new setting in settings.json). If possible, your OSD setting should respect it as well and hide it as necessary.

Updated and pushed.

@Renbo2024
Copy link
Contributor Author

This should really be two separate PRs. That would make it easier for us to discuss, review, and test.

I apologize for the bundling. This all started as a "deal-breaker" feature requirement list for my wife, but then I thought these changes might be useful to others.

@jimdogx
Copy link
Contributor

jimdogx commented Oct 22, 2024

This should really be two separate PRs. That would make it easier for us to discuss, review, and test.

I apologize for the bundling. This all started as a "deal-breaker" feature requirement list for my wife, but then I thought these changes might be useful to others.

No worries. To be honest, there have been several times I wished I could "Set Watched" on an episode, this PR allows me to do that 😄

Unfortunately, I gotta get to work now. It's one of the reasons testing PRs takes time around here. Not sure if you are on The Matrix (see roku client github home page)... but it can be a good place to bounce ideas off of other Roku users and even get others to help with testing.

@Renbo2024
Copy link
Contributor Author

I like the idea here. However, while testing it, I noticed that the Episode Details page still has the "extra info" popup available (where you'd normally see the "Cast" and "More like this"), except it's empty.

I am having a hard time duplicating this behavior. When you have a chance, can you take a screen shot of the issue? Perhaps I can find a way to see it happen on my system, and then can address.

@jimdogx
Copy link
Contributor

jimdogx commented Oct 22, 2024

I am having a hard time duplicating this behavior. When you have a chance, can you take a screen shot of the issue? Perhaps I can find a way to see it happen on my system, and then can address.

When you're on the screen, just push the down arrow. Do you have info filled out on your popup? Maybe I just have missing info on my Episodes?

@Renbo2024
Copy link
Contributor Author

I am having a hard time duplicating this behavior. When you have a chance, can you take a screen shot of the issue? Perhaps I can find a way to see it happen on my system, and then can address.

When you're on the screen, just push the down arrow. Do you have info filled out on your popup? Maybe I just have missing info on my Episodes?

I do have data there.

@Renbo2024
Copy link
Contributor Author

Per discussion on the matrix chat, we'll be leaving the back behavior in place, and leaving the scene stack untouched for now.

@Renbo2024 Renbo2024 requested a review from jimdogx October 23, 2024 12:12
@jimdogx jimdogx merged commit f394171 into jellyfin:master Oct 23, 2024
7 checks passed
@cewert cewert added the new-setting A new user setting. label Nov 10, 2024
@cewert cewert changed the title New Features Create Episode Next Up Behavior and OSD Remaining Time Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-setting A new user setting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants