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

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Anton Solovyev]

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

Comment 1

[Nikolay Shopik]

reporter is gone.

Comment 2

[Nikolay Shopik]

confirm version 3.0a2pre (2008060703)

Comment 3

[Magnus Melin [:mkmelin]]

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.

Comment 4

[Thomas D. (:thomas8)]

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.

Comment 5

[Magnus Melin [:mkmelin]]

Generally if you want to do anything with files using command line or in scripts, spaces in filenames will make things much harder.

Comment 6

[Thomas D. (:thomas8)]

(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

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

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

Comment 8

[Magnus Melin [:mkmelin]]

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)

Comment 9

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

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.

Comment 10

[:aceman]

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.

Comment 11

[[e-mail]]

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?

Comment 12

[Magnus Melin [:mkmelin]]

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

Comment 13

[Magnus Melin [:mkmelin]]

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.

Comment 14

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Made the changes.
Since the changes are significant, re-requesting review.

Comment 15

[:aceman]

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"

Comment 16

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Fixed.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 790527 [details] [diff] [review]
Patch v4

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

r=mkmelin, sorry for the delay

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/da1554544210

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 20

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5

Moved it below the if ... else block.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 22

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5 (revised)

Sorry, actually I thought that previous one would be backed out.

Fixed.
Carrying over review from mkmelin.

Comment 23

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/8fbe4c7c058d