419356 - Allow extensions to add custom filter actions

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement, P1)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Extensions like lightning should be able to add custom filter actions. From a UI standpoint this seems to be possible, I was successfully able to add a filter item to the message list, but actually executing the rule on mails is C++ code I cannot hook into. Two requests:


* Make it easier to add actions to the menulist of actions:
  This is currently in a binding, the content can only be overridden. This makes
  it hard for two different extensions to add actions without overriding each
  other.


* Allow actions to be executed on incoming emails etc. This probably includes
  rethinking the logic used. Currently, the filter actions are indexed in an
  interface. This makes it hard for extensions to find unused numbers. As an idea
  extensions might implement an interface using a contract id like
  @mozilla.org/whatever/message-action;1?type=mycustomtype
  where mycustomtype is the short name used in the bindings.


This bug is a requirement for better Lightning integration. See calendar bug 415550 for use case.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Fixing this bug could also open the gates for a number of other bugs.

Comment 2

[Karsten Düsterloh]

JFTR: This is (in parts) a dupe of bug 11037.

Comment 3

[David Ascher (:davida)]

Philipp: what do you mean, "open the gates"?  That we shouldn't fix it?  Or that fixing it would let us fix other bugs?

Comment 4

[David Ascher (:davida)]

This seems like real goodness, so accepting as blocking.  Would be good to have an owner.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

David, I meant that fixing it would allow to fix a bunch of other bugs. See the Blocks list.

Comment 6

[Mark Banner (:standard8)]

Retriaging according to new policy for flags.
https://wiki.mozilla.org/Thunderbird:Release_Driving
(bugs marked wanted- don't indicate we wouldn't accept patches, but that they're not going to be the focus for release drivers)

Comment 7

[Kent James (:rkent)]

From an implementation perspective, I don't see the difference between this and bug 11037. I would dupe it to 11037.

Comment 8

[Karsten Düsterloh]

Extensions might want to implement filter actions written in C++...

Comment 9

[Joshua Cranmer [:jcranmer]]

(In reply to comment #7)
> From an implementation perspective, I don't see the difference between this and
> bug 11037. I would dupe it to 11037.

Bug 11037, AIUI: let the user input some JS to execute as a filter action.
This bug, AIUI : let an extension author provide custom filter actions.

Comment 10

[Kent James (:rkent)]

I've been working on this for the last few days, so I should assign it to myself.

Comment 12

[Kent James (:rkent)]

Attachment: Custom filters

Another attachment I will add next is a demo extension that adds four custom actions of varying complexity.

There are no known problems with this patch. But there are some issues that might result in future bugs.

1) The action string is implemented as an ACString in the existing code, and I have carried that through. But for some possible uses, for example the Append to Subject action, an AString would be really needed.

2) I have supported the async handling in the filter-after-the-fact code. But that code itself has problems, as it always moves to the next filter and not the next action (bug 448337).

3) In some existing filter action code, an array of headers is passed, while in others a message header is passed. This is partly related to whether the action implementation is expected to use a folder or header based operation. There are some subtle tradeoffs between folder-based actions, database-based actions, and header-based actions that are not going to be understood by the average extension writer - plus some misuses in the current code (such as using mDatabase->SetStringProperty in contexts where the header has not been added to the database). I've done nothing to help with that.

4) There are interactions in IMAP with things like the moveCoalescer that I have not given hooks to. The assumption is that custom actions will be able to determine scope and context, and take appropriate action. (See for example the Copy As Unread custom action in the demo.)

In spite of these issues, the custom actions work really well for simple things like removing stars. Even the complex Copy As Unread works on both IMAP and Local folders.

I'll add review flags after I've made another review pass myself.

Comment 13

[Kent James (:rkent)]

Attachment: Demo extension with four custom actions

This extensio adds four custom actions.

1) Append text to the subject
2) Remove a star
3) Remove a tag
4) Copy a message, and mark the copy as unread.

Note the "remove a star" action description is in Russian to demonstrate international text.

This is not intended to be a finished extension, but simply a demonstration of how to use features of custom actions.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 341079 [details] [diff] [review]
Custom filters

+		nsIMsgFilterCustomAction.idl \

I don't see this file...

 interface nsMsgFilterAction {
...
+    // Custom filters
+    const long Custom=100;

Is Custom a filter action dependent on what the extension has defined? If so, I don't see why you've left the gap in the numbers. It implies (at a quick glance) that its used in a file or something, and the space was left for other action types - if that is the case, then I would say a gap of 82 isn't enough.

Comment 15

[Kent James (:rkent)]

Attachment: Includes nsIMsgFilterCustomAction.idl

Oops, forgot to add the file.

Re the "100" for the custom action type: there are places in the code where non-custom actions have a list of actions, and the custom action is missing from that list. So the custom action needs to be the last action. It could be bigger - though I would expect that in the future most new actions would use the custom format, not the non-custom format. It might be a good idea, for example, to add a laundry list of possible actions to a disabled-by-default list of preinstalled custom actions, with UI to enable them. Otherwise the list of actions gets long and scary.

Comment 16

[Joshua Cranmer [:jcranmer]]

(In reply to comment #12)
> 1) The action string is implemented as an ACString in the existing code, and I
> have carried that through. But for some possible uses, for example the Append
> to Subject action, an AString would be really needed.

AUTF8String ?
> 3) In some existing filter action code, an array of headers is passed, while in
> others a message header is passed. This is partly related to whether the action
> implementation is expected to use a folder or header based operation. There are
> some subtle tradeoffs between folder-based actions, database-based actions, and
> header-based actions that are not going to be understood by the average
> extension writer - plus some misuses in the current code (such as using
> mDatabase->SetStringProperty in contexts where the header has not been added to
> the database). I've done nothing to help with that.

At some point, you'll need documentation saying what can and can't be done with message headers outside of the database. I know that the last time I tried messing with threads outside the db, it blew up in my face.

Comment 17

[Kent James (:rkent)]

Attachment: First revision

This is now ready for review. Revised demo extension to follow. Note I remove trailing spaces in searchWidgets.xml, and quite a few lines had trailing spaces that I did not otherwise change.

Comment 18

[Kent James (:rkent)]

Attachment: First revision of demo extension

This has four custom actions of varying complexity.

Comment 19

[[e-mail]]

Comment on attachment 342032 [details] [diff] [review]
First revision

>+.ruleactiontarget[isCustom="true"] {
>+  display: -moz-deck;
>+  -moz-binding: url("chrome://messenger/content/searchWidgets.xml#ruleactiontarget-custom");
>+}
I think you should use three custom targets (menu, textbox and folder), since you already have working bindings for the latter two.

>+interface nsIMsgFilterCustomDisplay
>+{
>+  const long none =   0;   // no value box is shown
>+  const long text =   1;   // text box
>+  const long menu =   2;   // custom filter supplies a menu using values and labels
>+  const long folder = 3;   // folder picker displayed. value returned is the folder URI
>+};
IIRC module style seems to be to omit the "I" for "interfaces" that purely exist to define constants.

>+  nsIArray values(in nsIMsgFolder actionFolder, in nsMsgFilterTypeType filterType);
This really ought to be a string array, although we don't have a UTF-8 array (string is ASCII and wstring is UTF-16).

>+  nsIArray labels(in nsIMsgFolder actionFolder, in nsMsgFilterTypeType filterType);
And this should be a string array too.

>+  void apply(in nsIArray msgHdrs /* nsIMsgDBHdr array */,
>+             in AUTF8String actionValue,
>+             in nsIMsgCopyServiceListener copyListener,
>+             in nsMsgFilterTypeType filterType,
>+             in nsIMsgWindow msgWindow);
>+
>+  /* does this action start an async action? If so, a copy listener must
>+   * be used to continue filter processing after the action. This only
>+   * applies to after-the-fact (manual) filters. Call OnStopCopy when done
>+   * using the copyListener to continue.
>+   */
>+  readonly attribute boolean isAsync;
Why not make apply return the boolean?

>+    /**
>+     * add a custom filter action
>+     *
>+     * @param  aAction   the custom action to add
>+     */
>+    void addCustomAction(in nsIMsgFilterCustomAction aAction);
>+
>+    /**
>+     * get the array of custom actions
>+     *
>+     * @return   array of nsIMsgFilterCustomAction objects
>+     */
>+    nsIArray getCustomActions();
Neither of your callers specifically wants an nsIArray. One wants an enumeration of some sort, while the other wants a hash lookup. I think we might have a hash table that provides an enumeration of values.

Comment 20

[Kent James (:rkent)]

Attachment: Changed to CSS-driven custom bindings (rev 2)

Implementing and more Neil's suggestion that I reuse the existing bindings, I've changed the approach so that the custom action's UI is set from CSS to either an existing binding, or to user-defined xml. That eliminates the value and label lookup for the menu type, as the user can now do that themself in a XBL binding.

I switched to an enumerator and lookup in the custom action interface as suggested.

I did not move the isAsync to the return of the apply action. isAsync is rather rare, really a special feature of the after-the-fact filters. I am reluctant to elevate it to the main apply interface for that reason, preferring to let it default to false in most cases.

I also added logging for the custom filters.

Comment 21

[Kent James (:rkent)]

Attachment: Sample extension (rev2)

I added a demo that used a custom XBL binding, since the previous demo of a custom menu can now be implemented with the existing binding for tags.

Comment 22

[[e-mail]]

Comment on attachment 342847 [details] [diff] [review]
Changed to CSS-driven custom bindings (rev 2)

This patch might be much simpler than the previous one, which is good, but I still think bienvenu should review too.

>+      if (filterActionString.length)
Don't need the .length

>-            document.getAnonymousNodes(document.getAnonymousNodes(this)[0])[0].value = filterActionStr;
This has to be done first so that the XBL is ready for the switch.

>+              switch (gFilterActionStrings[aFilterAction.type])
If we switched on the type, would that avoid special-casing custom? I see you almost managed to avoid it later on. I suppose that would rely on custom filters not duplicating names.

>+                menuitem.setAttribute("label", "***");
>+                menuitem.setAttribute("value", filterActionStr);
>+                menuitem.setAttribute("isCustom", "true");
>+                menuitem.hidden = true;
I think we should set a custom label e.g. [Missing action %S] also maybe the menuitem should be disabled instead of hidden.

>+                consoleService.logMessage(scriptError);
Apart from the icon I don't see why you don't just log a string.

>+                    for (var i = 0; i < gCustomActions.length; i++)
>+                      if (gCustomActions[i].id == filterActionString)
Would it be better to use the filter service lookup method?

> NS_IMETHODIMP nsMsgRuleAction::SetPriority(nsMsgPriorityValue aPriority)
> {
>-  NS_ENSURE_TRUE(m_type == nsMsgFilterAction::ChangePriority,
>+  NS_ENSURE_TRUE(m_type == nsMsgFilterAction::ChangePriority ||
>+                 m_type == nsMsgFilterAction::Custom,
>                 NS_ERROR_ILLEGAL_VALUE);
Surely custom actions are only allowed a strValue?

>+  if (m_customAction)
>+  {
>+    // found the correct custom action
>+    NS_ADDREF(*aCustomAction = m_customAction);
>+    return NS_OK;
>+  }
>+  return NS_ERROR_FAILURE; // custom action not found
Prefer if (!m_customAction)\n  return NS_ERROR_FAILURE;

Comment 23

[Kent James (:rkent)]

Attachment: Fixed Neil's nits

Fixed Neil's nits, with the exception of:

(In reply to comment #22)
> (From update of attachment 342847 [details] [diff] [review])
> This patch might be much simpler than the previous one, which is good, but I
> still think bienvenu should review too.
>
That was an oversight, I returned here to my previous pattern for r/sr = neil/bienvenu

> >-            document.getAnonymousNodes(document.getAnonymousNodes(this)[0])[0].value = filterActionStr;
> This has to be done first so that the XBL is ready for the switch.
>
I don't think that is true, and I did not change it. It was moved to the bottom because filterActionStr, for custom, is set in the switch. The main value that affects the XBL bindings is set in initializeDialog(filter) in FilterEditor.js
> >+                menuitem.setAttribute("label", "***");
> >+                menuitem.setAttribute("value", filterActionStr);
> >+                menuitem.setAttribute("isCustom", "true");
> >+                menuitem.hidden = true;
> I think we should set a custom label e.g. [Missing action %S] also maybe the
> menuitem should be disabled instead of hidden.
>
I changed this to a string, but didn't add the %S because, if I do, the line is too long in the filter action list. The user sees the invalid action id in the error console instead (though the error message there is untranslated). I'm not sure of the best way to handle this.

> >+                consoleService.logMessage(scriptError);
> Apart from the icon I don't see why you don't just log a string.
>
Well, because I want the error icon. Is there an easier way to get it?

> >+                    for (var i = 0; i < gCustomActions.length; i++)
> >+                      if (gCustomActions[i].id == filterActionString)
> Would it be better to use the filter service lookup method?
>
I considered that earlier, but decided against it because the list of custom actions is likely to be short, so the js version should be fine.

Comment 24

[[e-mail]]

Comment on attachment 342924 [details] [diff] [review]
Fixed Neil's nits

>+    // Custom Action. Leave lots of room for additional non-custom actions.
>+    const long Custom=100;
I don't suppose a more magic value, such as -1, might work better?

>+          if (gFilterActionStrings.indexOf(this.menulist.value) < 0)
>+            this.parentNode.setAttribute('isCustom', 'true');
>+          else
>+            this.parentNode.removeAttribute('isCustom');
I'm wondering whether it's worth maintaining the isCustom attribute and use the indexOF test to detect custom actions throughout.

>-            document.getAnonymousNodes(document.getAnonymousNodes(this)[0])[0].value = filterActionStr;
When I saw this my eyes glazed over :-(

>+            var fa = Components.interfaces.nsMsgFilterAction;
var nsMsgFolderAction = Components.interfaces.nsMsgFilterAction; is the style we use in other places.

>+                    var customAction = null;
>+                    for (var i = 0; i < gCustomActions.length; i++)
>+                      if (gCustomActions[i].id == filterActionString)
>+                      {
>+                        customAction = gCustomActions[i];
>+                        break;
>+                      }
>+                    if (customAction)
>+                      errorString = customAction.validateActionValue(value,
>+                                                   gFilterList.folder,
>+                                                   gFilterType);
Nit: you only use customAction once, so you may as well write gCustomActions[i].validateActionValue(...) inside the loop instead.
Note that you need to prompt the return of validateActionValue; simply falling through as you do does not work as the error code assumes that string is the name of a property in the filter string bundle.

>+            var actionItem = document.getAnonymousNodes(actionTarget);
Out of interest, can you produce a case where this is null?
[Edit: there probably is, given the following code!]

>+                default:
>+                  if (actionItem)
>+                    filterAction.strValue = actionItem[0].value;
Why not move the custom action code here?

>-                 m_type == nsMsgFilterAction::CopyToFolder,
>+                 m_type == nsMsgFilterAction::CopyToFolder ||
>+                 m_type == nsMsgFilterAction::Custom,
Doesn't this change belong in the [GS]etStrValue methods?

>+    rv = filterService->GetCustomAction(m_customId, getter_AddRefs(m_customAction));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  if (!m_customAction)
>+    return NS_ERROR_FAILURE; // custom action not found
Won't the filter service have already failed by now?

>+      PRBool haveName = PR_FALSE;
...
>+      {
>+        customAction->GetName(filterActionName);
>+        if (filterActionName.Length())
>+          haveName = PR_TRUE;
>+      }
>+      if (!haveName)
Get rid of haveName and just use if (filterActionName.IsEmpty())

>+        nsAutoString errorMessage;
>+        errorMessage = NS_LITERAL_STRING("filterMissingCustomAction");
>+        bundle->GetStringFromName(errorMessage.get(), getter_Copies(filterActionName));
This is bad string fu but fortunately all you need to use is
NS_LITERAL_STRING("filterMissingCustomAction").get()

>         else if (type == nsMsgFilterAction::Forward || type == nsMsgFilterAction::Reply
>-          || type == nsMsgFilterAction::AddTag)
>+          || type == nsMsgFilterAction::AddTag || type == nsMsgFilterAction::Custom)
Would you mind fixing the wrapping so that the || is at the end of the line?

>+        PRBool isAsync;
>+        customAction->GetIsAsync(&isAsync);
>+        if (isAsync)
>+          return NS_OK;
Nit: should deal with failure somehow e.g. init isAsync to PR_FALSE

>+NS_IMETHODIMP nsMsgFilterService::GetCustomAction(const nsACString & aId,
>+                                    nsIMsgFilterCustomAction** aResult)
Did you mean:
NS_IMETHODIMP
nsMsgFilterService::GetCustomAction(const nsACString & aId,
                                    nsIMsgFilterCustomAction** aResult)

Comment 25

[Kent James (:rkent)]

Attachment: Fixed more Neil issues

I implemented all of Neil's issues. One small variation: I left the isCustom attribute on the menu dropdown, but removed it from the action lists.

Comment 26

[[e-mail]]

Comment on attachment 345245 [details] [diff] [review]
Fixed more Neil issues

>+              var errorString = errorString ? 
>+                                  gFilterBundle.getString(errorString) :
>+                                  customError;
Nit: you've already declared errorString, no need to do it again.

>             filterAction.type = gFilterActionStrings.indexOf(filterActionString);
>             var actionTarget = document.getAnonymousNodes(this)[1];
>+            var actionItem = document.getAnonymousNodes(actionTarget);
>+            if (filterAction.type < 0)
>+            {
>+              filterAction.type = Components.interfaces.nsMsgFilterAction.Custom;
So, this is already true, due to our innovative choice of -1 as the const :-)
Might be worth documenting that so nobody tries to change it ;-)

>+              filterAction.customId = filterActionString;
You could change the switch to switch (filterAction.type) (changing the strings to the respective constants) then the last entry becomes:
case nsMsgFilterAction.Custom:
  filterAction.customId = filterActionString;
  // fall through to set the value
default:
  if (actionItem)
    filterAction.strValue = actionItem[0].value;

>+              if (actionItem)
>+                filterAction.strValue = actionItem[0].value;
This already happens as a result of the default branch of the switch.

>+            var listBox = this.mListBox; // "this" will go away soon
What does this mean?

>   NS_ENSURE_TRUE(m_type == nsMsgFilterAction::MoveToFolder ||
>-                 m_type == nsMsgFilterAction::CopyToFolder,
>+                 m_type == nsMsgFilterAction::CopyToFolder ||
>+                 m_type == nsMsgFilterAction::Custom,
>                  NS_ERROR_ILLEGAL_VALUE);
Not sure what this is doing here, custom actions only have string values.

Comment 27

[Kent James (:rkent)]

Attachment: One more time

More Neil fixes implemented.

Re:
>+            var listBox = this.mListBox; // "this" will go away soon
What does this mean?

I slipped in a fix for an unrelated bug, because it was causing error messages in my console during testing. I clarified the comment though. The original code was supposed to disable remove when you have only one row; now it does.

Comment 28

[[e-mail]]

(In reply to comment #27)
>(In reply to commant #26)
>>(From update of attachment 345245 [details] [diff] [review])
>>>+            var listBox = this.mListBox; // "this" will go away soon
>>What does this mean?
>I slipped in a fix for an unrelated bug, because it was causing error messages
>in my console during testing. I clarified the comment though. The original code
>was supposed to disable remove when you have only one row; now it does.
Ah yes, I remember now, a recent XBL change causes all the members to get deleted when the element is removed from the document.

Comment 29

[David :Bienvenu]

Comment on attachment 345765 [details] [diff] [review]
One more time

very nice, this is a challenging problem.

+    var filterActionString;
+    if (filterAction.type == Components.interfaces.nsMsgFilterAction.Custom)
+      filterActionString = filterAction.customId;
+    else
+      filterActionString = gFilterActionStrings[filterAction.type];
+    newActionRow.setAttribute('value', filterActionString);

this can just be newActionRow.setAttribute('value', filterAction.type == Components.interfaces.nsMsgFilterAction.Custom ? filterAction.customID : gFilterActionStrings[filterAction.type];

(with the appropriate line wrapping)

this handles suite but not TB:

diff --git a/suite/locales/en-US/chrome/mailnews/filter.properties b/suite/locales/en-US/chrome/mailnews/filter.properties
--- a/suite/locales/en-US/chrome/mailnews/filter.properties
+++ b/suite/locales/en-US/chrome/mailnews/filter.properties

Do you really need to bump the file version?

+const PRInt16 kFileVersion = 10;
+const PRInt16 kCustomActionVersion = 10;

the filter code is generally pretty good about loading and maintaining strings it doesn't know the meaning of. It's better not to bump the version if you don't have to; otherwise, I think we won't be able to run older versions of TB against filter files in profiles that have been touched by newer versions of TB

Comment 30

[David :Bienvenu]

Comment on attachment 345765 [details] [diff] [review]
One more time

modulo the nits, and the question about whether the filing version has to change, sr=me.

Comment 31

[Kent James (:rkent)]

Attachment: Fixed David's issues

Carrying over r/sr  Fixed David's nits.

Bienvenu: filter.properties was changed in both /suite and /mail already.  Did I miss something, or did you not notice that? Please confirm that this issue is resolved before I ask for checkin.

Comment 32

[David :Bienvenu]

yes, you're right of course; it was hiding right there at the very top of the diff :-)

Comment 33

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/74a7415a42a9 I'm guessing this is probably fixed, but I'll leave it to Kent to mark.

Comment 34

[Joshua Cranmer [:jcranmer]]

According to Kent, this is fixed. This wants documentation somewhere, so adding dev-doc-needed.

Comment 35

[mikedoak]

Thanks, Guys for all your comments and hard work!! The remedy "actions" however, to implement these changes and solutions are way above me.... Can you suggest what "exact steps" I need to do to implement these patches and new changes, so I can address these items??

Thanks,

Mike Doak
760/729~8111(in Carlsbad, CA)

Comment 36

[Kent James (:rkent)]

Mike,

The custom action patch is now in trunk, which means if you use the TB3 nightlies, or upcoming beta1, this feature is possible.

Writing a custom action is another matter. Right now, the only available documentation, other than the code, is the sample extension on this bug. I hope to write some documentation, but that will not be any time soon.

What are you trying to accomplish with a custom action? I also hope to publish an extension soon that contains a variety of requested custom actions, and perhaps I could include your needs there.

It might be better to take this discussion to another location, such as m.d.a.thunderbird

Comment 37

[Egor]

Very useful feature, but why isAsync works only with manual filters?
If action takes time (for example, if you need to proceed incoming message body by IMAP), you have to choose between thunderbird's freezes on synchronous method and risk of not have time to process message if the next user action is delete or move message and you use asynchronous method to read it. Also there is functions in Thunderbird that has no synchronous implementation, so, you are forced to choose the second variant.

Comment 38

[Kent James (:rkent)]

This is not really the place for this kind of discussion, for a bug landed that almost 4 years ago, but whatever I accidentally noticed your comment so I will respond. Can't guarantee I will see any further comments though.

Async is a critical need in incoming filters, but this bug was trying to add the custom action with as little change to the core filter functions as possible. (Anything you touch in Mozilla code seems to lead to a chain, sometimes unending, of regressions, so major changes to tricky code are very difficult). The existing manual filter implementation had a rudimentary async capability, while incoming did not. That's why is was only supported in manual filters.

Comment 39

[Egor]

(In reply to Kent James (:rkent) from comment #38)

Thank you for reply! I've added new bug #753682 that describes this problem.