Sri Kasetti Reporter
	Description • 11 years ago

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

	Jim Porter (:squib) Assignee
	Comment 1 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 2 • 11 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 3 • 11 years ago

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

	Jim Porter (:squib) Assignee
	Comment 4 • 11 years ago

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.

	Jim Porter (:squib) Assignee
	Comment 7 • 11 years ago

(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...

	Jim Porter (:squib) Assignee
	Comment 8 • 11 years ago

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.

	Jim Porter (:squib) Assignee
	Comment 9 • 11 years ago

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!

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 10 • 11 years ago

c.c. Yuren for c8

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 11 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 12 • 11 years ago

(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?

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)
	Comment 13 • 11 years ago

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

	Jim Porter (:squib) Assignee
	Comment 14 • 11 years ago

(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.

	Jim Porter (:squib) Assignee
	Comment 15 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 16 • 11 years ago

(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.

	Marco Chen [:mchen]
	Comment 17 • 11 years ago

(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.

	Jim Porter (:squib) Assignee
	Comment 18 • 11 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 19 • 11 years ago

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

	Jim Porter (:squib) Assignee
	Comment 20 • 11 years ago

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).

	Patryk Adamczyk [:patryk] UX
	Comment 21 • 11 years ago

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

	Marcia Knous [:marcia]
	Comment 22 • 11 years ago

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

	Hema Koka [:hema]
	Comment 23 • 11 years ago

As discussed, moving this feature to 1.3

	Jim Porter (:squib) Assignee
	Comment 24 • 11 years ago

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?

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 25 • 11 years ago

Comment on attachment 812870 [details]
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.

	Jim Porter (:squib) Assignee
	Comment 26 • 11 years ago

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.

	Jim Porter (:squib) Assignee
	Comment 27 • 11 years ago

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!

	Jim Porter (:squib) Assignee
	Comment 28 • 11 years ago

Comment on attachment 812870 [details]
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.

	Jim Porter (:squib) Assignee
	Comment 29 • 11 years ago

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

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

	Gareth Aye [:gaye] (back from PTO)
	Comment 30 • 11 years ago

Comment on attachment 812870 [details]
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.

	Jim Porter (:squib) Assignee
	Comment 31 • 11 years ago

(In reply to Gareth Aye [:gaye] from comment #30)
> Comment on attachment 812870 [details]
> 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.

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 32 • 11 years ago

Comment on attachment 812870 [details]
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

	Jim Porter (:squib) Assignee
	Comment 35 • 11 years ago

Comment on attachment 812870 [details]
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!

	Stephany Wilkes
	Comment 36 • 11 years ago

Jacqueline is working on this today.

	Jacqueline Savory [:jsavory] UX
	Comment 37 • 11 years ago

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

	Jim Porter (:squib) Assignee
	Comment 38 • 11 years ago

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

Setting ui-review+ per comment 37. Thanks!

	Jim Porter (:squib) Assignee
	Comment 39 • 11 years ago

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

	Marcia Knous [:marcia]
	Comment 40 • 11 years ago

#10964 added to Moztrap to cover this case.

	Preeti Raghunath(:Preeti)
	Comment 41 • 11 years ago

1.3+ for uplift.

	Jim Porter (:squib) Assignee
	Comment 42 • 11 years ago

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

	EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)
	Comment 43 • 11 years ago

Yeah, 

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