875371 - a sound when locking the device

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System::Lockscreen, enhancement)

Description

[Jovan Gerodetti]

I miss a sound when locking the screen.

Comment 1

[Jovan Gerodetti]

Attachment: Pull Request on GitHub

Comment 2

[Jovan Gerodetti]

Comment on attachment 754537 [details] [diff] [review]
Pull Request on GitHub

<script>
window.location.replace('https://github.com/mozilla-b2g/gaia/pull/9969');
</script>

Comment 3

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

Comment on attachment 754537 [details] [diff] [review]
Pull Request on GitHub

There's already a sound. see lockscreen.js
You should enable it from settings/sound panel

Comment 4

[Jovan Gerodetti]

(In reply to Alive Kuo [:alive] from comment #3)
> Comment on attachment 754537 [details] [diff] [review]
> Pull Request on GitHub
> 
> There's already a sound. see lockscreen.js
> You should enable it from settings/sound panel

The sound in lockscreen.js is for unlocking the device, but I added a lock sound for locking and switching off the device / display.

Comment 5

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

NI -> UX:
Do we need this?

Comment 6

[Stephany Wilkes]

Assigning to Casey - quick audio check.

Comment 7

[Casey Yee]

It would be nice to have a sound for locking the device as well.

Flagging Patryk for support here.

Comment 8

[Prateek Jadhwani [:prateekjadhwani]]

Can someone assign this bug to me. I dont think I have the edit flag

Comment 9

[Joshua Smith [:joshua-s]]

Assigning Prateek.  @Jovan is this okay?

Comment 10

[Jovan Gerodetti]

(In reply to Joshua Smith [:joshua-s] from comment #9)
> Assigning Prateek.  @Jovan is this okay?

yes, no problem.

Comment 11

[Prateek Jadhwani [:prateekjadhwani]]

Just created a pull request to fix this issue.
here is the link for that

https://github.com/mozilla-b2g/gaia/pull/18263

I still dont know what would be the correct procedure to notify people about the pull requests. Let me know if this is not the correct way to do it.

Comment 12

[Joshua Smith [:joshua-s]]

Add a text attachment with the content "https://github.com/mozilla-b2g/gaia/pull/18263", and then set a review flag on it.  Probably r?=[e-mail]

Comment 13

[Prateek Jadhwani [:prateekjadhwani]]

Attachment: Pull Request on GitHub

Comment 14

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Joshua Smith [:joshua-s] from comment #12)
> Add a text attachment with the content
> "https://github.com/mozilla-b2g/gaia/pull/18263", and then set a review flag
> on it.  Probably r?=[e-mail]

Done, thanks for the help Joshua

Comment 15

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

Comment on attachment 8406420 [details] [review]
Pull Request on GitHub

If UX wants this...

BTW the timing when we do 'lock' is when screen turns off.

Comment 16

[GitHub Autolander Bot]

Attachment: [gaia] prateekjadhwani:Bug-875371 > mozilla-b2g:master

Comment 17

[Alberto Pastor [:albertopq]]

Comment on attachment 8672402 [details] [review]
[gaia] prateekjadhwani:Bug-875371 > mozilla-b2g:master

Left a comment in GH and we need unit tests for this. Please, reflag me again when that's addressed. Thanks!

Comment 18

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8672402 [details] [review]
[gaia] prateekjadhwani:Bug-875371 > mozilla-b2g:master

Made changes as per suggestions/comments.

Comment 19

[Alberto Pastor [:albertopq]]

Thanks for the changes! It looks like we have 3 repeated lines now. It would be great to extract it in a method, but I won't block the review on it. What I do want to see is some unit tests for this before landing. Thanks!

Comment 20

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Alberto Pastor [:albertopq] from comment #19)
> Thanks for the changes! It looks like we have 3 repeated lines now. It would
> be great to extract it in a method, but I won't block the review on it. What
> I do want to see is some unit tests for this before landing. Thanks!

Thanks Alberto, I made the changes as per the comments. Just a quick question. What is the unit test supposed to check here? Thanks

Comment 21

[Alberto Pastor [:albertopq]]

I think you should check that it calls unlockAudio.play() when executing the method 'overlayLocked', and 'this.unlockSoundEnabled' === true. Basically, check that this new line [1] gets called correctly

https://github.com/mozilla-b2g/gaia/pull/32385/files#diff-aa38453bb76aabcfec055ec11d292da6R571

Thanks!

Comment 22

[Jovan Gerodetti]

shouldn't we use a different sound for locking? 
The current unlock sound perfectly fits unlocking, but not really locking the device.

Comment 23

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Alberto Pastor [:albertopq] from comment #21)
> I think you should check that it calls unlockAudio.play() when executing the
> method 'overlayLocked', and 'this.unlockSoundEnabled' === true. Basically,
> check that this new line [1] gets called correctly
> 
> https://github.com/mozilla-b2g/gaia/pull/32385/files#diff-
> aa38453bb76aabcfec055ec11d292da6R571
> 
> Thanks!

Sure, let me implement that in test cases. Thank You

Comment 24

[Prateek Jadhwani [:prateekjadhwani]]

(In reply to Jovan Gerodetti from comment #22)
> shouldn't we use a different sound for locking? 
> The current unlock sound perfectly fits unlocking, but not really locking
> the device.

Hi Jovan, I personally like the lock and unlock sound being the same, but if that is a requirement, can we have another bug for that. Or if you have any lock sounds, would it be possible you add it as an attachment, so that I can change it?

Thanks

Comment 25

[Jovan Gerodetti]

(In reply to Prateek Jadhwani [:prateekjadhwani] from comment #24)
> (In reply to Jovan Gerodetti from comment #22)
> > shouldn't we use a different sound for locking? 
> > The current unlock sound perfectly fits unlocking, but not really locking
> > the device.
> 
> Hi Jovan, I personally like the lock and unlock sound being the same, but if
> that is a requirement, can we have another bug for that. Or if you have any
> lock sounds, would it be possible you add it as an attachment, so that I can
> change it?
> 
> Thanks

Unfortunately I don't have one.
I just thought you might want to implement it in a way, that an additional sound can be added later... just a suggestion. Like create a copy of the unlock sound file and if someone comes around with a good sound, we can just replace it.

Comment 26

[Prateek Jadhwani [:prateekjadhwani]]

Sorry for the delay Alberto, I have been trying to understand how Marionette and trying to fix npm issues with it in my workspace. Just wanted to keep you posted incase you were wondering. 

I will work on Jovan's suggestion after that.

Comment 27

[Prateek Jadhwani [:prateekjadhwani]]

Alberto, I added the test case for that function. Please let me know if you need any more changes.

Comment 28

[Alberto Pastor [:albertopq]]

Comment on attachment 8672402 [details] [review]
[gaia] prateekjadhwani:Bug-875371 > mozilla-b2g:master

One more thing, let's squash all the commits in just 1, with the commit comment starting with 'Bug 875371 - ...'.
Apart from that, the code looks good to me, but let's make sure that UX is aware of this change.

Rob, this patch is adding a sound when locking the phone (the same one that when unlocking). Does that sound good? Should we change the setting for saying Lock/Unlock?
Thanks!

Comment 29

[GitHub Autolander Bot]

Attachment: [gaia] prateekjadhwani:875371 > mozilla-b2g:master

Comment 30

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8678077 [details] [review]
[gaia] prateekjadhwani:875371 > mozilla-b2g:master

Alberto, I had to re-open the PR because of the merge and squash problems.

Comment 31

[Alberto Pastor [:albertopq]]

The code looks good, but we can't land this without UX approval. Could you please try to confirm with UX if this is ok?

Comment 32

[Prateek Jadhwani [:prateekjadhwani]]

Alberto, is robmac the person I need to add as a reviewer for UX?

Thanks

Comment 33

[Alberto Pastor [:albertopq]]

I'm not sure, but he will know how is the right person if he is not :)

Comment 34

[Prateek Jadhwani [:prateekjadhwani]]

Gweng, I was wondering if you could tell me who would be the right person to do a UX-review of this code?
Thanks

Comment 35

[Greg Weng [:snowmantw][:gweng][:λ]]

First I've some concerns about the code and I haven't been notified about the patch and the review; second for your question you can contact Amy ([e-mail]).

If you don't mind please set review to me, thanks.

Comment 36

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8678077 [details] [review]
[gaia] prateekjadhwani:875371 > mozilla-b2g:master

Setting reviewer to Greg as per the comments.

Comment 37

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8678077 [details] [review]
[gaia] prateekjadhwani:875371 > mozilla-b2g:master

Thanks for set it. I commented one issue on the PR page. You can discuss that with me on the PR page or bug comments, if you feel there are some other concerns. And feel free to set review again after that, I think it's good to be r+ except that.

Comment 38

[Prateek Jadhwani [:prateekjadhwani]]

Hi Greg, Currently, There is no `details` object passed when the user clicks the Lock Button. Should I try to make it work in a similar way that `lockscreen_state_unlock.js` does it?

Or should I simply rename the function and get done with it.

I also added this question in the PR, but it seems you did not get any notifications. Do i need to tag you there so that you can get the notification?

Thanks.

Comment 39

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8406420 [details] [review]
Pull Request on GitHub

Marking this PR as obsolete since it was closed 2 YRS ago.

Comment 40

[Prateek Jadhwani [:prateekjadhwani]]

Comment on attachment 8406420 [details] [review]
Pull Request on GitHub

>https://github.com/mozilla-b2g/gaia/pull/18263

Comment 41

[Greg Weng [:snowmantw][:gweng][:λ]]

See my comment on the PR, thanks.

And GitHub notification is an issue since they are too many. I think to NI or feedback (after your batch fixing) or review (if you're ready to land it) is better.