William Hsu [:whsu] Reporter
	Description • 11 years ago

* 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

	Ivan Tsay (:ITsay)
	Comment 1 • 11 years ago

Need to fix it

	Preeti Raghunath(:Preeti)
	Comment 3 • 11 years ago

Arthur,

Any updates on this bug?

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 4 • 11 years ago

Preeti,

I plan to work on this bug later this week. Plan to land it by the end of the next week.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 5 • 11 years ago

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!

	Corey Frang [:gnarf]
	Comment 6 • 11 years ago

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.

	GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]
	Comment 7 • 11 years ago

Comment on attachment 817762 [details]
link to https://github.com/mozilla-b2g/gaia/pull/12885

works good on device and browser.

r=me, thanks.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 8 • 11 years ago

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?

	Corey Frang [:gnarf]
	Comment 9 • 11 years ago

(In reply to GaryChen [:GaryChen] from comment #7)
> r=me, thanks.

Without any unit tests?

	Corey Frang [:gnarf]
	Comment 10 • 11 years ago

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

	David Flanagan [:djf]
	Comment 11 • 11 years ago

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.

	Corey Frang [:gnarf]
	Comment 12 • 11 years ago

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.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 13 • 11 years ago

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

	Corey Frang [:gnarf]
	Comment 14 • 11 years ago

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.

	Corey Frang [:gnarf]
	Comment 15 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 16 • 11 years ago

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

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 17 • 11 years ago

Move to Settings component because after Bug 912010, this is mainly an optimization work on writing to mozSettings DB.

	Evelyn Hung
	Comment 18 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 19 • 11 years ago

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.

	Evelyn Hung
	Comment 20 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 21 • 11 years ago

master: 3d12a4704f20672267886caa85060ad0e1f58b4d

	William Hsu [:whsu] Reporter
	Comment 22 • 11 years ago

Needinfo "whsu" to verify this bug

	John Ford [:jhford] CET/CEST Berlin Time
	Comment 23 • 11 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 24 • 11 years ago

v1.2: 99f61082df3d89fa7d636561fc90938b62baaa6c

	William Hsu [:whsu] Reporter
	Comment 25 • 11 years ago

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.

	Ivan Tsay (:ITsay)
	Comment 26 • 11 years ago

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.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 27 • 11 years ago

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.

	William Hsu [:whsu] Reporter
	Comment 28 • 11 years ago

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.