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 no notification permission issue #221

Conversation

austinoneil
Copy link

@austinoneil austinoneil commented Apr 28, 2024

What is it?

  • Bugfix
  • Feature
  • Codebase improvement

Description of the changes in your PR

  • Fix issue when user could not add an event/task when notification permission was not granted.
  • Refactored some logic, extracting reused code into a reusable function.

Before/After Screenshots/Screen Record

  • Before:
  • After:

Fixes the following issue(s)

Acknowledgement

@Aga-C
Copy link
Member

Aga-C commented Apr 29, 2024

It should ask for the permission at least on the first app startup. Most people won't think about turning manually the permission. Remember that this app isn't done only for technically advanced users.

@austinoneil
Copy link
Author

@Aga-C Good idea. I added a check in the latest commit.

Comment on lines +187 to +188
val hasPermission = ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED
if (hasPermission) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be replaced with the extension function used in all Fossify apps:

hasPermission(PERMISSION_POST_NOTIFICATIONS)


val defaultReminders: Triple<Int, Int, Int>
get() {
val hasPermission = ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED
Copy link
Member

Choose a reason for hiding this comment

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

This should be replaced with the extension function used in all Fossify apps:

hasPermission(PERMISSION_POST_NOTIFICATIONS)

@naveensingh naveensingh added the waiting for author If the author does not respond, the issue will be closed. Otherwise, the label will be removed. label Nov 4, 2024
@naveensingh
Copy link
Member

naveensingh commented Nov 4, 2024

No, the app shouldn't ask for notification permission right at the startup, previous behavior was the correct behavior. To properly solve the issue of removing reminders without permission, we can try either of these options:

  • Add a cross button at the end of reminders fields that users can press to delete a given reminder.
  • Make the permission check more intelligent by asking for permission after the user selects anything other than the "No reminder" option. I like this option more.

@github-actions github-actions bot removed the waiting for author If the author does not respond, the issue will be closed. Otherwise, the label will be removed. label Nov 16, 2024
@naveensingh naveensingh added the waiting for author If the author does not respond, the issue will be closed. Otherwise, the label will be removed. label Nov 16, 2024
@fossifybot
Copy link

This pull request has been automatically closed due to inactivity. We requested additional information but have not received a response from the original author. Without the requested details, we cannot proceed. If you have the needed information or updates, please reopen the PR or comment so we can continue the review.

@fossifybot fossifybot closed this Dec 1, 2024
@Aga-C Aga-C removed the waiting for author If the author does not respond, the issue will be closed. Otherwise, the label will be removed. label Dec 1, 2024
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.

Cannot take off alarm without adding notification permission
4 participants