Closed
Bug 134796
Opened 22 years ago
Closed 15 years ago
"Save All" to root of drive (c:\) saves to wrong directory (to Working Folder/Current Directory)
Categories
(MailNews Core :: Attachments, defect)
Tracking
(Not tracked)
VERIFIED
WORKSFORME
People
(Reporter: youying, Unassigned)
References
Details
Attachments
(5 obsolete files)
From Bugzilla Helper: User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.9+) Gecko/20020331 BuildID: 2002033109 After saving all attachments in MozillaMail via "Save All" feature, the files are not saved to the specific directory. Reproducible: Always Steps to Reproduce: 1.Just save attachments via "Save All" in C:\ 2.You will find those files are in Mozilla directory. Actual Results: Files should be saved in the specific directory.
Comment 1•22 years ago
|
||
Reporter, are you still seeing this in Mozilla 1.0?
Comment 2•22 years ago
|
||
So far...It's ok in 1.0 final.
Comment 3•22 years ago
|
||
Resolving worksforme as per comment. Reporter, if this problem occurs again, please reopen this bug. Thank you!
Status: UNCONFIRMED → RESOLVED
Closed: 22 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 4•22 years ago
|
||
Hello, Would you please tell me how to re-open ? I notice that some bugs I filed had not been checked and reproduced. I wonder if the QA contact was gone...:-Q
Reporter | ||
Comment 5•22 years ago
|
||
Now it occurs again in Mozill 1.1 Alpha (2002061104), not nighty.
Reporter | ||
Comment 6•22 years ago
|
||
This occurs in Mozilla 1.1 Alpha. So I reopen this bug.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Reporter | ||
Updated•22 years ago
|
Severity: normal → major
Keywords: mozilla1.1
Comment 7•22 years ago
|
||
where ARE they saved ? its normal that mozilla creates a directory with the site name and only leaves the index.html in the folder you selected ...
Reporter | ||
Comment 9•22 years ago
|
||
>>------- Additional Comment #7 From Michael Gabriel 2002-09-27 18:02 -------
>>where ARE they saved ?
>>its normal that mozilla creates a directory with the site name and only leaves
>>the index.html in the folder you selected ...
It was saved in x:\program files\mozilla.org\mozilla\
Updated•22 years ago
|
QA Contact: trix → yulian
QA Contact: yulian → stephend
Comment 10•21 years ago
|
||
confirmed with build 2003032104 WinXP
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 11•21 years ago
|
||
Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030615
Comment 12•21 years ago
|
||
*** Bug 173949 has been marked as a duplicate of this bug. ***
Updated•21 years ago
|
Summary: "Save All" did not save files to my specific directory → "Save All" to root of drive (c:\) saves to wrong directory
Comment 13•21 years ago
|
||
*** Bug 147495 has been marked as a duplicate of this bug. ***
Comment 14•21 years ago
|
||
*** Bug 194569 has been marked as a duplicate of this bug. ***
Comment 15•21 years ago
|
||
Still there in 1.5 final. It saves the files into the current directory on the selected drive. This happens to be something like Comment 9 said, if no other program changed the dir after Mozilla.
Comment 16•21 years ago
|
||
Same bug also in Thunderbird 0.4 You can save on root in all other drives except the drive where [Mozilla's] program directory is situated
Comment 17•20 years ago
|
||
still there in Mozilla 1.7a 20040208 winxp saves to GRE base folder (C:\Program Files\Fichiers communs\mozilla.org\GRE\1.7a_2004020808) incredible ! (happilly, the download manager is able to locate it !)
Comment 18•20 years ago
|
||
It works in Thunderbird 0.4. Based on duplicates, this bug appears to be a Windows-only bug that affects both 9x and NT.
Keywords: mozilla1.1
Comment 19•20 years ago
|
||
*** Bug 235231 has been marked as a duplicate of this bug. ***
Comment 20•20 years ago
|
||
reproduced on winxp 1.7b build 2004031508
Comment 21•20 years ago
|
||
Confirmed with 2004040608-trunk/Win-2K(SP4). As aceman said in Comment #15, problem is probably described as follows ; When specified drive is same as one for "Working Directry" in shortcut(=current directry, usually Mozilla's program directry is set) and when root directry is specified on save, file save is done to current directry. This is same situation as following scenario. (On starting of Mozilla) X: CD \Program_Directry_of_Mozilla mozilla.exe (On saving file to X: from C:\TEMP) COPY C:\TEMP\FILE.EXT filename.ext => copied to current directry or COPY C:\TEMP\FILE.EXT X:filename.ext => copied to current directry This should be ; COPY C:\TEMP\FILE.EXT X:\filename.ext or X: COPY C:\TEMP\FILE.EXT \filename.ext or X: CD \ COPY C:\TEMP\FILE.EXT filename.ext
Comment 22•20 years ago
|
||
The problem is that nsLocalFile::InitWithNativePath("c:\") <http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#748> will trim the trailing slash from the directory name to end up with "c:" because it doesn't check to see if the final directory character is the 3rd character. This bit needs to be wrapped like if (pathLen > 3) { // kill any ... } However, once this is corrected (and any follow on bugs squashed), you then run into the problem of nsLocalFile::GetParent() <http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#1757> also trimming the trailing slash from "c:\" to get "c:". This bit also needs to be wrapped like if (parentPath.Length() > 3) { // cannot ... } However what is the desired result of calling GetParent() when there is no parent? Return the same directory or give an error?
Comment 23•20 years ago
|
||
This is a patch which does what I just wrote in the comment. I've lightly tested it on XP and it seems to do what it should (saves in c:\ or d:\, displays c:\ or d:\ as the current directory next time Save all attachments is shown). It also doesn't seem to break anything.
Comment 24•20 years ago
|
||
I found some problems with this patch. On vacation for a week and will update it after that.
Comment 25•20 years ago
|
||
This is a complete replacement for xpcom/tests/nsIFileTest.cpp which I needed during testing of the new patch. There is no point in attaching a diff (although I can) as this is completely rewritten. I have included all the old tests as well as created a heap of new ones. The new test program doesn't need any manual inspection of the results (it is completely automated).
Comment 26•20 years ago
|
||
This is the new proposed patch. It is for the latest current version of nsLocalFileWin.cpp Would someone please have a look through it and see what you think of the proposed changes. Essentially I have added a helper function which determines the type of path that the object is looking at because it helps to simplify things.
Attachment #145725 -
Attachment is obsolete: true
Comment 27•20 years ago
|
||
Note that the new nsIFileTest.cpp has only been tested on Windows. I've included tests for Linux which test what I assume is desired functionality for Linux (pretty much the same as Windows since nsIFile is an abstraction). I don't have access to a linux box at the moment for testing, so I would appreciate if someone else would compile and check it for me.
Attachment #147788 -
Flags: superreview?(dougt)
Attachment #147788 -
Flags: review?(blizzard)
Comment 28•20 years ago
|
||
Comment on attachment 147789 [details] [diff] [review] new patch Apologies if you aren't the correct people to request reviews from. Let me know if someone else would be preferred.
Attachment #147789 -
Flags: superreview?(dougt)
Attachment #147789 -
Flags: review?(blizzard)
Updated•20 years ago
|
Attachment #147789 -
Flags: review?(blizzard) → review?(darin)
Attachment #147788 -
Flags: review?(blizzard) → review?(darin)
Comment 29•20 years ago
|
||
Comment on attachment 147789 [details] [diff] [review] new patch You picked the right reviewers... I'll try to give this a close look sometime this week.
Comment 30•20 years ago
|
||
Comment on attachment 147789 [details] [diff] [review] new patch >+typedef enum { >+ PT_INVALID, >+ PT_LOCAL_ROOT, >+ PT_LOCAL_LEAF, >+ PT_SHARE_ROOT, >+ PT_SHARE_LEAF >+} EPathType; traditionally enums are eInterCaps >+static EPathType >+GetPathType(const nsACString & aFilePath, PRBool aValidate = PR_FALSE) >+{ >+ // all valid paths have at least 3 characters (c:\) ^^^ i know this is a windows file, but code duplication happens, please consider clarifying "valid paths" as "valid windows paths" (yes it's 1:30am and i should be sleeping). >+ // windows paths don't have forward slashes This restriction is actually fairly annoying and we might get rid of it at some point... >+ // determine if we are share or local I'm not fond of 'we' in code. 'the path is local or a share' or something ... >+ if (char1 >= 'A' && char1 <= 'Z' && char2 == ':' && char3 == '\\') >+ { >+ // local drive (e.g. c:\) >+ return (++begin == end) ? PT_LOCAL_ROOT : PT_LOCAL_LEAF; >+ } in mozilla code, placing an |else| after a |return| is frowned upon. >+ else you don't need it because the only way to reach the code following the block containing the return is if the if condition is false. ... if (char1 == '\\' && char2 == '\\' && char3 != '\\') >+ { >+ ++begin; >+ if (!FindCharInReadable('\\', begin, end)) >+ return PT_SHARE_ROOT; Can I have a multibyte computer name? I haven't checked the spec and it's too hard for me to rename my computer (it's in a domain). >+ return (++begin == end) ? PT_SHARE_ROOT : PT_SHARE_LEAF; >+ } else after return: >+ else >+ { >+ return PT_INVALID; >+ } >+} >@@ -731,44 +782,30 @@ general note, you might consider adding -p to your diff flags, it adds pretty trails to the @@ line (not useful here, but worth noting). > NS_IMETHODIMP > nsLocalFile::InitWithNativePath(const nsACString &filePath) > { >+ // check path for validity and get the type (e.g. root or leaf, local or network share) >+ EPathType type = GetPathType(filePath, PR_TRUE); >+ if (type == PT_INVALID) > return NS_ERROR_FAILURE; >+ >+ MakeDirty(); >+ >+ // this is a native path >+ char * path = ToNewCString(filePath); >+ if (!path) >+ return NS_ERROR_OUT_OF_MEMORY; >+ PRUint32 pathLen = filePath.Length(); >+ >+ // kill any trailing '\' provided it isn't the second char of DBCS >+ // and it isn't a local root this code is the bit that should remind us to worry about multibyte earlier (it sucks that I actually remembered this code when I read the earlier bit). >+ if (type != PT_LOCAL_ROOT) >+ { >+ PRUint32 len = pathLen - 1; >+ if (path[len] == '\\' && (!::IsDBCSLeadByte(path[len-1]) || >+ _mbsrchr((const unsigned char *)path, '\\') == (const unsigned char *)path+len)) >+ { >+ path[len] = '\0'; >+ pathLen = len; >+ } >@@ -1761,24 +1816,28 @@ this is where -p would be helpful > { > NS_ENSURE_ARG_POINTER(aParent); > >+ // we can't return the parent if we are at the root already I think this will break the code which I worked so hard to add. Without it, you can't find siblings of c: Yes. please don't do that.
Comment 31•20 years ago
|
||
This includes tests for win32 drive enumeration and japanese locales.
Attachment #147788 -
Attachment is obsolete: true
Comment 32•20 years ago
|
||
This new patch addresses timeless's comments, fixes problems with MBCS support, and fixes the problem with the drive enumerator. However in this version I have added the path type as a member variable to the class, I'm sure this will not be desirable. However, it's basically a trade off of either having the member var for speed, or calling the GetPathType function all the time.
Attachment #147789 -
Attachment is obsolete: true
Attachment #148403 -
Flags: superreview?(dougt)
Attachment #148403 -
Flags: review?(darin)
Attachment #147788 -
Flags: superreview?(dougt)
Attachment #147788 -
Flags: review?(darin)
Attachment #147789 -
Flags: superreview?(dougt)
Attachment #147789 -
Flags: review?(darin)
Comment 33•20 years ago
|
||
Comment on attachment 148404 [details] [diff] [review] patch version #3 Sorry for the bug spam.
Attachment #148404 -
Flags: superreview?(dougt)
Attachment #148404 -
Flags: review?(darin)
Comment 34•20 years ago
|
||
Comment on attachment 148404 [details] [diff] [review] patch version #3 Arg. My mozillas are all crashed, so i'm typing in this tiny ie window. nsLocalFile::OpenNSPRFileDesc(PRInt32 flags, PRInt32 mode, PRFileDesc **_retval) { + // ensure we have a valid path for this action + if (mPathType == ePathInvalid || mPathType == ePathLocalEnumerator) + return NS_ERROR_FILE_INVALID_PATH; Can nspr really open "\\server"? I can't imagine it can. A quick test app shows CreateFile("\\server",...) results in: Could not open file (error 123), so i'd blacklist servers too. nsLocalFile::GetParent(nsIFile * *aParent) { NS_ENSURE_ARG_POINTER(aParent); - nsCAutoString parentPath(mWorkingPath); + // ensure that we have a valid path for this action + if (mPathType == ePathInvalid || mPathType == ePathLocalEnumerator) + return NS_ERROR_FILE_UNRECOGNIZED_PATH; you're changing behavior. parent of \\. must be null. -- As for the rest of the changes in the middle. I want to be able to append to \\., it's legal. If you want to special case it so that it does something better than simply \\.\c:\ then we can do that.. Note that there are other things which are legal children of \\., See the documentation for CreateFile \\.\TAPE0 for example, also there is actually a distinction between \\.\c: and \\.\c:\ speaking of which, to accomodate that, the: + // if this isn't a local root drive (e.g. c:\) then kill any trailing backslash + // (provided it isn't the second char of DBCS) code in nsLocalFile::InitWithNativePath should probably be friendly to the distinction between them ...
Comment 35•20 years ago
|
||
Is this really desired behaviour for nsIFile/nsILocalFile? The way I understand it, it is meant to be a cross-platform abstraction mechanism for file and directory access. The sort of changes that you are wanting are supported by Win32 only (and some by NT/2K/XP only). If you are writing Win32 only code, wouldn't it be better for you to use CreateFile directly instead of sending it through nsILocalFile?
Comment 36•20 years ago
|
||
context: i'd like to be able to do everything from js. I could write my own win32 impl of nsIFile/nsILocalFile to expose extra bits of CreateFile, but that'd be fairly silly. I may never add tape support, that'd be ok. I'd like nsIFileTest to be writable using xpcshell js. I do have plans to add a share enumerator and a server enumerator. But they're not top priorities. Basically a xul app developer should be able to write a complete app js and not have to care that the os is windows or unix. When I actually finish w/ the computer enumerator, i'll probably join \\. with the other servers and have the server enumerator be the parent of both \\. and all other servers. (the server enumerator would have null as its parent.) By complete app, I mean something where the app doesn't have arbitrary limitations that make it feel broken to users of native apps on a given platform (e.g. windows).
Comment 37•20 years ago
|
||
Comment on attachment 148404 [details] [diff] [review] patch version #3 The patch for bug 243473 fixes this too.
Attachment #148404 -
Attachment is obsolete: true
Attachment #148404 -
Flags: superreview?(dougt)
Attachment #148404 -
Flags: review?(darin)
Attachment #148403 -
Attachment is obsolete: true
Attachment #148403 -
Flags: superreview?(dougt)
Attachment #148403 -
Flags: review?(darin)
Comment 38•20 years ago
|
||
*** Bug 222089 has been marked as a duplicate of this bug. ***
Comment 39•20 years ago
|
||
Might it be that this bug is resolved now (somone said so in Bug 222089)?
Comment 40•20 years ago
|
||
Thunderbird version 0.8 (20040913) In a mail with attachements, Save All... Choose a directory where the file already exists... When prompted to overwrite, click cancel... It opens a requester for the new filename... Problem: The filename starts with "C-\" instead of "C:\".
Comment 41•20 years ago
|
||
This bug is not fixed. Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041019 With MailNews, the files are no longer stored in the Mozilla.exe program directory, but in C:\program files\common files\mozilla.org\GRE\(version). In Thunderbird, they are stored in the tbird.exe program directory; TB does not have a separate GRE directory.
Comment 42•20 years ago
|
||
Yes, this bug is not fixed. The fix for bug 243473 was originally going to fix this, but it complicated that patch unduly and so I removed it. Fixing this bug isn't straight-forward as there are a number of dependencies on the internal representation of the path. The new nsIFile/nsILocalFile test harness at bug 247456 shows some of the problems with this.
Comment 43•20 years ago
|
||
This bug is still not fixed in Thunderbird version 0.9 (20041103) (In reply to comment #40) > Thunderbird version 0.8 (20040913) > > In a mail with attachements, > Save All... > Choose a directory where the file already exists... > When prompted to overwrite, click cancel... > It opens a requester for the new filename... > > Problem: > The filename starts with "C-\" instead of "C:\".
Updated•20 years ago
|
Product: MailNews → Core
Comment 44•20 years ago
|
||
*** Bug 239890 has been marked as a duplicate of this bug. ***
Comment 45•19 years ago
|
||
*** Bug 265572 has been marked as a duplicate of this bug. ***
Comment 46•19 years ago
|
||
comment 40 / comment 43 are about bug 262866, not this bug.
Comment 47•19 years ago
|
||
Just confirming that this bug still exists in Thunderbird version 1.0+ (20050626) under Windows 2000 Professional. Saved attachments are placed in the Thunderbird program directory instead of the selected "C:" location.
Comment 48•19 years ago
|
||
*** Bug 320910 has been marked as a duplicate of this bug. ***
Updated•17 years ago
|
Summary: "Save All" to root of drive (c:\) saves to wrong directory → "Save All" to root of drive (c:\) saves to wrong directory (to Working Folder/Current Directory)
Comment 50•17 years ago
|
||
Whoa, this bug already existed in 2002? Anyway, I just experienced the bug myself for the first time. Tried saving 7 JPGs from an email that was in my Local Folders to C:\ (I usually never do that, but anyway, I did now) and couldn't find them there - they were in "C:\Program Files\Mozilla Thunderbird" instead. Exactly like Mike Stockman described in the previous message here,.. over two years ago :) I'm using 2.0.0.6
Updated•16 years ago
|
Assignee: mscott → nobody
QA Contact: stephend → attachments
Assignee | ||
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 51•15 years ago
|
||
Is this true on XP (sorry only have Vista who dislikes writing on c:\) ? can anyone confirm it is still an issue on recent thunderbird 3.0 releases (http://stage.mozillamessaging.com/en-US/thunderbird/early_releases/downloads/) ?
Comment 53•15 years ago
|
||
Reverified with current builds on Windows XP. While this is still reproducible for TB 2.0.0.22 (attachment saved to installation folder), the 20090715 nightly for TB 3.0b3pre saves correctly to the root of C:\ or any other drive selected. Also works for the SM 2.0b1pre nightly. -> WFM (fixed on trunk, not a candidate to get fixed on the 1.8.1 branch)
Status: NEW → RESOLVED
Closed: 22 years ago → 15 years ago
Resolution: --- → WORKSFORME
Updated•15 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•