-
Notifications
You must be signed in to change notification settings - Fork 182
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
Youtube Playlist URL support #244
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the Youtube API gives us the playlist items in paginated form, so we get a maximum of 50 items at a time and then we have to call the API again.
You can take a bit of the solution from watchLater()
on YouTube Watch later.
@@ -39,6 +39,21 @@ function getYouTubeVideoDescription(videoID, youTubeDataApiKey, onSuccess, onFai | |||
}).fail(onFail); | |||
} | |||
|
|||
function getYouTubeListItems(listID, youTubeDataApiKey, onSuccess, onFail) { | |||
// Request the video description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not accurate
@@ -47,10 +62,17 @@ function parseYoutubeVideoID(url) { | |||
}; | |||
var shortUrlDomain = "youtu.be"; | |||
var longUrlDomain = "youtube.com"; | |||
var playlistUrlElement = "&list="; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be smarter about this, list
could be the first query parameter - represented as ?list=foo
.
We should be able to check the return value of getParameterByName(url, "list")
against being null
, which means it's not in the URL.
Also, this means we don't have to declare this variable and it'll simplify the code below
} | ||
else { | ||
console.log("loading song"); | ||
if(window.location.href.indexOf("playlistid=") !== -1) isPlayingPlaylist = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this line has any effect
if(isPlayingPlaylist) { | ||
url += "&playlistindex=" + currentListIndex + "&playlistid=" + currentListID; | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This else
is noop
@@ -377,6 +421,11 @@ function getCurrentVideoID() { | |||
return v; | |||
} | |||
|
|||
function getCurrentListID() { | |||
var v = getParameterByName(window.location.search, "list"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v
is a bad variable name here
return xmlHttp.responseText; | ||
} | ||
|
||
function loadList(listID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessarily complicated, let's move this logic to zap-common.js
and use the same format as the jQuery HTTP requests.
var listData; | ||
var isPlayingPlaylist = false; | ||
|
||
function getSource(url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After refactoring loadList()
this function can go away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start. I think we can just use the same query arg names as YouTube - so playlistindex
can be list
and playlistid
can be id
I have made most of the changes you requested.
I made a separate name |
@apoorvnandan Let's get the code cleaned up so Travis CI can run tests. Click the details link on the codeclimate check above |
How do I resolve the codeclimate issue
The function is defined in zap-common.js along with other functions like |
@apoorvnandan at the top of |
After some manual tests this works pretty well @apoorvnandan!
FYI Travis CI won't run tests until the merge conflicts are resolved, so no worries there. |
Closing due to inactivity |
Closes #1
Motivation and Context
Closes #1
(where 1 would be the issue number) to your commit message.Types of changes
What types of changes does your code introduce? Check all the boxes that apply:
Description
Youtube playlist URLs work after these changes.
makeListenURL()
, we make it append the playlist id and index of the item as parameters in the url. (for example:http://localhost:8080/?v=PT2_F-1esPk&playlistindex=0&playlistid=PLx0sYbCqOb8TBPRdmBHs5Iftvv9TPboYG
)makeListenURL()
again so that it can make a new URL with the videoID and playlist index parameters changed to that of the next video in the playlist (if the playlist has not ended)(for example,
http://localhost:8080/?v=PT2_F-1esPk&playlistindex=0&playlistid=PLx0sYbCqOb8TBPRdmBHs5Iftvv9TPboYG
changes tohttp://localhost:8080/?v=UprcpdwuwCg&playlistindex=1&playlistid=PLx0sYbCqOb8TBPRdmBHs5Iftvv9TPboYG
One expected problem is that the Youtube API gives us the playlist items in paginated form, so we get a maximum of 50 items at a time and then we have to call the API again. I have not yet addressed this and so if the playlist contains more than 50 items, we get the first 50 items only.