Skip to content
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

Improve NewPipe's sharing and fix crash when opening content in browser on some devices #5129

Closed
wants to merge 19 commits into from
Closed

Improve NewPipe's sharing and fix crash when opening content in browser on some devices #5129

wants to merge 19 commits into from

Conversation

AudricV
Copy link
Member

@AudricV AudricV commented Dec 7, 2020

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

This PR fixes the crash when no browser is set as default on some devices (NewPipe thinks that the system app chooser is a browser because its package ID isn't "android" due to some OEM's system customizations) by catching the ActivityNotFound exception and uses the openInDefaultApp method. I also made improvements in NewPipe's sharing by trying to use Android's system share sheet instead of using OEM's share sheet (I used this last as a fallback if this first has been removed) and also made it for opening content in browser. Here is the difference between the original code and my changes (my device is an Honor 9X, running EMUI 10):

Before this PR After this PR
Share panel before Share panel after
Share panel before this PR Share panel after this PR
Open in browser before Open in browser after
Open in browser before this PR Open in browser after this PR

I also updated NewPipe's URLs (API and links in AboutActivity) with the new domain name.

Fixes the following issue(s)

Fixes #3925

APK testing

NewPipe fix-crash-open-in-browser.apk.zip

Due diligence

Important note: I am not an Android / Java developer so please be indulgent with me.

@TobiGr
Copy link
Member

TobiGr commented Dec 7, 2020

thank you for the pull request! It looks like you messed up the git history. please drop 074987f and rebase your branch on our dev branch afterwards.

@AudricV
Copy link
Member Author

AudricV commented Dec 8, 2020

@TobiGr Done

@AudricV
Copy link
Member Author

AudricV commented Dec 8, 2020

Does someone know where is the code to open links from descriptions and comments? I can't found it so my improvements aren't applied to those links. And also, how to apply they to the app update notification?

Some OEMs changed the app chooser where there is not default browser, for example "com.huawei.android.internal.app" instead of "android" on EMUI devices. NewPipe crashed on those devices because it thinks that it's a browser. I catched the exception and I updated the comments to explain this.
@TobiGr
Copy link
Member

TobiGr commented Dec 10, 2020

Ah, seems like you've figured it out yourself! You can use git reset to remove 2932eef.

The update notification is only triggered when we release a new version. You cannot trigger it.

Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.
Please open a separate PR for unrelated changes next time. That speeds the reviewing process up.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@AudricV
Copy link
Member Author

AudricV commented Dec 12, 2020

@TobiGr Changes applied

Some OEMs changed the app chooser where there is not default browser, for example "com.huawei.android.internal.app" instead of "android" on EMUI devices. NewPipe crashed on those devices because it thinks that it's a browser. I catched the exception and I updated the comments to explain this.
@Stypox
Copy link
Member

Stypox commented Dec 15, 2020

It seems like there is still something wrong with the commit history of this PR.
@B0pol could you take care of this, afterwards, since you improved sharing functions a while ago?

@Stypox Stypox assigned Stypox and B0pol and unassigned Stypox Dec 15, 2020
@B0pol
Copy link
Member

B0pol commented Dec 15, 2020

@Stypox I'll look at this the next weekend

@AudricV
Copy link
Member Author

AudricV commented Dec 15, 2020

@Stypox @B0pol Can I reopen a proper PR in a new branch when I have free time (not in a near future)?
(I am new to open source projects and very bad with Git for now, sorry :( .)

@AudricV AudricV closed this Dec 15, 2020
@Stypox
Copy link
Member

Stypox commented Dec 15, 2020

@TiA4f8R don't worry, we have all been new to git once ;-)
If you need any help feel free to ask :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error while opening content in browser
6 participants