Closed Bug 939197 Opened 8 years ago Closed 7 years ago

[Clock] Alarms need volume control

Categories

(Firefox OS Graveyard :: Gaia::Clock, defect)

defect
Not set
normal

Tracking

(tracking-b2g:backlog)

VERIFIED FIXED
1.4 S5 (11apr)
tracking-b2g backlog

People

(Reporter: zly_wilk, Assigned: mcav)

References

Details

(Whiteboard: [priority][p=8])

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131025151332

Steps to reproduce:

I set up an alarm to wake me up.


Actual results:

Alarm was activated, but available sounds vere too silent.


Expected results:

Default alarm sounds should be louder. For example old phone ring sound should be able to set as alarm sound.
ni? to UX for comment.
Flags: needinfo?(swilkes)
Flagging Juwei to see if Clock defaults should be adjusted, and how the default volume currently is set/behaves.
Flags: needinfo?(swilkes) → needinfo?(jhuang)
Attached image Clock_new alarm.png
There are two possible problems of default alarm:
1. Each alarm sound is not as same volume as each other. Ex. Classic Buzz is quite loud, yet Into the Void is low. 
Patryk, could you help to advice in here?

2. It's hard to find where to setup default alarm, which is under Settings. My suggestion would be: add another volume control when create new alarm.
Here's the flow:(Please refer to attachment)
In clock app -> Tap on alarm tab ->  Tap on Add alarm -> Adjust the volume control to setup alarm sound. --> Tap on done
Please note that every time when users adjust volume control, the value will automatically set by default, which means when users change one alarm's volume, other alarms will change as well.
Flags: needinfo?(jhuang) → needinfo?(padamczyk)
blocking-b2g: --- → backlog
Whiteboard: [priority][p= ]
> There are two possible problems of default alarm:
> 1. Each alarm sound is not as same volume as each other. Ex. Classic Buzz is
> quite loud, yet Into the Void is low. 
> Patryk, could you help to advice in here?

Yes that is true, the sounds were initially created fall of 2012 when we had no volume controls on device, so we modified the volume on the sound file. Once we add the alarm volume controls we can normalize the volume of the sounds... please make a bug against me for that.
Flags: needinfo?(padamczyk)
Juwei, can you clarify this part:

> Please note that every time when users adjust volume control, the
> value will automatically set by default, which means when users
> change one alarm's volume, other alarms will change as well.

Do you mean (a) that the alarm volume changes all existing alarm
volumes (that every alarm will always match whatever that slider is),
(b) that it only stores the default for when you create new alarms, or
(c) something else?
Flags: needinfo?(jhuang)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hi Marcus, I mean {a}Change all existing alarm volumes
Flags: needinfo?(jhuang)
This seems potentially confusing to me, having a volume slider on the "New Alarm" page. If I saw that option on that page, I would think that it would only change the volume for the current alarm... does that seem confusing to you?
Flags: needinfo?(jhuang)
Yes I was thinking this situation, too. 
However, consider about an actual user scenario, I think there is little chance that users really want to set different volume for each alarm. If users change the volume at the moment they set up new alarm, I suppose they are consider the volume is too silent or too loud for them. So I'm okay with change all existing alarm values when adjusting one alarm volume.
And I also think about another solution is to have a link to access volume settings in order to remind users that they are adjusting the default volume. Yet it somehow too empty for the setting page since there's only one volume control can be shown on screen, which is bad, so I abandon it.
Is it okay with you? Let me know :)
Flags: needinfo?(jhuang)
Assignee: nobody → m
Whiteboard: [priority][p= ] → [priority][p=5]
Target Milestone: --- → 1.4 S1 (14feb)
Summary: default alarm sounds are very silent → [Clock] Alarms need volume control
Comment on attachment 8374318 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16183

This patch adds a slider volume control to the "Edit Alarm" screen.
The slider behaves in the following ways:

- The default volume is 1.0 (of a range of 0 to 1).

- When you move the volume slider and then SAVE the alarm, the volume
  for _all_ alarms gets changed to the value specified by the slider.

- When you PREVIEW a sound (by changing the selection in the dropdown
  when editing an alarm), the previewed sound plays at the volume the
  slider is currently set at, even if you haven't saved.

- If you "cancel" editing an alarm, your changes are discarded,
  including any changes to the volume control.

I'm flagging Adam because this bug has effectively become a feature,
and I don't want to land it until we know that this is actually
something we wanted; flagging Juwei for UX review, upon which these
changes are based; flagging Miller for code review because Mike
already has several in his queue and he also has a full-time job.
Attachment #8374318 - Flags: ui-review?(jhuang)
Attachment #8374318 - Flags: review?(mmedeiros)
Attachment #8374318 - Flags: feedback?(arogers)
Comment on attachment 8374318 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16183

even if you guys decide not to implement this feature, I think we should merge the `audio_manager.js` file, and changes related to that. It is a good abstraction, avoided some code duplication, and simplified things a little bit.

I also found it weird that volume affects all the alarms. (I thought it was a bug..) I think a better label could help to reduce the confusion; maybe something like "Global Alarm Volume". But I would still find it confusing, specially since some sounds are very loud on the maximum volume while others are quiet if volume is below 0.5... - It made me think if volume should be tied to the "Sound" instead of simply being a global setting (which I don't think is a good idea, since it will make things even more complex/confusing).

what makes it really confusing is that all the other settings are specific to a single alarm instance, and the volume is "global"...

it is also not clear to me if these changes will reflect on the system settings or not - for me it looks like the "Settings > Sound" slider isn't synched with the clock app changes.
Attachment #8374318 - Flags: review?(mmedeiros)
Thanks for finding those glitches in the patch, Miller. Agreed with all your Github comments; I'll wait to post another review until we get feedback from Adam and Juwei. I agree that it seems potentially confusing, but I can't think of a better way to handle that while still keeping the volume slider in the "Edit Alarm" panel.
Comment on attachment 8374318 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16183

Thank you Marcus, the patch is excellent!! except for 2 minor items:
1. Default volume value should be in medium, which means the dot should be in the middle of the control bar by default.
2. The volume value should align to the setting's alarm sound. As Miller said, the changes need to reflect on system settings. I'm not quite sure if it's doable by just our side or we should cooperate with system side? 

Miller, I am fully understand your concern. However, as I said on comment 8, I suppose users won't expect to set different volumes for each alarm. I believe this design is a better solution for users to easily change volume when set up alarm. And of course we need to fix the basic sound volume first as I said in comment 3 list 1.

And Adam, it would be great if you could help me to check this bug and have some feedback to see if it's good to implement to current build.
Attachment #8374318 - Flags: ui-review?(jhuang) → ui-review-
Comment on attachment 8374318 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16183

Here are some changes based on your feedback:

- I renamed "Alarm Volume" to "System Alarm Volume" to see if it makes things clearer. Do you think that's better?

- The system Settings alarm volume is now linked (two-way) with the volume slider in Clock. Changing one changes the other; they will always match.

- If it's clear to the user that the "System Alarm Volume" slider is for the system, my instinct was to assume that the user expects the system volume to change immediately when that slider is changed, even if they hit "X" to close the edit window. In other words, since it's not an alarm-specific setting, it seems like it makes sense to just save its changes always and immediately. That's what this patch does now, but of course open to your feedback.

- Fixed some code things from the previous review.
Attachment #8374318 - Flags: ui-review?(jhuang)
Attachment #8374318 - Flags: ui-review-
Attachment #8374318 - Flags: review?(mmedeiros)
Oh, and there's one more thing about the default volume -- Juwei, you mentioned that it should be set to halfway by default. However, the System Settings app has its own default of all the way up. If we want the default to be halfway, that would need to be updated in the Settings app. Do you know who the UX person is for that app, or if we need to consult them before changing that value?
Comment on attachment 8374318 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16183

Looks great !! Thank you Marcus!

- Renamed the title to "System Alarm Volume" seems better! let's keep it. 
- System alarm volume is linked now: checked!
- Save the volume value immediately even when hit "X": same behaviour as building blocks, checked!
- Default volume: sorry I thought default was medium, it should be in maximum value. Your patch is right, so checked!
Attachment #8374318 - Flags: ui-review?(jhuang) → ui-review+
Comment on attachment 8374318 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16183


Sounds like you have it well in hand.   I'm ok with this feature, let's land it in 1.4.
Attachment #8374318 - Flags: feedback?(arogers) → feedback+
Comment on attachment 8374318 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16183

patch code is looking REALLY good! nice job.
Attachment #8374318 - Flags: review?(mmedeiros) → review+
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Target Milestone: 1.4 S2 (28feb) → ---
This patch got r+ over a month ago - any reason it's not landed yet?
Yeah, it didn't make the 1.4 train, so we had to hold off until 1.4 branched last week; it took a week or so for me to clear out some other 1.3T blockers before I could get back to this.
(And by didn't make the 1.4 train, I mean it was considered a feature, so we held off.) It should be landed shortly.
master: https://github.com/mozilla-b2g/gaia/commit/bb7e44e428640f15e1c07b0ab89c712e797fb9ea
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [priority][p=5] → [priority][p=8]
Target Milestone: --- → 1.4 S5 (11apr)
I did a diff between this patch and the last one and I agree with the changes. (only a couple lines). Travis is failing right now because of github issues. Make sure test passes before landing! good work!
Attachment #8374318 - Attachment is obsolete: true
Attachment #8400247 - Flags: review+
relanding: https://github.com/mozilla-b2g/gaia/commit/6e6cf2c0e135542da4aacce710cd534794b9627e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
same failures; will attempt to use Gaia try before relanding tomorrow.

https://github.com/mozilla-b2g/gaia/commit/e9be52f52b1774ac04ac03221559449a85444422
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
For the context and amusement of those watching:

- The first PR failed TBPL because of an erroneous Clock unit test monkeypatch of navigator.mozSettings. The failures seen in TBPL here were found by running the unit tests on b2g-desktop locally.

- The second PR failed TBPL, even though the unit tests pass _do_ pass running b2g-desktop locally.

So there's something wonky differing between b2g-desktop on my machine and TBPL. :mhenretty suggested running using the gaia try server, which seems like a possible source of help.
After much confusion, it is now clear that navigator.mozSettings does not, in fact, exist at all in unit tests. This third, and likely final, landing, guards the existence of mozSettings properly. It was not clear that this was the case due to differences in the order that the TBPL harness runs things in; will likely file a bug later for that.

master: https://github.com/mozilla-b2g/gaia/commit/039037c16948c0985f6779886fac595b111979f4
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
[Environment]
Gaia      553da99ab09b6b894d9f95bb06b16b6e1ddbf0a1
Gecko     https://hg.mozilla.org/mozilla-central/rev/5b6e82e7bbbf
BuildID   20140414160205
Version   31.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013


[Checked criteria ( comment 16 ) and test result]
- Renamed the title to "System Alarm Volume"  (PASS)
- System alarm volume is linked now (PASS)
- Save the volume value immediately even when hit "X". (Fail)
- Default volume should be in maximum value. (PASS)

Please double confirm the volume must be saved immediately or not.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 996739
Thanks for catching this, Edward. I've landed a fix in bug 996739.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
[Environment]
Gaia      37d029e584d79aeaca8d30474c394eddcdfade03
Gecko     https://hg.mozilla.org/mozilla-central/rev/b735e618c2a8
BuildID   20140416160202
Version   31.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013

I VERIFIED the volume value immediately even when hit "X", its PASS.
Status: RESOLVED → VERIFIED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.