528508 - In folder tabs, indicate unread count in tab title

Status: ASSIGNED

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[David Ascher (:davida)]

I like to have one tab per folder that I check, and then collapse the folder pane.  This is fairly easy to setup, and folder persistence works well.  

The only problem is that the tab title doesn't have (N) indicating unread messages in that tab/folder.

I suspect there's some notification we just need to hook a tab title update to.

Comment 2

[mikey.osd600]

Attachment: This patch provides the functionality in concatenating the opened folder tabs' titles with their number of unread messages.

I tried making it so that if a folder has a new number of unread messages, all tabs displaying this folder would be updated with the new number as well. This was done by adding a new function, refreshTabTitles(), in the tabmail.xml - which has similar code as setTabTitle(). This refreshTabTitles() is used with an event listener whenever the user moves their mouse.

Comment 3

[Rain [:rain1] (inactive)]

A quick drive-by review -- since tabmail.xml works with all sorts of tabs, it doesn't make sense to have any sort of logic for this or an extra parameter there. Any logic for unread messages needs to be part of the folder tab type's onTitleChanged function in mailTabs.js.

Comment 4

[Rain [:rain1] (inactive)]

Also, please use let instead of var, and why would tab.title and oldTabTitle be out of sync?

Comment 5

[Ludovic Hirlimann [:Usul]]

(In reply to comment #2)
> Created an attachment (id=417622) [details]
> This patch provides the functionality in concatenating the opened folder tabs'
> titles with their number of unread messages.
> 
You'll need to ask for review when you feel your patch is ready see https://developer.mozilla.org/en/comm-central#Requirements .

Comment 6

[mikey.osd600]

Thanks for the advices. I am just wondering, but am I correct when getting the number of unread messages by using the status bar panel's unreadMessageCount or should I think of another approach?

Comment 7

[David :Bienvenu]

You should get them from the nsIMsgFolder object that corresponds to the folder being displayed in the tab.

Comment 8

[mikey.osd600]

Attachment: I added some additional code to the onTitleChanged function to add the number of unread messages, but as well, in the commandglue.js' UpdateStatusMessageCounts function for when a folder is changed.

Comment 9

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

Comment on attachment 418162 [details] [diff] [review]
I added some additional code to the onTitleChanged function to add the number of unread messages, but as well, in the commandglue.js' UpdateStatusMessageCounts function for when a folder is changed.

(review was requested via irc)

Thank you for working on this!

Instead of modifying UpdateStatusMessageCounts in commandglue.js, I would suggest modifying FolderDisplayWidget.  UpdateStatusMessageCounts is legacy code to deal with the status bar, and FolderDisplayWidget only tells it about events when that FolderDisplayWidget is on the active tab.  This means that you would not receive events for background tabs, which would be sad.

You can find FolderDisplayWidget in this file:
http://mxr.mozilla.org/comm-central/source/mail/base/content/folderDisplay.js

If you search for UpdateStatusMessageCounts in that file, you will find that it gets called in two places:

1) When the onMessageCountsChange notification occurs.  This case is important.

2) In _updateContextDisplay which is called by makeActive.  makeActive is called whenever the tab is switched to or whenever the folder is changed.  You can ignore this case for the former because whether the tab is active or not is irrelevant to you (but the status bar does care).  You can ignore this case for the folder change case because we will already be changing the tab title in that situation and your tab titling code will already be handling that.

I would just copy the existing case where FolderDisplayWidget calls out to setTabTitle into the message counts change situation, making sure that you always do it, regardless of whether the thing is active.

I would also suggest that in the mailTabs logic in addition to modifying the title that you also do something with a DOM attribute.  By exposing whether there are new messages or not using setAttribute.  That way the tabs can be styled using CSS to change their color or have a star on them or something.  I would suggest exposing a boolean via an attribute name of "HasNewMessages" or something like that.  setAttribute coerces the second arg to a string automatically.

Comment 10

[Chris Pepper]

I think bug 488551 is a duplicate of this one.

Comment 12

[Wayne Mery (:wsmwk)]

mikey seems to be gone

Comment 13

[rossi]

taking

Comment 14

[Wayne Mery (:wsmwk)]

I don't know that this is true, but if tab title isn't always reflected in window title automatically, including message count, it might be nice to do so.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

rossi, still taking this?

Comment 16

[rossi]

yes. sorry for the delay, holidays, mercurial newbie, lost code, lots of other work. i have sort of reconstructed everything but trying to do some more stuff, like setting an attribute for new and one for unread messages, clearing new messages on tab switch and stuff like that...

i'll attach the whole thing at the end of the week.

Comment 18

[Thomas D. (:thomas8)]

Rossi, you can almost still make it "at the end of the week" plus one year to provide the patch :)
Are you trying again, or you want yourself unassigned from this bug?

(In reply to rossi from comment #16)
> yes. sorry for the delay...
> i'll attach the whole thing at the end of the week.

Comment 19

[Thomas D. (:thomas8)]

[patchlove] sounds fine, but is this little thing helpful enough for many users to deserve [UXprio], compared over other urgent bug fixes and UX improvements?

Comment 20

[Wayne Mery (:wsmwk)]

It has always surprised me how many users care about message counts, so my guess would be this bug should rank pretty high in both the tabs and the general paper cuts categories

Comment 21

[Thomas D. (:thomas8)]

Rossi, you're always welcome to continue this work, but you seem to be absent (no reply to comment 18). Until such time, let's indicate that this isn't currently attended.
If you have any work-in-progress patches you can attach here, that might help, too.

Comment 22

[skysailor]

I have noticed a small yellow star appears in the SUBfolder now when there are new messages.  This is ok, but my primary complaint was that there is no way to know if there are new messages if the MAIN FOLDERS are collapsed.  They must still be expanded to know if new mail is present.

Originally the subfolders would be bold if there was new mail.  Currently, they are now bold with a star, so you see the star in the subfolder does not add any functionality, it only duplicates the new mail indication.  Is it possible to move the star to the primary folder?  That way if the folders are collapsed one will know there is new mail with a glance instead of having to uncollapse every folder.

Comment 23

[:aceman]

I think there is a difference between the star and the bold folder names.

Star means NEW messages. Bold folder name means UNREAD messages. Yes, when new messages arrive they are both NEW and UNREAD. But once you read them, you can then toggle the UNREAD state with 'm' and while the folders will be bold, you do not get the star to reappear (as they are not new now).

Comment 24

[Wayne Mery (:wsmwk)]

actually, iirc it's a starburst :) ... distinct from a star

Comment 25

[skysailor]

Thanks for the clarification.  But do you understand the point? When the FOLDERS are collapsed it is impossible to know if there is new mail or unread mail.  The FOLDER is always bold without a "starburst". I am not trying to annoy you, I assumed that information from a daily users point of view would be welcomed and helpful in creating a better product.  Do what you will with the info.

Comment 26

[:aceman]

Yes, I understand that part. It just does not belong into this bug. Can you find a better one or file a new one for your request?

Comment 27

[Hessam Salehi]

Attachment: 528508-v1.patch

I created the new patch and have made the changes as Andrew said.
But for exposing whether there are new messages or not is there any way
to bold tab title ?

Comment 28

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

Yes, as I mention in my last paragraph in comment 9, you can set an attribute on the tab node.  For example, in tabmail.js starting around line 441 we set a bunch of attributes:
http://dxr.mozilla.org/comm-central/source/suite/mailnews/tabmail.js?from=tabmail.js&case=true#441

If you were to add something like this to your existing patch's code:
  aTabNode.setAttribute("HasNewMessages", folder.getNumUnreadMessages() > 0);

Then you could also add a block like so to tabmail.css:
  .tabmail-tab[HasNewMessages="true"] {
    font-weight: bold;
  }

I think that would accomplish the desired effect.

Comment 29

[Hessam Salehi]

Attachment: V2

Comment 30

[Hessam Salehi]

I used .hasNewMessages instead of .getNumUnreadMessages
But after checking New Message the tab title font doesn't turn back to normal until checking the second message.

Comment 31

[Richard Marti (:Paenglab)]

Please use the tabmail.css in themes directories. The content CSS files aren't for styling.

Comment 32

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

Comment on attachment 8399140 [details] [diff] [review]
V2

- I'm not actually a Thunderbird reviewer anymore, so although I still know a good deal about how this stuff works because I originally refactored/wrote it, I can't speak to what the code owners will think is a good implementation, etc.  Module owners and reviewers can be found at https://wiki.mozilla.org/Modules/Thunderbird.  I'm changing the feedback request to Mike Conley since this is a 3-pane/standalone message view type of thing.  I believe there are other experienced developers in this area too, but they either aren't listed or I'm not sure how the work distribution goes.

- setAttribute("label") should not be called; tabmail's setTabTitle method should be used:
http://dxr.mozilla.org/comm-central/source/mail/base/content/tabmail.xml#95
http://dxr.mozilla.org/comm-central/source/mail/base/content/tabmail.xml#1170

- In onMessageCountsChanged I don't think you want to do a for-loop traversal of all the tabs and perform matching based on a regexp.  It's inefficient and sounds like it's asking for weird bugs in the future.  The FolderDisplayWidget instance already has a "_tabInfo" attribute on it.  This can be used with the tabmail instance's _getTabContextForTabbyThing which it uses internally:
http://dxr.mozilla.org/comm-central/source/mail/base/content/tabmail.xml#389

The method is prefixed with an underscore and the style of the name tells me I created it.  I assume the goal was to mediate access to the tabs through the API so that dealing with things that were impacted by the active tab (cursor) or involved styling that the tab itself should not be aware of (spinner/etc) would be hidden from the tabs.  I'm not sure the API needs to exercise that level of control over things that strictly relate to the tab-specific style of the tab.

I could make some guesses, but I think it's best for :mconley to make that determination, especially since he was involved in the Firefox Australis refactor and probably knows how Firefox is handling tabs right now.

- In my last comment, I actually linked to SeaMonkey's tabmail implementation!  (I'm new to using dxr with comm-central and forgot to check for that possibility and it's been long enough that I forgot tabmail was tabmail.xml.)  Sorry if I confused you by linking to that instead of tabmail.xml and for not recognizing the differences.

The notable bit there is that the SeaMonkey code already is setting a whole bunch of folder-based stylistic attributes there, including one called "NewMessages".  It's probably best to use the same attribute names as SeaMonkey unless there's a good reason not to like varying semantics.  It's probably not appropriate to put the setting logic in tabmail.xml like it is in tabmail.js; I understand tabmail.js to already be a shim-layer specific to mailnews tab-types since SeaMonkey already has another true tab-handling implementation for its non-mail types.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

via email:


> I'm working on Bug 528508 but I'm relatively new to Thunderbird and I have a problem.
> All Issues that mentioned on the last comment (32) have been resolved as Andrew said,
> But the current problem is after checking the all new messages the tab title font doesn't
> turn back to normal until checking the another *unread* message.
> Can you point somewhere that I can set the tab attribute to turn font back to normal
> immediately after checking the last new message ?

Comment 34

[Hessam Salehi]

Attachment: 528508-v3.patch

Since it seems no one has any idea about this issue, I updated the patch and fixed the issue with my own way. It seems works fine for me now.

* please assign the bug to me.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Sorry this has taken forever. I'm going to look at this today.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8402206 [details] [diff] [review]
528508-v3.patch

Review of attachment 8402206 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this took so long. I think this looks good, based on what I know about folder display code (which admittedly, isn't too too much).

Thanks!

::: mail/base/content/folderDisplay.js
@@ +1226,2 @@
>    /**
>     * Update the status bar to reflect our exciting message counts.

Please update this comment to mention that we're updating the tab title as well.

@@ +1230,3 @@
>      if (this.active)
>        UpdateStatusMessageCounts(this.displayedFolder);
> +    document.getElementById('tabmail').setTabTitle(this._tabInfo);

Double-quotes please.

::: mail/base/content/folderPane.js
@@ +1842,4 @@
>      let index = this.getIndexOfFolder(aItem);
>      if (index != null)
>        this._tree.invalidateRow(index);
> +    if (aProperty == "NewMessages" && aNew == false)

!aNew.

::: mail/base/content/mailTabs.js
@@ +257,4 @@
>          // The user may have changed folders, triggering our onTitleChanged
>          // callback.
>          let folder = aTab.folderDisplay.displayedFolder;
> +        let numUnreadMessages = (folder.getNumUnread(false) > 0 ? " (" + folder.getNumUnread(false) + ")" : "");

0 is already false-y, and Gecko has template strings now, so we can do this:

let title = folder.prettyName;
let numUnreadMessages = folder.getNumUnread(false);

if (numUnreadMessages) {
  title += `(${numUnreadMessages})`;
}

aTab.title = title;

::: mail/themes/windows/mail/tabmail.css
@@ +69,4 @@
>    -moz-image-region: rect(0px, 16px, 16px, 0px);
>  }
>  
> +.tabmail-tab[HasNewMessages="true"] {

To fit in with the rest of the attribute selectors in this file, let's lowercase and dash separate, with:

.tabmail-tab[has-new-messages="true"] {
}

Also, since this rule is common across the tabmail.css's, it can be put into the shared copy:

mail/themes/shared/mail/tabmail.css

just the one time.

Comment 37

[George Politis [:gp]]

This is mostly JS. Would it make sense to wrap it in an add-on? Also, now that TB has Chat support, do we care about showing the unread chat messages in the tab?

Comment 38

[Kevin Field]

I don't use chat yet, but I just started using multiple tabs since my folder list got too long.  I would love it if the show-count feature were implemented.