The default bug view has changed. See this FAQ.

[port] bug 464770 - Pluggable filter lists

RESOLVED FIXED in seamonkey2.0b2

Status

SeaMonkey
MailNews: Backend
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Bruno 'Aqualon' Escherl, Assigned: Bruno 'Aqualon' Escherl)

Tracking

Trunk
seamonkey2.0b2
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

1.95 KB, patch
Bruno 'Aqualon' Escherl
: review+
Bruno 'Aqualon' Escherl
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Created attachment 388065 [details] [diff] [review]
port v1

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

8 years ago
Assignee: nobody → aqualon

Comment 1

8 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 2

8 years ago
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+

Comment 3

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

8 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

8 years ago
Created attachment 388894 [details] [diff] [review]
Patch v2

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

8 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

8 years ago
Created attachment 389370 [details] [diff] [review]
Patch v3

Removed the superfluous ';'

Taking over r+/sr+
Attachment #388894 - Attachment is obsolete: true
Attachment #389370 - Flags: superreview+
Attachment #389370 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 8

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

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

Updated

8 years ago
Keywords: checkin-needed

Comment 9

8 years ago
Pushed with whitespace fixup as <http://hg.mozilla.org/comm-central/rev/c324b2a2bbc2>.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
No longer blocks: 360488
Target Milestone: --- → seamonkey2.0b2
You need to log in before you can comment on or make changes to this bug.