95478 - upgrade Navigation Services to use Unicode directly on MacOS X

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Frank Tang]

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]

see 95481 about file system issues 

Comment 2

[Frank Tang]

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]

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]

/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]

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

Comment 6

[Frank Tang]

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]

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

Comment 8

[Frank Tang]

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]

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

Comment 10

[Frank Tang]

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

Comment 11

[Frank Tang]

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

Comment 12

[Frank Tang]

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

Comment 13

[David Hyatt]

--> saari

Comment 14

[saari (gone)]

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

Comment 15

[Mike Pinkerton (not reading bugmail)]

over to dagley, who wrote most of the filePicker.

Comment 16

[Frank Tang]

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

Comment 17

[nhottanscp]

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]

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

Comment 19

[nhottanscp]

Attachment: A part of the fix, passing title as unicode.

Comment 20

[nhottanscp]

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 [e-mail]. Steve, does that sound okay?

Comment 21

[Steve Dagley]

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)]

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

Comment 23

[nhottanscp]

Attachment: For Carbon only change to use Navigation Services 3.0 for GetLocalFile, GetLocalFolder, PutLocalFile.

Comment 24

[Steve Dagley]

! 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 [no longer active]]

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]

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

Comment 27

[nhottanscp]

Attachment: patch '-u'

Comment 28

[Conrad Carlen (not reading bugmail)]

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]

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]

Attachment: Changed to get encoding from system script instead of the encoding hint.

Comment 31

[Conrad Carlen (not reading bugmail)]

Comment on attachment 66354 [details] [diff] [review]
Changed to get encoding from system script instead of the encoding hint.

r=ccarlen

Comment 32

[nhottanscp]

Simon, could you sr?

Comment 33

[Simon Fraser [no longer active]]

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]

Attachment: A new patch addresses simon's comment.

Comment 35

[Simon Fraser [no longer active]]

Comment on attachment 66821 [details] [diff] [review]
A new patch addresses simon's comment.

sr=sfraser

Comment 36

[nhottanscp]

checked in