Last Comment Bug 68784 - when sending mail, should first check "no recipient", then "no subject" (it's currently the other way round)
: when sending mail, should first check "no recipient", then "no subject" (it's...
Status: RESOLVED FIXED
: polish
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Thunderbird 23.0
Assigned To: :aceman
:
Mentors:
: 528819 846877 (view as bug list)
Depends on: 872041
Blocks: TB2SM 431217
  Show dependency treegraph
 
Reported: 2001-02-14 01:22 PST by Henrik Gemal
Modified: 2013-06-17 06:10 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
I added a check for the to field before checking the subject field (2.59 KB, patch)
2009-12-13 14:40 PST, muhammad
standard8: review-
clarkbw: ui‑review+
Details | Diff | Splinter Review
patch (2.28 KB, patch)
2013-01-25 15:49 PST, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
WIP patch v2 (9.18 KB, patch)
2013-01-28 15:40 PST, :aceman
standard8: feedback+
mkmelin+mozilla: feedback+
Details | Diff | Splinter Review
patch v3 (10.20 KB, patch)
2013-03-10 07:40 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch v4 (10.43 KB, patch)
2013-03-13 10:53 PDT, :aceman
standard8: review-
Details | Diff | Splinter Review
patch v5 (11.11 KB, patch)
2013-03-22 10:55 PDT, :aceman
mkmelin+mozilla: review-
Details | Diff | Splinter Review
patch v6 (11.12 KB, patch)
2013-03-24 05:47 PDT, :aceman
no flags Details | Diff | Splinter Review
patch v7 (10.52 KB, patch)
2013-03-27 13:24 PDT, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
patch v8 (10.52 KB, patch)
2013-04-03 14:11 PDT, :aceman
standard8: superreview+
Details | Diff | Splinter Review

Description Henrik Gemal 2001-02-14 01:22:40 PST
When you compose a new message mozilla first checks to see if there's any
subject and then if there's any recipients. The check should be the other way
around.
Comment 1 stephend@netscape.com (gone - use stephen.donner@gmail.com instead) 2001-02-15 11:05:51 PST
Henrik, are you referring to us loading the body, then the subject and then
recipients?  If so, I think this is covered in bug 62452, "message compose
window should have To field focused as soon as it opens"
Comment 2 Henrik Gemal 2001-02-15 11:18:16 PST
it's when sending...
Comment 3 Håkan Waara 2001-04-07 16:15:55 PDT
Henrik, why should it be the other way around? Is there any good reason of why
to put effort into fixing this bug?
Comment 4 Henrik Gemal 2001-05-14 05:22:44 PDT
moving the check for "no recipient" from the cpp code to js, would also make it 
easier to set focus after the alert to the To field.
Comment 5 Jean-Francois Ducarroz 2001-05-14 08:43:19 PDT
reassign to varada. Before doing this kind of modification, we need to check with the UI team.
Comment 6 (not reading, please use seth@sspitzer.org instead) 2002-12-16 12:53:07 PST
taking all of varada's bugs.
Comment 7 (not reading, please use seth@sspitzer.org instead) 2007-06-21 14:36:23 PDT
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Comment 8 Serge Gautherie (:sgautherie) 2008-06-20 10:36:58 PDT
Filter on "Nobody_NScomTLD_20080620"
Comment 9 Thomas D. (currently busy elsewhere; needinfo?me) 2009-11-15 11:28:42 PST
This looks like a good and rather simple idea to polish. You can't send without recipient, so user should fix that first. He'll take another look at the message and possibly notice the missing subject, too. In that case, the need for showing the "no subject" message would no longer arise. Top-down approach to this seems more intuitive.
Comment 10 muhammad 2009-12-13 14:40:15 PST
Created attachment 417396 [details] [diff] [review]
I added a check for the to field before checking the subject field
Comment 11 Mark Banner (:standard8) 2009-12-14 01:18:08 PST
Comment on attachment 417396 [details] [diff] [review]
I added a check for the to field before checking the subject field

Please select a correct reviewer from

https://developer.mozilla.org/en/Mailnews_and_Mail_code_review_requirements

+                                   bundle.getString("12556"),
+                                   bundle.getString("12511"),

These lines look wrong - there are #defines for strings that should be used at a minimum.
Comment 12 Mark Banner (:standard8) 2010-02-01 03:30:05 PST
Comment on attachment 417396 [details] [diff] [review]
I added a check for the to field before checking the subject field

Bryan, this patch needs a little work, but the idea is possibly reasonable. It will duplicate some of the checking that is done in the back end, but if absolutely nothing is entered in the compose fields, then it will prompt before subject.

If invalid items (or just some text and no email address) is enterned, then it will still prompt after the subject. We can fix that with a bit more duplication, but I thought I'd like to get your thoughts on this first.
Comment 13 Bryan Clark (DevTools PM) [@clarkbw] 2010-02-05 08:04:32 PST
Comment on attachment 417396 [details] [diff] [review]
I added a check for the to field before checking the subject field

yeah, the idea is right to me.  you need to have a sender, a subject is actually secondary (and not required)
Comment 14 Mark Banner (:standard8) 2010-02-08 07:50:00 PST
Comment on attachment 417396 [details] [diff] [review]
I added a check for the to field before checking the subject field

Thanks for proposing the patch. Sorry for the delay in reviewing this.

Bryan says the idea is fine, which is good. However I think we could do better with the implementation.

nsMsgSend already does a very similar check:

http://hg.mozilla.org/comm-central/file/7073dcec0ba3/mailnews/compose/src/nsMsgSend.cpp#l3044

Looking at nsMsgSend, that is dealing with an object derived from nsIMsgCompFields. In your patch, you are in GenericSendMessage and a little bit above where your code is, msgCompFields is filled in. This is also an nsIMsgCompFields.

Therefore I think what you could do to eliminate similar code in two places is:

1) Add a read-only attribute, e.g. "recipientsAreEmpty" to nsIMsgCompFields:

http://mxr.mozilla.org/comm-central/source/mailnews/compose/public/nsIMsgCompFields.idl

2) In the get function for that attribute, do the call to mime_sanity_check_fields that you see in nsMsgSend.

3) Replace the call in nsMsgSend with a call to get the new attribute, and likewise use the call in your javascript code.

How does that sound?

We also now have a policy of all patches requiring unit tests to go with them. See https://developer.mozilla.org/en/Thunderbird/Thunderbird_Automated_Testing for more information.

If you feel you're not capable of doing unit tests, let me know and I can work with you to produce some.
Comment 15 Thomas D. (currently busy elsewhere; needinfo?me) 2013-01-14 13:30:11 PST
has draft patch with review comments what to change (comment 14) - aceman, would you be able to finish this off?
Comment 16 :aceman 2013-01-14 14:25:27 PST
What about just moving CheckValidEmailAddress up inside GenericSendMessage?
But I'd like to do what standard8 suggests I just am not sure we need full mime_sanity_check_fields function. It seems it also takes subject and other fields and we'd need to distinguish what exactly failed to show different alerts.
Comment 17 Mark Banner (:standard8) 2013-01-24 03:07:01 PST
(In reply to :aceman from comment #16)
> What about just moving CheckValidEmailAddress up inside GenericSendMessage?

That feels like it is worth a try.

> But I'd like to do what standard8 suggests I just am not sure we need full
> mime_sanity_check_fields function. It seems it also takes subject and other
> fields and we'd need to distinguish what exactly failed to show different
> alerts.

That's an old suggestion, so feel free to come up with something new/better :-)
Comment 18 :aceman 2013-01-25 15:49:44 PST
Created attachment 706647 [details] [diff] [review]
patch
Comment 19 Magnus Melin 2013-01-26 04:04:06 PST
Comment on attachment 706647 [details] [diff] [review]
patch

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

Looks good, but it doesn't resolve this bug. r=mkmelin
Comment 20 :aceman 2013-01-26 13:19:44 PST
Why not?
Comment 21 Magnus Melin 2013-01-27 02:31:05 PST
because "no recipient" isn't an invalid address.
(But yes, checking for invalid addresses before checking subject is also an improvement.9
Comment 22 :aceman 2013-01-27 13:44:12 PST
OK, I see we touch the same problem in bug 431217 too. I'll try to rework this a bit.
Comment 23 :aceman 2013-01-28 15:40:30 PST
Created attachment 707332 [details] [diff] [review]
WIP patch v2

standard8, what type of return value do you imagine for the recipientsAreEmpty attribute? I'd not want to downgrade it to bool as it can return 3 values for now (no sender, no recipients, OK). Or do we expect the sender to be always fine when in the compose window? So that when 'recipientsAreEmpty = sender empty || recipients empty' and it returns false we assume it is the recipients empty case and show the proper error ?

I've come up with a preliminary patch but I don't think this is OK yet.
Comment 24 Magnus Melin 2013-01-30 12:06:59 PST
Comment on attachment 707332 [details] [diff] [review]
WIP patch v2

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

::: mailnews/compose/public/nsIMsgCompFields.idl
@@ +14,5 @@
>    attribute AString replyTo;
>    attribute AString to;
>    attribute AString cc;
>    attribute AString bcc;
> +  readonly attribute bool recipientsAreEmpty;

I'd have named it hasRecipients or something

::: mailnews/compose/src/nsMsgCompFields.cpp
@@ +197,5 @@
> +  NS_ENSURE_ARG_POINTER(_retval);
> +
> +  nsresult rv = mime_sanity_check_fields(
> +    nullptr, nullptr, GetTo(), GetCc(), GetBcc(), nullptr, GetNewsgroups(),
> +    nullptr, nullptr, nullptr, nullptr, nullptr);

Shouldn't these nulls be filled, at least for the usage in nsMsgSend.cpp?
Comment 25 Mark Banner (:standard8) 2013-02-07 00:08:53 PST
(In reply to :aceman from comment #23)
> standard8, what type of return value do you imagine for the
> recipientsAreEmpty attribute? I'd not want to downgrade it to bool as it can
> return 3 values for now (no sender, no recipients, OK). Or do we expect the
> sender to be always fine when in the compose window? So that when
> 'recipientsAreEmpty = sender empty || recipients empty' and it returns false
> we assume it is the recipients empty case and show the proper error ?

I don't think it matters too much - the compose window should have valid info, otherwise iirc you've pretty much got no accounts set up and I can't remember if TB lets you into the compose window in that case.

However, supporting three values we could just do with semi enumerations in idl like is done in other places.
Comment 26 rsx11m 2013-03-05 05:08:46 PST
*** Bug 846877 has been marked as a duplicate of this bug. ***
Comment 27 :aceman 2013-03-10 07:40:48 PDT
Created attachment 723200 [details] [diff] [review]
patch v3

What about this?
Comment 28 Magnus Melin 2013-03-13 05:06:24 PDT
Comment on attachment 723200 [details] [diff] [review]
patch v3

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

Looks ok to me r=mkmelin. Some this though

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2901,5 @@
>  }
>  
> +/**
> + * Check if the entered addresses are valid.
> + *

and alert the user if they are not.

@@ +2913,5 @@
> +
> +    return false;
> +  }
> +
> +  let invalidStr = null;

don't need to initialize this.

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +165,5 @@
> +          const char *bcc,
> +          const char *newsgroups)
> +{
> +  if (to)
> +    while (IS_SPACE (*to))

no space before parenthesis ;)

@@ +181,5 @@
> +  if ((!to || !*to) && (!cc || !*cc) &&
> +      (!bcc || !*bcc) && (!newsgroups || !*newsgroups))
> +    return NS_MSG_NO_RECIPIENTS;
> +  else
> +    return NS_OK;

no else after return. just use
if (foo)
  return y;
return x;

@@ +219,4 @@
>    if (!from || !*from)
>      return NS_MSG_NO_SENDER;
>    else
> +    return mime_sanity_check_fields_recipients(to, cc, bcc, newsgroups);

no else after return
Comment 29 :aceman 2013-03-13 10:53:53 PDT
Created attachment 724497 [details] [diff] [review]
patch v4

Thanks.
Comment 30 Mark Banner (:standard8) 2013-03-22 07:22:24 PDT
Comment on attachment 724497 [details] [diff] [review]
patch v4

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2909,4 @@
>  {
> +  if (!aMsgCompFields.hasRecipients) {
> +    Services.prompt.alert(window, getComposeBundle().getString("addressInvalidTitle"),
> +                          getComposeBundle().getString("12511"));

I really think the number here shouldn't be a number. Either we should convert that l10n string to have a character based id, or at a minimum, we should be defining a constant which is NS_MSG_NO_RECIPIENTS to hold the value and aid searching.
Comment 31 :aceman 2013-03-22 10:55:41 PDT
Created attachment 728312 [details] [diff] [review]
patch v5

Changing composeMsgs.properties would probably be hard, the msg ID to display looks to be taken from the error code. And also Seamonkey uses these /mailnews files too, to access their composeMsgs.properties. I'd vote for a constant in our MsgComposeUtils.js.
Comment 32 Magnus Melin 2013-03-23 04:34:32 PDT
Comment on attachment 728312 [details] [diff] [review]
patch v5

Somethings wrong with this patch, as it won't let you send...
Comment 33 :aceman 2013-03-24 05:47:08 PDT
Created attachment 728717 [details] [diff] [review]
patch v6

Yeah, sorry.
Comment 34 Magnus Melin 2013-03-27 12:56:01 PDT
Comment on attachment 728717 [details] [diff] [review]
patch v6

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

Bitrotted :(
Comment 35 :aceman 2013-03-27 13:24:42 PDT
Created attachment 730319 [details] [diff] [review]
patch v7
Comment 36 Magnus Melin 2013-04-01 02:51:12 PDT
Comment on attachment 730319 [details] [diff] [review]
patch v7

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

Looks good to me.

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +216,3 @@
>        followup_to++;
>  
> +  /* TODO: sanity check other_random_headers for newline conventions */

I'd prefer // comments for short notes inside metods
Comment 37 :aceman 2013-04-03 14:11:10 PDT
Created attachment 733026 [details] [diff] [review]
patch v8

OK, so let's try to finish this.
Comment 38 Mark Banner (:standard8) 2013-04-15 13:11:32 PDT
Comment on attachment 733026 [details] [diff] [review]
patch v8

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

As Magnus has already reviewed this, I'll make it an sr+ as it has an interface change ;-)
Comment 39 :aceman 2013-04-15 13:19:28 PDT
Cool, thanks.
Comment 40 Ryan VanderMeulen [:RyanVM] 2013-04-15 19:06:52 PDT
https://hg.mozilla.org/comm-central/rev/463b60506120
Comment 41 Thomas D. (currently busy elsewhere; needinfo?me) 2013-06-17 06:10:58 PDT
*** Bug 528819 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.