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

WIP: feat(synced-lyrics): lyrics offset #3240

Draft
scratchusernamemrtbts wants to merge 9 commits from scratchusernamemrtbts/master into master
pull from: scratchusernamemrtbts/master
merge into: YTMD:master
Conversation 13 Commits 9 Files changed 9 +242 -23
scratchusernamemrtbts commented 2025-04-15 14:55:06 +00:00 (Migrated from github.com)
Copy link

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

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
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-04-16 14:10:33 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy link

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'),
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments. <details> <summary>Files not reviewed (1)</summary> * **src/i18n/resources/en.json**: Language not supported </details> <details> <summary>Comments suppressed due to low confidence (1)</summary> **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'), ``` </details>
arjix commented 2025-04-21 22:12:20 +00:00
Owner
Copy link

Hello @scratchusernamemrtbts , would you be interested in inviting me to your fork so I can enhance this further?
I would like to add a way of changing the offset directly from the lyrics panel, instead of having to go to the menu settings.

I could of course do that on a follow-up PR, this ain't a dealbreaker

Hello @scratchusernamemrtbts , would you be interested in inviting me to your fork so I can enhance this further? I would like to add a way of changing the offset directly from the lyrics panel, instead of having to go to the menu settings. --- I could of course do that on a follow-up PR, this ain't a dealbreaker
👍 1
scratchusernamemrtbts commented 2025-05-02 14:15:36 +00:00 (Migrated from github.com)
Copy link

Hello @scratchusernamemrtbts , would you be interested in inviting me to your fork so I can enhance this further? I would like to add a way of changing the offset directly from the lyrics panel, instead of having to go to the menu settings.

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've just invited you to my fork now

> Hello @scratchusernamemrtbts , would you be interested in inviting me to your fork so I can enhance this further? I would like to add a way of changing the offset directly from the lyrics panel, instead of having to go to the menu settings. > > 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've just invited you to my fork now
arjix commented 2025-05-02 17:28:45 +00:00
Owner
Copy link

@scratchusernamemrtbts

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.

https://github.com/user-attachments/assets/b5fbeaf6-cc54-48eb-815e-675c93a420fc

what do you think?

(ofc the CSS has to be improved)

@scratchusernamemrtbts 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. https://github.com/user-attachments/assets/b5fbeaf6-cc54-48eb-815e-675c93a420fc what do you think? (ofc the CSS has to be improved)
👍 1
scratchusernamemrtbts commented 2025-05-04 17:59:49 +00:00 (Migrated from github.com)
Copy link

not sure if the three dots provide a clear indication that it's for lyrics offset
(maybe a timer symbol)

not sure if the three dots provide a clear indication that it's for lyrics offset (maybe a timer symbol)
arjix commented 2025-05-04 18:01:16 +00:00
Owner
Copy link

not sure if the three dots provide a clear indication that it's for lyrics offset (maybe a timer symbol)

The three dots are not for the offset, but for "advanced options"

because it allows for more complex options in the lyrics picker, w/o bloating the UI.

> not sure if the three dots provide a clear indication that it's for lyrics offset (maybe a timer symbol) The three dots are not for the offset, but for "advanced options" > > because it allows for more complex options in the lyrics picker, w/o bloating the UI.
arjix commented 2025-06-23 19:59:35 +00:00
Owner
Copy link

Final design I can come up with

image

some minor changes should be made so it feels less rough, but other than that I am satisfied with it

Final design I can come up with ![image](https://github.com/user-attachments/assets/d2baf6a3-1249-44b8-be81-db7e494cb6a3) some minor changes should be made so it feels less rough, but other than that I am satisfied with it
arjix commented 2025-06-23 20:00:29 +00:00
Owner
Copy link

PS: There is a weird bug when the lyricsOffset is updated using the original menu.

PS: There is a weird bug when the lyricsOffset is updated using the original menu.
scratchusernamemrtbts commented 2025-06-25 13:28:39 +00:00 (Migrated from github.com)
Copy link

apparently the counter prompt type doesn't handle negative numbers correctly so I've also changed the prompt type to a normal input instead

apparently the counter prompt type doesn't handle negative numbers correctly so I've also changed the prompt type to a normal input instead
scratchusernamemrtbts commented 2025-06-25 13:29:35 +00:00 (Migrated from github.com)
Copy link

PS: There is a weird bug when the lyricsOffset is updated using the original menu.

Not sure if this is the bug your talking about but i made the ui update when changing the config through the menu prompt

> PS: There is a weird bug when the lyricsOffset is updated using the original menu. Not sure if this is the bug your talking about but i made the ui update when changing the config through the menu prompt
scratchusernamemrtbts (Migrated from github.com) reviewed 2025-06-25 13:30:33 +00:00
src/plugins/synced-lyrics/menu.ts
@ -13,0 +35,4 @@
ctx.window,
);
if (!newOffset) return
ctx.setConfig({
scratchusernamemrtbts (Migrated from github.com) commented 2025-06-25 13:30:33 +00:00
Copy link

also if anyone's got a better solution than this please lmk

also if anyone's got a better solution than this please lmk
arjix commented 2025-07-02 11:22:27 +00:00
Owner
Copy link

I will be rebasing on master later this day week month, since I am the most familiar with the changes.

Edit: 😭

I will be rebasing on master later this ~~day~~ ~~week~~ month, since I am the most familiar with the changes. Edit: :sob:
👍 1
arjix commented 2025-07-08 22:34:43 +00:00
Owner
Copy link

Some slight CSS changes still need to be done after the rebase.

@scratchusernamemrtbts can you convert this into a draft for now?

Some slight CSS changes still need to be done after the rebase. @scratchusernamemrtbts can you convert this into a draft for now?
This pull request has changes conflicting with the target branch.
  • src/plugins/synced-lyrics/menu.ts
  • src/plugins/synced-lyrics/renderer/components/LyricsPicker.tsx
  • src/plugins/synced-lyrics/renderer/index.ts
  • src/plugins/synced-lyrics/types.ts
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin scratchusernamemrtbts/master:scratchusernamemrtbts/master
git switch scratchusernamemrtbts/master

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master
git merge --no-ff scratchusernamemrtbts/master
git switch scratchusernamemrtbts/master
git rebase master
git switch master
git merge --ff-only scratchusernamemrtbts/master
git switch scratchusernamemrtbts/master
git rebase master
git switch master
git merge --no-ff scratchusernamemrtbts/master
git switch master
git merge --squash scratchusernamemrtbts/master
git switch master
git merge --ff-only scratchusernamemrtbts/master
git switch master
git merge scratchusernamemrtbts/master
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
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#3240
No description provided.
Delete branch "scratchusernamemrtbts/master"

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?