747284 - Actions in msgFilterRules.dat is better sorted by INDEX value of action, in order to avoid user's confusion

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[WADA:World Anti-bad-Duping Agency]

Filter actions has INDEX value, and order of action execution is determined by the INDEX value instead of "order of actions written by user".   
> http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgFilter.cpp?force=1#317
> 317 // The order of actions returned by this method:
> 318 //   index    action(s)
> 319 //  -------   ---------
> 320 //     0      FetchBodyFromPop3Server
> 321 //    1..n    all other 'normal' actions, in their original order
> 322 //  n+1..m    CopyToFolder
> 323 //    m+1     MoveToFolder or Delete
> 324 //    m+2     StopExecution
So, actions in msgFilterRules.dat is better sorted by INDEX value of action, in order to avoid user's confusion.

Because UI shows actions in order defined in msgFilterRules.dat, user's confusion is reduced only by "sorted actions in msgFilterRules.dat".
However, delimiter at UI between MoveToFolder/Delete/StopExecution and others is better, because all of MoveToFolder, Delete, StopExecution means or implies "stop further application of filter rules for the hit mail".

Comment 1

[:aceman]

Yes, this would be very nice. I think there exists a function to sort the actions before the filter is executed. I'll try to look if it can be reused before the filter definition is saved (from filter editor).

Comment 2

[:aceman]

Attachment: proof of concept patch

This seems to work. It sorts the action when the filters are saved to the file.

I'd like to sort them sooner (maybe already when the filter is created/edited) but that is in Javascript parts and I don't know how to call GetSortedActionList() from there.

Comment 3

[:aceman]

Attachment: proof of concept patch v2

Comment 4

[David :Bienvenu]

Comment on attachment 617088 [details] [diff] [review]
proof of concept patch v2

seems reasonable to me...

Comment 5

[:aceman]

Rkent, any idea how I could apply the sorting sooner? Like immediatelly after closing the filter editor dialog?

Comment 6

[:aceman]

rkent: ping?

Comment 7

[:aceman]

Hmm, but getSortedActionList is exposed in http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsIMsgFilter.idl#99... I'll check that out.

Comment 8

[:aceman]

Attachment: patch v3

So it works now:)

The patch contains both options:
1. the action list is sorted immediately when closing the filter editor. When the user reopens the filter he finds the actions sorted.
2. in all filters actions are sorted when saving the whole filter list. So this covers even filters never touched by the user (to trigger the immediate sort).

bwinton and rkent, you can now decide which option to leave in. Or both as some filters will never be opened in the editor so may not get sorted. And just opening them and clicking Cancel will also not sort them.

Comment 9

[Kent James (:rkent)]

I have mixed feelings about this. One could view the ordering of the filter actions as an implementation detail that now is leaking into the user interface. I would expect that we will get reports complaining that Thunderbird is changing the filter action order.

If we are going to surprise the user by reordering the filter actions, I don't like doing it only after saving. Would it be possible to do that as soon as the filter action is entered? Or would the performance costs of that be too high?

Blake, what do you think?

(sorry I did not give you feedback earlier)

Comment 10

[Blake Winton (:bwinton) (:☕️)]

So, we're running the filters in a different order than the one we show the user?
I guess I disagree with the comment at the top of the linked function: "All the rules' actions form a unit, with no real order imposed."  I think the user has imposed an order by ordering the actions, and we should use that order.

If we're not doing that, then our choice is among two surprising behaviours.  Either we re-order the filter actions on the user (Surprise!), or we run the filters in a different order than the one the user sees (Surprise!).

If we need to keep the ordering defined by that function, then I think our best option is to re-order the actions after every edit (if possible, and in an animated way, if possible), so that the user will always see how their actions will be run.

Thanks,
Blake.

Comment 11

[Kent James (:rkent)]

Just to be clear, the reordering is only done of actions that make no sense unless reordered. Like you cannot filter until you have downloaded, so downloading always needs to be done first. Other actions only apply when the message is in its current folder, so it makes no sense to move the message before you have done other actions.

Comment 12

[:aceman]

rkent, NOT ordering the actions today is already confusing the users because the actions are executed differently, see e.g. bug 419368. If this one gets implemented we could close bug 419368 as INVALID as it is working as designed. Now the users would see why and would see the actual order of the actions.
So yes, it is an implementation detail but it is affecting users (that think the order as they set it is significant, while it really isn't). So we should provide the info.

Reordering and animating while editing would be something but I am not sure how to implement it sanely. Is would reorder an action row as soon as the uses changes the action type from the popup menu. That could be messy :)

Yes, there is a PRO for not doing any reordering. If the actions stay untouched in the .dat file, and if in future we implement the execution of actions to be as the user specified then yes, users having filters tuned NOW would actually get them working properly.

bwinton, honoring the user's order NOW would probably need heavy changes in the filter backend.

Comment 13

[:aceman]

OK, another proposal:
Instead of permanently reordering the actions thus killing the user's tuning, how about just prompting the user on OK that the actions will be executed differently (and showing him a text list of the final order) ? I can optimize it to only show the warning when the order really would change (i.e. user's list != ordered list).

Comment 14

[:aceman]

Or even less intrusive:
while editing the filter if the dialog detects that the current list of actions will be reordered when executing, it pops up a status bar message that this will happen allowing the user to click a link in the message that would <menupopup> (or something) the sorted list. If he doesn't care, he can ignore that and proceed as today without any new dialogs.

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

The Filter Rules dialog doesn't have a status bar right now, but some unused space to the left of the bottom buttons, so this is where a short text could be displayed in case the actions need reordering. If the dialog is not wide enough, the text would be cropped, but the user could resize the dialog to see the full text. Downside: The available space depends on locale and dialog width.

Alternatively, a notification bar could be introduced that popped up at the bottom of the actions area. In that case, the bar could also contain an Apply button (which would not be possible with a standard status bar).

Third option: Temporarily alter the line above the actions area (reading "Perform these actions:"), e.g. by adding something in parentheses or adding a new label to the right of the line, possibly right-aligned. I wouldn't add a button there, though (think line height).

In any case, modal pop-up dialogs should be avoided because those disrupt the user's work-flow. The user should have an easy "I know what I'm doing / don't care" way out.

Comment 16

[:aceman]

There would be added a hidden statusbar (vbox/anything) on the bottom of the dialog that gets expanded only when this warning is to be shown.

Comment 17

[WADA:World Anti-bad-Duping Agency]

A simple way to notify user of "action is reordered" in order to avoid unwanted bug like "Action order was changed by Tb!!!!!".
(i) If action order is changed by internal reordering, add "reordered=yes" line in entry for the filter rule of msgFilterRules.dat.
(ii) If "reordered=yes", show "actions is reordered" message at UI upon next filter rule editing, and when filter save, change to "reordered="no" or remove this line.

If older Tb fails when unkown msgFilterRules.dat line exists, above is impossible.
If older Tbs are torelant with 'enabled="yes",reorderd="yes"' like line(parameter is added), this can be used.

(In reply to :aceman from comment #13)
> OK, another proposal:
> Instead of permanently reordering the actions thus killing the user's tuning, (snip)

"Order of actions in same INDEX category" is preserved by internal sorting, isn't it?
I thought sort is done by "INDEX value" which Tb currently assigns upon filter execution...

Or your "user's tuning" means intentional action order change in msgFilterRules.dat by power user(who knows logic of nsMsgFilter.cpp well) for ease of msgFilterRules.dat file editing?

Comment 18

[:aceman]

(In reply to WADA from comment #17)
> > Instead of permanently reordering the actions thus killing the user's tuning, (snip)
> 
> "Order of actions in same INDEX category" is preserved by internal sorting,
> isn't it?
> I thought sort is done by "INDEX value" which Tb currently assigns upon
> filter execution...
> 
> Or your "user's tuning" means intentional action order change in
> msgFilterRules.dat by power user(who knows logic of nsMsgFilter.cpp well)
> for ease of msgFilterRules.dat file editing?

I mean setting a specific order inside the standard filter editor UI.

Comment 19

[Kent James (:rkent)]

I like the idea of some sort of non-popup status bar notifying the user that the filters will be reordered, and then doing the actual reordering later. It seems like a reasonable compromise. I'll leave the exact plans for that status UI up to the UI folks.

Comment 20

[Kent James (:rkent)]

Comment on attachment 664651 [details] [diff] [review]
patch v3

So this patch needs rework per the earlier discussions to add some warnings. I did not thoroughly review it for that reason.

Comment 21

[:aceman]

(In reply to Kent James (:rkent) from comment #19)
> I like the idea of some sort of non-popup status bar notifying the user that
> the filters will be reordered, and then doing the actual reordering later.

The actual reordering will only be done temporarily before execution as it is done today. In no point in this new plan will the user set order be changed. He will only be informed the order will be executed differently.

Comment 22

[:aceman]

Attachment: proof of concept patch v4

So this is an experiment implementing the proposal with the appearing "statusline". Many things are hardcoded. It is intended to just see the feel of the solution and how non-invasive the code change can be.

Comment 23

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 666055 [details] [diff] [review]
proof of concept patch v4

Yeah, I don't mind this so much.  A couple of notes:

1) I think, if at all possible, the status bar should be inside the listbox, since that's what it relates to.  (Also, the list box options move up when the status bar is displayed, which isn't great.)

2) The dark grey is a little dark.

3) The list of actions in real order of execution should be less code-y, and more like what the user sees in the list.

4) Perhaps we could use a twisty to show/hide the list right in the content, under the list of actions, instead of in a popup?

But yeah, like I said, this seems to work better than I thought it might.  ;)

Thanks,
Blake.

Comment 24

[Kent James (:rkent)]

I'm having a hard time being enthusiastic about this change. It's a nag, telling me I've done something wrong, and so I want to fix it. But there is no easy way to fix it since there is no ability to move filters easily up and down in the list. You have to delete the action and recreate it in the correct place. There is also no real need to fix it, since the reordering that is done is in most cases the only reasonable interpretation of the filter action order. It was also no obvious to me that clicking on the link would show me the actual order of execution of the actions.

Here's what I would prefer. Get rid of the word "warning", maybe just "Action Execution order" as the label, and have it popup when it is different than the entered list as it does now. Somehow change the UI element to make it clearer that it represents a link to a popup (not sure how).

Comment 25

[Blake Winton (:bwinton) (:☕️)]

What do you think about a button on the new popup which would automatically fix it for you?

Comment 26

[Kent James (:rkent)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #25)
> What do you think about a button on the new popup which would automatically
> fix it for you?

That would be nice, but not really necessary, because it does not actually need "fixing".

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 666055 [details] [diff] [review]
proof of concept patch v4

I was on holiday so only came around to test this now, sorry about that.

In general I like the approach and agree with the comments already made, especially that the items listed on the popup need to be general language instead of internal names like "copymessage" (esp. with l10n in mind, but also concerning plain English).

However, I'd also like to see some tweaks on the UI. I'm f-'ing this mainly so that I can see how that would look like.

1) Having the whole label as a link draws too much attention to it and lacks the information what will happen if you click it. I suggest splitting the text, making it two labels, one plain and one clickable (or the second one being a button as you proposed). Example: "Note: Filter actions will be reordered at execution time. _See actual order_"

2) The popup items are selectable and clickable. They shouldn't be.

Comment 28

[:aceman]

Attachment: proof of concept patch v5

Thanks for the comments. This is a more polished version containing most of your wishes. I excluded only the ideas about showing something inside the action listbox. That would be quite hard and invasive (much other code would need to be adapted to not choke on the unexpected special element).

Comment 29

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 675984 [details] [diff] [review]
proof of concept patch v5

Getting much better.  There are still a couple of things I think we should change.

1) "Note: Filter actions will be reordered at execution time." seems a little technical.  How about "Note: Filter actions will be run in a different order."?

2) This is a clever, but odd, use of the popup menu.  I would be more comfortable with a regular button which showed a similar popup, with the text not disabled?

3) It would be totally awesome to show what the arguments of the actions were, i.e. "Copy Message to Account1", "Set Junk Status to Junk", so that we can tell which copy message action will run first.  (Sure, they'll occur in the same order as in the list, but we've just told the user that we're re-ordering the items, so they may not know that.)

Thanks,
Blake.

Comment 30

[:aceman]

(In reply to Blake Winton (:bwinton) from comment #29)
> 2) This is a clever, but odd, use of the popup menu.  I would be more
> comfortable with a regular button which showed a similar popup, with the
> text not disabled?
What's wrong with it? It is the same as in account manager -> account actions :) The items are all disabled to not be selectable. Is there any better way? I see it does not look good.

> 3) It would be totally awesome to show what the arguments of the actions
> were, i.e. "Copy Message to Account1", "Set Junk Status to Junk", so that we
> can tell which copy message action will run first.  (Sure, they'll occur in
> the same order as in the list, but we've just told the user that we're
> re-ordering the items, so they may not know that.)
Yeah this would be nice, but totally non-trivial.

Comment 31

[Blake Winton (:bwinton) (:☕️)]

(In reply to :aceman from comment #30)
> (In reply to Blake Winton (:bwinton) from comment #29)
> > 2) This is a clever, but odd, use of the popup menu.  I would be more
> > comfortable with a regular button which showed a similar popup, with the
> > text not disabled?
> What's wrong with it? It is the same as in account manager -> account
> actions :)

I think my problem is that the things we show aren't actions, and will never be selectable.

> The items are all disabled to not be selectable. Is there any
> better way? I see it does not look good.

There should be a better way.  Maybe Mike has an idea?

> > 3) It would be totally awesome to show what the arguments of the actions
> > were, i.e. "Copy Message to Account1", "Set Junk Status to Junk", so that we
> > can tell which copy message action will run first.  (Sure, they'll occur in
> > the same order as in the list, but we've just told the user that we're
> > re-ordering the items, so they may not know that.)
> Yeah this would be nice, but totally non-trivial.

Can you start in on it, and let me know how non-trivial it would be?  :)

Thanks,
Blake.

Comment 32

[:aceman]

(In reply to Blake Winton (:bwinton) from comment #31)
> > > 3) It would be totally awesome to show what the arguments of the actions
> > > were, i.e. "Copy Message to Account1", "Set Junk Status to Junk", so that we
> > > can tell which copy message action will run first.  (Sure, they'll occur in
> > > the same order as in the list, but we've just told the user that we're
> > > re-ordering the items, so they may not know that.)
> > Yeah this would be nice, but totally non-trivial.
> 
> Can you start in on it, and let me know how non-trivial it would be?  :)

The main problem is that after sorting I loose the relation between the actions and the listitems that represent them. So while I'd like to show the exact strings in the value columns as the user sees them, I only get the backend technical values, that will be stored. And they look differently than what the user sees. Hmmm, it would be now possible to reinitialize the action listbox from the new sorted array to show the human readable strings and the new order. But I refrained from that as we must then prevent the user from messing with that order and somehow reset it back on some event.
I'll think about it.

Comment 33

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 675984 [details] [diff] [review]
proof of concept patch v5

First of all thanks for switching to general language instead of using internal terms.

(In reply to :aceman from comment #30)
> (In reply to Blake Winton (:bwinton) from comment #29)
> > 2) This is a clever, but odd, use of the popup menu.  I would be more
> > comfortable with a regular button which showed a similar popup, with the
> > text not disabled?
> What's wrong with it? It is the same as in account manager -> account
> actions :) The items are all disabled to not be selectable.

That's the difference. In the Account Manager (TB's at least; SM doesn't have a popup there), the popup entries are actions. Selectable, clickable. Sure you can disable them as you did, but then the popup is pointless because the user doesn't get what s/he expects: When you open a menupopup you expect to have at least one selectable, clickable item; otherwise the menupopup itself should be disabled.

> Is there any better way? I see it does not look good.

I think the way to go is a plain link (on a text node), just not the whole label as you had it before. That link should then open a modal window that simply lists what you currently show in the popup, possibly in a numbered way (1. 2. 3. etc.). The window should only have an OK button.

Comment 34

[:aceman]

Oh, open a duplicate filter editor dialog in a special mode that shows actions sorted and disables all widgets except OK? Looks quite heavy :)

Comment 35

[:aceman]

OR disable the current dialog, sort the actions and only allow to scroll through them (every widget (including the +/- buttons) except the listbox will be disabled). But then where to click to get back to the edit mode?

Comment 36

[Jens Hatlak (:InvisibleSmiley)]

I think you misunderstood me. KISS. Just a dialog like this:

---------------------------
| Actual Filter Execution |
---------------------------
| 1. aaaa                 |
| 2. bbbbbbb              |
|                 | OK |  |
---------------------------

Comment 37

[:aceman]

But then the problem of interpreting action arguments (not types) holds.

Comment 38

[Jens Hatlak (:InvisibleSmiley)]

(In reply to :aceman from comment #37)
> But then the problem of interpreting action arguments (not types) holds.

Not an issue to me, this is between bwinton and you. I would even propose to "collapse" identical actions (irrespective of their arguments), i.e. only list them once. So instead of

1. Copy [to "a"]
2. Copy [to "b"]
3. Tag [with "x"]

I would just output

1. Copy
2. Tag

(The above is just an example; didn't check whether this would be reordered at all.)

Comment 39

[:aceman]

This would be doable.
Bwinton, what do you think about a separate modal dialog with duplicate actions merged?

Comment 40

[Blake Winton (:bwinton) (:☕️)]

I'm not a giant fan of merging the actions, at least not with just the label "Copy".  I'm also worried about what we would do in a case like:

1. Copy [to "a"]
2. Tag [with "x"]
3. Copy [to "b"]

Do we still show two copy lines?  How do we differentiate them there?  If that's going to be a problem, then how about we just don't merge them at all?

Comment 41

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Blake Winton (:bwinton) from comment #40)
> I'm not a giant fan of merging the actions, at least not with just the label
> "Copy".

That was just to keep the example short (I'm lazy). Basically I was referring to what the latest patch outputs.

> how about we just don't merge them at all?

I was assuming that identical actions would always appear consecutively. I didn't really check whether that's the case so you can drop this part of my suggestion if need be. The core part was the "modal OK dialog showing a simple enumeration of strings" idea.

Comment 42

[Blake Winton (:bwinton) (:☕️)]

I love the "modal OK dialog showing a simple enumeration of strings" part!  :)

Comment 43

[:aceman]

You are right, actions of the same type do not appear grouped in the result of the ordering function. So e.g.
-tag X
-add star
-tag Y

does keep this order. The result IS NOT e.g. tag X, tag Y, add star.

Comment 44

[Kent James (:rkent)]

Comment on attachment 675984 [details] [diff] [review]
proof of concept patch v5

OK I think this is now looking really good. f+

The existing code does not get the label for a custom action, so custom actions show up as "undefined". This will need to be fixed in the final code.

Comment 45

[:aceman]

What I want to say is that I can only imagine to show proper action arguments if we extend nsIMsgRuleAction with some "id" member. The id is generated and stored in the action listitem. Then when the actionlist is reordered I can follow the ids to lookup the action arguments in the proper listitem and show the label from the 2nd column. Or does anybody see a better option?

Comment 46

[:aceman]

Attachment: proof of concept patch v6

What about this?

Comment 47

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 687460 [details] [diff] [review]
proof of concept patch v6

Two nits, but ui-r=me with them fixed.

A) I think we should remove the colon, so instead of:
      1. Copy Message to: NewName
      2. Move Message to: Spam on NewName
      3. Stop Filter Execution: Highest
   we would have:
      1. Copy Message to NewName
      2. Move Message to Spam on NewName
      3. Stop Filter Execution Highest

B) I set the second item to "Set Priority to: Highest", and then set it to "Stop Filter Execution", and got the results you saw above.  I don't think we should clear the second part (the "Highest" part) when hiding it, since the user might want to put it back the way it was, so you may need to see whether the element is hidden, and not add that text, or something.

Thanks,
Blake.

Comment 48

[:aceman]

I can't reproduce the problem. Can you tell the exact steps?

Comment 49

[Kent James (:rkent)]

Comment on attachment 687460 [details] [diff] [review]
proof of concept patch v6

(Sorry for the slow response, I've been fighting my own issues the last few weeks)

I tried this and it works fine, including with a custom filter action, so f+ from me.

Comment 50

[:aceman]

rkent, so you are OK with the addition of the ID attribute?

Comment 51

[Kent James (:rkent)]

(In reply to :aceman from comment #50)
> rkent, so you are OK with the addition of the ID attribute?

Yes that is fine.

Comment 52

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 687460 [details] [diff] [review]
proof of concept patch v6

Sweet! f=me on a purely front-end experience point of view.

Please take care that the final version is l10n-safe (I could imagine some languages might need to have action and argument in a different order or with something in between).

[Sorry for the delay]

Comment 53

[:aceman]

Attachment: patch v7

This should be the proper patch.
It depends on bug 821743 and bug 821253 being checked in.

Comment 54

[Kent James (:rkent)]

I spent several hours today going over this, but I'll need to finish this another day.

Fundamentally though I am bothered by having the js front end try to set and maintain this nsIMsgRuleAction.id attribute, which is really an attempt to allow you to use the C++ sortedActionList code, while being able to relate that back to the original order of the filter actions. I am considering if I should recommend a different approach, and if so what. Yes I know you asked for feedback already on this, but seeing the real code makes me uncomfortable.

One approach would be for the C++ code to maintain the value of that attribute.

A second approach would be to add a C++ method GetSortedActionIndices that returns the index of the actions rather than a list of actions.

A third approach would be to add a filter method GetActionIndex(action) that would lookup the location of the action in the filter.

I'll try to finish the review and give more concrete recommendations and commets in the next couple of days.

Comment 55

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 696128 [details] [diff] [review]
patch v7

Something's wrong with the argument determination. I tested with 1x move, 2x copy and three different target folders (say X Y Z), and the dialog displayed
1. Copy Message to Z
2. Copy Message to <nothing>
3. Move Message to Z

Comment 56

[Blake Winton (:bwinton) (:☕️)]

(In reply to :aceman from comment #48)
> I can't reproduce the problem. Can you tell the exact steps?

It sounds like Jens is having similar errors.


STR (from memory):

Create a rule.  Add three actions.
1. Copy Message to: NewName
2. Move Message to: Spam on NewName
3. Set Priority to: Highest

Check the message.

Set the third item to "Stop Filter Execution".

Check the message.  (For me, it said "Stop Filter Execution: Highest".)


Thanks,
Blake.

Comment 57

[:aceman]

Attachment: patch v8

I think I have found the problem Jens and Blake saw. I was collecting the arguments at the time the row (action) was created and not after it was changed (e.g. a new target folder selected). So I now collect the arguments when the reordered list is to be displayed.

I'll now wait on what rkent decides from comment 54. I manipulate the ID from JS here so that it can be used for some other purpose in other parts of code later. I understand that could be dangerous and non-intuitive for the coder (we'd have to document that the ID is only temporary). So I have no opposition to implementing the options from comment 54.

Comment 58

[Kent James (:rkent)]

Attachment: post-v6 patch to use filter.getActionIndex

Rather than add this id attribute to the msg action list, I think I would prefer if you allowed a filter to return the index of a specific action. This patch is a proof of concept, on top of your version 6 patch, that shows what this looks like. You need to save the created temporary filter so that you can recover the index from it.

As I said in comment 54, the thing that bothers me about adding the "id" to the msgFilterAction is that this is really a property of the filter and not the filter action. You are using it there as a temporary variable.

Comment 59

[:aceman]

Yes, I have no problem with this solution. Should I merge the patches?

Comment 60

[Kent James (:rkent)]

Comment on attachment 696128 [details] [diff] [review]
patch v7

Review of attachment 696128 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I made an error, my patch was on top of version 7, not of version 6.

That patch will require changes. Here though are some additional comments.

::: mailnews/base/search/content/FilterEditor.js
@@ +29,5 @@
>                              "fetchfrompopserver", "copymessage", "addtagtomessage",
>                              "ignoresubthread", "markasunread"];
>  
> +let gActionListOrdered = null;
> +let gFlatActionList = [];

You need to give a brief one-line description of the definition of each of these globals. I also don't understand in what way this action list is "flat". It's also confusing to call this an "actionList" when you have other items that are also called "actionList" which are arrays on XPCOM objects. Perhaps gActionStrings would be a better name?

@@ +493,5 @@
> + */
> +function checkActionsReorder()
> +{
> +  // Create a temporary disposable filter.
> +  let filter = gFilterList.createFilter("____________randomFilterName238472384728348_____________");

The name is never used, so just give it a blank name.

::: mailnews/base/search/content/searchWidgets.xml
@@ +279,5 @@
> +          ]]>
> +        </body>
> +      </method>
> +
> +

nit: extra blank line

More substantially, all this method does is to transfer to a global function checkActionsReorder. Why include it in the XBL at all if it is not using any of the XBL nodes? Just call it directly when you need it. If you need the setTimeout, include that in the core function.

@@ +435,5 @@
>              document.getAnonymousNodes(this.mRuleActionType)[0]
>                      .value = filterActionStr;
>              this.mRuleActionTargetInitialized = true;
>              this.clearInitialActionIndex();
> +            document.getAnonymousNodes(this)[0].checkActionsReorder();

nit: since the object has defined a shorthand for document.getAnonymousNodes(this)[0] you should use it, that is this.mRuleActionType (But note my earlier comment that you should just call checkActionsReorder directly)

@@ +563,5 @@
>              this.mListBox.ensureElementIsVisible(listItem);
>  
>              // make sure the first remove button is enabled
>              this.updateRemoveButton();
> +            document.getAnonymousNodes(this)[0].checkActionsReorder();

nit: since the object has defined a shorthand for document.getAnonymousNodes(this)[0] you should use it, that is this.mRuleActionType (But note my earlier comment that you should just call checkActionsReorder directly)

@@ +577,5 @@
>              if (listBox.getRowCount() > 1)
>                listBox.removeChild(this);
>              // can't use 'this' as it is destroyed now
>              listBox.getItemAtIndex(0).updateRemoveButton();
> +            document.getAnonymousNodes(listBox.getItemAtIndex(0))[0].checkActionsReorder();

(Note my earlier comment that you should just call checkActionsReorder directly)

Comment 61

[Kent James (:rkent)]

(In reply to :aceman from comment #59)
> Yes, I have no problem with this solution. Should I merge the patches?

My patch is not complete (that is I did not remove the .id implementation that you did) but otherwise you should merge them.

Comment 62

[:aceman]

Attachment: patch v9

Combined cleaned up patches.

Comment 63

[Kent James (:rkent)]

Comment on attachment 699391 [details] [diff] [review]
patch v9

Review of attachment 699391 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=me with the one issue of the annoying "beep" replaced by my proposed code or an alternative.

::: mailnews/base/search/content/FilterEditor.js
@@ +553,5 @@
> +    actionList += gFilterBundle.getFormattedString("filterActionItem",
> +      [(i + 1), action.label, action.argument]);
> +  }
> +
> +  Services.prompt.alert(window, gFilterBundle.getString("filterActionOrderTitle"),

I don't really like the beep that occurs when this dialog comes up, as the user requested it so you don't need to get his attention.

You could replace this with:

Services.prompt.confirmEx(window, gFilterBundle.getString("filterActionOrderTitle"),
                          actionList, Components.interfaces.nsIPromptService.BUTTON_TITLE_OK,
                          null, null, null, null, {value:false});

Comment 64

[:aceman]

Attachment: patch v10

Do we need a Seamonkey reviewer here? Maybe for the strings?

Comment 65

[Jens Hatlak (:InvisibleSmiley)]

(In reply to :aceman from comment #64)
> Do we need a Seamonkey reviewer here? Maybe for the strings?

Normally not, and certainly not for strings that you only add as copy of TB strings because the code/functionality is shared. :-)

That said, I cannot give you any feedback right now since the build system seems to be f'd up right now (builds fail for me on both Windows and Linux due to Python errors during configure).

Comment 66

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 702996 [details] [diff] [review]
patch v10

Looks awesome!  ui-r=me!  I'm going to go ahead and request review from rkent, and then hopefully we can land this fix!  :)

(Also, apologies for the delay in reviewing…)

Thanks,
Blake.

Comment 67

[:aceman]

Comment on attachment 702996 [details] [diff] [review]
patch v10

rkent already reviewed v9 of this. v10 addresses his nits. Only your ui-r was missing here:)

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/017966bfe67b