Closed Bug 1046061 Opened 8 years ago Closed 8 years ago

Simplify the layout page switching of pinyin

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S2 (15aug)
Tracking Status
b2g-v2.1 --- fixed

People

(Reporter: rudyl, Assigned: rudyl)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
timdream
: review+
lchang
: feedback+
Details | Review
+++ This bug was initially created as a clone of Bug #1020779 +++

When dealing with Bug 1020779, we found that it is difficult to handle multiple symbol pages defined for pinyin input method.

 1. It defines them as separate layouts, and switch to those layouts by the logic in jspinyin IME Engine.
 2. In its layout definition, we could see several keys/keycodes defined for page switching.
    However, in Bug #1020779, we would like to adjust the size of these page switching key, as those keys are generated with js code for other layouts.

It would be better we could have an alternative way to handle these symbol pages with general keyboard code, instead of letting IME engine handle this, so that we don't have to do this for each IME engine.
Attached file WIP (obsolete) —
This is an attempt to address this issue, by adding "pageSwitching" to a key, so that it could specify which page it could switch to.

With this, we could define as many symbol pages as we want, and specify the relations between these pages with only "pageSwitching" property.

Tim, could you help take a look if this is a proper way for this issue?
Thanks.
Attachment #8464616 - Flags: feedback?(timdream)
Comment on attachment 8464616 [details] [review]
WIP

We cannot allow layout to define arbitrary sub-layout names because the loader will not properly process them. You reviewed the code, you should know that.

https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/layout_loader.js#L150-L155

I also don't think it make sense to allow layout/IMEngine to specify arbitrary string to currentLayoutPage. Page is a emum value, for very important reasons.

I recommend we tackle the use case of Pinyin IME by keep using the same API, but move the 'zh-Hans-Pinyin-Symbol-Ch-1' to alternateLayout and 'zh-Hans-Pinyin-Symbol-Ch-2' to symbolLayout. 'zh-Hans-Pinyin-Symbol-En-1' and 'zh-Hans-Pinyin-Symbol-En-2' can remain switch-able with forced layout name, with 全/半 button.

You can invent new key code for each of the 全/半 buttons to specify the target layout/age independently.
Attachment #8464616 - Flags: feedback?(timdream) → feedback-
Assignee: nobody → rlu
Status: NEW → ASSIGNED
Whiteboard: [p=1]
Target Milestone: --- → 2.1 S1 (1aug)
Attached file Patch V1
Patch updated as discussed.
Please note that I have a second commit so that jspinyin won't need to know which page it is in to send the correct keyCode.

Thanks.
Attachment #8464616 - Attachment is obsolete: true
Attachment #8465276 - Flags: review?(timdream)
Comment on attachment 8465276 [details] [review]
Patch V1

Discussed offline.
Attachment #8465276 - Flags: review?(timdream)
Comment on attachment 8465276 [details] [review]
Patch V1

Tim,

As offline discussion, please help consider to take patch first, so that we could unblock other layout_manger related work.
And I'll open another a have a proper fix for layout switching.

Luke,

Could you please give your feedback on the changes made to jspinyin, I just removed some logic so that jspinyin won't need to know which layout page it is in, and use a special keycode to identify "'" used for pinyin composition.

Thanks.
Attachment #8465276 - Flags: review?(timdream)
Attachment #8465276 - Flags: feedback?(lchang)
Comment on attachment 8465276 [details] [review]
Patch V1

Let's file another bug to document the proper approach of this bug. Thanks for the patch.
Attachment #8465276 - Flags: review?(timdream) → review+
Summary: Allow each layout define more symbol pages and the relations for switching → Simplify the layout page switching of pinyin
Comment on attachment 8465276 [details] [review]
Patch V1

Hi Rudy,

It looks great! Really thanks for improving pinyin input method and making its architecture more consistent with others.
Attachment #8465276 - Flags: feedback?(lchang) → feedback+
Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/df462661aac633b20ba8c7310c55135a90e552e9

--
Tim, Luke,

Thanks for the review.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Depends on: 1048682
You need to log in before you can comment on or make changes to this bug.