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

VERIFIED FIXED in Firefox OS v1.2

Status

Firefox OS
Gaia::Settings
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: whsu, Assigned: arthurcc)

Tracking

unspecified
1.2 C4(Nov8)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 fixed)

Details

(Whiteboard: [FT:System-Platform], [Sprint:3], [3rd-party-keyboard])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
blocking-b2g: --- → koi?
Whiteboard: [FT:System-Platform], [Sprint:3]
(Reporter)

Updated

4 years ago
Depends on: 891673

Comment 1

4 years ago
Need to fix it
Assignee: nobody → arthur.chen
blocking-b2g: koi? → koi+
Duplicate of this bug: 912034
Arthur,

Any updates on this bug?
Flags: needinfo?(arthur.chen)
Preeti,

I plan to work on this bug later this week. Plan to land it by the end of the next week.
Flags: needinfo?(arthur.chen)
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!
Attachment #817762 - Flags: review?(gchen)
Attachment #817762 - Flags: review?(ehung)

Comment 6

4 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.
Comment on attachment 817762 [details]
link to https://github.com/mozilla-b2g/gaia/pull/12885

works good on device and browser.

r=me, thanks.
Attachment #817762 - Flags: review?(gchen) → review+
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?
Flags: needinfo?(gnarf37)

Comment 9

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

Without any unit tests?
Flags: needinfo?(gnarf37)
(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.
Flags: needinfo?(gnarf37)
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.
Flags: needinfo?(gnarf37)
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. :)

Updated

4 years ago
Target Milestone: --- → 1.2 C4(Nov8)
Whiteboard: [FT:System-Platform], [Sprint:3] → [FT:System-Platform], [Sprint:3], [3rd-party-keyboard]
Move to Settings component because after Bug 912010, this is mainly an optimization work on writing to mozSettings DB.
Status: NEW → ASSIGNED
Component: Gaia::Keyboard → Gaia::Settings
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
Attachment #817762 - Flags: review?(ehung)
Attachment #817762 - Flags: review+
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.
Attachment #817762 - Flags: review?(ehung)
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.
Attachment #817762 - Flags: review?(ehung) → review+
master: 3d12a4704f20672267886caa85060ad0e1f58b4d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-b2g-v1.2: --- → affected
(Reporter)

Comment 22

4 years ago
Needinfo "whsu" to verify this bug
Flags: needinfo?(whsu)
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
Flags: needinfo?(arthur.chen)
v1.2: 99f61082df3d89fa7d636561fc90938b62baaa6c
status-b2g-v1.2: affected → fixed
Flags: needinfo?(arthur.chen)
(Reporter)

Comment 25

4 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.
Status: RESOLVED → REOPENED
Flags: needinfo?(whsu)
Resolution: FIXED → ---

Comment 26

4 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.
Flags: needinfo?(arthur.chen)
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.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: needinfo?(arthur.chen)
Resolution: --- → FIXED
(Reporter)

Comment 28

4 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.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

3 years ago
Summary: [Keyboard][V1.2] The enabled keyboard menu cannot synchronous display enabled keyboards → [Keyboard][V1.2] The enabled keyboard menu cannot synchronously display enabled keyboards
You need to log in before you can comment on or make changes to this bug.