Add Lyric support #4733

robert-hamilton36 · 2023-07-28T02:06:44Z

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}

ebkalderon · 2024-01-24T17:03:09Z

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?

robert-hamilton36 · 2024-02-02T03:51:25Z

Oh no, Sorry. I didn't mean to leave it in draft. I think its good to go and be reviewed.

thornbill · 2024-02-21T20:34:53Z

I believe this will require changes to be compatible with jellyfin/jellyfin#9951

jellyfin-bot · 2024-03-24T08:06:26Z

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

dmitrylyzo

I reviewed without testing.

src/components/itemContextMenu.js

src/components/nowPlayingBar/nowPlayingBar.js

src/controllers/itemDetails/index.js

src/controllers/lyrics.js

src/controllers/playback/queue/index.html

src/scripts/deleteHelper.js

thornbill · 2024-03-26T07:37:37Z

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.

dmitrylyzo

Another pass.

Shouldn't we close the lyrics page after music stops?
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.
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?

src/components/itemContextMenu.js

src/components/remotecontrol/remotecontrol.js

src/controllers/itemDetails/index.js

src/controllers/lyrics.html

robert-hamilton36 · 2024-03-26T21:51:42Z

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.
   ![lyrics-small-button](https://private-user-images.githubusercontent.com/56478732/316880558-840ac490-0514-4e65-bf3a-5b482cca6da4.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTE0ODIyOTUsIm5iZiI6MTcxMTQ4MTk5NSwicGF0aCI6Ii81NjQ3ODczMi8zMTY4ODA1NTgtODQwYWM0OTAtMDUxNC00ZTY1LWJmM2EtNWI0ODJjY2E2ZGE0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAzMjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMzI2VDE5Mzk1NVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWIwMTI2ZTYwM2YzNTI3N2Y2N2RmN2RiZjg1YWY0YzllYThjYzljYWFkZjU0NWNhZDA0YTNmOWE3MjA2OGJjN2ImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.1bXojUGQ-M3MNf5AotbfuqQf4JkUBu0zsjLNZ1uyqpc)

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

robert-hamilton36 · 2024-03-27T05:30:56Z

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.
   ![lyrics-small-button](https://private-user-images.githubusercontent.com/56478732/316880558-840ac490-0514-4e65-bf3a-5b482cca6da4.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTE1MTcxNTksIm5iZiI6MTcxMTUxNjg1OSwicGF0aCI6Ii81NjQ3ODczMi8zMTY4ODA1NTgtODQwYWM0OTAtMDUxNC00ZTY1LWJmM2EtNWI0ODJjY2E2ZGE0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAzMjclMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMzI3VDA1MjA1OVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWEyYzQwNTM1ZTVjNzlmOTY4OGM4ZDUxOWQwNzkzMDYxZTQ2NzNlYjJhZTYxMmI1MGJkNTY3MjNlZWQ3NjgyNjYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.mTo9kmkdQzTnUj9nyn5q1TgYbniqka92DiX0cXpT-YE)

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 73c01fb
possible fix suggested above
fixed with cd8d5c7 and then 83bee29
fixed with 286e0bf

robert-hamilton36 · 2024-03-27T05:33:09Z

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 f1d955a

Or else I might need a bit of guidance

github-actions

ESLint doesn't pass. Please fix all ESLint issues.

src/components/nowPlayingBar/nowPlayingBar.js

src/controllers/lyrics.js

dmitrylyzo · 2024-03-28T10:54:07Z

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.

robert-hamilton36 · 2024-03-28T20:52:50Z

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

dmitrylyzo · 2024-03-28T21:08:05Z

If you manually enter /#/queue into 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 Device button (top right button before Search) on Main.
Select another client (not My Device).
Run music on Main client.
Click Cast to Device on Main. Then Remote 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. 🤷‍♂️

robert-hamilton36 · 2024-03-29T22:21:45Z

I've reverted onPlaybackStop Behaviour to goHome(), until a work around is found

src/controllers/lyrics.js

src/components/itemContextMenu.js

src/components/nowPlayingBar/nowPlayingBar.js

src/components/remotecontrol/remotecontrol.js

src/components/nowPlayingBar/nowPlayingBar.js

dmitrylyzo

Oops, I forgot to submit review.

src/styles/lyrics.scss

src/controllers/lyrics.js

src/controllers/lyrics.html

src/controllers/lyrics.js

dmitrylyzo

I think these are the last ones. 🤞

This PR requires squashing before merging.

src/controllers/lyrics.js

src/components/nowPlayingBar/nowPlayingBar.js

src/controllers/playback/queue/index.html

dmitrylyzo

LGTM.

Who will squash the commits?

robert-hamilton36 · 2024-04-08T21:39:38Z

I can squash, assuming this is alright
git reset --soft master
git push-f master LyricsSupport

dmitrylyzo · 2024-04-09T21:21:54Z

I can squash, assuming this is alright
git reset --soft master
git 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, where upstream is 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 must pick the first commit, others - fixup.
Fix conflicts if any. Then, git rebase --continue. Until rebase is complete.
You can abort rebase with git 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.

robert-hamilton36 · 2024-04-09T22:52:08Z

I can squash, assuming this is alright
git reset --soft master
git 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):

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

sonarcloud · 2024-04-11T06:30:23Z

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

jellyfin-bot · 2024-04-11T06:33:55Z