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

Fix error notification on KitKat #7977

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Mar 2, 2022

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

The app was crashing when trying to display an error notification on KitKat, this PR fixes that by using a non-vector-drawable on API < Lollipop. I also replaced NotificationManager with NotificationManagerCompat, just like in the player's NotificationUtil.

The crash was encountered while testing other thing and is about "Bad notification posted from package ... Couldn't create icon ..."

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

It was crashing due to a drawable icon being used; also use NotificationManagerCompat
@TobiGr TobiGr added bug Issue is related to a bug device/software specific Issues that only happen on some devices or with some specific hardware/software labels Mar 2, 2022
@sonarcloud
Copy link

sonarcloud bot commented Mar 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

Superseded by #7613

@Stypox
Copy link
Member Author

Stypox commented Mar 3, 2022

@litetex I know, but if we release another release without #7613 this can be merged (but let's make next release with #7613)
Oh, and it also contains a small improvement independent of #7613

@Stypox
Copy link
Member Author

Stypox commented Mar 19, 2022

@litetex can you review and merge? #7613 will not be in this release

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM (didn't test it so far)

And another merge conflict for #7613 I guess... yay.

@leetfin
Copy link

leetfin commented Mar 20, 2022

Thank you, as a KitKat NewPipe user I appreciate it! Hope this gets merged and released before support drops.

@opusforlife2
Copy link
Collaborator

Ping, so someone merges this approved PR.

@litetex litetex merged commit 1ecb0ca into TeamNewPipe:dev Mar 25, 2022
@AudricV AudricV mentioned this pull request Apr 7, 2022
5 tasks
@Stypox Stypox mentioned this pull request Apr 16, 2022
12 tasks
@Stypox Stypox deleted the error-notification-kitkat branch August 4, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug device/software specific Issues that only happen on some devices or with some specific hardware/software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants