Closed Bug 1047837 Opened 7 years ago Closed 7 years ago

Allow each layout define more symbol pages and the relations for switching

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S5 (26sep)

People

(Reporter: rudyl, Assigned: timdream)

References

Details

(Whiteboard: [p=5])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1046061 +++

[Background]
with bug 1046061 comment 0, we would like to define a generic way to handle layout page switching, especially for the case that there are more than 2 symbol pages, e.g. pinyin input method.
However, I tried to land a partial fix for this problem to unlock other layout related issues.

[Intent to implement]
This bug should be implement the following things, 
 1. To allow each layout define several layout page (or sub-layout), not only alternateLayout and symbolLayout.
    (The above sub layouts should be loaded by layout loader module.)
 2. Can specify a key which is capable of switching between these sub layouts.
    e.g. { value: 'Alt', pageSwitching: 'sub-layout2'}.
    With this, we could probably make layoutManager.currentLayoutPage as a string instead of ENUM.
Taking as way to move forward to implement Emoji keyboard.
Assignee: nobody → timdream
Blocks: 993899
Status: NEW → ASSIGNED
This patch do the following:

-- Officially separate the idea of layout and layout page, where pages are put into an array under "pages" property of the layout
-- A normalize step in the loader will try to create the 0th page from properties of the layout (that should really go to the page under the new concept)
-- alternateLayout is now the 1st page in the array
-- symbolLayout is now the 2nd page in the array
-- forced layout is removed. Zhuyin, Pinyin, and Japanese layouts now use the 3rd and 4th page of the layout definition.
-- All switching keys now comes with keyCode=ALT, with a "targetPage" property to tell the which page it is going to.
-- currentLayoutPage is now currentPageIndex.
-- currentModifiedLayout is now currentPage (and expose the information of the page, not the layout)
-- however, some layout information is copy over from layout to the page.
-- currentLayout should not be used by others and is removed.

Apologies for not being able to split the patch into nicer chunks. I wasn't sure the scope of this patch until I actually work on it. I have a few bugs already filed that is actually done in this patch -- I will dup them over here.
Attachment #8487698 - Flags: review?(rlu)
Attachment #8487698 - Flags: feedback?(jlu)
Duplicate of this bug: 1046001
Duplicate of this bug: 1044743
Comment on attachment 8487698 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/23936

Overall this great work looks good to me! (Y).
What I'm a little bit concerned about is the use of |undefined| in place of |pages[0]| in the majority of the layout definition files, which may seem little self-explanatory for other people who desire to add new layouts in the future, without studying your patch or any comments in the layout definition file. We could add comments there, but maybe another property, with a descriptive name, in the definition file would be more robust.

Also some keyboard tests in Gip are failing, and you might want to check if your patch has not broke gaiatest/apps/keyboard/app.py -- there is a function called |_switch_to_correct_layout| that could smell fishy.

> -- All switching keys now comes with keyCode=ALT, with a "targetPage"
> property to tell the which page it is going to.

Yappie! I was going to normalize that in bug 1044525. Glad I haven't touched that change.
Attachment #8487698 - Flags: feedback?(jlu) → feedback+
keyboard/app.py updated.
Comment on attachment 8487698 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/23936

Zac, would you might checking the keyboard/app.py changes? I updated the DOM of switching keys so I update the test accordingly.
Attachment #8487698 - Flags: review?(zcampbell)
Comment on attachment 8487698 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/23936

r+, tested on device and it's good.
Attachment #8487698 - Flags: review?(zcampbell) → review+
Comment on attachment 8487698 [details] [review]
Github: https://github.com/mozilla-b2g/gaia/pull/23936

Looks good to me as well.
Thanks for taking over this change for me.
Attachment #8487698 - Flags: review?(rlu) → review+
master: https://github.com/mozilla-b2g/gaia/commit/2f5a1aff6b39f6e8e03e3a69c05d57d5b0987409
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [p=5]
Target Milestone: --- → 2.1 S5 (26sep)
You need to log in before you can comment on or make changes to this bug.