1185417 - Wrong logic on asterisks for new messages

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Konstantin Svist]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150708164734

Steps to reproduce:

Upgraded Thunderbird


Actual results:

If a folder has unread messages and has subfolders and subfolders are not visible, count of unread messages is displayed with an asterisk (*)
This wrongly applies to new messages in the visible folder only; when expanded to view inner folders, count stays the same but asterisk disappears

e.g.:

> Inbox (*123)
and
v Inbox (123)
      Foo



Expected results:

Asterisk indicator would make a lot more sense if it were used for unread-in-inner-folder condition only.

e.g.:

> Inbox (123)
and
v Inbox (123)
      Foo
      Bar


Note, this case is already correct:
> Inbox (*123)
and
v Inbox (120)
      Foo (3)

Comment 1

[:aceman]

This looks like what bug 1190609 is also proposing but this one is clearly defined.

Comment 2

[:aceman]

Attachment: patch

This should implement the requested feature.

There is some more support for this feature in bug 1190609 so I have CCed the interested people here.

aleth, can you try this out?

Comment 3

[Charles]

Thanks aceman!

Fyi, the same logic bug exists for the asterisk with respect to the column number indicators.

Ie, a parent folder with a size of 10MB, and one or more sub folders that are empty, the asterisk still displays when the parent is collapsed.

Comment 4

[Charles]

Sorry, forgot to ask - should I file a new bug for this?

Comment 5

[:aceman]

No, I have implemented the logic it for all 4 values (3 columns + unread count tacked on folder name).

Comment 6

[Charles]

awesome, thx!

Comment 7

[aleth [:aleth]]

Comment on attachment 8643995 [details] [diff] [review]
patch

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

Looks OK to me. Charles is probably a better person to test this, as he seems to have a profile with use cases ;)

::: mail/base/content/folderPane.js
@@ +2224,5 @@
>          if (!document.getElementById("folderUnreadCol").hidden)
>            return text;
>  
> +        let totalUnread = this._folder.getNumUnread(gFolderStatsHelpers.sumSubfolders);
> +        let unread = this._folder.getNumUnread(false);

Might be easier to change getNumUnread so that it returns both.

@@ +2832,4 @@
>      },
>  
>      /**
>       * Recursively get the size of specified folder and all its subfolders.

The latter only happens if this.sumSubFolders is true. Should be added to the comment.

Though the code might be simpler to read if you had

getFolderSize()  (Doesn't include subfolders)
getTotalFolderSize() (Does include subfolders, calls getTotalFolderSize recursively if there are subfolders and getFolderSize if there are no subfolders. Can still return an array.)

@@ +2843,4 @@
>        try {
> +        folderSize = aFolder.sizeOnDisk;
> +        if (folderSize < 0)
> +          return [this.kUnknownSize, null];

Is [this.kUnknownSize, this.kUnknownSize] more consistent?

Comment 8

[Charles]

So... I'd be happy to provide feedback, but is there a build available I can install? I'm not a dev and not sure how to go about applying this patch...

Comment 9

[:aceman]

Aleth, can you push it to try server to get the try build?

Comment 10

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26944316616b

Comment 11

[aleth [:aleth]]

Once completed, builds and logs will be available at:
https://ftp-ssl.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-fd215cd1a275

Comment 13

[:aceman]

Somehow I can't see if the push job did succeed. But there are binaries created in the mentioned folder. Charles, can you try out the builds?

Comment 14

[aleth [:aleth]]

These builds failed, c-c is busted atm.

Comment 15

[aleth [:aleth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da503db032b5

Comment 16

[aleth [:aleth]]

Builds here on success https://ftp-ssl.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-89deafcb2e44

Comment 17

[:aceman]

aleth, why do the 2 links have different hashes? The treeherder link does not show any state of the push. However the ftp folder does contain some builds. Charles, can you try the fix in the builds?

Comment 18

[:aceman]

Attachment: patch v2

Addressed aleth's notes.

Comment 19

[Charles]

My apologies, been extremely busy at $dayjob...

If there is a new try build available with the new patch, I can test it out today...

Comment 20

[:aceman]

There should be no functional changes in the new patch, you can use the existing build (comment 11).

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 8666390 [details] [diff] [review]
patch v2

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

LGTM, r=mkmelin

Comment 22

[:aceman]

https://hg.mozilla.org/comm-central/rev/f70c0c96cc07