Skip to content

Commit 8afb00d

Browse files
authored
Merge pull request #12603 from Stypox/better-error-panel
2 parents a3ddd61 + f27ec53 commit 8afb00d

File tree

14 files changed

+180
-136
lines changed

14 files changed

+180
-136
lines changed

app/src/main/java/org/schabi/newpipe/RouterActivity.java

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,7 @@
5858
import org.schabi.newpipe.extractor.StreamingService;
5959
import org.schabi.newpipe.extractor.StreamingService.LinkType;
6060
import org.schabi.newpipe.extractor.channel.ChannelInfo;
61-
import org.schabi.newpipe.extractor.exceptions.ContentNotAvailableException;
62-
import org.schabi.newpipe.extractor.exceptions.ContentNotSupportedException;
6361
import org.schabi.newpipe.extractor.exceptions.ExtractionException;
64-
import org.schabi.newpipe.extractor.exceptions.ReCaptchaException;
6562
import org.schabi.newpipe.extractor.linkhandler.ListLinkHandler;
6663
import org.schabi.newpipe.extractor.playlist.PlaylistInfo;
6764
import org.schabi.newpipe.extractor.stream.StreamInfo;
@@ -253,7 +250,8 @@ private void handleUrl(final String url) {
253250
showUnsupportedUrlDialog(url);
254251
}
255252
}, throwable -> handleError(this, new ErrorInfo(throwable,
256-
UserAction.SHARE_TO_NEWPIPE, "Getting service from url: " + url))));
253+
UserAction.SHARE_TO_NEWPIPE, "Getting service from url: " + url,
254+
null, url))));
257255
}
258256

259257
/**
@@ -262,23 +260,19 @@ private void handleUrl(final String url) {
262260
* @param errorInfo the error information
263261
*/
264262
private static void handleError(final Context context, final ErrorInfo errorInfo) {
265-
if (errorInfo.getThrowable() != null) {
266-
errorInfo.getThrowable().printStackTrace();
267-
}
268-
269-
if (errorInfo.getThrowable() instanceof ReCaptchaException) {
263+
if (errorInfo.getRecaptchaUrl() != null) {
270264
Toast.makeText(context, R.string.recaptcha_request_toast, Toast.LENGTH_LONG).show();
271265
// Starting ReCaptcha Challenge Activity
272266
final Intent intent = new Intent(context, ReCaptchaActivity.class);
273267
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
268+
intent.putExtra(ReCaptchaActivity.RECAPTCHA_URL_EXTRA, errorInfo.getRecaptchaUrl());
274269
context.startActivity(intent);
275-
} else if (errorInfo.getThrowable() instanceof ContentNotAvailableException
276-
|| errorInfo.getThrowable() instanceof ContentNotSupportedException) {
270+
} else if (errorInfo.isReportable()) {
271+
ErrorUtil.createNotification(context, errorInfo);
272+
} else {
277273
// this exception does not usually indicate a problem that should be reported,
278274
// so just show a toast instead of the notification
279275
Toast.makeText(context, errorInfo.getMessage(context), Toast.LENGTH_LONG).show();
280-
} else {
281-
ErrorUtil.createNotification(context, errorInfo);
282276
}
283277

284278
if (context instanceof RouterActivity) {
@@ -641,7 +635,8 @@ private void handleChoice(final String selectedChoiceKey) {
641635
startActivity(intent);
642636
finish();
643637
}, throwable -> handleError(this, new ErrorInfo(throwable,
644-
UserAction.SHARE_TO_NEWPIPE, "Starting info activity: " + currentUrl)))
638+
UserAction.SHARE_TO_NEWPIPE, "Starting info activity: " + currentUrl,
639+
null, currentUrl)))
645640
);
646641
return;
647642
}
@@ -828,10 +823,10 @@ private void openAddToPlaylistDialog(final int currentServiceId, final String cu
828823
})
829824
)),
830825
throwable -> runOnVisible(ctx -> handleError(ctx, new ErrorInfo(
831-
throwable,
832-
UserAction.REQUESTED_STREAM,
826+
throwable, UserAction.REQUESTED_STREAM,
833827
"Tried to add " + currentUrl + " to a playlist",
834-
((RouterActivity) ctx).currentService.getServiceId())
828+
((RouterActivity) ctx).currentService.getServiceId(),
829+
currentUrl)
835830
))
836831
)
837832
);
@@ -971,7 +966,7 @@ public void handleChoice(final Choice choice) {
971966
}
972967
}, throwable -> handleError(this, new ErrorInfo(throwable, finalUserAction,
973968
choice.url + " opened with " + choice.playerChoice,
974-
choice.serviceId)));
969+
choice.serviceId, choice.url)));
975970
}
976971
}
977972

app/src/main/java/org/schabi/newpipe/download/DownloadDialog.java

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -389,8 +389,7 @@ private void fetchStreamsSize() {
389389
}
390390
}, throwable -> ErrorUtil.showSnackbar(context,
391391
new ErrorInfo(throwable, UserAction.DOWNLOAD_OPEN_DIALOG,
392-
"Downloading video stream size",
393-
currentInfo.getServiceId()))));
392+
"Downloading video stream size", currentInfo))));
394393
disposables.add(StreamInfoWrapper.fetchMoreInfoForWrapper(getWrappedAudioStreams())
395394
.subscribe(result -> {
396395
if (dialogBinding.videoAudioGroup.getCheckedRadioButtonId()
@@ -399,8 +398,7 @@ private void fetchStreamsSize() {
399398
}
400399
}, throwable -> ErrorUtil.showSnackbar(context,
401400
new ErrorInfo(throwable, UserAction.DOWNLOAD_OPEN_DIALOG,
402-
"Downloading audio stream size",
403-
currentInfo.getServiceId()))));
401+
"Downloading audio stream size", currentInfo))));
404402
disposables.add(StreamInfoWrapper.fetchMoreInfoForWrapper(wrappedSubtitleStreams)
405403
.subscribe(result -> {
406404
if (dialogBinding.videoAudioGroup.getCheckedRadioButtonId()
@@ -409,8 +407,7 @@ private void fetchStreamsSize() {
409407
}
410408
}, throwable -> ErrorUtil.showSnackbar(context,
411409
new ErrorInfo(throwable, UserAction.DOWNLOAD_OPEN_DIALOG,
412-
"Downloading subtitle stream size",
413-
currentInfo.getServiceId()))));
410+
"Downloading subtitle stream size", currentInfo))));
414411
}
415412

416413
private void setupAudioTrackSpinner() {

app/src/main/java/org/schabi/newpipe/error/AcraReportSender.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ public void send(@NonNull final Context context, @NonNull final CrashReportData
3636
ErrorUtil.openActivity(context, new ErrorInfo(
3737
new String[]{report.getString(ReportField.STACK_TRACE)},
3838
UserAction.UI_ERROR,
39-
null,
4039
"ACRA report",
40+
null,
4141
R.string.app_ui_crash));
4242
}
4343
}

app/src/main/java/org/schabi/newpipe/error/ErrorInfo.kt

Lines changed: 113 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import androidx.core.content.ContextCompat
77
import com.google.android.exoplayer2.ExoPlaybackException
88
import com.google.android.exoplayer2.upstream.HttpDataSource
99
import com.google.android.exoplayer2.upstream.Loader
10-
import kotlinx.parcelize.IgnoredOnParcel
1110
import kotlinx.parcelize.Parcelize
1211
import org.schabi.newpipe.R
1312
import org.schabi.newpipe.extractor.Info
@@ -29,70 +28,108 @@ import org.schabi.newpipe.extractor.exceptions.YoutubeMusicPremiumContentExcepti
2928
import org.schabi.newpipe.ktx.isNetworkRelated
3029
import org.schabi.newpipe.player.mediasource.FailedMediaSource
3130
import org.schabi.newpipe.player.resolver.PlaybackResolver
31+
import java.net.UnknownHostException
3232

33+
/**
34+
* An error has occurred in the app. This class contains plain old parcelable data that can be used
35+
* to report the error and to show it to the user along with correct action buttons.
36+
*/
3337
@Parcelize
3438
class ErrorInfo private constructor(
3539
val stackTraces: Array<String>,
3640
val userAction: UserAction,
37-
val serviceId: Int?,
3841
val request: String,
42+
val serviceId: Int?,
3943
private val message: ErrorMessage,
44+
/**
45+
* If `true`, a report button will be shown for this error. Otherwise the error is not something
46+
* that can really be reported (e.g. a network issue, or content not being available at all).
47+
*/
48+
val isReportable: Boolean,
49+
/**
50+
* If `true`, the process causing this error can be retried, otherwise not.
51+
*/
52+
val isRetryable: Boolean,
53+
/**
54+
* If present, indicates that the exception was a ReCaptchaException, and this is the URL
55+
* provided by the service that can be used to solve the ReCaptcha challenge.
56+
*/
57+
val recaptchaUrl: String?,
58+
/**
59+
* If present, this resource can alternatively be opened in browser (useful if NewPipe is
60+
* badly broken).
61+
*/
62+
val openInBrowserUrl: String?,
4063
) : Parcelable {
4164

42-
// no need to store throwable, all data for report is in other variables
43-
// also, the throwable might not be serializable, see TeamNewPipe/NewPipe#7302
44-
@IgnoredOnParcel
45-
var throwable: Throwable? = null
46-
47-
private constructor(
65+
@JvmOverloads
66+
constructor(
4867
throwable: Throwable,
4968
userAction: UserAction,
50-
serviceId: Int?,
51-
request: String
69+
request: String,
70+
serviceId: Int? = null,
71+
openInBrowserUrl: String? = null,
5272
) : this(
5373
throwableToStringList(throwable),
5474
userAction,
55-
serviceId,
5675
request,
57-
getMessage(throwable, userAction, serviceId)
58-
) {
59-
this.throwable = throwable
60-
}
76+
serviceId,
77+
getMessage(throwable, userAction, serviceId),
78+
isReportable(throwable),
79+
isRetryable(throwable),
80+
(throwable as? ReCaptchaException)?.url,
81+
openInBrowserUrl,
82+
)
6183

62-
private constructor(
63-
throwable: List<Throwable>,
84+
@JvmOverloads
85+
constructor(
86+
throwables: List<Throwable>,
6487
userAction: UserAction,
65-
serviceId: Int?,
66-
request: String
88+
request: String,
89+
serviceId: Int? = null,
90+
openInBrowserUrl: String? = null,
6791
) : this(
68-
throwableListToStringList(throwable),
92+
throwableListToStringList(throwables),
6993
userAction,
70-
serviceId,
7194
request,
72-
getMessage(throwable.firstOrNull(), userAction, serviceId)
73-
) {
74-
this.throwable = throwable.firstOrNull()
75-
}
95+
serviceId,
96+
getMessage(throwables.firstOrNull(), userAction, serviceId),
97+
throwables.any(::isReportable),
98+
throwables.isEmpty() || throwables.any(::isRetryable),
99+
throwables.firstNotNullOfOrNull { it as? ReCaptchaException }?.url,
100+
openInBrowserUrl,
101+
)
102+
103+
// constructor to manually build ErrorInfo when no throwable is available
104+
constructor(
105+
stackTraces: Array<String>,
106+
userAction: UserAction,
107+
request: String,
108+
serviceId: Int?,
109+
@StringRes message: Int
110+
) :
111+
this(
112+
stackTraces, userAction, request, serviceId, ErrorMessage(message),
113+
true, false, null, null
114+
)
115+
116+
// constructor with only one throwable to extract service id and openInBrowserUrl from an Info
117+
constructor(
118+
throwable: Throwable,
119+
userAction: UserAction,
120+
request: String,
121+
info: Info?,
122+
) :
123+
this(throwable, userAction, request, info?.serviceId, info?.url)
76124

77-
// constructor to manually build ErrorInfo
78-
constructor(stackTraces: Array<String>, userAction: UserAction, serviceId: Int?, request: String, @StringRes message: Int) :
79-
this(stackTraces, userAction, serviceId, request, ErrorMessage(message))
80-
81-
// constructors with single throwable
82-
constructor(throwable: Throwable, userAction: UserAction, request: String) :
83-
this(throwable, userAction, null, request)
84-
constructor(throwable: Throwable, userAction: UserAction, request: String, serviceId: Int) :
85-
this(throwable, userAction, serviceId, request)
86-
constructor(throwable: Throwable, userAction: UserAction, request: String, info: Info?) :
87-
this(throwable, userAction, info?.serviceId, request)
88-
89-
// constructors with list of throwables
90-
constructor(throwable: List<Throwable>, userAction: UserAction, request: String) :
91-
this(throwable, userAction, null, request)
92-
constructor(throwable: List<Throwable>, userAction: UserAction, request: String, serviceId: Int) :
93-
this(throwable, userAction, serviceId, request)
94-
constructor(throwable: List<Throwable>, userAction: UserAction, request: String, info: Info?) :
95-
this(throwable, userAction, info?.serviceId, request)
125+
// constructor with multiple throwables to extract service id and openInBrowserUrl from an Info
126+
constructor(
127+
throwables: List<Throwable>,
128+
userAction: UserAction,
129+
request: String,
130+
info: Info?,
131+
) :
132+
this(throwables, userAction, request, info?.serviceId, info?.url)
96133

97134
fun getServiceName(): String {
98135
return getServiceName(serviceId)
@@ -205,8 +242,7 @@ class ErrorInfo private constructor(
205242
// other extractor exceptions
206243
throwable is ContentNotSupportedException ->
207244
ErrorMessage(R.string.content_not_supported)
208-
// ReCaptchas should have already been handled elsewhere,
209-
// but return an error message here just in case
245+
// ReCaptchas will be handled in a special way anyway
210246
throwable is ReCaptchaException ->
211247
ErrorMessage(R.string.recaptcha_request_toast)
212248
// test this at the end as many exceptions could be a subclass of IOException
@@ -234,5 +270,36 @@ class ErrorInfo private constructor(
234270
ErrorMessage(R.string.error_snackbar_message)
235271
}
236272
}
273+
274+
fun isReportable(throwable: Throwable?): Boolean {
275+
return when (throwable) {
276+
// we don't have an exception, so this is a manually built error, which likely
277+
// indicates that it's important and is thus reportable
278+
null -> true
279+
// the service explicitly said that content is not available (e.g. age restrictions,
280+
// video deleted, etc.), there is no use in letting users report it
281+
is ContentNotAvailableException -> false
282+
// we know the content is not supported, no need to let the user report it
283+
is ContentNotSupportedException -> false
284+
// happens often when there is no internet connection; we don't use
285+
// `throwable.isNetworkRelated` since any `IOException` would make that function
286+
// return true, but not all `IOException`s are network related
287+
is UnknownHostException -> false
288+
// by default, this is an unexpected exception, which the user could report
289+
else -> true
290+
}
291+
}
292+
293+
fun isRetryable(throwable: Throwable?): Boolean {
294+
return when (throwable) {
295+
// we know the content is not available, retrying won't help
296+
is ContentNotAvailableException -> false
297+
// we know the content is not supported, retrying won't help
298+
is ContentNotSupportedException -> false
299+
// by default (including if throwable is null), enable retrying (though the retry
300+
// button will be shown only if a way to perform the retry is implemented)
301+
else -> true
302+
}
303+
}
237304
}
238305
}

0 commit comments

Comments
 (0)