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?

RESOLVED FIXED in Thunderbird 3.1b1

Status

Thunderbird
Message Compose Window
--
trivial
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mlissner@michaeljaylissner.com, Assigned: mlissner@michaeljaylissner.com)

Tracking

unspecified
Thunderbird 3.1b1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

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

Comment 2

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

Comment 3

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

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

Comment 5

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

Comment 6

9 years ago
Created attachment 417154 [details] [diff] [review]
diff adding an !
Attachment #417154 - Flags: approval-thunderbird3.0.1?

Comment 7

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

Updated

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

Comment 8

9 years ago
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).

Comment 11

9 years ago
Oops, sorry for not being more specific about the review rules...
(Assignee)

Comment 12

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

Comment 14

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

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

Updated

9 years ago
Attachment #417154 - Attachment is obsolete: true
Attachment #417154 - Flags: superreview?(bugzilla)
Attachment #417154 - Flags: review?(bugzilla)
(Assignee)

Comment 16

9 years ago
Created attachment 417229 [details] [diff] [review]
Patch fixing the error plus correcting the spacing as mentioned above.
Attachment #417229 - Flags: superreview?(bugzilla)
Attachment #417229 - 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.
(Assignee)

Comment 18

9 years ago
No worries Mark. Thanks for the review and super review. I'll add the keywords so this can get checked in.
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
pushed as: http://hg.mozilla.org/comm-central/rev/6f60894753e5

Thanks!
Status: NEW → RESOLVED
Last Resolved: 9 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.