325554 - add UI for honoring ISP spam headers - SeaMonkey version

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Giacomo Magnini]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060202 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060202 SeaMonkey/1.5a

Port the UI patch in bug 290237 to SM.

Reproducible: Always

Comment 1

[Giacomo Magnini]

Attachment: First very rough cut

It compiles and shows correctly. Not very sure about some aviarism in the js file, but don't know if which are the SM equivalents. UI seems to work with no JS console errors, tho.

Comment 2

[Stefan [:stefanh]]

--> Giacomo

Comment 3

[Karsten Düsterloh]

Comment on attachment 210458 [details] [diff] [review]
First very rough cut

Just a couple of code nits, it seems to *work* okay (afaict without having the respective server side header generating programs available). :)


>Index: mailnews/base/resources/content/junkMail.js
>===================================================================
> const kJunkOnLocalFolderURI = "mailbox://nobody@Local%20Folders/Junk";
>+const KEY_APPDIR = "XCurProcD";
>+const KEY_PROFILEDIR = "PrefD";

Change these two constants to lowerCamelCase, too.

>+const kDefaultFilterServerName = "SpamAssassin"; // used only to force an initial default value in the combo box (if this entry exists)

You should wrap lines at a suitable point (see below), overly long comments should probably  go before the respective code then. 

>+  // set up trusted IP headers
>+  document.getElementById("useServerFilter").checked = obj.settings.useServerFilter; // if we have a server filter name...

Wrap long lines.

>+  var serverFilterList = document.getElementById("useServerFilterList");
>+  var defaultServerItem = serverFilterList.getElementsByAttribute("value", obj.settings.serverFilterName)[0] 
>+                         || serverFilterList.getElementsByAttribute("value", kDefaultFilterServerName)[0];

Operators (like ||) should stay at the end of the previous line in Mozilla.org code.

>+  if (defaultServerItem)
>+    serverFilterList.selectedItem = defaultServerItem;
>+  else
>+   serverFilterList.selectedIndex = 0;

Take care of the indentation.

>   // set up the manual mark UI
>   document.getElementById("manualMark").checked = obj.settings.manualMark;
>   document.getElementById("manualMarkMode").selectedItem = document.getElementById("manualMarkMode" + obj.settings.manualMarkMode);

Wrap again.

>+function buildServerFilterMenuList()
>+  // First, scan the profile directory for any .sfd files we may have there. 
>+  var fileLocator = Components.classes["@mozilla.org/file/directory_service;1"]
>+                              .getService(Components.interfaces.nsIProperties);
>+
>+  var profileDir = fileLocator.get(KEY_PROFILEDIR, Components.interfaces.nsIFile);

See above about the const names.

>+  // Then, fall back to defaults\messenger and list the default sfd files we shipped with

Use / instead of \ - we're not Win-only. ;-)

>+// helper function called by buildServerFilterMenuList. Enumerates over the passed in
>+// directory looking for .sfd files. For each entry found, it gets appended to the menu list

It = the function? ;-)
"Each entry found gets appended to the menulist."

>+function buildServerFilterListFromDir(aDir)

You're calling this function twice at least, so the name is quite misleading, imo. Probably  addServerFilterListItemsFromDir() would be better.

>+  while (entries.hasMoreElements())
>+  {

The rest of this file uses the more compact brace style ( { at end of a line etc.), so you should use that, too.

>+    var entry = entries.nextFile;
>+    if (entry.isFile())
>+    { 
>+      var fileName = entry.leafName;
>+      
>+      // we only care about files that end in .sfd 
>+      if (fileName.length > 4 && (fileName.lastIndexOf('.sfd') == fileName.length - 4))
>+      { 
>+        fileName = fileName.substring(0, fileName.length - 4);

Now that's far too complicated.

var filename = entry.leafname.match(/(.+)\.sfd$/);
if (filename)
  filename = filename[1];

>+        // if we've already added an item with this name, then don't add it again.
>+        if (ispHeaderPopup.getElementsByAttribute("value", fileName)[0])
>+          continue;

This is quite inconsequent. Either you break off your if-cascade above with continue statements everywhere or you don't, but please don't mix.

>+        // strip off the extension

This comment doesn't belong here.

>Index: mailnews/base/resources/content/junkMail.xul
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/base/resources/content/junkMail.xul,v
>retrieving revision 1.29
>diff -u -8 -p -r1.29 junkMail.xul
>--- mailnews/base/resources/content/junkMail.xul	25 Jul 2005 16:25:38 -0000	1.29
>+++ mailnews/base/resources/content/junkMail.xul	2 Feb 2006 09:28:57 -0000
>@@ -91,37 +91,54 @@
>   </hbox>
> 
>   <description width="1">&info1.label;</description>
>   <description width="1">&info2.label;</description>
> 
>   <checkbox id="level" oncommand="conditionallyEnableUI(null);" accesskey="&level.accesskey;" label="&level.label;"/>
> 
>   <vbox align="start">
>+        <grid>

Please use "normal" two-space indentation between new and old - you're using it already in the new code anyway...

>+              <checkbox id="useServerFilter" oncommand="conditionallyEnableUI('useServerFilter');" label="&ispHeaders.label;" 
>+                        accesskey="&ispHeaders.accesskey;"/>

It'd probably better to put each attribute on its own line.

Comment 4

[Giacomo Magnini]

Attachment: Rev. 2

Hopefully addressed all review comments... Thanks Mnyromyr for the IRC help/chat!

Comment 5

[Karsten Düsterloh]

Comment on attachment 215377 [details] [diff] [review]
Rev. 2

It'd be utterly invaluable if you'd test your patches before attaching them. Especially if you cut and paste my typod examples... ;-)

>Index: mailnews/base/resources/content/junkMail.js
>===================================================================
>+      var filename = entry.leafname.match(/(.+)\.sfd$/);
>+
>+      if (filename) {
>+        filename = filename[1];
>+
>+        // if we've already added an item with this name, then don't add it again.
>+        if (!ispHeaderPopup.getElementsByAttribute("value", fileName)[0]) {

r=me if you uppercase those poor diminished 'n's in filename and leafname again, so that the dialog can actually work. :-)

Comment 6

[Giacomo Magnini]

(In reply to comment #5)
> r=me if you uppercase those poor diminished 'n's in filename and leafname
> again, so that the dialog can actually work. :-)

Fixed locally but the dialog still doesn't completely "populate": checkboxes are all unchecked, no default account in dropdown list and no SpamAssassin in the new dropdown... Any idea on why?

Comment 7

[Karsten Düsterloh]

> >+      var filename = entry.leafname.match(/(.+)\.sfd$/);
                  ^                ^
> >+      if (filename) {
                  ^
> >+        filename = filename[1];
                ^          ^
These 5 changes made it work for me.
Did you change the correct file/rebuild chrome? What does the JDC say after opening the dialog?

Comment 8

[Giacomo Magnini]

Attachment: Ok, now it works.

Comment 9

[[e-mail]]

(In reply to comment #3)
>(From update of attachment 210458 [details] [diff] [review])
>>   // set up the manual mark UI
>>   document.getElementById("manualMark").checked = obj.settings.manualMark;
>>   document.getElementById("manualMarkMode").selectedItem = document.getElementById("manualMarkMode" + obj.settings.manualMarkMode);
>Wrap again.
Except that this wasn't relevant to the patch... please don't do random fixes.

Comment 10

[Karsten Düsterloh]

> >>   document.getElementById("manualMarkMode").selectedItem = document.getElementById("manualMarkMode" + obj.settings.manualMarkMode);
> >Wrap again.
> Except that this wasn't relevant to the patch... please don't do random fixes.

Opps, I got carried away, I meant that only for new lines introduced by Giacomo. :-[

Comment 11

[[e-mail]]

Comment on attachment 215501 [details] [diff] [review]
Ok, now it works.

>+// used only to force an initial default value in the combo box (if this entry
exists)
>+const kDefaultFilterServerName = "SpamAssassin";
I don't see the point of this. Just default to the first entry.

> function onJunkMailLoad()
> {
>+  buildServerFilterMenuList();
>   gMessengerBundle = document.getElementById("bundle_messenger");
Leave the bundle first please.

>+  var serverFilterList = document.getElementById("useServerFilterList");
>+  var defaultServerItem = serverFilterList.getElementsByAttribute("value",
obj.settings.serverFilterName)[0] || 
>+                          serverFilterList.getElementsByAttribute("value",
kDefaultFilterServerName)[0];
>+
>+  if (defaultServerItem)
>+    serverFilterList.selectedItem = defaultServerItem;
>+  else
>+    serverFilterList.selectedIndex = 0;
This is just awful. Set the menulist value to the saved name, then only if that
hasn't set the selectedItem should you set the selectedIndex to 0.

>+// helper function called by buildServerFilterMenuList. The function numerates
"e"numerates

>+      // we only care about files that end in .sfd 
>+      var fileName = entry.leafName.match(/(.+)\.sfd$/);
Consider using either /^(.+)\.sfd$/.test(entry.leafName) and RegExp.$1 or
/\.sfd$/.test(entry.leafName) and RegExp.leftContext or entry.leafName.slice(-4)
== ".sfd" and entry.leafName.slice(0, -4)

>+        if (!ispHeaderPopup.getElementsByAttribute("value", fileName)[0]) {
Should use .item(0) instead of [0] to avoid strict JS warnings.

>+          ispHeaderPopup.appendChild(menuitem);
Should use useServerFilterList.appendItem(fileName, fileName);

>+    <grid>
>+      <columns>
>+        <column flex ="1"/>
Nit: kill the space before the = sign.

>+        <row>
>+          <checkbox id="useServerFilter"
>+                    oncommand="conditionallyEnableUI('useServerFilter');"
>+                    label="&ispHeaders.label;" 
>+                    accesskey="&ispHeaders.accesskey;"/>
The problem here is that the checkbox's focus outline stretches across.

>+          <menulist id="useServerFilterList">
>+            <menupopup id="useServerFilter-menupopup">
>+            </menupopup>
If you use appendItem above then the menupopup becomes unnecessary. Either way
remember to use <tag/> for childless tags.

Comment 12

[[e-mail]]

Comment on attachment 215501 [details] [diff] [review]
Ok, now it works.

Oh, and I found a bug - if junk mail controls are disabled then both the new checkbox and menulist need to be disabled.

Comment 13

[Giacomo Magnini]

Do we still want this after bug 257990?

Comment 14

[Karsten Düsterloh]

> Do we still want this after bug 257990?

No, we got this "for free" there. 
I know it's hard to see one's work being dumped... :(
-> Marking as fixed by bug 257990.