Closed Bug 533775 Opened 10 years ago Closed 6 years ago

Order of Local folders and Newsgroups is wrong in favorites view

Categories

(Thunderbird :: Folder and Message Lists, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 30.0

People

(Reporter: lakhia, Assigned: aceman)

References

(Blocks 1 open bug)

Details

(Keywords: regression, relnote, Whiteboard: [tb31features])

Attachments

(2 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.14) Gecko/2009082505 Red Hat/3.0.14-1.el5_4 (CK-dwa) (CK-dwa) Firefox/3.0.14
Build Identifier: Thunderbird 3.0

The newsgroups show up before local folders rather than after. In Thunderbird 2.0, this order was correct.  Note that this order is incorrect for:
   "Favorite Folders"
   "Unread Folders"


Reproducible: Always

Steps to Reproduce:
1. Subscribe to a newsgroup and mark it favorite
2. Mark a local folder as favorite
3. Switch to "Favorite Folders" view (View->Folders->Favorite)
Actual Results:  
Newsgroups show up first, Local folders appear after.

Expected Results:  
Local folders should show up first, Newsgroups should appear after.

I have a screen shot for this. Unfortunately, I can't find a way to attach this.
(In reply to comment #0)
> 
> I have a screen shot for this. Unfortunately, I can't find a way to attach
> this.

Use add attachment link above.
taking ali's word that this is a regression (makes sense, but I'm not going to retest v2 myself), and I do see this ordering in trunk, so confirming.
Severity: trivial → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: Order of Local folders and Newsgroups is wrong → Order of Local folders and Newsgroups is wrong in favorites view
Version: unspecified → 3.0
Order is not just wrong; it's randomly non sense.

There used to be a "sort folder" plugin for a very old TB (like 0.9 or 1.x) but it's deprecated since years by now. The root issue is that TB never allowed users to re-order folders in the pannel view; feature request bug stands since years about this.

Favourite view issue is just a side effect of the fact Mozilla dev team just never minded about folder order. Never ever.

I don't have news before local; I have:
-imap 1 inbox
- imap 2 inbox
- imap 3 inbox
- imap 1 subfolder
- local folder 1
- local folder 2
- local folder 3
- newsgroup 1
- local RSS 1
- imap 1 subfolder 2
- local RSS 2
- local subfolder
- imap 1 subfolder 3
- local RSS 3
- imap 1 subfolder 4

It's not ordered, there is no logic. It's just all messed up. Completely random.

Except that ... all root folders are put first.

They are not put in creation order on server or local.

They are not in alpha order.

No cap first.

Not sorted by account creation time.
I can confirm that the order of Favourites is strange. I get newsgroup as last item but the folders from POP3/IMAP/Local folders accounts are not in the order they appeared in the 'All' view.

Does anybody know where the Favorite flag is stored?
Assignee: nobody → acelists
aceman which version are you using ? so that I can put that version on my blacklist. I have absolutely no reason to update before this bug is fixed. Still using 3.1 ... and I wish I could downgrade to 1.5 ... 1.5 was the best ever.
I tried it on TB17.
Can anybody test if this is still happening after bug 749200 was fixed? You have to try a TB version higher than 19.
Flags: needinfo?(lakhia)
Flags: needinfo?(doublehp)
Flags: needinfo?(doublehp)
Ok so mail/base/content/folderPane.js::sortFolderItems takes all the favorite folders (http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.js#l1276) and sorts them according to their sort key. But that key does not account for different accounts. So the result is something comment 3 describes. From the Favorite set, Inboxes go first, then Drafts etc., see ordering at http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#5272)
Unread view has the same problem.

But sortFolderItems() is used also for other view types (e.g. Smart folders (Unified view)) where we may want to keep this grouping.
So I'll try to make 2 versions of the sorting and use them where needed.
But I'd like hints from the UX guys what ordering we actually want to get.
Flags: needinfo?(bwinton)
Keywords: uiwanted
OS: Linux → All
Hardware: x86 → All
Attached image Correct Approach β€”
Here's what I believe we want. Flagging bwinton for confirmation.
Attachment #789714 - Flags: ui-review?(bwinton)
Flags: needinfo?(bwinton)
Comment on attachment 789714 [details]
Correct Approach

Yep!
Attachment #789714 - Flags: ui-review?(bwinton) → ui-review+
I'm not sure I am able to include the account nodes too, but let's try.

Plop, would the order in the image work for you?
The attachment by Josiah Bruner is exactly what I am looking for. Thank you!
Flags: needinfo?(lakhia)
Attached patch patch (obsolete) β€” β€” Splinter Review
OK, this is an attempt to implement what is specified in the screenshot. This is my first experiment in the folder views code so please hammer on it if you can find any problems. I found one in mailWidgets.xml that I commented out.
Differing from the screenshot, if you have deep folder hierarchy on some deep level subfolder is Favorite but its parent is not, the parent(s) is still shown to allow expanding the tree to expose the Favorite child.

If this is deemed acceptable, I can try to modify the Unread view in a similar way.
Attachment #789813 - Flags: feedback?(mkmelin+mozilla)
Attachment #789813 - Flags: feedback?(josiah)
Attachment #789813 - Flags: feedback?(bwinton)
Comment on attachment 789813 [details] [diff] [review]
patch

I love it, works great with one caveat: Please make sure that the folders are opened by default in the hierarchy so the children can be seen. It threw me off a little bit when I clicked on "Favorite" to have only accounts be shown.

I am not (and assume you didn't intend for me to) review the code, since mkmelin is much better suited for that. However, as far as the ui goes, great job!
Attachment #789813 - Flags: feedback?(josiah) → feedback+
If I force opening of all the folders, do we then allow the user to collapse them? Should we then store the state when he switches the views? If yes, I am not sure we can determine when is the first time he uses the Favorites view and open all the folders.
(In reply to :aceman from comment #15)
> If I force opening of all the folders, do we then allow the user to collapse
> them? Should we then store the state when he switches the views? If yes, I
> am not sure we can determine when is the first time he uses the Favorites
> view and open all the folders.

Hmm. Could we then just use the same state they are in when in the default view mode?
Comment on attachment 789813 [details] [diff] [review]
patch

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

Sweet!!
Not necessarily in this bug, but we should do this for all modes, except unified of course. Generally i've never used anything except "All" mode much, exactly because it's a mess trying to find things as they aren't sorted logically.
Attachment #789813 - Flags: feedback?(mkmelin+mozilla) → feedback+
Comment on attachment 789813 [details] [diff] [review]
patch

If you've got f+ from both Josiah and Magnus, you should just go for it.  ;)

Thanks,
Blake.
Attachment #789813 - Flags: feedback?(bwinton) → feedback+
OK, I'll add the Unread view in a similar way.

The auto-expanding of folders that Josiah proposed is not covered yet. In the worst case (I do not find a clever way) maybe we can just force expand all the folders when user sets the Favorites or Unread view and do not persist the collapsed state when switching views.
Status: NEW → ASSIGNED
Aceman: can we move forward with this?
Keywords: uiwanted
Attached patch patch v2 (obsolete) β€” β€” Splinter Review
OK, this is getting shape.
-I addressed Josiah's issue with closed rows by adding infrastructure to not persist closed states in selected views.
-I fixed the display of account + folder name in the tooltip of a row (it is probably not used currently, but may serve in case some view has folders without accompanying it with account name in any way). Refer to the comment in addLocationInfo in previous version of the patch.
-I even made new/removed Favorite folders refresh the view so that they appear/disappear properly (e.g. a folder in Favorite view can be made not Favorite and it would not hide with the patch).
Attachment #789813 - Attachment is obsolete: true
Attachment #8344195 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8344195 [details] [diff] [review]
patch v2

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

Looks really good! We should do Recent mode too. r=mkmelin

::: mail/base/content/folderPane.js
@@ +728,5 @@
>      return this._rowMap[aIndex].level;
>    },
>  
>    /**
> +   * The ftvItems take care of assigning this when building children lists

copied, but this documentation could actually say what it does.

@@ +1129,5 @@
>      }
>      openLevel();
>    },
>  
> +  _persistItemClosed: function ftv_unpersistItem(aItemId) {

please add doc

@@ +1139,5 @@
> +    if (persistMapIndex != -1)
> +      this._persistOpenMap[mode].splice(persistMapIndex, 1);
> +  },
> +
> +  _persistItemOpen: function ftv_persistItem(aItemId) {

please add doc
Attachment #8344195 - Flags: review?(mkmelin+mozilla) → review+
Attached patch patch v3 (obsolete) β€” β€” Splinter Review
I got mixed opinions on IRC from the UI guys on including Recent view. I also feel there are some open issues in the Recent mode and it also isn't so trivial to implement.

Would you be OK with landing this as is, seeing how users receive it and add Recent view in another bug if we are confident it is wanted? After all, this bug only requests Favorite mode.
Attachment #8344195 - Attachment is obsolete: true
Attachment #8345544 - Flags: review+
Sure, lets land and discuss Recent mode in a follow-up.
Blocks: 949127
Thanks, I filed the follow-up bug.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/eed75aaebc9b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
Backed out for mozmill orange.
https://hg.mozilla.org/comm-central/rev/e979c0479a34

https://tbpl.mozilla.org/php/getParsedLog.php?id=31953482&tree=Thunderbird-Trunk
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 29.0 → ---
Attached patch patch v4 (obsolete) β€” β€” Splinter Review
OK, this also tweaks the folder view tests and actually tries to test the new behaviour a bit.

Aryx, please push to try server, mozmill tests only.
Attachment #8345544 - Attachment is obsolete: true
Flags: needinfo?(archaeopteryx)
Attached patch patch v5 β€” β€” Splinter Review
So now that we knew which tests failed, I fixed them. If the tests pass now, I'll explain what has changed.

Aryx, please try another try run, mozmill only. Thanks.
Attachment #8377889 - Attachment is obsolete: true
Flags: needinfo?(archaeopteryx)
Now the tree seems to be working, please push this again.
Flags: needinfo?(archaeopteryx)
Comment on attachment 8378551 [details] [diff] [review]
patch v5

Too green to be true :)

OK, so there is no code change to the patch that was reviewed. This one just fixes tests to pass with the new behaviour.

It is so big because I move test-display-message-with-folder-modes.js from folder-display to folder-pane directory because in folder-display the tests running before it create a ton of folders and do not clean up after themselves. This was already observed in the past and is written in the comment in test-folder-pane.js :) I need a predictable number of folders and their properties to be able to check what should be displayed in the folder views.

Next I rename test-folder-names-in-favorite-mode.js to test-folder-names-in-recent-mode.js because favorite view no longer behaves as the test expects. The folder name changing functionality is not used in favorite mode so the test would be useless. So I adapt it to check similar feature in the Recent mode, which is not affected in this bug.
Attachment #8378551 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8378551 [details] [diff] [review]
patch v5

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

LGTM. r=mkmelin
Attachment #8378551 - Flags: review?(mkmelin+mozilla) → review+
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b654c1648f7d
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
Depends on: 978616
Depends on: 978592
Keywords: relnote
Whiteboard: [tb31features]
Duplicate of this bug: 370737
You need to log in before you can comment on or make changes to this bug.