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.

Sign up for GitHub

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

Jump to bottom

Fix crash when no browser is present and use an ACTION_CHOOSER intent in the app update notification #5429

Merged
merged 4 commits into from Jan 18, 2021
Merged

Fix crash when no browser is present and use an ACTION_CHOOSER intent in the app update notification #5429

merged 4 commits into from Jan 18, 2021

Conversation

AudricV
Copy link
Member

@AudricV AudricV commented Jan 16, 2021 •
edited

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

Show a Toast when no app is present on user's device to open a content in an app and in a browser and use an ACTION_CHOOSER intent with the ACTION_VIEW intent put as an extra intent in the update notification.

Fixes the following issue(s)

#5187 (comment)

Due diligence

Stypox
Stypox previously requested changes Jan 16, 2021
View reviewed changes
app/src/main/java/org/schabi/newpipe/util/ShareUtils.java Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/ShareUtils.java Outdated Show resolved Hide resolved
@AudricV AudricV requested a review from Stypox January 16, 2021 17:54
TobiGr
TobiGr requested changes Jan 16, 2021
View reviewed changes
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.
Since you are now returning null, we need to add checks whenever the methods are used. Running the current implementation just elevates the NPEs up in the call hierarchy.

app/src/main/java/org/schabi/newpipe/util/ShareUtils.java Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/ShareUtils.java Outdated Show resolved Hide resolved
@XiangRongLin XiangRongLin added the bug Issue is related to a bug label Jan 17, 2021
@AudricV
Copy link
Member Author

AudricV commented Jan 17, 2021

@TobiGr Is what I've done better?

@TobiGr
Copy link
Member

TobiGr commented Jan 18, 2021

Yes, that looks good. Please update the JDoc to not contain the null info

@AudricV
Copy link
Member Author

AudricV commented Jan 18, 2021

@TobiGr Done. What do you think of the string shown in the toast (No app installed on your device can open this)?

@AudricV
Fix crash when no browser is present and use an ACTION_CHOOSER intent…
27fdbc7 
… for app update notification

Show a Toast when no app is present on user's device to open a content
in an app and in a browser and use an ACTION_CHOOSER intent with the
ACTION_VIEW intent put as an extra intent in the update notification.
@AudricV
Copy link
Member Author

AudricV commented Jan 18, 2021

@TobiGr This should be now fixed. Also, can you change in 0.20.9 changelog the sentence Fixed crash when no default browser is set on some devices and improve share dialogs to Fixed crash when no default browser is set and improve share dialogs (on some devices) (because the share improvements are only applied on some devices, particularly EMUI devices)?

@Stypox Stypox mentioned this pull request Jan 18, 2021
Closed
14 tasks
@AudricV
Copy link
Member Author

AudricV commented Jan 18, 2021 •
edited

@TobiGr String and its French translation changed.

@TobiGr
Copy link
Member

TobiGr commented Jan 18, 2021

I changed that in the GitHub release notes, but I cannot change the fastlane / F-Droid changelog, because that has a size of 497 bytes. 500 are allowed, so adding more characters will cause the other parts of the release notes to be cut off.

TobiGr
TobiGr previously approved these changes Jan 18, 2021
View reviewed changes
@AudricV
Copy link
Member Author

AudricV commented Jan 18, 2021

Sorry for doing all changes in separate commits, I am on my phone and I can only use Github Web Editor right now.

@TobiGr TobiGr merged commit 9a65f02 into TeamNewPipe:dev Jan 18, 2021
@TobiGr
Copy link
Member

TobiGr commented Jan 18, 2021

No problem. There is a squash & merge button ;)

@TobiGr TobiGr mentioned this pull request Jan 18, 2021
Merged
@AudricV AudricV deleted the share-improvements branch January 18, 2021 21:20
@AudricV AudricV restored the share-improvements branch January 20, 2021 11:52
@ghost ghost mentioned this pull request Apr 11, 2021
Open
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 21, 2021
@AudricV @tossj
Fix crash when no browser is present and use an ACTION_CHOOSER intent…
888c812 
… in the app update notification (TeamNewPipe/NewPipe#5429)

Fix crash when no browser is present and use an ACTION_CHOOSER intent for app update notification
Show a Toast when no app is present on user's device to open a content in an app and in a browser and use an ACTION_CHOOSER intent with the ACTION_VIEW intent put as an extra intent in the update notification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stypox Stypox Stypox approved these changes

@TobiGr TobiGr TobiGr left review comments

Assignees
No one assigned
Labels
bug Issue is related to a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@AudricV @TobiGr @Stypox @XiangRongLin