Last Comment Bug 503668 - [port] bug 464770 - Pluggable filter lists
: [port] bug 464770 - Pluggable filter lists
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: MailNews: Backend (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: seamonkey2.0b2
Assigned To: Bruno 'Aqualon' Escherl
:
Mentors:
Depends on: 464770
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-11 08:30 PDT by Bruno 'Aqualon' Escherl
Modified: 2009-07-19 18:12 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
port v1 (1.99 KB, patch)
2009-07-11 08:30 PDT, Bruno 'Aqualon' Escherl
neil: superreview+
Details | Diff | Review
Patch v2 (1.95 KB, patch)
2009-07-16 02:26 PDT, Bruno 'Aqualon' Escherl
mnyromyr: review+
bugzilla: superreview+
Details | Diff | Review
Patch v3 (1.95 KB, patch)
2009-07-19 08:05 PDT, Bruno 'Aqualon' Escherl
bugzilla: review+
bugzilla: superreview+
Details | Diff | Review

Description Bruno 'Aqualon' Escherl 2009-07-11 08:30:30 PDT
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.
Comment 1 Philip Chee 2009-07-11 09:55:25 PDT
+      // 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 neil@parkwaycc.co.uk 2009-07-11 15:54:53 PDT
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 neil@parkwaycc.co.uk 2009-07-11 15:56:49 PDT
(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 2009-07-11 20:33:04 PDT
> 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 2009-07-16 02:26:44 PDT
Created attachment 388894 [details] [diff] [review]
Patch v2

I rewrote the comment and addressed the other review nits.
Comment 6 Karsten Düsterloh 2009-07-18 07:27:01 PDT
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 2009-07-19 08:05:36 PDT
Created attachment 389370 [details] [diff] [review]
Patch v3

Removed the superfluous ';'

Taking over r+/sr+
Comment 8 Philip Chee 2009-07-19 10:45:33 PDT
+      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 2009-07-19 11:15:35 PDT
Pushed with whitespace fixup as <http://hg.mozilla.org/comm-central/rev/c324b2a2bbc2>.

Note You need to log in before you can comment on or make changes to this bug.