Last Comment Bug 419356 - Allow extensions to add custom filter actions
: Allow extensions to add custom filter actions
Status: RESOLVED FIXED
: calendar-integration, dev-doc-needed
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: unspecified
: All All
: P1 enhancement with 1 vote (vote)
: Thunderbird 3.0b1
Assigned To: Kent James (:rkent)
:
:
Mentors:
: 366094 (view as bug list)
Depends on:
Blocks: 272796 317548 372479 384787 398875 415550 66425 348275
  Show dependency treegraph
 
Reported: 2008-02-24 14:34 PST by Philipp Kewisch [:Fallen]
Modified: 2012-05-10 01:48 PDT (History)
22 users (show)
standard8: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Custom filters (66.61 KB, patch)
2008-09-30 01:53 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
Demo extension with four custom actions (4.25 KB, application/octet-stream)
2008-09-30 01:57 PDT, Kent James (:rkent)
no flags Details
Includes nsIMsgFilterCustomAction.idl (73.03 KB, patch)
2008-09-30 02:31 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
First revision (57.38 KB, patch)
2008-10-07 00:30 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
First revision of demo extension (4.32 KB, application/x-xpinstall)
2008-10-07 00:31 PDT, Kent James (:rkent)
no flags Details
Changed to CSS-driven custom bindings (rev 2) (54.25 KB, patch)
2008-10-12 23:45 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
Sample extension (rev2) (5.45 KB, application/octet-stream)
2008-10-12 23:49 PDT, Kent James (:rkent)
no flags Details
Fixed Neil's nits (51.69 KB, patch)
2008-10-13 12:37 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
Fixed more Neil issues (51.71 KB, patch)
2008-10-28 23:59 PDT, Kent James (:rkent)
no flags Details | Diff | Splinter Review
One more time (51.07 KB, patch)
2008-10-31 12:13 PDT, Kent James (:rkent)
neil: review+
mozilla: superreview+
Details | Diff | Splinter Review
Fixed David's issues (50.48 KB, patch)
2008-11-04 10:46 PST, Kent James (:rkent)
rkent: review+
rkent: superreview+
Details | Diff | Splinter Review

Description Philipp Kewisch [:Fallen] 2008-02-24 14:34:03 PST
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] 2008-02-24 14:44:30 PST
Fixing this bug could also open the gates for a number of other bugs.
Comment 2 Karsten Düsterloh 2008-06-10 15:09:40 PDT
JFTR: This is (in parts) a dupe of bug 11037.
Comment 3 David Ascher (:davida) 2008-06-17 13:53:00 PDT
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) 2008-06-17 14:54:56 PDT
This seems like real goodness, so accepting as blocking.  Would be good to have an owner.
Comment 5 Philipp Kewisch [:Fallen] 2008-06-18 07:23:20 PDT
David, I meant that fixing it would allow to fix a bunch of other bugs. See the Blocks list.
Comment 6 Mark Banner (:standard8) 2008-08-21 11:57:14 PDT
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) 2008-09-14 22:29:52 PDT
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 2008-09-14 23:16:37 PDT
Extensions might want to implement filter actions written in C++...
Comment 9 Joshua Cranmer [:jcranmer] 2008-09-15 04:36:43 PDT
(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) 2008-09-23 15:17:22 PDT
I've been working on this for the last few days, so I should assign it to myself.
Comment 11 Joshua Cranmer [:jcranmer] 2008-09-28 16:46:28 PDT
*** Bug 366094 has been marked as a duplicate of this bug. ***
Comment 12 Kent James (:rkent) 2008-09-30 01:53:19 PDT
Created attachment 341079 [details] [diff] [review]
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) 2008-09-30 01:57:01 PDT
Created attachment 341080 [details]
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) 2008-09-30 02:11:32 PDT
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) 2008-09-30 02:31:27 PDT
Created attachment 341085 [details] [diff] [review]
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] 2008-09-30 04:45:09 PDT
(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) 2008-10-07 00:30:27 PDT
Created attachment 342032 [details] [diff] [review]
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) 2008-10-07 00:31:25 PDT
Created attachment 342033 [details]
First revision of demo extension

This has four custom actions of varying complexity.
Comment 19 neil@parkwaycc.co.uk 2008-10-07 10:02:09 PDT
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) 2008-10-12 23:45:47 PDT
Created attachment 342847 [details] [diff] [review]
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) 2008-10-12 23:49:15 PDT
Created attachment 342848 [details]
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 neil@parkwaycc.co.uk 2008-10-13 09:52:00 PDT
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) 2008-10-13 12:37:49 PDT
Created attachment 342924 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2008-10-26 17:06:17 PDT
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) 2008-10-28 23:59:21 PDT
Created attachment 345245 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2008-10-31 09:03:23 PDT
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) 2008-10-31 12:13:17 PDT
Created attachment 345765 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2008-10-31 14:04:05 PDT
(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 2008-11-03 13:44:57 PST
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 2008-11-03 13:53:07 PST
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) 2008-11-04 10:46:11 PST
Created attachment 346286 [details] [diff] [review]
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 2008-11-04 11:47:08 PST
yes, you're right of course; it was hiding right there at the very top of the diff :-)
Comment 33 Mark Banner (:standard8) 2008-11-05 06:42:04 PST
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] 2008-11-14 11:01:49 PST
According to Kent, this is fixed. This wants documentation somewhere, so adding dev-doc-needed.
Comment 35 mikedoak 2008-11-14 12:30:43 PST
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) 2008-11-14 12:53:20 PST
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 2012-05-09 06:20:41 PDT
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) 2012-05-09 19:18:05 PDT
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 2012-05-10 01:48:06 PDT
(In reply to Kent James (:rkent) from comment #38)

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

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