857951 - DuT doesn't show "busy line" screen or play "busy line" tone

Status: RESOLVED FIXED

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

Comment 1

[Beatriz Rodríguez [:brg]]

When calling to a busy line, DuT doesn't show "busy line" screen and doesn't play "busy line" tone. It just finish the call and go back to previous screen (call log, contact...). Same occured when rejecting call in the other side.

- Steps to Reproduce:
    
1.- Call to a busy line --> check the screen and tones
2.- Call to a phone number and reject the call --> check the screen and tones
    
- Actual Result:
    
DuT doesn't show "busy line" screen and doesn't play "busy line" tone

- Expected Result (correct behaviour wished):
    
It may show the "busy line" screen and play the corresponding tone 

Buildid "20130321070205", device: ikura

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

DuT stands for 'Device Under Test'.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

It seems that the dialer app doesn't handle the 'onbusy' events. This is not only a bug for Ikura devices but also for all devices.

Comment 4

[Germán Toro del Valle (:gtorodelvalle)]

José Antonio is absolutely right :-O

Ayman, could you please provide me with the concrete text you would like to show the user to notify him that the line is busy? I guess this informative message would be shown to the user using the Confirm BB: http://buildingfirefoxos.com/building-blocks/confirm/ including only one button. Please, provide me also with the text of that button and if it should be a "normal" one or a "danger" (red) one. Thank you very much!

Comment 5

[Daniel Coloma:dcoloma]

Germán, is there any way we can avoid requiring late strings for this solution?

Comment 6

[Germán Toro del Valle (:gtorodelvalle)]

We may just close the attention screen (as it is already done right now) while playing the "busy line" tones for some time (2.5 seconds or so since it seems they are 0.5s long tones (http://www.tech-faq.com/frequencies-of-the-telephone-tones.html)) before actually hanging the call.

Another option may be to show some kind of icon in the outgoing call attention screen instead of the timer shown now when the call is established to somehow notify the user that the other side is busy, as well as playing the "busy line" tones of course, until the user clicks on the hang up button of the outgoing call attention screen to close it and actually hang up the call.

I hope I made myself clear ;-)

Comment 7

[Germán Toro del Valle (:gtorodelvalle)]

Requesting info from Dani regarding comment 6. Just to help you keep track of this ;-) Thanks!

Comment 8

[David Palomino [:dpalomino]]

Changing prio to P1, as this is a certification blocker.

Comment 9

[Andrew Overholt [:overholt]]

Is the busy tone or the icon/text (please no ;) the blocker here?  Or is it both?

Comment 10

[Juan Perez-Bedmar [:juanpbf]]

Adding some colleagues in CC

Comment 11

[David Palomino [:dpalomino]]

Hi, 

We are going to check if it'd be enough implementing just the busy tone. I hope to have an answer today. 

Thanks!
David

Comment 12

[David Palomino [:dpalomino]]

Hi again, 

The busy tone for sure is a blocker. We are trying the carrier to accept not to include the text for the call ending. 

I hope to have some news tomorrow... 

Thanks!
David

Comment 13

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! Don't know if it helps but using Android 4.2.2, no busy line tones are played. The outgoing call screen just shows a "Finalized call" message (hard to see, BTW) and it disappears after a couple of seconds ;-)

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

(In reply to gtorodelvalle from comment #13)
> Hi guys! Don't know if it helps but using Android 4.2.2, no busy line tones
> are played. The outgoing call screen just shows a "Finalized call" message
> (hard to see, BTW) and it disappears after a couple of seconds ;-)

I tested the same on Kiss II device (unagi) running Android ICS and busy tones were played.

Comment 15

[Andrew Overholt [:overholt]]

ni?(dpv) while he gets information from comment 12.

Comment 16

[Daniel Coloma:dcoloma]

We have manage to agree that for certifying 1.0.1 we only need the tone (as we are already string frozen), however we need to open a follow up bug to display the busy message for 1.1

Comment 17

[Andrew Overholt [:overholt]]

Will we have country-specific tones?

http://en.wikipedia.org/wiki/Busy_signal

Comment 18

[Dietrich Ayala (:dietrich)]

German, when do you estimate to have a fix for this?

Comment 19

[Germán Toro del Valle (:gtorodelvalle)]

Hi! I would say today EOB ;-) Basically, due to the testing.

Comment 20

[Germán Toro del Valle (:gtorodelvalle)]

BTW, I am leaning the solution on Etienne's patch for https://bugzilla.mozilla.org/show_bug.cgi?id=859354 where the tone playing library has been refactored. So basically, both bugs will be solved at the same time, or at least that's the intention ;-)

Comment 21

[Etienne Segonzac (:etienne)]

(In reply to gtorodelvalle from comment #20)
> So basically, both bugs will be solved at the
> same time, or at least that's the intention ;-)

Sounds like a good plan :)

Comment 22

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Proposed patch.

This patch is dependent on the patch proposed by Etienne for https://bugzilla.mozilla.org/show_bug.cgi?id=859354 which can be checked here: https://github.com/mozilla-b2g/gaia/pull/9151

This patch MUST NOT be merged before https://github.com/mozilla-b2g/gaia/pull/9151 lands.

Comment 23

[Germán Toro del Valle (:gtorodelvalle)]

Etienne, if it helps you can find the branch I created from your branch bug-859354-call-waiting-alert here: https://github.com/gtorodelvalle/gaia/tree/dialer-bug-857951-busy-tone-from-etiennesegonzac-bug-859354-call-waiting-alert

The commit including this patch is this one: https://github.com/gtorodelvalle/gaia/commit/9da6c4b2317dcffffd1b852cce6482e00dc4536e

Both include obviously the updates made in the patch I just attached.

Comment 24

[Etienne Segonzac (:etienne)]

Comment on attachment 738149 [details] [diff] [review]
Proposed patch.

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

To sum up:
- we need to add this small test, happy to help

And we should be able to cover all the cases with the |busyNotificationLock|
- false when we start
- true when we start notifying
- false once we're done (before the exitCallScreen call)
- false whenever somebody presses end (before we check if we need to close)

::: apps/communications/dialer/js/handled_call.js
@@ +181,5 @@
>  };
>  
> +HandledCall.prototype.busy = function hc_busy() {
> +  OnCallHandler.notifyBusyLine();
> +};

Can you add a quick test to |handled_call_test.js|?
Should be pretty straightforward, ping me if needed.

::: apps/communications/dialer/js/oncall.js
@@ +153,5 @@
>    var closing = false;
>    var animating = false;
>    var ringing = false;
> +  var notifying = false;
> +  var ignoreNotifying = false;

|var busyNotificationLock = false|;

@@ +447,4 @@
>    }
>  
>    function exitCallScreen(animate) {
> +    if (closing || (notifying && !ignoreNotifying)) {

if (closing || busyNotificationLock)

@@ +612,1 @@
>  

-- in the end() function we release the lock
|busyNotificationLock = false;|

@@ +612,4 @@
>  
>      // If not we're rejecting the last incoming call
>      if (!handledCalls.length) {
> +      ignoreNotifying = true;

not needed anymore (I think)

@@ +617,5 @@
>        return;
>      }
>  
>      var lastCallIndex = handledCalls.length - 1;
> +    ignoreNotifying = true;

not needed either

@@ +647,4 @@
>      postToMainWindow(message);
>    }
>  
> +  function notifyBusyLine() {

|busyNotificiationLock = true|

::: apps/communications/dialer/oncall.html
@@ +11,4 @@
>      <!-- Localization -->
>      <script defer type="text/javascript" src="/shared/js/lazy_loader.js"></script>
>      <script defer type="application/javascript" src="/dialer/js/lazy_l10n.js"></script>
> +    <link rel="resource" type="audio/ogg" href="/shared/resources/media/notifications/notifier_bap.opus"/>

probably a merge issue, I don't think we need this anymore.

Comment 25

[Germán Toro del Valle (:gtorodelvalle)]

Very nice comments as always, Etienne ;-)

You can find the updated local commit at https://github.com/gtorodelvalle/gaia/commit/cd640703da956b54491cb4e21fa563cd30c19c66

Comment 26

[Germán Toro del Valle (:gtorodelvalle)]

Ups! Give me a sec :-O Solving Lint issues! :-)

Comment 27

[Germán Toro del Valle (:gtorodelvalle)]

The right local commit (Lint issues solved ;-)): https://github.com/gtorodelvalle/gaia/commit/75e39551b4a2b02971bad809ca4127eefa7a5d28

Comment 28

[Etienne Segonzac (:etienne)]

(In reply to gtorodelvalle from comment #27)
> The right local commit (Lint issues solved ;-)):
> https://github.com/gtorodelvalle/gaia/commit/
> 75e39551b4a2b02971bad809ca4127eefa7a5d28

Looking good and all tests green!

Comment 29

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Associated PR.

Comment 30

[Etienne Segonzac (:etienne)]

Comment on attachment 738617 [details]
Associated PR.

\o/

Comment 31

[Germán Toro del Valle (:gtorodelvalle)]

Commit in master: https://github.com/mozilla-b2g/gaia/commit/060ceed93b781cf43a4454ffccabf48ef06c681e

Comment 32

[James Lal [:lightsofapollo]]

Uplifted 060ceed93b781cf43a4454ffccabf48ef06c681e to:
v1-train: 31875d104c1c90e06db21e0596c620171ef08f44
v1.0.1: 89d509e1c196022b426a51d1d44ba11b3fd1751d

Comment 33

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! This patch is highly dependent on the one for bug 859354 which it seems has not already been landed either in v1.0.1 or v1-train and consequently this patch will not work in both branches :-(

Comment 34

[Juan Perez-Bedmar [:juanpbf]]

I have tested this issue in the following version
  build ID:20130426165700
  build information:
  gaia commit:
  e25b349 Merge pull request #9273 from gasolin/issue-863126
  gecko commit:
  6dfd179 Bug 849757 - Part 4: xpcshell tests. r=vicamo, a=tef+
  AU:V1.01.00.01.019.085
and it is still failing. 
Since the patch should be already uplifted to the v1.0.1 branch, I reopen the bug

Comment 35

[Alex Keybl [:akeybl]]

qawanted - is this fixed on any branch in that case?

Hopefully we can get this fixed ASAP.

Comment 36

[Jason Smith [:jsmith]]

Not to be entire stickler on process, but can we please open a new bug and mark it as a dependency on this bug? I'd rather not reopen bugs with patches that have already landed.

Feel free to put qawanted again though on the followup bug.

Comment 37

[Juan Perez-Bedmar [:juanpbf]]

(In reply to Jason Smith [:jsmith] from comment #36)
> Not to be entire stickler on process, but can we please open a new bug and
> mark it as a dependency on this bug? I'd rather not reopen bugs with patches
> that have already landed.
> 
> Feel free to put qawanted again though on the followup bug.

No problem at all. Sorry about that. I'll open a new bug

Comment 38

[Germán Toro del Valle (:gtorodelvalle)]

And please include the proper dependencies amongst bugs for easier tracking ;-) Thanks!