Closed Bug 55891 Opened 25 years ago Closed 25 years ago

Saved files have incorrect type/creator information

Categories

(Core :: XPCOM, defect, P3)

Product:
Core
Component:
XPCOM
Platform:
PowerPC
Mac System 8.5
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: sfraser_bugs, Assigned: ccarlen)

References

Details

(Whiteboard: [rtm need info] SR=sfraser, NEED R= brade)

Attachments

(4 files)

patch to add current process creator code
2.52 KB, patch
Details | Diff | Splinter Review
patch which does type and creator
7.10 KB, patch
Details | Diff | Splinter Review
patch which fixes nsIFileStream too
8.43 KB, patch
Details | Diff | Splinter Review
as stuffit file
26.32 KB, application/octet-stream
Details
When saving files from Mozilla/Netscape 6, we fail to set type/creator information on files that we create. This causes a number of serious usability problems for Mac users: 1. Can't double-click or drag-drop files onto other applications to open them (e.g. JPEG in JPEGView) 2. Can't double-click saved HTML files to open them in Mozilla/Netscape. 3. Files will not show up in Open dialogs of other applications. etc. In addition, some files created by Mozilla do have type/creator info, but use 'MOSS' (the Netscape signature) instead of 'MOZZ' (the Mozilla signature). We should be consistent.
Nominate for RTM, nsmac1 keyword.
Keywords: nsmac1, rtm
I was under the impression, as per a previous MacDev meeting, that the decision had been made to retain the Netscape signature (MOSS). Has this changed?
MOSS is and was the Netscape 6 (commercial) signature. Mozilla is a different product, and uses 'MOZZ'.
See also: bug 46435 (which can probably be marked dup of this one (it's too specific in pointing out only .HQX files)).
We need to be able to set the right creator whether it is Mozilla or NS6. What we could do a make a special creator code which, when passed to nsILocalFileMac::SetFileTypeAndCreator, means "creator of current process." The current process creator could be a static member variable in nsLocalFileMac, looked up once and kept for this purpose.
I had a similar thought last night. There are about 20 places in the code base (a quick scan... don't hold me to this) which are currently hard coded to either MOSS or MOZZ. These all need to be changed to get the info from a common source. This source should be easily obtainable with a minimum amount of overhead. The nsLocalFile class could keep its own cached copy (or could be the owner I suppose), but others will need to access this data (I seem to recall widgets in the XLR query for some reason.) Does anyone have any thoughts on who should own this data? I'm thinking something like an application class or a (mac) utility class, but I don't know the codebase well enough.
Conrad: that sounds like an excellent idea. We can get the signature from the process manager.
The patch here makes it pretty easy. Just pass CURRENT_PROCESS_CREATOR as the creator to SetFileTypeAndCreator and it will be set to that of the current process. Since nearly all of the code is the same between NS6 and mozilla, this will do the right thing without relying on any different code.
That fixes the creator code. Now we need some IC goodness for the file type.
Please fix this for RTM. It's quite difficult to deal with files that don't have a file type. If I save a file in Composer, it gets saved with a filetype of 'TEXT' but if I save it in the browser, it gets saved with a filetype of '????' which makes it hard to deal with in MacOS.
*** Bug 56092 has been marked as a duplicate of this bug. ***
My patch to fix the creator problem was the easy part. The file type is harder because the callers have to specify the right type. That would probably involve many callers all over the place. One easier way, along the lines of the special creator code I added, is to do the same thing with the file type: A special file type could be added to nsILocalFileMac that would tell it to try and determine the file type from the suffix. We practically always have a file suffix when saving files. Then, as long as the file name had been specified, this would just involve making sure that nsILocalFileMac::SetFileTypeAndCreator was called using the magic constants for both type and creator. This would be only one line of non-XP code for the callers instead of putting IC code all over the place in XP code.
After some investigation it seems that nsLocalFile::OpenNSPRFileDesc() is somewhat of a choke point. It seems that a large portion of files are created through here (i.e. cache files, saved links & images). Given this, I would toss the following ideas out for target practice... First I would suggest that the creator be set from the global in the nsLocalFile constructor (currently initialized to 'MOSS'). This would insure the correct setting unless someone forcibly changes it. This would fix the fact that all cache files are currently created as type 'MOSS' (I realize that this is trivial, but it is a symptom of a larger problem.) Next OpenNSPRFileDesc() could attempt to set the the file type based on the file extension. This would be fairly well contained and, I believe, go a long way towards fixing our file typing issues. Comments, suggestions, and criticisms all cheerfully accepted :)
Brian, we can't just hardcode the creator. This will be wrong for either NS6, mozilla, and any embedding app. We need to use the patch I posted here before to get the creator from the current process. Putting something in OpenNSPRFileDesc is a very good idea. That would cover many cases without changing callers. The other case where callers may need to be changed is when the user gets to name the saved file with UI. If the user chops off the final suffix - and many Mac users think "We don't need no stinkin' suffix" - we can't rely on this.
I'm sorry if I implied something or wasn't entirely clear. I was suggesting moving your creator code into the constructor so that it gets set *correctly* when the object is created. Right now it is being set incorrectly to 'MOSS' all the time. Your point about the user naming is a good one. I was hoping to do some sort of MIME type/extension based conversion, but if the user doesn't accept the default name (i.e. the one presented to them when they save the link) all bets are off. That does potentially limit the usefulness of placing the change in OpenNSPRFileDesc. As you noted, I was hoping to not have to force all the callers to set the type as this would require #ifdef XP_MAC code everywhere. I guess the real question is... Do we 1) force all callers to do it, hoping that people will eventually learn that they must add Mac specific support to all file calls. Or 2) Try and insulate the general public from it by doing our best to glean the appropriate file type from data available to us?
We probably should do both: (1) the OpenNSPRFileDesc trick would catch most cases. (2) Find the callers which create files after prompting the user with a save dialog and add code to them to set the file type and creator. There are probably pretty few of these. I agree that setting the creator from the constructor is the right place to do it. Then at least that would be right even if the caller didn't call SetFileTypeAndCreator.
Marking [rtm need info] since you are working on this bug. Can you get an updated patch attached soon and then get r=/sr= so PDT has a chance to approve this before it's too late?
Whiteboard: [rtm need info]
I'm (once again) plagued with Mac plugin bugs and, although I have done some research on this, have not really started to implement fixes. If someone else (ccarlen possibly) has time, it should probably be reassigned.
Reassigning to ccarlen as per discussion yesterday.
Assignee: bnesse → ccarlen
Brian, thanks for the additional info - starting work on it.
Status: NEW → ASSIGNED
The above patch should do it. It does what the earlier patch did with the creator but does it from constructor. You should never have to worry about the creator again. It uses nsIInternetConfigService to get the file type. It does this automatically whenever a file is created. I also put two more methods into the API for setting the file type from either suffix or mime type. I think parts of the code which prompt for a file name should use this for the case of the user lopping off the extension.
Can I get review on this? Also, in looking for places which put up UI for saving files, what should I look for nsIFileWidget?
Whiteboard: [rtm need info] → [rtm need info][need review]
This patch has been here for a while. I marked it as [need review] and asked for review. Anybody out there?
To get reviews, send a message to the specific reviewers (see mozilla.org list), and copy mozilla-reviewers@mozilla.org (I think that is teh address). Comments in bugs don't get as much attention.
Comments on the patch: Nit: There are mixed tabs/spaces in the file, causing bad indentation. In nsLocalFile::SetOSTypeFromExtension, you get to the file extenion like this; + extPtr = strrchr(localExtBuf, '.'); This will break with files that have multiple extensions (e.g. "foo.sit.bin"). It would be better to look backwards from the end of the filename for the last '.'. I think the MakeUnique code does this. + extPtr = const_cast<char *>(extension); // really, we won't touch it can extPtr be declared as a const char* ? Other than that, it looks good. Revise the patch, and I'll give an sr.
Simon, I am looking backwards from the end. I'm using strrchr which finds the last occurance of a char in a string. strchr (one 'r') finds the first. I'll look at the const issue and the indentation I'll fix.
oops, you're right on the strrchr. Too similar to strrchr (stupid standard library naming)
sfraser, are you giving sr= on this when cast question is clarified? Who would be a good r=? Is anyone willing to go to the PDT meeting and try to convince people a big patch is a good idea with a candidate we like already in test?
sr=sfraser on the latest path.
Whiteboard: [rtm need info][need review] → [rtm need info] SR=sfraser, NEED R= ?
This patch looks gigantic. Is there anything shorter we can do? Perhaps with a hacky fix for hardcoded 'MOSS' only on the RTM branch? This issue seems bad, but we have no time to shake out a big patch.
This path may be large but is very low risk. If you actually look at the code you'll see that several routines were added to do the work. They're called in only 3 places. The important thing to notice is that it's completely tolerant of error. If there is any problem is getting the right type/creator, it just uses the default. Not fixing this is a slap in the face to Mac users. Few things will be more irritating than files having no icon or an invalid type.
Is there any plan on getting this reviewed (and moved to [rtm+])?
Yes, I've asked scc for review. I would very much like to see it get rtm+.
This is pretty ugly, but given the date and the need to get a product out for manufacturing, I think that we could release note this. OK. You can all call me an idiot now.
I refuse to call you an idiot Michael, you're too nice of a guy;), but I don't see this as a release note issue. There isn't much point it adding a release note that says "This application isn't capable of assigning it's own creator type to a file and we have no idea what type of data we are saving." I know this looks like a lot of code, but as Conrad states above, it is low risk. This code uses system level code and services to assign the correct icons to files. This is a huge win for us in the Mac user experience department. This is one of those things that a Macintosh application is expected to do. I hate to say this but if we do it correctly, no one will notice. However, if we don't do it correctly we will get flamed for it. Even Microsoft gets the appropriate file types on their files.
I agree. *Most* mac users are totally incapable of solving file type/creator issues, so this bug will mean that they just won't be able to open many of the files created by NS6. Heck, you can't even double-click our Install log file, and have it open in an application!
The only honest and helpful release note we could make on this problem would be: Find and download FileTyper 5, make a pile of AutoTypers with it, and use the right one on every file you save out of Netscape 6. Really. Not good.
Doesn't seem like anyone answered the question of whether there's a smaller workaround for N6. Is this really the shortest/safest fix? If this is the shortest/safest fix, what sort of testing has been done? And what about the second code review?
There really isn't a shorter fix. The right type has to be determined for each file, and this is about as short as that can be. You can't just change a constant because there are many distict types. In terms of how safe it is: It uses operating system services to find the right creator. It uses the internet config service to get the type. That code is very proven. Also, determining the type/creator does not propagate any errors. All errors in doing this are trapped and, even if one occurred, it would continue without the right type/creator.
conrad: could we hook this up in a unit test, and beat upon it that way? Also, would be worth testing edge cases, like what happens if Internet config is not installed.
Simon, Yeah let's do that - the unit test to beat on it. As far as InternetConfig not being present, when it does the NS_WITH_SERVICE on the InternetConfig service, that should fail. If it does this code silently returns. But, we should test that.
Simon, I'm working on the unit test. I'm making some code which, given a dir, recurses through it, and for each file, creates a file of that name in the temp folder, then deletes it. File creation will go through this patch.
I want to add to the comments above... "most" users is like 99% of all Macintosh users. I can't come up with a good analogy for Windows users so they'd understand how broken this is on Mac. The best I have is something along the lines of saving a file with no extension and no way to rename it to get the extension the OS has. We really shouldn't ship with this bug. Add relnoteRTM to this bug in case it doesn't make it into final RTM candidate.
Keywords: relnoteRTM
Phil--if we *really* truly needed a smaller patch, I bet we could come up with one so that users couldn't save files from the browser. :-(
Conrad: can you please attach a sitball of fixed files, so we can all apply and test?
Attached file as stuffit file —
Which exact file saves are broken?
All of them. Every time that NS 6 creates a file, that file has a 'generic' file type. This affects: * Saving web pages * Saving images * Saving mail attachments * Saving files in composer * Saving FTP downloaded files I have the changes in my branch build, and have seen no problems so far.
Hmmm. You go to the trouble to define a name const OSType kDefaultCreator = 'MOSS'; but then you don't exploit this in your fallback case: creator = mgCurrentProcessSignature != 0 ? mgCurrentProcessSignature : 'MOSS'; Do you really want both the function to get the current process signature, and all clients to _both_ try to fallback? Won't the client code never get the opportunity to fallback? Where ever possible, prefer direct-initialization over copy-initialization, e.g., prefer nsCOMPtr<nsIFile> asFile( do_QueryInterface(macFile, &res) ); over the form using |=|. Also, your second and third patches look like they're full of tabs :-) So: pretty good, not too much to fix here.
creator = mgCurrentProcessSignature != 0 ? mgCurrentProcessSignature : 'MOSS'; Are we looking at the same thing? The latest patch uses kDefaultCreator. The tabs - My patch is not full of tabs - the original file is full of tabs to begin with. Fixing this would womp the history of the whole file. nsCOMPtr<nsIFile> asFile( do_QueryInterface(macFile, &res) ); You're right - I'll change this.
there're 4 attachments. I assumed the total patch was the first three, and that the fourth was just an archive of them. So I examined the first three. Did I do the wrong thing?
Scott, the 4th is the one. It's the same as the 3rd but whole files instead of diffs.
dude :-) I can't review whole files (well, I could, but it would take me much longer) ... and apparently the patches _are_ different than what you put in the whole files ... hence your earlier comment. Make a single patch out of all the changes you want reviewed (if you're doing this on Mac, then you can select-all/copy/paste the individual -w -u2 diffs from MacCVS into a single file to attach).
Scott, maybe I phrased it wrong, The 3rd and 4th patch are the same in that the 4th is just the diffs of the 3rd pre-applied so people could try it out, The 3rd patch is the one you want.
I think we've missed the boat on this one, so don't sweat it. We should get this into the trunk. An enhancement that I'd like to see is that we save some file types with not the Mozilla creator code, but with the creator from IC. That way, saved GIF, JPEGs etc get more appropriate creator codes. We should probably do this for all file types that are not in an exceptions list (or maybe we should go by extension or mime type? I want .c files to be CodeWarrior files, and .html files to be mozilla files.)
have you noticed we can't even load .c files into the content area? but that's another bug...
No, I've not noticed. Well actually, I had never tried :-) I just did though and whether I used "Open File?" or typed in a file URL, it worked for me. In any case, not related to this.
r=brade; get this on the trunk asap!
Whiteboard: [rtm need info] SR=sfraser, NEED R= ? → [rtm need info] SR=sfraser, NEED R= brade
So, to load C (and JS) files and the like, it seems that we need to override the MIME type that we got (from IC?) with text/plain, or have some concept of a "display type" which overides the normal mime type. Where should this stuff happen?
Checked in this morning. I'll say it's fixed and the enhancement Simon suggested on 11-08 could be made into a new "enhancement" bug.
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
*** Bug 99164 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: