Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Updated NotificationHelper to use highest notification priority (PRIORITY_MAX) #203

Merged

Conversation

jomapp
Copy link
Contributor

@jomapp jomapp commented Jun 5, 2020

We should ensure that notifications always have the highest priority also on older devices.
Resolves #202

Checklist

  • Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
  • If this PR comes from a fork, please Allow edits from maintainers
  • Set a speaking title. Format: {task_name} (closes #{issue_number}). For example: Use logger (closes # 41)
  • Link your Pull Request to an issue (if applicable)
  • Create Work In Progress [WIP] pull requests only if you need clarification or an explicit review before you can continue your work item.
  • Make sure that your PR is not introducing unnecessary reformatting (e.g., introduced by on-save hooks in your IDE)

Description

…RITY_MAX)

We should ensure that notifications always have the highest priority also on older devices.
@timbrueggenthies
Copy link
Contributor

To use the high priority, it is also needed to adjust this setting on the NotificationChannel on Devices >= API 26 (see here)

@jomapp
Copy link
Contributor Author

jomapp commented Jun 6, 2020

To use the high priority, it is also needed to adjust this setting on the NotificationChannel on Devices >= API 26 (see here)

For the NotificationChannel the current setting ("IMPORTANCE_HIGH") is already the highest. The Google documentation specifies "unused" for "IMPORTANCE_MAX". (see here)

@timbrueggenthies
Copy link
Contributor

Okey, didn't knew that. Thanks for pointing out! 👍

@jomapp
Copy link
Contributor Author

jomapp commented Jun 6, 2020

Okey, didn't knew that. Thanks for pointing out! 👍

Sure :) Was also quite surprised seeing that "IMPORTANCE_MAX" is not used anymore.

@htooisap
Copy link
Contributor

htooisap commented Jun 7, 2020

@jomapp thank you for your contribution this. As you already correctly noted, we are using PRIORITY_HIGH because PRIORITY_MAX is not in used.

@htooisap htooisap closed this Jun 7, 2020
@jomapp
Copy link
Contributor Author

jomapp commented Jun 7, 2020

@jomapp thank you for your contribution this. As you already correctly noted, we are using PRIORITY_HIGH because PRIORITY_MAX is not in used.

You're welcome but it's different from what you wrote. We should change it to PRIORITY_MAX.

In the project we are currently using IMPORTANCE_HIGH, for the NotificationChannel, which is fine (since IMPORTANCE_MAX is unused.) But for the notification itself we should use PRIORITY_MAX instead of PRIORITY_HIGH since for older devices below API 26 this ensures that notifications are also shown with the highest priority.

Infact: IMPORTANCE_HIGH (>= API 26) == PRIORITY_MAX (< API 26)

@jomapp
Copy link
Contributor Author

jomapp commented Jun 8, 2020

@HeeTattSap Can we reopen this PR? I think it's relevant (please see my last comment) Thank you!

@htooisap htooisap reopened this Jun 8, 2020
@htooisap
Copy link
Contributor

htooisap commented Jun 8, 2020

@jomapp thank you. I misunderstood that this was about the channel.

Your fix works and I have validated this on Android 6 and Android 10 devices. We will merge this change. Thank you again for your contribution :)

@htooisap htooisap merged commit bf26eb4 into corona-warn-app:dev Jun 8, 2020
@jomapp
Copy link
Contributor Author

jomapp commented Jun 8, 2020

@HeeTattSap thanks! No worries, thank you for merging it :)

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

Successfully merging this pull request may close these issues.

4 participants