-
Notifications
You must be signed in to change notification settings - Fork 422
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
Hotfixes for YouTube and improve unavailable cases #267
Conversation
Thanks a lot for your PR! I wanted to implement the things you wrote about, but now that's not needed anymore. I'll take a closer look in a few hours. |
I just found another bug, which could be fixed by looping over all badges in getStreamType() in YoutubeStreamInfoItemExtractor. Could you please do that? |
...org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeStreamInfoItemExtractor.java
Show resolved
Hide resolved
final String viewCount = getTextFromObject(viewCountObject); | ||
|
||
if (viewCount.toLowerCase().contains("no views")) { | ||
return 0; |
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.
Are there no other ways than relying on English?
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.
At the moment there is no other way. We might need to check whether the value contains specific words like "no" or "you", which should be quite similar in most languages. In that way, we do not need to make a list for all translations later, but can reduce that to a minimum of items to check. But that ca wait for a later PR.
@wb9688 Do you mean something like JsonArray badges = videoInfo.getArray("badges");
for (Object badge : badges) {
if (((JsonObject) badge).getObject("metadataBadgeRenderer").getString("label").equals("LIVE NOW")) {
return StreamType.LIVE_STREAM;
}
} |
@TobiGr: That's exactly what I meant. |
The test implementation was throwing an exception instead of just returning the response and letting the caller handle it.
Will be latest one in this commit because there's need to check the history of redirects as of now.
Also remove catch because it may hide bugs when the page changes.
- Added tests for these cases.
Added a test case as well.
One more issue though: when opening a premiere video, it's not throwing the right exception. We should add more status checks here. |
Also, @TobiGr what about we disable these protected branches shenanigans? In a lot of pull requests I see, it's infested with "merge branch dev into <branch>", because people keep pressing the update button. For example, this one and #265 should be perfectly able to perform a simple merge. Then you don't need to overwrite the commits and rewrite history (I like seeing those verified commits). |
@mauriciocolli |
Yes, I hate that, too. I'd like people to rebase their commits to a have a readable history. We need to find a solution for this. |
@TobiGr: Does GitHub not have a rebase and merge button? |
@TobiGr no, it should be latest url that response was produced to. In case of a redirection, the response would be made to the redirected url. Here's a diff of what should be done: diff --git a/app/src/main/java/org/schabi/newpipe/DownloaderImpl.java b/app/src/main/java/org/schabi/newpipe/DownloaderImpl.java
index 0ae072c66..6591fcbda 100644
--- a/app/src/main/java/org/schabi/newpipe/DownloaderImpl.java
+++ b/app/src/main/java/org/schabi/newpipe/DownloaderImpl.java
@@ -171,7 +171,9 @@ public class DownloaderImpl extends Downloader {
responseBodyToReturn = body.string();
}
- return new Response(response.code(), response.message(), response.headers().toMultimap(), responseBodyToReturn);
+ final String latestUrl = response.request().url().toString();
+
+ return new Response(response.code(), response.message(), response.headers().toMultimap(), responseBodyToReturn, latestUrl);
}
/** The Also, there's the other issue that I mentioned, so please wait a little bit longer.
Edit: I was thinking of the merge situation when I wrote below, but yes, would be nice. I think this is about the protected branch: I think if we disable that, we can merge requests without that merge hell. Imagine 10 pull requests and having to update each one. I think the update button would work, but then even more merge commits would be produced. |
Thanks. I'll change this when updating the extractor version.
Ok. Are you on it, or should I take a look at it?
ok. |
TobiGr pushed this 3525223.
I'm currently testing it., I'll open another pull request soon. |
Oh, and @TobiGr, sorry if it caused a misunderstanding, but you would have to disable it, as I don't have permissions to change that setting (I don't even see here, screenshot from another repo). |
Fix the issues that I mentioned in #262, and some more that I found along the way.
YouTube
Downloader