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

Add Notification Permission Feature #106

Merged
merged 6 commits into from
Sep 10, 2024
Merged

Conversation

himalia416
Copy link
Contributor

This PR introduces a new feature to handle notification permissions in the app. Specifically, it adds logic to request and check the status of notification permissions for Android devices running Android 13 (API level 33) and above.

@himalia416 himalia416 requested a review from philips77 September 3, 2024 08:04
Copy link
Collaborator

@philips77 philips77 left a comment

Choose a reason for hiding this comment

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

The PR looks very good.

My only question is regarding a purpose of it. Why do you REQUIRE notifications? Users should be able to deny them and app should work just fine. Even foreground service can be started with a notification, which will be hidden by the system, but the service will work just fine.

In my opinion the lib should allow easy way for requesting it if they are not granted, but it should be some method called as Lunch effect, or from a View Model. Your current solution suggests that notifications are required to show some UI, and I don't see any situation where it would be true.

get() = context.getSharedPreferences(SHARED_PREFS_NAME, Context.MODE_PRIVATE)

val isTiramisuOrAbove: Boolean
@ChecksSdkIntAtLeast(api = Build.VERSION_CODES.TIRAMISU)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is so cool!

@philips77 philips77 force-pushed the permission-notification branch from eb0de9d to 98266a2 Compare September 10, 2024 12:43
@philips77 philips77 merged commit fbdb807 into main Sep 10, 2024
1 check passed
@philips77 philips77 deleted the permission-notification branch September 10, 2024 12:45
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.

2 participants