660796 - Files are saved without their extension

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Pierre Rudloff]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en; rv:1.9.2.18pre) Gecko/20110531 Camino/2.1a2pre (like Firefox/3.6.18pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en; rv:1.9.2.18pre) Gecko/20110531 Camino/2.1a2pre (like Firefox/3.6.18pre)

When I save a file, it is saved without its extension.

Reproducible: Always

Steps to Reproduce:
1.Go to http://s3.amazonaws.com/cbo/img/logo.png
2.Use File > Save As...

Actual Results:  
Camino tries to save "logo".

Expected Results:  
It should save "logo.png".

I switched back to Tiger a couple of weeks ago and I don't remember having this bug on Snow Leopard. So it is either a Tiger-only bug or a bug introduced in a recent nightly.

Comment 1

[philippe (part-time)]

Hmm, this works fine here (10.6).

1/ does it work with Camino 2.0.7 ?
2/ Can you check if it works with nightlies from before february 2011 (I think bug 583818 is the most recent changes to this stuff)

Comment 2

[Samuel Sidler (:ss)]

I've seen this too (10.6) but only with the right click option "Download Link Target".

Comment 3

[Pierre Rudloff]

It works with 2.0.7 but not with 20110131002336.

Comment 4

[Pierre Rudloff]

I can reproduce on OS 10.6 with a fresh profile.

Comment 5

[Samuel Sidler (:ss)]

STR:

  1. Go to http://5by5.tv/talkshow/49
  2. Right-click (ctrl-click) on the "MP3 (46.8 MB)" link and select "Download
     Link Target..."
  3. Note that the save as window does not specify a filetype.
  4. Save.

ER:
A file gets downloaded called "talkshow-049.mp3".

AR:
A file gets downloaded called "talkshow-049".

Confirming...

Comment 6

[philippe (part-time)]

Hmm. So that works for me – the 'Save' dialog shows filename.mp3, and the file ends up on my desktop as filename.mp3.

That being said -
1. I have my Finder preferences set to 'Show all filename extensions' [*]
2. When turning OFF that pref
   * the Save dialog shows a 'hide extension' checkbox at the bottom right
   * that checkbox is checked (by default ?)
   * then the file is saved without extension.
3. Toggling that checkbox has no effect (the extension is not shown)
4. After toggling, canceling the 'Save' dialog and selecting 'Download link target' again does show the extension in the 'Save' dialog (and the saved file has the extension). [**]

It doesn't matter what type of file is selected (.html. .jpg, .mp3, …). While testing, I noticed that Safari never shows that checkbox.

[*] in case of 1. above, the 'Hide extension' checkbox is not present in the 'Save' dialog.

[**] If the 'Hide extension' checkbox is _not_ checked when opening the 'Save dialog', toggling does work.
This reminds me of a similar odd behaviour while testing in bug 420418. See bug 420418 comment 11.

Comment 7

[philippe (part-time)]

That toggling behaviour broke between the 20110201 and 20110203 builds  (bug 583818). Further testing shows that independently of the state of that checkbox, files have their extension when viewed in the Finder with the 20110201 build, but not with the 20110203 builds.

http://hg.mozilla.org/camino/pushloghtml?fromchange=ec378b316279&tochange=a11e90030225

Comment 8

[Samuel Sidler (:ss)]

fwiw, I have whatever is default (I'm on a new install of 10.6), which happens to be "Show all filename extensions" unchecked. Good find Philippe!

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #7)
> That toggling behaviour broke between the 20110201 and 20110203 builds  (bug
> 583818). Further testing shows that independently of the state of that
> checkbox, files have their extension when viewed in the Finder with the
> 20110201 build, but not with the 20110203 builds.
> 
> http://hg.mozilla.org/camino/
> pushloghtml?fromchange=ec378b316279&tochange=a11e90030225

Was it bug 583818 or bug 407222 that broke this?  Both are in that range.

It seems like fixing this should be as simple as checking to see if that box is checked, and, if so, re-appending the extension before telling Gecko to save the file.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: WIP fix for extension-stripping

This is a useable fix for the biggest problem, the fact that extensions get lost when hidden, that shouldn't regress any of the other fixes we've made to the Save panel since 2.0.

There are a couple of drawbacks, though I don't think either of them are fatal:

1) Because we're appending the extension to the filename string we're passing to Gecko, this fix also suffers from bug 675025.

2) Toggling from "hidden extension" to "visible extension" during a save does not restore the extension (either to the filename shown in the text box, or to the final file).  As philippe noted above, the next save/cancelling and resaving does the right thing, until you toggle off again.

The way toggling appears to work is that it depends on having a required  extension ("filetype") set; if there's a required filetype, NSSavePanel automatically shows and hides the extension when checking that box.  (Interestingly, in cases where multiple extensions are allowed (HTML), if I switch the extension from the initial one to another allowed one, hide the extension, and then show it again, NSSavePanel remembers which one of the allowed extensions I had chosen.)

==

I've also tried just about every permutation I can think of for calling |setAllowedFileTypes:| for the non-HTML path, but no matter what I do, I either get into a state that regresses bug 583818, causes the default extension to be appended on top of any other extension, or makes files be saved with no extension when the extension is hidden (or sometimes two of the three).

So aside from possibly something like http://paste.lisp.org/display/50122 (which I can't figure out how to adapt to make it compile here) or possibly getting the editor and mucking around in there (similar to http://ns.runcode.us/q/nssavepanel-selecting-part-of-file-name), neither of which seem very palatable, it's either this imperfect patch, this regression, or re-opening the regression in bug 583818 (which worked right in 2.0) and undoing the fix for bug 359373.

:grmbl:

My head hurts.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Saner fix

This is a much saner (and safer fix) that doesn't seem to regress anything from 2.0 or the desired fixes from 2.1 development (adding the "Hide Extension" checkbox was just something we did for completeness/parity between the various panels).

1) It removes the ability to hide the extension in the Save panels (which was cosmetic to the Save panels only and never persisted into the Finder AFAICT, so it was rather useless except for triggering this bug), throughout Camino; the default value for setCanSelectHiddenExtension: is NO, so we can just remove that line of code from all NSSavePanel call sites.

2) In the two call sites that I broke for 2.1 (the two call sites where it became possible to set/change an arbitrary extension), unilaterally set the desired "extension checkbox state" value in the NSUerDefaults before setting up the Save panel.

Note that it's possible for a user to "inherit" a hidden extension value from the OS and not have the key in the Camino defaults, so that's why we don't check to see if the default exists or not.

(The other two NSSavePanel call sites set a required file type, so there's no way to kill their extensions, so no cleanup is necessary there.)

Sam/Pierre, I'll see about getting a build for you guys to play with; since everyone was seeing something slightly different, the patch would benefit from additional eyeballs to make sure I haven't missed anything.

Comment 12

[philippe (part-time)]

Comment on attachment 550496 [details] [diff] [review]
Saner fix

Works fine here on 10.6 with a large variety of file types (including data: uri), Save as and Download Link Target as. All files end up with the appropriate extension as viewed in the Finder. File extensions can still be manipulated and bug 407222 didn't regress either.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 550496 [details] [diff] [review]
Saner fix

http://www.ardisson.org/smokey/moz/Camino-2.1b2pre-i386-2011-08-06.dmg will be available momentarily.

Sam, Pierre, can you guys make sure this build behaves as you'd expect?

Comment 14

[Samuel Sidler (:ss)]

Comment on attachment 550496 [details] [diff] [review]
Saner fix

LGTM

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 550496 [details] [diff] [review]
Saner fix

Stuart, see comment 11 for details/rationale of this method of fixing the problem (and comment 10 for why other methods of fixing are no good/worse options).

Note: I didn't do a "migration check" user default because this can really screw things up (rendering saved files "unopenable") and I didn't want any forward-back-forward people to be left in the lurch (2.0.x can trigger this bug in 2.1 if a user selected "Hide Extension" in the Export Bookmarks or Back Up Certs panels in 2.0.x and then later uses 2.1, or of course by having that setting in the Finder and never having toggled it in Camino).

If it were only possible to get in this situation by using 2.1 builds alone, I would have done the migration check (and, in 2.2, we could safely move to using one), but it just feels like there are too many ways to get into the extension-stripping situation right now.

Comment 16

[Stuart Morgan]

Comment on attachment 550496 [details] [diff] [review]
Saner fix

I'm fine with the general approach, but I'd rather see the user defaults stuff done in the startup/migration section of PreferenceManager instead of embedded into the various save points.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Saner fix, v1.2

Yeah, that makes more sense (I remember considering that at some point, but for some reason misremembered and thought we only twiddled Gecko prefs there :P ).

Herewith the improved patch.

Comment 18

[Stuart Morgan]

Comment on attachment 553554 [details] [diff] [review]
Saner fix, v1.2

Review of attachment 553554 [details] [diff] [review]:
-----------------------------------------------------------------

sr=smorgan

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/a8d4cdc5d7e0

Sorry for the delay in getting the patch together, guys :-(