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.

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"

it's when sending...

Henrik, why should it be the other way around? Is there any good reason of why to put effort into fixing this bug?

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.

reassign to varada. Before doing this kind of modification, we need to check with the UI team.

taking all of varada's bugs.

sorry for the spam. making bugzilla reflect reality as I'm not working on these bugs. filter on FOOBARCHEESE to remove these in bulk.

Filter on "Nobody_NScomTLD_20080620"

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

has draft patch with review comments what to change (comment 14) - aceman, would you be able to finish this off?

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.

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

Why not?

because "no recipient" isn't an invalid address. (But yes, checking for invalid addresses before checking subject is also an improvement.9

OK, I see we touch the same problem in bug 431217 too. I'll try to rework this a bit.

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

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

What about this?

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

Thanks.

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.

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 on attachment 728312 [details] [diff] [review] patch v5 Somethings wrong with this patch, as it won't let you send...

Yeah, sorry.

Comment on attachment 728717 [details] [diff] [review] patch v6 Review of attachment 728717 [details] [diff] [review]: ----------------------------------------------------------------- Bitrotted :(

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

OK, so let's try to finish this.

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

Cool, thanks.