795255 - B2G RIL: improve error handling mechanism for 'setCallWaiting'

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Revise the current error handling mechanism to avoid endless setting.

Comment 1

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

Attachment: patch

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.

Comment 2

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

Attachment: Patch (v2)

addressed nit.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

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 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665855 [details] [diff] [review]
Patch (v2)

Vicamo's comment makes sense to me. Vicamo, can you do the review perhaps? Thanks!

Comment 5

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

(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.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(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?

Comment 7

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

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.

Comment 8

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

(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 9

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

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 :)

Comment 10

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

Attachment: 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.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(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 -> ...

Comment 12

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

(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...

Comment 13

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

(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.

Comment 14

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

(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.

Comment 15

[Etienne Segonzac (:etienne)]

(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...

Comment 16

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

(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.

Comment 17

[Etienne Segonzac (:etienne)]

(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.

Comment 18

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

setting call waiting is v1 feature.

Comment 19

[Andrew Overholt [:overholt]]

Blocking based on comment #18.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: 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.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 666188 [details] [diff] [review]
WIP: set null when error

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

I have no further questions.

Comment 22

[Josh Carpenter [:jcarpenter]]

I have asked Ayman to chime in here. He is UX owner on Dialer and SMS apps.

Comment 23

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

(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!

Comment 24

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

(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 25

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

gaia issue https://github.com/mozilla-b2g/gaia/issues/5611

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 666188 [details] [diff] [review]
WIP: set null when error

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

Go! Go! Go!

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/da5df2c1ae16

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/da5df2c1ae16