Skip to content

Conversation

robroid
Copy link

@robroid robroid commented Sep 19, 2025

hello everyone, this is my first time ever contributing code, lol.

the new enhanced line effect is inspired by better-lyrics.
when i first saw their animation, i wanted it in this plugin too, so i spent about a week replicating and improving it. the effect should now feel smoother and closer to that experience.

along the way, i also fixed some related issues with providers and others (details below).

about

  1. new enhanced line effect
  2. improved character between lyrics
  3. "•••" and "..." characters now fade with a brief pause at the end, preparing for the next lyric line
  4. set enhanced line effect and "•••" character as defaults
  5. option to always show or hide the character, except on the current line
  6. make the lyrics perfectly synced option is now precise (it was buggy before)
  7. time codes now align correctly with make the lyrics perfectly synced option
  8. empty lines from lrclib now display correctly, and consecutive empty lines are merged consistently across lrclib, youtube, and musixmatch

screenshot

Screenshot_938 Screenshot_939

video

enhanced1.mp4
enhanced2.mp4
features.mp4

note: these changes only apply to the enhanced style, not the others.

fancy.mp4

what do you think? should i also apply this to the other styles, or keep it exclusive to enhanced?

@Heckmxxn
Copy link

damn this looks amazing, also seems to be really smooth, absolutely fantastic work!

@kimjammer
Copy link
Contributor

Hi! Nice work, I'm the person that originally added the Fancy line effect. You might have already known, but the purpose of the Fancy line effect was to recreate the effects from Better Lyrics anyway. So in my opinion, instead of adding a new but slightly different version, Fancy should just become what you have with Enhanced. I would defer to what the maintainers think though.

@robroid
Copy link
Author

robroid commented Sep 27, 2025

Hi! Nice work, I'm the person that originally added the Fancy line effect. You might have already known, but the purpose of the Fancy line effect was to recreate the effects from Better Lyrics anyway. So in my opinion, instead of adding a new but slightly different version, Fancy should just become what you have with Enhanced. I would defer to what the maintainers think though.

hey @kimjammer, and thanks! i really appreciate you creating the fancy effect in the first place, it made things a lot easier for me and really helped when adding the enhanced style. i thought about merging it with fancy, but i decided to keep them separate for now because i thought some people might still enjoy the original fancy style. i figured it’s better to let people choose which effect feels best for them. so for now enhanced is its own option (but default).

if the maintainers feel it makes more sense to combine fancy and enhanced, i’m happy to adjust 👍

@Windowstechtips
Copy link

Hi any way to either bring Word synced lyrics or just put that plugin into Youtube Music bundled?

@ArjixWasTaken
Copy link
Member

ArjixWasTaken commented Oct 2, 2025

This is amazing!

But, you will need to clean up the code a bit.
There is a lot of code that is repeated instead of being extracted into a function, e.g. for merging blank lines together

Once you've done that, I'll see if there is a need for a more in-depth review

@robroid
Copy link
Author

robroid commented Oct 2, 2025

thanks @ArjixWasTaken, i cleaned up the repeated code and moved it into shared functions, should be much cleaner now, let me know what you think👍🏻

@ArjixWasTaken
Copy link
Member

Amazing work, love what you did to the lyric scrolling.
But, you made a breaking change...

the default text string behaves differently after your changes
before you could do ascii art, and have a different kaomoji for each state, now all states are visible at once, and get highlighted as time passes

Before your PR:

ytmd-pr-before.mp4

After your PR:

ytmd-pr-after.mp4

@ArjixWasTaken
Copy link
Member

Not only that, if you intended to break the previous functionality, you did not write a config migration for ['•', '••', '•••'] -> ['•', '•', '•']

@ArjixWasTaken
Copy link
Member

Maybe instead of an array of states, it should be an object that declares if it's an animation (array of frames) or an array of characters to get highlighted

@ArjixWasTaken
Copy link
Member

Do you mind adding me to your fork so we can work on this together?

@robroid
Copy link
Author

robroid commented Oct 3, 2025

Not only that, if you intended to break the previous functionality, you did not write a config migration for ['•', '••', '•••'] -> ['•', '•', '•']

Maybe instead of an array of states, it should be an object that declares if it's an animation (array of frames) or an array of characters to get highlighted

yeah, i actually tried to keep the ['•', '••', '•••'] behavior, but i ran into duplication problems when doing the fade in animation. it would render like •••••• instead of •••. since i could'n’t find a good solution, i simplified it to ['•', '•', '•'], which worked better for per character effects. i agree this does change the behavior. i wasn’t trying to intentionally break ascii art/kaomoji configs, i just couldn’t find a clean way to support both at the time. please feel free to take a look and sure, i’ll add you to my fork so we can work on this together.

@robroid
Copy link
Author

robroid commented Oct 4, 2025

Amazing work, love what you did to the lyric scrolling. But, you made a breaking change...

the default text string behaves differently after your changes before you could do ascii art, and have a different kaomoji for each state, now all states are visible at once, and get highlighted as time passes

hey, just pushed the updated version. the behavior’s fixed now, arrays with different values (like kaomojis/ascii art) only show the current state, and identical values (like "•••") use the fade animation on each state.
the migration also maps old configs automatically (['•','••','•••'] -> ['•','•','•'], etc.), so everything should work as expected now.

fixascii.mp4

Maybe instead of an array of states, it should be an object that declares if it's an animation (array of frames) or an array of characters to get highlighted

regarding the idea of using an object to separate animation and highlight states, it’s a good idea, but i think the current approach already covers both cases cleanly without adding extra complexity.
if you’d like to try it out, feel free to modify the code :)

@robroid
Copy link
Author

robroid commented Oct 4, 2025

added a small helper for the end delay empty line, works the same as before, just cleaner and easier to tweak later if someone wants to modify it.

@ArjixWasTaken ArjixWasTaken requested a review from Copilot October 4, 2025 22:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces an enhanced line effect for synced lyrics with smoother animations, improves character display between lyrics, and fixes various provider handling issues.

  • Adds new "enhanced" line effect with refined animations inspired by better-lyrics
  • Improves handling of empty lines and character display with fade effects and better timing
  • Fixes provider handling, time alignment, and empty line processing across all providers

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/plugins/synced-lyrics/types.ts Adds new enhanced line effect type and showEmptyLineSymbols config
src/plugins/synced-lyrics/style.css Extensive styling updates for enhanced line effect and hover animations
src/plugins/synced-lyrics/shared/lines.ts New utility functions for merging empty lines and ensuring proper line padding
src/plugins/synced-lyrics/renderer/utils.tsx Adds utilities for time formatting, blank detection, and font loading
src/plugins/synced-lyrics/renderer/scrolling.ts New scrolling constants and utilities for enhanced scroll behavior
src/plugins/synced-lyrics/renderer/renderer.tsx Major refactor with enhanced scroll animations and improved line effect handling
src/plugins/synced-lyrics/renderer/components/SyncedLine.tsx Enhanced empty line handling with fade effects and improved timing
src/plugins/synced-lyrics/renderer/components/LyricsPicker.tsx Improved provider switching with fast scroll integration
src/plugins/synced-lyrics/providers/*.ts Updated all providers to use new line processing utilities
src/plugins/synced-lyrics/parsers/lrc.ts Improved LRC parsing with better millisecond handling
src/plugins/synced-lyrics/menu.ts Menu restructuring with new enhanced option and improved organization
src/plugins/synced-lyrics/index.ts Updated default configuration for enhanced experience
src/i18n/resources/en.json Added translations for new enhanced effect and empty line symbols
src/config/store.ts Migration logic for placeholder default changes

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 92 to 97
const stepCount = states().length;
const precise = config()?.preciseTiming ?? false;

if (stepCount === 1) return 0;

const endDelayMs = computeEndDelayMs(total);
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

The computeEndDelayMs(total) function is called on every render when stepCount > 1. Consider memoizing this calculation since it only depends on the total parameter which comes from props.line.duration.

Copilot uses AI. Check for mistakes.

GGG-KILLER added a commit to GGG-KILLER/nixos-configs that referenced this pull request Oct 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants