Open Bug 1846550 Opened 11 months ago Updated 6 days ago

add ability to manually sort (order) folders in folder pane

Categories

(Thunderbird :: Folder and Message Lists, enhancement)

enhancement

Tracking

(thunderbird_esr115 wontfix, thunderbird120 wontfix)

ASSIGNED
Tracking Status
thunderbird_esr115 --- wontfix
thunderbird120 --- wontfix

People

(Reporter: itagagaki, Assigned: itagagaki)

References

(Depends on 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

+++ 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.

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

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?

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... :)

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.

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.

The point is that this should be a core function and not an add-on, that's why itagagaki opened this bug here.

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.

(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.

(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.

Status: UNCONFIRMED → RESOLVED
Closed: 9 months ago
Duplicate of bug: 318467
Resolution: --- → DUPLICATE

(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

Status: RESOLVED → REOPENED
No longer duplicate of bug: 318467
Ever confirmed: true
Resolution: DUPLICATE → ---
Duplicate of this bug: 318467

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.

Version: Thunderbird 115 → unspecified

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.

(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

(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.

(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.

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

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.

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

https://twitter.com/itagagaki/status/1712126954785431987

The drag indicator has been further improved. It seems to work well now.
https://twitter.com/itagagaki/status/1713147694829720025

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.

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: nobody → itagagaki
Attachment #9359252 - Attachment description: WIP: Bug 1846550 - add ability to manually sort (order) folders in folder pane → Bug 1846550 - add ability to manually sort (order) folders in folder pane r=darktrojan,freaktechnik,mkmelin
Target Milestone: --- → 121 Branch

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

Status: REOPENED → RESOLVED
Closed: 9 months ago8 months ago
Resolution: --- → FIXED
Regressions: 1864510

https://bugzilla.mozilla.org/show_bug.cgi?id=1848132 seems to have become a permanent failure following merge of this patch.

See Also: → 1848132
Regressions: 1864582

Let's back out 1e6c184038ca until the issues discovered have been sorted out.

Duplicate of this bug: 1864582
Attachment #9359252 - Attachment description: Bug 1846550 - add ability to manually sort (order) folders in folder pane r=darktrojan,freaktechnik,mkmelin → WIP: Bug 1846550 - add ability to manually sort (order) folders in folder pane

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.)
Attachment #9359252 - Attachment is obsolete: true

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.

(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.

Sorry, quite confusing: "itagagaki requested review of this revision. Wed, Nov 15, 3:45 PM".

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.

Attachment #9363694 - Attachment description: WIP: Bug 1846550 - add ability to manually sort (order) folders in folder pane → Bug 1846550 - add ability to manually sort (order) folders in folder pane r=BenC,darktrojan

Incorporated the fix from bug 1864510 comment 7 and awaiting review.

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

(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.

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.

(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.

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.

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

Good hint. Yes, it's related to the caching part. We debugged it, found the cause and will post an update soon.

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.

Merged and awaiting review.

(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.

It may not be a writing problem, but a different key to get it.

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.

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.

Maybe we should add mUserSortOrder to nsMsgDBFolder and keep the value?

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.

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

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

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.

  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.

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.

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.

Here are some minimal C++ changes that will solve the issue from comment #58.

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.

Flags: needinfo?(benc)

(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:

  1. dragging to reorder a folder within the level it's already in (ie just changing the userSortOrder)
  2. 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 calling nsMsgFolderCache::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...

Flags: needinfo?(benc)

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.

(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:

  1. dragging to reorder a folder within the level it's already in (ie just changing the userSortOrder)
  2. 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.

Flags: needinfo?(benc)

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

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.

Depends on: 1870463

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.

Flags: needinfo?(benc)

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

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:

  1. An option to restore the natural order
  2. The system detects the natural order
  3. Despite some manual reordering, the system still inserts folders into natural order, not at the end.

(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?

Flags: needinfo?(benc)

I'm still on Thundebird 1.02.15 waiting for this to be fixed.
Thanks for working on it.

(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!

(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?

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!

(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.

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

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!

Flags: needinfo?(benc)

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!

(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!

Target Milestone: 121 Branch → ---

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.

Depends on: 1901386

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.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: