902974 - [Music] [User Story] Utility tray should display information about the song when music is still playing

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Sri Kasetti]

User Story:
As a user, when I swipe down the play icon on the status bar I want the utility tray to display the current song playing, album art

Acceptance Criteria:
1. If the user finds the play icon on the status bar and decides to take some action, he/she can swipe the notifications icon or swipe down from the top of the screen. This action opens up the utility tray with details of the current song at the bottom of the utility tray 
2. The utility tray should display the song playing, the album art and the music icon 
3. The user by selecting the song in the utiity tray can go to the music app to take some action

Comment 1

[Jim Porter (:squib)]

Taking this and setting blocking bugs to define the order of how things should go.

Comment 2

[Jim Porter (:squib)]

For those following along, my PR is here: <https://github.com/jimporter/gaia/tree/music-nowplaying-notification>. I'm mostly done, except for integration tests and fixing up some album art issues.

On the subject of album art, one thing that's making this more complex than necessary is that we essentially have two kinds of album art: embedded album art in the audio file, and the placeholder album art. It would be nice if we could merge these two. djf had some good ideas about using <canvas> to create unique placeholder art with the artist and album name. It might actually be easier to do that than to try to send the existing placeholder art across apps.

If both of the above options turn out to be too hard, we could also go with a quick fix: copy the placeholder art from the music app to the system app, and then just send over the background-index telling us which placeholder to use.

Comment 3

[Jim Porter (:squib)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/11847

Ok, this should be done now. There are some tests, although they aren't as thorough as I'd like (the music app needs some test infrastructure).

Everything should be designed so that other (certified) apps can hook into this. However, weird things might happen if multiple music apps tried to send now playing info; luckily that can't happen yet.

For now, I put the inter-app comms code for the music app in apps/music/js/music.js. Once Dominic lands his Bluetooth AVRCP changes, I'll move this code over to wherever he puts his shared remote control code.

Comment 4

[James Lal [:lightsofapollo]]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

[my review is about tests only]

I think you could take this another step further by running the settings stuff from another app but this is an excellent starting point and it reads well to me.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

f+ because I had argued before that I shouldn't land workaround if due day is not this week. An least acceptable solution to me is a followup bug to use IAC to replace the settings API hack, and put the bug number in this patch.

Again, I don't mean I accept this kind of hack even you have a followup. I'm not a manager but, please, be aggressively on tracking  and pushing IAC implementation status to remove this workaround.

It's front-end engineer's duty and honor to link and combine the backend's and the UX's effort together to reach the wonderful world.

Lastly, I don't want to see there would be another person say, hey, music is rushing with mozSettings API, so you shall accept that I implement XXXXXX feature with mozSettings API hack.

I believe I had stated my concern very well, if someone thinks I'm wrong, please steal the review or set review to another one.

Comment 6

[Jim Porter (:squib)]

Yeah, I don't much like landing this either without the IAC API. We could also do something where we don't land this, but I keep working on other parts of this on a branch (e.g. the music controls).

Comment 7

[Dominic Kuo [:dkuo]]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

Jim,

The code looks good, except one minor issue that before passing the metadata to system app, the empty album/artist should localized to the Unknown strings. And are you able to see the album art that set in the mozSettings then notify to the system app? The code of getAlbumArtBlob() looks correct but I only see the default music icon/placeholder on the notification bar, I am giving r+ assuming the album art works for you, or do I miss anything?

Comment 8

[Jim Porter (:squib)]

Thanks for the catch on the localization and album art, Dominic! It turns out I had added some error detection for grabbing the album art that broke things. I've committed some fixes if you want to look at them.

Comment 9

[Jim Porter (:squib)]

Sri, do you have any comments re: comment 5 and 6? I'd sort of prefer to go with Alive's advice of not landing this with the Settings API, especially since it seems like the IAC API is making decent progress. That said, I think this is more up to product folks since this feature may be important enough to land some ugly code.

Comment 10

[Sri Kasetti]

From product perspective this feature and bug 891014 go together. While I agree with comments in 5 and 6 do we know if there is any performance or user experience impact if we land this? If there is no impact then as Alive suggested let's file a bug to replace this implementation with IAC API once it is ready. Also isn't IAC API supposed to be done end of August?

Comment 11

[Jim Porter (:squib)]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

Alive: I've updated my pull request to use the inter-app comms API from bug 876397. Do these changes look sane to you?

Comment 12

[Jim Porter (:squib)]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

James: Asking for review on the tests again since I pretty much totally rewrote them.

Gene: Could you take a look at this and make sure I'm using the IAC API correctly? As mentioned in bug 876397, it seems that sometimes the IAC message port stalls and no new messages go through. I'm not sure if that's a bug with your code or with mine, since I haven't found a way to reliably reproduce it yet.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

Awesome.
I'd like to know is it safe to land this before IAC platform work is landed?

Comment 14

[Gene Lian (I already quit Mozilla)]

Hi Jim! I'm pinging Bent to take a look on the final patch. I'll give it a try based on your codes. Are you joining the Oslo work week? I'm hoping to have a quick talk with you. Thanks!

Comment 15

[Jim Porter (:squib)]

(In reply to Alive Kuo [:alive] from comment #13)
> Comment on attachment 797440 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/11847
> 
> Awesome.
> I'd like to know is it safe to land this before IAC platform work is landed?

It should be safe if I make a couple of tweaks. Right now, the music app would keep a queue of all the messages it hasn't sent (and can't send without IAC), meaning that we'd end up consuming more and more memory. I could probably modify the code so that we don't queue up messages when IAC isn't present, though.

Comment 16

[Jim Porter (:squib)]

(In reply to Gene Lian [:gene] (please use needinfo?) from comment #14)
> Hi Jim! I'm pinging Bent to take a look on the final patch. I'll give it a
> try based on your codes. Are you joining the Oslo work week? I'm hoping to
> have a quick talk with you. Thanks!

I am at the work week. I should be free in a few minutes to chat. I'll ping you on IRC when I'm ready and we can meet up.

Comment 17

[Rob MacDonald [:robmac]]

Hi Jim - For the UI review, let's just meet at your desk here in Oslo. - Rob

Comment 18

[Gene Lian (I already quit Mozilla)]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

The Gaia codes look good!

Regarding the random issue of messaging failure, I've been spent half of a day to investigate that, eventually I found it's a general IPC issue instead of the IAC logic error. I heard someone saying we're having the same issue in the WebApp APIs. Sound like a regression.

Comment 19

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (please use needinfo?) from comment #18)
> Comment on attachment 797440 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/11847
> 
> The Gaia codes look good!
> 
> Regarding the random issue of messaging failure, I've been spent half of a
> day to investigate that, eventually I found it's a general IPC issue instead
> of the IAC logic error. I heard someone saying we're having the same issue
> in the WebApp APIs. Sound like a regression.

This is a know issue at Bug 915598.

Comment 20

[Jim Porter (:squib)]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

ui-review+ from Rob MacDonald in-person.

Alive: Sorry to bug you for review again, but I added the ability to open the music app by clicking on the Now Playing notification. I just wanted to make sure the way I did it is ok. The changes are in a separate commit in my PR to make it easier to review. Thanks!

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

Commented

Comment 22

[Jim Porter (:squib)]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

Alive: I updated the PR. Would this be an ok way to address your comments on GitHub? It seems a bit hackish. Maybe it would be better to create a new type of event to do this?

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

Let's change as discussed in github.

Comment 24

[Jim Porter (:squib)]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

Alive: Ok, I did as you suggested on GitHub. How does this look? I tried to write a Marionette test for the "displayapp" code, but I'm pretty sure I was running into some bugs in Marionette (e.g. I waited for an element to be displayed, and then immediately clicked on it, and got an error saying it wasn't visible).

Dominic: I changed a bunch of my code on the music app side since you landed your Bluetooth changes. Does this still look good?

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

Great!

Comment 26

[Jim Porter (:squib)]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

David, could you take a look at this (just the music app parts)? Dominic is out at the moment. Note that it already had an r+ from Dominic earlier, so you probably don't have to be as thorough as you would for a full review. Thanks!

Comment 27

[David Flanagan [:djf]]

Comment on attachment 797440 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11847

r+, but I'm not thrilled with the code that xhr's a blob from the blob url. The album art cache ought to be able to return the blob directly.

Also, please don't land without resolving the issue where any durationchange event causes the album art to be xhr'ed again.

Comment 28

[Jim Porter (:squib)]

Setting koi? since this is supposed to land for 1.2. Hopefully this is the right thing to do. If not, koi- it, I guess!

Note that this will be landing momentarily on master, once I see a happy Travis run.

Comment 29

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/commit/63a90ad59764cd327ac60c69522efefd1ee5ffb9

Comment 30

[Hema Koka [:hema]]

Yes based on the email discussions with product/ux/qa, this is targeted for 1.2.

Comment 31

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 63a90ad59764cd327ac60c69522efefd1ee5ffb9 to:
v1.2: 647a85534cd02231dc617e6b930d285e5cf37ed5

Comment 32

[Marcia Knous [:marcia]]

Note that the user story indicates that this was supposed to be at the bottom of the utility tray, but in the UI I am seeing it at the top.

Comment 33

[Jim Porter (:squib)]

I think we actually *want* it at the top (mainly for when 1.3 comes along and we have the playback controls included). That makes it easier to get at the controls to pause the music, which I think is pretty important. I mentioned this to Rob and I recall him agreeing, but just to be sure, I'll double-check with him.

Rob, thoughts on comment 32 (and this comment)?

Comment 34

[Sri Kasetti]

I remember agreeing to moving the controls to the top. Rob, please confirm...

Comment 35

[Rob MacDonald [:robmac]]

With two corroborating witnesses, I did indeed agree to move the controls to the top. Thanks!