Closed Bug 795255 Opened 7 years ago Closed 7 years ago

B2G RIL: improve error handling mechanism for 'setCallWaiting'

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: hsinyi, Assigned: hsinyi)

Details

Attachments

(2 files, 2 obsolete files)

Revise the current error handling mechanism to avoid endless setting.
Attached patch patch (obsolete) — Splinter Review
added an indication message 'setByRIL' when setting 'ril.callwaiting.enabled', so RIL knows the key was set internally and doesn't need to handle this.
Attachment #665851 - Flags: review?(philipp)
Attached patch Patch (v2) (obsolete) — Splinter Review
addressed nit.
Attachment #665851 - Attachment is obsolete: true
Attachment #665851 - Flags: review?(philipp)
Attachment #665855 - Flags: review?(philipp)
Comment on attachment 665855 [details] [diff] [review]
Patch (v2)

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +769,5 @@
>      if (!message.success) {
>        newStatus = !newStatus;
>        let lock = gSettingsService.createLock();
> +      lock.set("ril.callwaiting.enabled", newStatus, null, "setByRIL");
> +      this._callWaitingEnabled = newStatus;

How about pass rilRequestError up here and always set to false if the error code is 'unsupported'. That matches both the meaning of the settings value and the actual behavior. And it solves this issue, of course.
Comment on attachment 665855 [details] [diff] [review]
Patch (v2)

Vicamo's comment makes sense to me. Vicamo, can you do the review perhaps? Thanks!
Attachment #665855 - Flags: review?(philipp) → review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> Comment on attachment 665855 [details] [diff] [review]
> Patch (v2)
> 
> Review of attachment 665855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +769,5 @@
> >      if (!message.success) {
> >        newStatus = !newStatus;
> >        let lock = gSettingsService.createLock();
> > +      lock.set("ril.callwaiting.enabled", newStatus, null, "setByRIL");
> > +      this._callWaitingEnabled = newStatus;
> 
> How about pass rilRequestError up here and always set to false if the error
> code is 'unsupported'. That matches both the meaning of the settings value
> and the actual behavior. And it solves this issue, of course.

Nice idea, thanks. Will upload a new patch.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> Nice idea, thanks. Will upload a new patch.

I re-examined my idea last night again. To set false or to set true is the question! What value does it meet user's expectation most? If we always set it to false, it means no call waiting. But if the carrier enables this, the user might be charged unexpectedly. On the contrast, if we always set it to true, if the carrier enables this, good, that's expected; if carrier disables this, the call is dropped, still no a single penny is ever spent unexpectedly. The user has to re-dial again.

I'm not sure I fully understand the use cases and have to investigate more. Hsin-Yi, could you share some thoughts about this?
Vicamo, you understand the use case very well. That's it.

I am thinking of the possibility that maybe we set "ril.callwaiting.enabled" to "null" or "unknown" when rilRequestError happens. gaia pops out "Ask your operator for more info..." or something like that, and keep the value null until the user tries to set it (to true/false) next time. Per nsISettingsService, aValue is jsaval, so should be no problem with the implementation but needs some work in gaia. How do you think about this?

CC @etienne to see if he has comments from the view point of gaia impl.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> Vicamo, you understand the use case very well. That's it.
> 
> I am thinking of the possibility that maybe we set "ril.callwaiting.enabled"
> to "null" or "unknown" when rilRequestError happens. gaia pops out "Ask your
> operator for more info..." or something like that, and keep the value null
> until the user tries to set it (to true/false) next time. Per
> nsISettingsService, aValue is jsaval, so should be no problem with the
> implementation but needs some work in gaia. How do you think about this?
> 

More clear, in my imagination, no dialog pops out for the initial setting even an error happens (but, of course, the value needs to be "null" or "unknown" in this case). The warning is displayed only when a user tries to set ril.callwaiting.enabled but it fails.
 
> CC @etienne to see if he has comments from the view point of gaia impl.
Comment on attachment 665855 [details] [diff] [review]
Patch (v2)

Cancel the review first. I'll provide another one until we come out a better solution. Thanks :)
Attachment #665855 - Flags: review?(vyang)
when setting call waiting fails, we set 'ril.callwaiting.enabled' to null, neither 'true' nor 'false'. Settings app should carefully handle the value here. Please see comment 7 and comment 8 for my detailed thoughts.
Attachment #665855 - Attachment is obsolete: true
Attachment #666188 - Flags: feedback?(vyang)
Attachment #666188 - Flags: feedback?(etienne)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> Created attachment 666188 [details] [diff] [review]
> WIP: set null when error
> 
> when setting call waiting fails, we set 'ril.callwaiting.enabled' to null,
> neither 'true' nor 'false'. Settings app should carefully handle the value
> here. Please see comment 7 and comment 8 for my detailed thoughts.

I like the idea making the setting become a tri-state one, but now it may bring same trouble to Gaia because it can't tell an empty setting from that being reset to null on error. Gaia finds it empty -> set it to true or false -> error -> reset to null -> Gaia finds it empty -> ...
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> > Created attachment 666188 [details] [diff] [review]
> > WIP: set null when error
> > 
> > when setting call waiting fails, we set 'ril.callwaiting.enabled' to null,
> > neither 'true' nor 'false'. Settings app should carefully handle the value
> > here. Please see comment 7 and comment 8 for my detailed thoughts.
> 
> I like the idea making the setting become a tri-state one, but now it may
> bring same trouble to Gaia because it can't tell an empty setting from that
> being reset to null on error. Gaia finds it empty -> set it to true or false
> -> error -> reset to null -> Gaia finds it empty -> ...
The whole idea is "tri-state". We can use another meaningful value to represent error state. But I don't know why gaia will set it to true/false when it's empty...
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> > > Created attachment 666188 [details] [diff] [review]
> > > WIP: set null when error
> > > 
> > > when setting call waiting fails, we set 'ril.callwaiting.enabled' to null,
> > > neither 'true' nor 'false'. Settings app should carefully handle the value
> > > here. Please see comment 7 and comment 8 for my detailed thoughts.
> > 
> > I like the idea making the setting become a tri-state one, but now it may
> > bring same trouble to Gaia because it can't tell an empty setting from that
> > being reset to null on error. Gaia finds it empty -> set it to true or false
> > -> error -> reset to null -> Gaia finds it empty -> ...
> The whole idea is "tri-state". We can use another meaningful value to
> represent error state. But I don't know why gaia will set it to true/false
> when it's empty...
in my test, no loop happened as you mentioned.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #13)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #11)
> > > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> > > > Created attachment 666188 [details] [diff] [review]
> > > > WIP: set null when error
> > > > 
> > > > when setting call waiting fails, we set 'ril.callwaiting.enabled' to null,
> > > > neither 'true' nor 'false'. Settings app should carefully handle the value
> > > > here. Please see comment 7 and comment 8 for my detailed thoughts.
> > > 
> > > I like the idea making the setting become a tri-state one, but now it may
> > > bring same trouble to Gaia because it can't tell an empty setting from that
> > > being reset to null on error. Gaia finds it empty -> set it to true or false
> > > -> error -> reset to null -> Gaia finds it empty -> ...
> > The whole idea is "tri-state". We can use another meaningful value to
> > represent error state. But I don't know why gaia will set it to true/false
> > when it's empty...
> in my test, no loop happened as you mentioned.

The flow now is: 
initial: "ril.callwaiting.enabled: true" --> settings app reads the value from settings DB & app shows "true" & ril tries to enable call waiting --> error --> ril sets to null --> settings app is notified the value is changed to "null" & shows "null" --> the flow stops here unless a user sets the value. 

I think no matter what value we are going to use to replace "null",  app should let users understand that we don't have any info from operator.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> I think no matter what value we are going to use to replace "null",  app
> should let users understand that we don't have any info from operator.

Does it make sense to disable/gray out the control in the settings app when the value change to null?
I'm not sure to understand precisely in which cases it occurs...
(In reply to Etienne Segonzac from comment #15)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> > I think no matter what value we are going to use to replace "null",  app
> > should let users understand that we don't have any info from operator.
> 
> Does it make sense to disable/gray out the control in the settings app when
> the value change to null?
> I'm not sure to understand precisely in which cases it occurs...

The value of *ril.callwaiting.enabled* being true means that we ask the carrier to enable call waiting service, and it succeeds. So we are sure the service is enabled and we keep *ril.callwaiting.enabled* true. Similarly, *ril.callwaiting.enabled = false" means the service is disabled and we are sure about this, too.

But it may fail when we send the request to the carrier. In this case, there might be a problem with synchronizing the settings DB and the real carrier service status. We are unable to get the service status from the carrier, so I am planning to set 'ril.callwaiting.enabled' to null. I hope this avoids misleading users. 

So I think disable/gray out the control in the settings app isn't the case I expected. "null" doesn't mean we should disable the setting control, it does mean we cannot provide the operator status, but user can still try to set the key.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> The value of *ril.callwaiting.enabled* being true means that we ask the
> carrier to enable call waiting service, and it succeeds. So we are sure the
> service is enabled and we keep *ril.callwaiting.enabled* true. Similarly,
> *ril.callwaiting.enabled = false" means the service is disabled and we are
> sure about this, too.
> 
> But it may fail when we send the request to the carrier. In this case, there
> might be a problem with synchronizing the settings DB and the real carrier
> service status. We are unable to get the service status from the carrier, so
> I am planning to set 'ril.callwaiting.enabled' to null. I hope this avoids
> misleading users. 
> 
> So I think disable/gray out the control in the settings app isn't the case I
> expected. "null" doesn't mean we should disable the setting control, it does
> mean we cannot provide the operator status, but user can still try to set
> the key.

Thanks, this is really clear. I'll add Josh to the loop to get some UX insights on this.
setting call waiting is v1 feature.
blocking-basecamp: --- → ?
Blocking based on comment #18.
blocking-basecamp: ? → +
attached is a screenshot taken during lab test and has been confirmed a result of this issue. HsinYi's latest patch solved this problem.
Comment on attachment 666188 [details] [diff] [review]
WIP: set null when error

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

I have no further questions.
Attachment #666188 - Flags: feedback?(vyang)
I have asked Ayman to chime in here. He is UX owner on Dialer and SMS apps.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
> Created attachment 666669 [details]
> error dialog in testing 3G voice codecs
> 
> attached is a screenshot taken during lab test and has been confirmed a
> result of this issue. HsinYi's latest patch solved this problem.
The attachment is cool!
Attachment #666188 - Flags: feedback?(etienne) → review?(vyang)
(In reply to Josh Carpenter [:jcarpenter] from comment #22)
> I have asked Ayman to chime in here. He is UX owner on Dialer and SMS apps.

Josh, thanks! I would like to file a gaia issue and will invite you to join the discussion about ux and gaia app there. So, we can proceed the bug review process and land this platform bug first. :)
Comment on attachment 666188 [details] [diff] [review]
WIP: set null when error

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

Go! Go! Go!
Attachment #666188 - Flags: review?(vyang) → review+
https://hg.mozilla.org/mozilla-central/rev/da5df2c1ae16
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.