Closed
Bug 1021478
Opened 10 years ago
Closed 10 years ago
Set outgoing calls "Always ask" with two sim card inside, use bluetooth headset to dial, there will be no network message
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(blocking-b2g:2.0M+, b2g-v1.3T wontfix, b2g-v1.4 fixed, b2g-v2.0 wontfix, b2g-v2.0M verified, b2g-v2.1 verified)
People
(Reporter: wei.gao, Assigned: drs)
References
()
Details
Attachments
(2 files, 14 obsolete files)
9.78 KB,
patch
|
rik
:
review+
|
Details | Diff | Splinter Review |
8.28 MB,
video/mp4
|
Details |
OS version
---------------------------------------------
FireFoxOS v1.4
Reproduce steps:
---------------------------------------------
1) With two sim cards inside
2) Settings > SIM manager > Outgoing calls > Always ask
3) Just dial a phone using any card
4) Connect bluetooth headset, and double click button of bluetooth headset to dial
5) There will be no network message
Expected result:
---------------------------------------------
Dial normal
Reporter | ||
Comment 1•10 years ago
|
||
I think the cause is when set outgoing calls "Always ask", the value of 'ril.telephony.defaultServiceId' is -1.
When dial via bluetooth headset, navigator.mozMobileConnections[-1] is undefined, so there will be no network message.
There is one patch about this, and it will work normal.
Please help to review.
Thanks a lot.
Flags: needinfo?(ehung)
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8435487 [details] [diff] [review]
bluetooth_dial.patch
Hi Etienne
Can you help to review this patch?
Thank you very much.
Attachment #8435487 -
Flags: review?(etienne)
Updated•10 years ago
|
Flags: needinfo?(ehung)
Comment 3•10 years ago
|
||
IMHO SimSettingsHelper should return the original value and let the app display the corresponding UI.
Comment 4•10 years ago
|
||
Comment on attachment 8435487 [details] [diff] [review]
bluetooth_dial.patch
Review of attachment 8435487 [details] [diff] [review]:
-----------------------------------------------------------------
Forwarding to Doug.
Attachment #8435487 -
Flags: review?(etienne) → review?(drs+bugzilla)
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8435487 [details] [diff] [review]
bluetooth_dial.patch
(In reply to Arthur Chen [:arthurcc] from comment #3)
> IMHO SimSettingsHelper should return the original value and let the app
> display the corresponding UI.
I agree with this. Knowing that it's set to "always ask" is useful information, and without this we're actually liable to break anything that uses this function.
Attachment #8435487 -
Flags: review?(drs+bugzilla) → review-
Reporter | ||
Comment 6•10 years ago
|
||
> I agree with this. Knowing that it's set to "always ask" is useful information,
> and without this we're actually liable to break anything that uses this function.
Hi Doug
I think that's right. So I modify the code so as to show the "always ask".
Can you help to check this patch if it can be granted.
Thank you very much.
Flags: needinfo?(drs+bugzilla)
Assignee | ||
Comment 7•10 years ago
|
||
My understanding here is that the case you're handling is when the user hits the call button from a Bluetooth headset. Here, there's no easy way for the user to pick which SIM to call from. I think this really needs UX review. This is a problem that we have everywhere from v1.4 onward, so I think it's best if we deal with this properly, even if we create an interim bad solution before doing something better.
My opinion, acknowledging that I'm designing in a vacuum, is that (for now) we should just arbitrarily make the call from SIM 1. If the user has the device in their hands, then they wouldn't be using the Bluetooth call button to begin with. Or if they were, then they could end the call as soon as they see that it's being made from the wrong SIM. So I think it makes the most sense to assume that the user has the device in their pocket and doesn't want to have to take it out to make the call.
Another alternative is to have a second "make calls from" SIM setting for Bluetooth devices, where "Always ask" isn't an option. I actually like this more than the former option, but it would involve a lot more work. We could do this as a followup, though.
Carrie, what do you think?
Flags: needinfo?(drs+bugzilla) → needinfo?(cawang)
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Doug Sherk (:drs) from comment #7)
>
> My opinion, acknowledging that I'm designing in a vacuum, is that (for now)
> we should just arbitrarily make the call from SIM 1. If the user has the
> device in their hands, then they wouldn't be using the Bluetooth call button
> to begin with. Or if they were, then they could end the call as soon as they
> see that it's being made from the wrong SIM. So I think it makes the most
> sense to assume that the user has the device in their pocket and doesn't
> want to have to take it out to make the call.
>
Hi Doug and Carrie
I agree with you, let users to choose a sim is not a good user experience.
By the way, can't we designed to use the last sim to dial the last call number when make a call from a bluetooth headset?
Is that be granted?
Thanks.
Comment 9•10 years ago
|
||
Hi,
I wonder if it's double-click, it should be calling back to the most recent call of the history. In this case, we should use the SIM that the user used for dialing the call last time. Some BT devices can choose calls from call history and we can still adopt the rule here - always use the SIM that users used for that call log last time.
If users want to make call to other contacts which are not in the call log, they shall use voice command and I think we don't have this function yet on our OS. So probably we can exclude this use case for now.
Does this make sense to you guys? Thanks!
Flags: needinfo?(cawang)
Reporter | ||
Comment 10•10 years ago
|
||
Hi Doug and Carrie
I think this patch can realize our idea, when dial a number from bluetooth headset, we should dial the last call number through the sim used last time instead of letting users to choose a sim card.
Can this be granted?
Thanks.
Attachment #8435487 -
Attachment is obsolete: true
Reporter | ||
Comment 11•10 years ago
|
||
Hi,
I recommit a patch about this.
Please help to review.
Thanks.
Attachment #8441139 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8441223 [details] [diff] [review]
bug1021478_dial_the_last_callnumber_through_the_sim_used_last_time.patch
(In reply to Carrie Wang [:carrie] from comment #9)
> Hi,
>
> I wonder if it's double-click, it should be calling back to the most recent
> call of the history. In this case, we should use the SIM that the user used
> for dialing the call last time. Some BT devices can choose calls from call
> history and we can still adopt the rule here - always use the SIM that users
> used for that call log last time.
> If users want to make call to other contacts which are not in the call log,
> they shall use voice command and I think we don't have this function yet on
> our OS. So probably we can exclude this use case for now.
> Does this make sense to you guys? Thanks!
Carrie, I agree with this but I'm not sure if it's possible to get this done in time, or that it's safe enough, to land on v1.4. Would you be OK with using my suggestion for landing on v1.4, and your method for v2.0 onward?
(In reply to wei.gao from comment #11)
> Created attachment 8441223 [details] [diff] [review]
> bug1021478_dial_the_last_callnumber_through_the_sim_used_last_time.patch
>
> Hi,
>
> I recommit a patch about this.
> Please help to review.
> Thanks.
Sorry, I think I missed this. To help me find it, please set the review flag next time. We need Carrie's reply before I can review this, though. Thanks!
Attachment #8441223 -
Flags: review?(drs+bugzilla)
Flags: needinfo?(cawang)
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Doug Sherk (:drs) from comment #12)
> To help me find it, please set the review flag next time.
Sorry, I get it, I will do so in the future.
Comment 14•10 years ago
|
||
I would have concerns on using SIM 1 as default SIM in this case. Since users select always ask as their preference, we can't really predict which SIM will be their "expected default SIM" and this might cause confusion and even unexpected cost. However, I'll leave this time frame issue to EPM and if we really can't make it, I'd prefer still popping up the options for users to select the SIM, because this is all about cost and users do really care. Thanks!
Flags: needinfo?(cawang) → needinfo?(whuang)
Comment 15•10 years ago
|
||
This isn't a release blocker so just put into backlog.
Fix can be cherry picked from master if partner needs it.
blocking-b2g: --- → backlog
Flags: needinfo?(whuang)
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8441223 [details] [diff] [review]
bug1021478_dial_the_last_callnumber_through_the_sim_used_last_time.patch
(In reply to Carrie Wang [:carrie] from comment #14)
> I would have concerns on using SIM 1 as default SIM in this case. Since
> users select always ask as their preference, we can't really predict which
> SIM will be their "expected default SIM" and this might cause confusion and
> even unexpected cost. However, I'll leave this time frame issue to EPM and
> if we really can't make it, I'd prefer still popping up the options for
> users to select the SIM, because this is all about cost and users do really
> care. Thanks!
(In reply to Wesley Huang [:wesley_huang] from comment #15)
> This isn't a release blocker so just put into backlog.
> Fix can be cherry picked from master if partner needs it.
OK, thanks. In that case, this patch isn't the correct fix, so I have to r- it.
Wei, let me know if you need help implementing it.
Attachment #8441223 -
Flags: review?(drs+bugzilla) → review-
Reporter | ||
Comment 17•10 years ago
|
||
(In reply to Carrie Wang [:carrie] from comment #9)
> I wonder if it's double-click, it should be calling back to the most recent
> call of the history. In this case, we should use the SIM that the user used
> for dialing the call last time. Some BT devices can choose calls from call
> history and we can still adopt the rule here - always use the SIM that users
> used for that call log last time.
(In reply to Carrie Wang [:carrie] from comment #14)
> I would have concerns on using SIM 1 as default SIM in this case. Since
> users select always ask as their preference, we can't really predict which
> SIM will be their "expected default SIM" and this might cause confusion and
> even unexpected cost.
Ok, but now there still show no network message when setting outgoing calls "Always ask" with two sim card inside, and using bluetooth headset to dial.
I am confused about the strategy we desided at last. Is our decision to use the sim1 as default?
I wonder can this patch be granted.
Thanks.
Attachment #8444887 -
Flags: review?(drs+bugzilla)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8444887 [details] [diff] [review]
Bug319336_dial_the_last_callnumber_through_sim1.patch
(In reply to Wei Gao (Spreadtrum) from comment #17)
> I am confused about the strategy we desided at last. Is our decision to use
> the sim1 as default?
> I wonder can this patch be granted.
> Thanks.
Wei, we decided that the call should be placed on the SIM that the contact being called was last in a call with. So for example, if you call me on my SIM2, then we hang up, and then I hit the bluetooth call button, the call should be placed on SIM2.
I'm giving your patch feedback+ because I think it's better than what we currently have, and if a partner wants to cherry-pick it, it's better than nothing. But if we want to land this on the Mozilla Gaia repo, we'll have to have the better fix.
Attachment #8444887 -
Flags: review?(drs+bugzilla) → feedback+
Reporter | ||
Comment 19•10 years ago
|
||
(In reply to Doug Sherk (:drs) from comment #18)
> Wei, we decided that the call should be placed on the SIM that the contact
> being called was last in a call with. So for example, if you call me on my
> SIM2, then we hang up, and then I hit the bluetooth call button, the call
> should be placed on SIM2.
Hi Doug
Thanks for your attention.
I agree with you when the user set with "Always ask". But if the user set "Outgoing calls" with "Sim1" or "Sim2", when the user call a number through bluetooth headset, which sim should be used should depend on the setting in any case I think.
How do you think about it?
Thanks.
Flags: needinfo?(drs+bugzilla)
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Wei Gao (Spreadtrum) from comment #19)
> Hi Doug
>
> Thanks for your attention.
> I agree with you when the user set with "Always ask". But if the user set
> "Outgoing calls" with "Sim1" or "Sim2", when the user call a number through
> bluetooth headset, which sim should be used should depend on the setting in
> any case I think.
> How do you think about it?
>
> Thanks.
That's a great question. My knee-jerk answer based on my understanding of how our users use dual-SIM devices would be that it should call on the SIM you have set in your settings, but I think it's best if Carrie decides that.
Also, I'm sorry for taking so long to reply. It's not really an excuse but I got swamped with other things. I'll be more responsive from now on.
Flags: needinfo?(drs+bugzilla) → needinfo?(cawang)
Reporter | ||
Comment 21•10 years ago
|
||
(In reply to Doug Sherk (:drs) from comment #20)
> Also, I'm sorry for taking so long to reply. It's not really an excuse but I
> got swamped with other things. I'll be more responsive from now on.
That's nothing, it is no big deal.
No matter how, I should thank you for your kindly help.
Comment 22•10 years ago
|
||
Yes, if user has set SIM1/ SIM2 as primary SIM for outgoing calls in Settings, we should apply their settings even when they use BT headset to make calls. Thanks!
Flags: needinfo?(cawang)
Reporter | ||
Comment 23•10 years ago
|
||
(In reply to Carrie Wang [:carrie] from comment #22)
> Yes, if user has set SIM1/ SIM2 as primary SIM for outgoing calls in
> Settings, we should apply their settings even when they use BT headset to
> make calls. Thanks!
Hi Doug
Can this patch be granted?
Please help to review it.
Thanks.
Attachment #8441223 -
Attachment is obsolete: true
Attachment #8444887 -
Attachment is obsolete: true
Attachment #8450039 -
Flags: review?(drs+bugzilla)
Reporter | ||
Updated•10 years ago
|
Attachment #8450039 -
Flags: review?(drs+bugzilla)
Reporter | ||
Comment 24•10 years ago
|
||
I am sorry for there is a little problem in that patch.
I commit it again.
Please help to review.
Thanks a great.
Attachment #8450039 -
Attachment is obsolete: true
Attachment #8450041 -
Flags: review?(drs+bugzilla)
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8450041 [details] [diff] [review]
dial_via_bluetooth_headset.patch
Review of attachment 8450041 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Wei Gao (Spreadtrum) from comment #24)
> Created attachment 8450041 [details] [diff] [review]
> dial_via_bluetooth_headset.patch
>
> I am sorry for there is a little problem in that patch.
> I commit it again.
> Please help to review.
> Thanks a great.
I would strongly recommend adding tests for this functionality to dialer_test.js.
Attachment #8450041 -
Flags: review?(drs+bugzilla) → review-
Reporter | ||
Comment 26•10 years ago
|
||
(In reply to Doug Sherk (:drs) from comment #25)
> I would strongly recommend adding tests for this functionality to
> dialer_test.js.
As for v1.4, there is little tests and many mock file is not added in. I wonder I should add tests in v1.4 or master?
Thanks.
Flags: needinfo?(drs+bugzilla)
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Wei Gao (Spreadtrum) from comment #26)
> (In reply to Doug Sherk (:drs) from comment #25)
> > I would strongly recommend adding tests for this functionality to
> > dialer_test.js.
>
> As for v1.4, there is little tests and many mock file is not added in. I
> wonder I should add tests in v1.4 or master?
> Thanks.
I just checked, and everything you need should be on both branches. The patch might be slightly different when uplifted to v1.4, but I don't think it'll be significant. Is there anything in particular that you noticed was lacking that you need?
Flags: needinfo?(drs+bugzilla)
Reporter | ||
Comment 28•10 years ago
|
||
Hi Doug
As I know litter about how to write test file, maybe there will be many questions in my patch. Could you help to correct me?
Thanks a great.
Attachment #8450041 -
Attachment is obsolete: true
Flags: needinfo?(drs+bugzilla)
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8454890 [details] [diff] [review]
dial_via_bluetooth_headset.patch
Review of attachment 8454890 [details] [diff] [review]:
-----------------------------------------------------------------
This is actually good overall. There are just some small mechanical mistakes within the tests that I think will be easy to fix.
::: apps/communications/dialer/js/dialer.js
@@ +358,5 @@
> if (result && (typeof result === 'object') && result.number) {
> + var settings = navigator.mozSettings;
> + var getServiceId = settings.createLock().get('ril.telephony.defaultServiceId');
> + getServiceId.onsuccess = function getDefaultServiceId() {
> + var ServiceId = getServiceId.result['ril.telephony.defaultServiceId'] || 0;
s/ServiceId/serviceId/
@@ +359,5 @@
> + var settings = navigator.mozSettings;
> + var getServiceId = settings.createLock().get('ril.telephony.defaultServiceId');
> + getServiceId.onsuccess = function getDefaultServiceId() {
> + var ServiceId = getServiceId.result['ril.telephony.defaultServiceId'] || 0;
> + if(ServiceId == -1) {
Space between if and (
@@ +363,5 @@
> + if(ServiceId == -1) {
> + // dial the last call number via the sim used last time
> + CallHandler.call(result.number, result.serviceId);
> + }
> + else {
} else {
::: apps/communications/dialer/test/unit/dialer_test.js
@@ +460,5 @@
> + MockNavigatorSettings.mSettings['ril.telephony.defaultServiceId'] = -1;
> + getSpy.yield({number: '424242', serviceId: 0});
> + sinon.assert.calledWith(callSpy, '424242', 0);
> + getSpy.yield({number: '424242', serviceId: 1});
> + sinon.assert.calledWith(callSpy, '424242', 1);
I would write this as:
```js
suite('> Dialing last entry in the call log', function() {
var getSpy;
var callSpy;
setup(function() {
getSpy = this.sinon.stub(MockCallLogDBManager, 'getGroupAtPosition');
callSpy = this.sinon.stub(CallHandler, 'call');
sendCommand('BLDN');
sinon.assert.calledWith(getSpy, 1, 'lastEntryDate', true);
});
[0, 1].forEach(function(serviceId) {
test('should dial on correct SIM', function() {
// use serviceId var
// ...
});
});
test('should use last serviceId when set to always ask', function() {
// check if it calls on 0 and 1
// ...
});
});
```
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(drs+bugzilla)
Reporter | ||
Comment 30•10 years ago
|
||
Hi Doug
Thanks for your suggestion. That's very helpful for me.
I modified the patch, but there still some mistakes. I am confused about it.
Could you help me again?
Thanks a great.
Error: expected getGroupAtPosition to be called with arguments 1, lastEntryDate, true
Attachment #8454890 -
Attachment is obsolete: true
Flags: needinfo?(drs+bugzilla)
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Wei Gao (Spreadtrum) from comment #30)
> Created attachment 8455089 [details] [diff] [review]
> dial_via_bluetooth_headset.patch
>
> Hi Doug
>
> Thanks for your suggestion. That's very helpful for me.
> I modified the patch, but there still some mistakes. I am confused about it.
> Could you help me again?
> Thanks a great.
This is good progress, don't worry.
> Error: expected getGroupAtPosition to be called with arguments 1,
> lastEntryDate, true
Where are you getting this error?
Flags: needinfo?(drs+bugzilla)
Reporter | ||
Comment 32•10 years ago
|
||
Dear Doug
When I run "./bin/gaia-test" to make a unit test, this error will show in terminal.
I don't know what's the matter.
Is there anything wrong about my operation?
Flags: needinfo?(drs+bugzilla)
Assignee | ||
Comment 33•10 years ago
|
||
Hi Wei,
test('> Dialing a specific recent entry', function() {
should be
suite('> Dialing a specific recent entry', function() {
Same for '> Dialing the last recent entry'
Flags: needinfo?(drs+bugzilla)
Reporter | ||
Comment 34•10 years ago
|
||
(In reply to Doug Sherk (:drs) from comment #33)
> test('> Dialing a specific recent entry', function() {
>
> should be
>
> suite('> Dialing a specific recent entry', function() {
>
> Same for '> Dialing the last recent entry'
Thanks for your suggestion.
That's so correct. But there still has some errors in unit-test.
I think "MockNavigatorSettings.mSettings['ril.telephony.defaultServiceId'] = serviceId;" is not good for this.
Error: expected call to be called with arguments 424242, 0
Error: expected call to be called with arguments 424242, 1
Error: expected call to be called with arguments 424242, 0
Error: expected call to be called with arguments 333, 0
Error: expected call to be called with arguments 333, 1
Error: expected call to be called with arguments 333, 0
I am so sorry to trouble you, but I don't know how to write correctly.
By the way, is there some study materials of unit-test file.
Thanks for your kindly help.
Attachment #8455089 -
Attachment is obsolete: true
Attachment #8455919 -
Attachment is obsolete: true
Flags: needinfo?(drs+bugzilla)
Assignee | ||
Comment 35•10 years ago
|
||
Ok, here it is fixed up. This patch can be cherry-picked if a partner needs it.
Attachment #8439646 -
Attachment is obsolete: true
Attachment #8458480 -
Attachment is obsolete: true
Flags: needinfo?(drs+bugzilla)
Reporter | ||
Comment 36•10 years ago
|
||
(In reply to Doug Sherk (:drs) from comment #35)
> Created attachment 8460334 [details] [diff] [review]
> Use last used SIM or default call SIM when calling with Bluetooth.
>
> Ok, here it is fixed up. This patch can be cherry-picked if a partner needs
> it.
Oh, Thanks for your kindly help.
Ok, could I commit a PR for this patch, then it can be landed on master/v1.4/v1.3t ?
Thanks a lot.
Flags: needinfo?(drs+bugzilla)
Assignee | ||
Comment 37•10 years ago
|
||
Since this is closer to, if not the, correct solution, I think we should explore landing this again. I'm not sure if it'll get approval to land on 1.4 though. Partners might have to cherry-pick it themselves.
Carrie, here's what this patch implements. When the user hits the Bluetooth call button:
* If the outgoing call SIM is set to "always ask", we get the last-used SIM for the last-called contact and call back to them on it.
* If the outgoing call SIM is set to anything else, we use that SIM and call the last-called contact with it.
Does this seem reasonable to you? What should we do if the call log is empty? My suggestion would be to just ignore the button press, or open the keypad.
This isn't ready to land as it needs a proper review. Since I've written a lot of it, I can't review it anymore. I'd also like to do some cleanup on it before we take it back into review.
Some notes for myself when I come back to this:
* Can we do something cleaner than using setTimeout?
* Maybe we should assume the outgoing call SIM is -1/ALWAYS_ASK if it's not set.
* We need to fix the teardown logic.
* The CallHandler.call() function call can be consolidated a bit.
* We don't need |var settings|.
Flags: needinfo?(drs+bugzilla) → needinfo?(cawang)
Comment 38•10 years ago
|
||
Hi Doug,
Yes, it looks fine to me.
In addition, if the call log is empty, we just ignore the click, because users may not look at the screen while using BT headset. Thanks!
Flags: needinfo?(cawang)
Assignee | ||
Comment 39•10 years ago
|
||
Thanks Carrie. I'm taking this.
Assignee: nobody → drs+bugzilla
Status: NEW → ASSIGNED
Target Milestone: --- → 2.1 S1 (1aug)
Assignee | ||
Updated•10 years ago
|
status-b2g-v1.3T:
--- → affected
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
Summary: [1.4] Set outgoing calls "Always ask" with two sim card inside, use bluetooth headset to dial, there will be no network message → Set outgoing calls "Always ask" with two sim card inside, use bluetooth headset to dial, there will be no network message
Assignee | ||
Comment 40•10 years ago
|
||
Attachment #8460334 -
Attachment is obsolete: true
Attachment #8462799 -
Flags: review?(anthony)
Comment 41•10 years ago
|
||
Comment on attachment 8462799 [details] [diff] [review]
Call on the last used SIM for this contact/number when hitting the Bluetooth call button.
Review of attachment 8462799 [details] [diff] [review]:
-----------------------------------------------------------------
feedback+ for the general implementation.
Can you also take a look and see if we can safely remove the shim from bug 982163 after landing this?
::: apps/communications/dialer/js/dialer.js
@@ +380,5 @@
> CallLogDBManager.getGroupAtPosition(position, 'lastEntryDate', true, null,
> function(result) {
> if (result && (typeof result === 'object') && result.number) {
> + var request = navigator.mozSettings.createLock().get(
> + 'ril.telephony.defaultServiceId');
We should load sim_settings_helper here. We are going lazy load it anyway in CallHandler.call. That will avoid the duplication of ALWAYS_ASK_OPTION_VALUE.
::: apps/communications/dialer/test/unit/dialer_test.js
@@ +488,5 @@
> });
> }
>
> test('> Dialing a specific number', function() {
> + var callStub = this.sinon.stub(CallHandler, 'call');
Why all the spy -> stub changes? We're just watching the usage of CallHandler.call, we're not observing the return values.
Attachment #8462799 -
Flags: review?(anthony) → feedback+
Assignee | ||
Comment 42•10 years ago
|
||
The SimSettingsHelper.ALWAYS_ASK_OPTION_VALUE being loaded for the test is actually `undefined` because it's not included in the mock. Do you have any suggestions here to avoid code duplication?
Updated PR.
Attachment #8462799 -
Attachment is obsolete: true
Attachment #8463762 -
Flags: review?(anthony)
Comment 43•10 years ago
|
||
Comment on attachment 8463762 [details] [diff] [review]
Call on the last used SIM for this contact/number when hitting the Bluetooth call button.
Review of attachment 8463762 [details] [diff] [review]:
-----------------------------------------------------------------
::: apps/communications/dialer/js/dialer.js
@@ +379,5 @@
> if (result && (typeof result === 'object') && result.number) {
> + var request = navigator.mozSettings.createLock().get(
> + 'ril.telephony.defaultServiceId');
> + request.onsuccess = function() {
> + LazyLoader.load(['/shared/js/sim_settings_helper.js'], function() {
Sorry, my comment wasn't precise enough. We should load sim_settings_helper before and use SimSettingsHelper.getCardIndexFrom('outgoingCall', f() {}) instead of querying mozSettings directly.
::: apps/communications/dialer/test/unit/dialer_test.js
@@ +494,5 @@
> + var getStub;
> + var callSpy;
> +
> + setup(function() {
> + getStub = this.sinon.stub(MockCallLogDBManager, 'getGroupAtPosition');
getStub is hard to read later, please use a more meaningful variable name.
@@ +506,5 @@
>
> + suite('> Dialing the last recent entry', function() {
> + setup(function() {
> + sendCommand('BLDN');
> + sinon.assert.calledWith(getStub, 1, 'lastEntryDate', true);
Move this assert in a test.
@@ +510,5 @@
> + sinon.assert.calledWith(getStub, 1, 'lastEntryDate', true);
> + });
> +
> + [0, 1].forEach(function(serviceId) {
> + test('should dial on correct SIM ' + serviceId, function(done) {
'should dial on user preferred SIM n'
@@ +524,3 @@
>
> + [0, 1].forEach(function(serviceId) {
> + test('should use last serviceId ' + serviceId + ' if set always ask',
'should use serviceId (n) of last call'
@@ +542,5 @@
> + sinon.assert.calledWith(getStub, 3, 'lastEntryDate', true);
> + });
> +
> + [0, 1].forEach(function(serviceId) {
> + test('should dial on correct SIM ' + serviceId, function(done) {
'should dial on user preferred SIM n'
@@ +556,3 @@
>
> + [0, 1].forEach(function(serviceId) {
> + test('should use last serviceId ' + serviceId + ' when always ask',
'should use serviceId (n) of 3rd last call'
Attachment #8463762 -
Flags: review?(anthony) → review-
Assignee | ||
Comment 44•10 years ago
|
||
Updated PR.
Attachment #8463762 -
Attachment is obsolete: true
Attachment #8464130 -
Flags: review?(anthony)
Comment 45•10 years ago
|
||
Comment on attachment 8464130 [details] [diff] [review]
Call on the last used SIM for this contact/number when hitting the Bluetooth call button.
Review of attachment 8464130 [details] [diff] [review]:
-----------------------------------------------------------------
I forgot to catch this earlier: please remove all the setTimeout, they are not needed. To do before landing.
Attachment #8464130 -
Flags: review?(anthony) → review+
Assignee | ||
Comment 46•10 years ago
|
||
(In reply to Anthony Ricaud (:rik) from comment #45)
> I forgot to catch this earlier: please remove all the setTimeout, they are
> not needed. To do before landing.
Thanks, nice catch.
https://github.com/mozilla-b2g/gaia/commit/5e28fb653ee50f749cf450decc9d75c2ee5a02d1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 47•10 years ago
|
||
[Blocking Requested - why for this release]:
blocking-b2g: backlog → 1.4?
Reporter | ||
Comment 48•10 years ago
|
||
[Blocking Requested - why for this release]:
I think v1.4 also need this patch.
Can it be merged into v1.4?
Thanks.
Comment 49•10 years ago
|
||
Hi Doug,if no high risk or other concern on 1.4,Could you please kindly help provide 1.4 rebase patch for this? Thanks!
Flags: needinfo?(drs+bugzilla)
Updated•10 years ago
|
blocking-b2g: 1.4? → 1.4+
Comment 50•10 years ago
|
||
v1.4: https://github.com/mozilla-b2g/gaia/commit/3feb37ee2ed2319c9e556728723a5517dc1663ea
Note that per the recent policy change, you will need to explicitly request v2.0 approval if you want it to land there as well.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(drs+bugzilla)
Updated•10 years ago
|
Comment 51•10 years ago
|
||
Needs to be covered by new test case.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(srapanan)
Comment 52•10 years ago
|
||
Test case added in moztrap:
https://moztrap.mozilla.org/manage/case/14349/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(srapanan)
Flags: in-moztrap+
Updated•10 years ago
|
Updated•10 years ago
|
blocking-b2g: 2.0M? → 2.0M+
Comment 53•10 years ago
|
||
Comment 54•10 years ago
|
||
This problem is verified not to happen on Woodduck2.0 and Flame 2.1.
See attachment: Verify_video.MP4
Occurrence rate: 0/5
According to the Comment#37, we have verified successfully with another case:
When the outgoing call SIM is set to SIM 1 (or SIM2), double click bluetooth headset button, it will use SIM1 (or SIM2) and call the last-called contact successfully.
Woodduck 2.0 build:
Gaia-Rev afa87cffbd3cd9e2070b26d45dd556a9324bd4d5
Gecko-Rev 911e6cd6aecf8d37d42c203e162847b78a68a8d8
Build-ID 20141224050313
Version 32.0
Flame 2.1 build:
Gaia-Rev 17c7ad2e4919a994f0844239b483116090412dee
Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/39dfb662c82a
Build-ID 20141223001203
Version 34.0
Updated•10 years ago
|
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•