-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Conversation
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.
b7c574e
to
4b1824e
Compare
NICE. I've wanted to be able to do that often. |
app/src/main/java/org/schabi/newpipe/settings/custom/NotificationSlot.java
Outdated
Show resolved
Hide resolved
#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. |
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. |
There was a problem hiding this 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)
For me it brings a huge regression compared to #10580 and pre-33 notifications. It would be better to keep prev/next buttons semi-configurable for 33+ in settings rather than crippling their functionality. |
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
|
Co-authored-by: Tobi <[email protected]>
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? |
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 2 New issues |
I guess that would be simple to do, yeah. |
Need a new issue? Or is this enough of a TODO? |
They rarely make something new without screwing up something good and old.
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. |
@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). |
Thanks for looking it up! Yeah #7880 makes more sense than an option just for the notification |
So... you want a new issue as TODO? |
Ok |
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. |
@einzel New issue. |
What is it?
Description of the changes in your PR
MediaSessionConnector
on Android 13+, and only via the notification builder on Android 12-, to save batteryMediaSessionPlayerUi
are only updated when something really changes, to avoidsetCustomActionProviders()
callinginvalidateMediaSessionPlaybackState()
more times as needed, which I figure might be costlyNotificationSlot
fromNotificationActionsPreference
, andNotificationActionData
fromNotificationUtil
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
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