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

RESOLVED FIXED in seamonkey2.40

Status

SeaMonkey
MailNews: Message Display
--
minor
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: sgautherie, Assigned: ewong)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
seamonkey2.40
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.1 wontfix, seamonkey2.38 affected, seamonkey2.39 affected, seamonkey2.40 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
Needs bug 444913 (FilterListDialog part) first.
Flags: wanted-seamonkey2?

Comment 1

8 years ago
I don't see any reason to give this special priority, but it's also not unwanted, so cancelling the wanted flag.
Flags: wanted-seamonkey2.0?
(Reporter)

Comment 2

8 years ago
I agree it's too late for 2.0.
Nonetheless, I think we should complete (all) the de-rdf ports in 2.1(a1).
Flags: wanted-seamonkey2.1?

Updated

8 years ago
status-seamonkey2.1: --- → wanted
Flags: wanted-seamonkey2.1?

Updated

7 years ago
status-seamonkey2.1: wanted → wontfix
(Assignee)

Updated

7 years ago
Blocks: 657604
(Assignee)

Updated

7 years ago
Blocks: 657607

Updated

7 years ago
No longer blocks: 657604
(Reporter)

Updated

7 years ago
No longer blocks: 460953
(Assignee)

Comment 3

4 years ago
Created attachment 790598 [details] [diff] [review]
Port patch (v1)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #790598 - Flags: review?(neil)
(Assignee)

Updated

4 years ago
Blocks: 507669

Comment 4

4 years ago
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.]
Attachment #790598 - Flags: review?(neil) → review-

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
Created attachment 8350536 [details] [diff] [review]
port patch (v2)
Attachment #790598 - Attachment is obsolete: true
Attachment #8350536 - Flags: review?(neil)

Comment 7

4 years ago
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.)
Attachment #8350536 - Flags: review?(neil) → review-

Updated

4 years ago
Assignee: ewong → nobody
Keywords: helpwanted

Updated

4 years ago
Blocks: 984948

Updated

4 years ago
Assignee: nobody → ewong
Keywords: helpwanted

Comment 8

4 years ago
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

4 years ago
(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

4 years ago
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...
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

4 years ago
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

4 years ago
(In reply to neil@parkwaycc.co.uk 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

4 years ago
> 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

4 years ago
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

4 years ago
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.
Attachment #8441557 - Flags: feedback?(ewong)
(Assignee)

Comment 17

3 years ago
(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.
(Assignee)

Comment 18

3 years ago
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.
(Assignee)

Comment 19

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

Sorry for the very long delay. Looks like it's working.
Attachment #8441557 - Flags: feedback?(ewong) → feedback+

Comment 20

3 years ago
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.
Attachment #8441557 - Flags: review?(neil)
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

3 years ago
Created attachment 8616869 [details] [diff] [review]
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.
Attachment #8350536 - Attachment is obsolete: true
Attachment #8441557 - Attachment is obsolete: true
Attachment #8441557 - Flags: review?(neil)
Attachment #8616869 - Flags: review?(neil)

Updated

2 years ago
Attachment #8616869 - Flags: review?(neil) → review+

Comment 23

2 years ago
http://hg.mozilla.org/comm-central/rev/baa5505baccb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-seamonkey2.38: --- → affected
status-seamonkey2.39: --- → affected
status-seamonkey2.40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.40
You need to log in before you can comment on or make changes to this bug.