503668 - [port] bug 464770 - Pluggable filter lists

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Backend, defect)

Description

[Bruno 'Aqualon' Escherl]

Attachment: port v1

I've ported the two changes we still missed from the patch in bug 464770 and included some comments cleanup.

Comment 1

[Philip Chee]

+      // We have to do prefill filter so we are going to launch the
+      // filterEditor dialog and prefill that with the emailAddress.

We need to prefill the filter so we launch the filterEditor dialog prefilled with the emailAddress.

+      args = { filterList: folder.getEditableFilterList(msgWindow) };
       args.filterName = emailAddress;

args = { filterList: folder.getEditableFilterList(msgWindow),
         filterName: emailAddress };


-      /* if the user hits ok in the filterEditor dialog we set 
-         args.refresh=true there
-         we check this here in args to show filterList dialog */
+      // If the user hits ok in the filterEditor dialog we set args.refresh=true
+      // there we check this here in args to show filterList dialog.

I think that in the original comment there are two sentences:

1. If the user hits ok in the filterEditor dialog we set args.refresh=true there.

2. We check for refresh in args here to see if we need to show the filterList dialog.

You seem to have conflated these two.

Comment 2

[[e-mail]]

Comment on attachment 388065 [details] [diff] [review]
port v1

>+      args = { filterList: folder.getEditableFilterList(msgWindow) };
>       args.filterName = emailAddress;
[I wonder why we don't just define the object directly with both properties.]

>+      // If the user hits ok in the filterEditor dialog we set args.refresh=true
>+      // there we check this here in args to show filterList dialog.
As Philip says, this should be "there. We" (reformatting at your discretion).

Comment 3

[[e-mail]]

(In reply to comment #1)
> +      // We have to do prefill filter so we are going to launch the
> +      // filterEditor dialog and prefill that with the emailAddress.
> We need to prefill the filter so we launch the filterEditor dialog prefilled
> with the emailAddress.
I still think this is a bit repetitive... how about
We are going to prefill the filterEditor dialog with the emailAddress.

Comment 4

[Philip Chee]

> I still think this is a bit repetitive... how about
> We are going to prefill the filterEditor dialog with the emailAddress.
Or just:
"Prefill the filterEditor dialog with the emailAddress."

Comment 5

[Bruno 'Aqualon' Escherl]

Attachment: Patch v2

I rewrote the comment and addressed the other review nits.

Comment 6

[Karsten Düsterloh]

Comment on attachment 388894 [details] [diff] [review]
Patch v2

>diff --git a/suite/mailnews/mailWindowOverlay.js b/suite/mailnews/mailWindowOverlay.js
>+      args = { filterList: folder.getEditableFilterList(msgWindow),
>+               filterName: emailAddress; };

You didn't test that, I'm sure. ;-)
Just write:
  args = {filterList: folder.getEditableFilterList(msgWindow), filterName: emailAddress};

r=me with that.

Comment 7

[Bruno 'Aqualon' Escherl]

Attachment: Patch v3

Removed the superfluous ';'

Taking over r+/sr+

Comment 8

[Philip Chee]

+      args = { filterList: folder.getEditableFilterList(msgWindow),
+               filterName: emailAddress };

You didn't notice that Mnyromyr also removed the leading and trailing spaces in {}

Comment 9

[Karsten Düsterloh]

Pushed with whitespace fixup as <http://hg.mozilla.org/comm-central/rev/c324b2a2bbc2>.