Improve usage of Meego Input Method

RESOLVED FIXED in mozilla2.0b11

Status

RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: jbos, Assigned: jbos)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b11
ARM
MeeGo
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 11 obsolete attachments)

924 bytes, patch
romaxa
: review+
Details | Diff | Splinter Review
1.35 KB, patch
romaxa
: review+
Details | Diff | Splinter Review
6.72 KB, patch
Details | Diff | Splinter Review
790 bytes, patch
romaxa
: review-
Details | Diff | Splinter Review
9.07 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
OS: Linux → MeeGo
Hardware: x86 → ARM
(Assignee)

Comment 1

8 years ago
Created attachment 497488 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
Blocks: 619056

Comment 2

8 years ago
Does this fix bug 600897?
(Assignee)

Comment 3

8 years ago
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

8 years ago
Will you keep the other bug in mind as you go, and dupe it here if you find that you've killed it?
Duplicate of this bug: 621233
(Assignee)

Comment 6

8 years ago
Created attachment 505089 [details] [diff] [review]
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
Attachment #497488 - Attachment is obsolete: true
Attachment #505089 - Flags: review?(doug.turner)
(Assignee)

Comment 7

8 years ago
Created attachment 505091 [details] [diff] [review]
Part2, Cleaned Up Version, still need cuting in logical small pieces.

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

Comment 8

8 years ago
Created attachment 505092 [details] [diff] [review]
Part2, Cleaned Up Version, still need cuting in logical small pieces.

WIP Part2, Cleaned Up Version, still need cuting in logical small pieces.
Attachment #505091 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
Created attachment 505762 [details] [diff] [review]
Part2, Add Native Key for Qt.

This adds the native Key to the IPC API.
Attachment #505092 - Attachment is obsolete: true
Attachment #505762 - Flags: review?(doug.turner)
(Assignee)

Comment 10

8 years ago
Created attachment 505763 [details] [diff] [review]
Part3, Fn Key is not working

Fn Key (AltGr) is not working correctly on qt build. This makes it work.
Attachment #505763 - Flags: review?(doug.turner)
(Assignee)

Comment 11

8 years ago
Created attachment 505767 [details] [diff] [review]
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.
Attachment #505767 - Flags: review?(doug.turner)
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 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 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 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?
(Assignee)

Comment 16

8 years ago
Created attachment 505776 [details] [diff] [review]
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
Attachment #505776 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #505776 - Flags: review? → review?(mark.finkle)
(Assignee)

Comment 17

8 years ago
(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.
(Assignee)

Comment 18

8 years ago
(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.
> 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?
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.
(Assignee)

Comment 21

8 years ago
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.
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.
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.
(Assignee)

Comment 24

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

Comment 25

8 years ago
Created attachment 506358 [details] [diff] [review]
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.
Attachment #505762 - Attachment is obsolete: true
Attachment #505767 - Attachment is obsolete: true
Attachment #505776 - Attachment is obsolete: true
Attachment #506358 - Flags: review?(romaxa)
Attachment #505762 - Flags: review?(doug.turner)
Attachment #505767 - Flags: review?(doug.turner)
Attachment #505776 - Flags: review?(mark.finkle)
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 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?
(Assignee)

Comment 28

8 years ago
Created attachment 506386 [details] [diff] [review]
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.
Attachment #506358 - Attachment is obsolete: true
Attachment #506386 - Flags: review?(romaxa)
Attachment #506358 - Flags: review?(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?
(Assignee)

Comment 30

8 years ago
> 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 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...
(Assignee)

Comment 32

8 years ago
Created attachment 506699 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V3

Fixed Comments, removed nsWindow Part to add it to its own patch
Attachment #506386 - Attachment is obsolete: true
Attachment #506699 - Flags: review?(romaxa)
Attachment #506386 - Flags: review?(romaxa)
(Assignee)

Comment 33

8 years ago
Created attachment 506701 [details] [diff] [review]
Part 2, fix shortcuts for qt

Fix the shortcuts in qt desktop and use complete qkeyevent interface
Attachment #506701 - Flags: review?(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
Attachment #505089 - Flags: review?(doug.turner) → review+
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.
(Assignee)

Comment 36

8 years ago
Created attachment 506714 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V4

More Cleanup.
Attachment #506699 - Attachment is obsolete: true
Attachment #506714 - Flags: review?(romaxa)
Attachment #506699 - Flags: review?(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;
>+

{}
Attachment #506714 - Flags: review?(romaxa) → review-
(Assignee)

Comment 38

8 years ago
Created attachment 506717 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V5

Last Bits fixed
Attachment #506717 - Flags: review?(romaxa)
Comment on attachment 506717 [details] [diff] [review]
Part 1, minimal meego / Qt fixes V5

looks good now.
Attachment #506717 - Flags: review?(romaxa) → review+
(Assignee)

Updated

8 years ago
Attachment #505089 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #506717 - Flags: approval2.0?
(Assignee)

Comment 40

8 years ago
Created attachment 506738 [details] [diff] [review]
Part4, Meego VKB reports wrong size in portrait mode, Add workaround.

Workaround for size bug in portrait mode.
Attachment #506714 - Attachment is obsolete: true
Attachment #506738 - Flags: review?(romaxa)
(Assignee)

Updated

8 years ago
Attachment #505763 - Flags: review?(doug.turner) → review?(romaxa)
(Assignee)

Updated

8 years ago
Attachment #505089 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #506717 - Flags: approval2.0?
(Assignee)

Comment 41

8 years ago
This is NPOTB.
Keywords: checkin-needed
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...
Attachment #506738 - Flags: review?(romaxa) → review-
(Assignee)

Updated

8 years ago
Blocks: 628687
Comment on attachment 505763 [details] [diff] [review]
Part3, Fn Key is not working

name=value
    ^ space here.
Attachment #505763 - Flags: review?(romaxa) → review+
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.
Attachment #506701 - Flags: review?(romaxa)
(Assignee)

Comment 45

8 years ago
(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
(Assignee)

Comment 46

8 years ago
Created attachment 507089 [details] [diff] [review]
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
Attachment #506717 - Attachment is obsolete: true
Attachment #507089 - Flags: review+
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"
Attachment #507089 - Flags: review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/f2148c181f55
http://hg.mozilla.org/mozilla-central/rev/11db9166ba1c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Component: General → Widget: Qt
Product: Fennec → Core
QA Contact: general → qt
Resolution: --- → FIXED
Assignee: nobody → jeremias.bosch
Target Milestone: --- → mozilla2.0b11
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.