Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NicoNico Track #50

Merged
merged 18 commits into from
Dec 21, 2023
Merged

Fix NicoNico Track #50

merged 18 commits into from
Dec 21, 2023

Conversation

a9lim
Copy link
Contributor

@a9lim a9lim commented Dec 6, 2023

NicoNico removed their getflv API sometime this march. I implemented this blog post's solution and referenced this library's JSON repackaging.

@a9lim a9lim changed the title Fixed NicoNico Track Fix NicoNico Track Dec 6, 2023
@devoxin
Copy link
Member

devoxin commented Dec 6, 2023

I have many points to raise about this PR.

  1. Is an account necessary to stream tracks?
    1b. If it is, why is this source manager initialised with default values of null if it's unusable?
  2. It is advisable to use a ScheduledExecutor over a Timer.
  3. Parsed JSON responses should really be using the library-provided JsonBrowser class as it's a little more resilient to unexpected response changes.
  4. A lot of lines need breaking up into separate statements, for example, request.setEntity(new StringEntity(processJSON(loadVideoMainPage(httpInterface)).toString()));.
    4b. The monolithic function processJSON should also be split up into several lines rather than trying to create a JSONObject inside a JSONObject inside a JSONObject inside a ...
  5. If the sendHeartbeat function is detrimental to being able to stream a track, it should not be handled gracefully but instead stop the processing of the track as it would be unrecoverable.

@a9lim
Copy link
Contributor Author

a9lim commented Dec 6, 2023

Thank you so much for the fast response and feedback, so sorry about the issues with the code. I'm self-taught and first made the changes while modifying a music bot so I'm not too familiar with standard practice. I'll rewrite to to align with the feedback.

An account is not necessary to stream most tracks, and I haven't found a track that needs an account to access it. However, the niconico.py library still has a login feature, so I left it in as a precaution in case there's some scenario where it's needed.

@a9lim a9lim closed this Dec 6, 2023
@topi314 topi314 reopened this Dec 6, 2023
@topi314
Copy link
Member

topi314 commented Dec 6, 2023

There is no need to close this, you can simply push additional commits to this pr
We will squash this anyway

@a9lim
Copy link
Contributor Author

a9lim commented Dec 6, 2023

Thank you, this is my first pull request so I don't really know how stuff on that front works either. For point 4b, is there a recommended way on how I should be splitting up the JSON creation? Naively, I want to do something like
image
, but this feels like it still has the same issue. Thank you so much for your patience!

@a9lim
Copy link
Contributor Author

a9lim commented Dec 6, 2023

Tried using JsonBrowser as an alternative but am not sure if this is what the syntax should be looking like, let me know if this is acceptable:
image
Additionally, I'm a college student so I may be busy, but I will try to get this resolved as quickly as possible.

@a9lim
Copy link
Contributor Author

a9lim commented Dec 6, 2023

I've tried to implement the changes as advised, still unsure about processJSON but let me know what else I could do.

@JellyBrick
Copy link
Contributor

JellyBrick commented Dec 7, 2023

additional note: It resolves Walkyst/lavaplayer-fork#85

Copy link
Member

@topi314 topi314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please reformat your codestyle to match the rest of the codebase

@devoxin
Copy link
Member

devoxin commented Dec 10, 2023

I've tried to implement the changes as advised, still unsure about processJSON but let me know what else I could do.

You're on the right track with it but still looks extremely bulky. Is every field there really necessary for playback?

@a9lim
Copy link
Contributor Author

a9lim commented Dec 11, 2023

Seems like almost every field is necessary, I've tried to remove each of them in testing but only content_uri and priority can be omitted without the request 400'ing. I'll also join the discord, might be more convenient to discuss changes I should make there.

@a9lim
Copy link
Contributor Author

a9lim commented Dec 11, 2023

Copy link
Member

@topi314 topi314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also make getWatchUrl return a https link?

@devoxin
Copy link
Member

devoxin commented Dec 18, 2023

Done.

Copy link
Member

@topi314 topi314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should prob be fine now

@topi314 topi314 requested a review from devoxin December 21, 2023 00:32
@topi314 topi314 merged commit 53eb716 into lavalink-devs:main Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants