223549 - use validateFileName instead of GenerateValidFilename

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

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

from the checkin for Bug 92726

it added a function
+function GenerateValidFilename(filename, extension)
however, there is already validateFileName which seems to do the same job and
should probably be used instead (contentAreaUtils.js)

that code should use validateFileName at
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/contentAreaUtils.js#826
instead of reimplementing it

Comment 1

[Boris Zbarsky [:bzbarsky]]

.

Comment 2

[Ian Neal]

Just looking through the two functions there are similarities but also differences:
GenerateValidFilename strips out " completely
validateFileName replaces " with ' on windows only
GenerateValidFilename removes whitespace from the beginning and end
validateFileName doesn't do any checks for leading/trailing whitespace

GenerateValidFilename replaces each occurance of space or .\@/: with _
On windows
validateFileName replaces each occurance of *:? with a space and \/| with _
validateFileName replaces each occurance of < with ( and > with )
On macs
validateFileName replaces each occurance of :/ with _
On platforms that are neither windows or macs
validateFileName replaces each occurance of / with _

The questions that come to mind are:
1. On non-windows/non-mac platforms I'd say space was a bad thing, yes?
2. What should be done about @ and . and would it be the same for all platforms?

I'd say that GenerateValidFilename could call a slightly modified
validateFileName (as discussed just above) after stripping trailing/leading
whitespace and then return the result with the appended extension.

Thoughts?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 1. On non-windows/non-mac platforms I'd say space was a bad thing, yes?

No.  It's not.

> 2. What should be done about @ and . and would it be the same for all platforms?

Nothing should be done with them, imho.

Comment 4

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

I agree with bz

Comment 5

[Ian Neal]

>> 1. On non-windows/non-mac platforms I'd say space was a bad thing, yes?

>No.  It's not.

Maybe not a bad thing but certaintly not a good thing IMHO.

The same could be said for filenames with | or \ or * or ? in on
unix/linux/other systems.

From looking at
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/contentAreaUtils.js#826
it seems to suggest that :/ are the illegal characters for mac platforms but
http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsCRT.h#276 suggests it is just :

Which is correct or is it best to treat OSX the same as linux/unix?
Should OS2 be treated the same as Windows?
Should BeOS be treated the same as linux/unix?

Currently I'm proposing that:
On unix/linux platforms we don't use <>\/|:?*"' and space
On windows/os2 platforms we don't use <>\/|:?*"
On other platforms assume same as unix/linux

Which means:
On all platforms:
replace each occurance of < with ( and > with )
On windows/os2 platforms:
replace each occurance of \: with _
replace each occurance of /|?* with space
replace each occurance of " with '
On linux/unix/other platforms:
strip out each occurance of " and '
replace each occurance of \/|:?* and space with _

Comment 6

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

>Maybe not a bad thing but certaintly not a good thing IMHO. 
>The same could be said for filenames with | or \ or * or ? in on 
>unix/linux/other systems 
 
why? those are perfectly fine characters in filenames. most especially space 
is. and why replace : as your algorithm suggests? maybe the < and > replacing 
should also only be done on windows. 

Comment 7

[[e-mail]]

I've had a look at the file pickers. Now they have an attribute defaultString
which, at least on Windows, validates the file name. However all of the file
pickers allow this default string to be a path name despite nobody actually
using it in that way. Would it therefore be too much to ask if a) the interface
was clarified so that the defaultString does not contain a path component b) the
validation was moved into the defaultString setter?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Most Unix software can deal with spaces in filenames, really.  The cases that
can't are few, far between, and just plain badly coded...

Notice that this function is not about providing "safe" filenames.  It's about
providing _valid_ filenames (i.e. ones that the filesystem itself will not barf on).

GenerateValidFilename, in spite of the function name, is NOT about generating a
valid filename.  Perhaps it should be called GenerateSafeFilename?

In any case, the remaining argument is about whether validateFileName should in
fact produce a safe name instead.  Since the user will always have to confirm
the filename, I don't know that that's necessary... and people will complain if
it changes filenames too much.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and maybe Neil is on the right track.  I don't know who owns the filepicker
interface nowadays, but it may indeed make sense to move this logic into the
filepickers.

Comment 10

[Ian Neal]

Attachment: Patch v0.1 - make GenerateValidFilename use validateFileName

moves function validateFileName to utilityOverlay.js and adapts function
GenerateValidFilename to use that moved function.

Comment 11

[Ian Neal]

Accepting

Comment 12

[Boris Zbarsky [:bzbarsky]]

> +    filename = validateFileName(filename).replace(/(^\s+)|(\s+$)/g, '');

You probably want to lose the parens -- no reason for them here.

Comment 13

[Ian Neal]

Attachment: Patch v0.1a - as per patch v0.1 except without extra parens

Comment 14

[Ian Neal]

Attachment: Patch v0.1b - adjusts a check after return from function

GenerateValidFilename returns null rather than "", so adjusted check after
return to look for non-null rather than not "".

Comment 15

[[e-mail]]

Comment on attachment 134294 [details] [diff] [review]
Patch v0.1b - adjusts a check after return from function

As a JS solution it's OK, I suppose...

Comment 16

[Ian Neal]

Comment on attachment 134294 [details] [diff] [review]
Patch v0.1b - adjusts a check after return from function

Let's try someone who's not on vacation

Comment 17

[David :Bienvenu]

Comment on attachment 134294 [details] [diff] [review]
Patch v0.1b - adjusts a check after return from function

looks ok. sr=bienvenu

Comment 18

[[e-mail]]

Fix checked in.

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using LXR for code inspection.