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

VERIFIED INVALID

Status

P4
normal
VERIFIED INVALID
15 years ago
10 years ago

People

(Reporter: eagle.lu, Assigned: eagle.lu)

Tracking

unspecified
Thunderbird 3.0rc1
Bug Flags:
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

(Assignee)

Description

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

Comment 1

15 years ago
Created attachment 156867 [details] [diff] [review]
patch v0
(Assignee)

Comment 2

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


(Assignee)

Comment 3

15 years ago
Comment on attachment 156867 [details] [diff] [review]
patch v0

Can you give r? Thanks
Attachment #156867 - Flags: review?(mscott)
(Assignee)

Comment 4

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

Comment 5

15 years ago
I'll try to look at this patch during the 0.9 time frame. 
Target Milestone: --- → Thunderbird0.9

Comment 6

14 years ago
didn't make the 0.9 train. I must confess I get very lost when tryint go
understand the code being changed.
Target Milestone: Thunderbird0.9 → Thunderbird1.0

Updated

14 years ago
Target Milestone: Thunderbird1.0 → Thunderbird1.1

Comment 7

14 years ago
(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

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

Comment 9

14 years ago
I still haven't been able to figure out the code being changed :(. not a stop
ship bug
Target Milestone: Thunderbird1.1 → Thunderbird2.0

Comment 10

13 years ago
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.
QA Contact: general
still see this?
Assignee: mscott → nobody
Component: General → Mail Window Front End
QA Contact: general → front-end
(Assignee)

Comment 12

11 years ago
I just verified that the bug still exits in the trunk code
(Assignee)

Comment 13

11 years ago
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.
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?
Flags: wanted-thunderbird3?
OS: Linux → All
Hardware: PC → All
(Assignee)

Updated

11 years ago
Assignee: nobody → brian.lu
(Assignee)

Comment 15

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

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

Comment 17

11 years ago
(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".
Keep in mind, for users on windows (who haven't enabled show known file extensions), the save as dialog never show any extension...
(Assignee)

Comment 19

11 years ago
(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.  
Boying, can you cancel the mscott review request and pick new reviewer? Thanks
Whiteboard: [patchlove]
Comment on attachment 156867 [details] [diff] [review]
patch v0

mscott no longer actively reviewing, so canceling review
Attachment #156867 - Flags: review?(mscott)
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Priority: -- → P4
Summary: Wrong behavior when saving an e-mail → Wrong behavior wrt adding file name extension, when saving an e-mail
Target Milestone: Thunderbird2.0 → Thunderbird 3.0rc1
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
Attachment #156867 - Attachment is obsolete: true
Boying Lu, any way you can come up with a new patch?
(Assignee)

Comment 24

10 years ago
OK, I'll post a new patch soon.
(Assignee)

Comment 25

10 years ago
Created attachment 383881 [details] [diff] [review]
updated patch
(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
(Assignee)

Updated

10 years ago
Attachment #383881 - Flags: review?(bienvenu)

Comment 27

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

Updated

10 years ago
Attachment #383881 - Attachment is obsolete: true
Attachment #383881 - Flags: review?(bienvenu)

Comment 29

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

10 years ago
marking invalid as the code is going to go away...
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → INVALID
Whiteboard: [patchlove]
(Assignee)

Comment 31

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

Comment 32

10 years ago
I verified that the bug was fixed in the trunk
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.