-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(synced-lyrics): lyrics offset #3240
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/i18n/resources/en.json: Language not supported
Comments suppressed due to low confidence (1)
src/plugins/synced-lyrics/menu.ts:17
- [nitpick] The label and tooltip keys for the lyrics offset feature should clearly indicate its behavior. Consider updating the translation text to explicitly mention that a positive offset delays lyric display (lyrics show slower than audio) and a negative offset accelerates them, ensuring consistency with the feature description.
label: t('plugins.synced-lyrics.menu.offset.label'),
Hello @scratchusernamemrtbts , would you be interested in inviting me to your fork so I can enhance this further? I could of course do that on a follow-up PR, this ain't a dealbreaker |
i'm so sorry for just seeing this now |
I am quite satisfied with an implementation like this, because it allows for more complex options in the lyrics picker, w/o bloating the UI. picker-offset.mp4what do you think? (ofc the CSS has to be improved) |
not sure if the three dots provide a clear indication that it's for lyrics offset |
The three dots are not for the offset, but for "advanced options"
|
PS: There is a weird bug when the lyricsOffset is updated using the original menu. |
apparently the counter prompt type doesn't handle negative numbers correctly so I've also changed the prompt type to a normal input instead |
Not sure if this is the bug your talking about but i made the ui update when changing the config through the menu prompt |
type: 'input', | ||
inputAttrs: { | ||
type: 'number', | ||
style: 'text-align: center; width: unset;' as unknown as CSSStyleDeclaration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also if anyone's got a better solution than this please lmk
I will be rebasing on master later this Edit: 😭 |
Some slight CSS changes still need to be done after the rebase. @scratchusernamemrtbts can you convert this into a draft for now? |
Resolves #3237
please give me feedback on the labels and tooltips cus i am not a native english speaker
also right now positive offset means that lyrics show slower than the audio (used with bluetooth speakers which i think is the best usecase for this feature rn)
and negative might be used when the lyrics it pulled timestamp is offset slower than the actual song
might want to swap that but lmk which is best