The default bug view has changed. See this FAQ.

upgrade Navigation Services to use Unicode directly on MacOS X

RESOLVED FIXED in mozilla0.9.9

Status

()

Core
XUL
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: Frank Tang, Assigned: nhottanscp)

Tracking

Trunk
mozilla0.9.9
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: MacOS X)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

16 years ago
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 )
(Reporter)

Comment 1

16 years ago
see 95481 about file system issues 
(Reporter)

Comment 2

16 years ago
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(
(Reporter)

Comment 3

16 years ago
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

16 years ago
/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).

Updated

16 years ago
Summary: upgrade Navigator service to use Unicode directly on MacOS X → upgrade Navigation Services to use Unicode directly on MacOS X
(Reporter)

Comment 5

16 years ago
also see http://developer.apple.com/technotes/tn/tn2022.html
TN2022 The Death of typeFSSpec: moving along to typeFileURL
(Reporter)

Comment 6

16 years ago
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.
Whiteboard: MacOS X
(Reporter)

Comment 7

16 years ago
not sure I want to mass around this nsFilePicker code personally, any volunteers?
(Reporter)

Comment 8

16 years ago
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 
 
(Reporter)

Updated

16 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 9

16 years ago
not an easy job, move to m96 unless someone else volunteer for making it eariler.
Target Milestone: --- → mozilla0.9.6
(Reporter)

Comment 10

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

Updated

16 years ago
QA Contact: andreasb → ylong
(Reporter)

Comment 11

16 years ago
move target to --- since there are no real owner yet.
Target Milestone: mozilla0.9.6 → ---
(Reporter)

Updated

16 years ago
Blocks: 103669
(Reporter)

Comment 12

16 years ago
change to XP Toolkit now. Peter's group, can you assign someone to work on this ? 
Pinkerton ???
Assignee: ftang → hyatt
Status: ASSIGNED → NEW
Component: Internationalization → XP Toolkit/Widgets
QA Contact: ylong → jrgm

Comment 13

16 years ago
--> saari
Assignee: hyatt → saari

Comment 14

16 years ago
->Pinkerton, potential MacOS integration stuff. Target as you see fit.
Assignee: saari → pinkerton
over to dagley, who wrote most of the filePicker.
Assignee: pinkerton → sdagley
(Reporter)

Comment 16

16 years ago
dagley is going to sabbitical next week. reassign it back to pinkerton.
Assignee: sdagley → pinkerton
(Assignee)

Comment 17

16 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1
(Reporter)

Comment 18

16 years ago
reassign to nhotta.
nhotta, can you make a patch to use CFString. Ignore the document model and
other issue for now. 
Assignee: pinkerton → nhotta
Status: ASSIGNED → NEW
Target Milestone: mozilla1.1 → ---
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
(Assignee)

Comment 19

16 years ago
Created attachment 62411 [details] [diff] [review]
A part of the fix, passing title as unicode.
(Assignee)

Comment 20

15 years ago
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

15 years ago
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.
(Assignee)

Comment 23

15 years ago
Created attachment 66141 [details] [diff] [review]
For Carbon only change to use Navigation Services 3.0 for  GetLocalFile, GetLocalFolder, PutLocalFile.
Attachment #62411 - Attachment is obsolete: true

Comment 24

15 years ago
! 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

15 years ago
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|.
(Assignee)

Comment 26

15 years ago
Steve, the code you mentioned is the old part, I will repost with '-u' option.
(Assignee)

Comment 27

15 years ago
Created attachment 66157 [details] [diff] [review]
patch '-u'
Attachment #66141 - Attachment is obsolete: true
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
>+
(Assignee)

Comment 29

15 years ago
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.

(Assignee)

Comment 30

15 years ago
Created attachment 66354 [details] [diff] [review]
Changed to get encoding from system script instead of the encoding hint.
Attachment #66157 - Attachment is obsolete: true
Comment on attachment 66354 [details] [diff] [review]
Changed to get encoding from system script instead of the encoding hint.

r=ccarlen
Attachment #66354 - Flags: review+
(Assignee)

Comment 32

15 years ago
Simon, could you sr?

Comment 33

15 years ago
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.
(Assignee)

Comment 34

15 years ago
Created attachment 66821 [details] [diff] [review]
A new patch addresses simon's comment.
Attachment #66354 - Attachment is obsolete: true

Comment 35

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

sr=sfraser
Attachment #66821 - Flags: superreview+
(Assignee)

Comment 36

15 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.