939197 - [Clock] Alarms need volume control

Status: VERIFIED FIXED

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

Description

[Zly]

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.

Comment 1

[Dylan Oliver [:doliver]]

ni? to UX for comment.

Comment 2

[Stephany Wilkes]

Flagging Juwei to see if Clock defaults should be adjusted, and how the default volume currently is set/behaves.

Comment 3

[Juwei Huang[:juwei]]

Attachment: 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.

Comment 4

[Patryk Adamczyk [:patryk] UX]

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

Comment 5

[Marcus Cavanaugh [:mcav]]

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?

Comment 6

[Juwei Huang[:juwei]]

Hi Marcus, I mean {a}Change all existing alarm volumes

Comment 7

[Marcus Cavanaugh [:mcav]]

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?

Comment 8

[Juwei Huang[:juwei]]

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

Comment 9

[Marcus Cavanaugh [:mcav]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16183

Comment 10

[Marcus Cavanaugh [:mcav]]

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.

Comment 11

[Miller Medeiros [:millermedeiros]]

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.

Comment 12

[Marcus Cavanaugh [:mcav]]

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 13

[Juwei Huang[:juwei]]

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.

Comment 14

[Marcus Cavanaugh [:mcav]]

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.

Comment 15

[Marcus Cavanaugh [:mcav]]

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 16

[Juwei Huang[:juwei]]

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!

Comment 17

[Adam Rogers (:arog)]

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.

Comment 18

[Miller Medeiros [:millermedeiros]]

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.

Comment 19

[Dietrich Ayala (:dietrich)]

This patch got r+ over a month ago - any reason it's not landed yet?

Comment 20

[Marcus Cavanaugh [:mcav]]

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.

Comment 21

[Marcus Cavanaugh [:mcav]]

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

Comment 22

[Marcus Cavanaugh [:mcav]]

master: https://github.com/mozilla-b2g/gaia/commit/bb7e44e428640f15e1c07b0ab89c712e797fb9ea

Comment 23

[Michael Henretty [:mikehenrty][:mhenretty]]

Reverted for tbpl bustage:

https://github.com/mozilla-b2g/gaia/commit/e9434aaf9c6f25895a578c9a0057a13eb1a2fa40
https://tbpl.mozilla.org/php/getParsedLog.php?id=37090135&tree=B2g-Inbound

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Michael Henretty [:mhenretty] from comment #23)
> Reverted for tbpl bustage:
> 
> https://github.com/mozilla-b2g/gaia/commit/
> e9434aaf9c6f25895a578c9a0057a13eb1a2fa40
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37090135&tree=B2g-Inbound

And a link to the broken test log: https://tbpl.mozilla.org/php/getParsedLog.php?id=37090135&tree=B2g-Inbound

Comment 25

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17860

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!

Comment 26

[Marcus Cavanaugh [:mcav]]

relanding: https://github.com/mozilla-b2g/gaia/commit/6e6cf2c0e135542da4aacce710cd534794b9627e

Comment 27

[Marcus Cavanaugh [:mcav]]

same failures; will attempt to use Gaia try before relanding tomorrow.

https://github.com/mozilla-b2g/gaia/commit/e9be52f52b1774ac04ac03221559449a85444422

Comment 28

[Marcus Cavanaugh [:mcav]]

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.

Comment 29

[Marcus Cavanaugh [:mcav]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=37106890&tree=B2g-Inbound

Comment 30

[Marcus Cavanaugh [:mcav]]

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

Comment 31

[Edward Chen[:edchen]]

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

Comment 32

[Marcus Cavanaugh [:mcav]]

Thanks for catching this, Edward. I've landed a fix in bug 996739.

Comment 33

[Edward Chen[:edchen]]

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