Closed Bug 1190609 Opened 4 years ago Closed 4 years ago

Remove confusing asterisk from subfolder message count

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 45.0

People

(Reporter: aleth, Assigned: aceman)

References

Details

Attachments

(5 files, 3 obsolete files)

It's basically impossible to guess what it means:
https://twitter.com/supersole/status/626106596175646720

And it's redundant with the > arrow to the left.
Was there another reason this was added in the first place?
Flags: needinfo?(acelists)
The asterisk was added because it was easy to forget that the number represented a summary of subfolders, and not a value for a particular folder. I've had that confusion in the past myself, seeing a number that seemed to conflict with the actual folder count, and assuming there was a bug somewhere.
It is not redundant with the > arrow (if you mean the folder expanding grippy) as that one is always there. The asterisk (*) is there only when the counts are accumulated from subfolders. That is controlled by a pref (mail.folderpane.sumSubfolders), not the arrow.

I can understand it can be confusing as users do not know what the asterisk means. But nobody proposed a better solution yet. I mean while keeping the feature of accumulating subfolders toggleable (at least by a pref). Also the asterisk is localizable.
Depends on: 464973
Flags: needinfo?(acelists)
Maybe it should simply be fixed so that it doesn't show an asterisk unless the total actually includes unread messages from one or more sub-folders?

As it stands now, even if there are no unread messages in any sub-folders, and one or more unread messages in the parent folder, it still shows the asterisk.

This is what is confusing to me.

If the asterisk only showed up if one or more of the unread messages actually resided in a sub-folder, it would make much more sense.
(In reply to :aceman from comment #3)
> It is not redundant with the > arrow (if you mean the folder expanding
> grippy) as that one is always there.

Why wouldn't it be more intuitive to 
1) include the subfolders in the count when the subfolders are collapsed (> arrow), but 
2) not to sum the count when the subfolders are shown (v arrow)?

Is the idea to provide a way to *also* include subfolders in the count when the subfolders are shown?
(In reply to Charles from comment #4)
> Maybe it should simply be fixed so that it doesn't show an asterisk unless
> the total actually includes unread messages from one or more sub-folders?
> 
> As it stands now, even if there are no unread messages in any sub-folders,
> and one or more unread messages in the parent folder, it still shows the
> asterisk.
> 
> This is what is confusing to me.

Ah, I can understand this specific case. Maybe we could fine-tune this.

> If the asterisk only showed up if one or more of the unread messages
> actually resided in a sub-folder, it would make much more sense.
The asterisk is not specifically about unread messages. It summarizes the numbers from subfolders in the respective column, so also folder size, message count and yes, also unread msg count, if you have that enabled.

(In reply to aleth [:aleth] from comment #5)
> Why wouldn't it be more intuitive to 
> 1) include the subfolders in the count when the subfolders are collapsed (>
> arrow), but 
> 2) not to sum the count when the subfolders are shown (v arrow)?

I think that is the current behaviour. Do you see something else?
> The asterisk is not specifically about unread messages. 

Right. In the absence an unread column count, bold is what communicates the existence of unread messages.
And *color* communicates new unseen messages.
(In reply to :aceman from comment #3)
> It is not redundant with the > arrow (if you mean the folder expanding
> grippy) as that one is always there. The asterisk (*) is there only when the
> counts are accumulated from subfolders. That is controlled by a pref
> (mail.folderpane.sumSubfolders), not the arrow.

Isn't that a hidden pref? If you don't change it, it's redundant with the arrow. If you do change it, then you're just disabling the feature entirely. There should never be a situation where some folders are summed recursively and others aren't. I'm not sure what the asterisk actually provides here.
The pref is not hidden, it is listed normally in the all-thunderbird.js file.

It is not redundant. The comment in the code itself explains it:
// Only show counts / total size of subtree if the pref is set,
// we are in "All folders" mode and this folder row is not expanded.

So how can these 3 conditions be the same as only the last one (row not expanded = arrow) ?
(In reply to :aceman from comment #6)
> The asterisk is not specifically about unread messages. It summarizes the
> numbers from subfolders in the respective column, so also folder size,
> message count and yes, also unread msg count, if you have that enabled.

Ah, I keep forgetting about the other coluns because I never ever use them. The only thing that makes sense - to me - to be able to see at a glance is message unread count. If I need more details, I drill down, and check the status bar.

> (In reply to aleth [:aleth] from comment #5)
>> Why wouldn't it be more intuitive to 
>> 1) include the subfolders in the count when the subfolders are collapsed (>
>> arrow), but 
>> 2) not to sum the count when the subfolders are shown (v arrow)?
> 
> I think that is the current behaviour. Do you see something else?

This is indeed what I see (no asterisk when sub-folders are expanded).
(In reply to :aceman from comment #9)
> It is not redundant. The comment in the code itself explains it:
> // Only show counts / total size of subtree if the pref is set,
> // we are in "All folders" mode and this folder row is not expanded.
> 
> So how can these 3 conditions be the same as only the last one (row not
> expanded = arrow) ?

Well, it is redundant in that it displays the asterisk even if there are no messages in any sub-folders that meet the criteria (ie, no unread messages in sub-folders, only in parent folder)...

So, I guess what is needed is a bit of tweaking to the behavior, so the asterisk only displays when there are messages in sub-folders that meet the criteria (based on what columns are enabled)...
Yes, that looks like a good proposal that I could implement. It looks to me it is the same as bug 1185417? If yes, I could implement it there and leave the bug here for any further ideas aleth may have. Implementing the tweak you propose still does not make the users know more what the asterisk actually means :)
Assignee: nobody → acelists
Depends on: 1185417
Yes, perfect - but dunno what to do to make it more clear to users...

Thanks!
(In reply to :aceman from comment #9)
> The pref is not hidden, it is listed normally in the all-thunderbird.js file.
> 
> It is not redundant. The comment in the code itself explains it:
> // Only show counts / total size of subtree if the pref is set,
> // we are in "All folders" mode and this folder row is not expanded.
> 
> So how can these 3 conditions be the same as only the last one (row not
> expanded = arrow) ?

I think what we were trying to get at is that *if* you have the pref set, a non-expanded row in All Folders will *always* come with an asterisk, so it's redundant in that it carries no information (it will show up on all such folders). Of course that will no longer be true if comment 12 happens...
(In reply to Charles from comment #13)
> Yes, perfect - but dunno what to do to make it more clear to users...

Would replacing "(*5)" with "(5 in subfolders)" be too long?
(Note the number is at the beginning, so if the string gets cut off that's not necessarily terrible.)
(In reply to aleth [:aleth] from comment #15)
> Would replacing "(*5)" with "(5 in subfolders)" be too long?

For me, yes, absolutely - please don't do that - unless it is an option.

But if you were going to do it that way, I'd do:

(5, 3 in sub-folders) indicating there are 5 total, 2 in the parent folder and 3 in one or more sub-folders).

But that is just plain ugly to me... ;)
(In reply to aleth [:aleth] from comment #15)
> Would replacing "(*5)" with "(5 in subfolders)" be too long?
> (Note the number is at the beginning, so if the string gets cut off that's
> not necessarily terrible.)

This would IMHO make the folder pane too wide. What about (5+), + for additional folders?
Or better:

5:2,3

or something like that...
(In reply to Charles from comment #18)
> Or better:
> 
> 5:2,3
> 
> or something like that...

IMHO "5:2,3" or maybe just "2,3" could be a good solution, but only for the unread count, and only for users who do not display the unread count in a column, but allow the unread count to trail the folder name.  To show two or three separate numbers in a column, for either unread or total or size, would (IMHO) make the columns too wide and would probably be confusing.
(In reply to aleth [:aleth] from comment #15)
> Would replacing "(*5)" with "(5 in subfolders)" be too long?
> (Note the number is at the beginning, so if the string gets cut off that's
> not necessarily terrible.)

Yes, that would be very ugly. Also the other proposals like 5:2,3 seem non-working as we need to preserve right aligning the numbers in the columns.

Maybe the last proposal of only doing this right after the folder name (no column) would work as the right aligning is not needed there.
Some ideas given these constraints:

1) How about dropping the asterisk and instead adding a verbose tooltip (for (5), "2 here plus 3 in subfolders" or similar). That way if the user is surprised at the value of the number, they get a detailed explanation should they want one, in a discoverable way.

2) (v5) where v is a downward arrow similar to the one in the tree, ideally in the same colour (grey), that expands the folder entry if clicked. Again, the idea is to make the meaning discoverable. (Possible problem: I'm not sure tree items allow this kind of styling.)
(In reply to :aceman from comment #20)
> Maybe the last proposal of only doing this right after the folder name (no
> column) would work as the right aligning is not needed there.

Actually, that is what I was talking about, for the Unread count - sorry for not being clear.

I just looked at this more closely, and I don't think this should even be attempted for the column data, only for the Unread count in parenthesis right after the folder name.
(In reply to aleth [:aleth] from comment #21)
> 1) How about dropping the asterisk and instead adding a verbose tooltip (for
> (5), "2 here plus 3 in subfolders" or similar).

I actually like this idea, but...

I would leave the asterisk once the issue with the logic is fixed (ie, there should be no asterisk if all of the messages the criteria in question are in the parent folder).

So, when the user sees an asterisk, they could mouse-over to getr a much more informative tooltip.
(In reply to Richard Marti (:Paenglab) from comment #17)
> This would IMHO make the folder pane too wide. What about (5+), + for
> additional folders?

The problem with 5+ is that it's commonly used for "5 and above", as in "age 5+".
Some options:

"Folder (5+3)", where 5 is the number of unread messages in Folder, and 3 is the number in subfolders.

"Folder (5)", where the text is underlined if there are unread messages in subfolders (similar to how a collapsed thread is underlined if there are unread replies).
I have attached two screenshots (posteingang1.png and posteingang2.png) that show a problem with the current implementation:

If you have an unread mail in a folder and no unread mails in the subfolder, the asterisk should not be rendered since the count is correct even if the folder view is collapsed.
(In reply to Sven Neuhaus from comment #28)
> If you have an unread mail in a folder and no unread mails in the subfolder,
> the asterisk should not be rendered since the count is correct even if the
> folder view is collapsed.

If you want this, bug 1185417 is for you.
Now bug 1185417 is fixed for 45, it would be nice to make the explanation discoverable by modifying the tooltip when the asterisk is present (to include something like "8 unread, of which 2 in subfolders").
Flags: needinfo?(acelists)
One last idea: In addition to the tooltip, to avoid confusion with multiplication and to be more reminiscent of collapsed subfolders, one could replace the asterisk (*4) with the triangular bullet, (‣4).
cc'ing Paenglab for UI input on comment 31 (unlike comment 30, I'm not sure if it's a worthwhile improvement or not).
Flags: needinfo?(richard.marti)
Can we be sure this triangular bullet is in all fonts? If yes, a triangular bullet directing down could be better to point to folders below the main folder.

If we are not sure it's better to stay on the asterisk.
Flags: needinfo?(richard.marti)
(In reply to Richard Marti (:Paenglab) from comment #33)
> Can we be sure this triangular bullet is in all fonts? If yes, a triangular
> bullet directing down could be better to point to folders below the main
> folder.

The asterisk only shows up when the subfolders are collapsed, so a triangular arrow pointing down wouldn't point at anything, if that's what you were thinking?
 
> If we are not sure it's better to stay on the asterisk.

U+2023 ‣ triangular bullet (HTML ‣) is in the "general punctuation" unicode block. 

▼ - U+25BC BLACK DOWN-POINTING TRIANGLE is in "Geometric shapes".
▾ - U+25BE SMALL BLACK DOWN-POINTING TRIANGLE as well.

Examples: (▾5) (▼5) (‣5) (*5)

I don't know how you find out how common support for these is.
(In reply to aleth [:aleth] from comment #34)
> (In reply to Richard Marti (:Paenglab) from comment #33)
> > Can we be sure this triangular bullet is in all fonts? If yes, a triangular
> > bullet directing down could be better to point to folders below the main
> > folder.
> 
> The asterisk only shows up when the subfolders are collapsed, so a
> triangular arrow pointing down wouldn't point at anything, if that's what
> you were thinking?

Pointing was wrong wording, I hope "a symbol for subfolders" is better.

> > If we are not sure it's better to stay on the asterisk.
> 
> U+2023 ‣ triangular bullet (HTML ‣) is in the "general punctuation"
> unicode block. 
> 
> ▼ - U+25BC BLACK DOWN-POINTING TRIANGLE is in "Geometric shapes".
> ▾ - U+25BE SMALL BLACK DOWN-POINTING TRIANGLE as well.
> 
> Examples: (▾5) (▼5) (‣5) (*5)

(▾5) would be my preferred.

> I don't know how you find out how common support for these is.

That could be the problem when we are using this and then on some systems, like maybe older XP, this isn't shown correctly.
(In reply to Richard Marti (:Paenglab) from comment #35)
> That could be the problem when we are using this and then on some systems,
> like maybe older XP, this isn't shown correctly.

Windows XP's Arial indeed doesn't seem to have it, while Windows Vista and newer are OK:
http://www.microsoft.com/typography/fonts/font.aspx?FMID=1120
I'm pretty sure Gecko is smart enough to fall back to alternate fonts if the current one doesn't have the symbol you need.
(In reply to Richard Marti (:Paenglab) from comment #35)
> That could be the problem when we are using this and then on some systems,
> like maybe older XP, this isn't shown correctly.

At this point in time, with the already know limited resources Thunderbird development will have, I think we can simply not worry about supporting ancient/unsupported/end-of-life operating systems, don't you?
I do care for XP for now as I use/watch several such machines (which can't be upgraded, only replaced).

I copied the ▾ character from comment 35 and in DOM Inspector I put it into the "label" attribute of the "Write" toolbar button. It worked fine, albeit it is a but ugly in that font. Uglier than in the post here.

I notice there is a nice down arrow behind the "Get messages" button, called the drop marker. Is that an icon (image) or a character? Could we use that one in the folder tree?
Flags: needinfo?(acelists)
(In reply to :aceman from comment #39)
> I notice there is a nice down arrow behind the "Get messages" button, called
> the drop marker. Is that an icon (image) or a character? Could we use that
> one in the folder tree?

It's a image: https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/windows/global/arrow/arrow-dn.gif
(In reply to Charles from comment #38)
> > like maybe older XP, this isn't shown correctly.
> 
> At this point in time, with the already know limited resources Thunderbird
> development will have, I think we can simply not worry about supporting
> ancient/unsupported/end-of-life operating systems, don't you?

As far as I know, Firefox ESR 45 will still support XP, and so TB 45 will do the same.
Instead of the different characters that are being discussed, how 
about using an underline and/or a different color to indicate that a 
tooltip is available to provide more information?  This would follow 
a well-established convention.  It would have the additional small 
advantage of saving a little horizontal space (which is more important 
for folder pane columns than for the unread count in parentheses after 
the folder name).
As the improved tooltip will require strings, this should ideally be done soon if it is to be in 45.
Attached patch patch (obsolete) — Splinter Review
Would this generic message be enough? Or is it really important to show the real numbers for the specific cell inside the tooltip message?

The tooltip already shown only if you hover a cell containing the summarizing arrow. That is in contrast to the other tooltip contents that show on the whole row. I fixed that for the ellipsed folder name to only show if the name itself is hovered. I'm not sure if this should also be done for the new messages previews (whether they should only be shown if the folder name is hovered).

I tried the arrow symbol on Linux (KDE3) and Win XP and it works. Somebody should please try Win 7 and OS X.
Attachment #8697776 - Flags: ui-review?(richard.marti)
Attachment #8697776 - Flags: feedback?(aleth)
Comment on attachment 8697776 [details] [diff] [review]
patch

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

::: mail/base/content/folderPane.js
@@ +2223,5 @@
>          let unread = this._folder.getNumUnread(false);
>          let totalUnread = gFolderStatsHelpers.sumSubfolders ?
>                            this._folder.getNumUnread(true) : unread;
> +        subfoldersContributed = (unread != totalUnread);
> +        this._summarized.set(aColName, subfoldersContributed);

So in order to include the numbers in the tooltip, you'd have to do something like this instead?

this._count.set(aColName, [unread, totalUnread])

Is that a big complication?

::: mail/base/content/mailWidgets.xml
@@ +2419,5 @@
>            gFolderTreeView._tree.getCellAt(event.clientX, event.clientY, row, col, {});
> +          if (col.value.id == "folderNameCol") {
> +            let cropped = gFolderTreeView._tree.isCellCropped(row.value, col.value);
> +            if (tooltipnode.addLocationInfo(msgFolder, cropped))
> +              return true;

You probably don't want to return yet - it's possible you want to call addSummarized as well?

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +600,5 @@
>  <!ENTITY folderNameColumn.label "Name">
>  <!ENTITY folderUnreadColumn.label "Unread">
>  <!ENTITY folderTotalColumn.label "Total">
>  <!ENTITY folderSizeColumn.label "Size">
> +<!ENTITY subfoldersExplanation.label "Some of the items counted in this total number are located in subfolders of this folder">

This is a very long string for a tooltip. The advantage of including the numbers would also be that it makes it much shorter:
"11 unread, of which 6 in subfolders" or even
"11 unread messages, of which 6 are in subfolders"
(In reply to aleth [:aleth] from comment #45)
> So in order to include the numbers in the tooltip, you'd have to do
> something like this instead?
> 
> this._count.set(aColName, [unread, totalUnread])
> 
> Is that a big complication?
And then get the values for the tooltip. Surely it could be done, I just feel the feature already has quite some code attached, that must be run very often (when redrawing cells in folder pane). So I just ask if we want it :)

> ::: mail/base/content/mailWidgets.xml
> @@ +2419,5 @@
> >            gFolderTreeView._tree.getCellAt(event.clientX, event.clientY, row, col, {});
> > +          if (col.value.id == "folderNameCol") {
> > +            let cropped = gFolderTreeView._tree.isCellCropped(row.value, col.value);
> > +            if (tooltipnode.addLocationInfo(msgFolder, cropped))
> > +              return true;
> 
> You probably don't want to return yet - it's possible you want to call
> addSummarized as well?

The current tooltip logic is that the first highest priority text wins. There is currently a text for new messages, then for full folder name (if it is shown ellipsed (cropped)), then for the summarizing prefix.

Do we want to change that logic now and accumulate tooltip texts? If we change it that for the counts the first 2 texts are not shown, problem is solved there. But for the name column, there you can have all the 3 texts (when the unread count is attached to folder name). 

> > +<!ENTITY subfoldersExplanation.label "Some of the items counted in this total number are located in subfolders of this folder">
> 
> This is a very long string for a tooltip. The advantage of including the
> numbers would also be that it makes it much shorter:
> "11 unread, of which 6 in subfolders" or even
> "11 unread messages, of which 6 are in subfolders"

Yeah :)
These all seem like UI-r decisions to me ;)

(In reply to :aceman from comment #47)
> (In reply to aleth [:aleth] from comment #45)
> And then get the values for the tooltip. Surely it could be done, I just
> feel the feature already has quite some code attached, that must be run very
> often (when redrawing cells in folder pane). So I just ask if we want it :)

Performance-wise, you're already calculating these numbers, I don't think storing them makes much difference. (I haven't looked at whether the code could be more efficient about how/when it recalculates. How many folders do you think there are, typically? Is it worth worrying about?)
Attached image Win10SubfolderCount.png
How it looks on Win10.

I think too the actual tooltiptext is too long. Aleth's proposal would be a lot better, if doable.
(In reply to :aceman from comment #47)
> The current tooltip logic is that the first highest priority text wins.
> There is currently a text for new messages, then for full folder name (if it
> is shown ellipsed (cropped)), then for the summarizing prefix.
> 
> Do we want to change that logic now and accumulate tooltip texts? If we
> change it that for the counts the first 2 texts are not shown, problem is
> solved there. But for the name column, there you can have all the 3 texts
> (when the unread count is attached to folder name). 

I don't know this well enough to have an opinion about it. 

When the unread count is included in the folder name, some combination like "Foldername (11 unread, of which 6 in subfolders)" might work as well, depending on where the unread count is attached to the name I suppose.
(In reply to aleth [:aleth] from comment #50)
> (In reply to :aceman from comment #47)
> > The current tooltip logic is that the first highest priority text wins.
> > There is currently a text for new messages, then for full folder name (if it
> > is shown ellipsed (cropped)), then for the summarizing prefix.
> > 
> > Do we want to change that logic now and accumulate tooltip texts? If we
> > change it that for the counts the first 2 texts are not shown, problem is
> > solved there. But for the name column, there you can have all the 3 texts
> > (when the unread count is attached to folder name). 
> 
> I don't know this well enough to have an opinion about it. 
> 
> When the unread count is included in the folder name, some combination like
> "Foldername (11 unread, of which 6 in subfolders)" might work as well,
> depending on where the unread count is attached to the name I suppose.

This could become complicated with 3 texts to concatenate. I suggest we take it to a new bug.
Attached patch patch v2 (obsolete) — Splinter Review
Better?
Attachment #8697776 - Attachment is obsolete: true
Attachment #8697776 - Flags: ui-review?(richard.marti)
Attachment #8697776 - Flags: feedback?(aleth)
Attachment #8697789 - Flags: ui-review?(richard.marti)
Attachment #8697789 - Flags: feedback?(aleth)
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Comment on attachment 8697789 [details] [diff] [review]
patch v2

Yes, better.

I propose "2 unread messages in this folder, 4 in subfolders". Then it's clearer.
Attachment #8697789 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8697789 [details] [diff] [review]
patch v2

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

Thanks for looking at this for 45!

I don't know the surrounding code or the various circumstances in which this tooltip is filled well enough to comment further ;)
Attachment #8697789 - Flags: feedback?(aleth)
(In reply to Richard Marti (:Paenglab) from comment #53)
> I propose "2 unread messages in this folder, 4 in subfolders". Then it's
> clearer.

If you want the "unread" word in the string, then we would need to have 3 different strings (as each column has different contents). I intentionally did the string universal to avoid that.
(In reply to :aceman from comment #55)
> If you want the "unread" word in the string, then we would need to have 3
> different strings (as each column has different contents). I intentionally
> did the string universal to avoid that.

When it's too complicated, then it's okay like it is now.
Comment on attachment 8697789 [details] [diff] [review]
patch v2

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

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +514,5 @@
>  ## This string shows an indication that the value shown is actually a summary
>  ## accumulated from all subfolders.
>  ## %S = summarized value from all subfolders
> +folderSummarizedSymbolValue=▾%S
> +subfoldersExplanation=%1$S in this folder, %2$S in subfolders

Maybe add a l10n note saying that this string is for the tooltip, and list the kind of items it is used for, so the translators can also translate it "universally".
(In reply to Richard Marti (:Paenglab) from comment #56)
> (In reply to :aceman from comment #55)
> > If you want the "unread" word in the string, then we would need to have 3
> > different strings (as each column has different contents). I intentionally
> > did the string universal to avoid that.
> 
> When it's too complicated, then it's okay like it is now.

It's not that it would be too complicated, but I think it is overkill :)
Attached patch patch v2.1 (obsolete) — Splinter Review
Attachment #8697789 - Attachment is obsolete: true
Attachment #8697810 - Flags: review?(mkmelin+mozilla)
I have tested in TB45a1 and it is fixed.
Thanks for testing the try build Marco. For others, the patch is not in a proper nightly yet.
Comment on attachment 8697810 [details] [diff] [review]
patch v2.1

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

Seems to work fine. r=mkmelin with the changes below

::: mail/base/content/folderPane.js
@@ +2181,5 @@
>    this._folderFilter = aFolderFilter;
>    this._favicon = null;
> +  // Contains message counts for each folder column.
> +  // Values are of the format "[value_for_folder, total_value_with_subfolders]".
> +  this._summarizedCounts = new Map();

A Map looks like an odd choice for this as the values have no real connection. Just use a plain object, or even a pair.

@@ +2884,5 @@
> +     *
> +     * @param aSize  The size in bytes to format.
> +     * @param aUnit  Optional unit to use for the format.
> +     *               Possible values are "KB" or "MB".
> +     */

document @return
Attachment #8697810 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8697810 [details] [diff] [review]
patch v2.1

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

::: mail/base/content/folderPane.js
@@ +2181,5 @@
>    this._folderFilter = aFolderFilter;
>    this._favicon = null;
> +  // Contains message counts for each folder column.
> +  // Values are of the format "[value_for_folder, total_value_with_subfolders]".
> +  this._summarizedCounts = new Map();

A Map isn't a bad choice when you have no control over the values of the keys. No need to worry about hasOwnProperty or folder names like "__proto__" ;)
There's no name to worry about, it's just two basically unrelated numbers. 
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map: "If in contrast you have a fixed amount of keys, operate on them individually, and distinguish between their usage, then you want an object."
Well, there was a version of the patch that iterated over all the keys to get the final result :)
Also, we may add new columns or an extension now could, so there is not a fixed amount of keys.

The keys are column names, so none of them will hopefully be called "__proto__" :)
Oh right, I misread that. What you should do though is to add a note that the keys are column name
Attached patch patch v2.2Splinter Review
OK, done, thanks.
Attachment #8697810 - Attachment is obsolete: true
Attachment #8697885 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/94ea7534162c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
You need to log in before you can comment on or make changes to this bug.