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

Support previous button in video playlist #1941

Merged
merged 1 commit into from
Aug 18, 2022
Merged

Support previous button in video playlist #1941

merged 1 commit into from
Aug 18, 2022

Conversation

kevgrig
Copy link
Contributor

@kevgrig kevgrig commented Aug 6, 2022

Fixes #1940

Signed-off-by: Kevin G [email protected]

github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 6, 2022
View reviewed changes
app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackController.java
return mItems != null && mCurrentIndex - 1 >= 0;
}

public BaseItemDto getPreviousItem() {

Check notice

Code scanning / Android Lint

Unknown nullness

Unknown nullability; explicitly declare as @Nullable or @NonNull to improve Kotlin interoperability; see https://developer.android.com/kotlin/interop#nullability_annotations
@kevgrig
Copy link
Contributor Author

kevgrig commented Aug 6, 2022

@nielsvanvelzen fyi

@nielsvanvelzen nielsvanvelzen added this to the v0.15.0 milestone Aug 6, 2022
@mueslimak3r
Copy link
Member

Does this work even after the current video completes and "next up" is shown?

There's some weird logic used rn that clears old items from the video queue after playback completes, and afaik it's there because of some quirk in how next up works.

Can you test letting a video complete/go to "next up" and see if the "previous" button still works/shows up?

Here's the code I mentioned:

itemComplete() -> endPlayback() -> removePreviousQueueItems()

jellyfin-androidtv/app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackController.java

Line 1389 in ccb97ac

public void removePreviousQueueItems() {

@kevgrig
Copy link
Contributor Author

kevgrig commented Aug 8, 2022

Does this work even after the current video completes and "next up" is shown?

I forgot to test this as I had disabled the 'Next Up' function completely as I don't like it. I can confirm that it doesn't work if 'Next Up' is used. I'll check this out...

@kevgrig
Copy link
Contributor Author

kevgrig commented Aug 8, 2022 •
edited
Loading

There's some weird logic used rn that clears old items from the video queue after playback completes, and afaik it's there because of some quirk in how next up works.

Would it be okay for now if I only enabled the previous button if 'Next Up' is completely disabled?

@mueslimak3r
Copy link
Member

mueslimak3r commented Aug 8, 2022 •
edited
Loading

Would it be okay for now if I only enabled the previous button if 'Next Up' is completely disabled?

In my opinion, a more sustainable approach would be to fix the quirk in the next up behavior that required clearing the old queue items. I can look into that in the next couple days.

That's just a suggestion for how we might move forward with getting it working. 🙂

It looks like #1939 does a good chunk of what's needed before removing the queue clearing code thats used for next up

@mueslimak3r mueslimak3r mentioned this pull request Aug 10, 2022
Merged
@mueslimak3r
Copy link
Member

#1951 is merged so that should fix the issue with nextup breaking the previous button. Can you rebase and confirm things are good to go?

@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Aug 11, 2022
mueslimak3r
mueslimak3r approved these changes Aug 18, 2022
View reviewed changes
Copy link
Member

@mueslimak3r mueslimak3r left a comment

Choose a reason for hiding this comment

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

I tested again with the branch rebased and it works as expected 👍

@nielsvanvelzen nielsvanvelzen merged commit 9ee8c7f into jellyfin:master Aug 18, 2022
@kevgrig
Copy link
Contributor Author

kevgrig commented Aug 18, 2022

I'm sorry, somehow I missed the previous comment about rebasing. Thanks for doing that for me and merging!

@nielsvanvelzen nielsvanvelzen added the release-highlight Pull request might be suitable for mentioning in the release blog post label Aug 19, 2022
@nielsvanvelzen nielsvanvelzen added release-highlight Pull request might be suitable for mentioning in the release blog post and removed release-highlight Pull request might be suitable for mentioning in the release blog post labels Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mueslimak3r mueslimak3r mueslimak3r 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.

No previous button in video playlist
3 participants
@kevgrig @mueslimak3r @nielsvanvelzen