Closed Bug 1001071 Opened 6 years ago Closed 6 years ago

XBLify folder-selection menus in FilterListDialog and SearchDialog.xul

Categories

(SeaMonkey :: MailNews: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.29

People

(Reporter: philip.chee, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch Patch v1.0 Proposed fix (obsolete) — Splinter Review
> -    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.
Attachment #8412067 - Flags: review?(iann_bugzilla)
Attachment #8412067 - Flags: feedback?(neil)
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.
Attachment #8412067 - Flags: review?(iann_bugzilla)
Attachment #8412067 - Flags: feedback?(neil)
Attached patch Patch v1.1 back to the basics. (obsolete) — Splinter Review
> 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.
Attachment #8412067 - Attachment is obsolete: true
Attachment #8413862 - Flags: review?(neil)
(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.]
> 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.
Attachment #8413862 - Attachment is obsolete: true
Attachment #8413862 - Flags: review?(neil)
Attachment #8415917 - Flags: review?(neil)
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.)
Attachment #8415917 - Flags: review?(neil) → review-
>>+    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.
Attachment #8415917 - Attachment is obsolete: true
Attachment #8427072 - Flags: review?(neil)
(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?
> 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"/>
Attachment #8427847 - Flags: review?(neil)
(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 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;
Attachment #8427847 - Flags: review?(neil) → review+
Pushed to mozilla-central:
https://hg.mozilla.org/comm-central/rev/9c0ca191027a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.29
Attachment #8427072 - Flags: review?(neil)
[Bah, some CRs crept in on some of the lines.]
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.
Depends on: 1133212
You need to log in before you can comment on or make changes to this bug.