Closed Bug 314806 Opened 19 years ago Closed 12 years ago

No function to set default identity

Categories

(MailNews Core :: Account Manager, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: matt_heinrichs, Assigned: aceman)

References

(Blocks 2 open bugs)

Details

(Keywords: addon-compat)

Attachments

(1 file, 9 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5
Build Identifier: version 1.5 Beta 2 (20051006)

When there are a list of identities, by default the first identity is considered to be the default.  
If this is not the case - there is no way to change the ordering of the identities, or to set any other identity to the be the default.

In my case, I accidentally deleted my first identity, and my second identity became the default.  I re-created my main identity, but was unable to set it to be the default.  I had to transcribe the settings from my main into the first identity, and vice versa.



Reproducible: Always

Steps to Reproduce:
1. Create 2 identities
2. Attempt to set the second identity to be the default identity


Actual Results:  
Function does not exist
Workaround:
Change order of id's in next prefs.js entry after recovery(adding identity again).
    user_pref("mail.account.accountNN.identities", "idXX,idYY,idZZ");
 => user_pref("mail.account.accountNN.identities", "idZZ,idXX,idYY");
Thunderbird seems to simply use first one as default(top most one in "Manage Identities" panel).
If this is done thru "Config Editor", shutdown & restart of Thunderbird just after above change is required.
Severity: minor → enhancement
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
I also think there should be an easier way to tell two identities for the same account apart. In my case the difference is a sig and reply to so it's not hard when I'm actually composing, but for managing the identities I shouldn't have to click edit to find out which it is. A fix: Allow the user to set an identifier and use that to replace "idXX"?
The "Manage Identities" panel could have "move up/down" buttons. Above the dialog, a message would explain what the order means.

The "Message Filters" dialog has a similar interface, so it's not out-of-place.
Assignee: mscott → nobody
I manually edited prefs.js, but an user-interface to sorting Identities should be great. I couldn't find any help in the Mozila help aboud "Setting the default Account Identity".

I think Ori's suggestion is the right way to go.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I can try this.
Assignee: nobody → acelists
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
Attached patch WIP patch (obsolete) — Splinter Review
So this is some of the UI part. However, it seems to me the nsMsgAccount::setDefaultIdentity functions does not do a permanent change, maybe just for the session. To make the change permanent, it needs to shuffle the identity ids in the .identities pref string. Should there be made a new function nsMsgAccount::setDefaultIdentityPermanent, or the existing function updated?
Attachment #638474 - Flags: feedback?(dbienvenu)
IRC (2012-07-02 22:48:41) NeilAway: aceman: ah, so this is the default for a given account? I see now, that makes sense, the existing one should probably be made permanent, and maybe some way of setting it in the ui too (perhaps a separate bug).
Comment on attachment 638474 [details] [diff] [review]
WIP patch

thx for the patch!

you'd want to disable the button if there is only one identity (maybe hide in that case), or if the selected identity is the default. 

And yeah, the default identify stuff basically is just the first identity in the list, so you'd need to swap the order in memory and in the pref, which would need to be written to disk. I don't think m_defaultIdentity is ever read from; it's only set. So that could probably use some good cleanup.

you've changed the uuid of nsIMsgAccount, but haven't changed the interface from what I can see.
Attachment #638474 - Flags: feedback?(dbienvenu) → feedback+
Yes, if I can hide all the new functionality into the existing SetDefaultIdentity (and make it actually do something durable) then the uuid change is not needed, it is just a remnant from my previous version.
Depends on: 360926
Attached patch patch v2 (obsolete) — Splinter Review
Patch to be applied on top of patch in bug 360926.

Bwinton, there is a new button and I changed one string and added a new one. (The "Multiple Identities Support" string was kinda weird.)

Bienvenu, I extracted the pref saving code into a separate function as it is called in 2 places now. I also think about using the same function for the pref saving in AddIdentity. But I wonder why it checks if the key is already in the list, but then calls AddIdentityInternal unconditionally and appends the element. Can't that be all stripped away and the pref value constructed from the m_identities array (after AddIdentityInternal call), i.e. calling the new saveIdentitiesPref?
Attachment #638474 - Attachment is obsolete: true
Attachment #644031 - Flags: ui-review?(bwinton)
Attachment #644031 - Flags: review?(mozilla)
Status: NEW → ASSIGNED
Comment on attachment 644031 [details] [diff] [review]
patch v2

Ian, I know you are not for the C++ part, maybe just look on the UI parts or forward to someone better. Thanks.
Attachment #644031 - Flags: review?(iann_bugzilla)
Comment on attachment 644031 [details] [diff] [review]
patch v2

>+++ b/mailnews/base/prefs/content/am-identities-list.js
> function onSetDefault(event)
> {
>+  let identity = getSelectedIdentity();
>+  if (identity)
>+    gAccount.defaultIdentity = identity;
>+
>+  let selectedItemIndex = gIdentityListBox.selectedIndex;
>+  refreshIdentityList();
>+  gIdentityListBox.selectedIndex = selectedItemIndex;
Do we really want to select the item that was in the previous index position or do we want to keep the same selected item?

>+++ b/mailnews/base/prefs/content/am-identities-list.xul

>+    <button id="setDefaultButton" disabled="true"
>+            oncommand="onSetDefault(event);" label="Set Default"
>+            accesskey="&identitiesListDelete.accesskey;"/>
It would be good to have the correct label/accesskey entities created for this button.
I presume "S" would be a good accesskey for it.

Should the default identity be bold like the default account is bold. (I know the first one listed is the default identity, but making bold would be even more of a clue).

r=me with those fixed/addressed, though if the next patch is substantially different I would like to review that too.
Attachment #644031 - Flags: review?(iann_bugzilla) → review+
Yes, the entities are bad:)
I do not know how to make one line of a <listbox> bold. I added a label to say the first identity is the default, so that should be a clue. Bwinton?

"S" is very near to "D" which is "delete" but it seems OK in this case as Delete will have a confirmation dialog from 360926.
Or instead of bold and the new label in the header, the top identity could have a "(Default)" string attached to its name in the list. That is something I could do.
(In reply to :aceman from comment #14)
> Or instead of bold and the new label in the header, the top identity could
> have a "(Default)" string attached to its name in the list. That is
> something I could do.
Setting an appropriate attribute e.g.:
http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/AccountManager.js#402
http://mxr.mozilla.org/comm-central/source/mail/themes/gnomestripe/mail/accountManage.css#35
Not sure if you can just say the first item in a certain list is bold though.
The account manager uses a <tree>.
I can apply a class or id on the listitem here. But am-identities.list.xul includes only messenger.css and I would not like to pollute that with this trivial thing.

Is it allowed to apply a direct style on the element? :)
(In reply to :aceman from comment #16)
> The account manager uses a <tree>.
> I can apply a class or id on the listitem here. But am-identities.list.xul
> includes only messenger.css and I would not like to pollute that with this
> trivial thing.
You could add it to accountManage.css and add that as a stylesheet to am-identities.list.xul
> 
> Is it allowed to apply a direct style on the element? :)
I think use of stylesheets is preferred.
Ok, I can use accountManage.css.
Attached patch patch v3 (obsolete) — Splinter Review
I hope I addressed everything.
Attachment #644031 - Attachment is obsolete: true
Attachment #644031 - Flags: ui-review?(bwinton)
Attachment #644031 - Flags: review?(mozilla)
Attachment #644875 - Flags: ui-review?(bwinton)
Attachment #644875 - Flags: review?(iann_bugzilla)
(In reply to :aceman from comment #19)
> Created attachment 644875 [details] [diff] [review]
> patch v3
> 
> I hope I addressed everything.

You missed - suite/themes/classic/mac/messenger/accountManage.css
I think I have never touched that one.
I always changed themes/classic and themes/modern only. There is a third theme? It seems in only contains some of the files so it must take the rest from the classic theme?
(In reply to :aceman from comment #21)
> I think I have never touched that one.
> I always changed themes/classic and themes/modern only. There is a third
> theme? It seems in only contains some of the files so it must take the rest
> from the classic theme?

OSX overrides certain files for its classic theme, so occasionally there are 3 rather than 2 files to change. Lots of people get caught out by it (including me).
What about merging the css for default account and default identity? It would convey the info that that should by styled the same. But if some theme really wanted, it could separate them. Like this:
treechildren::-moz-tree-cell-text(isDefaultServer-true),
#isDefaultIdentity {
  font-weight: bold;
}
Comment on attachment 644875 [details] [diff] [review]
patch v3

>+++ b/mail/locales/en-US/chrome/messenger/am-identities-list.dtd
>@@ -1,13 +1,16 @@
>+<!ENTITY identitiesListDefaultDesc.label "The identity on the top is used as default.">

I think this should be: "The identity on the top is used as the default."
(And you probably want to change suite/locales/en-US/chrome/mailnews/pref/am-identities-list.dtd too.)

>+++ b/mailnews/base/prefs/content/am-identities-list.js
>@@ -3,26 +3,28 @@

As a side note, this failed to apply for me, I think because of the changes here:

>-  gIdentityListBox = document.getElementById("identitiesList");
>-  gAddButton       = document.getElementById("addButton");
>-  gEditButton      = document.getElementById("editButton");
>-  gDeleteButton    = document.getElementById("deleteButton");
>+  gIdentityListBox  = document.getElementById("identitiesList");
>+  gAddButton        = document.getElementById("addButton");
>+  gEditButton       = document.getElementById("editButton");
>+  gSetDefaultButton = document.getElementById("setDefaultButton");
>+  gDeleteButton     = document.getElementById("deleteButton");

which is one of the main reasons that lining up equals signs is evil.  :P

Other than that, ui-r=me!

Thanks,
Blake.
Attachment #644875 - Flags: ui-review?(bwinton) → ui-review+
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #24)
> >+++ b/mailnews/base/prefs/content/am-identities-list.js
> >@@ -3,26 +3,28 @@
> As a side note, this failed to apply for me, I think because of the changes
> here:
> >-  gIdentityListBox = document.getElementById("identitiesList");
> >-  gAddButton       = document.getElementById("addButton");
> >-  gEditButton      = document.getElementById("editButton");
> >-  gDeleteButton    = document.getElementById("deleteButton");
> >+  gIdentityListBox  = document.getElementById("identitiesList");
> >+  gAddButton        = document.getElementById("addButton");
> >+  gEditButton       = document.getElementById("editButton");
> >+  gSetDefaultButton = document.getElementById("setDefaultButton");
> >+  gDeleteButton     = document.getElementById("deleteButton");
> 
> which is one of the main reasons that lining up equals signs is evil.  :P
Or you didn't apply bug 360926 first :)
Attached patch patch v4 (obsolete) — Splinter Review
Fixed the string in TB and SM, merged defaultIdentity with defaultAccount CSS and added the SM Mac theme.
Attachment #644875 - Attachment is obsolete: true
Attachment #644875 - Flags: review?(iann_bugzilla)
Attachment #645825 - Flags: review?(iann_bugzilla)
Comment on attachment 645825 [details] [diff] [review]
patch v4

"The identity at the top is used as the default."
sounds better than
"The identity on the top is used as the default."
but r=me either way
Needs a ui-reviews from suite side for both mac and non-mac
Attachment #645825 - Flags: ui-review?(neil)
Attachment #645825 - Flags: review?(iann_bugzilla)
Attachment #645825 - Flags: review+
Attachment #645825 - Flags: feedback?(stefanh)
I'd still like somebody to review the backend part. I am not sure I can count on bienvenu. Can Neil do it?
(In reply to :aceman from comment #28)
> I'd still like somebody to review the backend part. I am not sure I can
> count on bienvenu. Can Neil do it?

neil, standard8 or bienvenu are all suitable reviewers, see https://wiki.mozilla.org/Modules/MailNews_Core for more options.
Attachment #645825 - Flags: review?(mozilla)
Comment on attachment 645825 [details] [diff] [review]
patch v4

Just as a note, this patch has been bitrotted in one line by the nsnull->nullptr patch (Bug 776630)
Attached patch patch v5 (obsolete) — Splinter Review
Thanks, updated.
Attachment #645825 - Attachment is obsolete: true
Attachment #645825 - Flags: ui-review?(neil)
Attachment #645825 - Flags: review?(mozilla)
Attachment #645825 - Flags: feedback?(stefanh)
Attachment #649000 - Flags: ui-review?(neil)
Attachment #649000 - Flags: review?(mozilla)
Attachment #649000 - Flags: feedback?(stefanh)
Attachment #649000 - Attachment is patch: true
Comment on attachment 649000 [details] [diff] [review]
patch v5

>+<!ENTITY identitiesListDefaultDesc.label "The identity at the top is used as the default.">
I would use the wording "The first identity is used by default."

>+#isDefaultIdentity,
> treechildren::-moz-tree-cell-text(isDefaultServer-true) {
>   font-weight: bold;
> }
[Somehow I feel it looks odd to combine these rules.]
Rather than setting an id, could you not use #identitiesList > listitem:first-child instead?

>-  for (var j = 0; j < identitiesCount; j++) 
>+  for (let j = 0; j < identitiesCount; j++) 
[If you are going to do an unrelated change, at least remove the trailing whitespace!]

>   if (gIdentityListBox.selectedItems.length <= 0)
[Not sure how this can happen, it's a single-selection listbox!]

>+    if ((gIdentityListBox.selectedItems.length == 1) &&
>+        (gIdentityListBox.selectedIndex > 0))
Nit: don't need those extra ()s

>+  let identity = getSelectedIdentity();
>+  if (identity)
>+    gAccount.defaultIdentity = identity;
>+
>+  // Rebuilt the identity list and select the moved identity again.
>+  refreshIdentityList();
>+  gIdentityListBox.selectedIndex = 0;
Surely you only need to refresh if you changed the identity?

>+<label control="identitiesList">&identitiesListManageDesc.label;</label>
>+<label>&identitiesListDefaultDesc.label;</label>
Unfortunately screen readers may only read the first label, since they don't know which control the second label is for. You would probably be better off using a single long label (it will wrap, so that's no problem).

>-  nsCOMPtr<nsIMsgIdentity> identity( do_QueryElementAt(m_identities, 0, &rv));
>+  nsCOMPtr<nsIMsgIdentity> identity = do_QueryElementAt(m_identities, 0, &rv);
[More unrelated changes creeping in...]

>+nsresult
>+nsMsgAccount::saveIdentitiesPref()
Nit: belongs after RemoveIdentity to avoid moving code around in the file.

>-  m_identities->Count(&count);
>-
>+  nsresult rv = m_identities->Count(&count);
>+  NS_ENSURE_SUCCESS(rv, rv);
>   NS_ENSURE_TRUE(count > 1, NS_ERROR_FAILURE); // you must have at least one identity
Nit: if Count fails, then count will be 0 anyway.
(In reply to neil@parkwaycc.co.uk from comment #32)
> >   if (gIdentityListBox.selectedItems.length <= 0)
> [Not sure how this can happen, it's a single-selection listbox!]
There are cases where no line is selected, e.g. when an identity is just deleted.
Should I ensure there is always one selected?

> >+  let identity = getSelectedIdentity();
> >+  if (identity)
> >+    gAccount.defaultIdentity = identity;
> >+
> >+  // Rebuilt the identity list and select the moved identity again.
> >+  refreshIdentityList();
> >+  gIdentityListBox.selectedIndex = 0;
> Surely you only need to refresh if you changed the identity?
The operation can only be done when a non-first identity is selected so there always will be a change. But I can play it safe and check it.
Attached patch patch v6 (obsolete) — Splinter Review
Thanks, addressed the nits. I also try to ensure there is always one identity selected.
Attachment #649000 - Attachment is obsolete: true
Attachment #649000 - Flags: ui-review?(neil)
Attachment #649000 - Flags: review?(mozilla)
Attachment #649000 - Flags: feedback?(stefanh)
Attachment #649356 - Flags: review?(neil)
(In reply to :aceman from comment #34)
> Created attachment 649356 [details] [diff] [review] (patch v6)
> Thanks, addressed the nits. I also try to ensure there is always one
> identity selected.

Aceman, since you are using refreshIdentityList() in this patch:
Does your patch fix [Bug 476044 - Adding a new identity doesn't update the list of "Identities for my account" dialogue immediately]?
Blocks: 476044
With this patch applied, what will happen in the following scenario:

STR

1 write new msg with default identity1 of account1
2 while composing that msg, goto account settings and "Manage Identities"
3 make identity2 of account1 the default identity, and close all related dialogues
4 look at your composition's From: selector

I suppose we should keep whatever is selected in From: dropdown of current compositions whenever possible, including this scenario.

I'm asking if after this patch, changing the default identity will change the from sender of currently open compositions (which it shouldn't), if they have been using the previous default identity.
(In reply to Thomas D. from comment #36)
> With this patch applied, what will happen in the following scenario:
> I'm asking if after this patch, changing the default identity will change
> the from sender of currently open compositions (which it shouldn't), if they
> have been using the previous default identity.

(This bug is one of the identity-list-changing cases mentioned in Bug 341574 Comment 13, and any identity-list changes should be updated in the identity dropdown of open compositions)
No longer blocks: 476044
Comment on attachment 649356 [details] [diff] [review]
patch v6

>+  if (aSelectIndex == null)
>+    aSelectIndex = 0;
[Not that you ever pass in null, do you?]

>+  else if (aSelectIndex > gIdentityListBox.getRowCount() - 1)
Nit: if (aSelectIndex >= gIdentityListBox.getRowCount())

>+  if (gIdentityListBox.getRowCount() > 1)
>+    gDeleteButton.removeAttribute("disabled");
[I've been trying to work out how the delete button becomes disabled again...]

>-function onSetDefault(event)
[If you do move this, I would move it before delete, so as to match with the order in the dialog.]

>+           seltype="single"
[Isn't single the default for a listbox?]

>   // now rebuild the identity pref
>   nsCAutoString identitiesKeyPref("mail.account.");
>   identitiesKeyPref.Append(m_accountKey);
[Bah, I should really fix nsMsgAccount to use a pref branch...]
(In reply to neil@parkwaycc.co.uk from comment #38)
> Comment on attachment 649356 [details] [diff] [review]
> patch v6
> 
> >+  if (aSelectIndex == null)
> >+    aSelectIndex = 0;
> [Not that you ever pass in null, do you?]
Safety net :) Should I remove it?

> >+           seltype="single"
> [Isn't single the default for a listbox?]
Yes it is, but I put it in for safety, as we rely on it. should I remove it?

> >   // now rebuild the identity pref
> >   nsCAutoString identitiesKeyPref("mail.account.");
> >   identitiesKeyPref.Append(m_accountKey);
> [Bah, I should really fix nsMsgAccount to use a pref branch...]
Yeah, I also didn't like the duplicate constructing of the pref name all over the place, but I refrained from touching that :) So please fix it.
Attached patch patch v7 (obsolete) — Splinter Review
Attachment #649356 - Attachment is obsolete: true
Attachment #649356 - Flags: review?(neil)
Attachment #651859 - Flags: review?(neil)
Attachment #651859 - Flags: review?(mconley)
(In reply to aceman from comment #39)
> (In reply to comment #38)
> > (From update of attachment 649356 [details] [diff] [review])
> > >   // now rebuild the identity pref
> > >   nsCAutoString identitiesKeyPref("mail.account.");
> > >   identitiesKeyPref.Append(m_accountKey);
> > [Bah, I should really fix nsMsgAccount to use a pref branch...]
> Yeah, I also didn't like the duplicate constructing of the pref name all
> over the place, but I refrained from touching that :) So please fix it.
I fixed it locally, but now of course your patch doesn't apply. D'oh!
Comment on attachment 651859 [details] [diff] [review]
patch v7

>+  // Ensure one identity is always selected.
>+  if (!aSelectIndex)
>+    aSelectIndex = 0;
>+  else if (aSelectIndex >= gIdentityListBox.getRowCount())
>+    aSelectIndex = gIdentityListBox.getRowCount() - 1;
>+  else if (aSelectIndex < 0)
>+    aSelectIndex = 0;
Nit: two conditions both setting aSelectIndex to 0

>+function setEnabled(aControl, aEnabled)
>+{
>+  if (!aControl)
>+    return;
>+
>+  if (aEnabled)
>+    aControl.removeAttribute("disabled");
>+  else
>+    aControl.setAttribute("disabled", true);
>+}
I don't think this is worth its own function. All the buttons should exist, so no need to test. If you don't want to set the attributes, just use the property, the XBL will do the attribute setting for you.

>+  // In this listbox there should always be one item selected.
>+  if (gIdentityListBox.selectedItems.length != 1) {
>+    // But in case this is not met (e.g. there is no identity for some reason),
>+    // disable all buttons.
[I think I might know how this happens: when the listbox is rebuilt, all of the listitems are removed, which triggers a select event, which is how we get here.]

>-  <listbox id="identitiesList" ondblclick="onEdit(event);" onselect="updateButtons();" flex="1" style="height: 0px;"/>
>+  <listbox id="identitiesList"
>+           ondblclick="onEdit(event);"
>+           onselect="updateButtons();"
>+           seltype="single"
>+           flex="1"
>+           style="height: 0px;"/>
[I still don't see why you have to change this.]

>-    <button id="editButton" disabled="true"
>+    <button id="editButton"
...
>-    <button id="deleteButton" disabled="true"
>+    <button id="setDefaultButton"
>+            oncommand="onSetDefault(event);" label="&identitiesListDefault.label;"
>+            accesskey="&identitiesListDefault.accesskey;"/>
>+    <button id="deleteButton"
The delete button will probably be initially disabled. The set default button will definitely be disabled initially! There is a minuscule advantage in having the attribute set correctly up front. Not sure what to do about the edit button though.

> <!ENTITY identitiesListDelete.label "Delete">
> <!ENTITY identitiesListDelete.accesskey "D">
>+<!ENTITY identitiesListDefault.label "Set Default">
>+<!ENTITY identitiesListDefault.accesskey "S">
Nit: put Set Default before Delete to match the XUL. (Sorry for not noticing this before.)
(In reply to neil@parkwaycc.co.uk from comment #42)
> >-  <listbox id="identitiesList" ondblclick="onEdit(event);" onselect="updateButtons();" flex="1" style="height: 0px;"/>
> >+  <listbox id="identitiesList"
> >+           ondblclick="onEdit(event);"
> >+           onselect="updateButtons();"
> >+           seltype="single"
> >+           flex="1"
> >+           style="height: 0px;"/>
> [I still don't see why you have to change this.]
Overlong line. Should I leave it and also not add seltype="single" ?
(In reply to aceman from comment #43)
> (In reply to comment #42)
> > >-  <listbox id="identitiesList" ondblclick="onEdit(event);" onselect="updateButtons();" flex="1" style="height: 0px;"/>
> > >+  <listbox id="identitiesList"
> > >+           ondblclick="onEdit(event);"
> > >+           onselect="updateButtons();"
> > >+           seltype="single"
> > >+           flex="1"
> > >+           style="height: 0px;"/>
> > [I still don't see why you have to change this.]
> Overlong line. Should I leave it and also not add seltype="single" ?
It's unusual to find seltype="single" on a listbox, but you can keep it if you want.
Attached patch patch v8 (obsolete) — Splinter Review
Fixes nits and also bitrot from bug 783821 (the pref branch rework), that was really nasty :)
Attachment #651859 - Attachment is obsolete: true
Attachment #651859 - Flags: review?(neil)
Attachment #651859 - Flags: review?(mconley)
Attachment #655451 - Flags: review?(neil)
Comment on attachment 655451 [details] [diff] [review]
patch v8

Sorry for the delay; now you have to deal with the nsCAutoString change too...
Attachment #655451 - Flags: review?(neil) → review+
Attached patch patch v9 (obsolete) — Splinter Review
Attachment #655451 - Attachment is obsolete: true
Attachment #659824 - Flags: review?(mconley)
aceman, can you attach a screenshot of the current patch in action?
Why? I have ui-r+ already. And there is not much change here, only one new button of "Set default" under "Delete" in the identity list dialog.
(In reply to :aceman from comment #49)
> Why? I have ui-r+ already. And there is not much change here, only one new
> button of "Set default" under "Delete" in the identity list dialog.

I was just wondering if we have any "Move up" or "Move down" buttons here because Blake suggested in Bug 244347 Comment 44 that we might re-use the identities dialogue as an UI for moving accounts around.
No, there are none yet.
Comment on attachment 659824 [details] [diff] [review]
patch v9

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

Just a couple of questions - see below.

::: mailnews/base/prefs/content/am-identities-list.js
@@ +44,5 @@
>  
> +  // Build the list from the identities array.
> +  let identities = gAccount.identities;
> +  let identitiesCount = identities.Count();
> +  for (let j = 0; j < identitiesCount; j++)

I think you could just use fixIterator here - like this: http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#27

@@ +123,4 @@
>    }
> +
> +  gEditButton.disabled = false;
> +  gDeleteButton.disabled = gIdentityListBox.getRowCount() <= 1;

So the delete and edit buttons should be enabled if there are 0 rows available?

::: mailnews/base/src/nsMsgAccount.cpp
@@ +266,5 @@
>  
> +  // The passed in identity is in the list, so we have at least one element.
> +  m_identities->MoveElement(position, 0);
> +
> +  saveIdentitiesPref();

What about saveIdentitiesPref's return value? Shouldn't we be returning that, instead of assuming everything is NS_OK?

@@ +333,5 @@
>      m_prefs->SetCharPref("identities", newIdentityList.get());
>    }
>  
>    // now add it to the in-memory list
> +  return addIdentityInternal(identity);

So if m_defaultIdentity is null... what then? What is the default identity?
Attachment #659824 - Flags: review?(mconley) → review-
Attached patch patch v10Splinter Review
I addressed all nits, except the last one. There is no m_defaultIdentity after the patch is applied. The default identity is returned by the GetDefaultIdentity() and that is unchanged. m_defaultIdentity was not used even before the patch, that is why it goes away.
Attachment #659824 - Attachment is obsolete: true
Attachment #661928 - Flags: review?(mconley)
(In reply to Mike Conley from comment #52)
> > +  gEditButton.disabled = false;
> So the delete and edit buttons should be enabled if there are 0 rows
> available?
How can the selection count be 1 if the are 0 rows?
Maybe err on the side of caution :)
Comment on attachment 661928 [details] [diff] [review]
patch v10

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

Ok, I'm happy with this / convinced. Thanks aceman!
Attachment #661928 - Flags: review?(mconley) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/1c00e984dde1

Should this have a test?
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
Blocks: 792334
Would be nice, opened bug 792334 to cover more operations in the identity list editor. Which of these 2 bugs should have the in-testsuite flag set?
As for me, active developer, community member, contractor and personal user, I would like to be able to set the order exactly. The reason is that I use the same set of about 10 accounts on my smartphone and webmail. The default identity is the one I use the most but 40% of my emails require me to choose one of the other addresses. Very frequently I have to choose one from the list. Being able set the same order eases that process a lot. I know exactly where to click and prevents me making mistakes by selecting the wrong sender address.

Perhaps the Move Up and Move Down buttons can be positioned on the left of the Cancel button and be aligned to the left under the list of identities.
Oops, excuse me for the previous comment. It was intended for bug 384303.
For Addons: the nsIMsgAccount::setDefaultIdentity function now does a permanent change of identity order.
Keywords: addon-compat
You need to log in before you can comment on or make changes to this bug.