YTMD/youtube-music
1
0
Fork 0
Code Issues 445 Pull requests 34 Projects Releases 87 Packages Wiki Activity Actions

perf(synced-lyrics): virtual scrolling #3162

Merged
arjix merged 17 commits from chore/synced-lyrics-virtual-scroll into master 2025-07-01 07:21:09 +00:00
Conversation 11 Commits 17 Files changed 15 +377 -279
arjix commented 2025-03-30 22:49:33 +00:00
Owner
Copy link

An attempt to improve the performance of the synced-lyrics renderer.

TODO:

  • use virtua for the virtual scrolling
  • move calculation of current|previous|upcoming to the parent component
  • fix auto-scroll when the current line is not rendered
  • lyrics picker
    • auto show/hide based on scroll-state and/or mouse movement

This should massively improve the performance of the lyrics renderer, and prevent the huge lags that take place after switching songs.

An attempt to improve the performance of the synced-lyrics renderer. TODO: - [x] use [`virtua`](https://www.npmjs.com/package/virtua) for the virtual scrolling - [x] move calculation of `current`|`previous`|`upcoming` to the parent component - [x] fix auto-scroll when the current line is not rendered - [x] lyrics picker - - [x] keep mounted - https://github.com/inokawa/virtua/pull/681 - - [x] auto show/hide based on scroll-state and/or mouse movement This should massively improve the performance of the lyrics renderer, and prevent the huge lags that take place after switching songs.
👍 2
JellyBrick (Migrated from github.com) requested changes 2025-03-30 22:58:55 +00:00
package.json
JellyBrick (Migrated from github.com) commented 2025-03-30 22:53:40 +00:00
Copy link
    "virtua": "0.40.3",

Please use -E option like pnpm i -E virtua

```suggestion "virtua": "0.40.3", ``` Please use **-E** option like `pnpm i -E virtua`
tsconfig.json
@ -9,3 +9,3 @@
"esModuleInterop": true,
"resolveJsonModule": true,
"moduleResolution": "node",
"moduleResolution": "bundler",
JellyBrick (Migrated from github.com) commented 2025-03-30 22:58:37 +00:00
Copy link

If moduleResolution is set to bundler, certain dependencies cannot be imported. e.g. youtubei.js.

If `moduleResolution` is set to `bundler`, certain dependencies cannot be imported. e.g. `youtubei.js`.
arjix reviewed 2025-04-21 23:12:49 +00:00
tsconfig.json
@ -9,3 +9,3 @@
"esModuleInterop": true,
"resolveJsonModule": true,
"moduleResolution": "node",
"moduleResolution": "bundler",
arjix commented 2025-04-21 23:12:49 +00:00
Author
Owner
Copy link

can you ellaborate on that? I see the issue
any other moduleResolution introduces issues

well, technically we are using a bundler, so this is more correct
but I'll have to try to resolve that issue as well

~~can you ellaborate on that?~~ I see the issue any other `moduleResolution` introduces issues well, technically we are using a bundler, so this is more correct but I'll have to try to resolve that issue as well
arjix reviewed 2025-04-21 23:26:42 +00:00
tsconfig.json
@ -9,3 +9,3 @@
"esModuleInterop": true,
"resolveJsonModule": true,
"moduleResolution": "node",
"moduleResolution": "bundler",
arjix commented 2025-04-21 23:26:42 +00:00
Author
Owner
Copy link

looks like it wasn't a big deal

looks like it wasn't a big deal
arjix commented 2025-04-22 01:30:34 +00:00
Author
Owner
Copy link

I am actually stuck about the lyrics picker no longer auto-hiding.
I opened a discussion on the virtua repository to discuss how this should be properly done.

Otherwise, I'll have to resort to weird trickery using absolute positioning, which would look ass.

I am actually stuck about the lyrics picker no longer auto-hiding. I opened a [discussion](https://github.com/inokawa/virtua/discussions/669) on the `virtua` repository to discuss how this should be properly done. Otherwise, I'll have to resort to weird trickery using absolute positioning, which would look ass.
arjix commented 2025-05-02 00:31:31 +00:00
Author
Owner
Copy link

by having a discriminated union, I can have the lyrics picker be an item in the virtual scroller, so I can make it have position: sticky!

image

div:has(> .lyrics-picker) {
  position: sticky !important;
}

so the only thing holding me back, is that the lyrics picker gets unmounted when you scroll away (because this is a virtual list)
waiting on https://github.com/inokawa/virtua/pull/681

by having a discriminated union, I can have the lyrics picker be an item in the virtual scroller, so I can make it have `position: sticky`! ![image](https://github.com/user-attachments/assets/431397ff-578f-4b79-aba5-ca5268178dbc) ```css div:has(> .lyrics-picker) { position: sticky !important; } ``` so the only thing holding me back, is that the lyrics picker gets unmounted when you scroll away (because this is a virtual list) waiting on https://github.com/inokawa/virtua/pull/681
arjix commented 2025-05-15 12:59:20 +00:00
Author
Owner
Copy link

Alright, I think by the end of today I'll mark this PR ready for review!

Alright, I think by the end of today I'll mark this PR ready for review!
❤️ 1
arjix commented 2025-05-15 22:20:02 +00:00
Author
Owner
Copy link

@JellyBrick could you put a warning in the release notes, that this will likely break any custom CSS applied to the synced-lyrics?

@JellyBrick could you put a warning in the release notes, that this will likely break any custom CSS applied to the synced-lyrics?
👍 1
arjix commented 2025-05-15 22:53:34 +00:00
Author
Owner
Copy link

Squashed the commits because they were too many.

Squashed the commits because they were too many.
arjix commented 2025-06-20 00:19:20 +00:00
Author
Owner
Copy link

@JellyBrick I decided to include a better romanization method in this PR

No need to worry about tiny-pinyin anymore, I not only fixed the w e i r d s p a c i n g it has for English, but I massively improved the quality of the romanized lyrics by using tinyld

and then as fallback I use the old detection methods, and I noticed a huge improvement in japanese/chinese songs

the reason I included these changes in this PR, is because it would be "laggy" to do it w/o virtual scrolling
virtual scrolling allows us to lazily romanize the lyrics

--

before:
image

after:
image

--

before:
image

after:
image

@JellyBrick I decided to include a better romanization method in this PR No need to worry about tiny-pinyin anymore, I not only fixed the `w e i r d s p a c i n g` it has for English, but I massively improved the quality of the romanized lyrics by using `tinyld` and then as fallback I use the old detection methods, and I noticed a huge improvement in japanese/chinese songs the reason I included these changes in this PR, is because it would be "laggy" to do it w/o virtual scrolling virtual scrolling allows us to lazily romanize the lyrics -- before: ![image](https://github.com/user-attachments/assets/8809af70-b505-4008-a081-50d4de8208a3) after: ![image](https://github.com/user-attachments/assets/cdba9a5a-ea84-4ef5-ae66-d815cfe43d24) -- before: ![image](https://github.com/user-attachments/assets/a749e383-ac91-4ff1-843d-12c394a13e48) after: ![image](https://github.com/user-attachments/assets/26651d57-d509-4bc7-a77f-08def9b79bd1)
👍 1
JellyBrick (Migrated from github.com) requested changes 2025-06-30 03:44:33 +00:00
package.json
JellyBrick (Migrated from github.com) commented 2025-06-30 03:44:30 +00:00
Copy link
    "tinyld": "1.3.4",
```suggestion "tinyld": "1.3.4", ```
JellyBrick (Migrated from github.com) approved these changes 2025-07-01 07:21:03 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
JellyBrick
Labels
Clear labels   
awaiting-reply

Awaiting reply

  
breaking changes

   
bug

Something isn't working

  
cannot-reproduce

Bug cannot be reproduced

  
dependencies

Pull requests that update a dependency file

  
documentation

   
duplicate

This issue or pull request already exists

  
electron-issue

It's an Electron or Electron related dependencies issue (not YTM-Desktop issue)

  
enhancement

New feature or request

  
fix-available

A fix to the issue is available in a new version

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update javascript code

  
need more information

Need more information about the issue

  
need rebase

This plugin need rebase

  
official-youtube-music-issue

It's an YouTube's issue (not YTM-Desktop issue)

  
plugin request

An issue/discussion requesting for a new plugin or new functionality to an existing plugin.

  
question

Further information is requested

  
release

Release

  
security

   
stale

   
Status: blocked

There are something to do before this

  
typo

   
wontfix

This will not be worked on

  
ytmd-issue

An issue/discussion about ytm desktop, unrelated to th-ch/youtube-music

No labels
awaiting-reply
breaking changes
bug
cannot-reproduce
dependencies
documentation
duplicate
electron-issue
enhancement
fix-available
good first issue
help wanted
invalid
javascript
need more information
need rebase
official-youtube-music-issue
plugin request
question
release
security
stale
Status: blocked
typo
wontfix
ytmd-issue
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: YTMD/youtube-music#3162
No description provided.
Delete branch "chore/synced-lyrics-virtual-scroll"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?