Closed Bug 964653 (system-dialog) Opened 10 years ago Closed 10 years ago

[Window Management] Implement System Dialog Window (for Dialog Overlay, Bluetooth Pairing Dialog)

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::System::Window Mgmt
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: iliu, Assigned: iliu)

References

Details

(Whiteboard: in-bubble-tea)

Attachments

(1 file)

pull request 16282
46 bytes, text/x-github-pull-request
alive
: review+
alive
: feedback+
Details | Review 
This is a refactor work for system dialog.
Alias: system-dialog
Summary: (system-dialog-window) [Window Management] Implement System Dialog Window (for Dialog Overlay, Bluetooth Pairing Dialog) → [Window Management] Implement System Dialog Window (for Dialog Overlay, Bluetooth Pairing Dialog)
Blocks: 859168
Attached file pull request 16282 — 
This patch including of BluetoothPairingDialog. I will remove it to bug 859168. Please ignore this section.

Alive, could you please help to have an architecture review in this feedback patch? Thanks.
Attachment #8376096 - Flags: feedback?(alive)
Comment on attachment 8376096 [details] [review]
pull request 16282

f+ for only SystemDialog part.
Attachment #8376096 - Flags: feedback?(alive) → feedback+
Comment on attachment 8376096 [details] [review]
pull request 16282

Alive, thanks for your guiding in the architecture. I have updated the patch. And add more comment in detail. Please help to review it, thanks. I will keep to add test later.

* Remove Bluetooth pairing dialog in this patch.
* Refactor system_simcard_dialog.
* Refactor fxa_dialog.
* Extends BaseUI for class `SystemDialog`.
* Add more comment for JSDoc.
* Revise loading sequence for these system dialogs.
Attachment #8376096 - Flags: review?(alive)
Comment on attachment 8376096 [details] [review]
pull request 16282

https://travis-ci.org/mozilla-b2g/gaia/builds/19084074

Please fix the error before sending review.
Attachment #8376096 - Flags: review?(alive)
Hi Julien,

May I ask you a question about jshint error "E:0200: Invalid JsDoc tag: requires"?(https://travis-ci.org/mozilla-b2g/gaia/jobs/19097447#L1087-L1088) 

I'm able to get the error while doing `git commit -m ...` in my local side. In order to fix the error, I try to revise it via `* @requires module:AppWindowManager` 
or remove all the comment above https://github.com/mozilla-b2g/gaia/pull/16282/files#diff-64f97262e745b967c87810a7dcb343edR27 
or other syntax.. 

Then, I still got the error. Not sure what's the error for the `tag: requires` here. I have no idea beside of remove the error tag.
Flags: needinfo?(felash)
This is actually a gjslint error, not a jshint error.

Therefore, I have 2 possible solutions for you:
1- fix the jshint issues in this file, remove it from build/jshint/xfail.list, so that it's not checked by the buggy gjslint anymore
2- add "requires" to this line, in the "custom jsdoc" parameter : https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L869

My preference would be 1. of course, but 2. is probably quicker ;)
Flags: needinfo?(felash)
Julien,

Your first solution is working for me. Once removed it from build/jshint/xfail.list, the file would be enabled to check the hint. Then, I don't receive the gjslint error again. Thanks for your command. It's really useful.:)
Comment on attachment 8376096 [details] [review]
pull request 16282

Alive, I fixed the error. And add unit test for these additional files. Could you please help to review my patch? Thanks.
Attachment #8376096 - Flags: review?(alive)
Comment on attachment 8376096 [details] [review]
pull request 16282

Nearly done, please see github comments, thanks.
Attachment #8376096 - Flags: review?(alive)
Comment on attachment 8376096 [details] [review]
pull request 16282

Alive, I have updated my patch with your comment. Please help to review it again. Once you agree with my patch, I'll revise the unit test again. Thanks.
Attachment #8376096 - Flags: review?(alive)
Comment on attachment 8376096 [details] [review]
pull request 16282

Some more nits. Last round needed.
Attachment #8376096 - Flags: review?(alive)
Comment on attachment 8376096 [details] [review]
pull request 16282

Revised some nits and leave comment on Github. 

Alive, could you please review it again? Thank you so much.
Attachment #8376096 - Flags: review?(alive)
Attachment #8376096 - Attachment description: A feedback patch for pr 16282 → pull request 16282
Comment on attachment 8376096 [details] [review]
pull request 16282

Well done! Thanks for your patience to complete it!

r+ but you have two failing unit tests.
Attachment #8376096 - Flags: review?(alive) → review+
Thanks for your reviewing effort with so much patience. I will revise unit test base on the latest patch. And ensure travis build pass for landing process.
Updated unit test. Also add test case in layout manager since we implemented new method "availableHeight()". Looks like unit test passed, but failed in non-relative marionette test(notification tests system replace notification).
Whiteboard: in-bubble-tea
Since it break unit test on bootstrap.js revert from gaia/bubble-tea: 353dbebe205b262211a7d832a3f4e6f546b9c0e7
Added and fixed bootstrap unit test. Merged in Gaia/bubble-tea: b77957794099e4af3f51f37bf6b73463cbf36413
Ian, though we plan to merge bubble-tea to master in order,
due to many conflict occurred in this patch, please help create a PR, check travis is green and land it to master. Thanks.
Flags: needinfo?(iliu)
Fred, thanks for your remind and effort. Looks like sim-pin dialog is revised before. I have solved the conflict. And wait all the unit tests passed.:)
Flags: needinfo?(iliu)
Since much conflict and unit test fail between other patch merged/revert, I spend much time to merge the pr in master. Finally, it's landed now.

Gaia/master: aa48f672f07fe18c590d31d6d19147193c0269ad
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
thanks!
Sorry, I accidentally committed some bad changesets to the Gaia repo which you had the misfortunate of pushing on top of. Rather than trying to rebase it all around, I ended up just reverting to the last good changeset, which means your push got lost. I'm *VERY* sorry for the inconvenience I've caused you :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #23)
> Sorry, I accidentally committed some bad changesets to the Gaia repo which
> you had the misfortunate of pushing on top of. Rather than trying to rebase
> it all around, I ended up just reverting to the last good changeset, which
> means your push got lost. I'm *VERY* sorry for the inconvenience I've caused
> you :(
Hi Ryan, could you please revert your patch? Because the commit log is not correct. There is no log about that 
9df14421065bcc5fcb94735f6ab7cb34d6ddc643
aa48f672f07fe18c590d31d6d19147193c0269ad
were reverted. If I create another pull request and merge the it again, the commit log are doubled. It will make it more mixed. How do you think my suggestion here? Thanks.
Flags: needinfo?(ryanvm)
Clean ni flag. I have created a pull request and merged it with Travis test passed.

Gaia/master:  09fe38e11a1979567838bfbadb5122af61ae8dcf
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Unfortunately, this had to be reverted for Marionette-webapi bustage. I can't say whether this would have happened on the original commit or not due to it landing on the other bustage then.
https://github.com/mozilla-b2g/gaia/commit/796624dc071d5318d7068627ccd2ff7d51436191

https://tbpl.mozilla.org/php/getParsedLog.php?id=36868084&tree=B2g-Inbound

04:48:30    ERROR -  errors.JavascriptException: JavascriptException: TypeError: this.states.activeDialog is null
04:48:30     INFO -  stacktrace:
04:48:30     INFO -  	@app://system.gaiamobile.org/js/system_dialog_manager.js, line 197
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
No longer blocks: 859168
Fixed error from TBPL(https://tbpl.mozilla.org/?tree=Try&rev=1dadff871a04). And Travis passed. We can merge the pr.

gaia/master:  fc9aee8c31a7c978319c3ee924947044b5414c9a
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Blocks: 971563
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: