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

.

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?

> 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.

I agree with bz

>> 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 _

>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.

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?

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.

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.

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

Accepting

> + filename = validateFileName(filename).replace(/(^\s+)|(\s+$)/g, ''); You probably want to lose the parens -- no reason for them here.

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

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 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 on attachment 134294 [details] [diff] [review] Patch v0.1b - adjusts a check after return from function looks ok. sr=bienvenu

Fix checked in.

Verified FIXED using LXR for code inspection.