hakerdefo / cmus-lyrics

Fetches lyrics of current song playing in cmus music player.
Creative Commons Zero v1.0 Universal
48 stars 10 forks source link

Auto-update on user-initiated song change #6

Closed JettScythe closed 4 years ago

JettScythe commented 4 years ago

Expected behaviour: auto-refresh when user changes songs (b using cmus), reload & display lyrics

What happens: Program hangs for indefinite amount of time. Will occasionally update lyrics for playing song, though usually continues to display lyrics for the last song played.

hakerdefo commented 4 years ago

Hello @JettScythe, Auto-refresh lyric view upon a song change requires constant monitoring (0.5 seconds to 1.0 seconds) and this constant watch utilises system resources. This is the reason why I've not implemented this feature in cmus-lyrics thus far. You might have noticed following line at the bottom of lyric view, PRESS "Q" TO REFRESH THE VIEW & DISPLAY NOW PLAYING LYRICS. In my view it's simpler and more efficient to press Q manually to fetch-view the current lyrics upon a song change than the non-stop polling required to monitor a song change. Auto-refresh can be implemented but is it necessary? Let me know what you think. Cheers!!!

hakerdefo commented 4 years ago

@JettScythe And yes sorry for the belated response! Cheers!!!

Kabouik commented 4 years ago

I also believe auto-update would be useful (not only for user-initiated song changes), maybe as a command line option to keep the default behaviour with lighter resources use. I think it would be particularly useful in a tmux layout with cmus and cmus-lyrics side by side.

This would make cmus-lyrics a good competitor to lyvi. lyvi aims at doing more things (chords, lyrics, artist bio, and covers although I never understood how to enable it) while cmus-lyrics is meant to be lightweight and minimal, but still I think the comparison is not irrelevant. I believe having lyrics always up to date without user input would suit several users' needs. Artist bio is also an interesting option, but that would probably be difficult and a bit out of scope for cmus-lyrics.

hakerdefo commented 4 years ago

@JettScythe @Kabouik How can one ignore popular demand?!? Here is an experimental version that refreshes lyrics automatically when a song changes in CMus, cmus-lyrics-auto-refresh I've also updated the README file to reflect this information. THE ABOVE IS A HISTORY NOW. PLEASE IGNORE. Cheers!!!

Kabouik commented 4 years ago

Awesome, thanks for considering this! I will try it later tonight. Can you elaborate why it is recommended to stick to the base version, is there any security concern with the auto-refresh hack?

[Edit] From what I can understand, the hack is making cmus-lyrics for the song duration, and then fetches new lyrics, this looks safe. Perhaps not going to refresh on songs skipped by user though.

hakerdefo commented 4 years ago

@Kabouik No security risks just that it was untested! But all that is a history now as I've updated main version of cmus-lyrics and it should now refresh lyrics for normal song change as well as user initiated song change. Please try it and let me know. @JettScythe Thanks for inspiring this change! Cheers!!!

JettScythe commented 4 years ago

This works exactly as I was expecting! Thank you for the quick addition :)

Kabouik commented 4 years ago

That's perfect!

hakerdefo commented 4 years ago

@JettScythe and @Kabouik Thanks for your feedback and suggestions! Cheers!!!

Kabouik commented 4 years ago

I found two songs that make it hang though, instead of displaying that no lyrics can be found. Then it stays stuck in that state even when changing song. Do you want me to send you those songs over email if you are interested in checking what is not handled correctly by the script?

hakerdefo commented 4 years ago

Sure. That'll be fine.

hakerdefo commented 4 years ago

@Kabouik I think I have fixed the problem described by you. Please test the updated version and let me know. Thanks again for your help in improving cmus-lyrics. Cheers!!!

Kabouik commented 4 years ago

No, thank you for making it and improving it!

Unfortunately the issue with cmus-lyrics hanging on some files still happens for me, and I also noticed bigger issues on other files (not sure if that was happening with the previous version too, I didn't play those files earlier today):

$ cmus-lyrics 
/tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
grep: /tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
cat: '/tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX'$'\n''A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ'$'\n''N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH'$'\n''E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF'$'\n''OJWWC3TO': File name too long
/usr/local/bin/cmus-lyrics: line 253: /tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
/usr/local/bin/cmus-lyrics: line 254: /tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
/usr/local/bin/cmus-lyrics: line 255: /tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
cat: '/tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX'$'\n''A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ'$'\n''N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH'$'\n''E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF'$'\n''OJWWC3TO': File name too long
/tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
grep: /tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
cat: '/tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX'$'\n''A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ'$'\n''N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH'$'\n''E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF'$'\n''OJWWC3TO': File name too long
/usr/local/bin/cmus-lyrics: line 253: /tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
/usr/local/bin/cmus-lyrics: line 254: /tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
/usr/local/bin/cmus-lyrics: line 255: /tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX
A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ
N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH
E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF
OJWWC3TO: File name too long
cat: '/tmp/cmus_lyrics//IRSXM2LMEBHXEICBNZTWK3CMN52SARDPNFWGY33OHMQMHCLUNFSW43TFEBCGC2DPHMQFA2DJNRUX'$'\n''A4DFEBCW45DSMVZXGYLOM5WGKOZAJVQXEY3FNRWG6ICHNF2WY2LBNZUTWICGOJQW5Q5HN5UXGICQ'$'\n''N5TWO2LPHMQEC3DFPBUXGICBNZSXE2LMNRSXGOZAJRQXK4TFNZ2CAQTBOJSGC2LONZSTWICGMFRH'$'\n''E2LDMUQE2YLSORUW4ZL2HMQFI2DPNVQXGICEMUQFA33VOJYXKZLSPE5SARDBNZUWK3BALJUW23LF'$'\n''OJWWC3TO': File name too long
^C
hakerdefo commented 4 years ago

@Kabouik The problem you described here happened because ext family (ext2, ext3 & ext4) file systems limit maximum filename length to 255 bytes and filenames created by cmus-lyrics in your case were longer than that limit. I've updated cmus-lyrics to fix this issue. Thanks for your valuable feedback. Cheers!!!

Kabouik commented 4 years ago

Thanks for the quick fix again @hakerdefo! I can confirm the issue with long file names no longer occurs with the same files, and files which were initially making the script hang (with no output) now correctly report that no lyrics were found. I don't know what was wrong with the latter files, because some other files with no lyrics found were just showing it in cmus-lyrics' output.

I have many files for which no lyrics can be found unfortunately, but I assume it's a limitation of the makeitpersonal database. I was previously using lyvi which relies on glyr to find lyrics, and it probably used a different datatabase.

hakerdefo commented 4 years ago

@Kabouik I've updated cmus-lyrics and this update should result in a slight performance enhancement, hopefully. Do test this one and give your valuable feedback please. And you are right about missing lyrics, there is nothing I can do there. You can contact @febuiles on the issue of missing lyrics. A great guy, he'll sure help you. Glyr scrapes 30 websites for lyrics so naturally its success rate is bound to be high! BTW I wrote the very first version of cmus-lyrics after a failed attempt to build Lyvi on a Debian system. Cheers!!!

Kabouik commented 4 years ago

Will test, thanks again @hakerdefo.

Yeah, glyr is clearly powerful. It's nice that it always finds some lyrics. Funnily enough, I managed to build lyvi on my SailfishOS phone with the glyr dependency, but failed on a full fledged desktop distribution. :/

febuiles commented 4 years ago

@Kabouik thanks for bringing glyr to my attention.

makeitpersonal uses a single source to fetch the lyrics. I opened an issue to investigate running glyr as a service, but I haven't really worked in this codebase for years now. If I end up doing something about this, I'll make sure @hakerdefo is aware of the changes :)

Kabouik commented 4 years ago

Oh that would be truly awesome @febuiles, thanks for considering it!