Closed Bug 649989 Opened 10 years ago Closed 10 years ago

display account names in favorite folders pane view only when there are duplicated folder names and location info in the tooltip

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 7.0

People

(Reporter: stef, Assigned: stef)

References

Details

Attachments

(2 files, 10 obsolete files)

Right now (tb 3.1, 3.3pre) in favorite folders pane view we display account name alongside every folder name (and optionally unread count). This brings a lot unwanted and not really needed noise to main user interface of Thunderbird.

In Thunderbird 2 (not sure when this changed) there were only folder names (and unread counts) and it was somewhat satisfying since one has explicit control over selection of folders to be displayed in this view and needs to explicitly optin to have sth displayed there (duplicated or not).

Probably to work around some (rare?) cases when user cannot change some folder names and still wants them to be displayed in this view there was a change to simply add account names…

Displaying account names for every folder is not needed (since user explicitly selected them and knows where they belong to), distracts and is only useful for duplicated folder names. We should provide a better way to distinguish folders with duplicated names to lower the noise factor.

Displaying account names only for folders with duplicated names should be good enough solution but there may be as well some better one.
Attached patch comm-central patch (obsolete) — Splinter Review
Display account names only for folders with duplicated names in favorite folders folder pane view
Attachment #525990 - Flags: review?
Attachment #525990 - Flags: feedback?
I definitely remember getting complaints because people couldn't remember which accounts their folders were in, but I don't remember if we changed it deliberately - perhaps it broke when we rewrote the folder pane in js.
(In reply to comment #2)
If this is the case for folders with unique names (I doubt and quite different one anyways, imo), displaying such information in tooltips would be much better idea (to remind location) but would not really work for duplicated names - I would treat this as separate issue.
(In reply to comment #2)
> I definitely remember getting complaints because people couldn't remember which
> accounts their folders were in

While searching for this I have found bugs like 468793, 540316 and not too much complains about remembering case but it was just fast search.
Comment on attachment 525990 [details] [diff] [review]
comm-central patch

We might need  test, but I'm not sure.

Thank for the patch.
Attachment #525990 - Flags: review?(bugmail)
Attachment #525990 - Flags: review?
Attachment #525990 - Flags: feedback?
Assignee: nobody → splewako
Status: NEW → ASSIGNED
Comment on attachment 525990 [details] [diff] [review]
comm-central patch

The folder list module owner is bienvenu, per https://wiki.mozilla.org/Modules:Thunderbird so transferring the review request to him.
Attachment #525990 - Flags: review?(bugmail) → review?(dbienvenu)
thx for the patch - I assume unames is uniqueNames?
(In reply to comment #7)
> I assume unames is uniqueNames?

Yes.
ok, thx for the patch - I would change the var name to be something a bit more descriptive like uniqueNames instead of uNames. Also, we should have a mozmill test for this - something similar to mail\test\mozmill\folder-tree-modes\test-unread-folders.js, except using the favorites mode, and using 

let folderFTVItem = mc.folderTreeView.getFTVItemForIndex(folderIndex);

to get the folder item and check its text. If you need help writing a mozmill test for this, please let me know.

I think we also want ui-review on this. I'll request one...

Finally, if I have two favorite sub-folders in the same account with the same name, they'll both get the account name, but it'll be the same. Probably nothing much we can do about that.
Attachment #525990 - Flags: ui-review?(bwinton)
I did try running with the patch, and it seems to work fine.
Comment on attachment 525990 [details] [diff] [review]
comm-central patch

Review of attachment 525990 [details] [diff] [review]:

r=me, if you change the variable name to be more descriptive. But we will need a mozmill test before we could land this.
Attachment #525990 - Flags: review?(dbienvenu) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Patch with updated variable name (as per comment #c9 and #c11)
Attachment #525990 - Attachment is obsolete: true
Attachment #525990 - Flags: ui-review?(bwinton)
Attachment #528562 - Flags: ui-review?(bwinton)
Comment on attachment 528562 [details] [diff] [review]
patch v2

Review of attachment 528562 [details] [diff] [review]:

Yeah, that definitely looks a lot better to me.

I would like to see the account as a tooltip, though.  And since we're going to need a new patch for the mozmill test anyways, I'll set this to ui-r- so that I can do a ui-review of the tooltip stuff.  (But I do mostly like the change!)

Thanks,
Blake.
Attachment #528562 - Flags: ui-review?(bwinton) → ui-review-
Attached file mozmill test, wip (obsolete) —
Since it was impossible for me to set up mozmill to run mail/test/mozmill/ tests correctly I coded this one for mozmill extension with profile setup (prefs as in file) extracted from runtest.py.

Generally it works (tweak assert_equals for unique folder may be needed depending if testing with the patch applied), albeit it is not perfect for sure and I'd appreciate any feedback…
Attachment #529280 - Flags: feedback?
Attachment #529280 - Flags: feedback? → feedback?(bugmail)
Attachment #529280 - Flags: feedback?(bugmail)
Attached patch patch v3 (obsolete) — Splinter Review
patch v2 + tooltip thing
Attachment #528562 - Attachment is obsolete: true
Attachment #530922 - Flags: ui-review?(bwinton)
Attachment #530922 - Flags: review?(dbienvenu)
Attached image patch v3 preview (obsolete) —
Comment on attachment 530922 [details] [diff] [review]
patch v3

I might have used a / instead of " - " for the separator, but then I'm a bit more of a geek than our average user.  :)

As it is, I like it!  ui-r=me

Thanks,
Blake.
Attachment #530922 - Flags: ui-review?(bwinton) → ui-review+
Attachment #530922 - Flags: review?(dbienvenu) → review+
Comment on attachment 529280 [details]
mozmill test, wip

So, basically I need to finish the mozmill part and this will be ready to land?

As stated in comment #14 I will need some feedback with the test since I'm unable to setup environment for mail/test/mozmill/ tests correctly (thus coded for extension, but should work anyway) - it could be in a form of attachment review or sent to my bugzilla email (doesn't matter for me).

Re-setting feedback flag since it got cleared.
Attachment #529280 - Flags: feedback?
Attachment #529280 - Flags: feedback? → feedback?(sid.bugzilla)
Comment on attachment 529280 [details]
mozmill test, wip

So the test looks fine but it doesn't follow house style. We have a lot of helpers in test-folder-display-helpers.js that'll make your life a ton simpler.

The first thing you'll want to do is import folder-display-helpers, like so: <https://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-message-reloads.js#49>. That should give you access to mc, Ci, Cc and a lot of other helpers.

>var mc;

You won't need this after that.

>
>const acc = Components.interfaces.nsIMsgAccount;

You can just use Ci.nsIMsgAccount everywhere.

>const acm = Components.classes["@mozilla.org/messenger/account-manager;1"]
>                      .getService(Components.interfaces.nsIMsgAccountManager);

MailServices will become available, which will allow you to use MailServices.accounts for this.

>const ffl = Components.interfaces.nsMsgFolderFlags;

Again, Ci.nsMsgFolderFlags everywhere is fine.

>
>function assert_equals(a, b) {
>  if (a != b)
>    throw new Error("a != b: '" + a + "' != '" + b + "'.");
>};

folder-display-helpers already contains this. :)

>
>var setupModule = function(module) {

House style is function setupModule(module) {

>  module.controller = mozmill.getMail3PaneController();
>  mc = module.controller;

You won't need this afterwards.

>};
>
>var testForBug649989 = function() {

function test_foo, and we prefer underscores to camelCase for this.

>  let ac1 = acm.accounts.QueryElementAt(0, acc);
>  let ac2 = acm.accounts.QueryElementAt(1, acc);
>  let rf1 = ac1.incomingServer.rootFolder;
>  let rf2 = ac2.incomingServer.rootFolder;
>
>  rf1.createSubfolder("uniqueName", null);
>  rf1.createSubfolder("duplicatedName", null);
>  rf2.createSubfolder("duplicatedName", null);
>  rf2.getFolderWithFlags(ffl.Inbox).createSubfolder("duplicatedName", null);
>
>  let fu0 = rf1.getChildNamed("uniqueName");
>  let fd1 = rf1.getChildNamed("duplicatedName");
>  let fd2 = rf2.getChildNamed("duplicatedName");
>  let fd3 = rf2.getFolderWithFlags(ffl.Inbox).getChildNamed("duplicatedName");
>
>  fu0.flags |= ffl.Favorite
>  fd1.flags |= ffl.Favorite
>  fd2.flags |= ffl.Favorite
>  fd3.flags |= ffl.Favorite

Semicolons after each line, please.

>
>  mc.window.gFolderTreeView.mode = "favorite";
>
>  assert_equals(mc.window.gFolderTreeView.getFTVItemForIndex(3).text,
>                "uniqueName - Local Folders");
>  assert_equals(mc.window.gFolderTreeView.getFTVItemForIndex(0).text,
>                "duplicatedName - Local Folders");
>  assert_equals(mc.window.gFolderTreeView.getFTVItemForIndex(1).text,
>                "duplicatedName - tinderbox@invalid.com");
>  assert_equals(mc.window.gFolderTreeView.getFTVItemForIndex(2).text,
>                "duplicatedName - tinderbox@invalid.com");

How about extracting this into an assert_folder_at_index_displayed_as(n, str) or similar?

feedback+ based on the fact that the test looks good otherwise.
Attachment #529280 - Flags: feedback?(sid.bugzilla) → feedback+
Attached patch patch v4 (obsolete) — Splinter Review
patch v3 + mozmill test
Attachment #529280 - Attachment is obsolete: true
Attachment #532918 - Flags: review?(dbienvenu)
This patch looks fine, but I can't get the mozmill test to run for some reason - it says 0 tests passing/failing, and the test doesn't fail in a build w/o the patch to the folder pane code. I'm not sure what's going on there...
Comment on attachment 532918 [details] [diff] [review]
patch v4

seems to be some sort of problem with mozmill on my machine, with trunk builds. Thx for the patch and test!
Attachment #532918 - Flags: review?(dbienvenu) → review+
(In reply to comment #17)
> Comment on attachment 530922 [details] [diff] [review] [review]
> patch v3
> 
> I might have used a / instead of " - " for the separator, but then I'm a bit
> more of a geek than our average user.  :)

Agreed. I'm probably not the most representative person either, but it took me a while to figure out what was going on in that screenshot> To be fair, I don't use favorite/unified folders much, so my unfamiliarity with them may be tainting my perspective.
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/3c71fa107bce

Thanks for the patch Stefan.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 7.0
I had to land a bustage fix for the mozmill tests:

http://hg.mozilla.org/comm-central/rev/24639f45f5c1
It appears this broke notifications on Linux (including more MozMill test failures). Mike said to me over irc:

in newmailalert.js, folderSummaryInfoEl.mMaxMsgHdrsInPopup is being called, which is defined in mailWidgets.xml - but mailWidgets.xml is failing out, complaining that:  "gFolderTreeView is not defined" on line 2137

Therefore I've backed out both the bustage fix and the main patch:

http://hg.mozilla.org/comm-central/rev/91f133c6ed33
http://hg.mozilla.org/comm-central/rev/c2d23014f95c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch v5 (obsolete) — Splinter Review
Patch with mozmill fix integrated (from #c25) and gFolderTreeView.mode as aTreeViewMode parameter for parseFolder.
Attachment #537118 - Flags: review?
Attachment #537118 - Flags: review? → review?(dbienvenu)
Comment on attachment 537118 [details] [diff] [review]
patch v5

why not move the popup tooltip handling to popupshowing in mailWidgets.xml? Then, we wouldn't be confusing newmailalert.js
Comment on attachment 537118 [details] [diff] [review]
patch v5

I'm going to minus this since I think it would simplify things to move the tooltip code up a level. If there's a good reason why that won't work, let me know and I'll reconsider.
Attachment #537118 - Flags: review?(dbienvenu) → review-
Attached patch patch v6 (obsolete) — Splinter Review
v6 moves location/name code part into its own binding separating it from message summaries
Attachment #539547 - Flags: review?(dbienvenu)
Stefan, thx for doing reworking this; that seems like a much better structuring - however, running with your patch, I'm not seeing the tooltips for the account name, and I'm seeing one of my Inboxes just called "Inbox" and the rest have account names. I also see this on the error console:

Warning: reference to undefined property uniqueNames[folder.text]
Source File: chrome://messenger/content/folderPane.js
Line: 1337
Comment on attachment 539547 [details] [diff] [review]
patch v6

ouch… sorry.
Attachment #539547 - Flags: review?(dbienvenu)
Attached patch patch v7 (obsolete) — Splinter Review
v7 - correct tree mode name in mailWidgets.xml and some changes in folderPane.js

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=90437ef573f9
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/splewako@aviary.pl-90437ef573f9/
Attachment #537118 - Attachment is obsolete: true
Attachment #539547 - Attachment is obsolete: true
Attachment #539837 - Flags: review?(dbienvenu)
ah, this looks better. But now when I hover over a folder with new messages, I see both the tooltip for inbox account name and the new msg summaries. Would it be possible to see just the new msg summaries?
(In reply to comment #35)
> Would it be possible to see just the new msg summaries?

Sure, it would be possible and even quite easy (small logic change in mailWidgets.xml#folderSummary-popup) one minute update and I would be happy providing updated patch, but…

> But now when I hover over a folder with new messages,
> I see both the tooltip for inbox account name and the new msg summaries.

In my understanding that is the right thing to do. I don't see any reasons why the arguments for displaying location thing in general (the "don't remember" case) would not apply when there are new messages in the folder… why are you asking for this?
(In reply to comment #36)

> In my understanding that is the right thing to do. I don't see any reasons
> why the arguments for displaying location thing in general (the "don't
> remember" case) would not apply when there are new messages in the folder…
> why are you asking for this?

I see your point. It looked a little odd to me, but it's really a question for Blake. I'll just go make sure the mozmill test works and note my r+.
Comment on attachment 539837 [details] [diff] [review]
patch v7

thx for the patch, r=me, with a bunch of nits:

don't need braces here: 

+          if (!uniqueNames[item._folder.abbreviatedName]) {
+            uniqueNames[item._folder.abbreviatedName] = 0;
+          }

or here:
+        for each (let item in faves) {
+          item.addServerName = (uniqueNames[item._folder.abbreviatedName] > 1) ? true : false;
         }

this:

+          if (addLocation || newMessages) {
+            return true
+          } else {
+            return false
+          }

can be just

return addLocation || newMessages;

need ';' at the end of the return statements here:

+              return true
+            }
+
+            return false

If you could attach a patch with the nits addressed, that would be much appreciated.

I'm having a bit of a problem with the mozmill test, but it could very easily be my machine. I've requested a try server build with your patch to see if the mozmill tests pass there.
Attachment #539837 - Flags: review?(dbienvenu) → review+
(In reply to comment #34)
> http://build.mozillamessaging.com/tinderboxpushlog/
> ?tree=ThunderbirdTry&rev=90437ef573f9
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> splewako@aviary.pl-90437ef573f9/

(In reply to comment #38)
> I'm having a bit of a problem with the mozmill test, but it could very
> easily be my machine. I've requested a try server build with your patch to
> see if the mozmill tests pass there.

? :-)

(In reply to comment #37)
> (In reply to comment #36)
> > In my understanding that is the right thing to do. I don't see any reasons
> > why the arguments for displaying location thing in general (the "don't
> > remember" case) would not apply when there are new messages in the folder…
> > why are you asking for this?
> 
> I see your point. It looked a little odd to me, but it's really a question
> for Blake.

Exactly!
Attached patch patch v8 (obsolete) — Splinter Review
Patch v7 with nits from #c38 addressed
(In reply to comment #39)
> > > In my understanding that is the right thing to do. I don't see any reasons
> > > why the arguments for displaying location thing in general (the "don't
> > > remember" case) would not apply when there are new messages in the folder…
> > > why are you asking for this?
> > I see your point. It looked a little odd to me, but it's really a question
> > for Blake.
> Exactly!

I've thought about it a little, and posted it on <http://breakingtheegg.tumblr.com/post/6629828018/so-if-there-are-new-messages-should-we-still>, and I've come to the conclusion that I think we should show the whole location thing (because it's still useful data), but it should be below the new message summaries (since those are more likely what the user is interested in).

Hopefully that doesn't totally mess up your patch.  :)

Thanks,
Blake.
Summary: display account names in favorite folders pane view only when there are duplicated folder names → display account names in favorite folders pane view only when there are duplicated folder names and location info in the tooltip
Attached patch patch v9Splinter Review
v8 + mainly changes requested in comment 41

http://hg.mozilla.org/try-comm-central/pushloghtml?changeset=c0652f2b053a
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=c0652f2b053a
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/splewako@aviary.pl-c0652f2b053a/
Attachment #530922 - Attachment is obsolete: true
Attachment #530923 - Attachment is obsolete: true
Attachment #532918 - Attachment is obsolete: true
Attachment #539837 - Attachment is obsolete: true
Attachment #540137 - Attachment is obsolete: true
Attachment #542183 - Flags: ui-review?(bwinton)
Attachment #542183 - Flags: review?(dbienvenu)
Attached image v9 screen
Comment on attachment 542183 [details] [diff] [review]
patch v9

thx, seems to work fine.
Attachment #542183 - Flags: review?(dbienvenu) → review+
Comment on attachment 542183 [details] [diff] [review]
patch v9

Sorry for not getting to this sooner.  Yeah, that's a little more info than I'ld ideally like, but I think it's good enough.  ui-r=me.

Thanks,
Blake.
Attachment #542183 - Flags: ui-review?(bwinton) → ui-review+
Keywords: checkin-needed
Thanks for all your work on this Stefan. Checked in:

http://hg.mozilla.org/comm-central/rev/1281fc89bac5
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Duplicate of this bug: 581740
As pointed out in IRC, I think we'll want this for all the views that don't have a hierarchy, so Unread and Recent should get the same code.

(But we should file a new bug for that, I think.)

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