Closed Bug 503668 Opened 11 years ago Closed 11 years ago

[port] bug 464770 - Pluggable filter lists

Categories

(SeaMonkey :: MailNews: Backend, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b2

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch port v1 (obsolete) — Splinter Review
I've ported the two changes we still missed from the patch in bug 464770 and included some comments cleanup.
Attachment #388065 - Flags: superreview?(neil)
Attachment #388065 - Flags: review?(mnyromyr)
Assignee: nobody → aqualon
+      // 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 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).
Attachment #388065 - Flags: superreview?(neil) → superreview+
(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.
> 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."
Attached patch Patch v2 (obsolete) — Splinter Review
I rewrote the comment and addressed the other review nits.
Attachment #388065 - Attachment is obsolete: true
Attachment #388894 - Flags: superreview+
Attachment #388894 - Flags: review?(mnyromyr)
Attachment #388065 - Flags: review?(mnyromyr)
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.
Attachment #388894 - Flags: review?(mnyromyr) → review+
Attached patch Patch v3Splinter Review
Removed the superfluous ';'

Taking over r+/sr+
Attachment #388894 - Attachment is obsolete: true
Attachment #389370 - Flags: superreview+
Attachment #389370 - Flags: review+
Keywords: checkin-needed
+      args = { filterList: folder.getEditableFilterList(msgWindow),
+               filterName: emailAddress };

You didn't notice that Mnyromyr also removed the leading and trailing spaces in {}
Keywords: checkin-needed
Pushed with whitespace fixup as <http://hg.mozilla.org/comm-central/rev/c324b2a2bbc2>.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
No longer blocks: TB2SM
Target Milestone: --- → seamonkey2.0b2
You need to log in before you can comment on or make changes to this bug.