496015 - Enable junk UI elements when rss or news junk is enabled

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Kent James (:rkent)]

There is presently some support in the backend for filtering news and rss for junk, enabled by hidden preferences or inherited folder properties. But none of the junk training and marking elements are enabled for news and rss, making the standard junk ui useless.

Allow enabling junk buttons for rss and news when the appropriate properties are set to enable junk in the backend.

Comment 1

[Kent James (:rkent)]

Attachment: WIP but no known issues

This patch, in addition to enabling junk commands in some recently added special cases, disables junk commands for RSS when inappropriate, which are falsely enabled in the current trunk.

Comment 2

[Kent James (:rkent)]

As I work on this, I see it is overkill to have two ways to enable junk processing for news, so let me remove that first before I finish this patch. See bug 510659.

Comment 3

[Kent James (:rkent)]

Attachment: ready for review

This patch, along with bug 198100 which allows filters to be used on the results of junk analysis, will enable at least experimental use of junk analysis of news messages. That is, if you enable junk processing on a news folder, you could then define a filter to, say, kill threads of messages above a certain junk threshold, and that should work. The enabling of junk processing of news is done with an inherited folder property, which I expect would typically be used on a per-folder basis. The enabling of junk on these special folders would normally be provided in an extension, and will be available in my JunQuilla extension for TB and SM.

As a side benefit, this patch disables junk commands for RSS folders in TB and SM, which currently falsely display junk commands as enabled. RSS could also be processed throught the junk filter though if desired by enabling it.

To test this, you need to be able to enable junk processing of news and rss. This can be done on a global basis for testing by setting the string preference "[e-mail]#junk" to the string value "true".

I feel that this patch is worth taking for upcoming SM and TB releases, if for no other reason than getting junk commands disabled for RSS (though my real motivation is experimental use of junk filtering of news.) But I could understand the argument to delay if that is desired instead.

I wasn't sure the proper reviewers for this, but I chose r=Magnus and sr=Neal to start. Please feel free to recommend others if more appropriate.

Comment 4

[Magnus Melin [:mkmelin] (away until July 8)]

Comment on attachment 395271 [details] [diff] [review]
ready for review

Looks ok to me, but i'd suggest changing the name of enableJunkCommands. Sounds like it would enable, when what it does is check if it should be enabled. junkControlsEnabled maybe?

>+ * We always allow junk commands on mail folders. For RSS and News, junk may be

Make "News" be "nntp" for clarity.

>+ * selectively enabled (or disabled) per server or folder using inherited
>+ * folder properites.
>+ *
>+ * @param aFolder   nsIMsgFolder to test
>+ *

Loose the blank line

>+function enableJunkCommands(aFolder)
>+{
>+  // a dummy header perhaps?
>+  if (!aFolder)
>+    return false;
>+
>+  let type;
>+  if (aFolder.server)
>+    type = aFolder.server.type;

All folders have a server, no?

>+// Determine whether junk commands should be enabled on this view.
>+// Junk commands are always enabled for mail. For news and rss, they
>+// may be selectively enabled using an inherited folder property.

Why not use /** */ style documentation for this?

Comment 5

[Kent James (:rkent)]

Attachment: Fixed Magnus's nits

Comment 6

[[e-mail]]

Comment on attachment 395986 [details] [diff] [review]
Fixed Magnus's nits

>+        return gFolderDisplay.selectedMessage &&
>+               junkControlsEnabled(gFolderDisplay.selectedMessage.folder);
Why not call getCommandStatus here?

>+ * Determine if junk commands should be enabled on a folder. This routine
>+ * reflects the decisions made by nsMsgDBFolder::CallFilterPlugins.
What about the mailnews.filter_news_for_junk preference?

>+  return aFolder.getInheritedStringProperty("[e-mail]#junk")
>+         == "true";
Bah, whose wacky idea was that string property name?

>   case nsMsgViewCommandType::runJunkControls:
>     // disable if no messages
>-    // no JMC on news yet
>     // XXX todo, check that we have JMC enabled?
>-    *selectable_p = GetSize() && !mIsNews;
>+    *selectable_p = GetSize() && JunkControlsEnabled(numIndices ? selection[0] : nsMsgViewIndex_None);
Does this command really depend on the selection?

>   case nsMsgViewCommandType::junk:
>   case nsMsgViewCommandType::unjunk:
>-    *selectable_p = haveSelection && !mIsNews;  // no junk for news yet
>+    *selectable_p = haveSelection && JunkControlsEnabled(numIndices ? selection[0] : nsMsgViewIndex_None);
Surely haveSelection implies you have indices. [Not sure why the range count is rechecked, given that we've already counted the selection.]

>+  // For normal mail, junk commands are always enabled.
>+  if (!(mIsNews || mIsRss || mIsNone))
>+    return PR_TRUE;
These are view flags, so may be wrong for cross-server views?

>+        return (GetNumSelectedMessages() > 0 && (gDBView &&
>+                junkControlsEnabled(gDBView.hdrForFirstSelectedMessage.folder)));
This should call gDBView.getCommandStatus (compare cmd_recalculateJunkScore).

>+        return (GetNumSelectedMessages() > 0 && (gDBView &&
>+                junkControlsEnabled(gDBView.hdrForFirstSelectedMessage.folder)));
I'd like to think that this could too.

Comment 7

[Kent James (:rkent)]

Attachment: Eliminate js junkControlsEnabled.

I managed to eliminate the js version of junkControlsEnabled, which is what Neil was implying in his review. That resulted in a few other changes as well.

Comment 8

[Kent James (:rkent)]

(In reply to comment #6)

Additional issues and responses to comments:

> >+ * Determine if junk commands should be enabled on a folder. This routine
> >+ * reflects the decisions made by nsMsgDBFolder::CallFilterPlugins.
> What about the mailnews.filter_news_for_junk preference?

That preference is scheduled for elimination in bug 510659. The inherited folder properties are sufficient control.

> >+  return aFolder.getInheritedStringProperty("[e-mail]#junk")
> >+         == "true";
> Bah, whose wacky idea was that string property name?

That would be mine. "dobayes" is really the preference, the rest is an RDF-inspired (which I know is frowned on in Mozilla) globally unique identifier for the "junk" trait in the bayesian classifier.

> >   case nsMsgViewCommandType::runJunkControls:
> >     // disable if no messages
> >-    // no JMC on news yet
> >     // XXX todo, check that we have JMC enabled?
> >-    *selectable_p = GetSize() && !mIsNews;
> >+    *selectable_p = GetSize() && JunkControlsEnabled(numIndices ? selection[0] : nsMsgViewIndex_None);
> Does this command really depend on the selection?

runJunkControls is used for both the case where it means per folder (and does not depend on selection) and for the case where we run controls on the selection. But I managed to move that complexity to js command controllers, so this is independent of selection in the C++ view.

> 
> >+  // For normal mail, junk commands are always enabled.
> >+  if (!(mIsNews || mIsRss || mIsNone))
> >+    return PR_TRUE;
> These are view flags, so may be wrong for cross-server views?

I misunderstood the scope of mIsNone in my previous patch, so I replaced this with an explicit check for a cross-server virtual folder.

Comment 9

[Kent James (:rkent)]

Neil, any chance you can complete your sr soon?

Comment 10

[[e-mail]]

Comment on attachment 396962 [details] [diff] [review]
Eliminate js junkControlsEnabled.

Sorry for the delay on this.

Comment 11

[Kent James (:rkent)]

Attachment: unbitrotted, patch to checkin

Comment 12

[Kent James (:rkent)]

Attachment: one more time really debitrotted now (I hope)

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

pushed:
http://hg.mozilla.org/comm-central/rev/23f393649289