tiliado / nuvolaplayer

Nuvola Player - Tight integration of web-based media streaming services with your desktop
https://nuvola.tiliado.eu/
BSD 2-Clause "Simplified" License
334 stars 27 forks source link

Add gecimi lyrics fetcher #602

Closed cognition9144 closed 4 years ago

cognition9144 commented 4 years ago

Since nuvola can't fetch lyrics of almost all Chinese songs, I add a new lyrics fetcher fetching from gecimi's API.

Not really tested because it's hard to compile. Locally it warns

File /usr/share/vala/vapi/libsoup-2.4.vapi is read-only; trying to patch anyway
patching file libsoup-2.4.vapi (read from /usr/share/vala/vapi/libsoup-2.4.vapi)
Hunk #1 FAILED at 148.
1 out of 1 hunk FAILED -- saving rejects to file libsoup-2.4.vapi.rej
Build failed
 -> task in 'libsoup-2.4.vapi' failed with exit status 1: 
        {task 139675452411344: libsoup-2.4.vapi libsoup-2.4.vapi,libsoup-2.4.patch -> libsoup-2.4.vapi}
' patch -i /home/xcffl/Development/nuvolaruntime/vapi/libsoup-2.4.patch -o libsoup-2.4.vapi /usr/share/vala/vapi/libsoup-2.4.vapi '

On Circle CI it has Errors during downloading metadata for repository 'updates-modular'... Well looks like Circle CI just has an outage. I'll test it later.

cognition9144 commented 4 years ago

Well it's impossible to build since vala lint requires vala 0.46, but the what you can get from vala's git repo is v0.48

cognition9144 commented 4 years ago

I think my part should work now. However, I don't have that many resources to clone cef, which is required for building valacef. Besides, circleci appears to have some issue on Diorite. So maybe the maintainer could consider building it and see if it works?

jiri-janousek commented 4 years ago

I think my part should work now. However, I don't have that many resources to clone cef, which is required for building valacef.

You can use Nuvola CDK.

Note that Nuvola is not in very good shape since I started working fulltime in April 2019. There are a lot of rough edges. Less time, less love :-(

jiri-janousek commented 4 years ago

I created an issue to fix CI - tiliado/nuvolaruntime#603

cognition9144 commented 4 years ago

You can use Nuvola CDK.

Wow, that's a perfect use case for flatpak. I did miss docker when I fell into the trap of cef.

But still, it lacks something

Traceback (most recent call last):
  File "/**/nuvola/valacef/valacefgen/cparser.py", line 77, in parse_header
    with open(path) as f:
FileNotFoundError: [Errno 2] No such file or directory: '/app/include/cef/include/capi/cef_audio_handler_capi.h'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "../genvalacef.py", line 125, in <module>
    parser.parse_header(path, c_include_path)
  File "/**/nuvola/valacef/valacefgen/cparser.py", line 85, in parse_header
    raise ParseError(self.context) from e
valacefgen.cparser.ParseError: [('header', '/app/include/cef/include/capi/cef_audio_handler_capi.h', 'capi/cef_audio_handler_capi.h')]
jiri-janousek commented 4 years ago

Use valacef branch 75.3770.x.

cognition9144 commented 4 years ago

~Wow... so finally I realize that there is another copy of Lyric component in https://github.com/tiliado/nuvolaplayer.git that really takes effect. This copy in nuvolaruntime seems simply redundant (if I'm wrong please tell me). Should I close this PR and send another one under nuvolaplayer?~

Ok, it's just its old name

cognition9144 commented 4 years ago

Ok, I managed to build and test this patch. It works. Although I realize that the API only support relatively old songs. Maybe I'll have to write another for mojim.com.

So basically this PR is ready to be merged.

jiri-janousek commented 4 years ago

I'm sorry I haven't managed to get to your PR for so long. I hope to look at it tomorrow though. You can add another lyrics fetcher if you wish :-)

cognition9144 commented 4 years ago

Thanks. I'm already using it anyway.

jiri-janousek commented 4 years ago

Ok, since you already use your plugin, I'm going to fix CI first and then I'll return to the PR.

jiri-janousek commented 4 years ago

CI is fixed. Could you rebase on the master branch and push again? It should run CI again. Thanks :-)

cognition9144 commented 4 years ago

That's great.

Thanks to the recipe, I managed to correct the linting problem. Hopefully Vala could be more supported in the future. For this reason, I added a commit containing config of the recommended uncrustify formatter according to https://github.com/tiliado/valalint/blob/master/.valalint.conf . It always a good thing when the formatting could be done automatically.

cognition9144 commented 4 years ago

Since the formatter lacks of the ability to correctly format anonymous functions (https://github.com/uncrustify/uncrustify/issues/2779), I've remove that commit. Maybe you can add a matched formatting config later.

jiri-janousek commented 4 years ago

Also, please don't mark comments as resolved. The reviewer does that.

cognition9144 commented 4 years ago

Seems CI fails because of some other reasons?

jiri-janousek commented 4 years ago
/root/workdir/nuvolaruntime/src/nuvolakit-runner/components/lyrics/GecimiLyricsFetcher.vala:50.9-50.38:
error: 1 missing arguments for `void Soup.Session.queue_message (owned Soup.Message, Soup.SessionCallback?)'
        session.queue_message(request);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Compare with:

SourceFunc callback = fetch_lyrics.callback;
session.queue_message(request, () => {
    Idle.add((owned) callback);
});
yield;
cognition9144 commented 4 years ago

Incase  you didn't notice, that error was fixed

cognition9144 commented 4 years ago

Sure. I've been busy these days. I think I'll have some time next week

-------- Original Message -------- On May 31, 2020, 9:11 PM, Jiří Janoušek wrote:

@fenryxo commented on this pull request.


In src/nuvolakit-runner/components/lyrics/GecimiLyricsFetcher.vala:

+

  • / Get lrc content /
  • string lrc_url;
  • try {
  • lrc_url = get_lrc_url(response);
  • } catch (Error e) {
  • throw new LyricsError.PARSE_FAILED(@"Failed to handle responses for song $song from Gecimi Lyrics");
  • }
  • if (lrc_url == "") {
  • throw new LyricsError.NOT_FOUND(@"Song $song was not found on Gecimi Lyrics");
  • }
  • request = new Soup.Message("GET", lrc_url);
  • session.queue_message(request, () => {
  • Idle.add((owned) callback);

When you are ready, please add a comment to let me know :-)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

jiri-janousek commented 4 years ago

Merged, thanks :-)