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

Feature: Show and make reposts - integrate SoundCloud API v2 [in progress] #430

Merged
merged 1 commit into from
Dec 1, 2015
Merged

Feature: Show and make reposts - integrate SoundCloud API v2 [in progress] #430

merged 1 commit into from
Dec 1, 2015

Conversation

mradionov
Copy link
Contributor

Note: work is still in progress

Implementation of #291: show reposts and allow to make resposts.

As discussed in the issue, it is possible to work with reposts only using SoundCloud API v2. In this PR:

  • introducing new service to work with v2 API, similar to existing service
  • moving a "rate limit reached" dialog to a separate service to reuse it for both v1 and v2 API
  • modifying stream page to work with v2 API response: track object is almost the same, except it is stored in the track property of collection item (origin property for v1 API) and it does not have user_favorite prop, so it is now set by force.

Basically, I would love to hear some feedback if I am going in the right direction and may continue working on reposts using this base. I am not sure if the entire application should be migrated to v2 at once. Thanks!

@weblancaster
Copy link
Member

@mradionov ok cool, let me know when it's done and I will review and test. Thanks.

@mradionov
Copy link
Contributor Author

@weblancaster , I guess I am on the finish line, only some refactoring and a little bit more testing left. It would be great if you could take a look at the PR.

Summary of the last 4 commits:

  • show the name of the user who reposted a track next to the author name (like on website)
    image
  • because v2 API does not return number of likes in a /stream response, I had to add extra request to get this information to a stream controller (see https://github.com/mradionov/soundnode-app/commit/7d29c4da318907752b42b9061fb32f47efce348e#diff-87d5925d59e92ac41e726d4d402b7899R86); it's a bit weird, SC website makes these requests too
  • adding implementations of undocumented v1 API methods for creating and deleting reposts
  • adding icon to a track partial to create and delete reposts
    image
  • hiding repost icon for user's tracks, because it is not supported (making reposts of your own tracks)

I plan to do some refactoring (plus adding comment to code), making fixes according to your notes and eventually squashing the commits. Thanks!

@mradionov
Copy link
Contributor Author

@weblancaster , PR is finished from my side. Additional fixes from latest updates:

  • add "Repost" menu item to queue
  • mark reposted tracks on other pages (user profile tracks, tag tracks, search tracks, etc)
  • fix bug with loading next page on tags page

It's up to you to decide whether or not should it end up in the app because of v2 API and undocumented API

@weblancaster
Copy link
Member

Great @mradionov I'm going to look into this today.. thanks.

@weblancaster
Copy link
Member

Hey @mradionov the code looks good but I found two issues.

  1. Major tracks doesn't play.
  2. Playlist style is broken (screenshot attached)

screen shot 2015-11-19 at 2 39 06 pm 2

@mradionov
Copy link
Contributor Author

@weblancaster , thanks for the feedback. May I ask you what do you mean by "Major" tracks?

@weblancaster
Copy link
Member

I can't play any track.. so that's a major issue.

@mradionov
Copy link
Contributor Author

Oh, that's weird. By the way, I have kinda the same thing when I'm running the app using "nw" command, but when I build the app - tracks can be played again. It is not related to this PR though, I've discovered that from the beginning of me trying to work with a project, maybe you know what can cause this behavior? Can tracks be played in a built version of the app in your case regarding this PR? I have Ubuntu 14.04.

@weblancaster
Copy link
Member

I built but still not working.. when I go back to master branch songs play normally so must be something in this PR that broke that @mradionov

@mradionov
Copy link
Contributor Author

Yeah, I've found the reason, on my way to fix it. Thanks

@weblancaster
Copy link
Member

Great thanks.

@mradionov
Copy link
Contributor Author

Fixed those two bugs that you've mentioned.
It is a bit strange, that tracks from a stream page were not playing. I think I did not have this issue at a time I was working on the PR, it might be possible that SoundCloud did change it's API (stream_url prop is not in a track object any more for /stream response)... or maybe I've just missed it.
Anyway, I was thinking about enabling v2 API options under a flag (turned on/off on settings page) in case if things like that (track not playing) will occur, so user will be kinda ready that something might be broken.

@weblancaster
Copy link
Member

Great @mradionov thanks for your hard work..

ps: playlist still not looking exactly like before haha but it's minor this time.

weblancaster added a commit that referenced this pull request Dec 1, 2015
Feature: Show and make reposts - integrate SoundCloud API v2 [in progress]
@weblancaster weblancaster merged commit 3653ceb into Soundnode:master Dec 1, 2015
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.

2 participants