126260 - nsIFilePicker needs default extension attribute

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Adam Lock]

The nsIFilePicker does not provide the means for callers to specify a default
file extension on platforms such as Win32.

This affects embedders who show a save as dialog, filter on some file type (e.g.
*.foo) and expect .foo to be automatically appended in the filename returned by
the call. 

Add a defaultFileExt attribute to nsIFilePicker in trunk & 0.9.4 branch and
ensure that Win32 version of file picker uses it.

Comment 1

[Adam Lock]

Attachment: Patch

Work in progress patch. Feedback please.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The patch in bug 117269 does this....

Comment 3

[Adam Lock]

This fix is needed on the 0.9.4 branch as well. I'll use this bug to cover the
branch and the other more comprehensive fix for bug 117269 can go in the trunk
when it's ready.

Both appear to do more or less the same thing as far as the file picker is
concerned, adding a "defaultExtension" attribute to nsIFilePicker and using it
in the Win32 nsFilePicker.cpp implementation.

Gus, can you test the patch when you have the time and see if it fixes your issue?

Thanks

Comment 4

[Judson Valeski]

can we get some review action: law, mscott?

Comment 5

[Gustavo Verdun]

Patch works... oddly, on win2k atleast, I can save a .exe file as .zip, .dll,
.doc, and probably any known or registered file extensions, but if i try .foo it
adds .exe to the end. (this is identical behavior in IE and other standard file
dialogs)

If I enter a file name and add a . the the file is saved w/o an extension.

Comment 6

[Bill Law]

Do all nsIFilePicker C++ implementations inherit from nsBaseFilePicker?

Also, the JS nsFilePicker for Linux will need some code added, I think.

I'm starting to have misgivings about using this feature of the Win32 file
picker.  The problem is that the user can change the file type, in which case
the default extension is wrong.  You don't show any of the code that would use
this feature, but if we specified "htm" as the default extension when we show
the file picker for "save page as...", and the user selects "plain text", then
wouldn't we end up saving the file as plainText.htm?

Maybe we need to embellish the file picker with code that detects the user
changing the file type and resetting the default extension when that happens
(e.g., they choose "plain text", we reset the default extension to "txt")?

Comment 7

[Brian Ryner (not reading)]

Hm, perhaps I'm misunderstanding the problem here.  Why does this functionality
need to be in the filepicker - i.e. why can't the caller of nsIFilePicker append
the extension after the filepicker is dismissed?

Comment 8

[Gustavo Verdun]

In response to comment #7:

Two reasons:
1) If a user enters "filename." we get "filename" so we would be adding an
extension when the user wanted none.
2) If the file exists (after default extension processing) the user is prompted
for overwrite. We would have to add the same logic on our side.

Comment 9

[Brian Ryner (not reading)]

Ok.  Re: Bill's comment #6, that would just involve grabbing the first file
extension from the selected filter, wouldn't it?  (as passed to AppendFilter).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Brian, the basic idea is that the extension thing should only happen on Windows
(and maybe OS2).  Thefore making callers (some of which are JS, where figuring
out the platform can be somewhat problematic) handle the extension stuff seems
wrong...  see bug 117269 for the consequences.

Comment 11

[Adam Lock]

The caller could append the extension after filepicker returns but it breaks the
overwrite confirmation behaviour. If a file called "blah.foo" exists on the disk
then they should be shown a "The file blah.foo already exists do you want to
replace it?" dialog whether they type "blah" or "blah.foo" in the filename field.

Comment 12

[Adam Lock]

Bill, the OS/2, Windows, Mac, Gtk & BeOS filepickers all derive from
nsBaseFilePicker, hence the reason I put the stock implementation in there. I
forgot the js implementation but I can implement that easily enough.

Comment 13

[Adam Lock]

Attachment: Updated patch

Updated patch includes patch for nsFilePicker.js and uses diff -b to hide some
whitespace noise.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> +    if (mDefaultExtension.Length() > 0) {

Please use |if (!mDefaultExtension.IsEmpty()) {|

> +        nsCAutoString ext; ext.AssignWithConversion(mDefaultExtension);

AssignWithConversion converts to ASCII, iirc.  Are there encoding issues that
need to be addressed here?

Comment 15

[Adam Lock]

Win32 accepts a 3 character maximum ascii string as the default extension so I
don't think it makes any difference what character conversion I use here.

I can change the Length() > 0 test to !IsEmpty()

Comment 16

[Adam Lock]

I should note that the lpstrDefExt is actually TCHAR so in theory this is not
always true, however we compile Mozilla with UNICODE undefined meaning it is
ascii for us.

Comment 17

[Adam Lock]

Attachment: Same as last except for Length test is replaced by !IsEmpty test

Comment 18

[Marek Z. Jeziorek]

Marking it edt0.9.4+ (i.e., approval for 0.9.4 check-in).
When could we get r= and sr=?

Comment 19

[rpotts (gone)]

Comment on attachment 70408 [details] [diff] [review]
Same as last except for Length test is replaced by !IsEmpty test

sr=[e-mail]

Comment 20

[Alec Flett]

Comment on attachment 70408 [details] [diff] [review]
Same as last except for Length test is replaced by !IsEmpty test

r=alecf but I wish there was a way to do this without the
AssignWithConversion() bit...(i.e. so we don't loose non-ascii extensions) but
that is probably a rare case.

Comment 21

[Chak Nanga]

Fix checked into 0.9.4 branch...

Comment 22

[Judson Valeski]

closing as fixed. I believe the trunk is addressed w/ the other patch in the
other bug. re-open if I'm wrong please.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Reopening.  :) Looking over this patch and my patch in the other bug I like this
one more.  This one also has all the requisite reviews.  Could you land it on
the trunk please?  Then the other bug can focus on using this new attribute to
solve the real problem over there.

Thanks,
Boris

Comment 24

[Brian Ryner (not reading)]

Adam, just a slight correction to your comment.  While a nsFilePicker.cpp for
gtk that derives from nsBaseFilePicker might exist on the 0.9.4 branch, that's
not what is used, the JS filepicker in xpfe/components/filepicker is.

Comment 25

[Adam Lock]

Attachment: Same patch tweaked for the trunk

Patch for the trunk, the same except for calling the new global ToNewUnicode
function.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

a=roc+moz for 0.9.9

Comment 27

[Adam Lock]

Closing again. Just discovered patch for the other bug got checked into the
trunk last night.

Comment 28

[sairuh (rarely reading bugmail)]

over to embed qa [load-balancing]...

Comment 29

[Michael Dunn]

See Bugscape bug:
http://bugscape.netscape.com/show_bug.cgi?id=12174