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 )

see 95481 about file system issues

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(

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

/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).

also see http://developer.apple.com/technotes/tn/tn2022.html TN2022 The Death of typeFSSpec: moving along to typeFileURL

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.

not sure I want to mass around this nsFilePicker code personally, any volunteers?

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

not an easy job, move to m96 unless someone else volunteer for making it eariler.

*** Bug 97203 has been marked as a duplicate of this bug. ***

move target to --- since there are no real owner yet.

change to XP Toolkit now. Peter's group, can you assign someone to work on this ? Pinkerton ???

--> saari

->Pinkerton, potential MacOS integration stuff. Target as you see fit.

over to dagley, who wrote most of the filePicker.

dagley is going to sabbitical next week. reassign it back to pinkerton.

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.

reassign to nhotta. nhotta, can you make a patch to use CFString. Ignore the document model and other issue for now.

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?

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.

That's right. See comment #5. nsLocalFile on the Mac will support that when bug 95481 is fixed.

! 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?

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|.

Steve, the code you mentioned is the old part, I will repost with '-u' option.

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 >+

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 on attachment 66354 [details] [diff] [review] Changed to get encoding from system script instead of the encoding hint. r=ccarlen

Simon, could you sr?

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 on attachment 66821 [details] [diff] [review] A new patch addresses simon's comment. sr=sfraser

checked in