Closed Bug 68784 Opened 23 years ago Closed 11 years ago

when sending mail, should first check "no recipient", then "no subject" (it's currently the other way round)

Categories

(Thunderbird :: Message Compose Window, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 23.0

People

(Reporter: bugzilla, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: polish)

Attachments

(1 file, 8 obsolete files)

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.
QA Contact: esther → laurel
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...
Summary: check for no subject then no recipient should be the other way around → check for no subject then no recipient when sending mail should be the other way around
Henrik, why should it be the other way around? Is there any good reason of why
to put effort into fixing this bug?
Keywords: polish
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.
Hardware: PC → All
Summary: check for no subject then no recipient when sending mail should be the other way around → "no subject" check then "no recipient" check when sending mail should be the other way around
reassign to varada. Before doing this kind of modification, we need to check with the UI team.
Assignee: ducarroz → varada
taking all of varada's bugs.
Assignee: varada → sspitzer
Product: MailNews → Core
sorry for the spam.  making bugzilla reflect reality as I'm not working on these bugs.  filter on FOOBARCHEESE to remove these in bulk.
Assignee: sspitzer → nobody
Filter on "Nobody_NScomTLD_20080620"
QA Contact: laurel → composition
Product: Core → MailNews Core
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.
Component: Composition → Message Compose Window
Product: MailNews Core → Thunderbird
QA Contact: composition → message-compose
Summary: "no subject" check then "no recipient" check when sending mail should be the other way around → when sending mail, should first check "no recipient", then "no subject" (it's currently the other way round)
Attachment #417396 - Flags: review?(felsayedmeawad)
Attachment #417396 - Flags: review?(felsayedmeawad)
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.
Assignee: nobody → muhammad88
Attachment #417396 - Flags: review?(bugzilla)
Status: NEW → ASSIGNED
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.
Attachment #417396 - Flags: ui-review?(clarkbw)
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)
Attachment #417396 - Flags: ui-review?(clarkbw) → ui-review+
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.
Attachment #417396 - Flags: review?(bugzilla) → review-
has draft patch with review comments what to change (comment 14) - aceman, would you be able to finish this off?
Assignee: muhammad88 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(acelists)
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.
Assignee: nobody → acelists
Flags: needinfo?(acelists) → needinfo?(mbanner)
(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 :-)
Flags: needinfo?(mbanner)
Attached patch patch (obsolete) — Splinter Review
Attachment #417396 - Attachment is obsolete: true
Attachment #706647 - Flags: review?(mkmelin+mozilla)
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
Attachment #706647 - Flags: review?(mkmelin+mozilla) → review+
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.
Attached patch WIP patch v2 (obsolete) — Splinter Review
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.
Attachment #707332 - Flags: feedback?(mbanner)
Attachment #707332 - Flags: feedback?(mkmelin+mozilla)
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?
Attachment #707332 - Flags: feedback?(mkmelin+mozilla) → feedback+
(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.
Attachment #707332 - Flags: feedback?(mbanner) → feedback+
Attached patch patch v3 (obsolete) — Splinter Review
What about this?
Attachment #706647 - Attachment is obsolete: true
Attachment #707332 - Attachment is obsolete: true
Attachment #723200 - Flags: review?(mbanner)
Attachment #723200 - Flags: review?(mkmelin+mozilla)
Blocks: 431217
Status: NEW → ASSIGNED
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
Attachment #723200 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v4 (obsolete) — Splinter Review
Thanks.
Attachment #723200 - Attachment is obsolete: true
Attachment #723200 - Flags: review?(mbanner)
Attachment #724497 - Flags: review?(mbanner)
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.
Attachment #724497 - Flags: review?(mbanner) → review-
Attached patch patch v5 (obsolete) — Splinter Review
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.
Attachment #724497 - Attachment is obsolete: true
Attachment #728312 - Flags: review?(mbanner)
Comment on attachment 728312 [details] [diff] [review]
patch v5

Somethings wrong with this patch, as it won't let you send...
Attachment #728312 - Flags: review?(mbanner) → review-
Attached patch patch v6 (obsolete) — Splinter Review
Yeah, sorry.
Attachment #728312 - Attachment is obsolete: true
Attachment #728717 - Flags: review?(mkmelin+mozilla)
Comment on attachment 728717 [details] [diff] [review]
patch v6

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

Bitrotted :(
Attachment #728717 - Flags: review?(mkmelin+mozilla)
Attached patch patch v7 (obsolete) — Splinter Review
Attachment #728717 - Attachment is obsolete: true
Attachment #730319 - Flags: review?(mkmelin+mozilla)
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
Attachment #730319 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v8Splinter Review
OK, so let's try to finish this.
Attachment #730319 - Attachment is obsolete: true
Attachment #733026 - Flags: review?(mbanner)
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 ;-)
Attachment #733026 - Flags: review?(mbanner) → superreview+
Cool, thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/463b60506120
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
Depends on: 872041
Blocks: TB2SM
You need to log in before you can comment on or make changes to this bug.