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

[feature request] Add oreo colored media notifications #1660

Closed
DanGLES3 opened this issue Sep 7, 2018 · 36 comments · Fixed by #3178
Closed

[feature request] Add oreo colored media notifications #1660

DanGLES3 opened this issue Sep 7, 2018 · 36 comments · Fixed by #3178
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background)

Comments

@DanGLES3
Copy link
Contributor

DanGLES3 commented Sep 7, 2018

Oreo allows media apps to set notification and buttons to match the video thumbnail's accent color,it would be great to have that on newpipe

@Stypox Stypox added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Aug 27, 2019
@Clae224
Copy link

Clae224 commented Sep 12, 2019

Making the non-standard NewPipe background player notification into a standard media player notification would solve this problem and a bunch of others: #1104 #1207 #2540 #1780 #2233 just to list a few.

In addition, Android 10 implements a seek bar in the notification itself, which would allow for seeking during playback from the notification.

I think this should be a priority feature request/milestone.

@alittlebitofit
Copy link

@Stypox I am asking this here cos the issue#2540 was closed.

You said:

You can create a local playlist from a remote one by playing it in background and then converting the background queue to a playlist.

How do I convert the background queue to a playlist?

@Stypox
Copy link
Member

Stypox commented Oct 2, 2019

Click on the notification to open the background queue, then click on the "add to playlist" button in the toolbar. @alittlebitofit

@alittlebitofit
Copy link

@Stypox Thanks :)

@Stypox Stypox added the player Issues related to any player (main, popup and background) label Oct 3, 2019
@hardBSDk
Copy link

hardBSDk commented Oct 3, 2019

@Stypox Create a playlist from the background queue would save the audio stream cache for play offline or is it a misunderstanding of me?

@Stypox
Copy link
Member

Stypox commented Oct 3, 2019

No, offline video caching does not exist in newpipe at the moment. What the button does is just creating a new local playlist containing the current items in the queue. @hardBSDk

@hardBSDk
Copy link

hardBSDk commented Oct 4, 2019

@Stypox Thanks!

@cool-student
Copy link
Contributor

cool-student commented Jan 13, 2020

So I looked into this over the weekend and I want to try to implement these new media style notifications, first up I want to clear up a misconception these colored media notifications are possible starting from Android Lollipop so all Android version that we support can use these (except of course the ancient kitkat).

I have a question about the scope of this though I hope someone can answer my question:

  • So these new notifications won't have a progressbar (except of course the ones on Android 10 via the new seekbar) our current ones have those tho so should I make a setting for people that want to keep using the current notifications ? Otherwise I would just add the new ones and make an exception for kitkat users. @TobiGr @Stypox @mauriciocolli ?

this is how my current work in progress looks like:
Screenshot_20200112-225852_One UI Home smaller

So the first notification here is from the vlc media player and then under that is a screenshot app you can ignore that, the one under that are the new colored notifications and below that the current notification. I can make the options of having a repeat and close icon available too when the notification is expanded. (but not when minimised since it would be too much clutter)

@Stypox
Copy link
Member

Stypox commented Jan 13, 2020

I really like the new notifications, though I am not sure about one thing: since KitKat is not supported, this would imply having to support and mantain two different notification types. An option to revert to the old notification would be a good idea.

The optimal thing would be to completely remove the code for the old notification (less code to maintain), but since this can't be done, I suggest enclosing both notifications in a common interface so that they are interchangeable (i.e. there would be no need for if (newNotification) ... else ...)

@cool-student
Copy link
Contributor

cool-student commented Jan 20, 2020

So I'm still working on this unfortunately I can only work on this on the weekend so my progress is slow. Something I realised recently after testing on Android Nougat, Oreo, Pie and 10 is that the colourization is indeed only possible on Oreo and up I initially assumed that it would be possible on Lollipop and up since Lollipop supports the media style notifications however Android < Oreo will simply ignore the colourization. Given this I will try to make it so that the new notifications will be enabled by default on Oreo and up and on Lollipop and up the current notifications will be the default unless someone enables them in the settings.

I have a test build but it's still early in development with some things not working yet, however if someone wants to test it here it is:

EDIT: apk got broken by youtube change. I'm going to compile a new version in a few days.

Changes in this build:

  • new media style notification for the background player that colour the notification based on the thumbnail colour (colourization only on Oreo and up)
  • progressbar in the download notifications (initially I tried some things out like having 1 notification per download as is suggested in Download notification with progress #2233 however I couldn't figure out how to make it work when the download queue is not limited to 1.)

Temporary Changes in this build:

  • Icon is gray (to better differentiate between multiple newpipe installations)
  • ACRA is not compiled in (I should probably enable this in the next build)

Known Issues:

  • Seekbar is not seek-able yet (Android 10 only)
  • Icons don't change yet when the state changes (pause to play and repeat icon)
  • Setting to enable the old notifications

@cool-student
Copy link
Contributor

cool-student commented Jan 27, 2020

I'm almost done with this. This weekend I got in all the important features that I wanted in and the notifications now work well on my test devices; the only thing I still need to do is code clean up and I need to add the setting to enable the old notifications. Around next week or the week after I should have my PR ready for this. In the meantime if anyone wants to test the current work in progress here is a test apk:

Fixes since v2:

  • seek bar is now seek-able
  • Icons change state depending on the Play state
  • made it so that the new notifications aren't updated all the time since it's not
    necessary on Android < 10.
  • fixed a progress bar bug with the download notifications

Known Issues:
#1660 (comment)

@spvkgn
Copy link

spvkgn commented Jan 27, 2020

@cool-student There is a regression in background player - seeking not works, playback always goes back to the beginning. Screenshot (Oreo 8.0)

@cool-student
Copy link
Contributor

@spvkgn ah yeah sorry about that, I commented out some code that updates the progress, I realised late at night that I commented it out at a place that would make it so that it won't update the backgroundplayeractivity's progress as well which is of course not good. So I'm aware of this issue and I will fix it in the next build.

@cool-student
Copy link
Contributor

cool-student commented Feb 3, 2020

I've chased down some bugs and have a new build ready:

Changes since v3-1:

  • background player activity progress was stuck on Android < 10
  • Android 10 seekbar seeking caused time to reset to 00:00 for a moment due to excessive notification recreation
  • added a buffering indicator icon when the stream is still being downloaded (this happens mostly on slow devices)
  • some lockscreen controllers showed the wrong play state
  • added a setting to restore the old notifications

Known Issues:

  • changing the notifications from old to new can cause a crash if the old notification wasn't closed
  • Android 10: livestream shows no seekbar fixed in v6
  • Android 10: cover image is not set correctly fixed in v6
  • Download Notification implementation is flawed I think I might revert this change for now until I can figure out a better way.

I still need to fix a few small bugs and clean up my code but the PR should come soon.

@Stypox
Copy link
Member

Stypox commented Feb 12, 2020

@cool-student the thumbnail in the new notification is squished and does not keep the 16:9 ratio. There are two ways to solve this, and each of them implies creating a new drawable based on the original thumbnail:

  • take only the central square of the image
  • make the image fit inside a square by adding transparent borders above and below

Another option would be to make the image space bigger, but I don't think this is a good idea since it would reduce space for buttons (and I don't even know if it is possible)

@cool-student
Copy link
Contributor

cool-student commented Feb 12, 2020

@Stypox so just for clarification you're not talking about the metadata album art that is shown on lock screen controllers but the image shown in the notifications itself ? @K1rakishou cropped the album art image in #2690 and I undid these changes but it depends on the oem skin how well they look.

this is how it looks on my lockscreen controller (and most modern Samsung phones S7 and newer):
old notifications:
Screenshot_20200202-181650_One UI Home small
new notificatons:
Screenshot_20200212-145649_One UI Home small

If it's about the image that is shown in the notifications itself then I have to think about it more, I didn't really consider this honestly and at the moment I just set what I get from the basePlayerImpl.getThumbnail()

Another option would be to make the image space bigger, but I don't think this is a good idea since it would reduce space for buttons (and I don't even know if it is possible)

I don't think this is possible we're not using remote views anymore for the new notifications so we can't do custom stuff like that, buttons are sadly also limited to 3 in compact view and 5 in expanded view otherwise I would have added a shuffle button but sadly it's not possible.

edit: maybe it looks different on your phone, can you show me how it looks for you ?

@Stypox
Copy link
Member

Stypox commented Feb 12, 2020

This is how it looks on my Android 7.1 Huawei p9 lite:
Screenshot_20200212-160331

@vkhomenk
Copy link

@cool-student I try to run your code but only get crash on attempting to watch video. The apk works fine. Am I doing something wrong?

@cool-student
Copy link
Contributor

@Stypox to be honest mediastyle notifications don't look so great below Oreo ^^';
I'll check on the weekend if I can fix the aspect ratio of the image or if I can find another way to make it look better on < Oreo devices.

@vkhomenk can you check your logcat so I can see what causes the crash, it should have an exception in there. Alternatively you can wait until Monday or Tuesday next week so I can compile a build that has ACRA enabled so that crashes show the crash dialog again.

@vkhomenk
Copy link

vkhomenk commented Feb 13, 2020

@cool-student I mean I get "this was not supposed to happen" screen in the app when attempting to watch video, while apk works fine

@vkhomenk
Copy link

@cool-student Whan you have time, please make a runable release. Thanks

@opusforlife2
Copy link
Collaborator

@vkhomenk Tried reinstalling? It works normally for me.

@cool-student
Copy link
Contributor

cool-student commented Feb 17, 2020

@Stypox so I checked what the notification look like on my LG G3 @ Nougat and I compared them to the vlc notifications (they also use mediastyle notifications) turns out they look pretty much the same so we can't improve the image size on the notifications.
But what I could do is fix the aspect ratio of the 16:9 image by turning it into a 1:1 image this should make the image look better.

@opusforlife2

After playing a background audio, I immediately felt the jarring lack of forward and rewind buttons and progress bar. I'm on Pie. Is it necessary to remove them?

about forward and rewind:
I can put those in but I think the way they worked in the old notifications is a bit counter intuitive since they only show up when you have only 1 item in the queue if you have more they disappear.
Another issue is that we can't have both prev and next at the same time as forward and rewind since we can only show 5 icons at once, this has the downside that when you have forward and rewind you cannot restart the track.
I think a better solution would be to have a setting to switch out prev and next with rewind and forward. What do you think ?

about the progressbar:
there is no progressbar on MediaStyle notifications. You get a seekbar (basically a superior progressbar) on Android 10 but not on older Android versions sadly.
The implementation that we used on the old notifications to make the progessbar work is also very "hacky" and leads to all kinds of issues.
If you feel strongly about the progressbar you can get a new device that has Android 10 or keep using the old notifications until you can upgrade.
It's very unfortunate that Google took so long to make MediaStyle notifications better...

@vkhomenk try the new build below.

Changes since v4:

  • merged dev into it up to commit 00b6bd5
  • buffering indicator wasn't showing up correctly on Android < 10
  • Android 10: livestreams showed no seekbar
  • Android 10: cover image wasn't set correctly
  • Android <= N MR1: scaled the notification image to a square aspect ratio to better fit
  • put forward and rewind back in

@opusforlife2
Copy link
Collaborator

@cool-student Sure! A toggle to choose between them would be great. My thinking is that with songs and such I generally want to skip tracks. With Youtube videos I generally find myself wanting to rewind.

Is it possible to have the setting control which pair of buttons are shown if there are multiple audios in the queue? Because it doesn't make sense to have the skip buttons if only a single audio is playing and the queue is empty. Then it should always show the forward and rewind buttons.

No progress bar is not a dealbreaker. My phone is getting LOS 17.1 soon anyway. ;)

@B0pol
Copy link
Member

B0pol commented Feb 18, 2020

Wow they look nice 😊
I found one unwanted behavior, I guess: when a video in background is finished, it won't close the notification automatically (I tested with vlc, it auto-close).
The same thing happens with playlists.

And another thing, when the audio is loading, the download symbol is shown, it feels weird because we don't download but only load (technically it's still downloading data, but it's streaming), but I don't know what could be used instead, and if something should be used instead.

Also, old notification are being used with popup player (and also with the lockscreen)
Thank you for your work

@Stypox
Copy link
Member

Stypox commented Feb 18, 2020

Buggy bebaviour: clicking on the download button blocks the notification (the video stops loading and no button works), and the only way to exit this strange situation is clicking the X button, that resets the notification.

@Stypox
Copy link
Member

Stypox commented Feb 18, 2020

Also, the thumbnail is still not 1:1 in app-debug-v6

EDIT: sorry, I didn't see you wrote "I could" ;-)

@opusforlife2
Copy link
Collaborator

@B0pol

I found one unwanted behavior, I guess: when a video in background is finished, it won't close the notification automatically (I tested with vlc, it auto-close).
The same thing happens with playlists.

No no no no no that's not unwanted behaviour at all! When you finish watching a video, you can replay it without downloading anything because it gets cached. The same thing happens with the audio. While the notification stays, the audio stays in the cache, and can be replayed any number of times without incurring additional data costs. This is a feature, not a bug.

@cool-student
Copy link
Contributor

cool-student commented Feb 19, 2020

Thank you guys for all the feedback so far.

@Stypox

Buggy bebaviour: clicking on the download button blocks the notification (the video stops loading and no button works), and the only way to exit this strange situation is clicking the X button, that resets the notification.

good catch I'll try to fix it.

Also, the thumbnail is still not 1:1 in app-debug-v6
EDIT: sorry, I didn't see you wrote "I could" ;-)

your first interpretation was correct the fix is in v6 so I'm confused why it doesn't work for you. Does your device not consider itself to be a part of <= Android N MR1 (API level 25) ? Have you tested it with more than 1 stream ? I could've sworn that it worked on my LG G3 @ Nougat...

@B0pol

I found one unwanted behavior, I guess: when a video in background is finished, it won't close the notification automatically (I tested with vlc, it auto-close).
The same thing happens with playlists.

as @opusforlife2 explained this is simply the current behaviour I didn't change it. However there is a future request somewhere requesting what you described as an optional feature. I will look at that again when I'm done with this initial PR.

And another thing, when the audio is loading, the download symbol is shown, it feels weird because we don't download but only load (technically it's still downloading data, but it's streaming), but I don't know what could be used instead, and if something should be used instead.

The reason why I added this is that I noticed on one of my old devices the audio would sometimes abruptly stop for a while and I initially didn't know why, so I wanted to indicate the buffering state.

Also, old notification are being used with popup player (and also with the lockscreen)

good catch I didn't even know that the popup player used a notification.

@opusforlife2

Is it possible to have the setting control which pair of buttons are shown if there are multiple audios in the queue? Because it doesn't make sense to have the skip buttons if only a single audio is playing and the queue is empty. Then it should always show the forward and rewind buttons.

I might have phrased this poorly but there is actually a use case for the previous button even if you only have 1 track and that is to restart the track. There was an attempt to add the previous button to the old notifications in that scenario for that reason (#2726) .
Thinking about it more ideally I would make a setting where you can choose between
(1) fastrewind/fastforward
(2) prev/next
(3) if queue == 1 fr/ff else prev/next (maybe we'll call it default or something like that ?)
I think this would be the best solution.


One more thing that I've been thinking about; I was thinking of removing the close button and replacing it with a deleteintent instead this would enable us to add a shuffle button. The way a delete intent works is that you need to pause the audio playback and then you can swipe away the notification, it's pretty much what the vlc guys are doing and I think we should do this too. What do you think ?

@B0pol
Copy link
Member

B0pol commented Feb 19, 2020

One more thing that I've been thinking about; I was thinking of removing the close button and replacing it with a deleteintent instead this would enable us to add a shuffle button. The way a delete intent works is that you need to pause the audio playback and then you can swipe away the notification, it's pretty much what the vlc guys are doing and I think we should do this too. What do you think ?

First, I think the cross button is better, because without it, the user need to know the trick (pause and swipe). And how you will inform him? There are no in-app changelog, no tours…

Secondly, you should be able to add the shuffle button even with the cross. Look at spotify, they have like button + controls + cross.
https://i.ibb.co/fQBq0Qc/Screenshot-20200219-091301-Notes.png

@cool-student
Copy link
Contributor

Secondly, you should be able to add the shuffle button even with the cross. Look at spotify, they have like button + controls + cross.
https://i.ibb.co/fQBq0Qc/Screenshot-20200219-091301-Notes.png

look at the picture again. there are only 5 buttons. that's also the number we are limited to, we already have prev play/pause next repeatmode close

@cool-student
Copy link
Contributor

First, I think the cross button is better, because without it, the user need to know the trick (pause and swipe). And how you will inform him? There are no in-app changelog, no tours…

yeah this is true but vlc is also doing it ...

but yeah if people don't like it I'll keep it as is for now and maybe later I'll add a setting for the button layout or something like that

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Feb 19, 2020

Thinking further, since there are 5 possible slots, the defaults should be ones which would see the maximum use. I suggest these options:

  • Play/pause
  • Rewind
  • Forward
  • Close
  • Restart

Since Shuffle and Repeat are queue operations, and not track specific like the 5 above, they could be changed by opening the background player from the notification like normal. I don't recall seeing any app where these options are exposed through the notification, anyway.

Edit: Does Android allow long pressing a notification button like you can within an app for a different function than normal? I was thinking it would be neat to have the forward/rewind buttons also function as previous track/next track upon long press.

Edit 2: Never mind Edit 1. Any long press anywhere brings up notification options. Google should really add this feature, though. It would make notifications more flexible.

@cool-student
Copy link
Contributor

cool-student commented Feb 20, 2020

@opusforlife2

the defaults should be ones which would see the maximum use

there are many use cases so it's not easy to decide; for example someone that listens to an eminem playlist would probably want prev and next to skip through tracks that he doesn't like and forward and rewind are useless in this case. (on Android 10 in particular forward and rewind become less important since you can just seek to a position)

Since Shuffle and Repeat are queue operations

being a queue operation doesn't mean much; prev and next are also queue operations but can be quite useful to have depending on the use case.
I do agree though that at least in the default configuration we don't need to have shuffle and repeat.

just as a food for thought mx player is using the following buttons for their notifications:
prev rewind play forward next (in addition to that they also have a close button but that's because they're using remote view notifications which look worse on newer android versions and they don't even use a mediasession so lockscreen controllers can't be used)


edit:
thinking about it more what if it was completely customizable for every slot ? so for example we could have these settings:

slot 0: prev or rewind or smart
slot 1: play/pause/buffering or play/pause
slot 2: next or forward or smart
slot 3: repeat or shuffle or close or rewind or N/A
slot 4: close or repeat or shuffle or forward or N/A

some explanation: smart just means that we show forward/rewind when there is 1 track and prev/next when there are more, basically what we do now.
N/A is possible on slot 3 & 4 since we will have a deleteIntent so a close button is optional and on Android 10 you can see more of the image if there are only 3 slots used.

@opusforlife2
Copy link
Collaborator

on Android 10 in particular forward and rewind become less important since you can just seek to a position

Not for long enough videos. For those, tapping on the seek bar becomes very inaccurate - an annoying exercise in hunting for the correct timestamp - and the buttons become essential.

prev and next are also queue operations

I knew I was forgetting something important when I wrote the comment! Yes. Previous and next are very essential for multiple items in the queue. Now I think the new defaults should be:

  • prev rewind play/pause forward next (just like MX, as you wrote), where prev also functions as restart
  • deleteIntent for closing
  • shuffle/repeat by opening the background player

what if it was completely customizable for every slot ?

That would be wonderful! :) More user choice is the democratic way of going about things.

@cool-student
Copy link
Contributor

@opusforlife2

Not for long enough videos. For those, tapping on the seek bar becomes very inaccurate - an annoying exercise in hunting for the correct timestamp - and the buttons become essential.

you're right I was thinking about short tracks like music.

I think your overall suggestion sounds good so I will use the mx player layout for the next build.


There are still a few things that I need to fix so the new "notification slot settings" might not make the next build. As soon as I am done with the code refactoring (only code changes to make the code more readable and better but no functional changes) I will provide the PR. So the next build might just be a bug fix build; tbh I'm a little sad that I'm a bit behind in my personal schedule, I wanted to have the PR up sooner since I might also need to address things from the code review but I just couldn't get it done earlier and I also had some confusing bugs that only happened on Android 10.

A list of things I still need to do:

  • revert download notification changes (my implementation is buggy and I need to think about these changes more, I decided to move this to a new PR after the initial notification improvement one)
  • fix popup player notification
  • improve image scaling fix for Android <= N MR1
  • Code Refactoring
  • change the default button layout to mx player style (this might not be the final default layout but it will be in the next build so that I can gather feedback on it)
  • add notification slot settings (this might happen in the build after the next one)
  • add deleteIntent
  • other misc bug fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants