1001071 - XBLify folder-selection menus in FilterListDialog and SearchDialog.xul

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Philip Chee]

The recent contretemps in Bug 984948 brought the FilterListDialog to my attention. There are several enhancements in the Thunderbird version that we definitely would like. Instead of going for a big bang approach with a massive un-reviewable mega-patch. I intend to proceed in an incremental manner.

Since FilterListDialog and SearchDialog are relatively self-contained I'll initially focus on these two.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=FilterListDialog&filetype=regexp&who=&whotype=match&sortby=Date&hours=2&date=all&mindate=&maxdate=&cvsroot=%2Fcvsroot

Looking at Bonsai there are several Thunderbird bugs to port in CVS trunk.

Bug 413781 – XBLify folder-selection menus
Bug 422474 - Excise nsMsgFilterDataSource and friends
Bug 435804 - remaining rdf cleanup for the filter dialog
Bug 437860 - Lots of windows/dialogs declare the nc-rdf namespace when they don't need to

Porting these will bring us up to parity with Thunderbird as of CVS tip. After this I'll start looking at Thunderbird comm-central for patches to cherry pick.

What this bug/patch does:
1. XBLify folder-selection menus (FilterListDialog and SearchDialog). This is part of Bug 444913 which I'm splitting off.
2. Port Relevant bits from TB Bug 422474 - Excise nsMsgFilterDataSource and friends.
(Most of Bug 422474 is useless churn. The only thing of interest is to cache the filter list in gCurrentFilterList and remove currentFilterList() )
3. Port Bug 437860 Remove the unnecessary nc-rdf namespace from FilterListDialog.xul and SearchDialog.xul

Porting TB Bug 435804 (remaining rdf cleanup) is currently being handled in Bug 507676.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Proposed fix

> -    return this.mFilterList.filterCount;
> +    return this.mFilterList && this.mFilterList.filterCount || 0;
I was testing with a profile that had no filters defined yet

>            <menulist id="serverMenu"
>                      class="folderMenuItem"
>                      IsServer="true"
>                      IsSecure="false"
>                      ServerType="none"
> -                    datasources="rdf:msgaccountmanager rdf:mailnewsfolders"
> ........
> -            <menupopup/>
> +                    oncommand="onFilterServerClick(event.target._folder.server.serverURI)">
> +            <menupopup id="serverMenuPopup"
> +                       class="menulist-menupopup"
> +                       type="folder"
> +                       mode="filters"
> +                       expandFolders="nntp"
> +                       showFileHereLabel="true"
> +                       showAccountsFileHere="true"/>

I'm cheating slightly here. All these attributes come from the current Thunderbird tip. I presume that these allow the XBL widgets to work properly.

> +         <button id="fileMessageButton"
> +                 type="menu"
> +                 label="&moveButton.label;"
> +                 accesskey="&moveButton.accesskey;"
> +                 observes="file_message_button">
> +           <menupopup type="folder"
> +                      showFileHereLabel="true"
> +                      mode="filing"
> +                      fileHereLabel="&moveHereMenu.label;"
> +                      fileHereAccessKey="&moveHereMenu.accesskey;"
> +                      oncommand="MoveMessageInSearch(event.target);"/>
> +         </button>

And these as well.

Comment 2

[[e-mail]]

Comment on attachment 8412067 [details] [diff] [review]
Patch v1.0 Proposed fix

Does selectServer have to be changed to work with the new picker? (I can't remember whether it updates its label automatically or not.)

>+var gCurrentFilterList;
Getting rid of currentFilterList() is noise which belongs in a separate patch. Also you should be consistent as to whether you are using gCurrentFilterList or gFilterTreeView.filterList; having both will only cause confusion.

> function onEditFilter() 
> {
>   var selectedFilter = currentFilter();
>+  if (!selectedFilter)
[How can this be true? All the callers select a filter first.]

>-    gFilterTree.view.selection.select(position);
>+    gFilterTreeView.selection.select(position);
Again, more noise, but you normally have to go through the view accessor because that's how the selection gets created in the first place, although I suppose at some point you can assume the selection has been created.

>-          <menulist id="runFiltersFolder" disabled="true"/>
>+          <menulist id="runFiltersFolder"
>+                    disabled="true"
>+                    class="folderMenuItem"
>+                    displayformat="verbose"/>
What does this change do?

>+                      oncommand="MoveMessageInSearch(event.target);"/>
Don't you have to update MoveMessageInSearch to expect a _folder?

>+         </button>
> 
>       <button id="deleteButton"
Indent is all wrong in this file but the following buttons appear to be at the correct indent, so might as well copy that indent for your changed buttons.

Comment 3

[Philip Chee]

Attachment: Patch v1.1 back to the basics.

> Does selectServer have to be changed to work with the new picker? (I can't 
> remember whether it updates its label automatically or not.)
Seems to work as is. I think further changes will be in Bug 507676 (Port |Bug 435804 - Remaining rdf cleanup for FilterListDialog| to SeaMonkey)

>>+var gCurrentFilterList;
> Getting rid of currentFilterList() is noise which belongs in a separate patch. 
> Also you should be consistent as to whether you are using gCurrentFilterList
> or gFilterTreeView.filterList; having both will only cause confusion.
OK. Splitting off into a separate patch

>> function onEditFilter() 
>> {
>>   var selectedFilter = currentFilter();
>>+  if (!selectedFilter)
> [How can this be true? All the callers select a filter first.]
Backed out.

>>-    gFilterTree.view.selection.select(position);
>>+    gFilterTreeView.selection.select(position);

> Again, more noise, but you normally have to go through the view accessor
> because that's how the selection gets created in the first place, although I
> suppose at some point you can assume the selection has been created.
Backed out.

>>-          <menulist id="runFiltersFolder" disabled="true"/>
>>+          <menulist id="runFiltersFolder"
>>+                    disabled="true"
>>+                    class="folderMenuItem"
>>+                    displayformat="verbose"/>
> What does this change do?

From Bug 878805:
> 1. meunulist has class="folderMenuItem" to use the icon and ensure box size uniformity.
....
>+         - This function returns a formatted display name for a menulist
>+         - selected folder.  The desired format is set as the 'displayformat'
>+         - attribute of the folderpicker's <menulist>, one of:
>+         - 'name' (default) - Folder
>+         - 'verbose'        - Folder on Account
>+         - 'path'           - Account/Folder/Subfolder

>>+                      oncommand="MoveMessageInSearch(event.target);"/>
> Don't you have to update MoveMessageInSearch to expect a _folder?
Thunderbird didn't change their MoveMessageInSearch().
Do you mean something like:
  oncommand="MoveMessageInSearch(event.target._folder)
And modifying MoveMessageInSearch() to take a folder?

>>+         </button>
>> 
>>       <button id="deleteButton"

> Indent is all wrong in this file but the following buttons appear to be at the 
> correct indent, so might as well copy that indent for your changed buttons.
Re-indented.

> -        var destMsgFolder = GetMsgFolderFromUri(destUri).QueryInterface(Components.interfaces.nsIMsgFolder);
> +        var destMsgFolder = MailUtils.getFolderForURI(destUri)
> +                                     .QueryInterface(nsIMsgFolder);
GetMsgFolderFromUri() just calls MailUtils.getFolderForURI().

> -          src="chrome://messenger/content/threadPane.js"/>
Imported by some overlay or other.

> -            <tree id="threadTree" flex="1" context="threadPaneContext"/>
> ...
> +      <tree id="threadTree"/>
Not sure how the context menu was supposed to work. Removed.

Comment 4

[[e-mail]]

(In reply to Philip Chee from comment #3)
> > >+                      oncommand="MoveMessageInSearch(event.target);"/>
> > Don't you have to update MoveMessageInSearch to expect a _folder?
> Thunderbird didn't change their MoveMessageInSearch().
> Do you mean something like:
>   oncommand="MoveMessageInSearch(event.target._folder)
> And modifying MoveMessageInSearch() to take a folder?

No, I was just expecting that MoveMessageInSearch would need to use the ._folder instead of messing around with RDF (via GetMsgFolderFromURI).

> > -          src="chrome://messenger/content/threadPane.js"/>
> Imported by some overlay or other.
[threadPane.xul]

> 
> > -            <tree id="threadTree" flex="1" context="threadPaneContext"/>
> > ...
> > +      <tree id="threadTree"/>
> Not sure how the context menu was supposed to work. Removed.
[Probably too much copy & paste when the dialog was first created.]

Comment 5

[Philip Chee]

Attachment: Patch v1.2 Use the _folder property.

> No, I was just expecting that MoveMessageInSearch would need to use the
> ._folder instead of messing around with RDF (via GetMsgFolderFromURI).
I've made MoveMessageInSearch() use the ._folder property. Seems to be working.

Comment 6

[[e-mail]]

Comment on attachment 8415917 [details] [diff] [review]
Patch v1.2 Use the _folder property.

>+    return this.mFilterList && this.mFilterList.filterCount || 0;
[Ah yes, the row count can get queried before a filter list has been found.]

>+                       oncommand="onFilterServerClick(event.target._folder.server.serverURI)"/>
This is wrong, you need the folder's URI rather than the server's, otherwise news filters don't work.
[At some point we should just use the folder everywhere, now that we have it, but that's probably better done in a follow-up bug.]

>+          <menulist id="runFiltersFolder"
>+                    disabled="true"
>+                    class="folderMenuItem"
>+                    displayformat="verbose"/>
[Hmm, don't we have some code trying to set the label of this menulist?]

>+           <menupopup class="searchPopup menulist-menupopup"
searchPopup only exists to attach some magic XBL to the menupopup, so you need to remove the CSS, and the XBL too. (And as a followup, locationpopup could be merged with popup-base, since there's no other consumer of that popup any more.)

Comment 7

[Philip Chee]

Attachment: Patch v1.3 fix nits

>>+    return this.mFilterList && this.mFilterList.filterCount || 0;
> [Ah yes, the row count can get queried before a filter list has been found.]

>>+                       oncommand="onFilterServerClick(event.target._folder.server.serverURI)"/>

> This is wrong, you need the folder's URI rather than the server's, otherwise 
> news filters don't work.
> [At some point we should just use the folder everywhere, now that we have it, 
> but that's probably better done in a follow-up bug.]
Fixed.

>>+          <menulist id="runFiltersFolder"
>>+                    disabled="true"
>>+                    class="folderMenuItem"
>>+                    displayformat="verbose"/>
> [Hmm, don't we have some code trying to set the label of this menulist?]
Yes, somewhere in msgFolderPickerOverlay.js I believe. 

In this patch "runFiltersFolder" is still a RDF tree. Thunderbird Bug 435804 (Remaining rdf cleanup) converts it to XBL. Our version of Bug 435804 is Bug 507676. So I'm leaving XBLfy "runFiltersFolder" for bug 507676.

>>+           <menupopup class="searchPopup menulist-menupopup"
> searchPopup only exists to attach some magic XBL to the menupopup, so you need 
> to remove the CSS, and the XBL too. (And as a followup, locationpopup could be 
> merged with popup-base, since there's no other consumer of that popup any more.)
Fixed.

Comment 8

[[e-mail]]

(In reply to Philip Chee from comment #7)
>>>+          <menulist id="runFiltersFolder"
>>>+                    disabled="true"
>>>+                    class="folderMenuItem"
>>>+                    displayformat="verbose"/>
>> [Hmm, don't we have some code trying to set the label of this menulist?]
>Yes, somewhere in msgFolderPickerOverlay.js I believe. 
So what do these changes achieve?

Comment 9

[Philip Chee]

Attachment: Patch v1.3a alternate patch

> So what do these changes achieve?
Nothing at the moment. I thought I'd get these in ahead of the next XBL bug/patch.

Here's an alernate patch without these changes:
> +                    class="folderMenuItem"
> +                    displayformat="verbose"/>

Comment 10

[[e-mail]]

(In reply to Philip Chee from comment #9)
> > So what do these changes achieve?
> Nothing at the moment. I thought I'd get these in ahead of the next XBL
> bug/patch.

Surely that will only make the next patch more confusing, if it's adding stuff that's not used yet?

Comment 11

[[e-mail]]

Comment on attachment 8427847 [details] [diff] [review]
Patch v1.3a alternate patch

>+    return this.mFilterList && this.mFilterList.filterCount || 0;
Nit: I've decided I'd prefer this.mFilterList ? this.mFilterList.filterCount : 0;

Comment 12

[Philip Chee]

Pushed to mozilla-central:
https://hg.mozilla.org/comm-central/rev/9c0ca191027a

Comment 13

[[e-mail]]

[Bah, some CRs crept in on some of the lines.]

Comment 14

[[e-mail]]

This caused bug 1106225 for SeaMonkey because you moved the oncommand from the button to the menupopup where it gets duplicated throughout the folder menu and fires multiple times.