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 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 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.
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 !
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 :)
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.
Arthur, if this is ok, I would start to write tests. Thanks :]
Per the offline discussion, cancel the ni? first. :)
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.
Overall looks good to me, thanks! I left some comments in github, please check them.
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 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 on attachment 8447878 [details] [review] patch on master Arthur, just addressed all your comments, please give a check ! Thanks :)
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.
Attachment #8447878 - Flags: review?(arthur.chen) → review+
Thanks all, landed on Gaia/master : 2e1a4507e356f32ae8693ebf56fb72d7f3d36189
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.