436630 - Thunderbird should not use the rdf-infected msgFolderPickerOverlay

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: part 1 v1

There are a bunch of rdf-driven templates in this file whose consumers we should port to the new js driven folder-binding.  Neil has commented that in many cases, these consumers probably want a flat tree-like list of folders (and I tend to agree), but for the moment, this patch aims simply for UI parity.  Once we have a js-driven flat-tree binding, we can discuss the proper UI.

Attached is part 1 of this, since removing the entire overlay in 1 patch would be a bit much.  This patch gets rid of consumers of "msgNewFolderPicker."  Also, the rename picker, and the 2nd action picker appear to be completely unused.  Part 2 will hopefully get rid of the remaining elements.

Comment 1

[Joey Minta]

Attachment: patch part 2 v1

Ok, we'll do this in 3 parts.  This part takes care of several other pickers used in the Account Settings window.  Part 3 will take care of the remaining 3 pickers.

Comment 2

[David :Bienvenu]

Comment on attachment 323179 [details] [diff] [review]
part 1 v1

I built and ran with this patch - when I tried to use the folder picker in the new folder and new saved search dialog, I saw 8 copies of each server in the top level menu, mostly consecutive, but some sprinkled here and there, depending on the account. I'm slightly out of date, so I'll see if rebuilding from the tip helps.

Comment 3

[David :Bienvenu]

Comment on attachment 323179 [details] [diff] [review]
part 1 v1

I rebuilt, and still have the problem. I happen to have 8 accounts total, which I doubt is a coincident. This is on OSX.

What I see in the menu is 8 consecutive items containing my default server (imap), then a pop3, a gmail, and an rss account take turns alternating 8 times, then an imap account appears just once (my newest account, fwiw), then local folders appears 8 times. One of my pop3 accounts uses the Global Inbox.

I'll poke around a little, but I don't have Venkman working on this machine.

Comment 4

[David :Bienvenu]

switching my global inbox account to a normal per-server inbox made that account appear 8 times in the folder picker.

I didn't see anything too interesting in the error console.

Comment 5

[David :Bienvenu]

when I backed out this patch (part 1), everything worked normally, so I don't think my problem was caused by all the nsISupportsArray exorcisms. Do you have multiple accounts in your profile?

Comment 6

[David :Bienvenu]

Comment on attachment 323179 [details] [diff] [review]
part 1 v1

turns out my prefs.js had many duplicate accounts in the accounts pref, which RDF "fixes" by removing dups. When I cleaned up my prefs.js, things were OK. If it gets messed up again, I'll see it pretty quickly once this patch lands.

One problem that needs to be fixed - getStringProperty will throw an exception if the property isn't stored, and there's no guarantee that MRUTime is set, so you need a try catch here:

            try {
              var time = aFolder.getStringProperty("MRUTime") || 0;
              if (time <= oldestTime)
                return;
            }
            catch (ex) {return;}

r/sr=bienvenu with that fixed. Sorry that my messed up prefs.js held up this patch.

Opening the move/copy context menu is rather slow the first time, with my debug build. I wonder if we're opening the db's for all the folders. I think I'll check that out really quickly...

Comment 7

[Joey Minta]

Part 1 checked in.  Regression testing should look at the folder-picker menus in the New Folder dialog and in the Save Search as Folder dialog.  Selecting various folders there should create the new folders in their proper locations.

Checking in mail/base/content/mail-folder-bindings.xml;
/cvsroot/mozilla/mail/base/content/mail-folder-bindings.xml,v  <--  mail-folder-bindings.xml
new revision: 1.5; previous revision: 1.4
done
Checking in mail/base/content/mailCommands.js;
/cvsroot/mozilla/mail/base/content/mailCommands.js,v  <--  mailCommands.js
new revision: 1.50; previous revision: 1.49
done
Checking in mail/base/content/mailWindowOverlay.js;
/cvsroot/mozilla/mail/base/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.202; previous revision: 1.201
done
Checking in mail/base/content/widgetglue.js;
/cvsroot/mozilla/mail/base/content/widgetglue.js,v  <--  widgetglue.js
new revision: 1.25; previous revision: 1.24
done
Checking in mailnews/base/resources/content/mailCommands.js;
/cvsroot/mozilla/mailnews/base/resources/content/mailCommands.js,v  <--  mailCommands.js
new revision: 1.115; previous revision: 1.114
done
Checking in mailnews/base/resources/content/mailWindowOverlay.js;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.js,v  <--  mailWindowOverlay.js
new revision: 1.269; previous revision: 1.268
done
Checking in mailnews/base/resources/content/msgFolderPickerOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/msgFolderPickerOverlay.xul,v  <--  msgFolderPickerOverlay.xul
new revision: 1.49; previous revision: 1.48
done
Checking in mailnews/base/resources/content/newFolderDialog.js;
/cvsroot/mozilla/mailnews/base/resources/content/newFolderDialog.js,v  <--  newFolderDialog.js
new revision: 1.9; previous revision: 1.8
done
Checking in mailnews/base/resources/content/newFolderDialog.xul;
/cvsroot/mozilla/mailnews/base/resources/content/newFolderDialog.xul,v  <--  newFolderDialog.xul
new revision: 1.20; previous revision: 1.19
done
Checking in mailnews/base/resources/content/virtualFolderProperties.js;
/cvsroot/mozilla/mailnews/base/resources/content/virtualFolderProperties.js,v  <--  virtualFolderProperties.js
new revision: 1.16; previous revision: 1.15
done
Checking in mailnews/base/resources/content/virtualFolderProperties.xul;
/cvsroot/mozilla/mailnews/base/resources/content/virtualFolderProperties.xul,v  <--  virtualFolderProperties.xul
new revision: 1.7; previous revision: 1.6
done
Checking in mailnews/base/resources/content/widgetglue.js;
/cvsroot/mozilla/mailnews/base/resources/content/widgetglue.js,v  <--  widgetglue.js
new revision: 1.177; previous revision: 1.176
done
Checking in mailnews/base/search/resources/content/SearchDialog.js;
/cvsroot/mozilla/mailnews/base/search/resources/content/SearchDialog.js,v  <--  SearchDialog.js
new revision: 1.108; previous revision: 1.107
done

Comment 8

[David :Bienvenu]

Comment on attachment 323190 [details] [diff] [review]
patch part 2 v1

two problems:

I think aEvent wants to be event here
-                        oncommand="noteSelectionChange('tmpl_selectFolder')"/>
+                        oncommand="noteSelectionChange('tmpl_selectFolder', aEvent)">
+                <menupopup id="msgTemplFolderPopup" type="folder" mode="filing"
+                           showFileHereLabel="true"
+                           fileHereLabel="&filemessageschoosethis.label;"/>

Here I get an exception when trying to ok out of account settings after picking an account for one of the specail folders:

-            picker = document.getElementById(accountPickerId);
-            uri = picker.getAttribute("uri");
+            uri = document.getElementById(accountPickerId).selectedItem.folder.URI;


selectedItem.folder is undefined.

Comment 9

[David :Bienvenu]

When editing a cross-folder saved search, and trying to add a folder to the scope, I get the following exception when trying to ok out of the folder properties dialog:

* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "gPickedFolder is undefined" {file: "chrome://messenger/content/virtualFolderProperties.js" line: 170}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

I don't know if this patch is involved, or some other one. I'll investigate.

Comment 10

[Joey Minta]

Attachment: part 2 v2

Fixes the problems identified in the last review.

Comment 11

[David :Bienvenu]

Attachment: [checked in]fix regression editing saved searches

Comment 12

[David :Bienvenu]

I changed the create new saved search code to return before getting to the code that fails in the new saved search case.

Comment 13

[David :Bienvenu]

Comment on attachment 324212 [details] [diff] [review]
part 2 v2

now, picking a non-default folder doesn't persist - I don't see anything in the js console, though. I tried changing the template folder to a random folder on an other account, and when I went back to the copies and folders page, the templates folder was set back to the default templates folder on the default server.

Comment 14

[Joey Minta]

Attachment: part 2 v3

Stupid error swallowing... This sets the right values to the global variables in question.

Comment 15

[David :Bienvenu]

Comment on attachment 324216 [details] [diff] [review]
part 2 v3

ok, seems to be working now, thx.

Comment 16

[Joey Minta]

Part 2 checked in.  Regression testing should test selecting folders for various preferences in the copies and server panes of the account manager.

Checking in mailnews/base/prefs/resources/content/am-copies.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-copies.js,v  <--  am-copies.js
new revision: 1.37; previous revision: 1.36
done
Checking in mailnews/base/prefs/resources/content/am-copiesOverlay.xul;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-copiesOverlay.xul,v  <--  am-copiesOverlay.xul
new revision: 1.8; previous revision: 1.7
done
Checking in mailnews/base/prefs/resources/content/am-server.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server.js,v  <--  am-server.js
new revision: 1.74; previous revision: 1.73
done
Checking in mailnews/base/prefs/resources/content/am-server.xul;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server.xul,v  <--  am-server.xul
new revision: 1.125; previous revision: 1.124
done
Checking in mailnews/base/resources/content/msgFolderPickerOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/msgFolderPickerOverlay.xul,v  <--  msgFolderPickerOverlay.xul
new revision: 1.50; previous revision: 1.49
done

Comment 17

[Joey Minta]

Attachment: part 3

This is part 3, which removes the remaining overlaid menuitems, and stops shipping the files.

Comment 18

[Joey Minta]

Patch checked in.  Regression testing should focus on selecting folders in the junk pane of the account manager, and on the advanced server options dialog in the manager.

Checking in mail/base/jar.mn;
/cvsroot/mozilla/mail/base/jar.mn,v  <--  jar.mn
new revision: 1.113; previous revision: 1.112
done
Checking in mail/base/content/mail-folder-bindings.xml;
/cvsroot/mozilla/mail/base/content/mail-folder-bindings.xml,v  <--  mail-folder-bindings.xml
new revision: 1.8; previous revision: 1.7
done
Checking in mailnews/base/prefs/resources/content/am-junk.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-junk.js,v  <--  am-junk.js
new revision: 1.14; previous revision: 1.13
done
Checking in mailnews/base/prefs/resources/content/am-junk.xul;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-junk.xul,v  <--  am-junk.xul
new revision: 1.9; previous revision: 1.8
done
Checking in mailnews/base/prefs/resources/content/am-server-advanced.js;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server-advanced.js,v  <--  am-server-advanced.js
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/base/prefs/resources/content/am-server-advanced.xul;
/cvsroot/mozilla/mailnews/base/prefs/resources/content/am-server-advanced.xul,v  <--  am-server-advanced.xul
new revision: 1.10; previous revision: 1.9
done
Checking in mailnews/base/resources/content/msgFolderPickerOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/msgFolderPickerOverlay.xul,v  <--  msgFolderPickerOverlay.xul
new revision: 1.51; previous revision: 1.50
done

With this patch, the bug is now fixed.

Comment 19

[Matthias Bockelkamp]

After opening the Copies & Folders page in my Mail & Newsgroups Account Settings (using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.2pre) Gecko/2008071103 SeaMonkey/2.0a1pre), i cannot use the account settings panel any more without first closing it. The error console shows "Error: document.getElementById(accountPickerId).selectedItem is null
Source File: chrome://messenger/content/am-copies.js
Line: 270". CVS log shows the last change to am-copies.js was in Part 2 of this bug, so maybe that is the cause.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> The error console shows "Error:
> document.getElementById(accountPickerId).selectedItem is null
> Source File: chrome://messenger/content/am-copies.js
> Line: 270".

That was bug 438647.

Comment 21

[:aceman]

TB is still using msgFolderPickerOverlay, e.g. in the Filter list window. Or does that not count as TB as it is in Mailnews core?