507676 - Port |Bug 435804 - Remaining rdf cleanup for FilterListDialog| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Serge Gautherie (:sgautherie)]

Needs bug 444913 (FilterListDialog part) first.

Comment 1

[Robert Kaiser]

I don't see any reason to give this special priority, but it's also not unwanted, so cancelling the wanted flag.

Comment 2

[Serge Gautherie (:sgautherie)]

I agree it's too late for 2.0.
Nonetheless, I think we should complete (all) the de-rdf ports in 2.1(a1).

Comment 3

[Edmund Wong (:ewong)]

Attachment: Port patch (v1)

Comment 4

[[e-mail]]

Comment on attachment 790598 [details] [diff] [review]
Port patch (v1)

[I don't seem to have got bugmail for this request. Strange...]

>-var gCurrentServerURI = null;
>+var gCurrentServer;
This variable was actually a misnomer. It actually held the current folder URI, so the new variable should be called gCurrentFolder (as it is an nsIMsgFolder).

>-function onFilterServerClick(selection)
>+function onFilterServerClick(aSelection)
> {
>-    var itemURI = selection.getAttribute('itemUri');
>+    var itemURI = aSelection.getAttribute('itemUri');
> 
>-    if (itemURI == gCurrentServerURI)
>+    if (itemURI == gCurrentServer)
>       return;
itemUri is part of the old RDF template and doesn't make sense here.

> // roots the tree at the specified server
>-function setServer(uri)
>+function setServer(aServer)
[Again, this really sets the folder, not the server.]

>+   var msgFolder = aServer.rootFolder;
>-   msgFolder = msgFolder.server.rootMsgFolder;
This is just wrong.

-function selectServer(uri)
+function selectServer(aServer)
[Again, this really selects the folder, not the server.]

>-    serverMenu.setAttribute("label", msgFolder.name);
>-    serverMenu.setAttribute("uri",uri);
>-    serverMenu.setAttribute("IsServer", msgFolder.isServer);
>-    serverMenu.setAttribute("IsSecure", msgFolder.server.isSecure);
>-    serverMenu.setAttribute("ServerType", msgFolder.server.type);
Now that we're removing this, it's possible that onFilterServerClick can just call setServer directly.

>+  var folder = document.getElementById("runFiltersFolder").selectedItem._folder;
[Strange that there isn't a public method for the selected folder.]

> /**
>   * get the selected server if it can have filters
>+  *
>+  * @returns an nsIMsgIncomingServer for the server
Again, this should return the folder, not the server.

>-                    // for news, select the news folder
>-                    // for everything else, select the folder's server
>-                    firstItem = (server.type == "nntp") ? msgFolder.URI : rootFolder.URI;
So this change is wrong for instance.

>+  * @returns an nsImsgIncomingServer
[Ditto.]

Comment 5

[[e-mail]]

(In reply to comment #4)
> [I don't seem to have got bugmail for this request. Strange...]

I've found it now. I can only guess that it got lost in a wave of bugmail. Sorry about that.

Comment 6

[Edmund Wong (:ewong)]

Attachment: port patch (v2)

Comment 7

[[e-mail]]

Comment on attachment 8350536 [details] [diff] [review]
port patch (v2)

>-function onFilterServerClick(selection)
>+function onFilterServerClick(aSelection)
> {
...
>-
>-    selectServer(itemURI);
I suspect you're going to need to call selectFolder at some point, so you want aSelection to be a folder.

>+   var msgFolder = aFolder.rootFolder;
Even if this existed it would give you the wrong folder.

>    //Calling getFilterList will detect any errors in rules.dat, backup the file, and alert the user
>    gFilterTreeView.filterList = msgFolder.getEditableFilterList(gFilterListMsgWindow);
Need to get the filter list from the original folder.

>+function selectFolder(aFolder)
> {
>     // update the server menu
>+    var serverMenu = document.getElementById("serverMenuPopup");
>+    serverMenu.selectFolder(aFolder.rootFolder);
Where does the rootFolder fit in?

>-  var msgFolder = resource.QueryInterface(Components.interfaces.nsIMsgFolder);
>+  var folder = document.getElementById("runFiltersFolder").selectedItem._folder;
If you name this msgFolder you'll avoid having to change the other two lines.

>-                    // for news, select the news folder
>-                    // for everything else, select the folder's server
>-                    firstItem = (server.type == "nntp") ? msgFolder.URI : rootFolder.URI;
>+                  return rootFolder;
What happened to the news check? Also, indentation was wrong.
>-    return firstItem;
>+    return firstItem.rootFolder;
This fails if firstItem is null. (Your change to getSelectedServerForFilters avoids this problem.)

Comment 8

[:aceman]

In bug 984948 comment 9 it was proposed to solve all this by unforking (merging) of SearchDialog.xul and FilterListDialog.xul and I think it received support from the SM side.
1. Can we decide whether to scrap the approach here and do the unforking?
2. Would TB also support it? The resulting files would be moved to /mailnews/base/search/content/ .
3. Do we do it in this bug or a new one?

I'd be willing to try it if there is agreement.

Comment 9

[[e-mail]]

(In reply to aceman from comment #8)
> In bug 984948 comment 9 it was proposed to solve all this by unforking
> (merging) of SearchDialog.xul and FilterListDialog.xul and I think it
> received support from the SM side.

Well, it wasn't clear that a merge was proposed - the patch provided simply replaced the SeaMonkey version with the Thunderbird version, but I don't have a list of differences between the two so I don't know how feasible a merge is.

Comment 10

[:aceman]

I proposed a merge in comment 17.
But yes, that is the point of my question. SM's versions of the files are proposed to be replaced by the TB's versions, so they would be identical right now. So it would be possible to unfork/merge them now.
Or you want to leave them separated to allow them drifting apart again in the future. I have no problem with that if that is the plan and intentional decision in Seamonkey, I just ask if you don't want to merge when there is the possibility now.

But it seems the replacement hasn't been done in bug 984948 yet. And the bug is resolved...

Comment 11

[[e-mail]]

You're confusing me.

When you say merge, I think of "cherry-pick the best changes from each file". I don't know whether this is feasible.

When you say unfork, I think of "blindly overwrite SeaMonkey's version". I don't think I'd appreciate it if this happened.

Comment 12

[Kent James (:rkent)]

I'm curious what is the defense of maintaining the forking.

My frustration with the forked approach is that it, for backend API changes that affect both (as most backend changes seem to do) the Thunderbird-focused developers have to choose between 1) just doing the changes for Thunderbird,  and breaking SeaMonkey nightlies or 2) doubling their work to do changes in both the SM and TB code, AND get both TB and SM reviewers to review before things move forward.

1) creates a lot of hard feelings which we would rather avoid
2) creates extra work, slower reviews, and the occasional dead patch because we cannot get reviews done (I'm thinking of you bug 695671)

Unforking solves both of these issues, so IMHO is A Good Thing.

Comment 13

[:aceman]

(In reply to [e-mail] from comment #11)
> When you say merge, I think of "cherry-pick the best changes from each
> file". I don't know whether this is feasible.
No, I do not mean this.

> When you say unfork, I think of "blindly overwrite SeaMonkey's version". I
> don't think I'd appreciate it if this happened.
Yes, I only talk about this version of unfork, as was proposed in bug 984948. Overwriting SM's version of the file (https://bug984948.bugzilla.mozilla.org/attachment.cgi?id=8407519), or actually make a new file in /mailnews (copying TB's version) and make TB and SM use that shared file.

I noticed now you opposed that patch in bug 984948 comment 46 so that is why it was abandoned.

Comment 14

[Philip Chee]

> I'm curious what is the defense of maintaining the forking.
We don't want to maintain a fork, but we don't want to lose some improvements in the SeaMonkey version by overwriting it with the Thunderbird version.

See Bug 1001071 Comment 0 on what I intend to do with the FilterListDialog

Comment 15

[:aceman]

OK, so the plan is to first do Neil's option 1 (merge) and there are some useful things in SM that could be moved over to TB.

Comment 16

[Philip Chee]

Attachment: Fix bit-rot from Bug 1001071

When I did Bug 1001071 I bitrotted this bug so I'm here to fix the bit-rot.
When porting Bug 435804 you need to also look at followup fixes and regression fixes. I've identified five.

Bug 437056 - run a message filter on a folder - "choose this folder" option missing, can't run on folders with subfolders
  http://hg.mozilla.org/comm-central/rev/d6628cc24bc5

Bug 441666 - Filter dialog fails to list newsgroups
  http://hg.mozilla.org/comm-central/rev/cfaac96a1c73

Bug 541408 - newsgroup filter from "create filter from message" is not created
  http://hg.mozilla.org/comm-central/rev/b7dedc7a90ae

Bug 527629 - Manual filters no longer are allowed on deferred-from servers
  http://hg.mozilla.org/comm-central/rev/60be35e6f4cc
    Bug 527950 - In filter list editor, Local Folders sometimes shows wrong filter list;
      http://hg.mozilla.org/comm-central/rev/8fa5c0e2a1d4

>+function onFilterServerClick(aSelection)
> {
> ...
>-
>-    selectServer(itemURI);
> I suspect you're going to need to call selectFolder at some point, so you want aSelection to be a folder.
Changed to:
  function onFilterFolderClick(aFolder)

>+   var msgFolder = aFolder.rootFolder;
> Even if this existed it would give you the wrong folder.
Removed rootFolder.

>    //Calling getFilterList will detect any errors in rules.dat, backup the file, and alert the user
>    gFilterTreeView.filterList = msgFolder.getEditableFilterList(gFilterListMsgWindow);
> Need to get the filter list from the original folder.
??

> {
>     // update the server menu
>+    var serverMenu = document.getElementById("serverMenuPopup");
>+    serverMenu.selectFolder(aFolder.rootFolder);
> Where does the rootFolder fit in?
It doesn't. Removed.

> If you name this msgFolder you'll avoid having to change the other two lines.
Done.

>-                    // for news, select the news folder
>-                    // for everything else, select the folder's server
>-                    firstItem = (server.type == "nntp") ? msgFolder.URI : rootFolder.URI;
>+                  return rootFolder;
> What happened to the news check? Also, indentation was wrong.
In Thunderbird, accidentally removed. Restored in Bug 541408. This bug also changed most functions to use folders instead of servers. See updated patch.

Comment 17

[Edmund Wong (:ewong)]

(In reply to Philip Chee from comment #16)
> Created attachment 8441557 [details] [diff] [review]
> Fix bit-rot from Bug 1001071
> 
> When I did Bug 1001071 I bitrotted this bug so I'm here to fix the bit-rot.
> When porting Bug 435804 you need to also look at followup fixes and
> regression fixes. I've identified five.
> 
> Bug 437056 - run a message filter on a folder - "choose this folder" option
> missing, can't run on folders with subfolders
>   http://hg.mozilla.org/comm-central/rev/d6628cc24bc5
> 
> Bug 441666 - Filter dialog fails to list newsgroups
>   http://hg.mozilla.org/comm-central/rev/cfaac96a1c73
> 
> Bug 541408 - newsgroup filter from "create filter from message" is not
> created
>   http://hg.mozilla.org/comm-central/rev/b7dedc7a90ae
> 
> Bug 527629 - Manual filters no longer are allowed on deferred-from servers
>   http://hg.mozilla.org/comm-central/rev/60be35e6f4cc
>     Bug 527950 - In filter list editor, Local Folders sometimes shows wrong
> filter list;
>       http://hg.mozilla.org/comm-central/rev/8fa5c0e2a1d4
> 
> >+function onFilterServerClick(aSelection)
> > {
> > ...
> >-
> >-    selectServer(itemURI);
> > I suspect you're going to need to call selectFolder at some point, so you want aSelection to be a folder.
> Changed to:
>   function onFilterFolderClick(aFolder)
> 
> >+   var msgFolder = aFolder.rootFolder;
> > Even if this existed it would give you the wrong folder.
> Removed rootFolder.
> 
> >    //Calling getFilterList will detect any errors in rules.dat, backup the file, and alert the user
> >    gFilterTreeView.filterList = msgFolder.getEditableFilterList(gFilterListMsgWindow);
> > Need to get the filter list from the original folder.
> ??
> 
> > {
> >     // update the server menu
> >+    var serverMenu = document.getElementById("serverMenuPopup");
> >+    serverMenu.selectFolder(aFolder.rootFolder);
> > Where does the rootFolder fit in?
> It doesn't. Removed.
> 
> > If you name this msgFolder you'll avoid having to change the other two lines.
> Done.
> 
> >-                    // for news, select the news folder
> >-                    // for everything else, select the folder's server
> >-                    firstItem = (server.type == "nntp") ? msgFolder.URI : rootFolder.URI;
> >+                  return rootFolder;
> > What happened to the news check? Also, indentation was wrong.
> In Thunderbird, accidentally removed. Restored in Bug 541408. This bug also
> changed most functions to use folders instead of servers. See updated patch.

Thanks Ratty!  Can't test this out until bug 1027241 is fixed.

Comment 18

[Edmund Wong (:ewong)]

Comment on attachment 8441557 [details] [diff] [review]
Fix bit-rot from Bug 1001071

Oh feedback fail. 

As per comment #17, will test it out once bug 1027241 is fixed.

Comment 19

[Edmund Wong (:ewong)]

Comment on attachment 8441557 [details] [diff] [review]
Fix bit-rot from Bug 1001071

Sorry for the very long delay. Looks like it's working.

Comment 20

[Philip Chee]

Comment on attachment 8441557 [details] [diff] [review]
Fix bit-rot from Bug 1001071

> When I did Bug 1001071 I bitrotted this bug so I'm here to fix the bit-rot.
> When porting Bug 435804 you need to also look at followup fixes and
> regression fixes. I've identified five.
> 
> Bug 437056 - run a message filter on a folder - "choose this folder" option
> missing, can't run on folders with subfolders
>   http://hg.mozilla.org/comm-central/rev/d6628cc24bc5
> 
> Bug 441666 - Filter dialog fails to list newsgroups
>   http://hg.mozilla.org/comm-central/rev/cfaac96a1c73
> 
> Bug 541408 - newsgroup filter from "create filter from message" is not
> created
>   http://hg.mozilla.org/comm-central/rev/b7dedc7a90ae
> 
> Bug 527629 - Manual filters no longer are allowed on deferred-from servers
>   http://hg.mozilla.org/comm-central/rev/60be35e6f4cc
>     Bug 527950 - In filter list editor, Local Folders sometimes shows wrong
> filter list;
>       http://hg.mozilla.org/comm-central/rev/8fa5c0e2a1d4
> 
>>+function onFilterServerClick(aSelection)
>> {
>> ...
>>-
>>-    selectServer(itemURI);
>> I suspect you're going to need to call selectFolder at some point, so you want aSelection to be a folder.
> Changed to:
>   function onFilterFolderClick(aFolder)
> 
>>+   var msgFolder = aFolder.rootFolder;
>> Even if this existed it would give you the wrong folder.
> Removed rootFolder.
> 
>>    //Calling getFilterList will detect any errors in rules.dat, backup the file, and alert the user
>>    gFilterTreeView.filterList = msgFolder.getEditableFilterList(gFilterListMsgWindow);
>> Need to get the filter list from the original folder.
> ??
> 
>> {
>>     // update the server menu
>>+    var serverMenu = document.getElementById("serverMenuPopup");
>>+    serverMenu.selectFolder(aFolder.rootFolder);
>> Where does the rootFolder fit in?
> It doesn't. Removed.
> 
>> If you name this msgFolder you'll avoid having to change the other two lines.
> Done.
> 
>>-                    // for news, select the news folder
>>-                    // for everything else, select the folder's server
>>-                    firstItem = (server.type == "nntp") ? msgFolder.URI : rootFolder.URI;
>>+                  return rootFolder;
>> What happened to the news check? Also, indentation was wrong.
> In Thunderbird, accidentally removed. Restored in Bug 541408. This bug also
> changed most functions to use folders instead of servers. See updated patch.

Comment 21

[[e-mail]]

Comment on attachment 8441557 [details] [diff] [review]
Fix bit-rot from Bug 1001071

>+Components.utils.import("resource:///modules/mailServices.js");
>+Components.utils.import("resource://gre/modules/Services.jsm");
Nit: Services before mailServices

>+      firstItem = getServerThatCanHaveFilters().rootFolder;
What happens if no servers can have filters?

> 
>     if (firstItem)
>-      selectServer(firstItem);
>-    else
>-      updateButtons();
>+      selectFolder(firstItem);
>+
>+    updateButtons();
Why remove the else?

>+ * note the function name 'onFilterFolderClick' is misleading, it would be
>+ * better named 'onServerSelect' => file follow up bug later.
onFolderSelect perhaps? (We're not necessarily selecting a server.)

>-   // root the folder picker to this server
>-   gRunFiltersFolderPicker.setAttribute("ref", rootFolderUri);
>+  // root the folder picker to this server
>+  var runMenu = document.getElementById("runFiltersPopup");
>+  runMenu._teardown();
>+  runMenu._parentFolder = msgFolder;
>+  runMenu._ensureInitialized();
[This didn't get done in bug 1001071?]

Comment 22

[Philip Chee]

Attachment: Patch v3.0 more fixes.

>>+Components.utils.import("resource:///modules/mailServices.js");
>>+Components.utils.import("resource://gre/modules/Services.jsm");
> Nit: Services before mailServices
Fixed.

>>+      firstItem = getServerThatCanHaveFilters().rootFolder;
> What happens if no servers can have filters?
I don't know. Anyway I've added a check for this.

>>     if (firstItem)
>>-      selectServer(firstItem);
>>-    else
>>-      updateButtons();
>>+      selectFolder(firstItem);
>>+
>>+    updateButtons();
> Why remove the else?
It wasn't clear that selectFolder() calls setFolder() which calls updateButtons().
I've put back the else clause.

>>+ * note the function name 'onFilterFolderClick' is misleading, it would be
>>+ * better named 'onServerSelect' => file follow up bug later.
> onFolderSelect perhaps? (We're not necessarily selecting a server.)
Changed to onFolderSelect.

>>-   // root the folder picker to this server
>>-   gRunFiltersFolderPicker.setAttribute("ref", rootFolderUri);
>>+  // root the folder picker to this server
>>+  var runMenu = document.getElementById("runFiltersPopup");
>>+  runMenu._teardown();
>>+  runMenu._parentFolder = msgFolder;
>>+  runMenu._ensureInitialized();
> [This didn't get done in bug 1001071?]
As far as I can tell, no it didn't.

Comment 23

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/baa5505baccb