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

Saved Songs Download and User Authentication #1240

Merged
merged 32 commits into from
May 18, 2021
Merged

Saved Songs Download and User Authentication #1240

merged 32 commits into from
May 18, 2021

Conversation

AZMCode
Copy link
Contributor

@AZMCode AZMCode commented Mar 29, 2021

What's on the tin.

It does add a few packages, but allows the downloading of private playlists & full library download.
Still not working, but I'm nearly there.

@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 29, 2021

I believe this would solve #1234

spotdl/__main__.py Outdated Show resolved Hide resolved
Problem is how can I feed the access and refresh tokens to spotipy?
@ghost
Copy link

ghost commented Mar 29, 2021

Please keep your commit messages clear, for example the last commit message:

Problem is how can I feed the access and refresh tokens to spotipy?

Is that the issue you fixed or the one you need to fix? If it's the latter, just what did you put into the commit then?

Even with the title:

Finished implementing OAuth2.

It can mean a lot of things. It could be a redesign of the auth process or you implemented spotipy oauth (which is done by default) which leaves the question of why are the access tokens an issue anymore...

See point 2 at this subsection

P.S. I haven't read the code, I just wanna say that your commit messages should cover (a) why changes were made and (b) how do those changes deal with the issue. What changes are made are indicated by the git diff so you don't have to mention that part.

@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 29, 2021

It's the latter. I'm sorry for the slip, I'll keep commit messages more focused in future.

Over all these commits I've added a flag to spotdl, which is --user-authorization.

This triggers a different authorization flow, which is the Authorization Code Flow, which I've implemented up to the point where the access/refresh tokens are first fetched.

To keep things easy for the user, I've used pywebview to open the login screen, which is necessary for the Authorization Code Flow.

I'm currently having issues integrating my implementation of the OAuth flow and spotipy.

@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 29, 2021

I've also begun adding support for the "saved" keyword

If, when parsing arguments, the word "saved" is found in the place of a URL, spotipy will download the user's Saved Songs. That's if the user also has used the --user-authorization flag. Otherwise it will raise an Exception telling the user to use the flag.

- Turned get_tokens function into SpotifyUserAuth (custom Spotipy-compatible Auth Manager)
- SpotifyUserAuth now automatically refreshes access tokens before they expire
- Added function to utils.py: get_saved_tracks, which gets all saved tracks by the current user. Fails without --user-auth
- Added "saved" keyword, so that if it appears as a url in the command line, it downloads all saved tracks instead.
- Shut off logging by http.server instance
@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 29, 2021

There goes the functionality as far as I'm aware. Now I need to write the tests for literally 300 lines of new code. That's for tomorrow though. I'm guessing it will be kinda hard to write tests with webview though.

@ghost
Copy link

ghost commented Mar 30, 2021

One more note, Its a good practice to keep commits restricted to one-change-a-commit. If you have to rollback a commit and you've grouped multiple changes into a single commit you'll loose a lot of good work.

@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 30, 2021

Thanks, I'll keep that in mind

@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 30, 2021

I'll be the first to admit: I'm new to python, and even after reading through the existing tests, I can't seem to figure out how to test downloading private playlists and the saved songs library. All current tests check out though, so after reverting to the old API keys I'm opening this for review.

@AZMCode AZMCode marked this pull request as ready for review March 30, 2021 12:59
@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 30, 2021

Well I'm stumped. Tests run fine on my machine.
Also, one of the tests failed when installing dependencies. Ffmpeg again.

@xnetcat
Copy link
Member

xnetcat commented Mar 30, 2021

Well I'm stumped. Tests run fine on my machine.
Also, one of the tests failed when installing dependencies. Ffmpeg again.

Sometimes python3.8 checks are failing, you just have to re-run all jobs

@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 30, 2021

I don't believe the macos 3.8 test should be taking this long... 20 minutes in when other tests finished in 1

@xnetcat
Copy link
Member

xnetcat commented Mar 30, 2021

I don't believe the macos 3.8 test should be taking this long... 20 minutes in when other tests finished in 1

GitHub actions are not perfect

@xnetcat
Copy link
Member

xnetcat commented Mar 30, 2021

I think that pywebview is overkill for this task. We could open the default browser instead with webbrowser that comes included with stdlib

https://docs.python.org/3/library/webbrowser.html

@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 30, 2021

The reason I'm choosing webview over the browser is that it makes the experience seamless. If the client closes the login window, for example, we can detect that and raise an Exception. In any case, webview could be replaced by a browser if you really think it's best.
Edit: I'm also lobbying over at pywebview to make this process serverless, to make things cleaner.

@Silverarmor
Copy link
Member

Please don't send the .tox/ directory.
add it to the .gitignore maybe

@AZMCode
Copy link
Contributor Author

AZMCode commented Mar 30, 2021

Will do

@Silverarmor
Copy link
Member

ah I see #1233 adds it to gitignore

spotdl/__main__.py Outdated Show resolved Hide resolved
AZMCode and others added 2 commits April 1, 2021 16:36
Co-authored-by: Jakub <42355410+xnetcat@users.noreply.github.com>
@AZMCode AZMCode requested a review from xnetcat April 1, 2021 20:39
Co-authored-by: Jakub <42355410+xnetcat@users.noreply.github.com>
@AZMCode AZMCode requested a review from xnetcat April 1, 2021 20:56
Co-authored-by: Jakub <42355410+xnetcat@users.noreply.github.com>
Comment on lines +172 to +204
def get_saved_tracks() -> List[SongObj]:
'''
returns a `list<songObj>` containing Url's of each track in the user's saved tracks
'''

spotifyClient = SpotifyClient()

initialRequest = spotifyClient.current_user_saved_tracks()
librarySize = int(initialRequest["total"])
with ThreadPoolExecutor() as pool:
futures = []
for i in range(librarySize):
if i % 20 == 0:
futures.append(
pool.submit(
spotifyClient.current_user_saved_tracks,
offset=i
)
)
doneFutures = wait(futures)[0]
doneLists = map(lambda x: x.result()["items"], doneFutures)
songObjFutures = []
for currList in doneLists:
for songSimplified in currList:
songObjFutures.append(
pool.submit(
SongObj.from_url,
'https://open.spotify.com/track/' + songSimplified['track']['id']
)
)
doneSongObjsFutures = wait(songObjFutures)[0]
doneSongObjs = list(map(lambda x: x.result(), doneSongObjsFutures))
return doneSongObjs
Copy link
Member

@xnetcat xnetcat Apr 1, 2021

Choose a reason for hiding this comment

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

I don't know if we should run this in parallel (we might hit youtube rate limit faster if user has a lot of songs). What do you think @Silverarmor?

Copy link

Choose a reason for hiding this comment

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

If we run it in parallel, spotDl can no longer be run on termux. Neither will it provide a significant boost over asyncio.

Between parallel and asyncio. Asyncio is better.

/cc @aklajnert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I go back to sequential fetching then?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, just like the other functions in utils.py

Copy link
Member

@xnetcat xnetcat left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@AZMCode
Copy link
Contributor Author

AZMCode commented Apr 1, 2021

:D

@Silverarmor
Copy link
Member

Please pull from upstream/dev

spotdl/__main__.py Outdated Show resolved Hide resolved
spotdl/__main__.py Outdated Show resolved Hide resolved
xnetcat and others added 3 commits April 11, 2021 13:15
Co-authored-by: Silverarmor <23619946+Silverarmor@users.noreply.github.com>
Co-authored-by: Silverarmor <23619946+Silverarmor@users.noreply.github.com>
spotdl/__main__.py Outdated Show resolved Hide resolved
@AZMCode AZMCode requested a review from aklajnert April 11, 2021 17:08
@xnetcat
Copy link
Member

xnetcat commented Apr 29, 2021

Please pull changes from upstream/dev

@xnetcat xnetcat linked an issue Apr 30, 2021 that may be closed by this pull request
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@Silverarmor, this PR is a Go.

@Silverarmor
Copy link
Member

@aklajnert Please re-review to verify your requested changes have been made

@Silverarmor Silverarmor merged commit 9c0966a into spotDL:dev May 18, 2021
@AZMCode
Copy link
Contributor Author

AZMCode commented May 18, 2021

Nice! Happy to see this finally come through.

Silverarmor added a commit that referenced this pull request May 25, 2021
Publish v3.6.0

* ignore .cache and other hidden files (#1274)

* Bump minimal required python version to 3.6.1 (#1278)

* Remove FFmpeg normalization causing "quiet" songs. (#1276)

* Saved Songs Download and User Authentication (#1240)

* regenerate cassettes (#1290)

* Use ffmpeg_path to check for version (#1289)

* Skip already downloaded songs before doing youtube search (#1287)

* Fix security risk (#1285)

* Song matching improvements (#1279)

* Artist songs fixes (#1284)

* More output formats (#1244)

* Bump version number to 3.6.0

* Update .gitignore to remove duplicate cache

* docs update (#1293)


Co-authored-by: Silverarmor <23619946+Silverarmor@users.noreply.github.com>
Co-authored-by: Jakub Kot <42355410+xnetcat@users.noreply.github.com>
Co-authored-by: Peyton Creery <44987569+phcreery@users.noreply.github.com>
Co-authored-by: AZMCode <adrianozambrana@protonmail.com>
Co-authored-by: Aiden Gardner <19619206+aiden2480@users.noreply.github.com>
Co-authored-by: Oliver Blanthorn <freedom4cows@gmail.com>
Co-authored-by: Andrzej Klajnert <github@aklajnert.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Enhancing spotDL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downloading all of a user's saved songs
4 participants