jellyfin-web
jellyfin-web copied to clipboard
jellyfin
Add Lyric support
Changes Adds route for lyrics. Will show current lyric highlighted if saved file allows it Add item detail page for individual songs that show the lyrics
Previews
#/Lyrics
#/details?id={songsitemId}
Kudos, SonarCloud Quality Gate passed! 
0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells
No Coverage information
2.7% Duplication
![sonarcloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
Kudos, SonarCloud Quality Gate passed!
0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells
No Coverage information
2.7% Duplication
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
Quality Gate passed
The SonarCloud Quality Gate passed, but some issues were introduced.
7 New issues
0 Security Hotspots
No data about Coverage
1.3% Duplication on New Code
See analysis details on SonarCloud
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
Hey there! Just wondering what other work might need to be done before this PR can be taken out of draft and reviewed. The implementation itself looks great. Might this be blocked on something that needs additional help shepherding through?
Oh no, Sorry. I didn't mean to leave it in draft. I think its good to go and be reviewed.
I believe this will require changes to be compatible with https://github.com/jellyfin/jellyfin/pull/9951
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
You also need to rebase or merge master back into this branch to fix the conflicts. We are still interested in getting this in for 10.9.0 if we can get it reviewed and any issues resolved this week.
Another pass.
1. Shouldn't we close the lyrics page after music stops? 2. This is not good in TV display mode as there is no scrolling. In fact, we don't handle text blocks in TV display mode well. 3. In Mobile display mode, the `Lyrics` button is small. This may be improved in the future. Maybe after a redesign or so. UPD: 4. There is a new section called
More From This Album. It doesn't seem to be related to the lyrics feature. Maybe it should be moved to the new PR?
- Yes that would be a good idea. I'm thinking i'll just have to add an event for 'playbackstop' that appRouter.back()
- Right silly me I guess I must have always used my scroll wheel, not realizing tvs couldn't do that. Assuming the keyboards arrow keys act the same as the tvs remotes navigation, I'll look into add scrolling. I'm not familiar with how its been implemented already in menus
- Hmm yes, secondary buttons are hidden in mobile mode. I didn't realize and left the lyric button visible. I'll hide it, the problem is getting back onto that lyric page. For now, what I can do is add that lyric button to the item details page like so
Which would require accessing view lyrics from the context menu and than clicking the mic button
- Yes its not directly related to the lyric feature. My thinking was, that the new section is only relevant because of the new route #/details?id={songsitemId} that is opened on the view lyrics context menu. But will move into a seperate PR
Another pass.
1. Shouldn't we close the lyrics page after music stops? 2. This is not good in TV display mode as there is no scrolling. In fact, we don't handle text blocks in TV display mode well. 3. In Mobile display mode, the `Lyrics` button is small. This may be improved in the future. Maybe after a redesign or so. UPD: 4. There is a new section called
More From This Album. It doesn't seem to be related to the lyrics feature. Maybe it should be moved to the new PR?
- fixed with https://github.com/jellyfin/jellyfin-web/pull/4733/commits/73c01fb688481c429f6d42092ec3ce3f2b6ff7f3
- possible fix suggested above
- fixed with https://github.com/jellyfin/jellyfin-web/pull/4733/commits/cd8d5c73de2d10c9b350dadcbc67f4cfe75983f7 and then https://github.com/jellyfin/jellyfin-web/pull/4733/commits/83bee29bd7c375a8804b71b34091180ff49453b6
- fixed with https://github.com/jellyfin/jellyfin-web/pull/4733/commits/286e0bf4f8e82897d7d87f3ad5b80304bd50b464
You also need to rebase or merge master back into this branch to fix the conflicts. We are still interested in getting this in for 10.9.0 if we can get it reviewed and any issues resolved this week.
I hope this was it https://github.com/jellyfin/jellyfin-web/pull/4733/commits/f1d955a941ce069b322cad69525a1b4f497d786f
Or else I might need a bit of guidance
I have found an issue. realeaseCurrentPlayer() doesn't seem to be unbinding events. So even after you exit the lyrics page the events are still being run in the background
EDIT:
I've found the problem. bindToPlayer() was adding the callback to each event multiple times(whenever it was called) and unbind was only removing it once
FIXED: https://github.com/jellyfin/jellyfin-web/pull/4733/commits/61197cee3637ccbc5b5835d066350d8e251791ae
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged.
The only thing that bothers me is this behavior:
1. Start music. 2. Click the now playing bar (open queue). 3. Open lyrics. 4. Stop music using the now playing bar. 5. The lyrics page executes `appRouter.back`. 6. The queue page appears to be broken.
Yes so I think appRouter.goHome() is the best option. Because if you stay on the lyric page, the arrow in the top left will still take you to the broken queue page
Yes so I think appRouter.goHome() is the best option. Because if you stay on the lyric page, the arrow in the top left will still take you to the broken queue page
This workaround seems to work:
diff --git a/src/controllers/playback/queue/index.js b/src/controllers/playback/queue/index.js
index d95081b91..60c541bea 100644
--- a/src/controllers/playback/queue/index.js
+++ b/src/controllers/playback/queue/index.js
@@ -1,4 +1,5 @@
import RemoteControl from '../../../components/remotecontrol/remotecontrol';
+import { appRouter, history } from 'components/router/appRouter';
import { playbackManager } from '../../../components/playback/playbackmanager';
import libraryMenu from '../../../scripts/libraryMenu';
import '../../../elements/emby-button/emby-button';
@@ -35,8 +36,19 @@ export default function (view) {
}
view.addEventListener('viewshow', function () {
+ const player = playbackManager.getCurrentPlayer();
+
+ if (!player) {
+ if (history.length > 1) {
+ appRouter.back();
+ } else {
+ appRouter.goHome();
+ }
+ return;
+ }
+
libraryMenu.setTransparentMenu(true);
- bindToPlayer(playbackManager.getCurrentPlayer());
+ bindToPlayer(player);
document.addEventListener('keydown', onKeyDown);
if (remoteControl) {
I hope it doesn't break anything.
Yes so I think appRouter.goHome() is the best option. Because if you stay on the lyric page, the arrow in the top left will still take you to the broken queue page
This workaround seems to work:
diff --git a/src/controllers/playback/queue/index.js b/src/controllers/playback/queue/index.js index d95081b91..60c541bea 100644 --- a/src/controllers/playback/queue/index.js +++ b/src/controllers/playback/queue/index.js @@ -1,4 +1,5 @@ import RemoteControl from '../../../components/remotecontrol/remotecontrol'; +import { appRouter, history } from 'components/router/appRouter'; import { playbackManager } from '../../../components/playback/playbackmanager'; import libraryMenu from '../../../scripts/libraryMenu'; import '../../../elements/emby-button/emby-button'; @@ -35,8 +36,19 @@ export default function (view) { } view.addEventListener('viewshow', function () { + const player = playbackManager.getCurrentPlayer(); + + if (!player) { + if (history.length > 1) { + appRouter.back(); + } else { + appRouter.goHome(); + } + return; + } + libraryMenu.setTransparentMenu(true); - bindToPlayer(playbackManager.getCurrentPlayer()); + bindToPlayer(player); document.addEventListener('keydown', onKeyDown); if (remoteControl) {I hope it doesn't break anything.
If you manually enter /#/queue into the browser.
The queue still has the playbar but with 3 added menus:
- Navigation
- Send message
- Enter text
They don't seem to do anything, but I don't know if its still in use or not. But this fix will stop you from getting to this menu.
Looking at all the uses of appRouter.showNowPlaying(), they all appear to only be available when playing music (apart from inputManager which doesn't appear to be in use at all). And so would never take you to that screen in this format
If you manually enter
/#/queueinto the browser. The queue still has the playbar but with 3 added menus:
When I tested, it redirected back if there was something open, or went to the home page when entered in a blank tab.
But this fix will stop you from getting to this menu.
I haven't tested remote control. This page should be accessible from the PlayOn button -> Remote control.
UPD:
When connecting to another client (Cast to Device button), a session player is created that allows the queue page to be used.
So it's normal that it doesn't work if you open it manually.
You can try it like this:
- Open Jellyfin in your normal browser. Let's call it
Main. - Open another Jellyfin in incognito window. You probably need to use a different Jellyfin user to log in.
- Click
Cast to Devicebutton (top right button beforeSearch) onMain. - Select another client (not
My Device). - Run music on
Mainclient. - Click
Cast to DeviceonMain. ThenRemote Control.
UPD2:
Hmm, the imported history doesn't seem to have length.
Using window.history helps, but not much - we can't redirect when entering .../#/queue in an empty tab (Chrome).
So I dunno. :man_shrugging:
I've reverted onPlaybackStop Behaviour to goHome(), until a work around is found
I can squash, assuming this is alright
git reset --soft master
git push-f master LyricsSupport
I can squash, assuming this is alright
git reset --soft mastergit push-f master LyricsSupport
I don't think this will collect all the changes because merge commits (instead of rebase) were used.
There may be an easier way, but I am used to do it this way (being on the PR branch):
- Update the upstream.
git fetch upstream, whereupstreamis a remote pointing to the upstream repo of jellyfin-web. - Backup current branch. To compare with it later.
git branch backup-rebase - Run interactive rebase.
git rebase -i upstream/master. In the rebase script, you mustpickthe first commit, others -fixup. - Fix conflicts if any. Then,
git rebase --continue. Until rebase is complete. You can abort rebase withgit rebase --abort. - Compare the current branch with the backup to see if there are any unnecessary changes.
- If it is fine, force push. If you ruin your branch, you can re-checkout it from the backup.
I can squash, assuming this is alright
git reset --soft mastergit push-f master LyricsSupportI don't think this will collect all the changes because merge commits (instead of rebase) were used.
There may be an easier way, but I am used to do it this way (being on the PR branch):
1. Update the upstream. `git fetch upstream`, where `upstream` is a remote pointing to the upstream repo of jellyfin-web. 2. Backup current branch. To compare with it later. `git branch backup-rebase` 3. Run interactive rebase. `git rebase -i upstream/master`. In the rebase script, you must `pick` the first commit, others - `fixup`. 4. Fix conflicts if any. Then, `git rebase --continue`. Until rebase is complete. You can abort rebase with `git rebase --abort`. 5. Compare the current branch with the backup to see if there are any unnecessary changes. 6. If it is fine, force push. If you ruin your branch, you can re-checkout it from the backup.
Awesome! Thanks so much for all your help
Quality Gate passed
Issues
4 New issues
0 Accepted issues
Measures
0 Security Hotspots
No data about Coverage
2.6% Duplication on New Code
See analysis details on SonarCloud
Cloudflare Pages deployment
| Latest commit | 9a56230 |
|---|---|
| Status | ✅ Deployed! |
| Preview URL | https://7e8d8d9d.jellyfin-web.pages.dev |
| Type | 🔀 Preview |
Very excited to see this feature finally land in the 10.9 release, and not a moment too soon! Thanks to all involved. :tada: