162666 - additional IME work / cleanup for widget/src/cocoa

Status: RESOLVED FIXED

(Camino Graveyard :: HTML Form Controls, defect)

Description

[Kathleen :Brade]

Some or all of the additional IME work will be covered in this bug.

 * Cleanup the calls to [self convert: theEvent ...]
 * Cleanup and simplification of keyDown
 * make IME more closely match Cocoa behaviors
 * candidate window shows in left top corner of screen
 * korean ime does not work correctly in "character" mode
 * mouse click other place before the ime text may cause problem 
 * mouse operation within IME does not work (neither carbon or window)
 * ime reconversion (japanese ime control+1) is not implemented yet.
 * remove the #ifdefs for DEBUG_IME

Comment 1

[Kathleen :Brade]

Attachment: fix for ime to make more consistent

reviews?

Comment 2

[Mike Pinkerton (not reading bugmail)]

-static void convertAttributeToGeckoRange(NSAttributedString *aString, PRUint32
inCount, nsTextRange* aRanges)
+static void convertAttributeToGeckoRange(NSAttributedString *aString, PRUint32
inCount, nsTextRange* aRanges, NSRange markRange)

i'd prefer that |aRanges| be last if it's the resulting "out" param of the function.

-    case 2:                      return NS_TEXTRANGE_SELECTEDCONVERTEDTEXT;

why did this go away?

r=pink.

Comment 3

[Kathleen :Brade]

>-    case 2:                      return NS_TEXTRANGE_SELECTEDCONVERTEDTEXT;
>
>why did this go away?

That was changed as part of the "hack removal"; it gets set later based on the
markRange (in convertAttributeToGeckoRange).

Comment 4

[Kathleen :Brade]

reopen bug for remaining issues

Comment 5

[waverider]

Attachment: more fix

@@ -171,7 +171,7 @@
simple fix.

@@ -209,18 +209,6 @@
1.effectiveRange.length is the length of each segment and be always smaller
than markRange.length. This part is not necessary.
2.Using NS_TEXTRANGE_RAWINPUT and NS_TEXTRANGE_SELECTEDRAWTEXT cause some bug
of IME.

@@ -2963,8 +2951,9 @@
IME can handle over 300 character at a time. MAX_BUFFER_SIZE 32 is too small.
I changed it to increase automatically.

Comment 6

[Kathleen :Brade]

Comment on attachment 133335 [details] [diff] [review]
more fix

I have concerns about performance with the last two chunks of this patch.  It
seems better to not allocate if we don't need to rather than always allocating.
 I might argue that MAX_BUFFER_SIZE should be bigger than 32 rather than always
calling new.

Can we get a patch that is tested with only the first two chunks of this patch?

Thanks!

Comment 7

[waverider]

Attachment: I agree with you.

Max character number that IME can handle:
Kotoeri (default IME of MacOS X) = 100
EGBridge (IME from elgosoft.corp) = about 300

I think MAX_BUFFER_SIZE = 512 will be safe.

Comment 8

[waverider]

Attachment: latest patch.

"new PRUnichar[len + 1]" and "buffer" are in the wrong order.

Comment 9

[waverider]

Attachment: when we cancel inputting with IME, we need to send an end composition event.

Comment 10

[Kathleen :Brade]

Comment on attachment 136043 [details] [diff] [review]
when we cancel inputting with IME, we need to send an end composition event.

for the ending segment in this patch, should this piece also be changed if len
== 0?

if (!mInComposition)
  {
    [self sendCompositionEvent:NS_COMPOSITION_START];
    mInComposition = YES;
  }

Comment 11

[waverider]

I edit the ending segment in this patch reffering to - (void)insertText.
In this part, NS_COMPOSITION_START and NS_COMPOSITION_END are used in pairs.

Comment 12

[Kathleen :Brade]

I understand that you only added the ending piece.  However, I'm wondering if we
should be calling NS_COMPOSITION_START and NS_COMPOSITION_END at all if len==0.

Should the block you didn't change be changed to:
  if (!mInComposition && len > 0)
  {
    [self sendCompositionEvent:NS_COMPOSITION_START];
    mInComposition = YES;
  }

Or maybe this is a non-issue if mInComposition would already be set to YES from
a previous keystroke if len == 0?

Comment 13

[waverider]

comment #12
It's better than my cheap way.
When I test camino that include your way, it run finely.

Comment 14

[waverider]

I take back comment #13.

If we start inputting with IME already and delete all markedtext, mInComposition
keep "Yes" until we call -insertText and turn mIncomposition "No".
I think that line for judgement len == 0 or not and sending "NS_COMPOSITION_END"
and turning mInComposition into "No" should be written in -setMarkedText.

Comment 15

[waverider]

Attachment: call unmarktext in -setMarkedText.

Comment 16

[Mike Pinkerton (not reading bugmail)]

this latest patch does nothing to fix 181198, just fwiw. what does it fix? how
can i verify it's working?

Comment 17

[Hiro]

This latest patch has no relation with bug181198.
The following things are corrected with this patch.
And this patch is working in my test build.

--  Long word is truncated on conversion or crash.
    The 3rd and the 4th block in a patch correspond.

    see bug 90910.

-- Improvement in the user experience in a Japanese input.
    Blocks other than the above.

Comment 18

[Mike Pinkerton (not reading bugmail)]

taking so we can get this landed.

Comment 19

[Mike Pinkerton (not reading bugmail)]

how do i test this? is the last patch (attachment 136659 [details] [diff] [review]) the only one i need to
land? once it's landed, what about the other things covered in comment 0 of this
bug?

Comment 20

[Mike Pinkerton (not reading bugmail)]

fwiw, this patch fixes our "go back in the middle of ime on backspace" bug.

Comment 21

[Mike Pinkerton (not reading bugmail)]

never mind the last part, i'm on crack. i was testing it incorrectly. my
original comment on how to test this remains.

Comment 22

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 136659 [details] [diff] [review]
call unmarktext in -setMarkedText.

landed this patch. what else needs to be done for this bug?

Comment 23

[Hiro]

Because i do not understand whether it is related to this bug, there is a bug
which is reported separately.(bug 232406)
When creating attachment 136659 [details] [diff] [review], it tried to fix of bug 232406.
But that was not possible...

Depending upon attachment 136659 [details] [diff] [review], Japanese input it becomes good in comparison
with until now.
As for remainder, when editing the HIRAGANA which is input temporarily, the
caret just is displayed in accurately.

The Japanese input of Camino will become equivalent to Mozilla/Mozilla Firebird,
when the problem of this caret fixes :-)

Comment 24

[Mike Pinkerton (not reading bugmail)]

well, since 232406 is already filed, i'm going to close this bug out. if anyone
thinks we should reopen it, please feel free to do so.

Comment 25

[Mike Pinkerton (not reading bugmail)]

f