Tag editor improvement

[tramhao]

Tag editor can fetch tag, fetch lyrics, preview lyrics, delete lyrics now.

[raziman18]

I will review this tonight

[tramhao]

The code to fetch lyric in tag editor is very ugly as I cannot do it by calling lyricPopup. The problem is, lyricPopup is embeded with go routines, and I need to update content of Dropdown and textview based on the result from lyricpopup. When doing so, my code of updaing runs before getting lyrics. I tried to use channel or waitgroup, but both method end up with totally freezing. Finally I have to copy code from lyricPopup again. Do you have some suggestions? Thanks.

[raziman18]

I think lyric package should have a LyricOptions struct with generalized func (l *LyricOptions) GetLyric() string method to fetch a particular language based on specified Lang member on LyricOptions struct. This way, we can generalize the lyricPopup function to lyricPopup(audioFile player.Audio, langExt string, serviceProvider string) where lyricPopupCN is not needed anymore.

[raziman18]

The problem is, lyricPopup is embeded with go routines, and I need to update content of Dropdown and textview based on the result from lyricpopup.

If lyric package is refactored as I suggested above, you can just use the GetLyricOptions method.

[raziman18]

I think lyric package should have a LyricOptions struct with generalized func (l *LyricOptions) GetLyric() string method to fetch a particular language based on specified Lang member on LyricOptions struct.

I think GetLyric and GetLyricOptions should be method for Lyric struct.

[tramhao]

I think lyric package should have a LyricOptions struct with generalized func (l *LyricOptions) GetLyric() string method to fetch a particular language based on specified Lang member on LyricOptions struct. This way, we can generalize the lyricPopup function to lyricPopup(audioFile player.Audio, langExt string, serviceProvider string) where lyricPopupCN is not needed anymore.

But the problem here is, between GetLyricOptions and GetLyric, there is a manual selection step involved.

[tramhao]

I think lyric package should have a LyricOptions struct with generalized func (l *LyricOptions) GetLyric() string method to fetch a particular language based on specified Lang member on LyricOptions struct. This way, we can generalize the lyricPopup function to lyricPopup(audioFile player.Audio, langExt string, serviceProvider string) where lyricPopupCN is not needed anymore.

Also, the code to fetch en lyric and cn lyric are different.

[tramhao]

The problem is, lyricPopup is embeded with go routines, and I need to update content of Dropdown and textview based on the result from lyricpopup.

If lyric package is refactored as I suggested above, you can just use the GetLyricOptions method.

Actually I'm using GetLyricOptions directly right now. It's just I have to repeat similar code for 3 buttons, and it doesn't feel good.

[tramhao]

I think we have three problems here:

1. Inside lyric package, lyric.go and lyric_chinese.go should be modified in some way, but the code inside these two files are quite different.
2. In popup.go, lyricpopup and lyricpopupCN should be merged.
3. In tageditor, the 3 buttons to fetch lyrics are very similar and need to be simplified.

[raziman18]

1. Inside lyric package, lyric.go and lyric_chinese.go should be modified in some way, but the code inside these two files are quite different.

For every language, they should be in their own file, example for english lyricEn.go and they need to have a function getLyricEn() (string, error) and getLyricOptionsEn() (map[string]string, error).

Lyric package should expose GetLyric(lang string) string and GetLyricOptions(lang string) (map[string]string, error).
Example of GetLyric implementation might be:

func GetLyric(lang string) string {
    switch lang {
      case "jp": return getLyricJp()
      case "cn": return getLyricCN()
      default: return getLyricEn()
    }
}

This will make it easier to support more language.

[tramhao]

Yes and I've done it in the latest push. Now it feels much better.

[raziman18]

Is there any way to fix the color around border? This is more visible on desktop with light background.

[raziman18]

3. In tageditor, the 3 buttons to fetch lyrics are very similar and need to be simplified.

I think language option to fetch lyric should be dropdown instead of button and below the dropdown should have the "fetch" button.

[raziman18]

there is typo here LyridID

[tramhao]

Is there any way to fix the color around border? This is more visible on desktop with light background.

This is an issue with chinese letters displayed when border padding of flex is set to 0.

You can check the left side and it's annoying.

When border padding is set to 1:

It seems all right.

[raziman18]

I mean the transparent color around the line. Not the border padding itself. I didn't notice this before since I was using dark desktop background.

[tramhao]

I mean the transparent color around the line. Not the border padding itself. I didn't notice this before since I was using dark desktop background.

Probably this pr in tview is related

rivo/tview#523

[raziman18]

this pr doesn't even get merged rivo/tview#524

[tramhao]

how about this one? seems more active

[tramhao]

https://gitlab.com/tslocum/cview/-/commit/1af0da7606b8476944b5740bb4f0b711aaf2a1df

[raziman18]

We will try converting library to https://gitlab.com/tslocum/cview on a new pr. Let's just focus on merging current pr to the master branch.

[raziman18]

Are there anything else to be done on this pr?

[tramhao]

Let me figure out the color inside dropdown in a minute.

[tramhao]

Some colors cannot be set. So nothing to change for now. Thanks.

[raziman18]

If you done, please remove the codes that have been commented out.

[tramhao]

done.

[raziman18]

Thank you for your contribution! Looking forward for more pr from you 🚀