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 app crashes if media was not found on YouTube #10

Merged
merged 1 commit into from Oct 16, 2018

Conversation

Projects
None yet
2 participants
@lnfnunes
Contributor

lnfnunes commented Oct 11, 2018

This PR fixes issue #9

Result:
image

@danguilherme

danguilherme requested changes Oct 11, 2018 edited

Thanks for the collaboration!

The repo is lacking a contribution guide, but the base branch should be develop.
I have changed it in the PR.

@@ -292,7 +291,8 @@ function downloadYoutubeVideo(name, location = './', { quality = 'highest', logg
return highland(videoSearchPromise) // search the video
.flatMap(video => highland((push, next) => {
if (!video) {
push(new Error("Video not found"));
warn(logger, `Video not found!`);
push(null, highland.nil);

This comment has been minimized.

@danguilherme

danguilherme Oct 11, 2018

Owner

You can remove this call, as throughStream seems to do that already.

@danguilherme

danguilherme Oct 11, 2018

Owner

You can remove this call, as throughStream seems to do that already.

This comment has been minimized.

@lnfnunes

lnfnunes Oct 11, 2018

Contributor

Sorry about the branch, didn't know! =/

About this call, that was my first approach!
But without that, the process stops on the first "Video not found" occurrence not allowing the playlist to continue fetching the next tracks.

As a user, I prefer to keep fetching all tracks even some is not found and that looks the desire in the issue too.

@lnfnunes

lnfnunes Oct 11, 2018

Contributor

Sorry about the branch, didn't know! =/

About this call, that was my first approach!
But without that, the process stops on the first "Video not found" occurrence not allowing the playlist to continue fetching the next tracks.

As a user, I prefer to keep fetching all tracks even some is not found and that looks the desire in the issue too.

This comment has been minimized.

@danguilherme

danguilherme Oct 11, 2018

Owner

Got it, I agree. Did you test downloading audio? Because it uses the throughStream approach:

spotivy/index.js

Lines 342 to 345 in 5b5a75b

if (!video) {
warn(logger, `Audio not found!`);
return throughStream();
}

@danguilherme

danguilherme Oct 11, 2018

Owner

Got it, I agree. Did you test downloading audio? Because it uses the throughStream approach:

spotivy/index.js

Lines 342 to 345 in 5b5a75b

if (!video) {
warn(logger, `Audio not found!`);
return throughStream();
}

This comment has been minimized.

@lnfnunes

lnfnunes Oct 11, 2018

Contributor

Hum... no, couldn't reproduce the scenário with audio!
But probably will be the same, I'll change that

@lnfnunes

lnfnunes Oct 11, 2018

Contributor

Hum... no, couldn't reproduce the scenário with audio!
But probably will be the same, I'll change that

@danguilherme danguilherme changed the base branch from master to develop Oct 11, 2018

@danguilherme danguilherme added this to the v0.4.4 milestone Oct 11, 2018

@lnfnunes

This comment has been minimized.

Show comment
Hide comment
@lnfnunes

lnfnunes Oct 11, 2018

Contributor

@danguilherme please check again!

Contributor

lnfnunes commented Oct 11, 2018

@danguilherme please check again!

@danguilherme

This comment has been minimized.

Show comment
Hide comment
@danguilherme

danguilherme Oct 12, 2018

Owner

That's very odd, but I'm trying to reproduce the issue now (in the develop branch first), and it always returns results from the YouTube search, thus not causing the original error.

I'm running:

$ node index.js playlist "Bostjan Bele" 37i9dQZF1DWTvNyxOwkztu --verbose # running locally
[spotivy v0.4.3]
                  [DEBUG] Loaded options:
 {
  "spotifyClientId": "xxx",
  "spotifyClientSecret": "xxx",
  "youtubeKey": "xxx",
  "output": "/home/danguilherme/git/danguilherme/spotivy/media",
  "format": "video",
  "quality": "highest",
  "flat": false,
  "audio": false,
  "username": "Bostjan Bele",
  "playlists": [
    "37i9dQZF1DWTvNyxOwkztu"
  ]
}
Saving media to '/home/danguilherme/git/danguilherme/spotivy/media'

                  [DEBUG] Spotify login
   [Downloading playlist] Chillout Lounge
                  [DEBUG] Fetching playlist tracks (37i9dQZF1DWTvNyxOwkztu)
                  [DEBUG] Fetch paginated: "getPlaylistTracks"
                  [DEBUG] Fetch paginated: loading 0..20
                  [DEBUG] Fetch paginated: loaded  20/90
                  [DEBUG] Fetch paginated: pushing down the stream
      [Downloading track] Daxten - Late Nights
                  [DEBUG] Search video: search "Daxten - Late Nights"
                  [DEBUG] Fetch paginated: next page
                  [DEBUG] Search video: 20 item(s) returned # <---- HERE - in yours, it doesn't return anything
                  [DEBUG] Search music video: None of the videos is considered a good result
                  [DEBUG] Search music video: selected "Daxten - Late Nights", by Stackin' Records
                  [DEBUG] Download from: https://www.youtube.com/watch?v=WjeZjeM0liY
                  [DEBUG] Download to  : /home/danguilherme/git/danguilherme/spotivy/media/Chillout Lounge/Daxten - Late Nights.mp4
                  [DEBUG] Finish write: Daxten - Late Nights
... continues ...

Do you have any idea on why this happens?

Owner

danguilherme commented Oct 12, 2018

That's very odd, but I'm trying to reproduce the issue now (in the develop branch first), and it always returns results from the YouTube search, thus not causing the original error.

I'm running:

$ node index.js playlist "Bostjan Bele" 37i9dQZF1DWTvNyxOwkztu --verbose # running locally
[spotivy v0.4.3]
                  [DEBUG] Loaded options:
 {
  "spotifyClientId": "xxx",
  "spotifyClientSecret": "xxx",
  "youtubeKey": "xxx",
  "output": "/home/danguilherme/git/danguilherme/spotivy/media",
  "format": "video",
  "quality": "highest",
  "flat": false,
  "audio": false,
  "username": "Bostjan Bele",
  "playlists": [
    "37i9dQZF1DWTvNyxOwkztu"
  ]
}
Saving media to '/home/danguilherme/git/danguilherme/spotivy/media'

                  [DEBUG] Spotify login
   [Downloading playlist] Chillout Lounge
                  [DEBUG] Fetching playlist tracks (37i9dQZF1DWTvNyxOwkztu)
                  [DEBUG] Fetch paginated: "getPlaylistTracks"
                  [DEBUG] Fetch paginated: loading 0..20
                  [DEBUG] Fetch paginated: loaded  20/90
                  [DEBUG] Fetch paginated: pushing down the stream
      [Downloading track] Daxten - Late Nights
                  [DEBUG] Search video: search "Daxten - Late Nights"
                  [DEBUG] Fetch paginated: next page
                  [DEBUG] Search video: 20 item(s) returned # <---- HERE - in yours, it doesn't return anything
                  [DEBUG] Search music video: None of the videos is considered a good result
                  [DEBUG] Search music video: selected "Daxten - Late Nights", by Stackin' Records
                  [DEBUG] Download from: https://www.youtube.com/watch?v=WjeZjeM0liY
                  [DEBUG] Download to  : /home/danguilherme/git/danguilherme/spotivy/media/Chillout Lounge/Daxten - Late Nights.mp4
                  [DEBUG] Finish write: Daxten - Late Nights
... continues ...

Do you have any idea on why this happens?

@lnfnunes

This comment has been minimized.

Show comment
Hide comment
@lnfnunes

lnfnunes Oct 12, 2018

Contributor

Hummm... think I got it!
image

This don't mean shouldn't download the video/audio, this only says no "good" result was found and the first one will be downloaded...

So, the bug (issue) is when the function searchVideo returns an empty array!

To simulate the bug, change line 31
resolve(results.items); to resolve([]);

My test case was wrong in the first... fixed now with an update.
image

Contributor

lnfnunes commented Oct 12, 2018

Hummm... think I got it!
image

This don't mean shouldn't download the video/audio, this only says no "good" result was found and the first one will be downloaded...

So, the bug (issue) is when the function searchVideo returns an empty array!

To simulate the bug, change line 31
resolve(results.items); to resolve([]);

My test case was wrong in the first... fixed now with an update.
image

@lnfnunes

This comment has been minimized.

Show comment
Hide comment
@lnfnunes

lnfnunes Oct 16, 2018

Contributor

Hey, @danguilherme could you test it again? If you find anything else please let me know

Contributor

lnfnunes commented Oct 16, 2018

Hey, @danguilherme could you test it again? If you find anything else please let me know

@danguilherme

This comment has been minimized.

Show comment
Hide comment
@danguilherme

danguilherme Oct 16, 2018

Owner

Sounds great, thanks!

Owner

danguilherme commented Oct 16, 2018

Sounds great, thanks!

@danguilherme danguilherme merged commit f771dfc into danguilherme:develop Oct 16, 2018

@lnfnunes lnfnunes deleted the lnfnunes:bugfix/video-not-found branch Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment