Closed Bug 528600 Opened 10 years ago Closed 10 years ago

Typo in Error Message: The message was sent successfully, but could not be copied to your Sent folder.Would you like to return to the compose window?

Categories

(Thunderbird :: Message Compose Window, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.1b1

People

(Reporter: mlissner+bugzilla, Assigned: mlissner+bugzilla)

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6pre) Gecko/20091112 Ubuntu/9.10 (karmic) Firefox/3.5.2
Build Identifier: 3.0pre

If TB has an issue copying a message to the Sent folder, you may get this error. It should have a space between the words "folder." and "Would."

Reproducible: Always

Steps to Reproduce:
1. Not sure...
Actual Results:  
Doesn't have a space.

Expected Results:  
Should.
These are actually two separate messages (#12532 and #12561), thus there is apparently an issue with concatenating those. Confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug]
I think I found where this needs to be fixed, but my C++ skills are highly lacking. I believe it's these lines from nsMsgSendReport.cpp:

366     if (askToGoBackToCompose)
367     {
368       PRBool oopsGiveMeBackTheComposeWindow = PR_TRUE;
369       nsString text1;
370       bundle->GetStringFromID(NS_MSG_ASK_TO_COMEBACK_TO_COMPOSE, getter_Copies(text1));
371       if (dialogMessage.IsEmpty())
372         dialogMessage.AppendLiteral("\n");
373       dialogMessage.Append(text1);
374       nsMsgAskBooleanQuestionByString(prompt, dialogMessage.get(), &oopsGiveMeBackTheComposeWindow, dialogTitle.get());
375       if (!oopsGiveMeBackTheComposeWindow)
376         *_retval = NS_OK;
377     }

I think on line 373, it needs to be changed to something like: dialogMessage.Append(" ", text1);

But I haven't done C++ since about 2001. If somebody wants to check this and point me in the right direction, I'd be happy to make the patch, and post it.
This is probably a no no, but I'm posting this link here, so I'll have it later. Seems like I'm gonna need it.

https://developer.mozilla.org/en/Creating_a_patch
Assuming that's the right location (and yes, the ID matches what I've looked up before as the second part of the message), this condition looks suspicious:

> 371       if (dialogMessage.IsEmpty())
> 372         dialogMessage.AppendLiteral("\n");

So, if dialogMessage contains the first part of the message already, then text1 is supposed to be added (line 373). However, the intention apparently was to add a newline to separate the two messages (line 372), but it's only added if the current dialogMessage is empty (line 371). That's of course the opposite.

It appears to me that all you'd need to do is adding a negator (!) if front of the dialogMessage.IsEmpty() condition, then the newline is added if a message already exists that needs to be separated. This would make sense.

Also see https://developer.mozilla.org/en/Simple_Thunderbird_build on how to get started with a build environment on Linux, then you can create the patch after making the change and testing it.
Ack! You're totally right, I was confused by the newline, and thought it must have something to do with some buggy behavior of the dialog window that I wasn't familiar with.

Here's my first patch, adding an ! to line 371.
Attached patch diff adding an ! (obsolete) — Splinter Review
Attachment #417154 - Flags: approval-thunderbird3.0.1?
Thanks, the patch applies and should solve the problem. Here is the tricky
part, you'll need reviews from the module owners/peers, which in this case are
listed in http://www.mozilla.org/about/owners.html#mail-and-news-backend and
request those with the "review"->? and "superreview"->? flags.
Attachment #417154 - Flags: review?(neil)
Attachment #417154 - Flags: review?(mnyromyr)
Attachment #417154 - Flags: review?(kaie)
Attachment #417154 - Flags: review?(dmose)
Attachment #417154 - Flags: review?(bugzilla)
Attachment #417154 - Flags: review?(bienvenu)
I think superreview is for things that change APIs or go across modules, so I just put all the reviewers and peers names in the review field. Hopefully that's the right approach. It seems spammy, but so do many other bugzilla things.

Thanks for the help.
Attachment #417154 - Flags: superreview?(bugzilla)
Attachment #417154 - Flags: review?(neil)
Attachment #417154 - Flags: review?(mnyromyr)
Attachment #417154 - Flags: review?(kaie)
Attachment #417154 - Flags: review?(dmose)
Attachment #417154 - Flags: review?(bienvenu)
Attachment #417154 - Flags: approval-thunderbird3.0.1?
Comment on attachment 417154 [details] [diff] [review]
diff adding an !

You don't need to request review from that many people - just one for now, and review rules do currently require superreview for mailnews/:

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

Also, as per https://wiki.mozilla.org/Thunderbird/Thunderbird3_Security_And_Stability_Releases#General_Process_for_landing_bugs_on_Thunderbird_3.0.x please don't request approval for a branch until the patch has been reviewed and landed.
(and thanks for the patch, I'll take a look at it soon).
Oops, sorry for not being more specific about the review rules...
No worries, rsx11m. There's a steep learning curve to these things, so I figure I'll fail early, fail often whether I want to or not. 

Thanks for the thorough replies Mark. Sorry again to spam - didn't know if it was a case of multiple people covering each other's backs or not. 

I guess the lesson is that it's not as simple as writing and submitting a patch. One must also learn the process.

Whoever flagged this as a good first bug was right indeed.
Comment on attachment 417154 [details] [diff] [review]
diff adding an !

>+      if (! dialogMessage.IsEmpty())
>         dialogMessage.AppendLiteral("\n");
>       dialogMessage.Append(text1);
>       nsMsgAskBooleanQuestionByString(prompt, dialogMessage.get(), &oopsGiveMeBackTheComposeWindow, dialogTitle.get());
>       if (!oopsGiveMeBackTheComposeWindow)
Note that we don't generally add a space after a ! operator.
I thought that was weird too, but I was following the convention of the ! operator elsewhere in the file. Is it worth making a different patch to get them all out of there?
The only other location I can find in nsMsgSendReport.cpp with a space after
the ! is in line 360 for a similar construct. It's a trivial thing, but if you want to catch both of them you can post a new patch. Use your current setup and make those changes, then you get a single patch with the next hg diff command.

When uploading it, you can obsolete your old patch in a checkbox, and set the review and superreview flags for your new patch to Mark's (standard8) address. This will cancel the old requests and replace them with your new ones.
Attachment #417154 - Attachment is obsolete: true
Attachment #417154 - Flags: superreview?(bugzilla)
Attachment #417154 - Flags: review?(bugzilla)
Attachment #417229 - Flags: superreview?(bugzilla)
Attachment #417229 - Flags: superreview+
Attachment #417229 - Flags: review?(bugzilla)
Attachment #417229 - Flags: review+
Comment on attachment 417229 [details] [diff] [review]
Patch fixing the error plus correcting the spacing as mentioned above.

Sorry for the big delay in getting to this.

r/sr=Standard8.

Please add checkin-needed to the keywords if you need this checking in and you haven't got access.
No worries Mark. Thanks for the review and super review. I'll add the keywords so this can get checked in.
Keywords: checkin-needed
pushed as: http://hg.mozilla.org/comm-central/rev/6f60894753e5

Thanks!
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1b1
Assignee: nobody → mlissner
You need to log in before you can comment on or make changes to this bug.