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+] Restore support of custom notification actions #10712

Merged
merged 9 commits into from
Dec 30, 2023

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Dec 29, 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+ custom notification actions #10580 and Android 13 notification action buttons #10567 because the hacky workaround to allow customizing 4 actions turned out to cause problems
  • Set notification actions only via MediaSessionConnector on Android 13+, and only via the notification builder on Android 12-, to save battery
  • Notification actions in MediaSessionPlayerUi are only updated when something really changes, to avoid setCustomActionProviders() calling invalidateMediaSessionPlaybackState() more times as needed, which I figure might be costly
  • Adapts the player notification settings page to allow customizing only two actions on Android 13+. The preference keys for the first three actions are actually still set, to avoid complicating the logic too much, and to allow seamless transitioning between Android 12- and 13+ back and forth (e.g. when a user exports and imports a database).
  • Allows each notification slot to contain any possible action (which required changing how compact notification slot indices are computed, since the assumption that "Nothing" slots are always last does not hold anymore)
  • Allows play/pausing from the notification when the player is buffering, which is in line with what happens in the main player ui
  • Cleans up the code by extracting NotificationSlot from NotificationActionsPreference, and NotificationActionData from NotificationUtil

I tested a bit and caught some small inconsistencies in the code that now are fixed, and everything seems to work as expected.

Before/After Screenshots/Screen Record

Before notification After notification Settings in 12- Settings in 13+
Before After After 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

This should avoid costly updates of the media session.
This change is in line with a recent change in how the play/pause button behaves in the player ui: if the buffering indicator is shown, it's still possible to toggle play/pause, to allow e.g. pausing videos before they even start.
This change was needed because on Android 13+ notification actions can't be null, and thus the buffering hourglass action wasn't shown.
@Stypox Stypox force-pushed the notification-actions-api-33-2 branch from b7c574e to 4b1824e Compare December 29, 2023 15:18
@TobiGr TobiGr added player notification Anything to do with the MediaStyle notification bug Issue is related to a bug labels Dec 29, 2023
@opusforlife2
Copy link
Collaborator

Allows play/pausing from the notification when the player is buffering, which is in line with what happens in the main player ui

NICE. I've wanted to be able to do that often.

@AudricV AudricV added device/software specific Issues that only happen on some devices or with some specific hardware/software codequality Improvements to the codebase to improve the code quality labels Dec 29, 2023
@AudricV AudricV changed the title Android 13+ custom notification actions [Android 13+] Restore support of custom notification actions for customizable actions Dec 29, 2023
@opusforlife2
Copy link
Collaborator

#10580 (comment) is still true here. In fact, it's especially noticeable if you don't load video details, and directly play audio from the feed or search results.

The list of actions needs to be cut down to remove the default fixed actions on A13+: Previous, Next, Play/Pause, Play/Pause/Buffering, Rewind/Previous, Fast-forward/Next. This will halve the list from 12 to 6 actions.

@Stypox
Copy link
Member Author

Stypox commented Dec 30, 2023

Is #10580 (comment) a bug of this PR? I think it's more of a consequence of AudricV's PR that fixed ANR popups by showing a notification asap. I know it's not perfect but it's better to show the notification asap to avoid getting errors for taking too much time to show the foreground service notification.

About cutting down actions: you're right but maybe some users would find some use for some of those options (even if they (partially) overlap with the already shown ones), so I wouldn't remove them as of now.

Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Code looks good to me. I did some testing and was not able to find any issues on API 33, 32, 29, 26.
(Please apply my above suggestion before merging)

@dvalter
Copy link

dvalter commented Dec 30, 2023

For me it brings a huge regression compared to #10580 and pre-33 notifications.
On my devices (A13, LineageOS) it removes rewind/fast-forward from the default notification actions (and an option to configure force rewind/fast-forward for playlists).

It would be better to keep prev/next buttons semi-configurable for 33+ in settings rather than crippling their functionality.

@Stypox
Copy link
Member Author

Stypox commented Dec 30, 2023

For me it brings a huge regression compared to #10580 and pre-33 notifications.

Yeah, I know... But that's how Google intended it, and I agree they made a bad choice there...

Unfortunately we can't add a configurable switch to enable the hacky workaround for #10580 for various reasons

  • maintenance is difficult when there are too many settings, due to more unexpected bugs popping up
  • most users wouldn't stumble upon the setting anyway, even if they need it, considering how niche this change is
  • users that enable the setting and then forget about it will start arguing that their bluetooth controls don't work

@Stypox Stypox changed the title [Android 13+] Restore support of custom notification actions for customizable actions [Android 13+] Restore support of custom notification actions Dec 30, 2023
@Stypox Stypox merged commit 1d8850d into TeamNewPipe:dev Dec 30, 2023
4 of 7 checks passed
@opusforlife2
Copy link
Collaborator

I think it's more of a consequence of AudricV's PR that fixed ANR popups

Oh, okay. Never mind, then. Though, is it possible to customise the dummy notification? Something like "Stream loading" until it's ready to be replaced by the actual content?

Copy link

Quality Gate Passed Quality Gate passed

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

2 New issues
1 Security Hotspot
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@Stypox
Copy link
Member Author

Stypox commented Dec 30, 2023

I guess that would be simple to do, yeah.

@opusforlife2
Copy link
Collaborator

Need a new issue? Or is this enough of a TODO?

@dvalter
Copy link

dvalter commented Dec 30, 2023

Yeah, I know... But that's how Google intended it, and I agree they made a bad choice there...

They rarely make something new without screwing up something good and old.

users that enable the setting and then forget about it will start arguing that their bluetooth controls don't work

That made me think, I have an idea, how to improve the UX in the most broken case. I'll open a new issue then.

@dvalter
Copy link

dvalter commented Dec 30, 2023

@Stypox looks like #7880 is what I was thinking about.

Reassigning next/prev actions for the whole player should also do the trick with A13 notifications. And that may even be used in a "smart next/forward" mode to make notification less useless for a single video playback (which is 99% of my usage of NewPipe).

@Stypox
Copy link
Member Author

Stypox commented Dec 30, 2023

Thanks for looking it up! Yeah #7880 makes more sense than an option just for the notification

@opusforlife2
Copy link
Collaborator

So... you want a new issue as TODO?

@Stypox
Copy link
Member Author

Stypox commented Jan 1, 2024

Ok

@einzel
Copy link

einzel commented Jun 10, 2024

Where should I post if I am still encountering a problem to this? I am on Android 14 and my notification icon shows no control buttons and only forwards to the app or closes by swiping.

@opusforlife2
Copy link
Collaborator

@einzel New issue.

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 codequality Improvements to the codebase to improve the code quality 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android 13+] Missing notification player actions
6 participants