335846 - Need a Junk Preferences Panel in Seamonkey

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Scott MacGregor]

As part of the work in Bug #257990, the following spam settings are becoming global instead of account specific:

loggingmanualMark, manualMarkMode and spamLogging

seamonkey is going to need a Mail News pref panel for junk settings to allow users to change these new global settings.

257990 will be landing on the trunk and the 1.8.1 branch, so this work would have to go into both places too.

Comment 1

[Karsten Düsterloh]

Attachment: SeaMonkey pref panel + help changes, v1

This is the first go at a SeaMonkey pref panel, plus needed help changes.
The diff is against trunk, so my slight changes to the new files of bug 257990 am-junk.js/.dtd (I readded the markAsReadOnSpam pref) result in these files marked as entirely new. You should have attachment 220151 [details] [diff] [review] applied to your tree as a prerequisite.

Comment 2

[Scott MacGregor]

Karsten, thank you for taking this on. 

Would it be easier for me to land the patch in 257990 so it becomes easier to test and see the pref panel patch for this bug? During the time between when I check in and when this bug lands, the only real user impact would be an inability to see the junk mail logs which could be something one could get by without for a day or two of nightly builds I think. That will also make it easier for me to see what changes need to be added to am-junk.xul/.js to support this patch as well. 

Comment 3

[Karsten Düsterloh]

Attachment: diff of am-junk.xul/.dtd between bug 257990 and here

Diffing my changed versions of am-junk.xul/.dtd against their initial versions in bug 257990. (The change to am-junk.js in attachment 221987 [details] [diff] [review] here is not needed, so I left it out.)

Scott, I'm fine with 257990 leading the way, go ahead.

Comment 4

[Scott MacGregor]

Attachment: [checked in trunk and branch] seamonkey changes to migrate several acct. prefs to their global equivalents

The suite will need this patch (or something like it) to migrate the settings which have become global. 

I also migrated the mark spam as read pref as well in this patch. I think that pref should be global and not account specific, thus adding it to the seamonkey pref panel (I just added it to the thunerbird options panel) instead of in the account panel.

Feel free to add this patch to your existing seamonkey pref panel patch or review it and I can check it in. I'm fine either way.

Comment 5

[Giacomo Magnini]

Just a sidenote: when updating the help doc, please be sure to double check the UI labels, especially with upper/lower case handling. As it is now, it's very unconsistent.

Comment 6

[[e-mail]]

Scott, mark as read on spam still seems to be implemented per-server.

Comment 7

[Karsten Düsterloh]

Comment on attachment 222536 [details] [diff] [review]
[checked in trunk and branch] seamonkey changes to migrate several acct. prefs to their global equivalents

r=me; this has already been checked in on trunk as part of bug 257990.

Comment 8

[Karsten Düsterloh]

Attachment: SeaMonkey pref panel, v2

Patch against the current trunk after bug 257990 got checked in.

Comment 9

[Karsten Düsterloh]

Attachment: SeaMonkey help changes, v2

The respective help changes.

Comment 10

[[e-mail]]

Comment on attachment 222742 [details] [diff] [review]
SeaMonkey pref panel, v2

>Index: mailnews/base/prefs/resources/content/pref-junk.js
>@@ -0,0 +1,44 @@
No licence :-(

>+  window.openDialog("chrome://messenger/content/junkLog.xul",
>+                    "junkLog", 
>+                    "chrome,modal,titlebar,resizable,centerscreen",
>+                    null);
Why the null?

>+  // if the user says no, then just fall out
>+  if (!promptService.confirm(window, title, text))
Might be worth using confirmEx so you can make No the default.

>+<!ENTITY junk.label                       "Junk">
>+<!ENTITY pref.junk.title "Junk">
Change these (and any I missed) to "Junk Settings" perhaps?

>+<!ENTITY junkSettings.caption "Global Junk Settings">
Either way I think I'd like this to say "Global Junk Mail Settings".

>-  // for security, disable JS
>-  gLogView.docShell.allowJavascript = false;
>+  gLogView.docShell.allowJavascript = false; // for security, disable JS
Why this change?

>+  var directoryService =  Components.classes["@mozilla.org/file/directory_service;1"]
>+                            .getService(Components.interfaces.nsIProperties);
.s not lined up.

>+    ioService = Components.classes["@mozilla.org/network/io-service;1"]
>+                  .getService(Components.interfaces.nsIIOService);
Again.

sr=me with these fixed or explained.

Comment 11

[Karsten Düsterloh]

> >Index: mailnews/base/prefs/resources/content/pref-junk.js
> >@@ -0,0 +1,44 @@
> No licence :-(

Oops.

> >+<!ENTITY junk.label                       "Junk">
> >+<!ENTITY pref.junk.title "Junk">
> Change these (and any I missed) to "Junk Settings" perhaps?

I considered that, but I think that the word "settings" does not hold any significance for a pref panel - what else should it contain than settings?
I actually don't like it in the mail account prefs, but there we have "server settings", too, so it probably doesn't too much.

> >-  // for security, disable JS
> >-  gLogView.docShell.allowJavascript = false;
> >+  gLogView.docShell.allowJavascript = false; // for security, disable JS
> Why this change?

To get that line closer to that above:
|    gLogView = document.getElementById("logView");
| -
| -  // for security, disable JS
| -  gLogView.docShell.allowJavascript = false;
I overhauled almost the whole function, so a blame history change shouldn't be that problematic.

> .s not lined up.

Bah, I definitely need a better editor on Mac. :-/

I'll do that other requested fixes. Ian, do you like that changed before your review?

Comment 12

[Ian Neal]

Comment on attachment 222742 [details] [diff] [review]
SeaMonkey pref panel, v2

>Index: mailnews/base/prefs/resources/content/pref-junk.js
>===================================================================
>+function Startup()
>+{
>+  UpdateManualMarkMode(document.getElementById('manualMark').checked);
>+  UpdateJunkLogButton(document.getElementById('enableJunkLogging').checked);
>+}
>+
>+function UpdateManualMarkMode(aEnableRadioGroup)
>+{
>+  document.getElementById('manualMarkMode').disabled = !aEnableRadioGroup;
You need to take into account locked prefs here I think.

>+}
>+
>+function UpdateJunkLogButton(aEnableButton)
>+{
>+  document.getElementById('openJunkLogButton').disabled = !aEnableButton;
Alot of buttons in prefs can be disabled by a pref setting, should we look having this one pref controlable?

Comment 13

[Ian Neal]

Comment on attachment 222743 [details] [diff] [review]
SeaMonkey help changes, v2

>Index: extensions/help/resources/locale/en-US/mail_help.xhtml
>===================================================================
>@@ -4703,16 +4730,56 @@ to filter unwanted mail.</p>
.
.
.
>+  <li><strong>Mark messages determined to be junk as read</strong>: Select this
>+    option to mark junk messages as read, so they will not show up as new.</li>
>+  <li><strong>Enable junk filter logging</strong>: Select this option to allow
>+    logging the history of Junk mail detections. Click the
There is space to move part of the line below up a line.
>+    <strong>Show log</strong> button to open a dialog showing this log.</li>
>+  <li><strong>Reset training data</strong>: Click this button to clear the
>+    training data of the adaptive junk filter. Since this will effectively
>+    destroy your personal junk profile, you will be asked for confirmation.</li>
The line above goes over the 80 column limit.

Comment 14

[Scott MacGregor]

(In reply to comment #7)
> (From update of attachment 222536 [details] [diff] [review] [edit])
> r=me; this has already been checked in on trunk as part of bug 257990.
> 

I didn't mean to check that file in before you had a chance to review it. Sorry about that. I'll land it on the branch too.

Comment 15

[Karsten Düsterloh]

Attachment: SeaMonkey pref panel, v3: incl. locked buttons

Made buttons lockable; added some licences; fixed most nits; renamed "Junk" panel to "Junk Mail" panel. Carrying over sr+, rerequesting r?.

Comment 16

[Karsten Düsterloh]

Attachment: SeaMonkey help changes, v3: updated to match code v3 + nits

Help changes synced up to the code changes (Junk->Junk Mail) + fixed nits.

Comment 17

[Karsten Düsterloh]

Comment on attachment 222939 [details] [diff] [review]
SeaMonkey pref panel, v3: incl. locked buttons

bah, wrong patch

Comment 18

[Karsten Düsterloh]

Attachment: SeaMonkey pref panel, real v3: incl. locked buttons

Correct version.

Comment 19

[Karsten Düsterloh]

Attachment: SeaMonkey help changes, real v3: updated to match code v3 + nits [checked in on trunk and MOZILLA_1_8_BRANCH]

Matching help patch for real code patch v3.

Comment 20

[[e-mail]]

Comment on attachment 222952 [details] [diff] [review]
SeaMonkey pref panel, real v3: incl. locked buttons

>+function UpdateManualMarkMode(aEnableRadioGroup)
>+{
>+  document.getElementById("manualMarkMode").disabled = !aEnableRadioGroup;
>+}
You forgot the lockpref test...

>+function UpdateResetTrainingDataButton()
>+{
>+  var resetTrainingDataButton = document.getElementById("resetTrainingData");
>+  var lockpref = resetTrainingDataButton.getAttribute("prefstring");
>+  var isLocked = parent.hPrefWindow.getPrefIsLocked(lockpref);
>+  resetTrainingDataButton.disabled = isLocked;
>+}
This button doesn't need a test; because it has a prefstring it is automatically disabled if the pref is locked. The problem with the other button and the radiogroup is to avoid making it appear enabled by mistake.

Comment 21

[Stefan [:stefanh]]

Comment on attachment 222953 [details] [diff] [review]
SeaMonkey help changes, real v3: updated to match code v3 + nits [checked in on trunk and MOZILLA_1_8_BRANCH]

 <p>[<a href="#organizing_your_messages">Return to beginning of section</a>]</p>
 
+
+
 <h1 id="controlling_junk_mail">Controlling Junk Mail</
.
.
.
 </div>
 
+
 <h2 id="using_junk_mail_controls">Using Junk Mail Controls</h2>
.
.
.
   </li>
-  <li>Open the Tools menu and choose <q>Junk Mail Controls...</q>. Enable the
-    feature for your mail account, and &brandShortName; will automatically
-    classify incoming messages. (See
-    <a href="#junk_controls_options">Junk Mail Controls Options</a>.)</li>
+  
+  <li>Open the Edit menu, and choose Mail &amp; Newsgroups Account Settings.
.
.
.
 <p>[<a href="#controlling_junk_mail">Return to beginning of section</a>]</p>
 
+
 <h2 id="junk_controls_options">Junk Mail Controls Options</h2>
.
.
.
 <p>[<a href="#controlling_junk_mail">Return to beginning of section</a>]</p>
 
+
 <h2 id="junk_controls_and_filters">Junk Mail Controls and Filters</h2>
.
.
.
 <p>[<a href="#controlling_junk_mail">Return to beginning of section</a>]</p>
 
+
+
 <h1 id="importing_mail_from_other_programs">Importing Mail from Other
   Programs</h1>
.
.
.
 <p>[<a href="#mail_and_newsgroups_account_settings">Return to beginning of
   section</a>]</p>
 
+
+<h2 id="junk_settings">Mail &amp; Newsgroups Account Settings - Junk
+  Settings</h2>
.
.
.
<p>[<a href="#mail_and_newsgroup_preferences">Return to beginning of
   section</a>]</p>
 
+
+<h2 id="global_junk_settings">Mail &amp; Newsgroups Preferences - 
.
.
.
+<p>[<a href="#mail_and_newsgroup_preferences">Return to beginning of
+  section</a>]</p>
+
+
 <h2 id="labels">Mail &amp; Newsgroups Preferences - Labels</h2>

Those empty lines you're adding doesn't conform with the rest of the file and the other help files, can you please remove them?

Comment 22

[Stefan [:stefanh]]

+<p>[<a href="#mail_and_newsgroup_preferences">Return to beginning of
+  section</a>]</p>
+
+
 <h2 id="labels">Mail &amp; Newsgroups Preferences - Labels</h2>

That is, one empty line is enough ;)

Comment 23

[Karsten Düsterloh]

> +<p>[<a href="#mail_and_newsgroup_preferences">Return to beginning of
> +  section</a>]</p>
> +
> +
>  <h2 id="labels">Mail &amp; Newsgroups Preferences - Labels</h2>
> 
> That is, one empty line is enough ;)

That help source file is almost unreadable. Since it's XHTML, adding empty lines and breaking lines per logic instead of by line size would help reading/editing it significantly, without altering the actual layout a single iota. 
I will, of course, do so if Ian requests it, but I really think that this won't be enhancements...

Comment 24

[Karsten Düsterloh]

Attachment: SeaMonkey pref panel, v4: corrected lockpref usage

Updated to Neil's comment.

Comment 25

[Karsten Düsterloh]

*** Bug 339971 has been marked as a duplicate of this bug. ***

Comment 26

[Ian Neal]

Comment on attachment 223079 [details] [diff] [review]
SeaMonkey pref panel, v4: corrected lockpref usage

>Index: mailnews/base/prefs/resources/content/pref-junk.js
>===================================================================
>+function Startup()
>+{
>+  UpdateManualMarkMode(document.getElementById("manualMark").checked);
>+  UpdateJunkLogButton(document.getElementById("enableJunkLogging").checked);
>+}
>+
>+function IsLocked(aElement)
>+{
>+  return parent.hPrefWindow.getPrefIsLocked(aElement.getAttribute("prefstring"));
>+}
>+
>+function UpdateManualMarkMode(aEnableRadioGroup)
>+{
>+  var manualMarkGroup = document.getElementById("manualMarkMode");
>+  manualMarkGroup.disabled = !aEnableRadioGroup || IsLocked(manualMarkGroup);
>+}
>+
>+function UpdateJunkLogButton(aEnableButton)
>+{
>+  var openJunkLogButton = document.getElementById("openJunkLog");
>+  openJunkLogButton.disabled = !aEnableButton || IsLocked(openJunkLogButton);
>+}
The two functions above just do the same thing to two different elements, so perhaps you could just make the three functions into one? (3rd one being IsLocked).

Comment 27

[Karsten Düsterloh]

Attachment: SeaMonkey pref panel, v5: consolidated update functions [checked in on trunk and MOZILLA_1_8_BRANCH]

Fixed IanN's review comments.

Comment 28

[Giacomo Magnini]

Please take a look at bug 257990 comment 47, and please close bug 180087 as it is fixed by now, right?

Comment 29

[Karsten Düsterloh]

> Please take a look at bug 257990 comment 47

Fixed typo on trunk and MOZILLA_1_8_BRANCH.

> close bug 180087 as it is fixed by now, right?

Right. Done.
Thanks for pointing out these glitches!