302184 - download manager renames existing files by incrementing number suffix if you use auto-download

Status: RESOLVED DUPLICATE of bug 381157

(SeaMonkey :: Download & File Handling, defect)

Description

[Hideo Oshima]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050725 SeaMonkey/1.0a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050725 SeaMonkey/1.0a

Seamonkey has the same bug as bug 231048 for firefox.


Reproducible: Always

Comment 1

[OstGote!]

Really? It's new to me that this was patched for Mozilla/SM trunk. HAve I missed
that?

Comment 2

[baffclan]

Try this file:
http://ftp.mozilla.org/pub/mozilla.org/mozilla/source/wintools-19990429.zip

SeaMonkey/2005072605-trunk/WinXP
wintools-19990429.zip (ok)
wintools-19990430.zip
wintools-19990431.zip
(bug 231048 comment 66)

Firefoz/2005072506-trunk/WinXP
wintools-19990429.zip
wintools-19990429(2).zip
wintools-19990429(3).zip

Comment 3

[Adam Guthrie]

Confirming. I guess this didn't fix SeaMonkey after all.

Comment 4

[OstGote!]

(In reply to comment #1)
> Really? It's new to me that this was patched for Mozilla/SM trunk. HAve I missed
> that?

Yes :-/ I found it. This happens only with the auto-download feature. Only as
note: This was implemented with Bug 7840.

Comment 5

[OstGote!]

I guess it is fairly easy to port the FF patch from bug 231048.

Comment 6

[James Napolitano]

Attachment: port of fix from Firefox's code

My very first patch!  Hopefully the first of many.  I simply ported over the new renaming code from the Firefox bug.  I tested that it works as expected with that wintools example file given above.  Now I can just figure out who to request review from...

Comment 7

[James Napolitano]

Comment on attachment 202651 [details] [diff] [review]
port of fix from Firefox's code

Found someone to request review from from Bonsai.  This is a low risk change so it probably can go into the SeaMonkey 1.0 branch as well as the Trunk.

Comment 8

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 202651 [details] [diff] [review]
port of fix from Firefox's code

I'm not qualified to review this.   Also, you patched code in toolkit/, which isn't used by SeaMonkey.  The relevant SeaMonkey code is in xpfe/

Comment 9

[James Napolitano]

Attachment: correct patch

Oops!  I attached the patch to the original FX bug; here is my patch to Seamonkey's code.

Comment 10

[James Napolitano]

Chris, can you review the xpfe code, or should I find someone else?

Comment 11

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #10)
> Chris, can you review the xpfe code, or should I find someone else?
> 

In general I can't review any FF/TB/SM code, so you'll have to find someone else.

Comment 12

[James Napolitano]

Comment on attachment 202693 [details] [diff] [review]
correct patch

Requesting review from cbiesinger for this simple patch, ported from a fix for Firefox.  Chris, thanks for the quick response.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 202693 [details] [diff] [review]
correct patch

I think I don't want to own this autodownload code, changing request to the original reviewer of this function

Comment 14

[[e-mail]]

Comment on attachment 202693 [details] [diff] [review]
correct patch

a) I'm not convinced that we want to change from foo-1.ext to foo (2).ext
b) you only need to match once, and concatenate the leaf name from the count

Comment 15

[James Napolitano]

(In reply to comment #14)
> (From update of attachment 202693 [details] [diff] [review] [edit])
> a) I'm not convinced that we want to change from foo-1.ext to foo (2).ext

<nit>Actually my patch would rename 'foo.ext' to 'foo(2).ext', w/o any space</nit>, but I see your point.

Having numbers separated by dashes/hyphens is common enough in file names on the web (see comment 2) that we would want to use some other suffix for renaming duplicate files.  There was some discussion in bug 231048 for FX about what renaming scheme to use.  We could try some other scheme like 'foo[2].ext', 'foo{2}.ext', 'foo.2.ext', or 'foo.copy1.ext'.  There rarely are spaces in the file names on the web (since people use _'s or -'s instead), so we could try 'foo (2).ext', or, since % is used an escape character in URLs, 'foo%2.ext'.  Or we could use a prefix like 'Copy(1) of foo.ext'.

Any preferences?

> b) you only need to match once, and concatenate the leaf name from the count
> 
Good catch.  I just copied over the new code from the FX patch, since I thought it would be nice to use the same code in FX and SeaMonkey, but since we're changing the code anyway, I'll rewrite it to be optimized.  Just want to get my grad school applications done first...

BTW, since there is a maximum file name length (at least with Windows), I wonder  what would happen if we automatically downloaded a second version of file with a name that already was at max length, and then tried to append '(2)' or '-1'.  Maybe we could consider that case and add a JS alert when it happens.

Comment 16

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #14)
> (From update of attachment 202693 [details] [diff] [review] [edit])
> a) I'm not convinced that we want to change from foo-1.ext to foo (2).ext

When creating multiple copies of the same file, Windows uses the "foo (2).ext" naming format.

Comment 17

[[e-mail]]

(In reply to comment #16)
>When creating multiple copies of the same file, Windows uses the "foo (2).ext"
>naming format.
That doesn't make it right :-P

Comment 18

[[e-mail]]

How about foo.2.ext?

Now that I look at it again, I'm not convinced by the .tar.gz detection - people will complain that foo.tar.bz3 (or whatever) isn't detected.

Comment 19

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #17)
> (In reply to comment #16)
> >When creating multiple copies of the same file, Windows uses the "foo (2).ext"
> >naming format.
> That doesn't make it right :-P
I would think it does, at least on Windows.

Comment 20

[Serge Gautherie (:sgautherie)]

See

<http://mxr.mozilla.org/mozilla/find?text=&kind=text&string=%2FcontentAreaUtils.js>
Both are currently shipped. (Should try and merge them, if possible.)

<http://mxr.mozilla.org/seamonkey/search?string=uniqueFile&case=on&find=%5C.js%24&findi=%5C.h%24&tree=seamonkey>
Should look into merging them, if possible. (See FF bug 296267.)

<http://mxr.mozilla.org/seamonkey/search?string=makeFileUnique&case=on&tree=seamonkey>

Comment 21

[Robert Kaiser]

This got fixed by bug 381157, see http://hg.mozilla.org/comm-central/rev/178d054e3163#l5.151

We probably should file a bug on the issue bug 296267 was for though. Serge, interested in that?