[port] bug 464770 - Pluggable filter lists

RESOLVED FIXED in seamonkey2.0b2

Status

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bugzilla, Assigned: bugzilla)

Tracking

Trunk
seamonkey2.0b2
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Posted 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)

Updated

10 years ago
Assignee: nobody → aqualon

Comment 1

10 years ago
+      // 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.

Comment 4

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

Comment 5

10 years ago
Posted 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 6

10 years ago
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+
(Assignee)

Comment 7

10 years ago
Posted 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+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed

Comment 8

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

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

Updated

10 years ago
Keywords: checkin-needed

Comment 9

10 years ago
Pushed with whitespace fixup as <http://hg.mozilla.org/comm-central/rev/c324b2a2bbc2>.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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.