a sound when locking the device

RESOLVED WONTFIX

Status

Firefox OS
Gaia::System::Lockscreen
--
enhancement
RESOLVED WONTFIX
5 years ago
9 months ago

People

(Reporter: Jovan Gerodetti, Assigned: prateekjadhwani)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
I miss a sound when locking the screen.
(Reporter)

Updated

5 years ago
Severity: normal → enhancement

Updated

5 years ago
Component: Gaia::System → Gaia::System::Lockscreen
(Reporter)

Comment 1

5 years ago
Created attachment 754537 [details] [diff] [review]
Pull Request on GitHub
Attachment #754537 - Flags: review?
(Reporter)

Updated

5 years ago
Attachment #754537 - Flags: review? → review?(alive)
(Reporter)

Comment 2

5 years ago
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-
(Reporter)

Comment 4

5 years ago
(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)

Comment 6

5 years ago
Assigning to Casey - quick audio check.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(kyee)

Comment 7

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

Flagging Patryk for support here.
Flags: needinfo?(kyee)
(Assignee)

Comment 8

4 years ago
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
(Reporter)

Comment 10

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

yes, no problem.
(Assignee)

Comment 11

4 years ago
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
(Assignee)

Comment 13

4 years ago
Created attachment 8406420 [details] [review]
Pull Request on GitHub
Attachment #8406420 - Flags: review?(alive)
(Assignee)

Comment 14

4 years ago
(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+
Created attachment 8672402 [details] [review]
[gaia] prateekjadhwani:Bug-875371 > mozilla-b2g:master
(Assignee)

Updated

3 years ago
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)
(Assignee)

Comment 18

3 years ago
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!
(Assignee)

Comment 20

3 years ago
(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)
(Reporter)

Comment 22

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

Comment 23

3 years ago
(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
(Assignee)

Comment 24

3 years ago
(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
(Reporter)

Comment 25

3 years ago
(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.
(Assignee)

Comment 26

3 years ago
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.
(Assignee)

Comment 27

3 years ago
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+
Created attachment 8678077 [details] [review]
[gaia] prateekjadhwani:875371 > mozilla-b2g:master
(Assignee)

Comment 30

3 years ago
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)
(Assignee)

Comment 32

3 years ago
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)
(Assignee)

Comment 34

3 years ago
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)
(Assignee)

Comment 36

3 years ago
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)
(Assignee)

Updated

3 years ago
Attachment #8672402 - Attachment is obsolete: true
Attachment #8672402 - Flags: ui-review?(rmacdonald)
(Assignee)

Comment 38

3 years ago
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)
(Assignee)

Comment 39

3 years ago
Comment on attachment 8406420 [details] [review]
Pull Request on GitHub

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

Comment 40

3 years ago
Comment on attachment 8406420 [details] [review]
Pull Request on GitHub

>https://github.com/mozilla-b2g/gaia/pull/18263
Attachment #8406420 - Attachment is obsolete: true
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)
(Reporter)

Updated

9 months ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.