Closed Bug 1033963 Opened 10 years ago Closed 10 years ago

Add mailnews.message_warning_size to prefs.

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: sshagarwal, Assigned: sshagarwal)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

This pref is used here:
http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#3239

to warn the user of large message size:
http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#3390
But it doesn't get set anywhere in the profile.

This bug is about adding the pref and improving its warning message "sendLargeMessageWarning" to a more user-friendly
statement.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #8450061 - Flags: feedback?(acelists)
Comment on attachment 8450061 [details] [diff] [review]
Patch v1

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

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +251,5 @@
>  copyMessageComplete=Copy complete.
>  copyMessageFailed=Copy failed.
>  
> +## Do not translate %S. It is the size of the message in user-friendly notation.
> +sendLargeMessageWarning=Warning! You are about to send a %S message. Are you sure that you want to do this?

You need to change the string name when you change the value. I would still like to have some "size" word in the message (the old word "byte" indicated it). Like "You are about to send a message of %S in size." The UI reviewers will tell better.

Also update the /suite version of the string.

::: mailnews/compose/src/nsMsgSend.cpp
@@ -3410,5 @@
> -          PR_Free(printfString);
> -          return NS_ERROR_FAILURE;
> -        }
> -        else
> -          PR_Free(printfString);

Looks like great cleanup removing the char16_t *printfString that needs to be freed manually.

::: mailnews/mailnews.js
@@ +477,5 @@
>  pref("mail.spam.display.sanitize", true); // display simple html for html junk messages
>  // the number of allowed bayes tokens before the database is shrunk
>  pref("mailnews.bayesian_spam_filter.junk_maxtokens", 100000);
>  
> +// hidden pref to warn the users of exceeding the size of the message being composed.

The pref is no longer hidden when we expose it in this file :)

@@ +478,5 @@
>  // the number of allowed bayes tokens before the database is shrunk
>  pref("mailnews.bayesian_spam_filter.junk_maxtokens", 100000);
>  
> +// hidden pref to warn the users of exceeding the size of the message being composed.
> +pref("mailnews.message_warning_size", 0);

Anyway I'd like it to be listed here (not hidden) and I would propose a size of 100MB. Otherwise the feature will not used by many people.
Attachment #8450061 - Flags: feedback?(acelists)
(In reply to :aceman from comment #2)
> mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> @@ +251,5 @@
> > +sendLargeMessageWarning=Warning! You are about to send a %S message. Are you sure that you want to do this?
> 
> You need to change the string name when you change the value. I would still
> like to have some "size" word in the message (the old word "byte" indicated
> it). Like "You are about to send a message of %S in size." The UI reviewers
> will tell better.
Thanks for the quick feedback.
I left it the same as we are temporarily changing the name in 952493 which will be renamed back to this name again :).
Also, As I see it, FormatFileSize() method is adding kB, MB itself so do we still need to write something here?

> Also update the /suite version of the string.
Will do.

> ::: mailnews/mailnews.js
> @@ +477,5 @@
> >  pref("mail.spam.display.sanitize", true); // display simple html for html junk messages
> >  // the number of allowed bayes tokens before the database is shrunk
> >  pref("mailnews.bayesian_spam_filter.junk_maxtokens", 100000);
> 
> Anyway I'd like it to be listed here (not hidden) and I would propose a size
> of 100MB. Otherwise the feature will not used by many people.
Okay will do.
But 100MB?
Should we have it a bit smaller that's achievable like 5-10MB? :)

Thanks.
(In reply to Suyash Agarwal (:sshagarwal) from comment #3)
> > Anyway I'd like it to be listed here (not hidden) and I would propose a size
> > of 100MB. Otherwise the feature will not used by many people.
> Okay will do.
> But 100MB?
> Should we have it a bit smaller that's achievable like 5-10MB? :)

In enterprises 5-10MB may be a common size to send.
It would be great if the admin set this pref to the max value allowed in the company.
But for home users I don't know. The limit may not be known. The SMTP server or other servers on the path to recipient may reject the big message. But we do not want to show the warning too often. Having it at 0 disables the feature so it is dead code. I vote for 100MB to catch some really bad cases where there is a high probability the message is really too big to send. Lowering it increases the chance of warning needlessly.
I don't think you usually can send anything close to 100M (and expect it to go through). Googling says limits are currently GMail 25M, Outlook 20M, Yahoo 25M. IIRC my IPSs have usually had crappy 10M limits... I suppose 20M could be a reasonable limit to warn by.
Sure, whatever a sane limit is.
Can we extend the warning to say why it is actually a problem? Some users may not be aware there are any limits of message size. So we could say the "size may exceed the allowed limit on the mail servers".
Attached patch Patch v1.6Splinter Review
Made the suggested changes. Please let me know if this is correct.

Thanks.
Attachment #8450061 - Attachment is obsolete: true
Attachment #8450369 - Flags: ui-review?(richard.marti)
Attachment #8450369 - Flags: review?(mkmelin+mozilla)
Attachment #8450369 - Flags: feedback?(acelists)
Comment on attachment 8450369 [details] [diff] [review]
Patch v1.6

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

I haven't run with it but the concept looks fine now.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +251,5 @@
>  copyMessageComplete=Copy complete.
>  copyMessageFailed=Copy failed.
>  
> +## LOCALIZATION NOTE (largeMessageSendWarning):
> +## Do not translate %S. It is the size of the message in user-friendly notation.

We write the localization comment directly after the colon (see copyMessageStart above).
Attachment #8450369 - Flags: feedback?(acelists) → feedback+
Comment on attachment 8450369 [details] [diff] [review]
Patch v1.6

Looks good, thanks.
Attachment #8450369 - Flags: ui-review?(richard.marti) → ui-review+
If an attachment exceeds 5MB, than this will allready trigger the Filelink warning message (mail.compose.big_attachments.threshold_kb). So, I'm wondering about the benefit of this additional warning? If I try to send a message with an > 20MB attachment, than I will see two warnings, the Filelink one and this new one, wouldn't I?
The Filelink warning comes directly after attaching a big file. After choosing Ignore it does never appear again. If it's the only file and it doesn't reach the warning-size limit, everything is okay. But you can pack again and again other attachments without any warning after declining the filelink message. It is good now to warn on send the user if he reached the warning_size.
Comment on attachment 8450369 [details] [diff] [review]
Patch v1.6

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

LGTM, r=mkmelin
Attachment #8450369 - Flags: review?(mkmelin+mozilla) → review+
Thanks.
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/ec27402dedd5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
You need to log in before you can comment on or make changes to this bug.