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

RESOLVED FIXED in Thunderbird 23.0

Status

Thunderbird
Message Compose Window
--
enhancement
RESOLVED FIXED
17 years ago
4 years ago

People

(Reporter: Henrik Gemal, Assigned: aceman)

Tracking

(Blocks: 1 bug, {polish})

Trunk
Thunderbird 23.0
polish
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

17 years ago
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.

Updated

17 years ago
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"
(Reporter)

Comment 2

17 years ago
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

Comment 3

16 years ago
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
(Reporter)

Comment 4

16 years ago
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)

Comment 10

8 years ago
Created attachment 417396 [details] [diff] [review]
I added a check for the to field before checking the subject field

Updated

8 years ago
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

Updated

8 years ago
Attachment #417396 - Flags: review?(bugzilla)

Updated

8 years ago
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)
(Assignee)

Comment 16

5 years ago
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)
(Assignee)

Comment 18

5 years ago
Created attachment 706647 [details] [diff] [review]
patch
Attachment #417396 - Attachment is obsolete: true
Attachment #706647 - Flags: review?(mkmelin+mozilla)

Comment 19

5 years ago
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+
(Assignee)

Comment 20

5 years ago
Why not?

Comment 21

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

Comment 22

5 years ago
OK, I see we touch the same problem in bug 431217 too. I'll try to rework this a bit.
(Assignee)

Comment 23

5 years ago
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.
Attachment #707332 - Flags: feedback?(mbanner)
(Assignee)

Updated

5 years ago
Attachment #707332 - Flags: feedback?(mkmelin+mozilla)

Comment 24

5 years ago
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+

Updated

4 years ago
Duplicate of this bug: 846877
(Assignee)

Comment 27

4 years ago
Created attachment 723200 [details] [diff] [review]
patch v3

What about this?
Attachment #706647 - Attachment is obsolete: true
Attachment #707332 - Attachment is obsolete: true
Attachment #723200 - Flags: review?(mbanner)
(Assignee)

Updated

4 years ago
Attachment #723200 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

4 years ago
Blocks: 431217
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED

Comment 28

4 years ago
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+
(Assignee)

Comment 29

4 years ago
Created attachment 724497 [details] [diff] [review]
patch v4

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

Comment 31

4 years ago
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.
Attachment #724497 - Attachment is obsolete: true
Attachment #728312 - Flags: review?(mbanner)

Comment 32

4 years ago
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-
(Assignee)

Comment 33

4 years ago
Created attachment 728717 [details] [diff] [review]
patch v6

Yeah, sorry.
Attachment #728312 - Attachment is obsolete: true
Attachment #728717 - Flags: review?(mkmelin+mozilla)

Comment 34

4 years ago
Comment on attachment 728717 [details] [diff] [review]
patch v6

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

Bitrotted :(
Attachment #728717 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 35

4 years ago
Created attachment 730319 [details] [diff] [review]
patch v7
Attachment #728717 - Attachment is obsolete: true
Attachment #730319 - Flags: review?(mkmelin+mozilla)

Comment 36

4 years ago
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+
(Assignee)

Comment 37

4 years ago
Created attachment 733026 [details] [diff] [review]
patch v8

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+
(Assignee)

Comment 39

4 years ago
Cool, thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/463b60506120
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
(Assignee)

Updated

4 years ago
Depends on: 872041

Updated

4 years ago
Blocks: 360488
Duplicate of this bug: 528819
You need to log in before you can comment on or make changes to this bug.