Last Comment Bug 793135 - Identity picker on compose window is hard to use when you have a lot of identities (Port TB Bug 679696)
: Identity picker on compose window is hard to use when you have a lot of ident...
Status: RESOLVED FIXED
[good first bug][mentor=IanN][lang=js...
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Composition (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.16
Assigned To: Lewis Rosenthal
:
:
Mentors:
Depends on: 679696
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-21 04:53 PDT by Philip Chee
Modified: 2012-11-06 22:58 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed integratioin of TB enhancmeent v1 (3.99 KB, patch)
2012-10-02 23:11 PDT, Lewis Rosenthal
mnyromyr: feedback-
Details | Diff | Splinter Review
patch v2 - menuseparator + bold; cleaner code (3.99 KB, patch)
2012-10-05 23:19 PDT, Lewis Rosenthal
no flags Details | Diff | Splinter Review
patch v3 - default identity in bold; no separator (3.39 KB, patch)
2012-10-05 23:22 PDT, Lewis Rosenthal
no flags Details | Diff | Splinter Review
patch v4 - default identity in bold; no separator; removed extraneous code (3.35 KB, patch)
2012-10-06 12:21 PDT, Lewis Rosenthal
iann_bugzilla: feedback+
Details | Diff | Splinter Review
patch v5 - default identity in bold; no separator; removed (more) extraneous code [checkin comment 21] (3.28 KB, patch)
2012-10-26 13:19 PDT, Lewis Rosenthal
iann_bugzilla: review+
mnyromyr: superreview+
Details | Diff | Splinter Review

Description Philip Chee 2012-09-21 04:53:58 PDT
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.
Comment 1 Lewis Rosenthal 2012-10-02 23:11:19 PDT
Created attachment 667336 [details] [diff] [review]
Proposed integratioin of TB enhancmeent v1
Comment 2 Lewis Rosenthal 2012-10-02 23:15:23 PDT
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.
Comment 3 Philip Chee 2012-10-03 03:09:31 PDT
Ooo. Thanks for this patch! I'll CC some mailnews person to review this patch.
Comment 4 Philip Chee 2012-10-03 03:10:34 PDT
Comment on attachment 667336 [details] [diff] [review]
Proposed integratioin of TB enhancmeent v1

Setting review flags.
Comment 5 Lewis Rosenthal 2012-10-03 07:06:31 PDT
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 6 Karsten Düsterloh 2012-10-03 09:21:56 PDT
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.
Comment 7 Lewis Rosenthal 2012-10-03 17:05:50 PDT
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?
Comment 8 Lewis Rosenthal 2012-10-05 23:19:31 PDT
Created attachment 668733 [details] [diff] [review]
patch v2 - menuseparator + bold; cleaner code

Essentially the same as v1, but cleaner code, per Karsten's suggestions.
Comment 9 Lewis Rosenthal 2012-10-05 23:22:56 PDT
Created attachment 668736 [details] [diff] [review]
patch v3 - default identity in bold; no separator

Alternative suggestion, setting default identity to bold only (no separator line).
Comment 10 Lewis Rosenthal 2012-10-06 06:16:42 PDT
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?
Comment 11 Lewis Rosenthal 2012-10-06 12:21:49 PDT
Created attachment 668808 [details] [diff] [review]
patch v4 - default identity in bold; no separator; removed extraneous code

Corrects a minor issue where a single line of code was leftover from previous version of patch.
Comment 12 Lewis Rosenthal 2012-10-06 12:23:59 PDT
(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.
Comment 13 Lewis Rosenthal 2012-10-18 19:06:11 PDT
Any thoughts on patch v4?
Comment 14 Philip Chee 2012-10-20 10:05:00 PDT
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
Comment 15 Ian Neal 2012-10-23 13:21:40 PDT
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.
Comment 16 Lewis Rosenthal 2012-10-26 13:19:16 PDT
Created attachment 675667 [details] [diff] [review]
patch v5 - default identity in bold; no separator; removed (more) extraneous code [checkin comment 21]

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).
Comment 17 Ian Neal 2012-10-27 04:14:24 PDT
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
Comment 18 Lewis Rosenthal 2012-10-30 13:22:39 PDT
(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 19 Karsten Düsterloh 2012-11-04 12:54:59 PST
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! :)
Comment 20 Philip Chee 2012-11-06 05:20:02 PST
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.
Comment 21 Philip Chee 2012-11-06 06:01:12 PST
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
Comment 22 Lewis Rosenthal 2012-11-06 12:15:59 PST
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.
Comment 23 Karsten Düsterloh 2012-11-06 22:58:40 PST
(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. :-)

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