Closed Bug 365386 Opened 14 years ago Closed 8 years ago

Implement option / pref to replace spaces (" ") with underscores ("_") in file names when saving messages as .eml files

Categories

(Thunderbird :: Mail Window Front End, enhancement)

All
Linux
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 26.0

People

(Reporter: solovam, Assigned: sshagarwal)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [good first bug][mentor=mkmelin])

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.9) Gecko/20061206 Firefox/1.5.0.9
Build Identifier: version 1.5.0.2 (20060420)

When e-mails are saved the default file name is the message subject with .eml extension. The file name will almost always have spaces in it. This is bad in UNIX shells. An underscore instead of space would be much better.

Reproducible: Always

Steps to Reproduce:
1. Hit File -> Save As
2. Observe the file name offered
3.
Actual Results:  
The default file name has spaces in it

Expected Results:  
I would propose replacing spaces with underscores

Does it never get tested on anything but Windows???
reporter is gone.
confirm version 3.0a2pre (2008060703)
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Assignee: mscott → nobody
Keywords: qawanted
GenerateFilenameFromMsgHdr is here: http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#357

This should probably be pref-controlled. I imagine windows users (and likely osx too) do want spaces.
Keywords: qawanted
Hardware: x86 → All
Whiteboard: [good first bug][mentor=mkmelin]
Perhaps we can first verify if and how this is a problem at all.
"This is bad in Unix Shells" is not a very precise description of the problem.

On windows, yes, we definitely want spaces in the filename.
Generally if you want to do anything with files using command line or in scripts, spaces in filenames will make things much harder.
(In reply to Magnus Melin from comment #5)
> Generally if you want to do anything with files using command line or in
> scripts, spaces in filenames will make things much harder.

I was mainly taking offence at the impression created by comment 0 that this is a bug in TB:

> Does it never get tested on anything but Windows???

Linux can handle spaces in filenames, and for scripts and command lines, the workaround can be as simple as adding quotes around the filename (alternatively, escape spaces), see [1]. Even for more advanced batch processing commands, handling spaces in file names does not appear to cause any significant problem except that you have to remember the fact; see [2].

So this isn't a bug in TB, perhaps a minor inconvenience to command shell users on Linux -> enh

[1] http://www.tuxfiles.org/linuxhelp/weirdchars.html
[2] http://unixjunkie.blogspot.de/2007/01/handling-filenames-with-spaces_15.html
Severity: minor → enhancement
Summary: When message is saved default .eml file name has spaces in it → Implement option / pref to replace spaces (" ") with underscores ("_") in file names when saving messages as .eml files
Attached patch Patch (obsolete) — Splinter Review
I have preferred space style and hence left it as default (due to the large number of Windows and Mac users & the fact that setting the pref would be left for linux users ;) ).
Attachment #788555 - Flags: review?(mkmelin+mozilla)
Attachment #788555 - Flags: feedback?(acelists)
Comment on attachment 788555 [details] [diff] [review]
Patch

Review of attachment 788555 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok, but haven't tried it yet. Please fix some minor things first

::: mail/app/profile/all-thunderbird.js
@@ +831,5 @@
>  
>  // If set to true, Thunderbird will collapse the main menu for new profiles
>  // (or, more precisely, profiles that start with no accounts created).
>  pref("mail.main_menu.collapse_by_default", true);
> +// mailCommands.js

remove this

@@ +834,5 @@
>  pref("mail.main_menu.collapse_by_default", true);
> +// mailCommands.js
> +// Let the bool value to be false if you need spaces in the file name while
> +// saving messages in .eml files (usually for Windows and OSX users) else
> +// set it to true for using underscores (usually for linux users).

Probably no need to assume who wants what in the comment. 

// If set to true, when saving a messages file, use underscore instead of space in the file name.

@@ +835,5 @@
> +// mailCommands.js
> +// Let the bool value to be false if you need spaces in the file name while
> +// saving messages in .eml files (usually for Windows and OSX users) else
> +// set it to true for using underscores (usually for linux users).
> +pref("underscoreInFileName", false);

Prefs are usually in the format mail.something.possiblysomethingelse

mail.save_msg_filename_underscores_for_space perhaps?

Probably want to have it defaulting to true on linux using
#ifdef UNIX_BUT_NOT_MAC

::: mail/base/content/mailCommands.js
@@ +334,5 @@
>  
>      let filename = GenerateValidFilename(name, ".eml");
> +    if (Services.prefs.getBoolPref("underscoreInFileName")) {
> +      filename = filename.replace(/ /g, '_');
> +    }

two nits here and the similar case: 1) no need for braces 2) prefer double quotes (for consistency)
Attachment #788555 - Flags: review?(mkmelin+mozilla) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Fixed the nits.

In reply to:
>two nits here and the similar case: 1) no need for braces 
Actually I was trying to follow what you said that if one if else has braces, all should have, so I thought it applies to nesting too.
Attachment #788555 - Attachment is obsolete: true
Attachment #788555 - Flags: feedback?(acelists)
Attachment #789073 - Flags: review?(mkmelin+mozilla)
Attachment #789073 - Flags: feedback?(acelists)
Comment on attachment 789073 [details] [diff] [review]
Patch v2

Review of attachment 789073 [details] [diff] [review]:
-----------------------------------------------------------------

Yes this seems to work but you add duplicate code. I am not sure why the filenames are generated differently in case of 1 vs. more messages to save, but that is not for this bug. Can you put the underscore-for-space logic inside validateFileName() so that it is in a single place? And there are already filename manglings for other OSes.

::: mail/app/profile/all-thunderbird.js
@@ +837,5 @@
> +#ifdef UNIX_BUT_NOT_MAC
> +pref("mail.save_msg_filename_underscores_for_space", true);
> +#else
> +pref("mail.save_msg_filename_underscores_for_space", false);
> +#endif

I'd like the Seamonkey guys to comment on this whether they would like the pref to be in "mailnews." namespace so that they can pick it up for the SM version of this feature.
Attachment #789073 - Flags: feedback?(neil)
Attachment #789073 - Flags: feedback?(iann_bugzilla)
Attachment #789073 - Flags: feedback?(acelists)
Attachment #789073 - Flags: feedback-
Comment on attachment 789073 [details] [diff] [review]
Patch v2

I have no idea why some preferences begin mail. and others mailnews.

I can't imagine wanting to port this change for SeaMonkey - filenames have spaces when we save web pages, so why should emails be any different?
Attachment #789073 - Flags: feedback?(neil)
(In reply to :aceman from comment #10)
> Comment on attachment 789073 [details] [diff] [review]
> -----------------------------------------------------------------
> 
> Yes this seems to work but you add duplicate code. I am not sure why the
> filenames are generated differently in case of 1 vs. more messages to save,

When you have many msgs they are quite likely to be from the same thread and therefore more info than just the subject is needed. For one file that's not the case.
Assignee: nobody → syshagarwal
Comment on attachment 789073 [details] [diff] [review]
Patch v2

Review of attachment 789073 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. After trying it out i think it's not as usable as one might imagine since there's other chars also that are not optimal... 
So lets default to not using it on linux too after all. r=mkmelin with that fixed.
Attachment #789073 - Flags: review?(mkmelin+mozilla) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
Made the changes.
Since the changes are significant, re-requesting review.
Attachment #789073 - Attachment is obsolete: true
Attachment #789073 - Flags: feedback?(iann_bugzilla)
Attachment #790408 - Flags: review?(mkmelin+mozilla)
Attachment #790408 - Flags: feedback?(acelists)
Comment on attachment 790408 [details] [diff] [review]
Patch v3

Review of attachment 790408 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this looks better to me.

::: mail/app/profile/all-thunderbird.js
@@ +831,5 @@
>  
>  // If set to true, Thunderbird will collapse the main menu for new profiles
>  // (or, more precisely, profiles that start with no accounts created).
>  pref("mail.main_menu.collapse_by_default", true);
> +// If set to true, when saving a messages file, use underscore instead

This should probably be "// If set to true, when saving a message to a file, use underscore instead"
Attachment #790408 - Flags: feedback?(acelists) → feedback+
Attached patch Patch v4 (obsolete) — Splinter Review
Fixed.
Attachment #790408 - Attachment is obsolete: true
Attachment #790408 - Flags: review?(mkmelin+mozilla)
Attachment #790527 - Flags: review?(mkmelin+mozilla)
Comment on attachment 790527 [details] [diff] [review]
Patch v4

Review of attachment 790527 [details] [diff] [review]:
-----------------------------------------------------------------

r=mkmelin, sorry for the delay
Attachment #790527 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/da1554544210
Status: NEW → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 26.0
Reopening because there can still be whitespaces in the filename.

Cause:
https://hg.mozilla.org/comm-central/rev/da1554544210#l2.18
adds whitespaces after the replacing gets done in
https://hg.mozilla.org/comm-central/rev/da1554544210#l2.13
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v5 (obsolete) — Splinter Review
Moved it below the if ... else block.
Attachment #790527 - Attachment is obsolete: true
Attachment #796013 - Flags: review?(mkmelin+mozilla)
Attachment #796013 - Flags: feedback?(archaeopteryx)
Attachment #796013 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 796013 [details] [diff] [review]
Patch v5

The patch already landed on comm-central, so you patch shouldn't touch anymore all-thunderbird.js and move the code utilityOverlay.js a few lines down. At the moment, applying your patch on comm-central tip should fail.
Attachment #796013 - Flags: feedback?(archaeopteryx) → feedback-
Sorry, actually I thought that previous one would be backed out.

Fixed.
Carrying over review from mkmelin.
Attachment #796013 - Attachment is obsolete: true
Attachment #796193 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/8fbe4c7c058d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Blocks: TB2SM
You need to log in before you can comment on or make changes to this bug.