Merge lp://staging/~cosmos-door/ubuntu-keyboard/japanese-keyboard-rebooted into lp://staging/ubuntu-keyboard

Hi Mitsuya,

 Thanks for updating this, I've only just started reviewing it so there'll be some more comments to come later, but the most immediate issue with this branch is that it doesn't install the japanese plugin's "keys" directory (plugins/ja/qml/keys).

 I've also made a couple of inline comments at locations where it's still using the old relative path imports for the global keys directory, this should now be done as "import keys 1.0" as this we allow the plugin to be installed separately from the rest of maliit in the future (e.g. as part of a custom tarball, or possibly as a click package at some point)

Thanks!

Hi Michael,

> Thanks for updating this, I've only just started reviewing it so there'll be
> some more comments to come later, but the most immediate issue with this
> branch is that it doesn't install the japanese plugin's "keys" directory
> (plugins/ja/qml/keys).

oh, sorry... I fixed qml.pro, it will install plugins/ja/qml/keys too.

> I've also made a couple of inline comments at locations where it's still
> using the old relative path imports for the global keys directory, this should
> now be done as "import keys 1.0" as this we allow the plugin to be installed
> separately from the rest of maliit in the future (e.g. as part of a custom
> tarball, or possibly as a click package at some point)

Changed to "import keys 1.0".

Thank you for your review!

Looking really good, just a few small things needing fixing:

1) The cursor keys move two characters at a time (it looks like they're moving the cursor on both the press and release events)

2) In the character selection pop-over the characters that aren't currently selected should use the same colour as the selection pop-over on the other keyboard layouts (this is done by setting the opacity to 0.6 when not selected)

3) If touch feedback is enabled in settings then sliding between characters on the selection pop-over should trigger haptic feedback via pressEffect.start() (same as on other keyboard layouts)

4) The copyright year on a lot of the added files is listed as 2012/2013/2014, this should be 2015 since these are new files

5) Is it correct that the URL and email layouts should be a non-Japanese qwerty layout? (If so it might be worth populating the URL key with useful Japanese TLDs like .jp, .co.jp, etc.)

6) Could you add a comment to the modifier.js explaining the purpose of the map and normalize arrays?

Thank you for more reviewing!

I fixed all of your pointed out. Please check it.

> 1) The cursor keys move two characters at a time (it looks like they're moving
> the cursor on both the press and release events)

Fixed typo (s/Released/Pressed/)

> 2) In the character selection pop-over the characters that aren't currently
> selected should use the same colour as the selection pop-over on the other
> keyboard layouts (this is done by setting the opacity to 0.6 when not
> selected)

Set opacity for unselected Text element.

> 3) If touch feedback is enabled in settings then sliding between characters on
> the selection pop-over should trigger haptic feedback via pressEffect.start()
> (same as on other keyboard layouts)

Add haptic feedback and not audio feedback (same as original extended key).

> 4) The copyright year on a lot of the added files is listed as 2012/2013/2014,
> this should be 2015 since these are new files

Set all 2015.

> 5) Is it correct that the URL and email layouts should be a non-Japanese
> qwerty layout? (If so it might be worth populating the URL key with useful
> Japanese TLDs like .jp, .co.jp, etc.)

Change email/url layout to same as normal Japanese layout.
Commonly used Japanese TLD is ".com" and ".jp".
Indeed ".co.jp" is used too, but doesn't manual typewith frequently as url/email .

Additionally to this, set email/url default layout alphabet instead of kana.

> 6) Could you add a comment to the modifier.js explaining the purpose of the
> map and normalize arrays?

Add comment.

Thanks!

Hi Michael,

Is there any chance to be merged for OTA-8 or OTA-9?

Hi Mitsuya,

 Sorry for the delay in replying, I hadn't spotted your updates; I'll try to get it re-reviewed and included for OTA-9

Cheers,
 Mike

Hi Mitsuya,

 I've just been re-reviewing this, and it looks great; but I've found one last issue, whereby the cursor keys don't work correctly whilst still in pre-edit. I'm wondering if it might be best to simply call Qt.inputMethod.commit() when pressing the cursor keys?

It looks like the cursor does get moved internally within maliit (so deletions and insertions occur in the correct place) but the visual cursor doesn't move. I think just committing the text prior to moving the cursor would be the simplest solution for this.

Hi Michael,

I rebased newer ubuntu-keyboard code.

About cursor key:
I think "commit" is just commit current preedit, then clear preedit and predictions.
In this means, a preedit in Japanese should not be cleared by cursor key,
should be moved just cursor position.

Can I move visible cursor position without commit?

Perhaps I can use cursor_pos_override argument of setPreedit() in src/lib/models/text.h.

However AbstractTextEditor::replacePreedit() uses text->setPreedit() without 2nd argument.
I will try to customize replacePreedit().

Umm... cursor_pos_override just updates m_cursor_position...
Anyway, when exists preedit, press twice left and insert any character, then update cursor position.
I hope there are any hint in this behaviour.

Hi Michael,

Finally fixed cursor key problem in rev412. Could you review it?

IMPORTANT NOTE:
Revision 412 modify code in singleBackspace() in src/view/abstracttexteditor.cpp.
It will affect other language not only Japanese.
I believe this modification does not change behaviours of backspace.

Hi Mitsuya,

 The changes appear to have fixed the cursor issue nicely, and I haven't seen any negative effects on other layouts, so that's looking good, great work :).

 Unfortunately I've found one more issue: if an ideographic space character (second function from the normal space) is inserted and then committed without any other characters, all predictions are broken until the user switches to another layout and back (effectively unloading and then reloading the Japanese word engine).

 One other quick question, is it normal in Japanese input methods for the return key to be used for committing pre-edit strings? For all other layouts this is done by the space key. I'm happy to leave it with the return key if that's what most Japanese users would already expect, but otherwise it would be good to keep consistent with other layouts if possible.

Thanks!
 Mike.

Actually after a bit of further testing it doesn't appear to be directly related to the ideographic space, it seems it can happen with any string if you type it quickly and the commit (presumably whilst anthy is still processing predictions?)

Thank you your review!

> quickly typing problem

I can't reproduce yet. Could you tell me any typing sample word (and any apps)?

> enter key

Yes, Japanese users expect return key as "commit preedit".
Additionally, in desktop, space key means "change next candidate of the preedit".
I think return key label should not be changed.

More generally, Japanese input method change label of return key
to "確定" ("commit" in Japanease) if in preedit.
I would like to implement this feature in the future.

Hmm... it seems that plugins/ja/src/anthyadapter.cpp isn't reentrant.
I'll check more deeply. Thanks!

Hi Micahel,

I found a problem in plugins/ja/src/japaneseplugin.cpp and fixed it.
Perhaps it will be related you found quickly typing problem.
Could you retry it?

Hi Mitsuya,

 That seems to have fixed it, great work! I'm just waiting on some fixes to unity8 that are causing some issues with the autopilot tests and then we can see about getting your branch landed :)

Hi Mitsuya,

 I'm afraid during my final run through the test plan for this I spotted another issue, the bottom row of keys isn't positioned correctly in the landscape view, here's a screenshot showing the issue: http://mikeasoft.com/~mike/jp-landscape.png

 Once that's fixed we should be able to land this :)

Cheers,
 Mike.

OK, I can reproduce it. However layout is changed English layout then back to Japanese layout, bottom row is positioned correctly. Umm...

Ah, OK. Language key is wrong height. Fixed.

Great, thanks, one last little thing I spotted, the backspace icon is offset slightly, this actually looks like a bug in the leftSide/rightSide handling for action keys, so for now I'd suggest you just set the width of that key to "panel.keyWidth". Then as a separate task I'll fix the action key icon positioning at a later date and also update all the existing layouts to use leftSide/rightSide for shift and backspace (which they don't currently do).

One other thing, would it not make more sense for the "." to be the primary key on the URL layout's url key instead of the ","?

packaging looks good

Hi Michael,

> I'd suggest you just set the width of that key to "panel.keyWidth".

If set width or remove "rightSide", then first row will be right offset.
This is caused by no rightMargin in CharKey.
Should I push the code to remain the layout corruption?

https://goo.gl/photos/bG5ZxHxfyMhExc926

> would it not make more sense for the "." to be the primary key on the URL layout's url key instead of the ","?

Indeed. To operation coherency, I will swap "," and "." at all keyboard.

Hi Mitsuya,

 Just did a quick test, you can work around that by setting the width on the UndoKey as well as the width on the BackspaceKey, then everything will align correctly.

Cheers,
 Mike.

Hi Michael,

Thanks you for nice advice!
I pushed codes to fix problems which you pointed out.

Thanks,

Great, thanks; I'll run that through the test plan one last time on Monday and then hopefully we can get it landed :)

Hey Mitsuya,

 Sorry! Spotted another issue, you must be getting really tired of me saying "one last thing..." ;)

 The commit key isn't inserting the primary candidate (the bold candidate) when in auto-complete mode, instead it inserts the user's input unmodified. Is this deliberate (i.e. does the Japanese keyboard not really support auto-complete at all)? If so then we shouldn't be highlighting a primary candidate for Japanese, otherwise pressing the commit key should input the primary candidate instead of the user input.

Hi Michael,

Indeed. Generally in Japanese, the commit key should insert the preedit string (primary purpose to input Hiragana).
To clarify which candidate will be committed, I set the primary candidate to the preedit string.

Is it okay?

Okay, that sounds good; I'll kick off a rebuild now and retest when it's done

Unfortunately I found an extra issue, one of the changes made to the core breaks both the email detection and mid-word insertion based disabling of pre-edit for all the other plugins.

const bool enablePreeditAtInsertion = d->word_engine->languageFeature()->wordEngineAvailable();

^ wordEngineAvailable isn't really an appropriate feature to check for this, as that's going to be true for pretty much all plugins. I think you really need to explicitly add a new language feature for this (which could be called enablePreeditAtInsertion), that's true for Japanese but false for all other plugins.

Sorry for my numerous failure...

I fixed wordEngineAvailable() bug. Could you check it?

That's great thanks, and no are apologies necessary :) This is by far the most advanced keyboard we've had contributed, so it's natural that there be a few issues in the review.

I've completed a full test now and everything looks good, so I'll propose it for landing now :)

Thanks for all your hard work on this!

Thank you for your review and merge!