1003846 - [Dialer][Call Screen] Show the call duration when it ends

Status: VERIFIED FIXED

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

Description

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

This bug aims to solve the functional gap currently existent in master regarding showing the call duration of the calls once they end. Since bug 899926 provides specs regarding how the UI will look like once the visual refresh (being implemented in bug 951606) lands, we need to previously implement the call duration functionality using the current look and feel available in master (prior to the visual refresh). Once this call duration functionality is available, the visual refresh will be applied getting the final appearance specified in bug 899926.

Comment 2

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

Attachment: callscreen_number_call_ended.png

Comment 3

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

Attachment: callscreen_contact_call_ended.png

Comment 4

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

Attachment: callscreen_callwaiting_first_call_ended.png

Comment 5

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

Attachment: callscreen_callwaiting_second_call_ended.png

Comment 6

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

Attachment: callscreen_conference_call_ended.png

Comment 7

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

Attachment: callscreen_conference_call_participant_call_ended.png

Comment 8

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

Attachment: 18934.html

Hi Etienne,

I included some screenshots regarding the cases which have been covered. In fact, there is an additional case which is the case when during a call, the Home button is clicked and the call status bar is shown. In that case, when the other party hangs up the call, the call duration is shown and the background is set to red. The point is that the attention screen is not kept time enough to get an screenshot :-)

On the other hand, notice that in attachment 8417278 [details], the name or number of the participant leaving the conference call is wrongly displayed :) , anyhow this is another bug we do have in master. I'll check it does not exist yet and create it if appropriate.

Last but not least, please focus on the call duration functionality since once this lands in master we will apply the visual refresh to it in bug 951606 . Thanks!

Comment 9

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

In fact and to be honest, there is one little change pending consisting on showing the "hang up" icon when the call bar status bar is shown on the left hand side of the status bar :) I have requested Pau the appropriate image and I'll include it as soon as I get it.

Comment 10

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

I created bug 1005862 regarding the notification message issue when a participant leaves the conference ;-)

Comment 11

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

I am currently fixing the callscreen app unit tests :-)

Comment 12

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

Call screen unit tests fixed ;-)

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8417279 [details]
18934.html

+1 for doing this in a separate patch :)

I'm redirecting this review to Doug as we discussed with Rik.

Comment 14

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8417279 [details]
18934.html

Thanks for your work on this. I left comments in the PR. In general, here are my thoughts:

1) There are no new tests. There are several things here that need tests.
2) I don't think we need a new "total-duration" field and can instead restyle the "duration" field that is currently there to look the way we want it to once the call has ended.
3) You added some images, and didn't seem to refer to them anywhere. I think there are similar ones already kicking around somewhere, because they're the same or almost the same as the "hang up" icon on the hangup bar when you're in a call. Maybe this is something you carried over from your visual refresh changes by accident? Not a big deal.
4) There are some changes to the mock HTML of the call screen, but no associated changes to the real HTML, or JS code that generates HTML, that I can see.
5) I didn't do a thorough review of the CSS because I suspect that it'll change pretty significantly on the next iteration. I left a few comments on general approaches.

Comment 15

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

Hi Doug, first off thanks for your comments. I left mine as comments to yours at https://github.com/gtorodelvalle/gaia/commit/407fdf575e27eded36ef33d76f6f37787fce201d BTW, you left yours in the previous commit instead of on the pull request files view (this is https://github.com/mozilla-b2g/gaia/pull/18934/files) which is much more useful since we can follow the evolution and implementation of the requested changes ;-)

Anyhow and regarding your comments:
1) There are no new tests: There is no way of simulating a call as far as I am concerned as a way to hang it up and check that everything is as expected. Of course, I could force (simulate) a duration for a supposedly ongoing call and then dispatch the 'disconnect' event to verify that the total duration is properly shown. Would it be fine with you?
2) We have to show the text "Call ended" plus the total duration of the call. The "Call ended" text is currently shown using the same element which holds the timer during the calls (it also holds the text "Incoming" or "Connecting" when the calls are being established) so I thought it would be wiser not to reuse that element further and have a new one to hold and to show the total duration. There are deeper reasons to this approach such as the distinct font sizes for the "Call ended" text and the total duration of the call as you can see at https://bug899926.bugzilla.mozilla.org/attachment.cgi?id=8383055 as well in the other hanging up scenarios.
3) The new hang up image is used and referenced here: https://github.com/gtorodelvalle/gaia/commit/407fdf575e27eded36ef33d76f6f37787fce201d#diff-a7fdcd5512321333c1aeaecb37d9ef44R112 For further reference, it is the icon shown in the status bar when the call is hung up. The one at the upper left corner of https://bug899926.bugzilla.mozilla.org/attachment.cgi?id=8377628
4) Yeah, I also noticed it :D My verdict is that we did not map some changes made in previous patches to the HTML files to the tests although they kept on passing :O Right now, I checked that the mocks reflect 100% the current HTML code.
5) 100% agreed ;-) Refactoring highly needed :-p

Thank you very much once again for your comments. I'll ask for a new review in a sec. for the updated PR and new commit. Please, feel free to add additional comments or ask me for further details on any of the previous ones.

Important to know if you would like me to include a kind of tricked unit test "to simulate" a call, programmatically set the duration and dispatch the "disconnect" event (in fact, I see it more like an integration one but there is no way of doing it right now as far as my knowledge :-) ).

Comment 16

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8417279 [details]
18934.html

(In reply to Germán Toro del Valle from comment #15)
> Hi Doug, first off thanks for your comments. I left mine as comments to
> yours at
> https://github.com/gtorodelvalle/gaia/commit/
> 407fdf575e27eded36ef33d76f6f37787fce201d BTW, you left yours in the previous
> commit instead of on the pull request files view (this is
> https://github.com/mozilla-b2g/gaia/pull/18934/files) which is much more
> useful since we can follow the evolution and implementation of the requested
> changes ;-)

Thanks for your changes and for this tip. This is one of my first Gaia reviews and I think I made the same mistake on the last few. I'm used to the Bugzilla review tool. :)

> 2) We have to show the text "Call ended" plus the total duration of the
> call. The "Call ended" text is currently shown using the same element which
> holds the timer during the calls (it also holds the text "Incoming" or
> "Connecting" when the calls are being established) so I thought it would be
> wiser not to reuse that element further and have a new one to hold and to
> show the total duration. There are deeper reasons to this approach such as
> the distinct font sizes for the "Call ended" text and the total duration of
> the call as you can see at
> https://bug899926.bugzilla.mozilla.org/attachment.cgi?id=8383055 as well in
> the other hanging up scenarios.

Ah, okay. In that case, we're mixing up our SoC by abusing one element to be used for several things. I think what makes more sense is to have the following:

* An element that stores the duration and gets restyled as needed when the call ends.
* An element that contains "Call Ended" and gets shown/hidden as needed.

That way, we're not sprinkling |node.innerHTML = x| everywhere. This will also make things easier to test as there are fewer states for each element, so it's easier to isolate functionality and test only that.

I'm going to hold off on doing a deeper review until we've really ironed this detail out.

> 1) There are no new tests: There is no way of simulating a call as far as I
> am concerned as a way to hang it up and check that everything is as
> expected. Of course, I could force (simulate) a duration for a supposedly
> ongoing call and then dispatch the 'disconnect' event to verify that the
> total duration is properly shown. Would it be fine with you?

Sure, it's ok if we're assuming a starting state. We don't have to go through the whole call flow. There are some tests already that do this which you can check out here: https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/test/unit/handled_call_test.js#L359

Do you have a good idea as to what we should be testing?

> 3) The new hang up image is used and referenced here:
> https://github.com/gtorodelvalle/gaia/commit/
> 407fdf575e27eded36ef33d76f6f37787fce201d#diff-
> a7fdcd5512321333c1aeaecb37d9ef44R112 For further reference, it is the icon
> shown in the status bar when the call is hung up. The one at the upper left
> corner of https://bug899926.bugzilla.mozilla.org/attachment.cgi?id=8377628

I asked on #gfx and was told that in general it's better to have two pictures at different sizes rather than dynamically resize them, so this is better. Thanks for pointing that out!

> 4) Yeah, I also noticed it :D My verdict is that we did not map some changes
> made in previous patches to the HTML files to the tests although they kept
> on passing :O Right now, I checked that the mocks reflect 100% the current
> HTML code.

There are now several things that could be considered to be followup fixes which are being squashed into this patch. I think this fix falls out of the scope of this bug, so I'd rather you filed a bug for it and fixed it there. I'm fine with https://github.com/gtorodelvalle/gaia/commit/9b6a2b65c5813d733fd53aa1b684dd86cc1e9370#diff-8c52bec747d7922bc983df471c349280R73 since it's so small and seems reasonable.

Clearing review flag for now.

Comment 17

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

OK, I'll include the unit test tomorrow and ask for a new review ;-)

Regarding 2) and although I totally agree with your proposal, I am not sure that it is worth the effort of including those changes/restructuring, I mean in this very PR. The reason is that there are many scenarios where the "Incoming"/"Connecting" text, ongoing timer, "Call ended" text and total duration are shown (1 to 1 calls, status bar calls, call waiting, conference calls, mainly) which we should cover and I think it could be kind of risky to include them on this bug since in some sense it is out of its scope. Anyhow, let me insist that I totally agree with your proposal.

Since I guess Etienne has a deeper insight about the future of the Call Screen app (iteration, refactoring, etc. :) ), I am going to ask for his opinion regarding moving the restructuring of the duration related elements proposed by Doug in comment 16 to a follow-up bug once this one lands. Another reason to support it is that the Call Screen visual refresh work already done (see bug 951606) highly depends on the current structure and we would have a hard rebase in front of us :-) As I said, it is just a matter of if it is worth the effort in the context of a forthcoming and more deeper refactoring of the whole Call Screen app.

BTW, you did and are doing an awesome review ;-)

Comment 18

[Doug Sherk (:drs) (inactive)]

(In reply to Germán Toro del Valle from comment #17)
> Regarding 2) and although I totally agree with your proposal, I am not sure
> that it is worth the effort of including those changes/restructuring, I mean
> in this very PR. The reason is that there are many scenarios where the
> "Incoming"/"Connecting" text, ongoing timer, "Call ended" text and total
> duration are shown (1 to 1 calls, status bar calls, call waiting, conference
> calls, mainly) which we should cover and I think it could be kind of risky
> to include them on this bug since in some sense it is out of its scope.
> Anyhow, let me insist that I totally agree with your proposal.

Oh, I don't think this will be a major change. To go into more detail into what I suggested:

* An element that stores the duration and gets restyled as needed when the call ends.
You will have to move the duration counter to this element, which shouldn't be difficult as it's only referred to a handful of times that I can see.
* An element that contains "Call Ended" and gets shown/hidden as needed.
This would remain unchanged in every other way, other than having to hide it when the call has ended.

I think the method we're using in this patch is non-ideal enough, through no fault of your own, that we should really deal with this now. But if Etienne thinks we should punt it, it's his call.

Comment 19

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

Yeah, yeah, probably not the major change we have faced in our lives :p but have a look at oncall.css (a 1000 lines long CSS file :-O) and the mess of rules we have and you'll know what I mean :p No need to say, that it can be faced of course. The point is if now or on a forthcoming bug + patch ;-)

Hopefully a deep refactor (covering much more of what you proposed and which I 100% support) of the Call Screen app is coming ;-) Apart from the deep dependency on the work already done in the visual refresh of the Call Screen which would be impacted by this change.

I'll prepare the test case and, as you mention, let Etienne decide :-p Absolutely no problem on my side to go for any of the proposed alternatives ;-)

Comment 20

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

I have included the unit tests in the PR: attachment 8417279 [details] ;-)

Comment 21

[Etienne Segonzac (:etienne)]

(In reply to Germán Toro del Valle from comment #17)
> Since I guess Etienne has a deeper insight about the future of the Call
> Screen app (iteration, refactoring, etc. :) ), I am going to ask for his
> opinion regarding moving the restructuring of the duration related elements
> proposed by Doug in comment 16 to a follow-up bug once this one lands.
> Another reason to support it is that the Call Screen visual refresh work
> already done (see bug 951606) highly depends on the current structure and we
> would have a hard rebase in front of us :-) As I said, it is just a matter
> of if it is worth the effort in the context of a forthcoming and more deeper
> refactoring of the whole Call Screen app.

Not sure if this is exactly the information needed but:
Using handledCall dom nodes for more than 1 thing already cost us more than a clean refactoring would have. Can't put my hand on it but I'm pretty sure a follow-up was filed at some point for the exact same kind of issue regarding the phone number display.
To be honest I thought the visual refresh would be the perfect opportunity to do this kind of cleanup, but I didn't push for it so my bad.

If it's a "few hours project" I'd vote for doing it right now since history shows we don't really follow-up on this kind of issues. If it's more complex than that because of other technical debts (looking at the css :)) let's prioritize the visual refresh.

Comment 22

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

Hey! Yeah, I could take care of that restructuring in the Call Screen visual refresh bug (this is bug 951606). My main desire is to land this bug :-p We need it to continue working on the visual refresh of the Call Screen :-p

In fact and once the visual refresh of the Call Screen lands in master, I would like to give a try to simplifying the Call Screen HTML and CSS code heavily.

What about this?: let's move that duration node restructuring to bug 951606. There are many updates already done in the visual refresh which heavily depend on the current structure. I think it will be easier to do that duration node restructuring from the visual refresh code than having to rebase with that code coming from master ;-) It would be on me :-p

Comment 23

[Doug Sherk (:drs) (inactive)]

(In reply to Germán Toro del Valle from comment #22)
> Hey! Yeah, I could take care of that restructuring in the Call Screen visual
> refresh bug (this is bug 951606). My main desire is to land this bug :-p We
> need it to continue working on the visual refresh of the Call Screen :-p
> 
> In fact and once the visual refresh of the Call Screen lands in master, I
> would like to give a try to simplifying the Call Screen HTML and CSS code
> heavily.
> 
> What about this?: let's move that duration node restructuring to bug 951606.
> There are many updates already done in the visual refresh which heavily
> depend on the current structure. I think it will be easier to do that
> duration node restructuring from the visual refresh code than having to
> rebase with that code coming from master ;-) It would be on me :-p

I'm not clear on how this is blocking other work since it seems fairly unrelated to the rest of the visual refresh. Perhaps you can enlighten me. As far as I know, we also still have quite a bit of time before feature completion for 2.0. I think it's great that you want to clean up the HTML and CSS, but landing additional work that we know is bad architecturally vs. punting cleanup on code that's already there are two different issues. I still think we should fix this here and now.

Comment 24

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

Absolutely! This is blocking the visual refresh because this functionality is being implemented in current master code since not already supported but has already been considered in the visual refresh of the Call Screen as you can see in bug 899926 and the files attached on it. So, we need to land this bug in master and then apply the visual refresh to it. On the other hand and as I have mentioned many times, a lot of work already done regarding the visual refresh of the Call Screen (please see the screenshots attached in bug bug 951606) depend heavily on the current structure of elements. Changing the structure of the elements in this bug will cause a pretty painful rebasing in the visual refresh bug.

On the other hand and in my opinion the additional work is not bad architecturally. What it is bad architecturally is the already existent code ;-) As I said it is a matter of the order of doing things and if we want to refactor code in a bug which is not focused on refactoring code ;-)

Anyhow and as a way to go directly to the desired option, I would highly appreciate if you could outline the final HTML code you would be fine with ;-) The current code is:
<div class="duration">
  <span class="font-light"></span>
  <div class="direction"></div>
  <div class="total-duration font-light"></div>
</div>

Are you thinking is something such as?:
<div class="duration-and-direction">
  <div class="call-ended font-light" data-l10n-id="callEnded"></div>
  <div class="duration font-light"></div>
  <div class="direction"></div>
</div>

Comment 25

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8417279 [details]
18934.html

(In reply to Germán Toro del Valle from comment #24)
> Absolutely! This is blocking the visual refresh because this functionality
> is being implemented in current master code since not already supported but
> has already been considered in the visual refresh of the Call Screen as you
> can see in bug 899926 and the files attached on it. So, we need to land this
> bug in master and then apply the visual refresh to it. On the other hand and
> as I have mentioned many times, a lot of work already done regarding the
> visual refresh of the Call Screen (please see the screenshots attached in
> bug bug 951606) depend heavily on the current structure of elements.
> Changing the structure of the elements in this bug will cause a pretty
> painful rebasing in the visual refresh bug.
> 
> On the other hand and in my opinion the additional work is not bad
> architecturally. What it is bad architecturally is the already existent code
> ;-) As I said it is a matter of the order of doing things and if we want to
> refactor code in a bug which is not focused on refactoring code ;-)

Okay, thank you for explaining. I agree with you now and I think this is better to do in a followup. Please file an actual bug for this issue as I'd rather address it on its own than squash it into another bug (since we're not going to do it here).

> Anyhow and as a way to go directly to the desired option, I would highly
> appreciate if you could outline the final HTML code you would be fine with
> ;-) The current code is:
> <div class="duration">
>   <span class="font-light"></span>
>   <div class="direction"></div>
>   <div class="total-duration font-light"></div>
> </div>
> 
> Are you thinking is something such as?:
> <div class="duration-and-direction">
>   <div class="call-ended font-light" data-l10n-id="callEnded"></div>
>   <div class="duration font-light"></div>
>   <div class="direction"></div>
> </div>

For future reference, yeah, this is something like what I was thinking. Though this:
>   <div class="call-ended font-light" data-l10n-id="callEnded"></div>

... would contain more than the "Call Ended" marker, as you indicated in comment 15, point 2. So a more general name would be preferable.

Also, this is kind of verbose:
> <div class="duration-and-direction">

But off the top of my head, I can't think of a better name.

Comment 26

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

Great, thanks! ;-) Sorry if I did not make myself clear before :-)

Regarding |<div class="call-ended font-light" data-l10n-id="callEnded"></div>| and just to share this with you (and with me :p) I guess you refer to the "Incoming" and "Connecting" messages shown when establishing the call. I totally agree with you that we could use that element to show this information about the status of the call and we may end up with something such as:
<div class="call-info"> <!-- Or any other informative name. -->
  <div class="call-status font-light"></div>
  <div class="duration font-light"></div>
  <div class="direction"></div>
</div>

Sounds good to me! ;-)

Comment 27

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

I created bug 1007148 as requested in comment 25 to deal with the needed refactoring proposed by Doug. Please, Doug, feel free to add any additional information to the bug or update the title if appropriate ;-) Thanks!

Comment 28

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8417279 [details]
18934.html

Okay, I left some more review comments on the PR. Here are some extra things that didn't make it into my comments:

Please fix the JSHint goop (exported/globals) in apps/callscreen/test/unit/conference_group_handler_test.js and apps/callscreen/test/unit/handled_call_test.js so we can remove them from xfail.list.

In call_screen.js, the duration timer is set using the following code: https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/call_screen.js#L524
We should make the new code in this patch, including the tests for it, use this instead of static text like '12:34', and checking for the string containing ':'.

Comment 29

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

I created bug 1007175 to deal with removing hard-coding HTML code from the unit tests related to this bug and moving them to external files ;-)

Comment 30

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

Comment on attachment 8417279 [details]
18934.html

Hi Doug! I think I have covered all your comments \o/ but please have a new look at the update PR.

Regarding your last comment: sadly, we cannot use the CallScreen.createTicker() function in the tests affected by this bug since they do not use the "real" CallScreen object but make use of MockCallScreen which as you can see at https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/test/unit/mock_call_screen.js does not have a functional createTicker() function.

Any further comments are more than welcome ;-) Thanks!

Comment 31

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8417279 [details]
18934.html

We're getting there! My comments in the PR basically cover all of my thoughts. I'll test this soon.

Comment 32

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

Comment on attachment 8417279 [details]
18934.html

Yeah, almost there :-)

Updated PR including your last comments, Doug ;-) Thanks!

Comment 33

[Doug Sherk (:drs) (inactive)]

Attachment: bug-1003846.mp4

German, I tested every flow other than conference calls because I wasn't able to merge calls and I doubt it will be any different there, based on the screenshots. I noticed a couple of problems.

The direction indicator now bounces around depending on the length of the time string.

Also, at the end, you can see that the "via SIM2" indicator and total duration indicator overlap with each other.

Comment 34

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8417279 [details]
18934.html

Also, I left some more comments on the PR. Once the issues in comment 33 have been fixed, I'll do another pass on reviewing the CSS.

Comment 35

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

(In reply to Doug Sherk (:drs) from comment #33)
> Created attachment 8419546 [details]
> bug-1003846.mp4
> 
> German, I tested every flow other than conference calls because I wasn't
> able to merge calls and I doubt it will be any different there, based on the
> screenshots. I noticed a couple of problems.
> 
> The direction indicator now bounces around depending on the length of the
> time string.
> 
Hi Doug, it always has :-) So I do not think we should be dealing with it in this bug. Do you want me to file a bug about it since I guess we'll have also to request UX advice on the issue?

> Also, at the end, you can see that the "via SIM2" indicator and total
> duration indicator overlap with each other.

I'll try to get a DSDS device to solve the second issue but to deal with it we also need UX guidance to decide the final look of it ;-) So need-infoing also Vicky about this second issue. Do we have an spec. about it? Thanks!

Comment 36

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

Indeed, I guess that new spec/visual should be included in bug 990536, as discussed with Noemí, applying the visual refresh. I will include in this bug an adaptation using the current code and look and feel in master and in bug 990536 we will apply the final appearance ;-)

Comment 37

[Doug Sherk (:drs) (inactive)]

(In reply to Germán Toro del Valle from comment #35)
> Hi Doug, it always has :-) So I do not think we should be dealing with it in
> this bug. Do you want me to file a bug about it since I guess we'll have
> also to request UX advice on the issue?

Yikes, I'm surprised we let that pass. I guess I just never noticed it before. Yeah, please file a bug.

Comment 38

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

Hi Doug, I filed bug 1008821 to deal with the direction icon moving depending on the duration of the call ;-) BTW, I reused your video :-)

Comment 39

[Victoria Gerchinhoren [:vicky]]

Attachment: Visual Mockup. Call Ended with call duration new position

Comment 40

[Victoria Gerchinhoren [:vicky]]

Attachment: Visual Mockup. Call Ended with call duration new position

Comment 41

[Victoria Gerchinhoren [:vicky]]

Uploaded a mockup of how to reposition the call duration in the ended call to avoid SIM indicator overlap when having dual sim.

Note that in this new position, call duration has the same font size as "Call ended" text but the style is "Regular" instead of "Light". 

Position of the icon is 1.5 rem gap between text and icon.

Thanks

Comment 42

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

Attachment: callscreen_number_call_ended_dsds.png

Please, do not focus on the concrete appearance since we will apply the visual refresh in bug 951606 ;-)

Comment 43

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8421009 [details]
callscreen_number_call_ended_dsds.png

The space between "Call ended" and the duration is a single space.

Also the icon is not correct but you tell me it is solved in a different bug.

Comment 44

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

Attachment: callscreen_number_call_ended_dsds.png

Minor correction although I'll ask for ui-review when the visual refresh is applied to the Call Screen in bug 951606.

Comment 45

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

Comment on attachment 8417279 [details]
18934.html

I think I have covered all your comments ;-) Thank you very much!

Comment 46

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8417279 [details]
18934.html

\o/

I left one little comment. Please address that and then I think we're good to go. I expect you to follow up on the bugs that we filed though. :)

ni? Etienne since I'm not sure if he wants to sr=? this or something since I'm not a dialer peer yet.

Comment 47

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

Yaaay!!! :-) Yeah, I included that final comment too ;-) Thanks!

Waiting on Etienne's and Travis' blessing :-p to land the patch.

Comment 48

[Etienne Segonzac (:etienne)]

(In reply to Doug Sherk (:drs) from comment #46)
> Comment on attachment 8417279 [details]
> 18934.html
> 
> \o/
> 
> I left one little comment. Please address that and then I think we're good
> to go. I expect you to follow up on the bugs that we filed though. :)
> 
> ni? Etienne since I'm not sure if he wants to sr=? this or something since
> I'm not a dialer peer yet.

Not needed, a peer forwarded you the review :)

Comment 49

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

Merged in master: https://github.com/mozilla-b2g/gaia/commit/a25035304a560d5f5099bb1afc615d2b3ebc7d8b

Comment 50

[Loli (:lolimartinezcr)]

Tested and depends: 1009526 y 1009515

Comment 51

[Loli (:lolimartinezcr)]

Tested and fine
2.0
Gecko-3e5340d
Gaia-7416a20