912020 - [Keyboard][V1.2] The enabled keyboard menu cannot synchronously display enabled keyboards

Status: VERIFIED FIXED

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

Description

[William Hsu [:whsu]]

Attachment: 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

Comment 1

[Ivan Tsay (:ITsay)]

Need to fix it

Comment 3

[Preeti Raghunath(:Preeti)]

Arthur,

Any updates on this bug?

Comment 4

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

Preeti,

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

Comment 5

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

Attachment: 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!

Comment 6

[Corey Frang [:gnarf]]

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 7

[GaryChen [:GaryChen][:PYChen][:gchen][:陳柏宇]]

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

works good on device and browser.

r=me, thanks.

Comment 8

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

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?

Comment 9

[Corey Frang [:gnarf]]

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

Without any unit tests?

Comment 10

[Corey Frang [:gnarf]]

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

Comment 11

[David Flanagan [:djf]]

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.

Comment 12

[Corey Frang [:gnarf]]

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.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 14

[Corey Frang [:gnarf]]

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.

Comment 15

[Corey Frang [:gnarf]]

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.

Comment 16

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

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

Comment 17

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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

Comment 18

[Evelyn Hung]

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 19

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

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 20

[Evelyn Hung]

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.

Comment 21

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

master: 3d12a4704f20672267886caa85060ad0e1f58b4d

Comment 22

[William Hsu [:whsu]]

Needinfo "whsu" to verify this bug

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 24

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

v1.2: 99f61082df3d89fa7d636561fc90938b62baaa6c

Comment 25

[William Hsu [:whsu]]

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.

Comment 26

[Ivan Tsay (:ITsay)]

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.

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 28

[William Hsu [:whsu]]

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.