Created attachment 798839 [details] Enabled keyboard menu * Description: This problem happens on v1.2 build(M-C). If you quick select/unselect keyboards then go back to enabled keyboard menu, you sometimes cannot see the enabled keyboards on the list. Attaching the screenshot. * Reproduction steps: 1. Launch the keyboard settings app 2. Tap "Active keyboard" 3. Tap "Add more keyboards" 4. Select multiple keyboard layouts 5. Unselect multiple keyboard layouts 6. Select multiple keyboard layouts 7. Go back to enabled keyboard list * Expected result: The menu displays all enabled keyboards * Actual result: The menu cannot display all enabled keyboards * Reproduction build:(Mozilla-central-unagi-eng/2013-09-02-04-02-02) + Mercurial-Information - Gecko revision="0457c197a382a219b1050fd2ccfddfdbbdad7a92" + Git-information - Gaia revision="1179318fb5aa" + Gecko version: 26.0a1
Need to fix it
Arthur, Any updates on this bug?
Preeti, I plan to work on this bug later this week. Plan to land it by the end of the next week.
Created attachment 817762 [details] link to https://github.com/mozilla-b2g/gaia/pull/12885 The patch solves two problems: 1. Out of sync problem. 2. The original implementation enables a large number of DB access, which messes up DB and drag down system performance. Evelyn, could you help review settings app part? Gary, could you help review keyboard helper part? Thanks!
The out of sync problem is also being solved by my bug 912010 proposed fix currently. I would definitely appreciate some numbers regarding the "large number of DB access" affecting performance here. I think this might actually just be solved if/when the code from my patch for 912010 lands.
Comment on attachment 817762 [details] link to https://github.com/mozilla-b2g/gaia/pull/12885 works good on device and browser. r=me, thanks.
Corey, thank you for taking care of bug 912010 and I can see a lot of effort in it. However, as we are in the bug fixing stage and all of the existing code has been tested for a period of time, it might not be a good timing to submit a huge patch without strong reasons. Is it possible to create separate bugs for refactoring and provide patches that focus on the issues of the bugs?
(In reply to GaryChen [:GaryChen] from comment #7) > r=me, thanks. Without any unit tests?
(In reply to Arthur Chen [:arthurcc] from comment #8) > Corey, thank you for taking care of bug 912010 and I can see a lot of effort > in it. However, as we are in the bug fixing stage and all of the existing > code has been tested for a period of time, it might not be a good timing to > submit a huge patch without strong reasons. Is it possible to create > separate bugs for refactoring and provide patches that focus on the issues > of the bugs? There are strong reasons... KeyboardHelper and all the code that deals with the settings is a huge minefiled because of how it has been designed. I have been talking about this strategy with :djf for a bit now, we wanted to get the refactoring of KeyboardHelper in.
If Corey's larger patch for bug 912010 also fixes this bug, please consider landing his code instead of the patch here, since that will save a lot of rebasing work for Corey.
I'd really like to see the fixes you put here for reducing calls to save applied after 912010, any chance you could help me with the rebase of this change either way? We fixed things very similarly inside the actual settings app so it would be easy enough for me to undo/redo those changes, its all the saving optimization and code in keyboard_helper that i'm really worried about finding a clean path to rebase. Could you help me play this rebase/merge conflict out? One nice thing about letting the patch for 912010 land first is that there will be units covering keyboard_helper.js so we can ensure these optimizations to saves work correctly.
(In reply to David Flanagan [:djf] from comment #11) > If Corey's larger patch for bug 912010 also fixes this bug, please consider > landing his code instead of the patch here, since that will save a lot of > rebasing work for Corey. Yes, however the risk of uplifting a large patch to v1.2 branch is a fair point to. If Corey is sure he can be responsible and available 100% of the time before koi+ code freeze, we could surely take that risk. If not, rebasing Corey's patch is probably a safer and a easier solution for Corey and anyone else.
Tim: If this patch was only solving the settings display issue, the rebase for me is simple. The problem is it also dives into keyboard_helper.js quite a bit to "save round trips to the database" and that whole file basically got changed by my patch. Lots of code moved around, etc. I do really want to see these optimizations also make it onto my branch, if arthur can help me understand exactly what was changed, or can just write a commit on top of my 912010 branch and send it my way, I will do my best to make sure to incorporate that part into my 912010 keyboard_helper rewrite, and this patch could be simplified to only fix the "sync display" problem. Only the code in keyboard.js should need to change. Also, w/r/t being around until the code freeze, I don't think that will be a problem at all - I'm more than happy to solve any bugs created by my patch.
FYI - I also submitted bug 928427 which would make this bug not exist anymore. The "Selected Keyboards" screen seems pretty useless to me, and a decent amount of code could just be removed to get rid of it.
Corey, we are now reviewing your proposed patch. And don't worry, the patch here is not going to be merged until we finish the review. :)
Move to Settings component because after Bug 912010, this is mainly an optimization work on writing to mozSettings DB.
Comment on attachment 817762 [details] link to https://github.com/mozilla-b2g/gaia/pull/12885 Cancel review request because we need some rebase work after bug 912010
Comment on attachment 817762 [details] link to https://github.com/mozilla-b2g/gaia/pull/12885 Evelyn. could you help review it again? Thanks. After applying the patch, it only saves to DB when leaving the panel and refresh the UI when entering the panel.
Comment on attachment 817762 [details] link to https://github.com/mozilla-b2g/gaia/pull/12885 r+, but the checkbox UI is slower than expected (I suspect there is an gecko issue). The patch works under normal case and assumes the user won't crazily tap these layouts on/off.
Needinfo "whsu" to verify this bug
I was not able to uplift this bug to v1.2. If this bug has dependencies which are not marked in this bug, please comment on this bug. If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval. Otherwise, if this is just a merge conflict, you might be able to resolve it with: git checkout v1.2 git cherry-pick -x -m1 3d12a4704f20672267886caa85060ad0e1f58b4d <RESOLVE MERGE CONFLICTS> git commit
Weird. I still can reproduce this bug on V1.2 build. As I discussed this bug with Arthur, this may be a Gecko issue since Arthur cannot reproduce this bug on V1.3-Gecko & v1.2 Gaia. * Test Build: - Gaia: be4ea00a50236b10eb0a03232a28ffd0048e0cb8 - Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3ba912717904 - BuildID 20131105004003 - Version 26.0 A easy way to reproduce this case 1. Launch the settings app 2. Tap "Keyboards" -> "Selected Keyboards" -> "Add more keyboards" 3. Quickly tap an item twice 4. Tap arrow icon "<" to go back to previous page. 5. Check the selected keyboard whether it can be added.
Arthur, If this bug might be related to Gecko, Would you work with gecko engineer to figure out what's happening here. Let me know if you need my help.
This bug shall remain closed because we are not back out any code here. Arthur will talk to Gecko developers to identify what is in m-c is need and should go to v1.2 for this bug not to occur.
Hi, all, Thanks for Arthur's help. After test cross, we found the 11/05 build can reproduce this bug. But 11/06 build fix this problem. I would like to close bug. Thanks! * Test Build: - Gaia: 2140c987fdde1c99097018f7e93b0bbd43d2125d - Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/6a831fcb96f4 - BuildID 20131106004004 - Version 26.0 => Cannot reproduce.