609843 - Improve usage of Meego Input Method

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Qt, defect)

Description

[Jeremias Bosch (:jbos)]

Meego Input Method is somewhat working right now in fennec, but it has certain issues with plugins, and also does not support all features provided by meego.

Comment 1

[Jeremias Bosch (:jbos)]

Attachment: fix keyboard for meego

this patch seems a bit messy but it contains needed changes for statusbar and dispatch NS_ACTIVATE/DEACTIVATE handling, 

It fixes all aspects of Keyboard Handling in meego and makes keyboard work perfectly. Even with plugins.

Special thing on Meego: You do not get any Keyevents, thats why we need to generate them.

Also special, its expected that the plugins are able to handle unicode keys.

Comment 2

[Brian Crowder]

Does this fix bug 600897?

Comment 3

[Jeremias Bosch (:jbos)]

mhm never tried that on n900, right now i checking for a way to get xulrunner compiling and running with some official sdk for meego.

as it changes a lot i would expect that it also effects this. Current upstream state is pretty bad and basically unusable for meego. This patch is still as it is still wip, it needs cleanup and cuting away of unneeded parts. also i need to check that i didn't miss something (as it is quite a monster with its 1200 lines..)

Comment 4

[Brian Crowder]

Will you keep the other bug in mind as you go, and dupe it here if you find that you've killed it?

Comment 6

[Jeremias Bosch (:jbos)]

Attachment: Disable Default Event Handling for MeegoTouch (due to the total lack of keyevents)

In meego, there are no more keyevents from the input method (=> virtual keyboard) you only get the input method events. Containing Preedit and Commit Strings. In order to handle Plugin-Key-Input and Websites listening on KeyEvents like google correctly, we can not only support commit strings and nothing else.

We need to handle this explicitly different (see part 2) -> Every preedit and commit generates KeyEvents. This way you can have:

-> Error Correction
-> Chinese Support (...)
and Full Web Interoperability with just one solution

Comment 7

[Jeremias Bosch (:jbos)]

Attachment: Part2, Cleaned Up Version, still need cuting in logical small pieces.

Part 2, WIP -> needs cutting in smaller pieces.

Comment 8

[Jeremias Bosch (:jbos)]

Attachment: Part2, Cleaned Up Version, still need cuting in logical small pieces.

WIP Part2, Cleaned Up Version, still need cuting in logical small pieces.

Comment 9

[Jeremias Bosch (:jbos)]

Attachment: Part2, Add Native Key for Qt.

This adds the native Key to the IPC API.

Comment 10

[Jeremias Bosch (:jbos)]

Attachment: Part3, Fn Key is not working

Fn Key (AltGr) is not working correctly on qt build. This makes it work.

Comment 11

[Jeremias Bosch (:jbos)]

Attachment: Part4, add Meego Input Fixes

Meego Input does not send any QKeyEvents anymore.

We need to support QInputMethod-Events (preedit and commit). In order to guarantee full web support we have generate out of these Preedit/Commit Text our own QKeyEvents. The solution here seems a bit hacky, but its actually quite nice, since it will allow easy introduction of Meego Error-Correction, Chinese (etc) to a later point of time, and keeps website features like google instant search possible even in those cases.

One important and sad aspect here is: The VKB does support different language layouts, but does not change the XKeyMap of the system. Also and because of that it is not possible for us to setup the QKeyEvent correctly (with nativeKeyCode), which means that we do have a issue when it comes to generate the nsKeyEvent. (Simply asking the XKeySym would lead to the wrong result -> also important i.e. for flash input)

In order to fix this we introduced here another mapping for text to QtKey, those can than correctly mapped to x values.

Comment 12

[Oleg Romashin (:romaxa)]

Comment on attachment 505089 [details] [diff] [review]
Disable Default Event Handling for MeegoTouch (due to the total lack of keyevents)

What is preventing us to use imComposeEvent everywhere?

Comment 13

[Oleg Romashin (:romaxa)]

Comment on attachment 505762 [details] [diff] [review]
Part2, Add Native Key for Qt.

I think this should be splitted into 3 patches:
1) - TabParent/TabChild key events
2) nativeKeyCode flag
3) nsObjectFrame chages (plugins related only)

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 505763 [details] [diff] [review]
Part3, Fn Key is not working

Should be easy to fix.
+        sAltGrModifier=false;
                       ^ add spaces here and in other places.

Comment 15

[Oleg Romashin (:romaxa)]

Comment on attachment 505767 [details] [diff] [review]
Part4, add Meego Input Fixes


>+#ifdef MOZ_PLATFORM_MAEMO
>+int
>+QtKeyEventToXKeySym(QKeyEvent* keyEvent)

what is this for and who is using that?

Comment 16

[Jeremias Bosch (:jbos)]

Attachment: Fennec Mobile-Browser Patch, disable ContentCustomKeySender for Maemo 6

After using a chrome input element, this would cause that every key gets entered twice in content input elements. -> Disable it for Maemo 6

Comment 17

[Jeremias Bosch (:jbos)]

(In reply to comment #12)
> Comment on attachment 505089 [details] [diff] [review]
> Disable Default Event Handling for MeegoTouch (due to the total lack of
> keyevents)
> 
> What is preventing us to use imComposeEvent everywhere?

Three Points what is preventing us:

1 ) It still need us to generate KeyEvents for Keys like Space, Return,... . On normal desktop the user does have usually a InputMethod_Extender_ and a normal Hardware Keyboard. The Concept of InputMethod_Extender_ where never be, to be the only way to enter text. 

2 ) Haveing them as _only_solution_ will cause that i.e. websites like Google Instant Search do not react on every entered key (only after a keyevent), or - in worst case - do not even display any text.

3 ) Taking Plugins, the NPAPI does not support TextEvents. In order to get typed text into i.e. a Flash Input-Element we have to provide X-KeyEvents no matter what. 


=> Doing it the way we do it here: Generating out of the provided Text our own KeyEvents allow us to have support everything with the smallest amount of code and bugs.

Comment 18

[Jeremias Bosch (:jbos)]

(In reply to comment #15)
> Comment on attachment 505767 [details] [diff] [review]
> Part4, add Meego Input Fixes
> 
> 
> >+#ifdef MOZ_PLATFORM_MAEMO
> >+int
> >+QtKeyEventToXKeySym(QKeyEvent* keyEvent)
> 
> what is this for and who is using that?

its getting used in nsWindow.cpp while KeyRelease.

I tried to get around it here:

One important and sad aspect of the VKB is: It does support different language
layouts, but does _not_ change the XKeyMap of the system. Also and because of
that it is not possible for us to setup the QKeyEvent correctly (with
nativeKeyCode), which means that we do have a issue when it comes to generate
the nsKeyEvent. (Simply asking the XKeySym would lead to the wrong result ->
also important i.e. for flash input) like Ä becomes an A.

In order to fix this we introduced here another mapping for KeyEvents (which have a Unicode Text Member) into KeySym-Code. And those can than correctly mapped to x values.

Comment 19

[Oleg Romashin (:romaxa)]

> its getting used in nsWindow.cpp while KeyRelease.

That is the problem, you have attached patch which is adding QtKeyEventToXKeySym definition and declaration, but no single line where it used... how did you test these patches?

Comment 20

[Oleg Romashin (:romaxa)]

Another problem is that you are adding TabParent/TabChild key events for all platforms, and disabling KeySender only for maemo6... this is very wrong.

Can we get single patch which is adding TabParent/TabChild key events (no native keys), remove CustomKeySender and get it reviewed/pushed for all platforms?

Not sure if mobile people have anything against TabParent/TabChild key events, but if that is not accepted, then we should try to make it works without looking at plugins and using NativeKeyCode...

And as soon basic usecase is working we can make it works for plugins too.

Comment 21

[Jeremias Bosch (:jbos)]

I agree. This patches fixing mostly every aspect which troubles meego input method but where never done under the aspect to take care about other platforms.

Its pretty challenging to review and update them as they are pretty complex and taking many different aspects in account. But currently input does not work at all anymore with Meego VKB, which is the first Issue to be addressed. 

=> So we need to take Part 4 Patch at first and get it even more simplified by removing everything for plugin usecase and compose another patch for this. You are right that ie. this patch misses the QtKeyEventToXKeySym call. It should have been introduced here too, i actually forgot it. (Before it was already introduced in Part2 patch...

Thanks for the feedback.

Comment 22

[Oleg Romashin (:romaxa)]

It is hard to review all these patches with title "It fixing key events for Meego".... 

it would be nice to have logically (functionality) separated patches, which are small enough and clear for review. like "Fn Key is not working" patch.

And first I would suggest to apply Qt only patches which are making Qt port keys/vkb working for meego (no plugins for now) without changing key events architecture/API for generic mozilla code.

Comment 23

[Oleg Romashin (:romaxa)]

When Qt port is working with current Mozilla API, we can try to kill CustomKeySender and switch fennec for using TabParent/TabChild native key IPC send/receive  and possibly make plugins keys working.

Comment 24

[Jeremias Bosch (:jbos)]

I agree, definitely we need to do it this way. I finish cleaning the qt only patch without any aspect of plugins tomorrow.

Comment 25

[Jeremias Bosch (:jbos)]

Attachment: Part 1, minimal meego / Qt fixes

ok, this is the minimal version of qt fixes. It only contains what is really needed to get it working again. 

-> you need to have "Disable Default Event Handling for MeegoTouch too.

Comment 26

[Oleg Romashin (:romaxa)]

Comment on attachment 506358 [details] [diff] [review]
Part 1, minimal meego / Qt fixes

>+    //OtherFocusReason most like means VKB was closed manual (done button)
>+    if (aEvent->reason() == Qt::OtherFocusReason
>+         && gKeyboardOpen) {

Do it like
+    if (aEvent->reason() == Qt::OtherFocusReason && gKeyboardOpen) {
or
+    if (aEvent->reason() == Qt::OtherFocusReason &&
+        gKeyboardOpen) {

>-#if (MOZ_PLATFORM_MAEMO==5)
>+#if (MOZ_PLATFORM_MAEMO==6)
                         ^ ^ - spaces missing here and in many other places of this patch

>+       || aEvent->key() == Qt::Key_Backspace)
>+      mReceiver->OnKeyPressEvent(aEvent);
>+    return;

This file is using 4 spaces indent, and for this patch it is totally random...



>+void MozQWidget::inputMethodEvent(QInputMethodEvent* aEvent)
>+{

Whole this function has totally broken indentation...

>+
>+    if (currentCommitString == " ") {
>+      QKeyEvent press(QEvent::KeyPress, Qt::Key_Space,Qt::NoModifier, QString(" "),0,1);
>+    if (currentCommitString == "\n")
>+      QKeyEvent release(QEvent::KeyRelease, Qt::Key_Return, Qt::NoModifier, QString("\n"),0,1);

should not we map commit string by index range of symbol (when currentCommitString length == 1) and parse them as "service" symbols..

> 
>+    if (!gKeyboardOpen) return;
{}, next line

>+
>-                    break;
>+        if ((domCharCode > 0xFF || domCharCode < 0x20) &&
>+                (aEvent->modifiers() == Qt::ControlModifier)) {
>+            if (aEvent->nativeScanCode() != 0
>+             && aEvent->nativeScanCode() >= (quint32)x_min_keycode

Style
+             if (aEvent->nativeScanCode() != 0 &&
+                 aEvent->nativeScanCode() >= (quint32)x_min_keycode

>+            if (domCharCode==0 &&
>+                    (domKeyCode > NS_VK_0 && domKeyCode < NS_VK_9) ||

style

>         event.charCode = domCharCode;
>@@ -1685,27 +1699,32 @@

Add -p option for diff/qdiff
[defaults]
diff=-p -U 8
qdiff=-p -U 8

[diff]
showfunc = 1

>+             && aEvent->nativeVirtualKey() != 0
>+             && aEvent->nativeScanCode() >= (quint32)x_min_keycode
>+             && aEvent->nativeScanCode() <= (quint32)x_max_keycode) {

what for you do all these modifications? are they related to basic work with Meego6, or it is some other bugs?

Comment 27

[Oleg Romashin (:romaxa)]

Comment on attachment 506358 [details] [diff] [review]
Part 1, minimal meego / Qt fixes


>+      QKeyEvent press(QEvent::KeyPress, Qt::Key_Space,Qt::NoModifier, QString(" "),0,1);
>+      mReceiver->OnKeyPressEvent(&press);
>+      QKeyEvent release(QEvent::KeyRelease, Qt::Key_Space,Qt::NoModifier, QString(" "),0,1);
>+      mReceiver->OnKeyReleaseEvent(&release);


duping code OnKeyPressEvent/OnKeyReleaseEvent, 

can we prepare press/release key event and do mReceiver->OnKeyPressEvent, mReceiver->OnKeyReleaseEvent in one place?

Comment 28

[Jeremias Bosch (:jbos)]

Attachment: Part 1, minimal meego / Qt fixes V2

Thanks, clear - :)

Fixed comments. The Native Stuff making Shortcuts working in Qt Firefox (desktop) and for Meego i.e. on Ideapad. I Consider that as quite important and essential functionality, so I kept it in.

Comment 29

[Oleg Romashin (:romaxa)]

Comment on attachment 506386 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V2


>+    //first check for some controllkeys send as text...
>+    if (currentCommitString == " ")
>+    {

if () {


>+                    if (letter->isUpper())
>+                        modi = Qt::ShiftModifier;
{}

>+                            if (letter->isUpper())
>+                                modi = Qt::ShiftModifier;
{}
>-                    if (altCharCode.mUnshiftedCharCode && altCharCode.mShiftedCharCode) {
>-                        break;
>+
>+            if (aEvent->nativeScanCode() != 0

Still no explanation why this is needed here


>+                        //last possible case, we had a PreeditString which was now completely changed.
>+                        //first, check if just one letter was removed (normal Backspace case!)

One thing I don't understand here... is why you are not using rest of QInputMethodEvent API
like 
int 	replacementLength () const
int 	replacementStart () const

and instead you trying to remember something, compare and guess?

Comment 30

[Jeremias Bosch (:jbos)]

> One thing I don't understand here... is why you are not using rest of
> QInputMethodEvent API
> like 
> int     replacementLength () const
> int     replacementStart () const
> 
> and instead you trying to remember something, compare and guess?

Because last i checked, those where not implemented.

Comment 31

[Oleg Romashin (:romaxa)]

Comment on attachment 506386 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V2


>+     mReceiver->OnKeyReleaseEvent(&release);
>+}
>+    
  ^ white spaces
> 
>+    if (!gKeyboardOpen) return;
>+
{}

And don't include nsWindow.cpp patch here... I don't know what is this about and why it is needed here.

also for maemo6 we should disable mReceiver->imComposeEvent, otherwise, it double typing...

and I think it should be somehow logically connected with "return QVariant(/*M::InputMethodModeNormal*/ 0 );" change...

Comment 32

[Jeremias Bosch (:jbos)]

Attachment: Part 1, minimal meego / Qt fixes V3

Fixed Comments, removed nsWindow Part to add it to its own patch

Comment 33

[Jeremias Bosch (:jbos)]

Attachment: Part 2, fix shortcuts for qt

Fix the shortcuts in qt desktop and use complete qkeyevent interface

Comment 34

[Oleg Romashin (:romaxa)]

Comment on attachment 505089 [details] [diff] [review]
Disable Default Event Handling for MeegoTouch (due to the total lack of keyevents)

this is the only way to make it work for maemo6

Comment 35

[Oleg Romashin (:romaxa)]

Comment on attachment 506699 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V3

+    if (!gKeyboardOpen) return
{}

+                    Qt::KeyboardModifier modi = Qt::NoModifier;
+
+                    if (letter->isUpper()) {
+                        modi = Qt::ShiftModifier;
+                    }

this is repeating 3 times...
can you move it also into sendPressReleaseKeyEvent ?

I hope we can Optimize this function and use all QInputMethodEvent params, and build more nice convertor commit/preedit->KeyEvents.

Comment 36

[Jeremias Bosch (:jbos)]

Attachment: Part 1, minimal meego / Qt fixes V4

More Cleanup.

Comment 37

[Oleg Romashin (:romaxa)]

Comment on attachment 506714 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V4


>+                        const QChar * text = currentPreeditString.unicode();
>+                        for (int i = gLastPreeditString.length(); i < currentPreeditString.length(); i++) {
>+                            const QChar* letter = &text[i];
>+                            sendPressReleaseKeyEvent(0, QString(*letter));
I guess it is wrong...


> 
>+    if (!gKeyboardOpen) return;
>+

{}

Comment 38

[Jeremias Bosch (:jbos)]

Attachment: Part 1, minimal meego / Qt fixes V5

Last Bits fixed

Comment 39

[Oleg Romashin (:romaxa)]

Comment on attachment 506717 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V5

looks good now.

Comment 40

[Jeremias Bosch (:jbos)]

Attachment: Part4, Meego VKB reports wrong size in portrait mode, Add workaround.

Workaround for size bug in portrait mode.

Comment 41

[Jeremias Bosch (:jbos)]

This is NPOTB.

Comment 42

[Oleg Romashin (:romaxa)]

Comment on attachment 506738 [details] [diff] [review]
Part4, Meego VKB reports wrong size in portrait mode, Add workaround.

this I totally don't understand... what is 500, what is 80, what is 140.... and how this supposed to work let say on N900 with meego installed or, Neetbook with meego Virtual keyboard and meego environment?

> ()-h
    ^ spaces are missing...

Comment 43

[Oleg Romashin (:romaxa)]

Comment on attachment 505763 [details] [diff] [review]
Part3, Fn Key is not working

name=value
    ^ space here.

Comment 44

[Oleg Romashin (:romaxa)]

Comment on attachment 506701 [details] [diff] [review]
Part 2, fix shortcuts for qt


>             }
>-
>             if (altCharCode.mUnshiftedCharCode || altCharCode.mShiftedCharCode) {

don't remove empty lines..

I would suggest to move shortcuts improvemnts in another bug and test it properly.. and figure out which shortcuts and in what case this patch fixing the problem.

Comment 45

[Jeremias Bosch (:jbos)]

(In reply to comment #44)
> Comment on attachment 506701 [details] [diff] [review]
> Part 2, fix shortcuts for qt
> 
> 
> >             }
> >-
> >             if (altCharCode.mUnshiftedCharCode || altCharCode.mShiftedCharCode) {
> 
> don't remove empty lines..
> 
> I would suggest to move shortcuts improvemnts in another bug and test it
> properly.. and figure out which shortcuts and in what case this patch fixing
> the problem.

628687  > already done

Comment 46

[Jeremias Bosch (:jbos)]

Attachment: Updates little typo, duno how it passed compiling and test server... anyway copied review+ from romaxa

Updates little typo, duno how it passed compiling and test server... anyway copied review+ from romaxa

Comment 47

[Oleg Romashin (:romaxa)]

Comment on attachment 507089 [details] [diff] [review]
Updates little typo, duno how it passed compiling and test server... anyway copied review+ from romaxa


>+    //first check for some controllkeys send as text...
>+    if (currentCommitString == " ") {
>+        sendPressReleaseKeyEvent(Qt::Key_Space, currentCommitString.unicode());
>+    } else {
>+        if (currentCommitString == "\n") {

use  } else if () {

so function will not be super wide in that case...

Also merge this patch with first patch imComposeEvent disabler, and add comment like "This does not work for maemo6, because IM framework is broken bla bla"

Comment 48

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/f2148c181f55
http://hg.mozilla.org/mozilla-central/rev/11db9166ba1c