Order of Local folders and Newsgroups is wrong in favorites view

RESOLVED FIXED in Thunderbird 30.0

Status

Thunderbird
Folder and Message Lists
--
minor
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: Ali Lakhia, Assigned: aceman)

Tracking

(Blocks: 1 bug, {regression, relnote})

Thunderbird 30.0
regression, relnote
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tb31features])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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.

Comment 2

8 years ago
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

Comment 3

7 years ago
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.
(Assignee)

Comment 4

6 years ago
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

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
I tried it on TB17.
(Assignee)

Comment 7

5 years ago
Can anybody test if this is still happening after bug 749200 was fixed? You have to try a TB version higher than 19.
(Assignee)

Updated

5 years ago
Flags: needinfo?(lakhia)
Flags: needinfo?(doublehp)

Updated

5 years ago
Flags: needinfo?(doublehp)
(Assignee)

Comment 8

5 years ago
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
Created attachment 789714 [details]
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+
(Assignee)

Comment 11

5 years ago
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?
(Reporter)

Comment 12

5 years ago
The attachment by Josiah Bruner is exactly what I am looking for. Thank you!
Flags: needinfo?(lakhia)
(Assignee)

Comment 13

5 years ago
Created attachment 789813 [details] [diff] [review]
patch

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+
(Assignee)

Comment 15

5 years ago
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 17

5 years ago
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+
(Assignee)

Comment 19

5 years ago
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

Comment 20

5 years ago
Aceman: can we move forward with this?
Keywords: uiwanted
(Assignee)

Comment 21

5 years ago
Created attachment 8344195 [details] [diff] [review]
patch v2

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 22

5 years ago
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+
(Assignee)

Comment 23

5 years ago
Created attachment 8345544 [details] [diff] [review]
patch v3

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+

Comment 24

5 years ago
Sure, lets land and discuss Recent mode in a follow-up.
(Assignee)

Updated

5 years ago
Blocks: 949127
(Assignee)

Comment 25

5 years ago
Thanks, I filed the follow-up bug.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/eed75aaebc9b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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 → ---
(Assignee)

Comment 28

4 years ago
Created attachment 8377889 [details] [diff] [review]
patch v4

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)
(Assignee)

Comment 30

4 years ago
Created attachment 8378551 [details] [diff] [review]
patch v5

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)
(Assignee)

Comment 32

4 years ago
Now the tree seems to be working, please push this again.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 34

4 years ago
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 35

4 years ago
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+
(Assignee)

Comment 36

4 years ago
Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b654c1648f7d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0

Updated

4 years ago
Depends on: 978616

Updated

4 years ago
Depends on: 978592
Keywords: relnote
Whiteboard: [tb31features]
(Assignee)

Updated

4 years ago
Duplicate of this bug: 370737
You need to log in before you can comment on or make changes to this bug.