Closed
Bug 68784
Opened 24 years ago
Closed 12 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)
Thunderbird
Message Compose Window
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)
(deleted),
patch
|
standard8
:
superreview+
|
Details | Diff | Splinter Review |
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"
Reporter | ||
Comment 2•24 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•24 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•24 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
Comment 5•24 years ago
|
||
reassign to varada. Before doing this kind of modification, we need to check with the UI team.
Assignee: ducarroz → varada
Updated•20 years ago
|
Product: MailNews → Core
Comment 7•17 years ago
|
||
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
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 9•15 years ago
|
||
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•15 years ago
|
||
Attachment #417396 -
Flags: review?(felsayedmeawad)
Updated•15 years ago
|
Attachment #417396 -
Flags: review?(felsayedmeawad)
Comment 11•15 years ago
|
||
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.
Updated•15 years ago
|
Assignee: nobody → muhammad88
Attachment #417396 -
Flags: review?(bugzilla)
Comment 12•15 years ago
|
||
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 13•15 years ago
|
||
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 14•15 years ago
|
||
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-
Comment 15•12 years ago
|
||
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•12 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)
Comment 17•12 years ago
|
||
(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•12 years ago
|
||
Attachment #417396 -
Attachment is obsolete: true
Attachment #706647 -
Flags: review?(mkmelin+mozilla)
Comment 19•12 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•12 years ago
|
||
Why not?
Comment 21•12 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•12 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•12 years ago
|
||
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 24•12 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+
Comment 25•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #707332 -
Flags: feedback?(mbanner) → feedback+
Assignee | ||
Comment 27•12 years ago
|
||
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)
Comment 28•12 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•12 years ago
|
||
Thanks.
Attachment #723200 -
Attachment is obsolete: true
Attachment #723200 -
Flags: review?(mbanner)
Attachment #724497 -
Flags: review?(mbanner)
Comment 30•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
||
Yeah, sorry.
Attachment #728312 -
Attachment is obsolete: true
Attachment #728717 -
Flags: review?(mkmelin+mozilla)
Comment 34•12 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•12 years ago
|
||
Attachment #728717 -
Attachment is obsolete: true
Attachment #730319 -
Flags: review?(mkmelin+mozilla)
Comment 36•12 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•12 years ago
|
||
OK, so let's try to finish this.
Attachment #730319 -
Attachment is obsolete: true
Attachment #733026 -
Flags: review?(mbanner)
Comment 38•12 years ago
|
||
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+
Comment 40•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
You need to log in
before you can comment on or make changes to this bug.
Description
•