Last Comment Bug 314806 - No function to set default identity
: No function to set default identity
Status: RESOLVED FIXED
: addon-compat
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
Mentors:
Depends on: 360926
Blocks: identity_ordering 792334
  Show dependency treegraph
 
Reported: 2005-11-02 11:16 PST by Matt Heinrichs
Modified: 2013-03-11 10:10 PDT (History)
14 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (4.97 KB, patch)
2012-07-02 13:30 PDT, :aceman
mozilla: feedback+
Details | Diff | Review
patch v2 (12.21 KB, patch)
2012-07-19 15:04 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Review
patch v3 (16.83 KB, patch)
2012-07-23 02:19 PDT, :aceman
bwinton: ui‑review+
Details | Diff | Review
patch v4 (17.50 KB, patch)
2012-07-25 11:19 PDT, :aceman
iann_bugzilla: review+
Details | Diff | Review
patch v5 (17.19 KB, patch)
2012-08-04 08:20 PDT, :aceman
no flags Details | Diff | Review
patch v6 (19.03 KB, patch)
2012-08-06 12:43 PDT, :aceman
no flags Details | Diff | Review
patch v7 (20.09 KB, patch)
2012-08-14 13:02 PDT, :aceman
no flags Details | Diff | Review
patch v8 (19.43 KB, patch)
2012-08-26 13:25 PDT, :aceman
neil: review+
Details | Diff | Review
patch v9 (19.43 KB, patch)
2012-09-10 12:34 PDT, :aceman
mconley: review-
Details | Diff | Review
patch v10 (20.38 KB, patch)
2012-09-17 14:43 PDT, :aceman
mconley: review+
Details | Diff | Review

Description Matt Heinrichs 2005-11-02 11:16:46 PST
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
Comment 1 WADA 2006-02-16 19:13:50 PST
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.
Comment 2 Ankit Singla 2007-03-24 17:43:56 PDT
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"?
Comment 3 Ori Avtalion (salty-horse) 2007-12-21 17:09:00 PST
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.
Comment 4 Panz 2012-01-26 05:15:05 PST
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.
Comment 5 :aceman 2012-07-02 00:00:18 PDT
I can try this.
Comment 6 :aceman 2012-07-02 13:30:30 PDT
Created attachment 638474 [details] [diff] [review]
WIP patch

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?
Comment 7 :aceman 2012-07-02 13:54:42 PDT
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 8 David :Bienvenu 2012-07-03 08:25:07 PDT
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.
Comment 9 :aceman 2012-07-03 11:27:33 PDT
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.
Comment 10 :aceman 2012-07-19 15:04:59 PDT
Created attachment 644031 [details] [diff] [review]
patch v2

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?
Comment 11 :aceman 2012-07-19 15:08:52 PDT
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.
Comment 12 Ian Neal 2012-07-22 12:22:49 PDT
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.
Comment 13 :aceman 2012-07-22 12:34:13 PDT
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.
Comment 14 :aceman 2012-07-22 13:32:04 PDT
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.
Comment 15 Ian Neal 2012-07-22 13:44:45 PDT
(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.
Comment 16 :aceman 2012-07-22 13:57:52 PDT
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? :)
Comment 17 Ian Neal 2012-07-22 14:54:52 PDT
(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.
Comment 18 :aceman 2012-07-22 21:41:19 PDT
Ok, I can use accountManage.css.
Comment 19 :aceman 2012-07-23 02:19:58 PDT
Created attachment 644875 [details] [diff] [review]
patch v3

I hope I addressed everything.
Comment 20 Ian Neal 2012-07-23 17:31:43 PDT
(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
Comment 21 :aceman 2012-07-23 22:00:20 PDT
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?
Comment 22 Ian Neal 2012-07-24 04:49:54 PDT
(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).
Comment 23 :aceman 2012-07-24 06:23:49 PDT
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 24 Blake Winton (:bwinton) (:☕️) 2012-07-25 08:53:34 PDT
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.
Comment 25 :aceman 2012-07-25 10:41:54 PDT
(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 :)
Comment 26 :aceman 2012-07-25 11:19:58 PDT
Created attachment 645825 [details] [diff] [review]
patch v4

Fixed the string in TB and SM, merged defaultIdentity with defaultAccount CSS and added the SM Mac theme.
Comment 27 Ian Neal 2012-07-28 18:35:10 PDT
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
Comment 28 :aceman 2012-07-29 05:26:40 PDT
I'd still like somebody to review the backend part. I am not sure I can count on bienvenu. Can Neil do it?
Comment 29 Ian Neal 2012-07-29 05:34:08 PDT
(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.
Comment 30 Ian Neal 2012-08-04 06:19:28 PDT
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)
Comment 31 :aceman 2012-08-04 08:20:18 PDT
Created attachment 649000 [details] [diff] [review]
patch v5

Thanks, updated.
Comment 32 neil@parkwaycc.co.uk 2012-08-05 13:59:23 PDT
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.
Comment 33 :aceman 2012-08-06 12:12:44 PDT
(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.
Comment 34 :aceman 2012-08-06 12:43:08 PDT
Created attachment 649356 [details] [diff] [review]
patch v6

Thanks, addressed the nits. I also try to ensure there is always one identity selected.
Comment 35 Thomas D. (currently busy elsewhere; needinfo?me) 2012-08-06 12:53:55 PDT
(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]?
Comment 36 Thomas D. (currently busy elsewhere; needinfo?me) 2012-08-06 14:01:31 PDT
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.
Comment 37 Thomas D. (currently busy elsewhere; needinfo?me) 2012-08-06 14:04:07 PDT
(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)
Comment 38 neil@parkwaycc.co.uk 2012-08-07 09:25:16 PDT
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...]
Comment 39 :aceman 2012-08-13 00:10:33 PDT
(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.
Comment 40 :aceman 2012-08-14 13:02:58 PDT
Created attachment 651859 [details] [diff] [review]
patch v7
Comment 41 neil@parkwaycc.co.uk 2012-08-17 14:27:24 PDT
(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 42 neil@parkwaycc.co.uk 2012-08-18 09:27:33 PDT
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.)
Comment 43 :aceman 2012-08-26 13:01:46 PDT
(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" ?
Comment 44 neil@parkwaycc.co.uk 2012-08-26 13:05:35 PDT
(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.
Comment 45 :aceman 2012-08-26 13:25:36 PDT
Created attachment 655451 [details] [diff] [review]
patch v8

Fixes nits and also bitrot from bug 783821 (the pref branch rework), that was really nasty :)
Comment 46 neil@parkwaycc.co.uk 2012-09-07 16:43:24 PDT
Comment on attachment 655451 [details] [diff] [review]
patch v8

Sorry for the delay; now you have to deal with the nsCAutoString change too...
Comment 47 :aceman 2012-09-10 12:34:29 PDT
Created attachment 659824 [details] [diff] [review]
patch v9
Comment 48 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-16 03:06:21 PDT
aceman, can you attach a screenshot of the current patch in action?
Comment 49 :aceman 2012-09-16 04:19:29 PDT
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.
Comment 50 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-16 05:16:38 PDT
(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.
Comment 51 :aceman 2012-09-16 05:37:03 PDT
No, there are none yet.
Comment 52 Mike Conley (:mconley) - (needinfo me!) 2012-09-17 13:35:11 PDT
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?
Comment 53 :aceman 2012-09-17 14:43:09 PDT
Created attachment 661928 [details] [diff] [review]
patch v10

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.
Comment 54 neil@parkwaycc.co.uk 2012-09-18 00:44:55 PDT
(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?
Comment 55 :aceman 2012-09-18 01:10:03 PDT
Maybe err on the side of caution :)
Comment 56 Mike Conley (:mconley) - (needinfo me!) 2012-09-18 12:16:47 PDT
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!
Comment 57 Ryan VanderMeulen [:RyanVM] 2012-09-18 18:26:02 PDT
https://hg.mozilla.org/comm-central/rev/1c00e984dde1

Should this have a test?
Comment 58 :aceman 2012-09-19 00:15:37 PDT
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?
Comment 59 Pander 2012-09-20 00:43:09 PDT
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.
Comment 60 Pander 2012-09-20 00:46:48 PDT
Oops, excuse me for the previous comment. It was intended for bug 384303.
Comment 61 :aceman 2013-03-11 10:10:11 PDT
For Addons: the nsIMsgAccount::setDefaultIdentity function now does a permanent change of identity order.

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