1032070 - [Settings] add done feature in onBeforsShow()

Status: RESOLVED FIXED

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

Description

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

In order to make panel decide whether onBeforeShow process is done or not, we have to add one more parameter called `done` for this feature.

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8447878 [details] [review]
patch on master

Arthur,

this is the quick prototype of the patch. Please give me some feedbacks to make sure we are thinking at the same way !

Thanks :]

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8447878 [details] [review]
patch on master

Thank you for the quick prototyping! As we discussed yesterday, I did not aware that we couldn't simply modify Panel to support this and which also means that any panel extending from Panel will have to do the implementation by itself.

Maybe we could simply make onBeforeShow in SettingsPanel return the result of options.onBeforeShow (we could also done this to other functions able to be overrided) and all the way back to Panel and SettingsService. And then we can check if the result is a promise (result.constructor === Promise) and handle it.

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Arthur, how about this approach ? 

https://github.com/mozilla-b2g/gaia/pull/21259

As you can see, each onXYZ function will be returned with a promise object. (In panel.js & settings_panel.js)

This part of code works well in our use case, and you can try it on battery_panel. All you have to do is fix the parameters of onBeforeShow in battery_panel and then use a setTimeout to call done().

By the way, I also cover the use case when users click on home button to exit (and enter on contrary). Please give it a check if you have time !

Any feedback / advices would be appreciated. Thanks ARthur !

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Already changed the code based on our offline discussion, Arthur. But there is one thing I have to remind you is we can't protect Settings with timer to make sure developers did call promise.resolve() during 10 ~ 15 seconds.

This is mainly because : 

1. Promise doesn't provide isResolved() method 
2. We rely on each panel's promise (we can't control it compared with done() mechanism we provide in previous patch)

Please give it a try, thanks Arthur :)

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Good work, EJ! It seems that after the patch applied `SettingsService` no longer has control over the navigation process. I think we could check the time when calling to these async functions in SettingsService and throw an error if the time exceeds a threshold, then we are able to know which panel takes longer times than expected.

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Arthur,

if this is ok, I would start to write tests.

Thanks :]

Comment 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

Per the offline discussion, cancel the ni? first. :)

Comment 9

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Arthur, I found the problem and fixed it ! Right now we can easily understand the calling sequences ! Would you mind to take a look again ? If there is no big problem, I think I can start to write tests.

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

Overall looks good to me, thanks! I left some comments in github, please check them.

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8447878 [details] [review]
patch on master

Arthur, I just addressed all comments and I think this patch is ready to go.

Can you give it a check when you have time ? Thanks !!

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8447878 [details] [review]
patch on master

Thanks for the patch! There are a few comment to be addressed before merging. Please check them in github.

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8447878 [details] [review]
patch on master

Arthur, just addressed all your comments, please give a check ! Thanks :)

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8447878 [details] [review]
patch on master

r=me with the comment addressed. We should also use 'done' in the tests when using promises. Thanks.

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all,

landed on Gaia/master : 2e1a4507e356f32ae8693ebf56fb72d7f3d36189