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

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[[e-mail]]

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.

Comment 1

[rsx11m]

These are actually two separate messages (#12532 and #12561), thus there is apparently an issue with concatenating those. Confirming.

Comment 2

[[e-mail]]

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.

Comment 3

[[e-mail]]

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

Comment 4

[rsx11m]

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.

Comment 5

[[e-mail]]

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.

Comment 6

[[e-mail]]

Attachment: diff adding an !

Comment 7

[rsx11m]

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.

Comment 8

[[e-mail]]

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.

Comment 9

[Mark Banner (:standard8)]

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.

Comment 10

[Mark Banner (:standard8)]

(and thanks for the patch, I'll take a look at it soon).

Comment 11

[rsx11m]

Oops, sorry for not being more specific about the review rules...

Comment 12

[[e-mail]]

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 13

[[e-mail]]

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.

Comment 14

[[e-mail]]

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?

Comment 15

[rsx11m]

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.

Comment 16

[[e-mail]]

Attachment: Patch fixing the error plus correcting the spacing as mentioned above.

Comment 17

[Mark Banner (:standard8)]

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.

Comment 18

[[e-mail]]

No worries Mark. Thanks for the review and super review. I'll add the keywords so this can get checked in.

Comment 19

[Justin Wood (:Callek)]

pushed as: http://hg.mozilla.org/comm-central/rev/6f60894753e5

Thanks!