add ability to manually sort (order) folders in folder pane
Categories
(Thunderbird :: Folder and Message Lists, enhancement)
Tracking
(relnote-thunderbird 139+, thunderbird_esr115 wontfix, thunderbird120 wontfix, thunderbird139 fixed)
People
(Reporter: itagagaki, Assigned: itagagaki)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 6 obsolete files)
+++ This bug was initially created as a clone of Bug #193314 +++
Searched thrue bugzilla, but could not find this RFE... I wonder does only I
need this?
I want to being eble to sort my folder. What I mean is:
Inbox
|
|-aaa
|-bbb
|-ccc
Now they sort only alphabetical. But I would like to have the ccc at the top of
all folders.
This should work also in subfolders.
Thunderbird was not designed to meet this demand. An add-on had helped, but with Thunderbird 115 (Supernova), the add-on is no longer available. It is time to make it a proper feature.
I was involved in the development of the add-on for Thunderbird 78 and later as a collaborator. So, as an option, I have considered developing another add-on for Thunderbird 115. This may not be impossible. But here I would consider implementing the ability to sort folders properly in Thunderbird.
I started by examining the source code for the new Thunderbird. Apparently the folder pane is var folderPane. The code is in about3Pane.js. And folderPane.handleEvent() looks like this:
handleEvent(event) {
switch (event.type) {
case "select":
this._onSelect(event);
break;
case "contextmenu":
this._onContextMenu(event);
break;
case "collapsed":
this._onCollapsed(event);
break;
case "expanded":
this._onExpanded(event);
break;
case "dragstart":
this._onDragStart(event);
break;
case "dragover":
this._onDragOver(event);
break;
case "dragleave":
this._clearDropTarget(event);
break;
case "drop":
this._onDrop(event);
break;
}
}
It would be good to modify handling dragstart, dragover, dragleave, and drop a bit. Currently, it is possible to move a folder as a child into the target where we drag and drop. This would be done by these handlers. Probably the only status is in drag or not. The first step is to add the following additional states to the state in drag.
- Move into the target as a child
- Move to the point before the target as a sibling
- Move to the point after the target as a sibling
The above three states may be determined by the relative position of the Y-coordinate of the mouse pointer on the target when dragging. (Not sure if there is a way to get that yet.)
And we need display materials that make the above three states visible.
Once the above is done, the next step is to actually insert the folder into the dropped location.
And the order of the folders sorted in this UI must be stored somewhere.
And it must be replicated next time. The current process of alphabetical sorting needs to be stopped.
The help of someone familiar with the current architecture is needed to ensure that the folder order can be stored and reproduced without contradiction.
Comment 2•2 years ago
|
||
Instead of drag and drop, or maybe in addition to it, I really liked the way "Manual sort Folders" worked with the up and down buttons to move a folder. To me, its a really important feature that went away with version 115. I'm considering rolling back to version 102 where this addon worked and I didn't have to dig in the menu's for "get all emails for all accounts" as in 115 but instead it had a simple button that opened up a list or a "get all" selection. I find 115 very confusing to use vs 102
Comment 3•2 years ago
|
||
As I've sketched in https://github.com/protz/Manually-Sort-Folders/issues/199#issuecomment-1722500980 it is easily possible to sort folders with the "order" css tag. So maybe that's a possible approach to set and store settings via some custom css file.
To keep work simple, it also might suffice to add context menu entries "move up/move down" which would move the folder one position up and down. Would take some clicks to move a folder up by e.g. ten positions, but as I said: how often do you do that... It could be an easier-to-implement first approach, and doing it with drag&drop could come in a later release if neccessary.
What is the point of stubbornly avoiding a pure and straightforward implementation, proposing a half-baked implementation, relying on add-ons, or using some other workaround?
Comment 5•2 years ago
|
||
Hmm, I just thought this could be a possible approach to implement this feature in thunderbird (not in an addon) as the folder tree already respects the css order tag. So using this tag and storing css values (as is done for other TB features) could work for the implementation. And for the interface, I don't mind if it's drag&drop or context-menu moveup/down. This was just an idea how it could be simple and therefore easier/quicker to realize... Sorry... :)
Comment 6•2 years ago
|
||
For me, i don't really care how its done except i don't want to have to manually edit a css file as I have a lot of folders in each account. The addon "Manually Sort Folders" was fine and only really lacked one feature "Move folder to top/bottom" in one step which can save a ton of mouse clicks to get a folder where you want it
I'm a bit leery about having mozilla implement it, their interface design ideas seem weird to me (see version 115 as an example)
There are three main things to consider.
UI, restructuring the tree view (display issues), and remembering the order (user preference data issues).
Yes, there could be several ways to go about the UI. But I would prefer the obvious way, which is intuitive for the users. It is odd that we can drag and drop in other hierarchies but not in the same hierarchy.
Next, regarding the display issues, there is a way to achieve this with the CSS you suggested. But since the system currently sorts the folders alphabetically and then reconstructs the tree for display, it would just be a matter of correcting the sorting function.
And in any case, the order in which users manually sort should be stored somewhere as user preference data. This part of the design needs help from the TB development team.
Comment 8•2 years ago
|
||
There may be other reports for this, but I think bug 318467 covers it.
Glad someone is talking about this. However, Bugzilla is for core code. https://thunderbird.topicbox.com/groups/addons is the place for discussing creating add-ons.
Comment 9•2 years ago
|
||
The point is that this should be a core function and not an add-on, that's why itagagaki opened this bug here.
| Assignee | ||
Comment 10•2 years ago
•
|
||
bug 193314 opened 21 years ago.
bug 318467 opened 18 years ago.
In a way, it is surprising that this issue has not progressed at all during this time.
Comment 11•2 years ago
|
||
(In reply to Frank Steiner from comment #9)
The point is that this should be a core function and not an add-on, that's why itagagaki opened this bug here.
I definitely second Frank here, and thanks Itagagaki for having reported this issue.
Comment 12•2 years ago
|
||
(In reply to Frank Steiner from comment #9)
The point is that this should be a core function and not an add-on, that's why itagagaki opened this bug here.
It's great that you are interested in making it possible, but the lack of duplicates / requests in a 20 year period indicates this is not in high demand. Items not in high demand are a primary reason for add-ons, without add-ons the world would be a sadder place. https://thunderbird.topicbox.com/groups/addons is the correct place to discuss creating an add-on or fixing an existing one.
| Assignee | ||
Comment 13•2 years ago
|
||
(In reply to Wayne Mery (:wsmwk) from comment #12)
this is not in high demand.
I am disappointed that you are not listening to your users. The need is not insignificant. It is the sensitivity of the ear to user demand that is lacking.
Have a look at this?
https://connect.mozilla.org/t5/ideas/thunderbird-add-ability-to-manually-sort-order-folders-in-folder/idi-p/39061
Updated•2 years ago
|
Comment 15•2 years ago
|
||
Bug 193314 which was used to create this bug report was closed on technical grounds when the folder pane was still XUL based. With version 115 the folder pane is no longer based on XUL.
To be absolutely clear, no one currently involved in Thunderbird is against implementing this in Thunderbird. Further discussions here should be about the code needed to implement the feature.
| Assignee | ||
Comment 16•2 years ago
|
||
Of course, I myself am not afraid to contribute code to core. However, it is necessary to involve the developers of the core and create a collaborative structure. How can we do that?
I think the first thing we need to do is assign the right person to start the conversation going. Of course, there are many tasks that have higher priorities, so it is understandable that they tend to get put on the back burner, but nothing will ever get done if we do not at least start the technical review and analysis.
Comment 17•2 years ago
|
||
(In reply to itagagaki from comment #16)
Of course, I myself am not afraid to contribute code to core.
Your starting point is https://developer.thunderbird.net/
Further development questions should be directed to matrix developer channel https://www.thunderbird.net/en-US/get-involved/#development
| Assignee | ||
Comment 18•2 years ago
|
||
(In reply to Wayne Mery (:wsmwk) from comment #17)
Your starting point is https://developer.thunderbird.net/
Further development questions should be directed to matrix developer channel https://www.thunderbird.net/en-US/get-involved/#development
No, no, as I said in a private email, it is not a guide I need. It's a collaborative system. We need to discuss this topic with the developers involved, and Mozilla needs to specify the specifications and assign people to work on them.
| Assignee | ||
Comment 19•2 years ago
|
||
(In reply to Wayne Mery (:wsmwk) from comment #15)
To be absolutely clear, no one currently involved in Thunderbird is against implementing this in Thunderbird. Further discussions here should be about the code needed to implement the feature.
Also, just to honor those who have spoken up here, until this topic was misinterpreted as being about creating add-ons, all people were doing was looking at specific considerations for implementation in the core code. I think there was nothing to warn about.
| Assignee | ||
Comment 20•2 years ago
|
||
Since my intentions in comment #1 here don't seem to be getting through to the key people, I'm going to sort out who should be doing what.
- Display and control markers when dragging a tree item -- maybe I can
- Design and create SVG material for that marker -- Someone from the TB team
- Process to update and hold the position of items moved by D&D as internal data in the core code, and also reflect it in the display -- maybe I can
- Design of a data structure to store the order of folders for each account as user preference data -- Someone from the TB team
- Store and retrieve the user preference data -- maybe I can, but TB team may be more appropriate
Comment 21•2 years ago
|
||
and just to back itagagaki, it is a very useful addon in previous versions of Thunderbird, so if it became a default part of the core, that would be very useful.
thanks itagagaki for pursuing this functionality, and indeed offering to help make it happen.
| Assignee | ||
Comment 22•2 years ago
|
||
I have implemented the simple drag indicator. Remaining tasks are:
- Have the folder order as internal data
- Reflect it in the tree view
- Save and load it as preference data
| Assignee | ||
Comment 23•2 years ago
|
||
The drag indicator has been further improved. It seems to work well now.
https://twitter.com/itagagaki/status/1713147694829720025
| Assignee | ||
Comment 24•2 years ago
|
||
The next step is to maintain the order internally. After examining the code, the folder object has methods SetSortOrder and GetSortOrder. However, the processing of SetSortOrder is not yet implemented, while GetSortOrder returns a constant value that varies depending on the type of folder.
NS_IMETHODIMP nsMsgDBFolder::SetSortOrder(int32_t order) {
NS_ASSERTION(false, "not implemented");
return NS_ERROR_NOT_IMPLEMENTED;
}
NS_IMETHODIMP nsMsgDBFolder::GetSortOrder(int32_t* order) {
NS_ENSURE_ARG_POINTER(order);
uint32_t flags;
nsresult rv = GetFlags(&flags);
NS_ENSURE_SUCCESS(rv, rv);
if (flags & nsMsgFolderFlags::Inbox)
*order = 0;
else if (flags & nsMsgFolderFlags::Drafts)
*order = 1;
else if (flags & nsMsgFolderFlags::Templates)
*order = 2;
else if (flags & nsMsgFolderFlags::SentMail)
*order = 3;
else if (flags & nsMsgFolderFlags::Archive)
*order = 4;
else if (flags & nsMsgFolderFlags::Junk)
*order = 5;
else if (flags & nsMsgFolderFlags::Trash)
*order = 6;
else if (flags & nsMsgFolderFlags::Virtual)
*order = 7;
else if (flags & nsMsgFolderFlags::Queue)
*order = 8;
else
*order = 9;
return NS_OK;
}
And sorting of folders is done by comparing {sortOrder}{name}. Currently the value of {sortOrder} is the constant 9 for a normal folder, so the folders are still sorted alphabetically by their names after all.
// static Helper function for CompareSortKeys().
// Builds a collation key for a given folder based on "{sortOrder}{name}"
nsresult nsMsgDBFolder::BuildFolderSortKey(nsIMsgFolder* aFolder,
nsTArray<uint8_t>& aKey) {
I think it would be a good idea to change the implementation of this part.
| Assignee | ||
Comment 25•2 years ago
|
||
However, there is a problem: if there are 10 folders with sortOrder from 1 to 10, the current comparison logic would result in this sort order: 1,10,2,3,4,5,6,7,8,9
I am a little unsure if it is safe to change the comparison logic.
| Assignee | ||
Comment 26•2 years ago
|
||
Updated•2 years ago
|
Updated•2 years ago
|
| Comment hidden (metoo) |
Updated•2 years ago
|
Comment 28•2 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1e6c184038ca
add ability to manually sort (order) folders in folder pane r=darktrojan,freaktechnik
Comment 29•2 years ago
•
|
||
bug 1848132 seems to have become a permanent failure following merge of this patch.
Comment 30•2 years ago
|
||
Let's back out 1e6c184038ca until the issues discovered have been sorted out.
Comment 31•2 years ago
|
||
Updated•2 years ago
|
Updated•2 years ago
|
| Assignee | ||
Comment 33•2 years ago
|
||
Changes summary:
- Added new property folderSortOrder to nsDBFolderInfo. This property has an order among siblings.
- Added new property userSortOrder to nsMsgDBFolder. This represents folderSortOrder above.
- The existing property getter nsMsgDBFolder::GetSortOrder returns it if userSortOrder is not NO_SORT_VALUE, and returns a value based on the conventional rule if it is NO_SORT_VALUE for the backward compatibility.
- The method to compare sortOrder has been changed. Previously the order was 1, 10, 2, changed to 1, 2, 10.
- Show folder drag indicator when dragging a folder to the edge of the target folder. It consists of three new svg.
- Set the order of the dragged folder so that it is between the order of its siblings before and after the position from which it was dropped. The order of the siblings is also incremented to ensure that this is the case.
- This also updates FolderTreeRow.folderSortOrder of the rows for the folder in the active views.
- Then, in each view, it updates the position of the moved folder row. This is accomplished by comparing the FolderTreeRow.folderSortOrder with its sibling rows.
- The behavior when dropping directly above the target folder is the same as before. However, if the siblings at the destination are sorted by the user, it will be placed at the bottom.
- As above for the newly added subfolder.
- I did a quick test with empty folders I created in the "Local Folders" and a new Gmail account I created for testing. But I think this needs careful testing. Please review carefully first.
Additional:
- Folders can only be moved within the same server, of course
- Folders can only be moved to the "All Folders" view. (can be moved from other views to the "All Folders" view.)
Updated•2 years ago
|
Comment 34•2 years ago
|
||
Is the version of "Wed, Nov 15, 3:45 PM" final? You're still calling ReadDBFolderInfo(true); and not caching the sort order, so this will open every single database on every startup and modify it(*). Wasn't the idea to integrate the patch from bug 1864510, here in its last version.
(*): Users who do incremental backups will backup many .msf files unnecessarily.
| Assignee | ||
Comment 35•2 years ago
|
||
(In reply to betterbird.project+12 from comment #34)
Is the version of "Wed, Nov 15, 3:45 PM" final? You're still calling
ReadDBFolderInfo(true);and not caching the sort order, so this will open every single database on every startup and modify it(*). Wasn't the idea to integrate the patch from bug 1864510, here in its last version.(*): Users who do incremental backups will backup many .msf files unnecessarily.
No. WIP.
Comment 36•2 years ago
|
||
Sorry, quite confusing: "itagagaki requested review of this revision. Wed, Nov 15, 3:45 PM".
| Assignee | ||
Comment 37•2 years ago
|
||
Asking reviewers for a review does not mean that the work is done. The goal is to get reviewers who are familiar with the topic to consider improvements to known performance issues. There is no need for you to be confused in the process of accepting this patch.
Updated•2 years ago
|
| Assignee | ||
Comment 38•2 years ago
•
|
||
Incorporated the fix from bug 1864510 comment 7 and awaiting review.
Comment 39•2 years ago
|
||
Please add the following to the Phab summary so it gets committed like in these examples: https://hg.mozilla.org/comm-central/log?rev=betterbird:
Code changes in nsMsgDBFolder::SetUserSortOrder(), nsMsgDBFolder::GetUserSortOrder() and nsMsgDBFolder::ReadDBFolderInfo() taken from
https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1864510-performance-bug-1846550.patch
| Assignee | ||
Comment 40•2 years ago
|
||
(In reply to betterbird.project+13 from comment #39)
Please add the following to the Phab summary
Done. Please let me know if there are any deficiencies.
Thank you very much for sharing your analysis and fixes with us.
Comment 41•2 years ago
|
||
Thank you very much for sharing your analysis and fixes with us.
It's our pleasure. Good to see this finally implemented in TB without having to use an add-on. If you decide to backport to ESR 115, we have a patch to do that. Ideally it should work for unified folders, too. Users may want to reorder folders of an account that are not part of the "unified scheme". Anyway, that's for the next bug.
Comment 42•2 years ago
|
||
(In reply to betterbird.project+13 from comment #41)
Thank you very much for sharing your analysis and fixes with us.
It's our pleasure. Good to see this finally implemented in TB without having to use an add-on. If you decide to backport to ESR 115, we have a patch to do that. Ideally it should work for unified folders, too. Users may want to reorder folders of an account that are not part of the "unified scheme". Anyway, that's for the next bug.
Please consider this.
Comment 43•2 years ago
|
||
We noticed some failures when reordering IMAP folders. We haven't found a 100% reliable case, but try this:
Create a folder XXX with subfolders A, B and C. Order: A B C. Move C to the top, order: C A B. Restart. Order: C A B. Now move C between A and B, order: A C B. Restart. Order restored to C A B. Not always, but mostly.
| Assignee | ||
Comment 44•2 years ago
|
||
(In reply to betterbird.project+13 from comment #43)
We noticed some failures when reordering IMAP folders
Thanks for the report.
If possible, it would be nice if you could try to see if you can reproduce this in the version before we added caching (before the performance fix).
Comment 45•2 years ago
|
||
Good hint. Yes, it's related to the caching part. We debugged it, found the cause and will post an update soon.
Comment 46•2 years ago
|
||
Please merge these additional changes. You can also see them here. The code would be simpler without catering for the deletion of folderCache.json. Note that moving a profile is as good as deleting the cache since sadly it records absolute file paths. So the complexity is warranted since otherwise moving a profile would already lose the sort order.
| Assignee | ||
Comment 47•2 years ago
|
||
Merged and awaiting review.
| Assignee | ||
Comment 48•2 years ago
|
||
(In reply to betterbird.project+13 from comment #46)
The cache does not seem to be write-through. Is flushing necessary?
Here's how to check:
Create folders XXX, A, B and C.
Order: A B C XXX.
Drop C directly above XXX. Then drop B directly above XXX. Both C and B don't have userSortOrder yet, their old-days sortOrder will both be 9. So the order in XXX is: B C.
Move C before B.
C will be get userSortOrder 10 and B will be get userSortOrder 11. Order: C B.
Now move A directly above XXX. It is expected that A will be given userSortOrder 12 and the order will be C B A because the maximum value of siblings is 11.
However, setSortOrderOnNewFolder doesn't give A a new value because both C and B have a userSortOrder value that is still NO_SORT_VALUE.
| Assignee | ||
Comment 49•2 years ago
|
||
It may not be a writing problem, but a different key to get it.
Comment 50•2 years ago
|
||
The cache is not write-through, it gets flushed here:
https://searchfox.org/comm-central/rev/81a9d6c515a510b0398b00711f5a11a53c15e5be/mailnews/base/src/nsMsgAccountManager.cpp#213
since bug 1774072.
Set SetUserSortOrder() writes to the cache and to the DB, but get GetUserSortOrder() only reads from the cache to avoid opening all DBs at startup. All that should be transparent to the rest of the code unless there are more sort values involved like "old-days" sort order.
Your example isn't completely clear. Apparently you are dropping A, B and C into XXX, hence changing the nesting depth. The cache values are stored against the folder path (open folderCache.json in an editor to see it), if you move a folder a level up or down in the tree, it won't have a cache value. Maybe in that case you just want to set the value again so it gets stored against the new path.
| Assignee | ||
Comment 51•2 years ago
|
||
I see. That is not good.
The following code is sufficient to verify.
folder.userSortOrder = order; // Update DB
console.log(order, folder.sortOrder, folder.userSortOrder);
or
folder.sortOrder = order; // Update DB
console.log(order, folder.sortOrder, folder.userSortOrder);
Both results:
console.log: 11 9 4294967295
The value is not flushed, so the getter returns the previous value.
The sort order is also given to the UI elements, which is reflected immediately, so it appears to work, but the order of the folders is only determined by the sortOrder comparison, so the getter must return the value set by the setter.
| Assignee | ||
Comment 52•2 years ago
|
||
Maybe we should add mUserSortOrder to nsMsgDBFolder and keep the value?
Comment 53•2 years ago
|
||
Our patch from bug 1864510 fixes the fact that you can't open all folder msf files on the system to get the "folderSortOrder" property at startup. Setting/getting nsIMsgFolder.userSortOrder sets/gets nsIDBFolderInfo.folderSortOrder, now via the folder cache.
We didn't inspect the logic of how nsIMsgFolder.sortOrder, nsIMsgFolder.userSortOrder are related. Seems "normal" that setting nsIMsgFolder.sortOrder would not affect nsIMsgFolder.userSortOrder and vice versa.
| Assignee | ||
Comment 54•2 years ago
|
||
Look.
let order = 11;
folder.userSortOrder = order;
console.log(order, folder.sortOrder);
The output of console.log must be 11 11
let order = 11;
folder.userSortOrder = order;
console.log(order, folder.userSortOrder);
The output of console.log must be at least 11 11
| Assignee | ||
Comment 55•2 years ago
|
||
Sorry. There was a writing error.
let order = 11;
folder.sortOrder = order;
console.log(order, folder.sortOrder);
The output of console.log must be 11 11
let order = 11;
folder.userSortOrder = order;
console.log(order, folder.userSortOrder);
The output of console.log must be 11 11
| Assignee | ||
Comment 56•2 years ago
|
||
The problem is that the assignment to userSortOrder is not reflected inside nsMsgDBFolder either. The cache mechanism may be moving values in and out, but it is not being returned by nsMsgDBFolder::GetUserSortOrder. Consequently nsMsgDBFolder::GetSortOrder also returns the old value.
As an aside, I think nsMsgDBFolder:: SetSortOrder should be restored to its pre-patch state. We should not assign to sortOrder.
Comment 57•2 years ago
|
||
let folder = gMessage.folder;
folder.userSortOrder = 55;
console.log(folder.userSortOrder, folder.sortOrder);
folder.sortOrder = 44;
console.log(folder.userSortOrder, folder.sortOrder);
In our product, that prints 55 twice and 44 twice.
SetSortOrder() calls SetUserSortOrder() and GetSortOrder() calls GetUserSortOrder(). That was taken from your initial version of Mon, Nov 20, 4:02 PM and Sat, Nov 25, 11:32 PM. In your latest version of Tue, Nov 28, 9:56 AM SetSortOrder() returns an error, of course then the result would be different.
Get/SetUserSortOrder() work via the cache (unless you change the folder hierarchy level) and as long as Get/SetSortOrder() mostly call the "user" versions, they also work.
Please advise if we can clarify anything else.
| Assignee | ||
Comment 58•2 years ago
|
||
OK. I found the first clue.
Let's put the following code in onFolderAdded and observe.
var folderListener = {
QueryInterface: ChromeUtils.generateQI(["nsIFolderListener"]),
onFolderAdded(parentFolder, childFolder) {
+
+ console.log(childFolder.userSortOrder, childFolder.sortOrder);
+ childFolder.userSortOrder = 55;
+ console.log(childFolder.userSortOrder, childFolder.sortOrder);
+ childFolder.sortOrder = 44;
+ console.log(childFolder.userSortOrder, childFolder.sortOrder);
+
if (childFolder.userSortOrder == Ci.nsIMsgFolder.NO_SORT_VALUE) {
// This folder is new.
folderPane.setSortOrderOnNewFolder(childFolder, parentFolder);
}
folderPane.addFolder(parentFolder, childFolder);
folderPane.updateFolderRowUIElements();
},
Try creating a new folder in Local Folders.
You will get the following results.
console.log: 4294967295 9
console.log: 55 55
console.log: 44 44
Fine.
Then let's move this folder into another folder.
console.log: 4294967295 9
console.log: 4294967295 9
console.log: 4294967295 9
The first state is correct, but subsequent lines show that value setting is not reflected.
When the folder hierarchy changes, it is treated as a new child folder of another parent folder and onFolderAdded is called. In this case, the previous cache can be discarded, but the new cache should be given as a new folder, I think.
Comment 59•2 years ago
|
||
Thanks for the debugging. We added further debugging:
void nsMsgDBFolder::SetUserSortOrderInCache(uint32_t order, bool overwrite) {
nsCOMPtr<nsIFile> dbPath;
GetFolderCacheKey(getter_AddRefs(dbPath));
+ nsString p;
+ p = dbPath->NativePath();
+ printf("=== SetUserSortOrderInCache %s %u %d\n", NS_ConvertUTF16toUTF8(p).get(), order, overwrite);
if (dbPath) {
nsCOMPtr<nsIMsgFolderCacheElement> cacheElement;
- GetFolderCacheElemFromFile(dbPath, getter_AddRefs(cacheElement));
+ nsresult rv;
+ rv = GetFolderCacheElemFromFile(dbPath, getter_AddRefs(cacheElement));
+ printf("=== SetUserSortOrderInCache %lx %p\n", static_cast<unsigned long>(rv), (void*)cacheElement);
if (cacheElement) {
uint32_t dummy;
if (overwrite ||
NS_FAILED(cacheElement->GetCachedUInt32("folderSortOrder", &dummy))) {
- cacheElement->SetCachedUInt32("folderSortOrder", order);
+ rv = cacheElement->SetCachedUInt32("folderSortOrder", order);
+ printf("=== SetUserSortOrderInCache %lx\n", static_cast<unsigned long>(rv));
}
}
}
}
and
NS_IMETHODIMP nsMsgDBFolder::GetUserSortOrder(uint32_t* order) {
NS_ENSURE_ARG_POINTER(order);
nsCOMPtr<nsIFile> dbPath;
nsresult rv = GetFolderCacheKey(getter_AddRefs(dbPath));
+ nsString p;
+ p = dbPath->NativePath();
if (dbPath) {
nsCOMPtr<nsIMsgFolderCacheElement> cacheElement;
rv = GetFolderCacheElemFromFile(dbPath, getter_AddRefs(cacheElement));
if (cacheElement) // try to get from cache
rv = cacheElement->GetCachedUInt32("folderSortOrder", order);
if (NS_FAILED(rv)) {
// Don't open DB for missing order property, if it's not there,
// it was never set.
+ printf("=== GetUserSortOrderInCache %s NOT SET\n", NS_ConvertUTF16toUTF8(p).get());
*order = static_cast<uint32_t>(nsIMsgFolder::NO_SORT_VALUE);
return NS_OK;
}
}
+ printf("=== GetUserSortOrderInCache %s %u\n", NS_ConvertUTF16toUTF8(p).get(), *order);
return rv;
}
On moving folder ddd6 into Sort, this is the result:
=== SetUserSortOrderInCache [snip]\Mail\Local Folders\Sort.sbd\ddd6.msf 55 1
=== SetUserSortOrderInCache 80040111 0000000000000000
=== GetUserSortOrderInCache [snip]\Mail\Local Folders\Sort.sbd\ddd6.msf NOT SET
At the time of the drop, the cache element for the new location doesn't exist yet, hence GetFolderCacheElemFromFile() fails. You can make it work by changing false to true here:
https://searchfox.org/comm-central/rev/360b1076cfcf4a236d7cc10e538860921b04da1c/mailnews/base/src/nsMsgDBFolder.cpp#617
This would need to be discussed with the author of the folder cache.
Alternatively, maybe some operation on the folder in your listener onFolderAdded() would get the cache element created and you delay setting the sort order. Or we add more error handling to SetUserSortOrderInCache() and create the missing cache element there.
Comment 60•2 years ago
|
||
Here are some minimal C++ changes that will solve the issue from comment #58.
| Assignee | ||
Comment 61•2 years ago
|
||
Short summary of the remaining issue
I added new property folderSortOrder to nsDBFolderInfo.
Then I added userSortOrder to nsMsgDBFolder side. This is to represent folderSortOrder above.
Due to performance issues, we want to cache userSortOrder.
Betterbird folks gave us the implementation nsMsgDBFolder::SetUserSortOrder and nsMsgDBFolder::GetUserSortOrder with caching. It seems to work well.
However, there is a problem when we move a folder to a different hierarchy because the folder cache uses the absolute path as the per-folder key. (Bug 1726660)
The only solution we have now is to let createIfMissing=true when calling nsMsgFolderCache::GetCacheElement, but I'm not sure if this is the best solution.
See https://phabricator.services.mozilla.com/D193659 for the patches.
Comment 62•2 years ago
|
||
(In reply to itagagaki from comment #61)
However, there is a problem when we move a folder to a different hierarchy because the folder cache uses the absolute path as the per-folder key. (Bug 1726660)
Ahh... In that case the folder is actually being moved/re-parented.
The issue is (I think) that the nsIFolderCacheElement doesn't get carried over along with the folder being moved?
So the key on the nsIFolderCacheElement should probably to be renamed to reflect the new location in the folder hierarchy.
(and the suggested solution in Bug 1726660 of using the folder URI as cache key won't help by itself , because the URI will change when the folder moved to a different parent).
Does that sound about right?
I don't entirely follow the JS side of things, but there's really two separate operations that could be performed:
- dragging to reorder a folder within the level it's already in (ie just changing the userSortOrder)
- dragging a folder so it now resides under another parent (ie full-blown folder move/copy)
Have I got that right?
The only solution we have now is to let
createIfMissing=true when callingnsMsgFolderCache::GetCacheElement, but I'm not sure if this is the best solution.
I guess that'd work... but it's a bandaid and doesn't really solve the underlying problem. I'm loathe to let too many more bandaids into the codebase as we've already got waaaaaaaay too many, and that stuff really accrues as technical debt.
I really want to get your folder reordering in there!
If we can fix it up so the foldercache survives folders being moved, then problem solved.
If not, we can put in something like the createIfMissing bandaid, along with a new Bug to fix it properly!
The folder copy/move code is pretty twisted, but I'll start looking into it and will report back...
Comment 63•2 years ago
|
||
The issue isn't moving the folder to a new parent, in fact, as we understand it, all moves will trigger onFolderAdded() in the folder listener in about3pane.js. The issue is that at the time of setting the sort order in the cache, in the special case of re-parenting, the cache entry doesn't exist yet for some reason. If onFolderAdded() were called after a cache entry was created by some other part of the system, there wouldn't be a problem. That's why our suggested (band-aid) solution creates the cache entry when needed. Otherwise some investigation would be required to determine why onFolderAdded() is called so/too early.
| Assignee | ||
Comment 64•2 years ago
|
||
(In reply to Ben Campbell from comment #62)
I don't entirely follow the JS side of things, but there's really two separate operations that could be performed:
- dragging to reorder a folder within the level it's already in (ie just changing the userSortOrder)
- dragging a folder so it now resides under another parent (ie full-blown folder move/copy)
Have I got that right?
Exactly.
The operation to insert the dragged folder to the desired position is the same,
except that in the case of 2, MailServices.copy.copyFolder is called beforehand (with isMove=true).
This will trigger the onFolderAdded callback, but then the folder will look brand new.
Yes, I hope a clean solution can be found.
Comment 65•2 years ago
|
||
(In reply to betterbird.project+13 from comment #63)
The issue is that at the time of setting the sort order in the cache, in the special case of re-parenting, the cache entry doesn't exist yet for some reason. If onFolderAdded() were called after a cache entry was created by some other part of the system, there wouldn't be a problem.
Agree - that's what I think the bug is. I'd consider the caching to be part of a valid folder, so if onFolderAdded() is called before the folder's cache is sorted out, then I'd consider that to be an incompletely initialised folder.
I'll be curious to find out if any of the existing folder move/copy code even attempts to do anything with folder caching! I'd suspect not, but you never know... in any case I'll do some investigation tomorrow.
(For my own reference: the onFolderAdded() is generated by calls to NotifyFolderAdded(), so that's the place to start looking. It's usually called from the folder creation functions, which are done differently for all the different folder types. But anything to do with caching will probably be up one level from the folder creation functions, if it even exists. Rename operations (and presumably move operations) are handled by pairing NotifyFolderdeleted() with NotifyFolderAdded()).
Comment 66•2 years ago
|
||
Based on this discussion, we tried the following:
NS_IMETHODIMP nsMsgDBFolder::NotifyFolderAdded(nsIMsgFolder* child) {
+ // Make sure the child has a cache entry.
+ nsCOMPtr<nsIFile> dbPath;
+ static_cast<nsMsgDBFolder*>(child)->GetFolderCacheKey(getter_AddRefs(dbPath));
+ if (dbPath) {
+ nsCOMPtr<nsIMsgFolderCacheElement> dummy;
+ GetFolderCacheElemFromFile(dbPath, getter_AddRefs(dummy), true);
+ }
+
NOTIFY_LISTENERS(OnFolderAdded, (this, child));
// Notify listeners who listen to every folder
nsresult rv;
to create a cache entry before any listeners get notified. It took a while to work out that this approach doesn't work since creating a cache entry here:
https://searchfox.org/comm-central/rev/76be3830816c8a07044e4d19eb1a31685ec5a2ff/mailnews/base/src/nsMsgDBFolder.cpp#617
doesn't actually create any entry in the cache, see:
https://searchfox.org/comm-central/rev/76be3830816c8a07044e4d19eb1a31685ec5a2ff/mailnews/base/src/nsMsgFolderCache.cpp#305-315
and the constructor:
https://searchfox.org/comm-central/rev/76be3830816c8a07044e4d19eb1a31685ec5a2ff/mailnews/base/src/nsMsgFolderCache.cpp#40-41
Only when a value is set on the cache element, it is put into the cache. Therefore the patch proposed in comment #60 seems to be the only solution since it creates the entry and then immediately sets a value on it.
While debugging, we also noticed bug 1869284.
Comment 67•2 years ago
|
||
I've written up what I think is the correct fix in Bug 1870463.
Sorry for the delayed response, but I'm traveling now. So I'll be in and out of here over the next few weeks. I'm happy to work on it, but it'll likely be slow going.
Updated•2 years ago
|
Comment 68•2 years ago
|
||
Turned out that a local trash folder gets replaced with a new trash folder upon emptying and extra code is necessary to carry over the sort order:
https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1846550-manually-sort-folders-fix-trash.patch
Comment 69•2 years ago
|
||
We found another usability issue with the current implementation: Once in a folder a subfolder has been re-ordered, even accidentally, new subfolders get added at the end, which will be puzzling to users with a long folder list who might even have forgotten that they reordered once upon a time. Example: Start with subfolders A, Y, Z, move Z before Y and back. If D is added now, it goes after Z.
Some solution ideas:
- An option to restore the natural order
- The system detects the natural order
- Despite some manual reordering, the system still inserts folders into natural order, not at the end.
Comment 70•2 years ago
|
||
Here's an implementation of point 3:
https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1846550-manually-sort-folders-fix-new-folder.patch
| Assignee | ||
Comment 71•2 years ago
|
||
(In reply to Ben Campbell from comment #67)
I've written up what I think is the correct fix in Bug 1870463.
Sorry for the delayed response, but I'm traveling now. So I'll be in and out of here over the next few weeks. I'm happy to work on it, but it'll likely be slow going.
Two months have passed. What is the progress?
Comment 72•2 years ago
|
||
I'm still on Thundebird 1.02.15 waiting for this to be fixed.
Thanks for working on it.
Comment 73•2 years ago
|
||
(In reply to itagagaki from comment #71)
Two months have passed. What is the progress?
Haven't forgotten about it, just been fighting fires elsewhere :-(
I'll leave this needinfo active and aim to take a look at it next week!
| Comment hidden (metoo) |
Comment 75•2 years ago
|
||
(In reply to Ben Campbell from comment #73)
(In reply to itagagaki from comment #71)
Two months have passed. What is the progress?
Haven't forgotten about it, just been fighting fires elsewhere :-(
I'll leave this needinfo active and aim to take a look at it next week!
Hi Ben, two more months went by. Do you have any updates on that?
Comment 76•2 years ago
|
||
I know that the work on Thunderbird is done by volunteers. Nevertheless, promises made should be kept promptly. If there are no ideas or basically no time, then this should also be communicated. I think many users are waiting for a development in one direction or another. Either this possibility should be available in Thunderbird itself or the existing extension should be adapted accordingly. Many users would be grateful!
Comment 77•2 years ago
|
||
(In reply to Boersenfeger from comment #76)
I know that the work on Thunderbird is done by volunteers. Nevertheless, promises made should be kept promptly. If there are no ideas or basically no time, then this should also be communicated. I think many users are waiting for a development in one direction or another. Either this possibility should be available in Thunderbird itself or the existing extension should be adapted accordingly. Many users would be grateful!
I'm sorry, but this is patently ridiculous. Do you have the time, knowledge, and resources to jump in and help? If so, then great, help! Otherwise, leave the poor people who are, as you correctly state, volunteers alone.
No one should be trying to shame volunteers for not working fast enough to please someone else. Taking advantage of volunteers' desire to help by telling them they're not living up to some arbitrary standard is how things like the xz utils backdoor happen.
Remember the human and keep in mind that people have mouths to feed and roofs to maintain over their heads and their families' heads while you are badgering them for a fix to something that, while it is annoying, is not something that is breaking Thunderbird.
Comment 78•2 years ago
|
||
(In reply to thenterprises2 from comment #77)
(In reply to Boersenfeger from comment #76)
I know that the work on Thunderbird is done by volunteers. Nevertheless, promises made should be kept promptly. If there are no ideas or basically no time, then this should also be communicated. I think many users are waiting for a development in one direction or another. Either this possibility should be available in Thunderbird itself or the existing extension should be adapted accordingly. Many users would be grateful!
I'm sorry, but this is patently ridiculous. Do you have the time, knowledge, and resources to jump in and help? If so, then great, help! Otherwise, leave the poor people who are, as you correctly state, volunteers alone.
No one should be trying to shame volunteers for not working fast enough to please someone else. Taking advantage of volunteers' desire to help by telling them they're not living up to some arbitrary standard is how things like the xz utils backdoor happen.
Remember the human and keep in mind that people have mouths to feed and roofs to maintain over their heads and their families' heads while you are badgering them for a fix to something that, while it is annoying, is not something that is breaking Thunderbird.
Also, keep in mind that:
'No abusing people. Constant and intense critique is one of the reasons we build great products. It's harder to fall into group-think if there is always a healthy amount of dissent. We want to encourage vibrant debate inside of the Mozilla community, we want you to disagree with us, and we want you to effectively argue your case. However, we require that in the process, you criticize things, not people. Examples of things include: interfaces, algorithms, and schedules. Examples of people include: developers, designers, and users. Attacking or encouraging attacks on a person may result in you being banned from Bugzilla.
'No obligation. "Open Source" is not the same as "the developers must do my bidding." Everyone here wants to help, but no one else has any obligation to fix the bugs you want fixed. Therefore, you should not act as if you expect someone to fix a bug by a particular date or release. Aggressive or repeated demands will not be received well and will almost certainly diminish the impact of and interest in your suggestions.'
(from the etiquette guide)
Comment 79•2 years ago
|
||
Just to note, I am a paid employee of MZLA, who does work on TB full time, so this is entirely my fault, and you're most welcome to hassle me mercilessly for it!
I shouldn't have over-promised on this - there's been a lot of other stuff going on and I just haven't had the chance to sort out Bug 1870463 yet.
It's a cool feature to get in, and itagagaki has been doing some great work in this area and he deserves to be supported.
Just bear with me a little longer and I will sort it out!
Comment 80•2 years ago
|
||
Hi Ben, I didn't mean to bully anyone! I took the liberty of expressing a criticism, if this was taken as harassment, I apologize!
By the way, I think @thenterprises2 reaction to this is completely over the top. I have now learned that there are people like you who work full-time for the project, thank you for that! :-) I am now waiting patiently for you to find the time to take care of it. Best regards.
To the others who have written to me privately: It is no longer necessary, I have realized that people here are obviously very sensitive and will take this into account in future posts. Have a good time everyone!
Comment 81•2 years ago
|
||
(In reply to Ben Campbell from comment #79)
Just to note, I am a paid employee of MZLA, who does work on TB full time, so this is entirely my fault, and you're most welcome to hassle me mercilessly for it!
I shouldn't have over-promised on this - there's been a lot of other stuff going on and I just haven't had the chance to sort out Bug 1870463 yet.
It's a cool feature to get in, and itagagaki has been doing some great work in this area and he deserves to be supported.
Just bear with me a little longer and I will sort it out!
Thanks for clarifying, Ben!
Updated•2 years ago
|
| Assignee | ||
Comment 82•2 years ago
|
||
The folder cache bug that was blocking was fixed, right?
So I applied the patch to the latest code in the repository again. However, it does not work as expected. Analysis is needed.
| Assignee | ||
Comment 83•2 years ago
|
||
Bug 1870463, which was a blocker, is still open, but I believe it is no longer a blocker and have rebased my patch. It took a bit work though, as there were changes and conflicts in about3Pane.js during that time.
But not only conflicts, there was also a new blocker, bug 1901386. This makes it difficult to check if my patch is correct. It has not been assigned to anyone yet, but hopefully someone will fix this degradation soon. After that, I will request a review if I can confirm that my patch works as expected.
Comment 84•2 years ago
|
||
It looks like the last blocker has been fixed.
@itagagaki, I hope you have some luck in getting your enhancement patches into release builds soon. Your work and persistence are very much appreciated. I've been through the process of submitting much simpler patches and the ordeal has put me off for good. Contributing bug fixes to Thunderbird has too much red tape in the way. New features must be even harder. I don't know how to effectively provide that feedback to Thunderbird/Mozilla policy makers, but the process discourages participation.
I am also surprised that this core feature has only received 10 Bugzilla votes. I bet that this is such a fundamental features that it's omission puts off potential Thunderbird users before they become committed enough to using Thunderbird, let alone creating as Bugzilla account and voting.
Again, thank you for working so hard to bring everyone this feature.
Comment 85•2 years ago
|
||
(In reply to itagagaki from comment #83)
But not only conflicts, there was also a new blocker, bug 1901386. This makes it difficult to check if my patch is correct. It has not been assigned to anyone yet, but hopefully someone will fix this degradation soon. After that, I will request a review if I can confirm that my patch works as expected.
The fix for this should appear in today's daily, and in next Monday's beta.
| Assignee | ||
Comment 86•2 years ago
|
||
Perhaps a good example of how user interest and developer interest don't always align is that my post for the Mozilla Connect community has 18x more kudos. I think that's what got the Mozilla folks going and kept me motivated.
https://connect.mozilla.org/t5/ideas/thunderbird-add-ability-to-manually-sort-order-folders-in-folder/idi-p/39061
I have confirmed the fix for bug 1901386. But unfortunately, my feature has not yet returned to the expected behavior.
This weekend will not be boring..
Comment 87•2 years ago
|
||
(In reply to itagagaki from comment #86)
I think that's what got the Mozilla folks going and kept me motivated.
https://connect.mozilla.org/t5/ideas/thunderbird-add-ability-to-manually-sort-order-folders-in-folder/idi-p/39061
I'm glad it kept you motivated but from MZLA's side what got things moving is getting past my unfortunate misunderstanding that I thought you were trying to implement an addon (which we sorted out in a private conversation - sorry for the misunderstanding), and then patiently connecting and working with the right people in conversation with an agreed upon goal (as I stated in that private conversation).
Thanks to everyone for all the hard work.
Comment 88•2 years ago
|
||
I'm also very impressed and thankful that itagagaki keeps going :) To be honest, I don't even know how to vote or give kudos here in bugzilla...?!? I must be blind as I don't find anything like that in this page...
Comment 89•2 years ago
|
||
(In reply to Frank Steiner from comment #88)
I'm also very impressed and thankful that itagagaki keeps going :) To be honest, I don't even know how to vote or give kudos here in bugzilla...?!? I must be blind as I don't find anything like that in this page...
You are not. It is well hidden. I also had to ask to find it. Expand the details section, there is a button to open the voting dialog.
Comment 90•2 years ago
|
||
Thanks! Well, then missing votes don't mean anything if people don't know about and, even if they know, can't find the function :-)
Comment 91•2 years ago
|
||
(In reply to Frank Steiner from comment #90)
Thanks! Well, then missing votes don't mean anything if people don't know about and, even if they know, can't find the function :-)
They likely don't mean much anyway. If you click the underlined 'Votes' link, you'll see that "Votes are not used by Mozilla developers to set priorities."
Comment 92•2 years ago
|
||
We have all the necessary patches rebased to 128 here, look for 1846550. As far we can tell, it all works based on 128.
| Assignee | ||
Comment 93•2 years ago
|
||
OK. I'll write down what currently happens below.
There are folders A and B.
Folder A has subfolders C and D.
Let's grab B and move (insert) it between C and D.
In JS, first make B a subfolder of A with transferFolder().
Then, call insertFolder() to change the userSortOrder of each folder so that the order is C-B-D.
This is how the patch I made 8 months ago works.
What happens now with this procedure?
transferFolder() calls MailServices.copy.copyFolder().
Then, apparently, B is re-created and added to A.
This is asynchronous and does not make it to transferFolder() in time for transferFolder().
Then, after some time, JS detects the new folder and sets a default value as its userSortOrder. This is the first problem.
I am not sure why it was fine 8 months ago.
So if we stop making copyFolder() asynchronous and make it a synchronous process, will that solve the problem?
No. B, which is a child of A as a result of copyFolder(), becomes a different object from the original B, which was a sibling of A.
However, since insertFolder() handles the original, it is not reflected in the new B.
If you have any ideas how to solve this, please let me know.
| Assignee | ||
Comment 94•2 years ago
|
||
This is asynchronous and does not make it to transferFolder() in time for transferFolder().
This is asynchronous and will not make it in time for transferFolder().
Comment 95•2 years ago
|
||
Works here on local folders, but not on IMAP folders. On IMAP folders, with the same drag operation, C goes before D, not between D and E.
I am not sure why it was fine 8 months ago.
Looks like it wasn't fine. We have the old set of patches in our version 115 and the same issue occurs.
| Assignee | ||
Comment 96•2 years ago
|
||
When MailServices.copy.copyFolder() is called to move a folder to another level, a new folder is created, but the original userSortOrder is not inherited and NO_SORT_VALUE is set and onFolderAdded() is is called after the NO_SORT_VALUE is set.
Similar to the issue from comment #58. But then that should have been fixed in Bug 1870463, right?
It didn't work with or without applying caching-additional-changes2.patch from Betterbird.
Next, let's look at the problem with IMAP.
transferFolder() calls MailServices.copy.copyFolder() asynchronously with setTimeout. If I replace it with a simple call, it returns after the copy is done for local folders. For IMAP, however, it returns before the copy is done. The copy is delayed. It seems to be asynchronous internally.
Should I prepare a listener and wait for onStopCopy? But onStopCopy doesn't know which folder or where the instruction came from. So I don't think it makes sense. Or is there another way?
Comment 97•2 years ago
|
||
On further testing and inspection of the code, there seems to be a logic error when moving a folder to a different parent. In 128 it doesn't work for local folders any more, likely due to timing changes between 115 and 128. This patch fixes the issue for local and IMAP folders.
| Assignee | ||
Comment 98•2 years ago
|
||
I was just working on rebuilding the logic as well, but your way may be smarter. I will consider adopting it.
| Assignee | ||
Comment 99•2 years ago
|
||
Okay. I did a quick test and it seems to work. I'm still a little nervous, but I'll have mozphab review it now.
I think the following are issues for the future.
- Better folder caching mechanism.
- Make sure all properties are inherited when copying folders
- Allow processing to continue on a folder after an asynchronous move
| Assignee | ||
Comment 100•2 years ago
|
||
Updated•2 years ago
|
Comment 101•2 years ago
|
||
Would you mind adding the complete attribution to your patch:
Folder cache:
https://github.com/Betterbird/thunderbird-patches/blob/116973bd79c366adc54aa9be4f7215e3b4e9f9cd/115/bugs/1864510-performance-bug-1846550.patch
https://github.com/Betterbird/thunderbird-patches/blob/main/128/bugs/1846550-manually-sort-folders-additional-caching-changes.patch
Move to different parent:
https://github.com/Betterbird/thunderbird-patches/blob/main/128/bugs/1846550-manually-sort-folders-fix-logic-error.patch
Drag and drop:
https://github.com/Betterbird/thunderbird-patches/blob/main/128/bugs/1846550-manually-sort-folders-additional-dnd-fix.patch
Fixing local trash:
https://github.com/Betterbird/thunderbird-patches/blob/main/128/bugs/1846550-manually-sort-folders-fix-trash.patch
Adding a new folder semi-alphabetically:
https://github.com/Betterbird/thunderbird-patches/blob/main/128/bugs/1846550-manually-sort-folders-fix-new-folder.patch
https://github.com/Betterbird/thunderbird-patches/blob/main/128/bugs/1846550-manually-sort-folders-fix-new-folder2.patch
These seven patches are our work.
Comment 102•1 year ago
|
||
Did this get any closer to being merged in the last 3 months? It seems like a lot of effort has been expended over a long period of time, it would be nice to get this across the line.
| Assignee | ||
Comment 103•1 year ago
|
||
(In reply to Peter Urbanec from comment #102)
Did this get any closer to being merged in the last 3 months? It seems like a lot of effort has been expended over a long period of time, it would be nice to get this across the line.
I have not been contacted about any of this, so I don't know for sure. However, some of their activity suggests that the Thunderbird team is now working on refactoring the folder structure/mechanism. I imagine they might revisit my ideas again when that is done, maybe. I can't be sure, it's just my imagination.
Comment 104•1 year ago
|
||
Alex is the major folder pane changes concluded?
Comment 105•1 year ago
|
||
Hi Itagagaki, apologies for the lack of reply.
We're currently actively working on bug 1921394, which is a completely new approach and rework of the folder database structure. A much more sane and clean implementation based on SQL that allows easy reordering out of the box.
We looked at your patch, thank you so much for the massive effort, but unfortunately a lot of the solutions carried over from Betterbird don't give us a lot of confidence. There are some hacks and workarounds that are needed due to our current folder implementation, and we feel that is more efficient to start a new system in parallel rather than trying to update the existing one with something that could very much introduce unwanted regressions.
Thank you again for your contribution.
Comment 106•1 year ago
|
||
That comment is surprising since what you landed initially (with confidence?) had to be backed out since it opened every single folder in the system on every start, see bug 1864510. We made the solution workable, indeed with many patches, and we've been shipping it since November 2023.
Comment 107•1 year ago
|
||
@betterbird.projekt+18
Hello, it would be nice if you could limit yourself to your project. This is about Thunderbird and not about your offshoot, if the TB developers wanted to work with you on the basis of your project, they would certainly have initiated it in other ways. Now in many posts here in Bugzilla and elsewhere on the net to emphasize your product Betterbird as the better and only working one is just annoying and does not bring the project Thunderbird one meter further. Thank you for understanding my criticism, but maybe I'm not alone.
TX Bo
Translated with DeepL.com
Comment 108•1 year ago
|
||
I vote to fix this bug, as well as the folder to open on startup
| Assignee | ||
Comment 109•1 year ago
|
||
Updated•1 year ago
|
| Assignee | ||
Comment 110•1 year ago
|
||
| Assignee | ||
Comment 111•1 year ago
|
||
The review of my latest revision is still ongoing, but I have fixed all the points raised.
However, there is one problem.
I did the following test last month (when I filed ⚙ D238193), and it worked fine then.
But when I tried it just now, I got a bad result.
The test steps:
- Copy subfolders in Local Folders.7z to <profile folder>/Mail/LocalFolders
- Run TB
- Verify that the timestamps of the .msf files have not been updated
This is probably related to what was reported in bug 1864510. I thought it had been resolved, but it has not (or has regressed).
Although I don't think it's related to the implementation of this issue (folder sorting feature), it was one of the factors that hindered the launch of this feature, so it should not be overlooked.
| Assignee | ||
Comment 112•1 year ago
|
||
I did the test I described in comment #111 again.
It took 7.5 seconds to start TB after adding 810 folders in my environment. But after that, it takes no time to start TB.
This means that the speed issue that was previously an issue has been resolved.
However, I am a little concerned about the updated timestamps in the newly discovered .msf files. Is this not a problem?
| Assignee | ||
Comment 113•1 year ago
|
||
I have done additional testing (the same test that I described in comment #111) on the following three TB Daily/Nightly versions.
https://ftp.mozilla.org/pub/thunderbird/nightly/2025/02/2025-02-12-10-04-58-comm-central/thunderbird-137.0a1.en-US.win64.zip
--> The timestamps have been updated and the startup is delayed about 4 or 5 seconds.
https://ftp.mozilla.org/pub/thunderbird/nightly/2023/11/2023-11-21-11-13-01-comm-central/thunderbird-122.0a1.en-US.win64.zip
--> The timestamps have been updated and the startup is delayed about 1 or 2 seconds.
https://ftp.mozilla.org/pub/thunderbird/nightly/2023/11/2023-11-15-10-54-40-comm-central/thunderbird-121.0a1.en-US.win64.zip
--> The timestamps have been updated and the startup is delayed about 2 or 3 seconds.
Since the bug 1864510 was closed, and until recently (before my this patch was applied), the behavior is almost the same. It takes a few seconds when there are newly discovered folders in LocalFolders. And the timestamps of the newly discovered folders are updated.
Then I measured the time again with my patch applied and it was about 5 seconds.
So this delay issue (and the .msf files being touched issue) seems does not seem to be related to my patch (this folder sorting feature).
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
| Assignee | ||
Comment 114•1 year ago
|
||
On Bugzilla, this bug is still depends on some bugs, but is not an obstacle to this new feature. I want to ship it now.
Comment 115•1 year ago
|
||
We can remove the last 2 blockers since they're not relevant for this feature anymore.
Comment 116•1 year ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/61cd87a2f6d0
add ability to manually sort (order) folders in folder pane. r=aleca,darktrojan,freaktechnik
Comment 117•1 year ago
|
||
Congrats itagagaki for getting this into the tree! Perhaps there are other bugs in this area that you'd like to work on? ;-)
| Assignee | ||
Comment 118•1 year ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #117)
Congrats itagagaki for getting this into the tree! Perhaps there are other bugs in this area that you'd like to work on? ;-)
Thanks to all who helped. And yeah, I will of course work on them in a small way when I feel like it :-)
Updated•1 year ago
|
Comment 119•1 year ago
|
||
Release Note Request
Why is this notable: Manually sort folders feature.
Suggested wording: Implemented the ability to manually sort folders in the folder pane.
[Links (documentation, blog post, etc)]:
Updated•1 year ago
|
Updated•1 year ago
|
Description
•