-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 crash when no default browser is set and improve share dialogs (on some devices) #5187
Conversation
@test2a Can you please test the above APK and let me know if the crash when opening content in browser if you didn't set a default browser is fixed for you and when you share a video or a playlist, all is working? Thank you in advance. |
@test2a This is intended, that's how Android works. Also, your chooser dialog is from Android system (I have it too on my old Asus Zenfone 4 Max running Android 8.1.0), so all is working when opening content in browser. About the share screen, can you share a playlist and say me if all is working by posting a screenshot from it? Moreover, with the informations you provided in #4140, it seems your device isn't running Android 10 and this share sheet is from Android 10. So it's normal. |
@TiA4f8R yeah. i am on 8 i think. anyways, how to share a playlist?? i see no option in bookmarked playlists |
@test2a Thanks for testing! All should work. |
@TobiGr @B0pol In Android Messages of Google, I can select a date from a SMS but the text selection is different and seems to be the text selection of Android because when I use Share command, it opens Android Share Sheet and not Huawei Share Sheet, contrary to NewPipe. Check the two screenshots below: Android Messages date text selection NewPipe text selection (that's also the case of other apps) Did you know if we can do something for text selection in NewPipe to get the same result? |
Can someone help me for the things to do in PR description? |
@MathJoDE Does the APK in description is working for sharing a video/playlist, opening a video/playlist in a browser, opening a NewPipe download, and a link in comment of a video? Thanks for testing in advance. |
@TiA4f8R Seems to work great! I tested successful (Huawei P9 lite, Android 7 / EMUI 5.0.3):
|
@MathJoDE Like on my device (Honor 9X, EMUI 10), is Android's system chooser showing on your device instead of Huawei chooser / Huawei Share? |
Yes, for all test cases I mentioned in my post above, now the Android system chooser pops up. (In contrast to current stable version 0.20.8, where the Huawei chooser pops up.) It's exemplary shown in the left screenshot below.
Also for opening a Download the Android system chooser shows up (right screenshot) Screenshots:
For links in video descriptions and also for links in these legal notices by youtube (that something belongs to a public broadcast service, or the COVID notices) below videos, still the Huawei Chooser pops up. (left screenshot below). Also at a channel's rss feed, still the Huawei Chooser is used (right screenshot).
I assume that's what the ToDos in the PRs opening comment are addressing, in which the RSS feed point seems to miss. I'm not really experienced with Java, and even less with Android developing, but based on the diff of your changes in this PR I fiddled a bit around[1]. So the following is absolutely untested and just guessed. Maybe it can help, if not, just ignore it ^^ I suppose here the [1] using |
@MathJoDE Thanks for your searches and your response! I will do in a future commit the trick for RSS feed. |
I disagree with last change. We don't want to open RSS feed in browser, but in default app configured to handle RSS or other feeds. It's fine to open RSS feed in your RSS app. On top of that, most popular browsers don't support RSS feeds anymore. The "why" I introduced this change initially — open in browser instead of open in default app — was because YouTube, PeerTube or other services links would loop in NewPipe if NewPipe is set as default for these links. |
@B0pol I understand what you mean, but I changed getDefaultAppPackageName to return the default app for an intent passed as an argument and not only for http links. The openUrlInBrowser method just create an intent like as previous for RSS feeds. I just tried with two RSS apps and if I set it to always open the links that they can open it opens the RSS app; if I don't set it, it will open a chooser. This behavior is the same in the original app and my modifications. Here is a screencast between current behaviour in NewPipe and my changes (please ignore toasts, it was for personal tests): RSS.feed.debug.vs.release.mp4 |
While debugging my changes in Chrome Developer Tools and opening a YouTube video, this crash happens. How to fix it? Exception
Crash log
|
There is no NewPipe/app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java Lines 1231 to 1241 in 395f6f6
|
@TobiGr I removed the use of disposables when preparing HTML descriptions in prepareDescription method of the VideoDetail fragment. I tried to open a YouTube video when debugging the app in Chrome Developer Tools and it opens fine with the description which is successfully shown. Is it ok to remove this use ? |
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 LinkHelper
is a good idea. Thank you!
app/src/main/java/org/schabi/newpipe/util/NavigationHelper.java
Outdated
Show resolved
Hide resolved
@TiA4f8R The changes you pushed are looking good, thanks ;-)
Since all other apps don't do this, I wouldn't do it here either. Or at least not in this PR, but in a future one.
Take a look here, and in particular the last few lines: NewPipe/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java Lines 321 to 374 in 9ee7740
For big HTMLs it can take a while to build the
I don't know how this could be achieved, but maybe you can loop through all |
I already do this in my first commit :) |
Oh, yeah, sorry ;-)
That's ok, don't worry, this PR solves many issues just by itself and a new one can still be opened by you or someone else in the future :-) Could you bring back the use of reactivex disposables, so that the |
@Stypox Can you do it please (note that it was only for HTML descriptions, may be it should be extended to other description types?) and also fix if you can #5187 (comment)? This crash is so annoying... Thank you in advance! (And also thanks for your review :) ) |
@TiA4f8R done, what do you think about my changes? |
@Stypox Looks good (I am not developer and I don't really know Java so you should ask to a real developer :)). Did you test your changes by trying to open a video / a track while debugging the app with Steho on your computer and see if description is showing successfully or if there is a crash? |
Let me tell you that you did a very good job given that you don't know Java ;-)
Why should I do that? I have never debugged anything in Stetho, though I have tested that the app works. Anyway, debugging instead of executing normally shouldn't change anything. |
@Stypox Oh sorry, I talk about Chrome Developer Tools ( |
@Stypox I just tested while debugging in Chrome Developer Tools and it works, no crash and descriptions are showing! Thanks for your commit! I have a question about the new app version update notification: currently, NewPipe uses an ACTION_VIEW intent in the notification which launches the Huawei Chooser on EMUI devices if there isn't a default browser. If I use an ACTION_CHOOSER intent and put the ACTION_VIEW intent as an extra intent and if there is a default browser, does the system will open the Android system chooser or it will open the default browser? Thanks in advance for your response. |
Improve NewPipe's share on some devices + fix crash when no browser is set on some devices Catching ActivityNotFoundException when trying to open the default browser Use an ACTION_CHOOSER intent and put as an extra intent the intent to open an URI / share an URI when no default app is set. Add a LinkHelper class which set a custom action when clicking web links in the description of a content. This class also helps to implement a confirmation dialog when trying to open web links in an external app.
Apply the requested changes, use ShareUtils.shareText to share an stream in the play queue and optimize imports for Java files, using Android Studio functionality. Apply the requested changes and do little improvements Apply the requested changes, use ShareUtils.shareText to share an stream in the play queue and optimize imports for Java files, using Android Studio functionality.
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 rebased and tested more, both on Android 4.4, Android 10 and Android 11. Everything works as expected. Thank you!
I get this on Android TV API 27 when trying to open in browser (there is no browser) Exception
Crash log
|
…ements Share improvements + fix crash when no default browser is set on some devices
What is it?
Description of the changes in your PR
General description of the changes
This PR improves sharing content to other apps with the following changes:
Copied to clipboard
shown when copying formatted report in crash report activityCode changes
Here is the difference between the original code and my changes (my device is an Honor 9X, running EMUI 10):
What I changed to do this
The trick to do this is to use an ACTION_CHOOSER intent and put the share intent (ACTION_SEND) or the view intent (ACTION_VIEW) as an extra intent.
Note also if Android's system chooser (in the intent ACTION_CHOOSER) has been directly modified by the OEM, this PR will not change it.
A few note about UX
With my changes, we can't choose which app to always open an intent but only choose for once (if there isn't a default app).
Fixes the following issue(s)
Fixes #3925
APK testing
NewPipe share improvements debug apk.zip (based on 92a87a5)
Due diligence
Important note: I am not an Android / Java developer so please be indulgent with me.