Last Comment Bug 482080 - new message created from compose window should use the same initial identity as its parent compose window
: new message created from compose window should use the same initial identity ...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- minor with 1 vote (vote)
: Thunderbird 14.0
Assigned To: Magnus Melin
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-07 14:39 PST by Wayne Mery (:wsmwk, NI for questions)
Modified: 2012-04-03 12:18 PDT (History)
8 users (show)
mkmelin+mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (15.02 KB, patch)
2012-03-16 04:14 PDT, Magnus Melin
mconley: review-
Details | Diff | Splinter Review
proposed fix v2 (15.48 KB, patch)
2012-03-22 06:25 PDT, Magnus Melin
mconley: review-
Details | Diff | Splinter Review
proposed fix v3 (15.49 KB, patch)
2012-03-24 11:58 PDT, Magnus Melin
mconley: review+
bwinton: ui‑review+
Details | Diff | Splinter Review

Description Wayne Mery (:wsmwk, NI for questions) 2009-03-07 14:39:12 PST
new message created from compose is forced to default account (B) when another account (A) is selected

1. click on non-default account A
2. create a new message 1 - it uses identity associated with account A
3. create new message 2 from compose window

expected results:
message 2 is addressed from account A

actual results: 
message 2 is addressed from default account B
Comment 1 WADA 2009-03-08 00:48:41 PST
Behaviour was observed with Tb trunk 2009/3/02 build on MS Win-XP SP3.
Which should be set as "pre-selected From:" do you think?
 (a) Main identity of default account (current behaviour)
 (b) Main identity of selected account at folder pane (you say your expectation)
 (c) Identity currently selected in compose window
If (b), which one is the "selected account" when multiple mail windows are opened and different accounts are selected?
Comment 2 Wayne Mery (:wsmwk, NI for questions) 2009-03-09 05:00:21 PDT
I would argue using context, so (c) in the case of compose.

It muddies and perhaps hurts my argument for changing compose behavior as suggested by comment 0, but I suppose we should consider the larger question - i.e. should behavior deviate (or not) for other cases. For example:
* when default account B is not even logged in
* when account A is selected and some other window is open, like Address book
Comment 3 Bryan Clark (DevTools PM) [@clarkbw] 2009-04-23 14:56:45 PDT
I think the choice order is supposed to be:

default account OR
  account of folder pane selection OR
     account of message focus selection

The last one is because we have cross account folders now.  I think the real expectation from users is to give the account of whatever you are looking at / focus is on.
Comment 4 Wayne Mery (:wsmwk, NI for questions) 2011-10-04 14:57:40 PDT
(In reply to Bryan Clark [:clarkbw] from comment #3)
> I think the choice order is supposed to be:
> 
> default account OR
>   account of folder pane selection OR
>      account of message focus selection
> 
> The last one is because we have cross account folders now.  I think the real
> expectation from users is to give the account of whatever you are looking at
> / focus is on.

In that case, current behavior is incorrect
Comment 5 Philip Prindeville 2012-03-10 11:49:17 PST
This should be fairly straightforward to implement, right?
Comment 6 Magnus Melin 2012-03-11 06:24:31 PDT
Since this is about creation of a new message from the compose window, shouldn't the default actually be to use the same identity that is currently selected in that window?
Comment 7 Philip Prindeville 2012-03-11 14:43:21 PDT
I'd say this isn't strictly limited to doing a "New Message" from the compose window, but in general.

That is, from the Mailbox view window a "New Message" should use the identity from the currently selected tab.
Comment 8 David :Bienvenu 2012-03-11 17:35:17 PDT
yeah, I agree that we should do this. I'm surprised that we don't, actually.
Comment 9 Magnus Melin 2012-03-16 04:14:50 PDT
Created attachment 606522 [details] [diff] [review]
proposed fix

Only the goOpenNewMessage changes are really changes here besides the onliner xxx fix to escape & in html. I got rid of two unnecessary if-blocks - we already used the foo variables of if(foo) later regardless, so obviously there were no need for the if blocks.
Comment 10 Andrew Sutherland [:asuth] 2012-03-16 14:38:31 PDT
Comment on attachment 606522 [details] [diff] [review]
proposed fix

mconley has indicated a willingness to review in the compose sub-module, so I am deferring review to him to keep this all legit.  I'll poke binevenu/standard8 to try and codify an explicit module owner/peers.
Comment 11 Mike Conley (:mconley) - (Needinfo me!) 2012-03-19 07:54:53 PDT
Comment on attachment 606522 [details] [diff] [review]
proposed fix

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

This looks good Magnus!

Just a few questions / comments / suggestions.

All the best,

-Mike

::: mail/components/compose/content/MsgComposeCommands.js
@@ +38,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +/**
> + * Commands for the message composition win

A nit - I don't see much advantage to shortening "window" to "win".  Might as well use the whole word here, since I was initially confused about what we were winning wrt composition.  :)

@@ +49,5 @@
>  
>  Components.utils.import("resource:///modules/MailUtils.js");
>  Components.utils.import("resource:///modules/errUtils.js");
>  Components.utils.import("resource:///modules/iteratorUtils.jsm");
>  Components.utils.import("resource://gre/modules/InlineSpellChecker.jsm");

This might be a lovely opportunity to import mailServices.js, since we can use it in goOpenNewMessage...

@@ +783,1 @@
>     msgComposeService.OpenComposeWindow(null, null, null,

If we import mailServices.js, we can just use MailServices.compose.OpenComposeWindow(... here.

@@ +832,2 @@
>  
>    gLastWindowToHaveFocus = focusedWindow;

Thanks for cleaning this stuff up. :)

@@ +2207,5 @@
>  
>    // Get the <editor> element to startup an editor
>    var editorElement = GetCurrentEditorElement();
>    gMsgCompose = composeSvc.initCompose(params, window, editorElement.docShell);
> +

You've removed the check to ensure gMsgCompose exists, and we didn't fail out of the initCompose function.  Was this on purpose?  If so, why?

@@ +2227,2 @@
>    {
> +    var editortype = gMsgCompose.composeHTML ? "htmlmail" : "textmail";

While you're here, if you wouldn't mind replacing this "var" with "let", that'd be awesome. :)

@@ -2259,5 @@
> -      InitLanguageMenu();
> -    }
> -
> -    var msgCompFields = gMsgCompose.compFields;
> -    if (msgCompFields)

A check for gMsgCompose.compFields was also removed.  On purpose?

@@ +2260,5 @@
> +      } catch(e) { cleanBody = body;}
> +
> +      body = body.replace("&", "&amp;", "g");
> +      gMsgCompose.compFields.body =
> +        "<br /><a href=\"" + body + "\">" + cleanBody + "</a><br />";

I'm no security expert, but are we not opening up ourselves here to a potential security issue for the sender if a specially crafted, malicious link gets accidentally inserted into the body?  Would it / could it execute chrome-level JS?

@@ +2273,5 @@
> +
> +  AddAttachments(gMsgCompose.compFields.attachments);
> +
> +  var event = document.createEvent("Events");
> +  event.initEvent("compose-window-init", false, true);

We've moved the compose-window-init Event to fire *before* the recycled check, instead of after now.  If at all possible, we should probably maintain the event order for our add-on developers.  If it's necessary to change the order of events here, we might need to update our developer documentation for the compose-window-init event (assuming we have some...)

@@ +2289,5 @@
> +      onFontColorChange();
> +      onBackgroundColorChange();
> +    }
> +
> +    // Reset the priorty field for recycled windows.

typo: priority

::: mail/test/mozmill/composition/test-newmsg-compose-identity.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Tests that compose new message chooses the correct initial identity.

We should mention that we're testing this from the context of an open composer.

@@ +11,5 @@
> +const RELATIVE_ROOT = "../shared-modules";
> +const MODULE_REQUIRES = ["folder-display-helpers",
> +                         "window-helpers", "compose-helpers"];
> +
> +var folderHelper = null;

We can cut these variables - see my comment in setupModule.

@@ +16,5 @@
> +var windowindowHelperelper = null;
> +var composeHelper = null;
> +
> +var testFolder1 = null;
> +var testFolder2 = null;

Neither of these vars seem to be used, and can be axed.

@@ +21,5 @@
> +
> +const identity1Email = "x@example.com";
> +const identity2Email = "y@example.com";
> +
> +var setupModule = function (module) {

I think we can just have this be:

function setupModule(module) {
  // ...
}

...unless there's some hidden advantage to setting it to a var like this that I'm not seeing.

@@ +22,5 @@
> +const identity1Email = "x@example.com";
> +const identity2Email = "y@example.com";
> +
> +var setupModule = function (module) {
> +  folderHelper = collector.getModule("folder-display-helpers");

We can do away with folderHelper, windowHelper and composeHelper using:

collector.getModule("folder-display-helpers").installInto(module);

where "folder-display-helpers" can also be "window-helpers" and "compose-helpers".

@@ +32,5 @@
> +
> +  setupAccounts();
> +}
> +
> +var setupAccounts = function() {

Again, not seeing much advantage to setting this function to a var like this... I think I'd prefer

function setupAccounts() {
  // ...
}

@@ +33,5 @@
> +  setupAccounts();
> +}
> +
> +var setupAccounts = function() {
> +  let acctMgr = Cc["@mozilla.org/messenger/account-manager;1"]

mailServices.js is the new hotness - if you import that in the header of the file, you can access the account manager via

MailServices.accounts

@@ +53,5 @@
> +}
> +
> +var checkCompIdentity = function(composeWin, expectedFromEmail) {
> +  let identityList = composeWin.e("msgIdentity");
> +  if (identityList.selectedItem.label.indexOf(expectedFromEmail) == -1)

Can we not use assert_equals, or at the very least, assert_true here?

@@ +61,5 @@
> +}
> +
> +function test_account_comp() {
> +  function test_account(aAccount) {
> +    be_in_folder(aAccount.incomingServer.rootFolder

Nit - I think I'd prefer:

be_in_folder(aAccount.incomingServer
                     .rootFolder
                     .getFolderWithFlags(Ci.nsIMsgFolderFlags.Inbox));

(If Bugzilla / Splinter has botched the formatting, I've moved rootFolder to the next line and lined up the periods.)

@@ +64,5 @@
> +  function test_account(aAccount) {
> +    be_in_folder(aAccount.incomingServer.rootFolder
> +                         .getFolderWithFlags(Ci.nsMsgFolderFlags.Inbox));
> +
> +    let mainCompWin = composeHelper.open_compose_new_mail();

you can just use open_compose_new_mail without composeHelper, since we installed it into this module.  Applies elsewhere in this function.

@@ +68,5 @@
> +    let mainCompWin = composeHelper.open_compose_new_mail();
> +    checkCompIdentity(mainCompWin, aAccount.defaultIdentity.email);
> +
> +    // Compose a new message from the compose window.
> +    windowHelper.plan_for_new_window("msgcompose");

Just use plan_for_new_window, instead of windowHelper.plan_for_new_window

@@ +70,5 @@
> +
> +    // Compose a new message from the compose window.
> +    windowHelper.plan_for_new_window("msgcompose");
> +    mainCompWin.keypress(null, "n", {shiftKey: false, accelKey: true});
> +    let newCompWin = composeHelper.wait_for_compose_window();

Just use wait_for_compose_window().
Comment 12 Magnus Melin 2012-03-22 06:23:21 PDT
(In reply to Mike Conley (:mconley) from comment #11)
> Comment on attachment 606522 [details] [diff] [review]

Thx for the review!

> You've removed the check to ensure gMsgCompose exists, and we didn't fail
> out of the initCompose function.  Was this on purpose?  If so, why?

Yes, it's not needed. 
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2233 then not created inside the if block, and directly after the if-block we'd still blow up since we access it at http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2337

> > -    var msgCompFields = gMsgCompose.compFields;
> > -    if (msgCompFields)
> 
> A check for gMsgCompose.compFields was also removed.  On purpose?

Yes,  gMsgCompose.compFields http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2337, then accessed (also) in http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2337 where we'd blow up.

I think in both cases there is some possibility it's null, likely OOM situations and maybe if you happen to hit the code during a suitable time during shutdown - but those would be situations where it doesn't really matter as things won't work anyway.

> > +      body = body.replace("&", "&amp;", "g");
> > +      gMsgCompose.compFields.body =
> > +        "<br /><a href=\"" + body + "\">" + cleanBody + "</a><br />";
> 
> I'm no security expert, but are we not opening up ourselves here to a
> potential security issue for the sender if a specially crafted, malicious
> link gets accidentally inserted into the body?  Would it / could it execute
> chrome-level JS?
 
No, it's just inserting text in an editor, the potential code is never run. When that page is viewed by someone it has whatever privileges its context gives it (what the recipients client allows it to do).

> We've moved the compose-window-init Event to fire *before* the recycled
> check, instead of after now.  If at all possible, we should probably
> maintain the event order for our add-on developers.  If it's necessary to
> change the order of events here, we might need to update our developer
> documentation for the compose-window-init event (assuming we have some...)

I didn't change the order here, what do you mean?

> > +var setupAccounts = function() {
> 
> Again, not seeing much advantage to setting this function to a var like
> this... I think I'd prefer

I don't know, though it seems to be fairly common. I always thought it had something to do with getting mozmill to not count executing that function as a passed test, but it doesn't seem to have much effect. Anyway, changed it.
Comment 13 Magnus Melin 2012-03-22 06:25:02 PDT
Created attachment 608315 [details] [diff] [review]
proposed fix v2
Comment 14 Jb Piacentino 2012-03-22 06:57:16 PDT
Does this patch address the Unified view case ? I can see the situation where, in this view, the 'Inbox' folder is selected with no particular account. TB should then use the default account.
Comment 15 David :Bienvenu 2012-03-22 07:39:46 PDT
(In reply to Jb Piacentino from comment #14)
> Does this patch address the Unified view case ? I can see the situation
> where, in this view, the 'Inbox' folder is selected with no particular
> account. TB should then use the default account.

that depends on whether there is a message selected at the time - if there is, it should use the identity best associated with that message.
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-03-22 07:52:14 PDT
(In reply to David :Bienvenu from comment #15)
> (In reply to Jb Piacentino from comment #14)
> > Does this patch address the Unified view case ? I can see the situation
> > where, in this view, the 'Inbox' folder is selected with no particular
> > account. TB should then use the default account.
> 
> that depends on whether there is a message selected at the time - if there
> is, it should use the identity best associated with that message.

Hm, that's definitely not what this patch is doing.

This patch only seems to consider the selection in the folderPane.

Magnus - is it possible to take into consideration the selected message?
Comment 17 Jb Piacentino 2012-03-22 08:14:49 PDT
(In reply to David :Bienvenu from comment #15)
> that depends on whether there is a message selected at the time - if there
> is, it should use the identity best associated with that message.
Not sure I agree here. I would have found more natural to ha
Comment 18 Jb Piacentino 2012-03-22 08:16:12 PDT
(...fingers slipped to tab/enter... arggh)
Not sure I agree here. I would have found more natural to have the default account selected if you _create_ a new message. Replying should obviously use the recipient's account.
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-03-22 08:29:10 PDT
CC'ing bwinton.
Comment 20 David :Bienvenu 2012-03-22 10:24:18 PDT
(In reply to Jb Piacentino from comment #17)
> (In reply to David :Bienvenu from comment #15)
> > that depends on whether there is a message selected at the time - if there
> > is, it should use the identity best associated with that message.
> Not sure I agree here. I would have found more natural to ha

You probably have a much stronger default account. For me, if I'm reading a work message, and click new message (or forward), I probably want it to come from my work account. I don't get why the folder that's selected is more meaningful than the message I'm reading, which is the case you're making.
Comment 21 Magnus Melin 2012-03-22 12:51:18 PDT
The 3pane doesn't currently look at the selected msg when choosing identity, only the folder. I agree it might be better to do that there though, to better accommodate unified folders and the global inbox.

For this bug it's more fuzzy and i bet creating a new message from the compose window is a lot less used. What if you went to read something in between while composing? Maybe we should do comment 6 instead? (clone the currently selected identity of the composer window opening a new msg)
Comment 22 David :Bienvenu 2012-03-22 12:53:55 PDT
oh, new message from an already open compose window? duh, how embarrassing, I didn't notice that part of this. I think we should clone the currently selected identity, like you say.
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-03-23 12:40:10 PDT
Comment on attachment 608315 [details] [diff] [review]
proposed fix v2

I like the plan from comment #6 - using the currently selected identity in the compose window that spawns the new composer.

If we're going with that, r-, since this patch doesn't do that.  :)
Comment 24 Magnus Melin 2012-03-24 11:58:40 PDT
Created attachment 609024 [details] [diff] [review]
proposed fix v3
Comment 25 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 11:14:29 PDT
Comment on attachment 609024 [details] [diff] [review]
proposed fix v3

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

Just two small nits.  Other than that, this looks great, and works as advertised.  +1 for a nice test case.  :)

Thanks for your work, Magnus!

-Mike

::: mail/test/mozmill/composition/test-newmsg-compose-identity.js
@@ +55,5 @@
> + * expected initial identity.
> + */
> +function test_compose_from_composer() {
> +  be_in_folder(account.incomingServer
> +                       .rootFolder

Lines 59 and 60 are misaligned by a single space.

@@ +71,5 @@
> +
> +  // Switch to identity2 in the main compose window, new compose windows
> +  // starting from here should use the same identiy as it's "parent".
> +  let identityList = mainCompWin.e("msgIdentity");
> +  identityList.selectedIndex++;

I think this is a little too generic.  For testing, I'd prefer a "harder" value, like:

identityList.selectedIndex = 1; // The identity at index 1 is the second identity
Comment 26 Magnus Melin 2012-03-26 11:44:59 PDT
(In reply to Mike Conley (:mconley) from comment #25)
> > +  // Switch to identity2 in the main compose window, new compose windows
> > +  // starting from here should use the same identiy as it's "parent".
> > +  let identityList = mainCompWin.e("msgIdentity");
> > +  identityList.selectedIndex++;
> 
> I think this is a little too generic.  For testing, I'd prefer a "harder"
> value, like:
> 
> identityList.selectedIndex = 1; // The identity at index 1 is the second
> identity

IIRC, it may not be 1 since we have one other account set up too. It's likely identity 2, but I think it's safer to just ++
Comment 27 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 11:46:00 PDT
(In reply to Magnus Melin from comment #26)
> IIRC, it may not be 1 since we have one other account set up too. It's
> likely identity 2, but I think it's safer to just ++

Suit yourself. It's not a big deal.
Comment 28 Magnus Melin 2012-04-02 10:52:44 PDT
bwinton: ui-r ping
Comment 29 Blake Winton (:bwinton) (:☕️) 2012-04-02 11:13:02 PDT
Comment on attachment 609024 [details] [diff] [review]
proposed fix v3

Yeah, that makes sense to me.  I agree that we might want to take a look at the currently selected message when determining the identity to use in unified (or search) folders, but that can totally be a separate bug.  :)

ui-r=me!

Later,
Blake.
Comment 30 Magnus Melin 2012-04-03 12:18:22 PDT
http://hg.mozilla.org/comm-central/rev/bc7c9ecd1292 ->FIXED

Note You need to log in before you can comment on or make changes to this bug.