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

Status: VERIFIED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Stefan Plewako [:stef]]

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.

Comment 1

[Stefan Plewako [:stef]]

Attachment: comm-central patch

Display account names only for folders with duplicated names in favorite folders folder pane view

Comment 2

[David :Bienvenu]

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.

Comment 3

[Stefan Plewako [:stef]]

(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.

Comment 4

[Stefan Plewako [:stef]]

(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 5

[Ludovic Hirlimann [:Usul]]

Comment on attachment 525990 [details] [diff] [review]
comm-central patch

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

Thank for the patch.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 7

[David :Bienvenu]

thx for the patch - I assume unames is uniqueNames?

Comment 8

[Stefan Plewako [:stef]]

(In reply to comment #7)
> I assume unames is uniqueNames?

Yes.

Comment 9

[David :Bienvenu]

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.

Comment 10

[David :Bienvenu]

I did try running with the patch, and it seems to work fine.

Comment 11

[David :Bienvenu]

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.

Comment 12

[Stefan Plewako [:stef]]

Attachment: patch v2

Patch with updated variable name (as per comment #c9 and #c11)

Comment 13

[Blake Winton (: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.

Comment 14

[Stefan Plewako [:stef]]

Attachment: mozmill test, wip

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…

Comment 15

[Stefan Plewako [:stef]]

Attachment: patch v3

patch v2 + tooltip thing

Comment 16

[Stefan Plewako [:stef]]

Attachment: patch v3 preview

Comment 17

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 18

[Stefan Plewako [:stef]]

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.

Comment 19

[Rain [:rain1] (inactive)]

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 - [e-mail]");
>  assert_equals(mc.window.gFolderTreeView.getFTVItemForIndex(2).text,
>                "duplicatedName - [e-mail]");

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.

Comment 20

[Stefan Plewako [:stef]]

Attachment: patch v4

patch v3 + mozmill test

Comment 21

[David :Bienvenu]

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 22

[David :Bienvenu]

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!

Comment 23

[Jim Porter (:squib)]

(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.

Comment 24

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/3c71fa107bce

Thanks for the patch Stefan.

Comment 25

[Mark Banner (:standard8)]

I had to land a bustage fix for the mozmill tests:

http://hg.mozilla.org/comm-central/rev/24639f45f5c1

Comment 26

[Mark Banner (:standard8)]

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

Comment 27

[Stefan Plewako [:stef]]

Attachment: patch v5

Patch with mozmill fix integrated (from #c25) and gFolderTreeView.mode as aTreeViewMode parameter for parseFolder.

Comment 28

[Stefan Plewako [:stef]]

Try build with patch v5 tinderboxpushlog:
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=346d8ad2095a

Packages and logs:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-346d8ad2095a/

Comment 29

[David :Bienvenu]

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 30

[David :Bienvenu]

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.

Comment 31

[Stefan Plewako [:stef]]

Attachment: patch v6

v6 moves location/name code part into its own binding separating it from message summaries

Comment 32

[David :Bienvenu]

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 33

[Stefan Plewako [:stef]]

Comment on attachment 539547 [details] [diff] [review]
patch v6

ouch… sorry.

Comment 34

[Stefan Plewako [:stef]]

Attachment: patch v7

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/[e-mail]-90437ef573f9/

Comment 35

[David :Bienvenu]

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?

Comment 36

[Stefan Plewako [:stef]]

(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?

Comment 37

[David :Bienvenu]

(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 38

[David :Bienvenu]

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.

Comment 39

[Stefan Plewako [:stef]]

(In reply to comment #34)
> http://build.mozillamessaging.com/tinderboxpushlog/
> ?tree=ThunderbirdTry&rev=90437ef573f9
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> [e-mail]-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!

Comment 40

[Stefan Plewako [:stef]]

Attachment: patch v8

Patch v7 with nits from #c38 addressed

Comment 41

[Blake Winton (:bwinton) (:☕️)]

(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.

Comment 42

[Stefan Plewako [:stef]]

Attachment: patch v9

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/[e-mail]-c0652f2b053a/

Comment 43

[Stefan Plewako [:stef]]

Attachment: v9 screen

Comment 44

[David :Bienvenu]

Comment on attachment 542183 [details] [diff] [review]
patch v9

thx, seems to work fine.

Comment 45

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 46

[Mark Banner (:standard8)]

Thanks for all your work on this Stefan. Checked in:

http://hg.mozilla.org/comm-central/rev/1281fc89bac5

Comment 48

[Blake Winton (:bwinton) (:☕️)]

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.