377507 - Account level folder (isServer-true) properties not being set, cannot show if closed folder has unread/new messages

Status: NEW

(Thunderbird :: Folder and Message Lists, defect)

Description

[alta88]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Tb2.0.0.0pre (20070411)


the property biffState is correctly set so that the css 
treechildren::-moz-tree-cell-text(isServer-true, closed, biffState-NewMail) {
	color: green !important}
works fine.  but the following properties are not set properly such that it is not possible to tell from a closed account level folder if there is new/unread mail:
newMessages
hasUnreadMessages
subfoldersHaveUnreadMessages

note that this works fine on any Account level folder's child folders and their children.


Reproducible: Always

Comment 1

[alta88]

see also bug 235956.

Comment 2

[:aceman]

So the code is like this:
  if (aFolder.isServer) {
    properties.push("isServer-true");
  }
  else
  {
    // We only set this if we're not a server
    let shallowUnread = aFolder.getNumUnread(false);
    if (shallowUnread > 0) {
      properties.push("hasUnreadMessages-true");
    }
    else
    {
      // Make sure that shallowUnread isn't negative
      shallowUnread = 0;
    }
    let deepUnread = aFolder.getNumUnread(true);
    if (deepUnread - shallowUnread > 0)
      properties.push("subfoldersHaveUnreadMessages-true");
  }

So it intentionally does not set *UnreadMessages-true for server rows.
Can you get UX agreement that this is actually wanted?

Comment 3

[alta88]

i can't think of a single even semi valid reason not to have this info on a closed account folder.  even if for some reason it isn't styled by default, the properties should still be set on the folder.  so it may be a different bug if/how a style should appear; this is about setting the properties.

Comment 4

[Lee_Dailey]

howdy y'all,

does this ...
235956 – add number of unread messages next to (collapsed) account name 
- https://bugzilla.mozilla.org/show_bug.cgi?id=235956

... mean that this bog bug can be closed?

the unread part seems to be covered. perhaps this otta be changed to remove the unread part?

take care,
lee

Comment 5

[alta88]

hi lee,

the counts are great, but no.  this is about properties being set on server folders.

Comment 6

[Lee_Dailey]

howdy alta88,

perhaps i should ask out-of-band ... but i don't see any difference. the account level folder in the folder tree shows up as isServer-true from what i can tell. that seems to be the exact thing that bug 235956 covered for unread msgs. this bug has the same unread msgs in it's title.

that's why i thot - if nothing else - that it might be best to remove the unread from the title of this bug. 

if i've got this wrong [again [*grin*]] please point me to the correct info.

take care,
lee

Comment 7

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

Given that we're not talking about the UI-facing portion of this change, I'm not sure why you need any info from me.  ;)  Just do it, as far as I'm concerned, and we can file followup bugs to fix the ui if we decide we don't like it.

Comment 8

[:aceman]

Attachment: patch

So this should set subfoldersHaveUnreadMessages on the server too.
hasUnreadMessages is questionable, as by the semantics on the normal folders, this is only set if the folder itself has unread messages. I think the root folder never has unread messages. newMessages is a folder attribute that also returns true only when the folder itself has new messages. And there is no 'deep' variant of this. Do we want to add a function for it similar to hasUnreadMessages() and create a subfoldersHaveNewMessages property for the (root)folder nodes in the tree?

Comment 9

[alta88]

(In reply to :aceman from comment #8)
> Created attachment 769485 [details] [diff] [review]
> patch

> this should set subfoldersHaveUnreadMessages on the server 

Yes.
> create a subfoldersHaveNewMessages property for the (root)folder nodes in the tree?

Yes.

Comment 10

[:aceman]

Is there anything to do here after bug 95193 ?
alta88, can you finish this?

Comment 11

[alta88]

Attachment: unread.patch

Comment 12

[alta88]

Attachment: unread.patch

rather than cargo cult old code that traversed each and every subfolder recursively to get a count when all that was wanted was a yes/no for the root ancestor, this patch uses the method in bug 95193 to exit on first hit.  so as not to add to the cpu-spike-on-mouseover 'feature' of folderpane.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 8377658 [details] [diff] [review]
unread.patch

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

::: mailnews/base/util/folderUtils.jsm
@@ +89,5 @@
> +    properties.push("hasUnreadMessages-true");
> +
> +  // A closed folder or any subfolder has unread messages. Currently, there is
> +  // no distinction between a folder having no unread and one of its subfolders
> +  // having unread; this can, however, be accomplished just with css if desired.

I would leave this "Currently..." out of the code as it may well change. Please after it lands, file a followup-bug to possibly differentiate the UI effect.

@@ +91,5 @@
> +  // A closed folder or any subfolder has unread messages. Currently, there is
> +  // no distinction between a folder having no unread and one of its subfolders
> +  // having unread; this can, however, be accomplished just with css if desired.
> +  if (!aOpen && aFolder.hasSubFolders && aFolder.getNumUnread(true) > 0)
> +    properties.push("subfoldersHaveUnreadMessages-true");

So, doesn't this make a folder with unread always have subfoldersHaveUnreadMessages-true if it has subfolders - but unread only in the parent? I don't think that's wanted.

Comment 14

[alta88]

Attachment: unread.patch

updated.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 8380730 [details] [diff] [review]
unread.patch

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

::: mailnews/base/util/folderUtils.jsm
@@ +91,5 @@
> +
> +  // A closed folder's subfolder has unread messages
> +  if (!aOpen && aFolder.hasSubFolders && !folderHasUnread &&
> +      aFolder.hasFolderOrSubfolderUnreadMessages)
> +    properties.push("subfoldersHaveUnreadMessages-true");

but you'd still not ever have hasUnreadMessages-true + subfoldersHaveUnreadMessages-true for a folder, even if that is true

Comment 16

[alta88]

yes, you would.

the prior patch set a property of subfoldersHaveUnreadMessages-true if either the folder itself or a subfolder has unread.

this patch sets subfoldersHaveUnreadMessages-true only if a subfolder has unread.

the folder itself always gets hasUnreadMessages-true (if it has unread) regardless.

Comment 17

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #16)
> this patch sets subfoldersHaveUnreadMessages-true only if a subfolder has unread.

AND "this" folder doesn't have unread, which was my point: at least conceptually it is wrong

Comment 18

[alta88]

(In reply to Magnus Melin from comment #17)
> (In reply to alta88 from comment #16)
> > this patch sets subfoldersHaveUnreadMessages-true only if a subfolder has unread.
> 
> AND "this" folder doesn't have unread, which was my point: at least
> conceptually it is wrong

i don't see how, or even why it's of any importance.  the property is subfoldersHaveUnreadMessages-true, and not subfoldersNotFolderHaveUnreadMessages-true.  with either patch any combination can be made visually different using various css pseudo elements.

why don't you just state exactly what you want.

Comment 19

[Magnus Melin [:mkmelin]]

Let*s say you have

 -MAIN (5)
 |- sub1 (2)

For MAIN we'd get "hasUnreadMessages-true", and not "hasUnreadMessages-true + subfoldersHaveUnreadMessages-true"

... which is what the current code does.

Comment 20

[alta88]

right, nor should it.  the bug is about closed folders..

so:
+MAIN (0)
 (sub1 (2))

MAIN would get subfoldersHaveUnreadMessages-true and with current css be bold.

+MAIN (5)
 (sub1 (2))

MAIN would get hasUnreadMessages-true + subfoldersHaveUnreadMessages-true and be bold (it could be purple too).

+MAIN (5)
 (sub1 (0))

MAIN would get hasUnreadMessages-true and be bold.

-MAIN (5)
 |- sub1 (2)

MAIN would get hasUnreadMessages-true and be bold.

-MAIN (0)
 |- sub1 (2)

MAIN would get no property, and not be bold.  sub1 being bold is enough; this is the existing convention with open folders.

Comment 21

[alta88]

oh and the other thing is that the deep recursion on an open folder, without using the info derived, is a non zero perf hit for no purpose.

Comment 22

[Magnus Melin [:mkmelin]]

Comment on attachment 8380730 [details] [diff] [review]
unread.patch

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

What's your take on this, Richard? 
Do you think we need to let theming differentiate display of a folder that itself has unread with also subfolders having unread, vs. a folder that only itself have unread.

Comment 23

[Magnus Melin [:mkmelin]]

Well this bug is about closed folders, but you change it for all cases.
I wonder how much the pref hit actually is. We now show the sum of unread for closed folders, but I think there were some evidence it did not cause any real pref hit. Wouldn't this be up that same alley?

Comment 24

[:aceman]

We are investigating a perf hit in bug 939462 but there is no evidence yet it is caused by analysing subfolders as is done here (and in existing functionality).

Comment 25

[Richard Marti (:Paenglab)]

Comment on attachment 8380730 [details] [diff] [review]
unread.patch

(In reply to Magnus Melin from comment #22)
> Comment on attachment 8380730 [details] [diff] [review]
> unread.patch
> 
> Review of attachment 8380730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What's your take on this, Richard? 
> Do you think we need to let theming differentiate display of a folder that
> itself has unread with also subfolders having unread, vs. a folder that only
> itself have unread.

I think some indication would be needed when the folder has unread messages and is closed with a subfolder with unread messages. Now it shows the summary of all unread messages. When all messages in this main folder are read the folder shows still a summary of unread messages. This could irritate the user when he doesn't remembers it has closed subfolders.

I would give a ui-r+ for the preparation of this but I remove the request as there is actually no change in UI.

A different color wouldn't be optimal as of to little usable system colors. Maybe underlining could do it like we have now on threads with unread messages in them (but only when the folder is closed).

Maybe also a splitted summary could do it, like "Folder (2/3)". The first digit for the unread messages in this folder and the second for the subfolders. With my example above "Folder (0/3)" would show all messages in folder are read but it has unread in subfolders. This is only a proposal and needs more thinking.

Comment 26

[Ulf Zibis]

(In reply to Magnus Melin from comment #22)
> Do you think we need to let theming differentiate display of a folder that
> itself has unread with also subfolders having unread, vs. a folder that only
> itself have unread.

+1

Hi, I'm new in this discussion, but I like to make a suggestion.

Simple 2 rules:

a) If folder is BOLD / if account folder has bracketed number ==> TAKE A CLOSER LOOK.

a1) If folder is expanded (equivalent to: has no subfolders)
   ... select/open it to see unread messages
a2) If folder is collapsed
   ... expand it to see folders with unread messages
a3) If account folder is expanded
   ... never show bracketed number (or see folders with unread messages)
a4) If account folder is collapsed
   ... expand it to see folders with unread messages

b) If folder is NORMAL / if account folder has no bracketed number ==> NO MORE HIDDEN INFO.

b1) If folder is expanded (equivalent to: has no subfolders)
   ... has no unread messages (but visible subfolders may have)
b2) If folder is collapsed
   ... may has unread messages if has bracketed number (but invisible subfolders don't have)
b3) If account folder is expanded
   ... see visible folders (or consider a3)
b4) If account folder is collapsed
   ... has no unread messages in any folder or subfolders


Assume following situation:
-*account*
|-*parent1* (1)
||-*sub2* (2)
||| sub3
|-*parent4* (4)
|| sub5
|-parent6
|| *sub7* (7)
|-parent8
 | sub9

1. Folder expanded
1.1. has unread
1.1.1. Subfolders have unread
|-*parent1* (1)
|| *sub2* (2)
||| sub3
1.1.2. Subfolders have no unread
|-*parent4* (4)
|| sub5
1.2. has no unread
1.2.1. Subfolders have unread
|-parent6
|| *sub7* (7)
1.2.2. Subfolders have no unread
|-parent8
 | sub9
2. Folder collapsed
2.1. has unread
2.1.1. Subfolders have unread
|+*parent1* (1)
2.1.2. Subfolders have no unread
|+parent4 (4)
2.2. has no unread
2.2.1. Subfolders have unread
|+*parent6*
2.2.2. Subfolders have no unread
|+parent8
3. Account folder expanded
3.1. Folders have unread
-*account*
|-*parent1* (1)
||+sub2 (2)       // Sub-Subfolders have no unread
...
3.2. Folders have no unread
-*account*
...
|-parent8
 | sub9
4. Account folder collapsed
4.1. Folders have unread
+*account* (14)   // Sum of all unread messages
4.2. Folders have no unread
+*account*


> Maybe underlining could do it like we have now on threads with unread
> messages in them (but only when the folder is closed).
Also good idea!

> Maybe also a splitted summary could do it, like "Folder (2/3)". The first
> digit for the unread messages in this folder and the second for the
> subfolders. With my example above "Folder (0/3)" would show all messages in
> folder are read but it has unread in subfolders. This is only a proposal and
> needs more thinking.
Sophisticated!

(In reply to Richard Marti (:Paenglab) from comment #25)
> I think some indication would be needed when the folder has unread messages
> and is closed with a subfolder with unread messages.
+1 (see above)

> Now it shows the
> summary of all unread messages. When all messages in this main folder are
> read the folder shows still a summary of unread messages. This could
> irritate the user when he doesn't remembers it has closed subfolders.

Additionally:
When there are many unread messages in this main folder AND some few in subfolders. This could
mislead the user to miss unread messages in closed subfolders.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 8380730 [details] [diff] [review]
unread.patch

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

I guess we want both properties to set when that's the case...

Comment 28

[alta88]

Attachment: unread.patch

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 8409695 [details] [diff] [review]
unread.patch

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

::: mailnews/base/util/folderUtils.jsm
@@ +90,5 @@
> +    properties.push("hasUnreadMessages-true");
> +
> +  // A folder's subfolder has unread messages.
> +  if (aFolder.hasSubFolders && aFolder.hasFolderOrSubfolderUnreadMessages)
> +    properties.push("subfoldersHaveUnreadMessages-true");

you're now setting subfoldersHaveUnreadMessages-true for any folder that has unread (itself) and subfolders

Comment 30

[alta88]

Attachment: unread.patch

Comment 31

[alta88]

Comment on attachment 8411807 [details] [diff] [review]
unread.patch

not quite right.

Comment 32

[alta88]

Attachment: unread.patch

Comment 33

[alta88]

Attachment: unread.patch

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 8413219 [details] [diff] [review]
unread.patch

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

::: mailnews/base/public/nsIMsgFolder.idl
@@ +241,5 @@
> +   * @param bool root - if true, include the root message folder;
> +   *                     if false, recursively check only subfolders until an
> +   *                     unread message count is found
> +   */
> +  boolean getHasFolderOrSubfolderUnreadMessages(in boolean root);

All in all I think this is a really ugly method. Can't we just have something like a hasSubfolderUnreadMessages property and have the callers work it out wrt the root?

Comment 35

[alta88]

There is nothing wrong with this method, what so ever.  What is wrong is the current code.

Comment 36

[Magnus Melin [:mkmelin]]

Comment on attachment 8413219 [details] [diff] [review]
unread.patch

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

Per previous comments

Comment 37

[Wayne Mery (:wsmwk)]

(In reply to Magnus Melin [:mkmelin] from comment #34)
> Comment on attachment 8413219 [details] [diff] [review]
> unread.patch
> 
> Review of attachment 8413219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/base/public/nsIMsgFolder.idl
> @@ +241,5 @@
> > +   * @param bool root - if true, include the root message folder;
> > +   *                     if false, recursively check only subfolders until an
> > +   *                     unread message count is found
> > +   */
> > +  boolean getHasFolderOrSubfolderUnreadMessages(in boolean root);
> 
> All in all I think this is a really ugly method. Can't we just have
> something like a hasSubfolderUnreadMessages property and have the callers
> work it out wrt the root?

I'm curious, because this went through several iterations of review, but then bombed with the comment above...

Initially no concerns were raised and it may be ugly, but why not approve the patch to fix the problem we have today, and file a follow up bug for the desired improvement? 

Or perhaps four years later someone as a more obvious and clear solution?

Comment 38

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #34)
> > +  boolean getHasFolderOrSubfolderUnreadMessages(in boolean root);
> All in all I think this is a really ugly method. Can't we just have
> something like a hasSubfolderUnreadMessages property and have the callers
> work it out wrt the root?

A similar name and concept already exists as hasFolderOrSubfolderNewMessages, as a property.
Do you suggest the caller to check both hasFolderOrSubfolderNewMessages and hasNewMessages whether the folder itself has new messages? And do this same concept for the unreadMessages?

Comment 39

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #38)
> A similar name and concept already exists as
> hasFolderOrSubfolderNewMessages, as a property.
> Do you suggest the caller to check both hasFolderOrSubfolderNewMessages and
> hasNewMessages whether the folder itself has new messages? And do this same
> concept for the unreadMessages?

Yes, I'm suggesting to have symmetry for the unreadMessages and newMessage cases - https://searchfox.org/comm-central/source/mailnews/base/util/folderUtils.jsm#78