891746 - [Gaia] Let user select DTMF tone type in CDMA network.

Status: RESOLVED FIXED

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

Description

[Ken Chang[:kenkai]]

User story said that user should be able to select DTMF tone type (normal vs long), which means that Gecko should provide this kind of interface and Gaia should also provide a interface for user selecting DTMF tone type.

Comment 1

[Kai-Chih Hu [:khu]]

Gecko part is ready. So, Gaia app should be able to control it now.

Comment 2

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=869772#c3 Thank you.

Comment 3

[Ken Chang[:kenkai]]

According to the comment 7 of bug 869772, we should be able to close this bug.

Comment 4

[Ken Chang[:kenkai]]

We still need to have an UI for this bug. Reopen this bug.

Comment 5

[Gabriele Svelto [:gsvelto]]

The idea here is to implement tones as described in bug 869772 comment 3: the user can select between two modes, a long mode in which the tone is played as long as a dialer key is pressed and a short mode in which the tone is played for only 120ms irrespective of how long the key is pressed. The only thing I'm missing here is where to put the option for choosing between the two. Would the "Call Settings" panel in the settings app do? I assume that the option should be visible only when a CDMA network is detected and hidden otherwise (and its value ignored by the dialer).

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Gabriele, the setting name is "phone.dtmf.type". In the end I use explicit strings as the value as you suggested. The values are `short` and `long`. The patch that adds the setting is ready for review. Let me know if you encounter any problem, thanks!

Comment 7

[Gabriele Svelto [:gsvelto]]

(In reply to Arthur Chen [:arthurcc] from comment #6)
> Gabriele, the setting name is "phone.dtmf.type". In the end I use explicit
> strings as the value as you suggested. The values are `short` and `long`.
> The patch that adds the setting is ready for review. Let me know if you
> encounter any problem, thanks!

Great, thanks!

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Support short DTMF tones

This is a tentative patch to support short dialtones when the relevant preference is enabled. The approach I've taken is to adjust all timers so that they stop playing the dialtone after 120ms instead of the specified length.

Unfortunately I'm not sure how to test this and verify that it's working properly :-( Writing unit-tests for this seems similarly challenging considering we don't seem to have the DTMF functionality covered already.

Comment 9

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Support short DTMF tones

Pointer to Github pull-request

Comment 10

[Etienne Segonzac (:etienne)]

Comment on attachment 803040 [details] [diff] [review]
[PATCH WIP] Support short DTMF tones

Review of attachment 803040 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure about the UX here.
Should we override the duration of the key press altogether in CDMA mode and just do the standardized short or long duration? (we might want to store the standard durations directly in the settings)
Should we honor the key press duration if it's higher than a certain threshold and to the standard otherwise?
Should we just use the standardized long or short duration as a minimum duration? as a maximum?

::: apps/communications/dialer/js/keypad.js
@@ +178,5 @@
> +
> +      settings.addObserver(this._DTMF_TONE_LENGTH_KEY, (function(v) {
> +        this._dtmfShortTone = v.settingValue == 'short';
> +      }).bind(this));
> +    }

you're going to love the SettingsListener helper :)

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/settings_listener.js

Comment 11

[Etienne Segonzac (:etienne)]

Also the final patch here will probably need the settings UI too.

Comment 12

[Etienne Segonzac (:etienne)]

See Comment 10.

Comment 13

[Ken Chang[:kenkai]]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> Comment on attachment 803040 [details] [diff] [review]
> [PATCH WIP] Support short DTMF tones
> 
> Review of attachment 803040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure about the UX here.
Here is the UX.
If user selects short DTMF, that means that no matter how long user presses the key, there is only a DTMF with 120ms.
If user selects long DTMF, that means the DTMF will keep generating until user releases the key.

Comment 14

[Gabriele Svelto [:gsvelto]]

(In reply to Etienne Segonzac (:etienne) from comment #11)
> Also the final patch here will probably need the settings UI too.

Settings are being handled in bug 914152.

(In reply to Ken Chang from comment #13)
> If user selects short DTMF, that means that no matter how long user presses
> the key, there is only a DTMF with 120ms.
> If user selects long DTMF, that means the DTMF will keep generating until
> user releases the key.

OK, my changes should do then. What I did is to leave the current behavior unchanged in long mode and switch to 120ms bursts in short mode for all tones. Updated patch coming soon.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Support short DTMF tones

This uses the SettingsListener and blends in better with the existing code. I still don't know how to test this though, Ken do you know if somebody in Taiwan could check if this is working properly?

BTW I've changed only the actual DTMF tones, not the audible ones. Ken should the audible tones be changed too?

Comment 16

[Ken Chang[:kenkai]]

(In reply to Gabriele Svelto [:gsvelto] from comment #15)
> Created attachment 803578 [details] [diff] [review]
> [PATCH] Support short DTMF tones
> 
> This uses the SettingsListener and blends in better with the existing code.
> I still don't know how to test this though, Ken do you know if somebody in
> Taiwan could check if this is working properly?
Oops! I don't think carrier will help us to check the duration of the DTMF.
I suggest verifying it in GSM emulator.
> 
> BTW I've changed only the actual DTMF tones, not the audible ones. Ken
> should the audible tones be changed too?
Yes.....

Comment 17

[Ken Chang[:kenkai]]

(In reply to Ken Chang from comment #16)
> (In reply to Gabriele Svelto [:gsvelto] from comment #15)
> > Created attachment 803578 [details] [diff] [review]
> > [PATCH] Support short DTMF tones
> > 
> > This uses the SettingsListener and blends in better with the existing code.
> > I still don't know how to test this though, Ken do you know if somebody in
> > Taiwan could check if this is working properly?
> Oops! I don't think carrier will help us to check the duration of the DTMF.
> I suggest verifying it in GSM emulator.
I don't think we have a good method for integration verification now.
We will verify this when we have CDMA partner. So just make sure your 
app works as you want for now.

Comment 18

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Support short DTMF tones

This is an updated patch and I can confirm it works correctly as I've managed to test it.

What I did was enable unconditionally the tones and added timers around them to measure for how long they would play. In long mode the tones play as before and in short mode I can measure ~120ms bursts so everything should be working alright.

I think a similar approach might be used to cover this functionality with unit-tests; not by measuring the time of the various events but by intercepting the values passed in the stop-timeouts and comparing them with what we excpect.

Comment 19

[Etienne Segonzac (:etienne)]

So don't we need some changes in the settings app too to fix this bug?

Comment 20

[Gabriele Svelto [:gsvelto]]

(In reply to Etienne Segonzac (:etienne) from comment #19)
> So don't we need some changes in the settings app too to fix this bug?

Yup, they already landed in bug 914152.

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 803742 [details] [diff] [review]
[PATCH v2] Support short DTMF tones

Review of attachment 803742 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Not pushing for unit tests since we know this is going to be re-written with WebAudio in the near future.

Comment 22

[Gabriele Svelto [:gsvelto]]

Thanks for the review! Merged to master:

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