Closed Bug 875371 Opened 12 years ago Closed 7 years ago

a sound when locking the device

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::System::Lockscreen
Platform:
Other
Gonk (Firefox OS)
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: jovan.gerodetti, Assigned: prateekjadhwani)

Details

Attachments

(2 files, 2 obsolete files)

Pull Request on GitHub
70 bytes, patch
alive
: review-
Details | Diff | Splinter Review
[gaia] prateekjadhwani:875371 > mozilla-b2g:master
46 bytes, text/x-github-pull-request
Details | Review
I miss a sound when locking the screen.
Severity: normal → enhancement
Component: Gaia::System → Gaia::System::Lockscreen
Attachment #754537 - Flags: review?
Attachment #754537 - Flags: review? → review?(alive)
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 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
Attachment #754537 - Flags: review?(alive) → review-
(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.
NI -> UX: Do we need this?
Flags: needinfo?(firefoxos-ux-bugzilla)
Assigning to Casey - quick audio check.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(kyee)
It would be nice to have a sound for locking the device as well. Flagging Patryk for support here.
Flags: needinfo?(kyee)
Can someone assign this bug to me. I dont think I have the edit flag
Assigning Prateek. @Jovan is this okay?
Assignee: nobody → prateekjadhwani
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → Other
(In reply to Joshua Smith [:joshua-s] from comment #9) > Assigning Prateek. @Jovan is this okay? yes, no problem.
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.
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?=alive@mozilla.com
Attached file Pull Request on GitHub (obsolete) —
Attachment #8406420 - Flags: review?(alive)
(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?=alive@mozilla.com Done, thanks for the help Joshua
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.
Attachment #8406420 - Flags: review?(alive) → review+
Attachment #8672402 - Flags: review?(apastor)
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!
Attachment #8672402 - Flags: review?(apastor)
Comment on attachment 8672402 [details] [review] [gaia] prateekjadhwani:Bug-875371 > mozilla-b2g:master Made changes as per suggestions/comments.
Attachment #8672402 - Flags: review?(apastor)
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!
(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
Flags: needinfo?(apastor)
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!
Flags: needinfo?(apastor)
shouldn't we use a different sound for locking? The current unlock sound perfectly fits unlocking, but not really locking the device.
(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
(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
(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.
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.
Alberto, I added the test case for that function. Please let me know if you need any more changes.
Flags: needinfo?(apastor)
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!
Flags: needinfo?(apastor)
Attachment #8672402 - Flags: ui-review?(rmacdonald)
Attachment #8672402 - Flags: review?(apastor)
Attachment #8672402 - Flags: review+
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.
Flags: needinfo?(apastor)
Attachment #8678077 - Flags: review?(apastor)
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?
Flags: needinfo?(apastor) → needinfo?(prateekjadhwani)
Alberto, is robmac the person I need to add as a reviewer for UX? Thanks
Flags: needinfo?(prateekjadhwani) → needinfo?(apastor)
I'm not sure, but he will know how is the right person if he is not :)
Flags: needinfo?(apastor)
Gweng, I was wondering if you could tell me who would be the right person to do a UX-review of this code? Thanks
Flags: needinfo?(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 (amlee@mozilla.com). If you don't mind please set review to me, thanks.
Flags: needinfo?(gweng)
Comment on attachment 8678077 [details] [review] [gaia] prateekjadhwani:875371 > mozilla-b2g:master Setting reviewer to Greg as per the comments.
Attachment #8678077 - Flags: review?(apastor) → review?(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.
Attachment #8678077 - Flags: review?(gweng)
Attachment #8672402 - Attachment is obsolete: true
Attachment #8672402 - Flags: ui-review?(rmacdonald)
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.
Flags: needinfo?(gweng)
Comment on attachment 8406420 [details] [review] Pull Request on GitHub Marking this PR as obsolete since it was closed 2 YRS ago.
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.
Flags: needinfo?(gweng)
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: