256681 - Wrong behavior wrt adding file name extension, when saving an e-mail

Status: VERIFIED INVALID

(Thunderbird :: Mail Window Front End, defect, P4)

Description

[Boying Lu]

re-produce steps:
1. select a mail say foo
2. select "File" -->"Save as"
   a dialog window will popup
3. click OK button
   a file named foo.eml will be saved
4. select the same mail and "File" -->"Save as" again
5. change the file type to "Text files"
   a alert window will popup and ask me if I want to replace current foo.eml or not
6. select Ok to replace the file

expected results:
   foo.eml will be rewritten to plain text file.

actual results:
   foo.eml is deleted and a file named "foo.eml.txt" is created.

This will make user very confused. Since he chooses to replace the foo.eml file
why not overwrite it. If mozilla chooses to create another file foo.eml.txt to
save the mail, why does it delete foo.eml?

Comment 1

[Boying Lu]

Attachment: patch v0

Comment 2

[Boying Lu]

I think the root cause lies in the following codes:
   if (dialogResult == nsIFilePicker::returnReplace) {
      // be extra safe and only delete when the file is really a file
      PRBool isFile;
      rv = localFile->IsFile(&isFile);
      if (NS_SUCCEEDED(rv) && isFile) {
        rv = localFile->Remove(PR_FALSE /* recursive delete */);
        NS_ENSURE_SUCCESS(rv, rv);
      }
    }
This excerpt tells us that if user choose to replace the file, the file is
deleted  first.

But the following codes following the previous excerpt:
   switch ( saveAsFileType )
    {
      // Add the right extenstion based on filter index and build a new localFile.
      case HTML_FILE_TYPE:
          if ( (fileName.RFind(HTML_FILE_EXTENSION, PR_TRUE, -1,
sizeof(HTML_FILE_EXTENSION)-1) == kNotFound) &&
               (fileName.RFind(HTML_FILE_EXTENSION2, PR_TRUE, -1,
sizeof(HTML_FILE_EXTENSION2)-1) == kNotFound) ) {
           fileName.AppendLiteral(HTML_FILE_EXTENSION2);
           localFile->SetLeafName(fileName);
        }
        break;
      case TEXT_FILE_TYPE:
        if (fileName.RFind(TEXT_FILE_EXTENSION, PR_TRUE, -1,
sizeof(TEXT_FILE_EXTENSION)-1) == kNotFound) {
         fileName.AppendLiteral(TEXT_FILE_EXTENSION);
         localFile->SetLeafName(fileName);
        }
        break;
      case EML_FILE_TYPE:
        if (fileName.RFind(EML_FILE_EXTENSION, PR_TRUE, -1,
sizeof(EML_FILE_EXTENSION)-1) == kNotFound) {
         fileName.AppendLiteral(EML_FILE_EXTENSION);
         localFile->SetLeafName(fileName);
        }
        break;
      case ANY_FILE_TYPE:
      default:
        // If no extension found then default it to .eml. Otherwise,
        // set the right file type based on the specified extension.
        PRBool noExtensionFound = PR_FALSE;
        if (fileName.RFind(".", 1) != kNotFound)
        {
          if ( (fileName.RFind(HTML_FILE_EXTENSION, PR_TRUE, -1,
sizeof(HTML_FILE_EXTENSION)-1) != kNotFound) ||
               (fileName.RFind(HTML_FILE_EXTENSION2, PR_TRUE, -1,
sizeof(HTML_FILE_EXTENSION2)-1) != kNotFound) )
            saveAsFileType = HTML_FILE_TYPE;
          else if (fileName.RFind(TEXT_FILE_EXTENSION, PR_TRUE, -1,
sizeof(TEXT_FILE_EXTENSION)-1) != kNotFound)
            saveAsFileType = TEXT_FILE_TYPE;
          else
            noExtensionFound = PR_TRUE;
        }
        else
          noExtensionFound = PR_TRUE;
        // Set default file type here.
        if (noExtensionFound)
        {
          saveAsFileType = EML_FILE_TYPE;
          fileName.AppendLiteral(EML_FILE_EXTENSION);
          localFile->SetLeafName(fileName);
        }
        break;
will append a "proper" suffix if the file name doesn't have one. So this will
cause if user want to save to a file named "foo.eml" as text file, thunderbird
will delete foo.eml and create a new file named foo.eml.txt.

Comment 3

[Boying Lu]

Comment on attachment 156867 [details] [diff] [review]
patch v0

Can you give r? Thanks

Comment 4

[Boying Lu]

The bug looks like the bug 96134, but they are not same

Comment 5

[Scott MacGregor]

I'll try to look at this patch during the 0.9 time frame. 

Comment 6

[Scott MacGregor]

didn't make the 0.9 train. I must confess I get very lost when tryint go
understand the code being changed.

Comment 7

[Peter Lairo]

(In reply to comment #2)
> this will
> cause if user want to save to a file named "foo.eml" as text file, thunderbird
> will delete foo.eml and create a new file named foo.eml.txt.

That still seems wrong. If foo.eml exists, and we are creating a *differently
named* file: foo.eml.txt, why do we need to delete foo.eml?

This seems counterintuitive to what a user is expecting. He may very well want
to have an "eml" and a "txt" file (assuming there is some point to being able to
file type to "Text files").

Comment 8

[Peter Lairo]

*assuming there is some point to being able to *change the* file type to "Text

Comment 9

[Scott MacGregor]

I still haven't been able to figure out the code being changed :(. not a stop
ship bug

Comment 10

[Peter Lairo]

This may be obvious, but here is what should happen (IMO):

1.  User selects "File / Save As / File" (Ctrl+S)

2a. "Save as type" defaults to "Mail Files" (*.eml) - Currently: "All Files"!!!

2c. If foo.eml exists, user writes/selects foo or foo.eml, 
    and user selects Type=txt 
    --> new file is saved as foo.txt (and not foo.eml or foo.eml.txt)

3a. If foo.eml already exists, and user writes "foo" and selects Type=eml 
    --> overwrite foo.eml

3b. If foo.eml already exists, and user selects Type = txt 
    --> keep foo.eml file and create new foo.txt file.

- There cannot be a "Save" Type=All. That is only for the "Open" Dialog.

- Filename=foo.eml and Type=eml should yield foo.eml and not foo.eml.eml 
- Filename=foo     and Type=eml should yield foo.eml and not just foo

- Type=eml, Filename=foo.eml --> Reselect Type=eml --> Filename=foo.eml
- Type=eml, Filename=foo     --> Reselect Type=eml --> Filename=foo.eml
- Type=eml, Filename=foo.eml --> Change to Type=txt --> Filename=foo.txt
- Type=eml, Filename=foo     --> Change to Type=txt --> Filename=foo.txt
- Type=eml, Filename=foo.wtf --> Change to Type=txt --> Filename=foo.wtf.txt

NOTE: The "Filename=" used above is the name as displayed in the dialog,
      and the *complete* filename on disk.

Comment 11

[Wayne Mery (:wsmwk)]

still see this?

Comment 12

[Boying Lu]

I just verified that the bug still exits in the trunk code

Comment 13

[Boying Lu]

I suggest to set "Filename=" to "foo" not "foo.eml" by default, so we can choose which suffix to use based on the selected file type.  e.g. we set file name to "foo", and default suffix is ".eml" (but not be shown in the file name field), if user select "txt file", the suffix will be changed to ".txt" (also no shown). This 
will make less confusion.

Comment 14

[Wayne Mery (:wsmwk)]

Thanks Boying.  On windows too so OS=ALL. nominating wanted‑thunderbird3 since target milestone had been TB2.

Do you need to modify the patch and assign the bug to yourself?

Comment 15

[Boying Lu]

I investigated this issue, I suggest that we don't append suffix to file name. e.g. if user choose to save an e-mail as "foo" in .txt format, we just save it 
as "foo" instead of "foo.txt"  I.e. The "Save As" of Thunderbird has the same behavior with that of Firefox

Currently if user chooses to save an e-mail (say foo) as txt file, foo.eml.txt will be created. If there is a file "foo.eml" in the same directory, gtk file chooser will pop up a window which tells user that "foo.eml" is already exist and user should choose to overwrite it or not.  But actually, what we want to create is "foo.eml.txt" instead of "foo.eml".

The root cause is that we don't have any control over the file name user chooses in gtk file chooser and the file chooser has internal logic to check file existence.

If we all agree on this, I'll make a patch this weekend. If anyone has better idea, please let me know.

Comment 16

[Peter Lairo]

Boying: Why would we need a "foo.eml.txt"? Windows has no concept of a *double* extension. This sounds like the fault of the gtk (Linux?) file system. I hope your fix doesn't "break" Windows' sensible behavior.

1. If type = eml and user types foo     --> save as foo.eml
2. If type = eml and user types foo.eml --> save as foo.eml
3. If type = eml and user types foo.txt --> save as foo.txt
                                            (the user made an EXPLICIT choice)

Comment 17

[Boying Lu]

(In reply to comment #16)
> Boying: Why would we need a "foo.eml.txt"? Windows has no concept of a *double*
> extension. This sounds like the fault of the gtk (Linux?) file system. I hope
> your fix doesn't "break" Windows' sensible behavior.
> 
> 1. If type = eml and user types foo     --> save as foo.eml

If there is a file named "foo" in current directory, file chooser will tell user that "foo" is already exist and he should choose to keep or replace it. But what we actually want to do is save "foo.eml" not "foo".

Comment 18

[Magnus Melin [:mkmelin]]

Keep in mind, for users on windows (who haven't enabled show known file extensions), the save as dialog never show any extension...

Comment 19

[Boying Lu]

(In reply to comment #18)
> Keep in mind, for users on windows (who haven't enabled show known file
> extensions), the save as dialog never show any extension...
> 

Yes. but gtk file chooser shows a different behavior. gtk file chooser uses the value in the "Name:" field as the exact file name, no extension will be added automatically. e.g. if the "Name:" field is "foo", gtk file chooser checks if there is a file named "foo" in the directory.  If yes, the chooser will ask user to replace "foo" or not.

I think we should have "Save As" of TB has the same behavior as the "Save As" of  FF. e.g. we don't add any file extension suffix, we just use the file name returned by file chooser.  

Comment 20

[Wayne Mery (:wsmwk)]

Boying, can you cancel the mscott review request and pick new reviewer? Thanks

Comment 21

[Wayne Mery (:wsmwk)]

Comment on attachment 156867 [details] [diff] [review]
patch v0

mscott no longer actively reviewing, so canceling review

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 156867 [details] [diff] [review]
patch v0

Patch has bitrotted.

$ patch --dry-run < ~/Desktop/tbTestPatches/256681.diff 
patching file nsMessenger.cpp
Hunk #1 FAILED at 1020.
1 out of 1 hunk FAILED -- saving rejects to file nsMessenger.cpp.rej

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Boying Lu, any way you can come up with a new patch?

Comment 24

[Boying Lu]

OK, I'll post a new patch soon.

Comment 25

[Boying Lu]

Attachment: updated patch

Comment 26

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #25)
> Created an attachment (id=383881) [details]
> updated patch

Great, thanks Boying, please also select a reviewer and ask for a review. :)

http://www.mozilla.org/mailnews/review.html

Comment 27

[David :Bienvenu]

I believe this will conflict with the patch in bug 340168 - what's less clear is whether this is needed after that patch lands but I suspect not...

Comment 28

[Rain [:rain1] (inactive)]

Yeah, we're removing this code entirely in bug 340168 (which I should hopefully be able to land tomorrow), so this isn't needed at all.

Comment 29

[David :Bienvenu]

Comment on attachment 383881 [details] [diff] [review]
updated patch

thx, Sid, marking obsolete, then. Brian, please let us know if this doesn't solve the issue for you...

Comment 30

[David :Bienvenu]

marking invalid as the code is going to go away...

Comment 31

[Boying Lu]

(In reply to comment #29)
> (From update of attachment 383881 [details] [diff] [review])
> thx, Sid, marking obsolete, then. Brian, please let us know if this doesn't
> solve the issue for you...

OK, I'll verify the bug if that patch is landed.

Comment 32

[Boying Lu]

I verified that the bug was fixed in the trunk