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

Re-add "Ends at" time to playback control overlay #616

Merged
merged 12 commits into from
Nov 12, 2020

Conversation

MrChip53
Copy link
Contributor

@MrChip53 MrChip53 commented Nov 6, 2020

Changes
Add the "Ends At: ####" back to the control overlay

Issues

@MrChip53
Copy link
Contributor Author

MrChip53 commented Nov 7, 2020

Just realized I didn't do a proper string resource but before I worry about fixing let me know if you all approve of this method and then if the colon should be in the ends at string

@nielsvanvelzen
Copy link
Member

Test notes:

  • When playback starts it takes a second or so before the "ends at" shows up
  • The format is always in 12-hour clock notation
  • The text style is different from the time/duration text

For the second you can use the DateFormat class which should follow system preferences.
DateFormat.getTimeInstance(DateFormat.SHORT).format(date)

@MrChip53
Copy link
Contributor Author

MrChip53 commented Nov 7, 2020

Test notes:

  • When playback starts it takes a second or so before the "ends at" shows up
  • The format is always in 12-hour clock notation
  • The text style is different from the time/duration text

For the second you can use the DateFormat class which should follow system preferences.
DateFormat.getTimeInstance(DateFormat.SHORT).format(date)

I can't check these off but I'm pretty sure I addressed all of them. I also added some null checks but I don't know if they are really needed. The delay on the text showing up is due to when updatePlaybackState gets called. I just made it show 'Loading...' by default. I also added the string resource for 'Ends at' and removed the colon so it now displays 'Ends at 9:00 PM' for example.

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.

I just made it show 'Loading...' by default.

I noticed the video duration shows up earlier so it should be possible to show the "ends at" text earlier too. Just need to find the right place to add it.

@MrChip53
Copy link
Contributor Author

MrChip53 commented Nov 8, 2020

I just made it show 'Loading...' by default.

I noticed the video duration shows up earlier so it should be possible to show the "ends at" text earlier too. Just need to find the right place to add it.

I addressed both the issues. Should we save the context to a variable and replace all calls to getContext() with the variable?

@MrChip53
Copy link
Contributor Author

MrChip53 commented Nov 9, 2020

I over complicated the solution. I'm going to move the textview creation back inside the onCreateViewHolder function and use the setEndsText function in there to remove the need for the null pointer check. But still, are we okay with storing the context received on class creation in a variable and reusing or are the calls to get context not taxing enough to worry about that?

@nielsvanvelzen
Copy link
Member

The parent class stores the context and getContext just returns it, so no need to store ourselves.

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.

When you pause a video for multiple minutes the "ends at" won't update. It will update when playback starts again but this is slightly confusing. We might want to start a handler or similar to update the contents every minute/few seconds while a video is paused - make sure to destroy it when playback starts again or the view is destroyed.

@MrChip53
Copy link
Contributor Author

MrChip53 commented Nov 10, 2020

I also found an issue where the TextView would not hide when scrolling on the seek bar. I fixed this in the handler but in order to have it be snappy I had to make the handler repeat faster. The handler also has to run 24/7 now when playing as it polls the other view for its visibility state. Let me know what you think about that and if you may have a better method that may not be so taxing, if this method even is taxing on the system.

It may need to repeat even faster to make it look like a natural part of the UI

@nielsvanvelzen
Copy link
Member

You might not need to keep the handler loop active when you use addOnLayoutChangeListener. I'm not sure if that listener is always called though.

@MrChip53
Copy link
Contributor Author

I'll try that out. I did also find another spot where I can catch the views being set invisible instantly so from there I could start a handler to check if they are visible again and then kill it so it won't need to run 24/7.

@MrChip53
Copy link
Contributor Author

MrChip53 commented Nov 11, 2020

Forgot to change the commit message on the first one. 🙈 Whoops lol. Anyways, I ended up doing the second approach. It seems to work nicely. The ends at time also updates while paused. I believe that addresses all issues.

@MrChip53
Copy link
Contributor Author

MrChip53 commented Nov 12, 2020

There is a live tv bug that is causing crashes either when changing the channel when a channel is already playing or when the hour/half hour happens and the program name and time is updated.

Probably just a NPE but not ready for merge yet.

Apparently I was running an old build lol

@nielsvanvelzen
Copy link
Member

For the current leanback player this works fine but we should definitely use another approach when rewriting the playback UI.

@MrChip53
Copy link
Contributor Author

For the current leanback player this works fine but we should definitely use another approach when rewriting the playback UI.

I agree. I honestly don't think it would be too hard to make our own playback controls and still use it with the Glue/Host template.

@nielsvanvelzen nielsvanvelzen merged commit 3a4716a into jellyfin:master Nov 12, 2020
@MrChip53 MrChip53 deleted the ends-at-v2 branch November 12, 2020 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants