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

Android 13+ custom notification actions #10580

Closed
wants to merge 2 commits into from

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Nov 16, 2023

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

  • Supersedes Android 13 notification action buttons #10567 because the code there was too hacky.
  • Create class NotificationActionData and move there (from NotificationUtil) the function that turns actions from the enum format (i.e. the number stored into preferences) into the triple (action string, name, icon). This allows deduplication of such function, instead of passing arrays around like in Android 13 notification action buttons #10567.
  • Set custom action providers in the media session to customize actions on Android 13+.
  • Use the workaround initially suggested in Android 13 notification action buttons #10567 to basically tell the system that we're not able to handle "prev" and "next", in order to have 4 customizable action slots instead of just 2.
  • On Android 13+ normal notification actions are useless, so they are not set into the notification builder anymore, to save battery
  • The opposite happens on Android 12-, where media session actions are not set because they don't seem to do anything
  • Exclude the play-pause and play-pause-buffering actions from ever being set to media session actions, as the play-pause-buffering action is always shown by the system. The only difference between the system-provided play-pause-buffering and ours is that when the a video completes, the replay icon is not shown, but just a normal "play".
  • Currently the UI in the settings has not been changed. This can be done in another PR, which should just remove one of the 5 customizable actions and the play-pause* actions in the settings (just for Android 13+), along with a settings migration that should kick in on Android system updates.

Before/After Screenshots/Screen Record

Before After
Before After

Fixes the following issue(s)

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. You can find more info and a video demonstration on this wiki page.

Due diligence

Copy link

sonarcloud bot commented Nov 16, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@opusforlife2
Copy link
Collaborator

Is 5 still the max number of buttons allowed?

along with a settings migration that should kick in on Android system updates

When does this happen? Upon the first time the app is opened after a system version upgrade? Or as part of the upgrade process somehow?

@AudricV AudricV added bug Issue is related to a bug device/software specific Issues that only happen on some devices or with some specific hardware/software player notification Anything to do with the MediaStyle notification labels Nov 16, 2023
@Stypox
Copy link
Member Author

Stypox commented Nov 16, 2023

Is 5 still the max number of buttons allowed?

On Android 13+ it's 4 customizable buttons instead of 5, but at the moment the settings still show 5 actions.

When does this happen? Upon the first time the app is opened after a system version upgrade?

Yes, but as I said it's not implemented yet and will be done in a separate PR. This way we can merge this into the RC and at least provide something that works to the user, and later cleanup the settings for Android 13+.

@Stypox Stypox mentioned this pull request Nov 16, 2023
21 tasks
@AudricV AudricV added the size/large PRs with less than 750 changed lines label Dec 21, 2023
Use a workaround initially suggested in TeamNewPipe#10567
Basically tell the system that we're not able to handle "prev" and "next", in order to have 4 customizable action slots instead of just 2
On Android 13+ normal notification actions are useless, so they are not set into the notification builder anymore, to save battery
The opposite happens on Android 12-, where media session actions are not set because they don't seem to do anything
Copy link

sonarcloud bot commented Dec 23, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
1 Security Hotspot
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Comment on lines +50 to +59
// As documented in
// https://developer.android.com/about/versions/13/behavior-changes-13#playback-controls
// starting with android 13, setting ACTION_SKIP_TO_PREVIOUS and ACTION_SKIP_TO_NEXT forces
// buttons 2 and 3 to be the system provided "Previous" and "Next".
// Thus, we pretend to not support those actions to have the ability to customize them in
// MediaSessionPlayerUi.updateMediaSessionActions().
return ACTION_SKIP_TO_QUEUE_ITEM
| (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU
? ACTION_SKIP_TO_NEXT | ACTION_SKIP_TO_PREVIOUS
: 0);
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to hurt Bluetooth media controls like double pressing the pause button on a headset to skip to the next item?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this change prevents to do the action you described, I just tested the artifact built by the CI with a Bluetooth speaker.

@opusforlife2
Copy link
Collaborator

If it is possible, the app shouldn't generate a notification until it has all the metadata it needs. The user ends up getting an "Unknown - Unknown" notification for several seconds, which then eventually gets turned into the proper media notification.

@Stypox
Copy link
Member Author

Stypox commented Dec 29, 2023

Oh, nevermind then, this approach is not really usable unfortunately. I will open a PR which just allows customizing the 2 remaining buttons.

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 player notification Anything to do with the MediaStyle notification size/large PRs with less than 750 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android 13+] Missing notification player actions
4 participants