891024 - [Music] [User Story] Provide access to music player controls when the screen is locked

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Music, enhancement, P2)

Description

[Sri Kasetti]

User Story: 

As a user while playing music on my phone and the homescreen is locked, I want to access music player controls without having to unlock the screen

Comment 1

[Jim Porter (:squib)]

Dominic: I've already started on parts of this, and I believe Dave Hylands was going to work with me on this.

Comment 2

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

Sorry for my jumping in without invitation to this issue.

I am heard that we'll introduce mozSettings abuse in this prototype,
and as you know this is a really sad hack.
I understand (1) the timeframe issue and (2) there're already some abuse of mozSettings in current b2g.

My thoughts:
* We already killed somebody doesn't mean we should kill another ;)
* Personally I really ran into many troubles from the idea: 'It just works'.
* We could still work on a long term solution in parallel with the gecko team. I think CJ's team would provide help here, either to fulfill David's original MediaControl design or enhance it to meet all UX requirement we have. Discussed w/ him yesterday, he intended not to use a generic communication API but make a specific one. Anyway, any of these is better than abuse mozSettings.
* There maybe some problems about the music app is crashed w/o updating mozSettings. With this we couldn't avoid to monitor 'protocol://music.gaia-mobile.org/'s life cycle in system app by hard coding the origin of music app.
* Pay attention to visibilitychange manipulation "in system app". "on lockscreen" and "on utility-tray".

So may I know what's the long term plan if we couldn't avoid that?

Sorry again to disturb you guys w/o invitation.

Comment 3

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

Also, have music app keep setting R/W permission for this reason is really sad..

Comment 4

[Jim Porter (:squib)]

After talking with Hema, I think the plan right now is that I'll implement this using the Settings API, but I *won't* land it until we have a proper API I can replace it with. I think the generic inter-app comms API is the one we want. I can't recall all the details of the media control API, but one issue I had was that I think we should be sending all the commands directly to the music app. This is especially important when hitting next/previous track when the music app is set to shuffle. (The shuffle logic should be handled by the music app, in part so that alternate music apps can invent their own shuffling rules.) A generic inter-app comms API satisfies all our needs there, and has the added bonus of being useful for other apps too.

Comment 6

[Dominic Kuo [:dkuo]]

Attachment: Music Player 1.2 release spec., v0.2

Comment 7

[Jim Porter (:squib)]

(In reply to Dominic Kuo [:dkuo] from comment #6)
> Created attachment 783632 [details]
> Music Player 1.2 release spec., v0.2

Hm, it seems I don't have access to this file. I had this issue before, and never got a clear answer on whether I should just create a box account or if I need to talk to someone to get an account with the right privileges...

Comment 8

[Jim Porter (:squib)]

Just a quick update on where I'm at: I've gotten the lockscreen UI to look pretty close to the final UI specs, though there are still a few kinks to work out. Most importantly, if anyone knows a lot about the behavior of the lockscreen's elastic "bounce", I could use some pointers for how best to totally disable it when the playback controls are present.

Comment 9

[Jim Porter (:squib)]

Mainly a note to myself, but I/we will need to investigate this issue: when you pause a track in the music app, then go to the lockscreen and try to hit play, it doesn't work, and the following error is logged in logcat:

  W/AudioFlinger( 2831): Thread AudioOut_1 cannot connect to the power manager service

This looks like some low-level Android stuff, so I don't know what to do about it, but I'm guessing the fix isn't in Javascript-land!

Comment 10

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

c.c. Yuren for c8

Comment 11

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

c.c. mchen for c9, he is the guy knows android audio well. But I bet you have better to provide gaia source code.

Comment 12

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

(In reply to Jim Porter (:squib) from comment #9)
> Mainly a note to myself, but I/we will need to investigate this issue: when
> you pause a track in the music app, then go to the lockscreen and try to hit
> play, it doesn't work, and the following error is logged in logcat:
> 
>   W/AudioFlinger( 2831): Thread AudioOut_1 cannot connect to the power
> manager service
> 
> This looks like some low-level Android stuff, so I don't know what to do
> about it, but I'm guessing the fix isn't in Javascript-land!

OOC, I guess you know the rule of audio competing / background play?

Comment 13

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

you can use LockScreen.setElasticEnabled() to disable it.

Comment 14

[Jim Porter (:squib)]

(In reply to Alive Kuo [:alive] from comment #12)
> (In reply to Jim Porter (:squib) from comment #9)
> >   W/AudioFlinger( 2831): Thread AudioOut_1 cannot connect to the power
> > manager service
> > 
> OOC, I guess you know the rule of audio competing / background play?

No, I don't. Do you have a source for that which explains it? And preferably, how to fix it? :)

(In reply to Yuren Ju [:yurenju] from comment #13)
> you can use LockScreen.setElasticEnabled() to disable it.

Sorry, I forgot to update that I'd fixed this issue, but for posterity, setElasticEnabled wasn't enough for me, since other events would re-enable it. I had to add a setElasticAllowed method.

Comment 15

[Jim Porter (:squib)]

Oh, if you'd like to look at the source code for this branch, you can find it here: https://github.com/jimporter/gaia/tree/music-lockscreen

Comment 16

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

(In reply to Jim Porter (:squib) from comment #14)
> (In reply to Alive Kuo [:alive] from comment #12)
> > (In reply to Jim Porter (:squib) from comment #9)
> > >   W/AudioFlinger( 2831): Thread AudioOut_1 cannot connect to the power
> > > manager service
> > > 
> > OOC, I guess you know the rule of audio competing / background play?
> 
> No, I don't. Do you have a source for that which explains it? And
> preferably, how to fix it? :)
> 

https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.gaia/HyAlHdFdX68

https://wiki.mozilla.org/WebAPI/AudioChannels

But I look into your code and it's still using content channel...so I don't know what happens. Maybe mchen could offer help.

Comment 17

[Marco Chen [:mchen]]

(In reply to Jim Porter (:squib) from comment #9)
>   W/AudioFlinger( 2831): Thread AudioOut_1 cannot connect to the power
> manager service
> 
> This looks like some low-level Android stuff, so I don't know what to do
> about it, but I'm guessing the fix isn't in Javascript-land!

This log is always there because we didn't have power manager from Android. So this is a normal message not a issue.

Comment 18

[Jim Porter (:squib)]

Hmm, strange. Do you have any idea why starting playback on a paused audio file when the app in question is in the background? This is an area I'm not really familiar with.

Comment 19

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

I'm trying to use your repo to look into this.

Comment 20

[Jim Porter (:squib)]

Patryk: I don't know if Rob MacDonald mentioned this to you, but at the Media Team work week, he took a look at my prototype of the lockscreen media controls and noticed that the buttons are a bit too small. I was following your UI mockup and just used the music app's icons for the buttons. I believe Rob's goal is to make the buttons nice and big so that they're easy to hit if you're out running and just want to go to the next song, for instance. Hopefully I'm getting all this right, since Rob is out this week.

Could you take a look at this and update the UI mockups? I think we could either 1) make new, bigger icons, or 2) just make the hit areas larger (like how the camera and unlock buttons are actually bigger than their icons).

Comment 21

[Patryk Adamczyk [:patryk] UX]

Attachment: Bigger Music Controls

Yes Rob and I spoke, I sent him the controls but maybe they got lost in his inbox. Try these.

Comment 22

[Marcia Knous [:marcia]]

https://moztrap.mozilla.org/manage/caseversion/69966/ is in draft form to cover the basic testing of this feature.

Comment 23

[Hema Koka [:hema]]

As discussed, moving this feature to 1.3

Comment 24

[Jim Porter (:squib)]

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

This isn't done yet, but I want to make sure I'm going in the right direction with the system changes. What do you think so far, Alive?

Comment 25

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

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

Jim, this is awesome! I love OO!

BTW I'm worried about the way using static method and static attribute on the class object. Please contact me if you have problems on what I said.

Thanks.

Comment 26

[Jim Porter (:squib)]

Ok, I've tested this out a bunch now (going on walks while listening to music; stress-testing the lockscreen stuff by turning the screen on, putting the phone in my pocket, and dancing around like an idiot), and I think hiding the playback controls until you tap on the "now playing" info is sufficient to prevent most accidental clicks. I actually found it easier to accidentally unlock the phone than to accidentally hit the controls.

I still need to make the UI for hiding the controls a bit prettier, since I was initially just doing a quick-and-dirty solution to test the functionality, but that shouldn't take too long.

Comment 27

[Jim Porter (:squib)]

Quick update: bug 911681 is going to bitrot me moderately badly, although the long-term effect will be significantly simpler code for this stuff, so yay!

Comment 28

[Jim Porter (:squib)]

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

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.


Ok, I think this is done.

Alive: Most of the changes are in the system app, naturally. Now that bug 911681 is landed, we use DOM events to distribute IAC messages throughout the system app. I think this ends up making the code a lot simpler, so hopefully you'll approve.

Rob: Could you take a look at the UI for this? I'll try to post screenshots and send you a short video tonight or this weekend.

EragonJ: I made a couple of minor changes to the iac_handler.js file. Could you take a quick look at them to make sure everything is ok?

Gareth: Could you look over the marionette tests? I'm getting random oranges on the "should hide now playing info by exiting" test, and I'm not sure what's causing it.

Comment 29

[Jim Porter (:squib)]

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

Oops, I didn't mean to request v1.2 approval!

Comment 30

[Gareth Aye [:gaye] (back from PTO)]

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

Hey Jim -- I've been playing around with your patch for a while and haven't been able to reproduce the failure :(. The test looks really good to me. Could you add a bit more info about the orange... perhaps a stacktrace?

On a brighter note, I really like some of the composition/coding style here :). Also, would you mind renaming or moving MediaPlaybackTest? When I see a class named SomeClassName I usually expect to find it in something like some_class_name.js, some-class-name.js, etc.

Comment 31

[Jim Porter (:squib)]

(In reply to Gareth Aye [:gaye] from comment #30)
> Comment on attachment 812870 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/12596
> 
> Hey Jim -- I've been playing around with your patch for a while and haven't
> been able to reproduce the failure :(. The test looks really good to me.
> Could you add a bit more info about the orange... perhaps a stacktrace?

Here's the stack trace: <https://pastebin.mozilla.org/3408934>. It's entirely possible that it's just some weirdness on my end, since I saw these oranges on master too, and Travis doesn't seem to be complaining.

> On a brighter note, I really like some of the composition/coding style here
> :).

Thanks!

> Also, would you mind renaming or moving MediaPlaybackTest? When I see a
> class named SomeClassName I usually expect to find it in something like
> some_class_name.js, some-class-name.js, etc.

Will do.

Comment 32

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

Hi Jim, 

it looks good to me. 

Thanks for helping me changing the related part in FTU. :P

Comment 33

[Jim Porter (:squib)]

Attachment: The patch in action

Comment 34

[Jim Porter (:squib)]

Attachment: The controls when tapping them

Comment 35

[Jim Porter (:squib)]

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

Redirecting ui-review? to Jacqueline. See the above attachments for the patch in action. Thanks in advance!

Comment 36

[Stephany Wilkes]

Jacqueline is working on this today.

Comment 37

[Jacqueline Savory [:jsavory] UX]

Hi Jim, I reviewed the screenshots that you have attached and everything looks great. Thank you!

Comment 38

[Jim Porter (:squib)]

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

Setting ui-review+ per comment 37. Thanks!

Comment 39

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/pull/12596
        https://github.com/mozilla-b2g/gaia/commit/bca3aa71ae7c93f50a99b38911f06462f3219b10

Comment 40

[Marcia Knous [:marcia]]

#10964 added to Moztrap to cover this case.

Comment 41

[Preeti Raghunath(:Preeti)]

1.3+ for uplift.

Comment 42

[Jim Porter (:squib)]

Does this really need uplifting? I thought this was already in 1.3, since I landed it a month ago.

Comment 43

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Yeah, 

you can search the hash : 6aa6430356162bf477a003078793a490f8af2c8a on v1.3