Closed Bug 793135 Opened 12 years ago Closed 12 years ago

Identity picker on compose window is hard to use when you have a lot of identities (Port TB Bug 679696)

Categories

(SeaMonkey :: MailNews: Composition, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.16

People

(Reporter: philip.chee, Assigned: lgrosenthal)

References

Details

(Whiteboard: [good first bug][mentor=IanN][lang=js][level=apprentice])

Attachments

(1 file, 4 obsolete files)

From Bug 679696 Comment 0:

> 90% of the time when I'm manually setting an identity, I actually want the 
> primary identity for an account, just a different one than it defaulted to.  The 
> primary identities are hard to find because they look just like the rest of them.
> 
> Two possible solutions for this:
> 1) Make the primary identities all show as boldface in the menu
> 
> or
> 
> 2) Draw a divider line between each account's identities within the menu, which 
> will still make primary identities easy to spot, since they're the first one 
> within each group.
Typo in my above text aside, the patch was against comm-central. Unfortunately, I don;t have that version installed to test :-( , but my back-port to the version I *am* running seems to achieve the same result as the TB enhancement.

I imported iteratorUtils.jsm, a la TB; I don't know if this is desirable.
Ooo. Thanks for this patch! I'll CC some mailnews person to review this patch.
Whiteboard: [good first bug][mentor=IanN][lang=js][level=apprentice]
Comment on attachment 667336 [details] [diff] [review]
Proposed integratioin of TB enhancmeent v1

Setting review flags.
Attachment #667336 - Flags: review?(iann_bugzilla)
Attachment #667336 - Flags: feedback?(mnyromyr)
I'll properly test his against trunk later today. While it effectively mimics the behavior in TB, and produces separator lines, for more clarity in the list, we still need some way to order/sort the array of identities (and I can find no bug for sorting/ordering the list of identities in the identity manager, either). In my own use case, sorting by name alone won't do it, as I have multiple domains with the same account name, as well as multiple account names with the same domain (and often, the real name attached to each of these, in both cases, is the same).
Comment on attachment 667336 [details] [diff] [review]
Proposed integratioin of TB enhancmeent v1

>+// Ensure the activity modules are loaded for this window.
>+Components.utils.import("resource:///modules/iteratorUtils.jsm");

That comment doesn't quite match the actual import. ;-)


>+   let accountHadSeparator = false;

Hm, I'm not quite convinced of the separator idea. The separators are very visible and provide an emphasis on that account which isn't justified. Making the base identities bold, otoh, is much less invasive while still helpful.

>+   let firstAccountWithIdentities = true;
>+   for (let acc = 0; acc < accounts.length; acc++) {

One space too much indent. Braces should go on their own line in suite/mailnews land.
Attachment #667336 - Flags: feedback?(mnyromyr) → feedback-
Hi, Karsten!

(In reply to Karsten Düsterloh from comment #6)
> Comment on attachment 667336 [details] [diff] [review]
> Proposed integratioin of TB enhancmeent v1
> 
> >+// Ensure the activity modules are loaded for this window.
> >+Components.utils.import("resource:///modules/iteratorUtils.jsm");
> 
> That comment doesn't quite match the actual import. ;-)
> 
;-)

Changed in my working copy to something more appropriate.

> Hm, I'm not quite convinced of the separator idea. The separators are very
> visible and provide an emphasis on that account which isn't justified.
> Making the base identities bold, otoh, is much less invasive while still
> helpful.
 
I may be in agreement with you. It would e nice to see, but my first attempt (based on an earlier approach in the TB bug) didn't seem to work too well (lost the identity list...oops...)

> >+   let firstAccountWithIdentities = true;
> >+   for (let acc = 0; acc < accounts.length; acc++) {
> 
> One space too much indent. Braces should go on their own line in
> suite/mailnews land.

Thanks for that. I wasn't sure what the proper etiquette was for us vs TB/FF. I tend to like them on separate lines (makes counting indents without a colorizing editor much easier to follow). I'll remember from now on. ;-)

I'll cobble this together over the next couple days, unless someone else picks up on it, and hopefully, I'll have something working. Ultimately, I'd like to add a pref to toggle this behavior - any thoughts on that, vs forcing users to accept the "new" look?
Essentially the same as v1, but cleaner code, per Karsten's suggestions.
Attachment #667336 - Attachment is obsolete: true
Attachment #667336 - Flags: review?(iann_bugzilla)
Alternative suggestion, setting default identity to bold only (no separator line).
The unintended consequence of this (fit & polish issue, IMO) is that in the Identity Manager list, the default identity (which is always listed first, anyway) is (unnecessarily) bold. Is this what we want, or do I need to be more specific as to when/where we're building the identity list?
Corrects a minor issue where a single line of code was leftover from previous version of patch.
Attachment #668736 - Attachment is obsolete: true
(In reply to Lewis Rosenthal from comment #10)
> The unintended consequence of this (fit & polish issue, IMO) is that in the
> Identity Manager list, the default identity (which is always listed first,
> anyway) is (unnecessarily) bold. Is this what we want, or do I need to be
> more specific as to when/where we're building the identity list?

Actually, this isn't as bad as I had originally thought, as changing the default identity in the Identity Manager results in the new default identity beocming bold and moving to the top of the list. The heavier font weight makes it easier for the eye to follow the change.

Apologies for the bug spam.
Any thoughts on patch v4?
Comment on attachment 668808 [details] [diff] [review]
patch v4 - default identity in bold; no separator; removed extraneous code

> Any thoughts on patch v4?
Setting the feedback? flag is the way to signal that you are looking for feedback/thoughts.

Here are some background reading materials:
https://developer.mozilla.org/en-US/docs/Creating_a_patch
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
Attachment #668808 - Flags: feedback?(mnyromyr)
Attachment #668808 - Flags: feedback?(iann_bugzilla)
Comment on attachment 668808 [details] [diff] [review]
patch v4 - default identity in bold; no separator; removed extraneous code

>+/*
>+ * activity module
>+ */
Comment is probably not needed, the name iteratorUtils is fairly self-explanatory.
>+Components.utils.import("resource:///modules/iteratorUtils.jsm");

>+   let firstAccountWithIdentities = true;
This variable is no longer used, so you can remove it.
Attachment #668808 - Flags: feedback?(iann_bugzilla) → feedback+
Assignee: nobody → lgrosenthal
Status: NEW → ASSIGNED
Addresses Ian's concerns. Should we really not identify the loading of the activity module in the same manner in which interfaces are identified? Just a minor nit. ;-)

Patch is against comm-central. I can port to other builds, if there is need (obviously).
Attachment #668808 - Attachment is obsolete: true
Attachment #668808 - Flags: feedback?(mnyromyr)
Comment on attachment 675667 [details] [diff] [review]
patch v5 - default identity in bold; no separator; removed (more) extraneous code [checkin comment 21]

Ask for some feedback/moa from Mnyromyr
Attachment #675667 - Flags: review+
Attachment #675667 - Flags: feedback?(mnyromyr)
Attachment #668733 - Attachment is obsolete: true
(In reply to Philip Chee from comment #14)
> Setting the feedback? flag is the way to signal that you are looking for
> feedback/thoughts.
> 
> Here are some background reading materials:
> https://developer.mozilla.org/en-US/docs/Creating_a_patch
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> How_to_Submit_a_Patch

Sorry I missed your earlier comment, Phil, and thanks so much for the references. I've started reading them, and hopefully, I'll get some of this "green" off from around my gills! :-)

Cheers from Long Island, NY, where we survived Sandy!
Comment on attachment 675667 [details] [diff] [review]
patch v5 - default identity in bold; no separator; removed (more) extraneous code [checkin comment 21]

>+     if (identities.length == 0)
>+       continue;

JFTR: It's common practice to write such checks as 

       if (!identities.length)
         continue;
 
moa=me either way.

> Cheers from Long Island, NY, where we survived Sandy!

Pleased to hear that! :)
Attachment #675667 - Flags: feedback?(mnyromyr) → superreview+
Now that you have r+ and sr+ the next step is to get this patch checked in. Since you don't have checkin rights adding checkin-needed will alert someone to check this in for you.  However I'll do that for you in a jiffy.
Keywords: checkin-needed
Your patch was severely bit-rotted by Bug 749200 but not to worry I've fixed that on check-in.

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/365e64d4b199
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.16
Attachment #675667 - Attachment description: patch v5 - default identity in bold; no separator; removed (more) extraneous code → patch v5 - default identity in bold; no separator; removed (more) extraneous code [checkin comment 21]
Thanks, Phil (and thanks, too, Karsten!)!

I was going to tweak that minor point Karsten made in his comment # 19, but no matter. I also completely missed Bug 749200; thanks for tidying things up for me.

Do you suppose there is any need to add a preference to turn this feature on or off (separate bug, I know; just gauging response)? Not sure of the criteria for proposing new prefs.
(In reply to Lewis Rosenthal from comment #22)
> Do you suppose there is any need to add a preference to turn this feature on
> or off (separate bug, I know; just gauging response)? 

No. :-)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: