23363 - [FEATURE]Enable Unicode on TSM1.5

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Frank Tang]

In order to fully implement the TSM 1.5 shipped w/ MacOS 8.5 and later and the
Unicode keyboard layout shipped with MacOS 9. We need to do the following
1. Check MacOS support TSM 1.5 or not
2. If it did, identify itself can handle unicode by adding kUnicodeDocument
'udoc' to the
http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsMacEventHandler.cpp#244

243   supportedServices[0] = kTextService;
244   err = ::NewTSMDocument(1,supportedServices,&mTSMDocument,(long)this);

We need to make sure supportedServices are > 2 and
add supportedServices[1] = kUnicodeDocument;
in that case

3. We need to add code to handle Unicode input in the current TSM code.
Howerver, we need to make sure MacOS which do not have TSM 1.5 still work.
4. We need to add additional AppleEvent Handler for kUnicodeNotFromInputMethod
event

Comment 1

[Frank Tang]

Not a beta feature, Mark it M20., Anyone from the net want to help ??

Comment 2

[Frank Tang]

mark it as future.

Comment 3

[Frank Tang]

IME related issue. REassign to yokoyama

Comment 4

[Jean-Marc Desperrier]

They are two open source editor that support this on the Mac :
MLTE demo and SUE.

Both of them have been written more as small demonstration of this feature than
anything else.

The autor of MLTE explicitly states :  "Full source code included and freely
reusable in other projects".

Therefore I think it would be possible to reuse some of this code to help
implement this in Mozilla, or from SUE that has a similar philosophy.

Both are described from this page that might be more explicit about their
features that the direct access one :
http://www.hclrss.demon.co.uk/unicode/utilities_editors.html

Direct access with :
http://www.merzwaren.com/snippets/index.html#mltedemo
http://members.tripod.com/%7Etomaszek/sue.html

Comment 5

[Roy Yokoyama]

Mac issue: assigning to [e-mail]

Comment 6

[nhottanscp]

Reassign to ftang.

Comment 7

[Frank Tang]

mark it as assigned

Comment 8

[Frank Tang]

Finally I have time to hack it.
I want to break down to two stages.
Statge one- upgrade the Input Method code to use Unicode of TSM 1.5 or higher is 
present.
Stage two- add kUnicodeNotFromInputMethod handler to support Unicode Keyboard. 

Here is the patch for the first step, Test on MacOS9.1, have not test on MacOSX 
yet.

Comment 9

[Frank Tang]

Attachment: patch to migrate to Unicode Input Method on TSM1.5

Comment 10

[Frank Tang]

try the latest patch on MacOS X. No problem with Japanese input (do not have 
Korean/chinese input method on MacOSX yet). 
The candidcte window some times show in wrong places. We probably need to fix 
that later by changing the kOffset2Pos and kPos2Offset handler. 

Please code review so I can land this and move to working on 77038.

Change this summary from "[FEATURE]Enable Unicode keyboard layout on TSM1.5" to
"[FEATURE]Enable Unicode on TSM1.5"

let this bug focus on Unicode Input Method, and let bug 77038 become the Unicode 
Keyboard Layout bug. We need to fix this first before adding Unicode 
kUnicodeNotFromInputMethod handler . 

Comment 11

[Simon Fraser [no longer active]]

Comments on the patch:

* All new code: please follow Mozilla's spacing conventions better. Spaces
  after 'if', spaces between argument parameters etc.

* Changes to nsMacTSMMessagePump.h/cpp look fine (apart from non-standard
  spacing and \ No newline at end of file)

* I think I'd like to see the TSM-related code in nsMacEventHandler.cpp
  move into a new file. You added a lot of new code that is IME-specific,
  and it seems that it would make sense in its own file.

* You left debug output on:

-//#define DEBUG_TSM
+#define DEBUG_TSM

I didn't check the logic in UnicodeHandleUpdateInputArea; i'll leave the reviewer 
to do that. But I did notice:

+               if(NS_FAILED(res)) 
+                       goto error; 

You'll leak in a number of places with an early return like this. Would be better 
to use auto_ptr or something to ensure cleanup.

+               if(xpTextRangeArray) 
+                       delete [] xpTextRangeArray;

This if() test is redundant for two reasons: 1) you know that if you got there, 
xpTextRangeArray is never non-null, and 2) even if it was, |delete| and |delete 
[]| are null-safe.

Another one:
\ No newline at end of file

Comment 12

[Frank Tang]

Attachment: widget/src/mac/nsTSMStrategy.cpp

Comment 13

[Frank Tang]

Attachment: widget/src/mac/nsTSMStrategy.h

Comment 14

[Frank Tang]

Attachment: patch v2 include keyboard stuff

Comment 15

[Frank Tang]

OK, the new patch include two new file widget/src/mac/nsTSMStrategy.h and widget/
src/mac/nsTSMStrategy.cpp  

and it also including the code to provide Unicode base keyboard code. 

Comment 16

[Frank Tang]

not sure which milastone I want to land this. [e-mail], can you r=
this one?
Maybe we should wait till m94 branch off. 

Comment 17

[Kathleen :Brade]

Sorry; I ran out of time before I could complete the code review.  I hope this is 
useful feedback/comments.

The two new files look fine except they appear to have tabs or a large number of 
spaces instead of the mozilla standard of 2 spaces.  Could you change these to 
use two spaces for each level of indenting?

Formatting issues:
-----------------
* In nsMacTSMMessagePump.cpp, search for your changes with "if(" and change to 
"if ("
* Also, nsMacTSMMessagePump::UnicodeUpdateHandler seems to have tabs instead of 2 
spaces per indentation level.
* I find it easier to read parameters if there is a space after the ',' 
especially if there are other spaces like this partial line:
(unicodeTextPtr,text_size/2,fixLength / 2,hiliteRangePtr)
should be:
(unicodeTextPtr, text_size/2, fixLength / 2, hiliteRangePtr)
* Also, note the spacing inconsistency above with "text_size/2" & "fixLength / 2"
* Fix tabs/spaces issues in nsMacEventHandler.cpp
* in nsMacEventHandler::InitializeKeyEvent, indent additional parameters over to 
be under nsKeyEvent& aKeyEvent
* also in InitializeKeyEvent, would like to see new parameters start with 'a' so 
we'd have aIsChar and aConvertChar
* Add a blank line before the comment for HandleUKeyEvent

Code issues:
-----------
* In nsMacTSMMessagePump::UnicodeUpdateHandler, why does err4 condition return 
noErr instead of the error encountered from the AE call?
* In nsMacTSMMessagePump::UnicodeUpdateHandler, inside an #if TARGET_CARBON, you 
have "nsAutoString unicodeText;" in both the #if and the #else, it could be 
pulled outside of the #if/#else.
* Variable "actualType" is not used in 
nsMacTSMMessagePump::UnicodeNotFromInputMethodHandler
* It might help cleanup UnicodeUpdateHandler and UnicodeNotFromInputMethodHandler 
if they had a helper function to grab the event handler and the unicode text.  
Could this helper method also do the memcopy to the nsAutoString "unicodeText" or 
could that be moved to another helper?
* In nsMacEventHandler.cpp, +#define DEBUG_TSM should not be changed (should not 
be set to true).

Other issues:
------------
* Has anyone checked for memory leaks?  Leaking 2 bytes per keystroke while in 
IME can cause HUGE problems (as Kat Momoi found back in 4.0 version).
* Also, I would feel more comfortable if Kat, Teruko, or someone else also tested 
this for 15-20 minutes of solid typing, since (from what I've seen), different 
IME users have different styles for inputting.  Some people invoke the list more 
frequently or make corrections in different ways (arrows vs backspace).
* If several people have tested on both OS9 and OSX and found no major issues 
(regressions) and we are confident we have no memory leaks in this area, I am ok 
with checking in sooner (rather than waiting until after 0.9.4).

Comment 18

[Frank Tang]

shoot for m94

Comment 19

[Frank Tang]

tiantian: do we really need this for enterprise ? I don't think we can make it
for m0.9.4 . We can still type in Japanese/Chinese/Korean /French/German without
fixint this one. We just cannot type vietnamese....

move to m0.9.6. Please let me know if you really really need it for m0.9.4

Comment 20

[grega]

marking nsenterprise-

Comment 21

[Frank Tang]

Attachment: v2 of mozilla:widget:src:mac:nsTSMStrategy.cpp which address brade's comment

Comment 22

[Frank Tang]

Attachment: v2 of mozilla:widget:src:mac:nsTSMStrategy.h which address brade's comment

Comment 23

[Frank Tang]

Attachment: v3 of mozilla:widget:src:mac:nsTSMStrategy.cpp addressed brade's comment

Comment 24

[Frank Tang]

Attachment: v3 of mozilla:widget:src:mac:nsTSMStrategy.h addressed brade's comment

Comment 25

[Frank Tang]

Attachment: v3 of patch for other files, address brade's comment. Without ignore space

Comment 26

[Frank Tang]

Attachment: same patch as above (v3) except ignore space (-b) in cvs diff

Comment 27

[Kathleen :Brade]

Comment on attachment 52758 [details] [diff] [review]
v3 of mozilla:widget:src:mac:nsTSMStrategy.cpp addressed brade's comment

r=brade with whitespace changes we discussed

Comment 28

[Frank Tang]

Comment on attachment 52758 [details] [diff] [review]
v3 of mozilla:widget:src:mac:nsTSMStrategy.cpp addressed brade's comment

submit new patch

Comment 29

[Kathleen :Brade]

Comment on attachment 52759 [details] [diff] [review]
v3 of mozilla:widget:src:mac:nsTSMStrategy.h addressed brade's comment

r=brade if you add a blank line before the class definition

Comment 30

[Frank Tang]

Attachment: v4 of mozilla:widget:src:mac:nsTSMStrategy.cpp

Comment 31

[Kathleen :Brade]

Comment on attachment 52919 [details]
v4 of mozilla:widget:src:mac:nsTSMStrategy.cpp

r=brade

Comment 32

[Kathleen :Brade]

in nsMacEventHandler.cpp, 
 * add blank lines before "static PRBool gUseUnicodeAPI = PR_FALSE;"
   and after "gInitUseUnicodeAPI = PR_FALSE;"
 * change this line:
     if(tsmstrategy.UseUnicodeForInputMethod()) {
     if (tsmstrategy.UseUnicodeForInputMethod()) {
 * change this line:
     err = ::NewTSMDocument(1,supportedServices,&mTSMDocument,(long)this);
     err = ::NewTSMDocument(1, supportedServices, &mTSMDocument, (long)this);
 * change this line:
     mIMECompositionStr=nsnull;
     mIMECompositionStr = nsnull;
 * change this block:
     aKeyEvent.eventStructType = NS_KEY_EVENT;
     aKeyEvent.message     = aMessage;
     aKeyEvent.point.x      = 0;
     aKeyEvent.point.y      = 0;
     aKeyEvent.time        = PR_IntervalNow();
   to something like:
     aKeyEvent.eventStructType = NS_KEY_EVENT;
     aKeyEvent.message = aMessage;
     aKeyEvent.point.x = 0;
     aKeyEvent.point.y = 0;
     aKeyEvent.time = PR_IntervalNow();
   Do the same with this code (changes already made):
     aKeyEvent.widget = aFocusedWidget;
     aKeyEvent.nativeMsg = (void*)&aOSEvent;
   The modifier flags assignments can stay as they are.
 * Change this line (note removal of space after ';' too):
     *aIsChar =  PR_FALSE;
     *aIsChar = PR_FALSE;
 * Change this line:
     if ( aKeyEvent.isControl )
     if (aKeyEvent.isControl)
 * Change this line:
     ( aKeyEvent.charCode <= 26 )
     (aKeyEvent.charCode <= 26)
 * Change this line:
     ( aKeyEvent.isShift )
     (aKeyEvent.isShift)
 * Change this line:
     } // if ( aKeyEvent.charCode <= 26 )
     } // if (aKeyEvent.charCode <= 26)
 * Change this block:
       aKeyEvent.keyCode  = 0;
     } // if ( aKeyEvent.isControl )
     else // else for if ( aKeyEvent.isControl )
   to:
       aKeyEvent.keyCode = 0;
     } // if (aKeyEvent.isControl)
     else // else for if (aKeyEvent.isControl)
 * Change this line:
     if ( !aKeyEvent.isMeta)
     if (!aKeyEvent.isMeta)
 * Change this line:
     } // if ( !aKeyEvent.isMeta)
     } // if (!aKeyEvent.isMeta)
 * Change this line to 2 lines (to be more consistent with rest of that method):
     if (aConvertChar) {
     if (aConvertChar)
     {
 * Move the "{" down for the line that begins "if (aKeyEvent.isShift" also
 * Change this line:
     } // else for if ( aKeyEvent.isControl )
     } // else for if (aKeyEvent.isControl)
 * Change this line (in HandleUKeyEvent):
     nsKeyEvent  keyEvent;
     nsKeyEvent keyEvent;
 * Change this line:
     InitializeKeyEvent(keyEvent,aOSEvent, focusedWidget,NS_KEY_PRESS, ...
     InitializeKeyEvent(keyEvent, aOSEvent, focusedWidget, NS_KEY_PRESS, ...
 * Move this line above "PRUint32 i;" and fix typo in "unicode":
     // it is a message with text, send all the unciode characters
     // it is a message with text, send all the unicode characters
 * Insert a blank line after the HandleUKeyEvent method (before the #pragma)
 * Change this line:
     nsresult nsMacEventHandler::UnicodeHandleUpdateInputArea(PRUnichar* 
text,long charCount, long fixedLength,TextRangeArray* textRangeList)
   to these lines (or similar; be sure to add space after ','):
     nsresult nsMacEventHandler::UnicodeHandleUpdateInputArea(PRUnichar* text,
                                          long charCount, long fixedLength,
                                          TextRangeArray* textRangeList)
 * Change this line and move it down to the 2.2.2 section (where it's used):
     int          i;
     int i;
 * Every place you see this should be changed:
     if(NS_FAILED(res))
     if (NS_FAILED(res))
 * Change this line:
     if(nsnull == mIMECompositionStr)
     if (nsnull == mIMECompositionStr)

More in a little bit...

Comment 33

[Kathleen :Brade]

continuing...
 * Change this line (partial:  in->at and typo):
     in the same time. The easies way
     at the same time. The easiest way
 * Change this line:
     if(0 != committedLen)
     if (0 != committedLen)
 * Change this line:
     if((-1 != fixedLength) && (charCount != fixedLength ))
     if ((-1 != fixedLength) && (charCount != fixedLength))
 * Change this line (typo and missing space):
     printf("Have new uncommited text from %d to text_size(%d)\n", 
committedLen,charCount);
     printf("Have new uncommitted text from %d to text_size(%d)\n", committedLen, 
charCount);
 * Change this line (typo):
     // 2.1 send compositionEvent to start the comosition
     // 2.1 send compositionEvent to start the composition
 * Change this line (typo):
     // if we aren't in composition mode alredy, signal the backing store w/ ...
     // if we aren't in composition mode already, signal the backing store w/ ...
 * Change this line (typo):
     }   // 2.1 send compositionEvent to start the comosition
     }   // 2.1 send compositionEvent to start the composition
 * Remove extra spaces (2) from this line:
     nsTextRangeArray  xpTextRangeArray  = new nsTextRange[ ...
     nsTextRangeArray xpTextRangeArray = new nsTextRange[ ...
 * Add spaces (4) to this line and remove extra spaces from the end of the line:
     for(i=0; i<rangeArray->fNumOfRanges; i++) {
     for(i = 0; i < rangeArray->fNumOfRanges; i++) {
 * Remove this line; it shouldn't be needed (per sfraser's comments above):
     if(xpTextRangeArray)
   Fix indent of delete [] line (of course).
 * Add space on this line:
     else if((0==charCount) && (0==fixedLength))
     else if ((0==charCount) && (0==fixedLength))
 * Fix typo on this line (un->up):
     In that case, we need to send a text event to clean un the input hole....
     In that case, we need to send a text event to clean up the input hole....
 * Add space on this line:
     res = HandleTextEvent(0,nsnull);
     res = HandleTextEvent(0, nsnull);
 * Add blank line before this line:
     error:
 * Is there really no cleanup to do in any of the error conditions?  If so, why 
not follow convention and just "return res;" instead of having a goto?


In nsMacTSMMessagePump.cpp,
 * The spacing doesn't look correct on these 2 lines (are there tabs?):
    mUpdateUPP = NewAEEventHandlerUPP(nsMacTSMMessagePump::UnicodeUpdateHandler);
    mUpdateUPP = NewAEEventHandlerUPP(nsMacTSMMessagePump::UpdateHandler);
    mKeyboardUPP = NewAEEventHandlerUPP(nsMacTSMMessagePump::UnicodeNotFrom ...
    err = AEInstallEventHandler(kTextServiceClass, kUnicodeNotFromInputMethod ...
    NS_ASSERTION(err==noErr, "nsMacTSMMessagePump::InstallTSMAEHandlers: ...
    err = AERemoveEventHandler(kTextServiceClass, kUnicodeNotFromInputMethod ...
    NS_ASSERTION(err==noErr, "nsMacTSMMessagePump::InstallTSMAEHandlers: ...
    ::DisposeAEEventHandlerUPP(mKeyboardUPP);
 * After this line:
    ::DisposeAEEventHandlerUPP(mUpdateUPP);
   There is a blank line with some spaces on it; remove the spaces.

more in a little while...

Comment 34

[Kathleen :Brade]

in nsMacTSMMessagePump.cpp,
 * in AETextToString, you should use SetLength instead of SetCapacity (according 
to scc)
 * check for tabs in this file; do a search/replace by copying/pasting the tab 
into the find edit field of the dialog.
 * Remove extra space (2 different lines):
     EventRecord         event ;
     EventRecord         event;
 * Change these lines (remove extra spaces):
     err = AEGetParamPtr ( theAppleEvent, keyAETSMEventRecord, 
typeLowLevelEventRecord, & returnedType,
         & event, sizeof ( event ), & actualSize );
   to:
     err = AEGetParamPtr(theAppleEvent, keyAETSMEventRecord, 
typeLowLevelEventRecord, 
         &returnedType, &event, sizeof(event), &actualSize);
 * Check for the result on this line:
     AETextToString(text, unicodeText, text_size);

more in a little while... (sorry I have so many interruptions!)

Comment 35

[Kathleen :Brade]

Attachment: code snippet of some factoring that might be done (with other cleanup as well)

Comment 36

[Frank Tang]

Comment on attachment 52778 [details] [diff] [review]
v3 of patch for other files, address brade's comment. Without ignore space

see the new patch

Comment 37

[Frank Tang]

Attachment: v4 of the diff of existing files

Comment 38

[Frank Tang]

Attachment: v4 of the diff of existing files igore space changes (with -b in cvs diff)

Comment 39

[Frank Tang]

The last two attached diff include all the comment brade gave previously
one is cvs diff -u one is cvs diff -u -b so we can see the diff ignore space
chagnes. 
I do need to change GetAppleEventTSMData which proposed by brade in her attachment
to static instead of class private since I don't want to introduce
nsMacEventHandler in that header file. and there are no technical reason it
should be a class memeber instead of a file static helper function.

brade, can you code review it again ?

Comment 40

[Frank Tang]

The latest patch don't run, it crash at
+    res = eventHandler->HandleUKeyEvent((PRUnichar*)unicodeText.get(),
text_size , event);


I need to find out what happen about it later.
brade, can you still review the formating stuff? (or not) 

Comment 41

[Frank Tang]

Attachment: v5, fix the crash of getting the event handler from apple event.

Comment 42

[Frank Tang]

Comment on attachment 53036 [details] [diff] [review]
v4 of the diff of existing files

crash, see the v5 diff

Comment 43

[Frank Tang]

Comment on attachment 53037 [details] [diff] [review]
v4 of the diff of existing files igore space changes (with -b in cvs diff)

crash , see the v5 diff for the real one

Comment 44

[Frank Tang]

Comment on attachment 52965 [details]
code snippet of some factoring that might be done (with other cleanup as well)

integrate into v5 diff

Comment 45

[Frank Tang]

brade, can you put your review comment here ?
I find out what happen to the v4 patch. The code you attached does not compiled, 
and I fix it in a wrong way. The v5 patch won't crash. Both keyboard and IME 
work perfect on MacOS X build. Once we pass the format review, I will spend more 
time on testing on MacOS 9 and get sr.

Comment 46

[Kathleen :Brade]

Attachment: version 5.1 (whitespace cleanup and fix typos)

Comment 47

[Kathleen :Brade]

I attached a newer patch with some white space cleanup and fix some typos.  
Note:  I didn't change any lines that weren't already being touched for this bug.

There is one place in there where I added a comment with "ftang" in it.  I think
that issue should be addressed and then the comment removed.

Comment 48

[Frank Tang]

+      goto err2;  // FTANG:  SHOULD THIS REALLY BE:  goto err1;
Yes, it should

Comment 49

[Kathleen :Brade]

r=brade with version 5.1 and the fix noted in the previous comment
(as well as getting lots of testing)

Comment 50

[Frank Tang]

what we need now is
http://bugzilla.mozilla.org/attachment.cgi?id=52759&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=52919&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=53285&action=view (with one line
change from err2 to err1)

sfraser- can you sr= it ?

Comment 51

[Simon Fraser [no longer active]]

ftang: it would be easier for me to review this if -
* You include everything in a single patch -- MacCVS Pro can show new files in
  a patch if you cvs add them first.
* Attachment 53285 [details] [diff] would be much easier to read if you diff ignoring whitespace.

Comment 52

[Frank Tang]

Attachment: v3 of nsTSMStrategy.h + v4 of nsTSMStrategy.cpp + v5.1 of diff + (one line change err2 to err1)

Comment 53

[Frank Tang]

new patch is the same as the one reviewed by brade. I just combine them with
different cvs option. (Plus one fix from err2 to err1 as noted by brade)

Comment 54

[Frank Tang]

sfraser: please sr it 

Comment 55

[Simon Fraser [no longer active]]

Comment on attachment 53741 [details] [diff] [review]
v3 of nsTSMStrategy.h + v4 of nsTSMStrategy.cpp + v5.1 of diff + (one line change err2 to err1)

sr=sfraser

Comment 56

[Frank Tang]

fixed and check in .

Comment 57

[Greg K.]

This does appear to be working on Mac/2001112011 (0.9.6) on Mac OS 9. However,
the process is very awkward.

Suppose I select the Extended Roman (U) keyboard layout. If I switch to another
window and back, boom: the keyboard layout switches back to US.

This can't be how things are intended to work. If I switch to a Unicode keyboard
layout in Mozilla, it should stay that way, certainly for a given window, and
perhaps even for the application as a whole until changed.

Comment 58

[Frank Tang]

[e-mail]- file a seperate bug for that issue. It's seems a different bug.