994495 - Solve various problems of Korean keyboard under FxOS 1.4+

Status: RESOLVED FIXED

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

Description

[Jinkyu Yi[:jincreator]]

Attachment: ko_keyboard_fx1.4.patch

Problem:
* Always show uppercase layout.
* Unable to type letter contains 쌍자음
* last typing letter lost when deactivate/switching to other layout.

Applying patch below will...
* Show secondLayout
* Use deactivate() and remove empty()
* Commit typing letter and reset ime at deactivate()
* Use resetUppercase() //any better way to reset Shift?

Comment 1

[Jinkyu Yi[:jincreator]]

Attachment: Github Pull Request

Comment 2

[Channy Yun [:channy]]

Tim, Jinkyu updated some of codes for changes in keyboard layout. I tested it today on fxdevconkr. It can be goes 1.4 branch?

Comment 3

[Jean Gong :jgong]

Bruce,  can you look into this if it should be part of v1.4?

Comment 4

[Jinkyu Yi[:jincreator]]

Attachment: GitHub Pull Request for v1.4

Comment 5

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

I shouldn't be the assignee. Please simply set r? or ni? me next time.

To land one 1.4 we should get an approval after landing to master.
https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.4.0

Comment 6

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

Comment on attachment 8404467 [details] [review]
Github Pull Request

I will review this later today or Monday.

Comment 7

[Bruce Huang [:bhuang] (inactive)]

Not a blocker for 1.4 as Korean isn't a target language.  We can keep the implementation on master.

Comment 8

[Channy Yun [:channy]]

(In reply to Bruce Huang [:bhuang] <[e-mail]> from comment #7)
> Not a blocker for 1.4 as Korean isn't a target language.  We can keep the
> implementation on master.

Actually I got a email from Delphine [e-mail] on April 3th. 

> * you still have unlocalized strings on gaia-l10n/. Please catch up asap since:
> * l10n testing starts in less then 2 weeks, April 14 to be exact. And what's more:
> * we *just* learned that there are new Camera features landing, which breaks our string freeze. 
> This entails 24 new strings landing and that will be exposed very shortly. 
> Please be on the loook-out for these, as these will be key features in the v1.4 release

It meant ko locale can be blocker for 1.4? I think if you have a plan to sell new devices with open channel in Asia this year, ko locale must be included.

Comment 9

[Delphine Lebédel [:delphine - Use needinfo]]

I confirm this is not a blocker for 1.4. 
We are asking all locales to catch up with l10n work as our goal is to have as many locales ready as possible, in order to be able to propose them. The best way for a locale to get included in future releases is for them to be ready to ship.
Channy: am writing an email to you now to further explain this.
So again, this is not a blocker for 1.4. Having this on master is fine

Comment 10

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

Comment on attachment 8404467 [details] [review]
Github Pull Request

The code looks good. However to make it acceptable please add the missing unit tests for the entire jshangul.js.

Please re-request for review after adding the test cases. You can learn how to run and write unit tests in
https://github.com/mozilla-b2g/gaia#unit-tests
https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

Thank you very much!

Comment 11

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

Can we simply do approval‑gaia‑v1.4 to get this land on 1.4? The ko keyboard is not built by default anyway.

I am reading conflicting e-mails about that so needinfo'ing you both.

Comment 12

[Channy Yun [:channy]]

Sangphil and Jinkyu, please check out Tim's message and add test cases too. Your efforts can be added in master in now.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #11)
> Can we simply do approval‑gaia‑v1.4 to get this land on 1.4? The ko keyboard
> is not built by default anyway.
> 
> I am reading conflicting e-mails about that so needinfo'ing you both.

Yup, that's fine.

Comment 14

[Preeti Raghunath(:Preeti)]

Tim

I'd be fine with that.

Comment 15

[Channy Yun [:channy]]

Hi, all. This patch merged to master? Please let me know the status.

Comment 16

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

(In reply to Channy Yun [:channy] from comment #15)
> Hi, all. This patch merged to master? Please let me know the status.

We are still waiting for the unit test to be written.

Comment 18

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

Channy, could you locate Jinkyu and see what's the status of the patch? If there are specific problems on writing unit tests, I can certainly help personally (or find someone else to).

Comment 19

[Channy Yun [:channy]]

Jinkyu or especially Sanphil, 

would you make unite tests for Korean keyboard for Tim?
He will help you.

Comment 20

[BYUN Sangpil]

Sorry to late response.
I will do it!
Please wait a few days.

Comment 21

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

Comment on attachment 8404467 [details] [review]
Github Pull Request

This is taking far too long so I think it make sense to land the smallish patch now and create a unit test in a follow up.

Comment 22

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

master: https://github.com/mozilla-b2g/gaia/commit/da11dba3895cc20bda2f98f8b6145f9d3417859e

Comment 23

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

Bug about unit test filed as bug 1052243.

Comment 24

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

Follow-up pushed on master to fix incomplete rebase: 254ecd807e539eee0b9a0af409b2f30cbb687fa5

Comment 25

[Sue]

Could you provide the detailed reproduce steps and video for me to verify this bug, thanks.

Comment 26

[Jinkyu Yi[:jincreator]]

This bug was fixed few month ago, so it should be unable to reproduce.

Comment 27

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

(In reply to Jinkyu Yi[:jincreator] from comment #26)
> This bug was fixed few month ago, so it should be unable to reproduce.

Jinkyu, Sue is looking for an STR for them to verify the patch indeed contain the valid fixes. It's part of the QA process. If you could provide that STR it would be great.