Skip to content

Add chapter selection to OSD #5011

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

TheMelmacian
Copy link
Contributor

@TheMelmacian TheMelmacian commented Nov 21, 2023

Changes
I added a selection menu to the OSD which allows to directly select a chapter.
It shows the number, chapter name and the start time.
If no name is set, a name in the form Chapter 01 is generated.
Th currently active chapter is calculated based on playtime and chapter start/end times and shown in the menu.

To prevent the volume from changing when scrolling through the menu, the mouse wheel listener is deactivated while the menu is visible.
Edit: This was fixed with #5401, so I adjusted my code accordingly.

For the button I used the ordered list icon:
jellyfin_chapter_selection

jellyfin_chapter_selection_menu

@TheMelmacian TheMelmacian requested a review from a team as a code owner November 21, 2023 23:23

This comment has been minimized.

@thornbill thornbill added feature New feature or request ui & ux This PR or issue mainly concerns UI & UX labels Nov 30, 2023
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Mar 24, 2024
@jellyfin-bot

This comment has been minimized.

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Mar 24, 2024

This comment has been minimized.

@Gauvino
Copy link

Gauvino commented Mar 30, 2024

Possible to add on 10.9 ?

@TheMelmacian
Copy link
Contributor Author

The 10.9 release is currently prepared and no more features are added: https://jellyfin.org/posts/testing-10.9.0
Probably 10.10 if the PR gets accepted.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Apr 21, 2024
@jellyfin-bot

This comment has been minimized.

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Apr 27, 2024
@1hitsong
Copy link
Member

1hitsong commented May 18, 2024

I like the idea. This resembles the same menu I created on Roku 🤘

If I click the Chapters button and click somewhere outside of the menu to close it (meaning, I did not click on a chapter), I get the following error:
image

@thornbill
Copy link
Member

That error is an artifact of our weird dialog code that rejects when closed... a couple places depend on it though so probably just need a catch to suppress it here.

@TheMelmacian
Copy link
Contributor Author

The settings, audio selection and subtitle selection are also affected by this.
Adding a catch to the actionsheet promise is easy but the question is should all errors be supressed

.catch(() => {
    // prevent 'ActionSheet closed without resolving' error
})

or only the ActionSheet closed without resolving error

.catch((error) => {
    // supress 'ActionSheet closed without resolving' error
    // log and throw all other errors
    if (error !== 'ActionSheet closed without resolving') {
        console.error('ActionSheet error', error);
        throw new Error(error);
    }
})

just let me now and I will provide a fix for this.

@thornbill
Copy link
Member

I would just catch all for now.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented May 19, 2024

Cloudflare Pages deployment

Latest commit 19af2bf
Status ✅ Deployed!
Preview URL https://f52fba65.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Copy link
Member

@1hitsong 1hitsong left a comment

Choose a reason for hiding this comment

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

Tested in Windows 11.

Functionality works well. Code looks good.

Possible followup idea: Display the chapter images to the side of the chapter names.

soultaco83 added a commit to soultaco83/jellyfin-web-unstable that referenced this pull request Sep 15, 2024
@TheMelmacian
Copy link
Contributor Author

since this didn't make the cut for 10.10, any plans to add it in 10.11 ?

@thornbill
Copy link
Member

Thanks for your patience @TheMelmacian! I will see if our UI/UX lead has feedback.

@thornbill
Copy link
Member

Sorry again for the delay on this. I reviewed the change with our ui/ux lead and I think we are going to hold off and add this functionality as part of the full redesign effort.

@TheMelmacian
Copy link
Contributor Author

As long as the redesign is planned for the next major release, I have no problem with that.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Feb 27, 2025
@jellyfin-bot
Copy link
Collaborator

This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.

@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Feb 27, 2025
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request ui & ux This PR or issue mainly concerns UI & UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants