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

VERIFIED FIXED in Thunderbird 7.0

Status

VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: stef, Assigned: stef)

Tracking

unspecified
Thunderbird 7.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
Created attachment 525990 [details] [diff] [review]
comm-central patch

Display account names only for folders with duplicated names in favorite folders folder pane view
Attachment #525990 - Flags: review?
Attachment #525990 - Flags: feedback?

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
(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.
(Assignee)

Comment 4

7 years ago
(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)

Comment 7

7 years ago
thx for the patch - I assume unames is uniqueNames?
(Assignee)

Comment 8

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

Yes.

Comment 9

7 years ago
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.

Updated

7 years ago
Attachment #525990 - Flags: ui-review?(bwinton)

Comment 10

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

Comment 11

7 years ago
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+
(Assignee)

Comment 12

7 years ago
Created attachment 528562 [details] [diff] [review]
patch v2

Patch with updated variable name (as per comment #c9 and #c11)
Attachment #525990 - Attachment is obsolete: true
Attachment #525990 - Flags: ui-review?(bwinton)
(Assignee)

Updated

7 years ago
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-
(Assignee)

Comment 14

7 years ago
Created attachment 529280 [details]
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…
Attachment #529280 - Flags: feedback?
Attachment #529280 - Flags: feedback? → feedback?(bugmail)
Attachment #529280 - Flags: feedback?(bugmail)
(Assignee)

Comment 15

7 years ago
Created attachment 530922 [details] [diff] [review]
patch v3

patch v2 + tooltip thing
Attachment #528562 - Attachment is obsolete: true
Attachment #530922 - Flags: ui-review?(bwinton)
Attachment #530922 - Flags: review?(dbienvenu)
(Assignee)

Comment 16

7 years ago
Created attachment 530923 [details]
patch v3 preview
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+

Updated

7 years ago
Attachment #530922 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 18

7 years ago
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+
(Assignee)

Comment 20

7 years ago
Created attachment 532918 [details] [diff] [review]
patch v4

patch v3 + mozmill test
Attachment #529280 - Attachment is obsolete: true
Attachment #532918 - Flags: review?(dbienvenu)

Comment 21

7 years ago
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

7 years ago
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+

Comment 23

7 years ago
(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.
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/3c71fa107bce

Thanks for the patch Stefan.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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 → ---
(Assignee)

Comment 27

7 years ago
Created attachment 537118 [details] [diff] [review]
patch v5

Patch with mozmill fix integrated (from #c25) and gFolderTreeView.mode as aTreeViewMode parameter for parseFolder.
Attachment #537118 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #537118 - Flags: review? → review?(dbienvenu)

Comment 29

7 years ago
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

7 years ago
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-
(Assignee)

Comment 31

7 years ago
Created attachment 539547 [details] [diff] [review]
patch v6

v6 moves location/name code part into its own binding separating it from message summaries
Attachment #539547 - Flags: review?(dbienvenu)

Comment 32

7 years ago
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
(Assignee)

Comment 33

7 years ago
Comment on attachment 539547 [details] [diff] [review]
patch v6

ouch… sorry.
Attachment #539547 - Flags: review?(dbienvenu)
(Assignee)

Comment 34

7 years ago
Created attachment 539837 [details] [diff] [review]
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/splewako@aviary.pl-90437ef573f9/
Attachment #537118 - Attachment is obsolete: true
Attachment #539547 - Attachment is obsolete: true
Attachment #539837 - Flags: review?(dbienvenu)

Comment 35

7 years ago
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?
(Assignee)

Comment 36

7 years ago
(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

7 years ago
(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

7 years ago
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+
(Assignee)

Comment 39

7 years ago
(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!
(Assignee)

Comment 40

7 years ago
Created attachment 540137 [details] [diff] [review]
patch v8

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

Updated

7 years ago
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
(Assignee)

Comment 43

7 years ago
Created attachment 542184 [details]
v9 screen

Comment 44

7 years ago
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+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Thanks for all your work on this Stefan. Checked in:

http://hg.mozilla.org/comm-central/rev/1281fc89bac5
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Status: RESOLVED → VERIFIED

Updated

7 years ago
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.