-
-
Notifications
You must be signed in to change notification settings - Fork 512
ChannelInfo: if getName() fails, try to check if there is a user readable error message #230
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
Conversation
extractor/src/main/java/org/schabi/newpipe/extractor/channel/ChannelInfo.java
Outdated
Show resolved
Hide resolved
...n/java/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeChannelExtractor.java
Outdated
Show resolved
Hide resolved
...n/java/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeChannelExtractor.java
Outdated
Show resolved
Hide resolved
| info.addError(nameFailure); | ||
| // this is so severe, that other extractions most likely fail anyway, | ||
| // so skip them | ||
| return info; |
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.
That's not good and will fail in a NullPointerExcepetion. The frontend assumes, that when a ChannelInfo is returned there is information which can be processed. We need to fail here because we cannot get any information.
Just throw the ParsingException when obtainErrorMessage() brings no result. When obtainErrorMessage() returned an error message, I suggest to throw a ContentNotAvailableException with the message and then adjust the frontend handlers to display it. Can you check how long the other messages are, which are passed to other ContentNotAvailableException's? If they are too long or not helpful, we might need to add a new displayMessage attribute.
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.
That's not good and will fail in a
NullPointerException. The frontend assumes, that when aChannelInfois returned there is information which can be processed.
but this is inline with the other cases. For instance
try {
info.setOriginalUrl(extractor.getOriginalUrl());
} catch (Exception e) {
info.addError(e);
}
would also render a NullPointerException if the originalUrl would be obtained without checking in frontend as there is no setting of an empty url in error case.
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 am now throwing the ContentNotAvailableException as requested. So I have renamed the PR as it now basically just gets a user readable error message.
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 thought about extending the ContentNotAvailableException like this:
public class ContentNotAvailableException extends ParsingException {
/**
* Reason why content is not available.
* Can be displayed to the user
*/
private String reason;
....
public ContentNotAvailableException(String message, String reason) {
super(message);
this.reason = reason;
}
public ContentNotAvailableException(String message, String reason, Throwable cause) {
super(message, cause);
this.reason = reason;
}
....
/**
* {@link ContentNotAvailableException#reason}
* @return null when no reason is available
*/
public String getReason() {
return reason;
}This allows us to display the reason in the app by checking if the reason is not null, otherwise display the generic "content unavailable" message.
We could also create an ContentNotAvailableErrorInfo Class which holds this information. @yausername @Stypox @Redirion What do you think?
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.
maybe instead of adding a new method (getReason) we could use an Java API method that should be fitting:
getLocalizedMessage (it actually is localized)
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 think @Redirion idea is good, since it follows the answer provided here: https://stackoverflow.com/questions/24988491/difference-between-e-getmessage-and-e-getlocalizedmessage
The issue is: what would getMessage() return? In theory it should return a non-localized reason, but obviously it can't, so is it OK for it to also return a localized message?
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.
If I understood @TobiGr correctly, he wants to have getMessage() return the none localized text "Could not get channel name" when getName() fails. Which was the state prior to this PR. With this PR getMessage() would return the localized error message from youtube that is now extracted with "obtainErrorMessage".
getLocalizedMessage just returns what getName returns if not overridden. So if we want to have both we could use getLocalizedMessage() (as an alternative to adding a new method like Tobi suggested)
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.
Oh that makes sense, then I agree with you, getLocalizedMessage is a good idea
…n with ErrorMessage as text
...n/java/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeChannelExtractor.java
Outdated
Show resolved
Hide resolved
these "final" modifiers don't seem to have any use there
This fixes #228
But in NewPipe we should also add a handling that will show the proper Youtube Error Message. At least it won't crash anymore if getName fails.
P.S.: don't judge me for committing on Christmas please. I have slow internet and thought I could do something useful while waiting.