Last Comment Bug 95478 - upgrade Navigation Services to use Unicode directly on MacOS X
: upgrade Navigation Services to use Unicode directly on MacOS X
Status: RESOLVED FIXED
MacOS X
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: mozilla0.9.9
Assigned To: nhottanscp
: John Morrison
Mentors:
: 97203 (view as bug list)
Depends on:
Blocks: 103669
  Show dependency treegraph
 
Reported: 2001-08-15 13:33 PDT by Frank Tang
Modified: 2002-01-29 16:46 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A part of the fix, passing title as unicode. (6.20 KB, patch)
2001-12-20 13:05 PST, nhottanscp
no flags Details | Diff | Splinter Review
For Carbon only change to use Navigation Services 3.0 for GetLocalFile, GetLocalFolder, PutLocalFile. (17.02 KB, patch)
2002-01-23 11:01 PST, nhottanscp
no flags Details | Diff | Splinter Review
patch '-u' (13.90 KB, patch)
2002-01-23 11:56 PST, nhottanscp
no flags Details | Diff | Splinter Review
Changed to get encoding from system script instead of the encoding hint. (14.00 KB, patch)
2002-01-24 14:38 PST, nhottanscp
ccarlen: review+
Details | Diff | Splinter Review
A new patch addresses simon's comment. (15.19 KB, patch)
2002-01-28 15:57 PST, nhottanscp
sfraser_bugs: superreview+
Details | Diff | Splinter Review

Description Frank Tang 2001-08-15 13:33:01 PDT
On MacOS X, the navigator service support Unicode. We should migrate our code to 
use the Unicode apis.

see 
"Changes in Navigation Services for Carbon" of "Navigation Services for Carbon: 
An Overview "
http://gemma.apple.com/techpubs/macosx/Carbon/Files/NavigationServices/
Navigation_Services/Concepts/Navigation_Services-2.html

"Navigation Services Reference"
http://gemma.apple.com/techpubs/macosx/Carbon/Files/NavigationServices/
Navigation_Services_Ref/index.html

It looks like we should change to use the folliowng apis:
NavCreateChooseFileDialog (from NavChooseFile )
NavCreateChooseFolderDialog ( from NavChooseFolder  )
NavCreateChooseObjectDialog ( from  NavChooseObject )
NavCreateChooseVolumeDialog ( from  NavChooseVolume )
NavCreateGetFileDialog  ( from  NavGetFile )
NavCreateNewFolderDialog ( from  NavNewFolder ) 
NavCreateAskDiscardChangesDialog ( from  NavAskDiscardChanges )
NavCreateAskSaveChangesDialog ( from NavAskSaveChanges  )
NavCreatePutFileDialog  ( from  NavPutFile )
NavDialogGetSaveFileName ( from kNavGetEditFileName selector with the 
NavCustomControl)
NavDialogSetSaveFileName ( from kNavSetEditFileName selector with the 
NavCustomControl function )
Comment 1 Frank Tang 2001-08-15 13:40:45 PDT
see 95481 about file system issues 
Comment 2 Frank Tang 2001-08-15 13:50:02 PDT
OK, here are the places currently uwe call the old api, we should consider to 
migrate to the new Unicode based apis:
/xpinstall/packager/mac/ASEncoder/src/nsFileSelector.cpp, line 88 -- err = 
NavChooseFile( &initDesc, &reply, &dlgOpts, eventProc, NULL, NULL, NULL, NULL ); 
/xpinstall/packager/mac/ASEncoder/src/nsFileSelector.cpp, line 132 -- err = 
NavChooseFolder( &initDesc, &reply, &dlgOpts, eventProc, NULL, NULL );
/xpinstall/wizard/mac/src/SetupTypeWin.c, line 281 -- /* NavChooseFolder vars */
/xpinstall/wizard/mac/src/SetupTypeWin.c, line 354 -- err = NavChooseFolder( &
initDesc, &reply, &dlgOpts, eventProc, NULL, NULL );
/widget/src/mac/nsFilePicker.cpp, line 373 -- anErr = ::NavChooseFolder(
/widget/src/mac/nsFileWidget.cpp, line 399 -- anErr = ::NavChooseFolder(


/widget/src/mac/nsFilePicker.cpp, line 304 -- anErr = ::NavGetFile(
/widget/src/mac/nsFileWidget.cpp, line 329 -- anErr = ::NavGetFile(



/widget/src/mac/nsFilePicker.cpp, line 432 -- anErr = ::NavPutFile(
/widget/src/mac/nsFileWidget.cpp, line 254 -- anErr = ::NavPutFile(
Comment 3 Frank Tang 2001-08-15 13:52:28 PDT
I don't think we should spend time on either
xpinstall/wizard/mac/src/ 
or
/widget/src/mac/nsFileWidget.cpp (Is it 100% obsoleted?)
I think we should focus on 
/widget/src/mac/nsFilePicker.cpp

add pavlov to the list since he wrote /widget/src/mac/nsFilePicker.cpp
Comment 4 Stuart Parmenter 2001-08-15 14:20:30 PDT
/widget/src/mac/nsFilePicker.cpp is what you want.

nsFileWidget is obsolete and I believe bryner is in the process of removing it 
from the tree (along with nsFileSpecWithUI).
Comment 5 Frank Tang 2001-08-15 22:45:58 PDT
also see http://developer.apple.com/technotes/tn/tn2022.html
TN2022 The Death of typeFSSpec: moving along to typeFileURL
Comment 6 Frank Tang 2001-08-16 01:28:33 PDT
looks like the following protected method should be change first

PRInt16 PutLocalFile(Str255 & inTitle, Str255 & inDefaultName, FSSpec*
outFileSpec) ;
PRInt16 GetLocalFile(Str255 & inTitle, FSSpec* outFileSpec);
PRInt16 GetLocalFolder(Str255 & inTitle, FSSpec* outFileSpec);

It will be easier to fix this bug if we change these method for both
carbon/non-carbon build to


PRInt16 PutLocalFile(nsString & inTitle, nsString& inDefaultName, FSSpec*
outFileSpec) ;
PRInt16 GetLocalFile(nsString & inTitle, FSSpec* outFileSpec);
PRInt16 GetLocalFolder(nsString & inTitle, FSSpec* outFileSpec);

we can push
133 nsMacControl::StringToStr255(mTitle,title);
134 nsMacControl::StringToStr255(mDefault,defaultName);

into these method for non-carbon build instead. and for carbon build, use the
CFString version of Nav Service APIs which take Unicode directly.
Comment 7 Frank Tang 2001-08-16 01:30:34 PDT
not sure I want to mass around this nsFilePicker code personally, any volunteers?
Comment 8 Frank Tang 2001-08-16 01:44:54 PDT
It looks there are other benefit that we should move to the new APIs in
additional to using Unicode as file name and button in options.

struct NavDialogCreationOptions have member CFArrayRef popupExtension; 
so it can control the filter 
 
Comment 9 Frank Tang 2001-08-21 15:37:19 PDT
not an easy job, move to m96 unless someone else volunteer for making it eariler.
Comment 10 Frank Tang 2001-08-31 13:23:43 PDT
*** Bug 97203 has been marked as a duplicate of this bug. ***
Comment 11 Frank Tang 2001-10-08 10:42:34 PDT
move target to --- since there are no real owner yet.
Comment 12 Frank Tang 2001-10-12 16:46:46 PDT
change to XP Toolkit now. Peter's group, can you assign someone to work on this ? 
Pinkerton ???
Comment 13 David Hyatt 2001-10-13 12:32:07 PDT
--> saari
Comment 14 saari (gone) 2001-10-14 18:19:10 PDT
->Pinkerton, potential MacOS integration stuff. Target as you see fit.
Comment 15 Mike Pinkerton (not reading bugmail) 2001-10-15 07:56:33 PDT
over to dagley, who wrote most of the filePicker.
Comment 16 Frank Tang 2001-10-19 14:56:25 PDT
dagley is going to sabbitical next week. reassign it back to pinkerton.
Comment 17 nhottanscp 2001-11-29 11:06:10 PST
I am attending Apple's Carbon kitchen and I did some experiments using 
new navigation functions.

One benefit of using the new navigation is that we can  do document 
modal dialogs instead of app modal, but it requires WindowRef which we 
don't currently pass into.

About files, it supports both FSRef and FSSpec.
Also conversions are supported.

* From FSRef to FSSpec,
	err = FSGetCatalogInfo(
			  &theFSRef, //const FSRef *         ref,
			  kFSCatInfoNone, //FSCatalogInfoBitmap   whichInfo,
			  NULL, //FSCatalogInfo *       catalogInfo,       /* can be NULL */
			  NULL, //HFSUniStr255 *        outName,           /* can be NULL */
			  &convertedFSSpec, //FSSpec *              fsSpec,            /* can be NULL */
			  NULL /*FSRef *               parentRef*/);         /* can be NULL */

* From FSRef to CFURLRef (Core Foundation URL),
theURLRef = CFURLCreateFromFSRef(NULL, &theFSRef);

Both FSRef and CFURLRef supports unicode which is not depending OS 
system charset.
Because we can convert FSRef to FSSpec, I think we can gradually 
migrate to FSRef instead of changing our code of using FSSpec at once.
Comment 18 Frank Tang 2001-12-20 12:53:13 PST
reassign to nhotta.
nhotta, can you make a patch to use CFString. Ignore the document model and
other issue for now. 
Comment 19 nhottanscp 2001-12-20 13:05:31 PST
Created attachment 62411 [details] [diff] [review]
A part of the fix, passing title as unicode.
Comment 20 nhottanscp 2002-01-14 17:37:55 PST
The current plan for 0.9.9 is to change the argument (the current patch) and
change to use new API instead of NavGetFile/NavPutFile, no plan to do the
unicode file name support for 0.9.9 (i.e. use FSSpec).
Cc to sdagley@netscape.com. Steve, does that sound okay?




Comment 21 Steve Dagley 2002-01-14 18:33:34 PST
Yes, we're going to have to get away from FSSpec's in the long run since they 
won't really allow us to use long file names.  One word of warning on FSRefs 
though - they can only refer to objects that exist.
Comment 22 Conrad Carlen (not reading bugmail) 2002-01-14 18:41:40 PST
That's right. See comment #5. nsLocalFile on the Mac will support that when bug
95481 is fixed.
Comment 23 nhottanscp 2002-01-23 11:01:02 PST
Created attachment 66141 [details] [diff] [review]
For Carbon only change to use Navigation Services 3.0 for  GetLocalFile, GetLocalFolder, PutLocalFile.
Comment 24 Steve Dagley 2002-01-23 11:29:17 PST
! nsFilePicker::PutLocalFile(nsString & inTitle, nsString & inDefaultName, 
FSSpec* outFileSpec)
  {
        PRInt16 retVal = returnCancel;
***************
*** 436,441 ****
                dialogOptions.dialogOptionFlags |= kNavDontAddTranslateItems;
                dialogOptions.dialogOptionFlags ^= kNavAllowMultipleFiles;
!               ::BlockMoveData(inTitle, dialogOptions.windowTitle, *inTitle + 
1);
!               ::BlockMoveData(inDefaultName, dialogOptions.savedFileName, *
inDefaultName + 1);

this looks to me like your treating inTitle as a Str255 rather than a nsString & 
- wouldn't that be bad?
Comment 25 Simon Fraser 2002-01-23 11:34:28 PST
Ugh, diff hard to read. Can you do a diff -u please? At first glance, I see a lot 
of nsString& parameters that look like they should be |const|.
Comment 26 nhottanscp 2002-01-23 11:47:57 PST
Steve, the code you mentioned is the old part, I will repost with '-u' option.
Comment 27 nhottanscp 2002-01-23 11:56:17 PST
Created attachment 66157 [details] [diff] [review]
patch '-u'
Comment 28 Conrad Carlen (not reading bugmail) 2002-01-24 12:17:30 PST
Comment on attachment 66157 [details] [diff] [review]
patch '-u'

>+      if (anErr == noErr && reply.validRecord) {
>+        AEKeyword   theKeyword;
>+        DescType    actualType;
>+        Size        actualSize;
>+        FSSpec      theFSSpec;
>+        FSRef       theFSRef;
>+        FSCatalogInfo catalogInfo;
>+
    These comments should be fixed. If you're not using typeFSS, why comment
about it?

>+        // The returned FSSpec by 'typeFSS' contains the directory info, 
>+        // 'saveFileName' contains the file name.
>+        // 'saveFileName' is CFString, need conversion before assigning to FSSpec.
>+        // Get the encoding through FSRef using FSGetCatalogInfo, then convert to a pascal string.
>+        // Also 'parID' of the spec contains the parent of the directory where the file to be saved.
>+        // But what we need is the 'nodeID' of the directory.
>+        // So I skip calling with 'typeFSS' and make FSSpec from FSRef.
>+        anErr = AEGetNthPtr(&(reply.selection), 1, typeFSRef, &theKeyword, &actualType,
>+                            &theFSRef, sizeof(theFSRef), &actualSize);
>+        if (anErr == noErr) {
>+          anErr = ::FSGetCatalogInfo(
>+                                  &theFSRef,
>+                                  kFSCatInfoTextEncoding | kFSCatInfoNodeID,
>+                                  &catalogInfo,
>+                                  NULL,
>+                                  &theFSSpec,
>+                                  NULL);
>+          if (anErr == noErr) {
>+            ::CFStringGetPascalString(
>+                                    reply.saveFileName,
>+                                    theFSSpec.name,
>+                                    sizeof(theFSSpec.name),
>+                                    catalogInfo.textEncodingHint);

    Is the textEncodingHint field giving expected results? Last I looked into
this,
    it wasn't. Also, if you instead use the system script, it be possible to
convert
    this string again in nsLocalFileMac. Whether that code uses uconv or OS
Unicode
    conversion code, it won't be able to know what encoding was used here on
this
    particular node.

>+			      theFSSpec.parID = catalogInfo.nodeID;
>+    				
>+    				*outFileSpec = theFSSpec;	// Return the FSSpec
>+
Comment 29 nhottanscp 2002-01-24 13:58:02 PST
I wrote the comment to explain why I did not use typeFSS unlike GetFile. I am
going to remove it.
textEncodingHint returns the encoding when the file was created. This is useful,
you can support the file name even after the user switches to a different locale.
But for PutFile, it seems always returns 0. So you are right, I need to fix that
part to use the system script.

Comment 30 nhottanscp 2002-01-24 14:38:49 PST
Created attachment 66354 [details] [diff] [review]
Changed to get encoding from system script instead of the encoding hint.
Comment 31 Conrad Carlen (not reading bugmail) 2002-01-24 16:12:56 PST
Comment on attachment 66354 [details] [diff] [review]
Changed to get encoding from system script instead of the encoding hint.

r=ccarlen
Comment 32 nhottanscp 2002-01-24 17:59:34 PST
Simon, could you sr?
Comment 33 Simon Fraser 2002-01-28 12:03:46 PST
Comments:

+  PRInt16 PutLocalFile(const nsString & inTitle,const  nsString & inDefaultName, 
FSSpec* outFileSpec) ;

Missing and extraneous spaces.

+  if ( mAllFilesDisplayed )
+    dialogCreateOptions.optionFlags |= kNavSupportPackages;            

Does the code do the right thing when you choose a package? In this case, Nav 
services returns an FSSpec whose parID is the directory ID of the package, and 
whose name is an empty string. Normally, to match what you get for a regular 
application file, you'd want to return an FSSpec whose parID is the parent 
directory, and whose name is the name of the package. Where does this logic live?

+  OSType typeToSave = 'TEXT';
+  OSType creatorToSave = 'MOZZ';

'MOZZ' should be 'MOSS' for commercial builds. nsLocalFileMac has some code that 
gets this from the process info. You need to do the same.

+                         // Get the FSRef for the file to be saved

This comment is confusing. The FSRef actually points to the parent directory, 
where the file is going to be saved, so the comment should reflect that. This 
also clarifies any confusion that might come from the fact that FSRefs cannot 
refer to non-existent file system entities.
Comment 34 nhottanscp 2002-01-28 15:57:10 PST
Created attachment 66821 [details] [diff] [review]
A new patch addresses simon's comment.
Comment 35 Simon Fraser 2002-01-28 16:10:38 PST
Comment on attachment 66821 [details] [diff] [review]
A new patch addresses simon's comment.

sr=sfraser
Comment 36 nhottanscp 2002-01-29 16:46:40 PST
checked in

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