search

issadarkthing / gomu

golang TUI music player
GNU General Public License v2.0
159 stars 15 forks source link

Fix race conditions #51

Closed tramhao closed 3 years ago

tramhao commented 3 years ago

Trying to fix race conditions in two places:

  1. player.go , ctrl and format need to lock before access
  2. playingbar.go, run() and newprogress() are accessing same values but run is in goroutines.

Please kindly check if the code make sense, or better solution you thought of.

Currently I don't have race conditions detected during several operation like skip, forward, download lyric etc.

issadarkthing commented 3 years ago

Why does the playlist_search command freezes the whole application?

tramhao commented 3 years ago

should be ok now. app.draw() seems not working in searchpopup.

tramhao commented 3 years ago

New problem when delete app.draw from searchpopup: in tageditor, the searchpopup is not shown unless any key is pressed.

tramhao commented 3 years ago

search popup is ok now, by adding app.draw after calling searchpopup in tageditor and lyricpopup.

issadarkthing commented 3 years ago

When queue is empty and no songs are playing, adding song to the queue does not trigger player to play music

tramhao commented 3 years ago

When queue is empty and no songs are playing, adding song to the queue does not trigger player to play music

fixed in new commits below.

issadarkthing commented 3 years ago

fixed in new commits below.

It does work for single song but does not work when adding the whole directory to the queue.

tramhao commented 3 years ago

fixed in new commits below.

It does work for single song but does not work when adding the whole directory to the queue.

Fixed in above commit. Not very sure of the progress. I separate the play from enqueue, because the logic is very complicated when mix them together.

tramhao commented 3 years ago

I'm kind of lost here. When I'm playing a song, and meanwhile, paste it to other folder, always there will be duplicated times generated in queue. I checked and found probably it's because I savequeue first and clearqueue then loadqueue, and this procedure save the current playing song in the queue, meanwhile it's still playing, so if I skip it, there will be 2 songs with the same name in queue. I tried to deleteItem by index of len(q.items)-1, but always delete more than I need. Do you have some idea about it? The queue handling is quite complicated, considering paste and rename, need to update related info in queue and current playing song. Do you think there are better way to handle it? It's just not so straight forward.

issadarkthing commented 3 years ago

Why does yanking and pasting adds new song to the queue? Is it the intended behaviour?

tramhao commented 3 years ago

Why does yanking and pasting adds new song to the queue? Is it the intended behaviour?

that's a problem. what I want to do is, to update the path if it's in queue. I did by savequeue,clearqueue and loadqueue.

tramhao commented 3 years ago

Why does yanking and pasting adds new song to the queue? Is it the intended behaviour?

I fixed it by adding a new function to update path in queue. Meanwhile, still a bug exists, that if the pasted file is current playing song, weird actions happened in queue. Randomly duplicated items. Maybe there are some underlying bug.

tramhao commented 3 years ago

Fixed but not very sure if it's implemented correctly. Too many things are involved:

  1. current song is not in queue, so need to handle differently
  2. findAudiofile in playlist.go use filename to find, so rename need different handling with paste.
  3. when rename and paste, folder and file need to handle differently.
issadarkthing commented 3 years ago

I separate the play from enqueue, because the logic is very complicated when mix them together.

I agree with this

tramhao commented 3 years ago

I'll not add new features to this PR, but right now I'm stucked in the syncronization of playlist and queue. The whole process is very complicated. What do you think if we could have a function that syncronize the queue with playlist changes like add, delete, paste, rename etc and is clear so that no errors will happen? Is there an on_change event of playlist?

tramhao commented 3 years ago

Finally I clear my head and fix the issues in this PR. I think current code is all right and please check when you have time. No rush. Meanwhile, I would like to ask, do you know if there is any library to show images? As now the photos of each song is already embeded into mp3 after download, it'll be nice to show them somewhere.

issadarkthing commented 3 years ago

Is there an on_change event of playlist?

I agree on adding an on_change event as it would really help to generalize the synchronization code and makes the codebase cleaner.

issadarkthing commented 3 years ago

I would like to ask, do you know if there is any library to show images?

I can't find one as Go library, but as an external program ueberzug, w3mimgdisplay and pxl may seem fit. You may want to view ranger on how they handle image preview in the terminal.

issadarkthing commented 3 years ago

There's a bug where current song is already finished playing and the queue is empty, exiting and entering the app again will automatically play previously played song even though the song was already finished playing. This bug was already fixed in the previous commit b4ba187be75d75a65fb47b6629f5ac266e1ad38a but it seems to reappear again.

tramhao commented 3 years ago

I would like to ask, do you know if there is any library to show images?

I can't find one as Go library, but as an external program ueberzug, w3mimgdisplay and pxl may seem fit. You may want to view ranger on how they handle image preview in the terminal.

Follow your idea, I found a package ueberzug-go, and get it work as expected. I tried to do it in a separate PR, but didn't work, as the PR includes all unmerged changes, so I merge it with current PR. Fortunately all modification for this to work is inside playingbar.go.

tramhao commented 3 years ago

There's a bug where current song is already finished playing and the queue is empty, exiting and entering the app again will automatically play previously played song even though the song was already finished playing. This bug was already fixed in the previous commit b4ba187 but it seems to reappear again.

I don't know why I comment it out but now I revert the change so it should be ok.

issadarkthing commented 3 years ago

I don't know why I comment it out but now I revert the change so it should be ok.

The bug hasn't been fixed.

issadarkthing commented 3 years ago

Follow your idea, I found a package ueberzug-go, and get it work as expected. I tried to do it in a separate PR, but didn't work, as the PR includes all unmerged changes, so I merge it with current PR. Fortunately all modification for this to work is inside playingbar.go.

Could you add some documentation to the README.md regarding song thumbnail? I can't get it to work. How can someone add thumbnail to a song? How does a song gets its thumbnail?

tramhao commented 3 years ago

Could you add some documentation to the README.md regarding song thumbnail? I can't get it to work. How can someone add thumbnail to a song? How does a song gets its thumbnail?

For songs downloaded from youtube, the thumbnail of video will be embeded into the song by ytdl. For others, could be added by kid3 as album cover.

tramhao commented 3 years ago

I don't know why I comment it out but now I revert the change so it should be ok.

The bug hasn't been fixed.

Wierd, I tested with no loop mode, and the steps are:

  1. First add some songs
  2. skip all until last song.
  3. Forward it to the end, and finish it.
  4. quit app and reopen app.
  5. Now the queue is empty and no song is playing. Is this the expected behavior?
issadarkthing commented 3 years ago

image

Why does the position of the thumbnail seems like a bit off. Could you make it center? My terminal is alacritty 0.7.2

issadarkthing commented 3 years ago

The bug hasn't been fixed.

My bad, the bug was actually fixed. I didn't pull the latest commit from the upstream.

tramhao commented 3 years ago

image

Why does the position of the thumbnail seems like a bit off. Could you make it center? My terminal is alacritty `0.7.2

very strange that when I test in alarcitty, and st, the position is not the same. I set position like this 

p.albumPhoto, err = ugo.NewImage(dstImage128, (x+3)*windowWidth/width, (y+2)*windowHeight/height)

In st it's centered vertically but in alacritty it's below center position...

issadarkthing commented 3 years ago

I figured out that it is because I have paddings in my alacritty config. The problem went away after disabling them.