Closed Bug 1513267 Opened 1 year ago Closed 5 months ago

nsIMsgFolder.Delete() does not update tree view

Categories

(Thunderbird :: Folder and Message Lists, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 72.0

People

(Reporter: nickhere, Assigned: aceman)

Details

Attachments

(1 file, 1 obsolete file)

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.
Maybe you need to send a refresh to the tree?
that can fix it can you tell me how.
i been going threw the commands some work some dont

resolved?

Flags: needinfo?(nickhere)
Whiteboard: [closeme 2019-10-01]

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

Flags: needinfo?(nickhere)
Component: Untriaged → Folder and Message Lists

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

Flags: needinfo?(paul)
Flags: needinfo?(geoff)
Whiteboard: [closeme 2019-10-01]

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.

Flags: needinfo?(paul)
Flags: needinfo?(geoff)
Flags: needinfo?(acelists)
Target Milestone: --- → Thunderbird 71.0
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: folder.delete does not update tree view → nsIMsgFolder.Delete() does not update tree view
Target Milestone: Thunderbird 71.0 → ---

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.

Flags: needinfo?(acelists)

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.

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

Attached patch 1513267.patch (obsolete) — Splinter Review

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.

Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #9104462 - Flags: review?(jorgk)
Attachment #9104462 - Flags: feedback?(geoff)
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.
Attachment #9104462 - Flags: review?(jorgk) → review+

Thanks.

Attachment #9104462 - Attachment is obsolete: true
Attachment #9104462 - Flags: feedback?(geoff)
Attachment #9104547 - Flags: review+
Attachment #9104547 - Attachment is patch: true

Pushed by mozilla@jorgk.com:
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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
You need to log in before you can comment on or make changes to this bug.