Last Comment Bug 23363 - [FEATURE]Enable Unicode on TSM1.5
: [FEATURE]Enable Unicode on TSM1.5
Status: RESOLVED FIXED
: helpwanted
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: PowerPC Mac System 8.6
: P3 normal (vote)
: mozilla0.9.6
Assigned To: Frank Tang
: Frank Tang
:
Mentors:
http://developer.apple.com/techpubs/m...
Depends on: 77038
Blocks: 103669 115887 284251
  Show dependency treegraph
 
Reported: 2000-01-07 12:08 PST by Frank Tang
Modified: 2005-03-03 14:31 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch to migrate to Unicode Input Method on TSM1.5 (17.65 KB, patch)
2001-08-09 19:02 PDT, Frank Tang
no flags Details | Diff | Splinter Review
widget/src/mac/nsTSMStrategy.cpp (1.90 KB, text/plain)
2001-08-15 17:32 PDT, Frank Tang
no flags Details
widget/src/mac/nsTSMStrategy.h (1.16 KB, text/plain)
2001-08-15 17:32 PDT, Frank Tang
no flags Details
patch v2 include keyboard stuff (25.07 KB, patch)
2001-08-15 17:35 PDT, Frank Tang
no flags Details | Diff | Splinter Review
v2 of mozilla:widget:src:mac:nsTSMStrategy.cpp which address brade's comment (2.83 KB, text/plain)
2001-10-09 11:50 PDT, Frank Tang
no flags Details
v2 of mozilla:widget:src:mac:nsTSMStrategy.h which address brade's comment (2.10 KB, text/plain)
2001-10-09 11:51 PDT, Frank Tang
no flags Details
v3 of mozilla:widget:src:mac:nsTSMStrategy.cpp addressed brade's comment (2.87 KB, patch)
2001-10-09 12:52 PDT, Frank Tang
brade: review+
Details | Diff | Splinter Review
v3 of mozilla:widget:src:mac:nsTSMStrategy.h addressed brade's comment (2.11 KB, patch)
2001-10-09 12:53 PDT, Frank Tang
brade: review+
Details | Diff | Splinter Review
v3 of patch for other files, address brade's comment. Without ignore space (41.12 KB, patch)
2001-10-09 14:31 PDT, Frank Tang
no flags Details | Diff | Splinter Review
same patch as above (v3) except ignore space (-b) in cvs diff (36.67 KB, patch)
2001-10-09 14:32 PDT, Frank Tang
no flags Details | Diff | Splinter Review
v4 of mozilla:widget:src:mac:nsTSMStrategy.cpp (2.89 KB, text/plain)
2001-10-10 10:33 PDT, Frank Tang
brade: review+
Details
code snippet of some factoring that might be done (with other cleanup as well) (4.64 KB, text/plain)
2001-10-10 14:37 PDT, Kathleen Brade
no flags Details
v4 of the diff of existing files (41.76 KB, patch)
2001-10-10 18:54 PDT, Frank Tang
no flags Details | Diff | Splinter Review
v4 of the diff of existing files igore space changes (with -b in cvs diff) (38.61 KB, patch)
2001-10-10 18:55 PDT, Frank Tang
no flags Details | Diff | Splinter Review
v5, fix the crash of getting the event handler from apple event. (41.39 KB, patch)
2001-10-11 10:35 PDT, Frank Tang
no flags Details | Diff | Splinter Review
version 5.1 (whitespace cleanup and fix typos) (41.40 KB, patch)
2001-10-12 11:06 PDT, Kathleen Brade
no flags Details | Diff | Splinter Review
v3 of nsTSMStrategy.h + v4 of nsTSMStrategy.cpp + v5.1 of diff + (one line change err2 to err1) (43.75 KB, patch)
2001-10-16 06:00 PDT, Frank Tang
sfraser_bugs: superreview+
Details | Diff | Splinter Review

Description Frank Tang 2000-01-07 12:08:46 PST
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 2000-01-07 12:13:59 PST
Not a beta feature, Mark it M20., Anyone from the net want to help ??
Comment 2 Frank Tang 2000-07-30 21:11:59 PDT
mark it as future.
Comment 3 Frank Tang 2001-01-16 17:27:37 PST
IME related issue. REassign to yokoyama
Comment 4 Jean-Marc Desperrier 2001-02-14 09:43:42 PST
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 2001-04-22 16:20:59 PDT
Mac issue: assigning to nhotta@netscape.com
Comment 6 nhottanscp 2001-07-11 17:24:37 PDT
Reassign to ftang.
Comment 7 Frank Tang 2001-07-12 01:58:42 PDT
mark it as assigned
Comment 8 Frank Tang 2001-08-09 19:01:27 PDT
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 2001-08-09 19:02:08 PDT
Created attachment 45345 [details] [diff] [review]
patch to migrate to Unicode Input Method on TSM1.5
Comment 10 Frank Tang 2001-08-09 19:46:24 PDT
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 2001-08-10 10:17:29 PDT
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 2001-08-15 17:32:08 PDT
Created attachment 46013 [details]
widget/src/mac/nsTSMStrategy.cpp
Comment 13 Frank Tang 2001-08-15 17:32:28 PDT
Created attachment 46014 [details]
widget/src/mac/nsTSMStrategy.h
Comment 14 Frank Tang 2001-08-15 17:35:20 PDT
Created attachment 46015 [details] [diff] [review]
patch v2 include keyboard stuff
Comment 15 Frank Tang 2001-08-15 17:36:15 PDT
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 2001-08-16 10:52:04 PDT
not sure which milastone I want to land this. brade@netscape.com, can you r=
this one?
Maybe we should wait till m94 branch off. 
Comment 17 Kathleen Brade 2001-08-16 11:54:20 PDT
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 2001-08-21 15:42:05 PDT
shoot for m94
Comment 19 Frank Tang 2001-08-31 11:26:52 PDT
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 2001-09-17 11:16:53 PDT
marking nsenterprise-
Comment 21 Frank Tang 2001-10-09 11:50:26 PDT
Created attachment 52740 [details]
v2 of mozilla:widget:src:mac:nsTSMStrategy.cpp which address brade's comment
Comment 22 Frank Tang 2001-10-09 11:51:00 PDT
Created attachment 52741 [details]
v2 of mozilla:widget:src:mac:nsTSMStrategy.h which address brade's comment
Comment 23 Frank Tang 2001-10-09 12:52:47 PDT
Created attachment 52758 [details] [diff] [review]
v3 of mozilla:widget:src:mac:nsTSMStrategy.cpp addressed brade's comment
Comment 24 Frank Tang 2001-10-09 12:53:42 PDT
Created attachment 52759 [details] [diff] [review]
v3 of mozilla:widget:src:mac:nsTSMStrategy.h addressed brade's comment
Comment 25 Frank Tang 2001-10-09 14:31:17 PDT
Created attachment 52778 [details] [diff] [review]
v3 of patch for other files, address brade's comment. Without ignore space
Comment 26 Frank Tang 2001-10-09 14:32:01 PDT
Created attachment 52779 [details] [diff] [review]
same patch as above (v3) except ignore space (-b) in cvs diff
Comment 27 Kathleen Brade 2001-10-10 10:30:59 PDT
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 2001-10-10 10:31:36 PDT
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 2001-10-10 10:32:25 PDT
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 2001-10-10 10:33:15 PDT
Created attachment 52919 [details]
v4 of mozilla:widget:src:mac:nsTSMStrategy.cpp
Comment 31 Kathleen Brade 2001-10-10 10:40:06 PDT
Comment on attachment 52919 [details]
v4 of mozilla:widget:src:mac:nsTSMStrategy.cpp

r=brade
Comment 32 Kathleen Brade 2001-10-10 11:13:19 PDT
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 2001-10-10 12:20:46 PDT
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 2001-10-10 13:50:15 PDT
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 2001-10-10 14:37:51 PDT
Created attachment 52965 [details]
code snippet of some factoring that might be done (with other cleanup as well)
Comment 36 Frank Tang 2001-10-10 18:50:01 PDT
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 2001-10-10 18:54:41 PDT
Created attachment 53036 [details] [diff] [review]
v4 of the diff of existing files
Comment 38 Frank Tang 2001-10-10 18:55:28 PDT
Created attachment 53037 [details] [diff] [review]
v4 of the diff of existing files igore space changes (with -b in cvs diff)
Comment 39 Frank Tang 2001-10-10 18:58:15 PDT
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 2001-10-10 19:22:36 PDT
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 2001-10-11 10:35:54 PDT
Created attachment 53109 [details] [diff] [review]
v5, fix the crash of getting the event handler from apple event.
Comment 42 Frank Tang 2001-10-11 16:11:11 PDT
Comment on attachment 53036 [details] [diff] [review]
v4 of the diff of existing files

crash, see the v5 diff
Comment 43 Frank Tang 2001-10-11 16:11:35 PDT
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 2001-10-11 16:12:03 PDT
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 2001-10-11 16:13:45 PDT
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 2001-10-12 11:06:02 PDT
Created attachment 53285 [details] [diff] [review]
version 5.1 (whitespace cleanup and fix typos)
Comment 47 Kathleen Brade 2001-10-12 11:08:06 PDT
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 2001-10-12 12:05:03 PDT
+      goto err2;  // FTANG:  SHOULD THIS REALLY BE:  goto err1;
Yes, it should
Comment 49 Kathleen Brade 2001-10-15 10:44:23 PDT
r=brade with version 5.1 and the fix noted in the previous comment
(as well as getting lots of testing)
Comment 51 Simon Fraser 2001-10-15 12:04:55 PDT
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 2001-10-16 06:00:29 PDT
Created attachment 53741 [details] [diff] [review]
v3 of nsTSMStrategy.h + v4 of nsTSMStrategy.cpp + v5.1 of diff + (one line change err2 to err1)
Comment 53 Frank Tang 2001-10-16 06:02:18 PDT
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 2001-10-16 06:02:42 PDT
sfraser: please sr it 
Comment 55 Simon Fraser 2001-10-16 11:43:36 PDT
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 2001-10-19 13:55:37 PDT
fixed and check in .
Comment 57 Greg K. 2001-12-15 23:30:34 PST
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 2001-12-18 11:32:22 PST
greg@tcp.com- file a seperate bug for that issue. It's seems a different bug. 

Note You need to log in before you can comment on or make changes to this bug.