1513267 - nsIMsgFolder.Delete() does not update tree view

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Nick Iacovelli]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Steps to reproduce:

java script in scarcthpad

var acctMgr = Components.classes["@mozilla.org/messenger/account-manager;1"].getService(Components.interfaces.nsIMsgAccountManager);
var accounts = acctMgr.accounts;
var account = accounts.queryElementAt(0, Components.interfaces.nsIMsgAccount);
var enumerator = account.incomingServer.rootFolder.subFolders;
var folder = enumerator.getNext().QueryInterface(Components.interfaces.nsIMsgFolder);
console.log(folder.prettyName);
folder.Delete();


Actual results:

folder get deleted  on hard drive  but does not remove from folder tree



Expected results:

The tree should have been update.

Comment 1

[Jorg K (CEST = GMT+2)]

Maybe you need to send a refresh to the tree?

Comment 2

[Nick Iacovelli]

that can fix it can you tell me how.
i been going threw the commands some work some dont

Comment 3

[Wayne Mery (:wsmwk)]

resolved?

Comment 4

[Nick Iacovelli]

I don't know it been months and I move on
try it and see if the tree updates
I have many problem with tbird

Comment 5

[Wayne Mery (:wsmwk)]

Paul or Geoff, do you know of any issues here?

Comment 6

[Jorg K (CEST = GMT+2)]

I saw this in Geoff's NI queue. I think he's the wrong person to ask. I ran the commands from comment #0 and I can confirm that folder.Delete() doesn't remove the folder from the UI. I fact, clicking onto it after the deletion shows an empty folder.

I guess Aceman will know best about this issue.

Comment 7

[:aceman]

It seems to me this is OK, the Delete method is intended to only delete the local storage of the folder, not the folder itself. There are other methods to do that. Maybe we should rename the function and add at least some comment description to the .idl file.

Comment 8

[Geoff Lankow (:darktrojan)]

We should change the (AFAICT, two, in tests) uses of this method and mark it [noscript] in the IDL. It's not the right one to use.

Comment 9

[:aceman]

It does not need to be hidden from script if it is properly named/documented, then the bad uses shouldn't happen.

Comment 10

[:aceman]

Attachment: 1513267.patch

I also found recursiveDelete is strange function, it does not remove the folder from the children of its parent so e.g. if called from JS the folders remain visible in the folder pane.
Maybe it is also used for internal purposes, similar to Delete(). So I made it [noscript] too.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9104462 [details] [diff] [review]
1513267.patch

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

Looks OK to me. You also had a successful try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cd2e286e686c046a3c48fb9385f94122964e1c98

It's amazing how much confused code we can still find :-( - Declaring unused variables, and calling ForceDBClosed() after deleting it. Surely no error was checked in that case.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3441,5 @@
>    while (count > 0) {
>      nsIMsgFolder *child = mSubFolders[0];
>  
>      child->SetParent(nullptr);
> +    rv = child->RecursiveDelete(deleteStorage, msgWindow);  // recur

Remove // recur.

::: mailnews/local/src/nsMsgBrkMBoxStore.cpp
@@ +516,5 @@
>      if (msgParent) {
>        // The files have already been moved, so delete storage false
>        msgParent->PropagateDelete(aSrcFolder, false, aMsgWindow);
>        oldPath->Remove(false);  // berkeley mailbox
> +      // We need to force close the source db.

Remove comment.

Comment 12

[:aceman]

Attachment: 1513267.patch v1.1

Thanks.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c6454775a433
rename nsIMsgFolder::Delete method to deleteStorage (what it really does) and document the other folder deleting methods. r=jorgk