392848 - message filter condition order incorrect within the list after editing (rule added to the end (bottom) of the rules/conditions list within the filter)

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Terry Mester]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: ThunderBird Version 2.0.0.6 (20070728)

There is a new small problem with the Message Filter function on new ThunderBird Version 2.0.0.6 (20070728).  When you add a new Line within the List of CONDITIONS (the top box -- not the bottom Action Box), after you click OK and save the Filter, the new Line is placed at the BOTTOM of the List.  It of course is supposed to remain where it was inserted within the List.  This problem doesn't occur with the bottom Action Box List.

Reproducible: Always

Steps to Reproduce:
1.  Edit a Message Filter, and add a new Line within (not at the bottom of) the List of Conditions.
2.  Click OK to save the Message Filter and exit.
3.  Again Click to Edit the Filter.
Actual Results:  
The new Line will have been saved at the bottom of the List.

Expected Results:  
The new Line should remain exactly where it was inserted within the Conditions List.

Comment 1

[Magnus Melin [:mkmelin]]

Confirmed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007091204 Thunderbird/3.0a1pre ID:2007091204

Updating summary.

Comment 2

[Terry Mester]

I now have Mozilla Thunderbird Version 2.0.0.9 (20071031).  I've tested it out, and this problem has not been fixed.

Comment 3

[Terry Mester]

I now have Thunderbird version 2.0.0.12 (20080213).  This problem has not been solved.

Comment 4

[Magnus Melin [:mkmelin]]

Yes, that's why this bug is still open. (No need to comment for every minor update, this isn't something that would get fixed before thunderbird3.)

Comment 5

[ISHIKAWA, Chiaki]

The bug is still there.

Version:
Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.9.2.9) Gecko/20100825 Thunderbird/3.1.3

Steps to re-produce:
I insert a new condition (line) before the last line by
first hitting [+] button on the current second to last line and
insert a new line below (before the last line), and edit the new line as a condition for size.

Save the rule.

Re-open the rule for editing.

Voila:
The second to last line I edited is now located at the LAST line.
The order is incorrect.

Actually, the insertion position doesn't seem to matter.

Funny, there was a time when I thought the operation worked somehow.
But once the bug was identified, the buggy behavior has been consistent. Hmm...

Comment 6

[Terry Mester]

Agreed.  This problem has NOT been fixed.

Comment 7

[Wayne Mery (:wsmwk)]

doesn't affect filter results or functionality, so => trivial

Comment 8

[ISHIKAWA, Chiaki]

(In reply to comment #7)
> doesn't affect filter results or functionality, so => trivial

Please, Wayne, not so fast.

I thought that the filter rule evaluation is "short-circuited" one.
Bug 154867
Bug 295088

Thus, if one puts a more limiting condition (picking fewer messages) in AND cases before other filters, or
if one puts a more likely condition (returning match result often) in OR cases before other filters, then the CPU and I/O workload filter evaluation should be less. 

Imagine a case that the last condition is to look for a string match in a message body while the newly inserted second to last condition is to check for, say, a date earlier than certain date or something in AND case.

We don't have to scan the string in the message body if the second to last
condition fails since the filter as a whole failed without such scanning.

So the order DOES matter in terms of performance IMHO.

Comment 9

[:aceman]

I have not noticed this so far. Ishikawa, can you describe any example filter? Exact steps to reproduce. Some sample conditions and actions.

Comment 10

[ISHIKAWA, Chiaki]

It could be that the filter may need to have a long list of conditions.

I just checked. The problem is still there.
I have a filter named mail-order which collects all the periodic sales mails from mail order houses. This is longish. Some 30 conditions :-(

0. move the cursor to the last filter condition.

1. Move the cursor above to the second to last filter condition.

2. Hit [+] next to he second to last filter condition.

   Observe that there is now an empty filter condition between
   the last and previously second to last filter condition.

   This empty filter position shall become the second to last condition.

3. Fill in a filter condition without changing the predicate.

   In my case, the previous second to last condition was the "From" 
   contains" (or something like this in English: I am using Japanese locale.) 
   so I typed in [e-mail]

   See the new second to last filter condition, 
   "From contains [e-mail]" 
   is completed now.

4. Hit OK near the lower right to complete the filter editing.

   The newly created filter condition should be the second to last condition.
   Right?

5.   Re-open the filter, and I found that the "From contains [e-mail]"
     condition is the last one.

     It should be the second to the last condition.

I checked the operation by opening a blank condition somewhere in the middle of the condition list. The result is the same. The new condition was inserted AT THE END after I hit OK. Bad.

Oh. It was TB 7.0.1 (I am using a note PC only used for a business trip).

I downloaded TB 8.0.
I re-checked. The result is the same.
The above behavior didn't change.
The bug is still there in V8.

I have no idea why some don't see it ???

Maybe, Terry Mester in Comment 6 can confirm that the bug is still there.

Comment 11

[:aceman]

Don't worry, nobody in the comments said he can't see it. Only me, because I didn't know the STR.
Thanks for them. I can see it now. It happens also on "short filters" (few conditions) in TB11, Win XP.
I'll try to look into the source, what is going on.

Comment 12

[:aceman]

Also I have noticed this only happens when editing an existing filter. When creating a new one and inserting conditions into the just created list, it is saved correctly.

Comment 13

[:aceman]

Let's see what is the reason for this.

Comment 14

[:aceman]

I think it is this code:
http://mxr.mozilla.org/comm-central/source/mailnews/base/search/content/searchTermOverlay.js#512
509                 // need to create a new searchTerm, and somehow save it to that
510                 searchTerm = termOwner.createTerm();
511                 gSearchTerms[i].obj.saveTo(searchTerm);
512                 termOwner.appendTerm(searchTerm);

It seems to always add new terms to the end of the list. Is it better to create a new insertTerm method in C++ or somehow simulate it with the existing methods of http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/nsIMsgFilter.idl ?

Comment 15

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #14)
> I think it is this code:
> http://mxr.mozilla.org/comm-central/source/mailnews/base/search/content/
> searchTermOverlay.js#512
> 509                 // need to create a new searchTerm, and somehow save it
> to that
> 510                 searchTerm = termOwner.createTerm();
> 511                 gSearchTerms[i].obj.saveTo(searchTerm);
> 512                 termOwner.appendTerm(searchTerm);
> 
> It seems to always add new terms to the end of the list. Is it better to
> create a new insertTerm method in C++ or somehow simulate it with the
> existing methods of
> http://mxr.mozilla.org/comm-central/source/mailnews/base/search/public/
> nsIMsgFilter.idl ?

It is nice to learn that someone is looking over pending bug report.
Should I take a look at the code to create a patch (in c++) or
creating an insertion method as in .idl?
(Or will someone take it up?)

Comment 16

[:aceman]

Yes, it would be nice if you could create the C++ part, however I'd like to first hear opinion from rkent if that is acceptable here.

I'd try to look at the Javascript part but it seems non-trivial to actually determine where to insert the new Term. There is a list of all active terms and also a list of all deleted terms and those must be somehow both applied onto the termOwner array.

Comment 17

[u446314]

Just to be clear, what this bug has been calling a "filter condition", we call instead a "search term" since these things are used both in traditional folder search as well as in filters.

I don't see any reason to go to C++ here. The list of search terms is an nsISupportsArray, and you can get that list, change it, and save it back all in javascript AFAICT. Chiaki, feel free to submit a patch. I believe aceman is correct that this is the spot to change - but you never really know until you try it.

Comment 18

[:aceman]

OK, I'll try it.

Comment 19

[:aceman]

Attachment: patch 1

This seems to work for me.

Comment 20

[:aceman]

ISHIKAWA chiaki, it seems the patch does everything needed in Javascript. No need to do anything in C++ so far. Thanks for your offer.

Comment 21

[Kent James (:rkent)]

Comment on attachment 646905 [details] [diff] [review]
patch 1

># HG changeset patch
># Parent a3b46de2b0865c1a39360676f9cf1125283957fc
># User aceman <[e-mail]>
>Bug 392848 - new filter rule line should really be inserted into the position where it is shown in the editor, not at the end of the list. r=rkent
>
>diff --git a/mailnews/base/search/content/searchTermOverlay.js b/mailnews/base/search/content/searchTermOverlay.js
>+/**
>+ * Save the search terms from the UI back to the actual search terms
>+ * @param searchTerms  nsISupportsArray of terms
>+ * @param termOwner    object which can contain and create the terms
>+ *                     (will be unnecessary if we just make terms creatable
>+ *                     via XPCOM)
>+ */

Personally I tend to avoid using /* style comments in the main code,
as I frequently comment out sections of code, and this makes it hard
to do. Have others encouraged you to do this?

> function saveSearchTerms(searchTerms, termOwner)

Looking at this function, it seems to me what we really want to do is
just reform searchTerms from the values saved in gSearchTerms[i].obj.searchTerm,
making sure that we create or update the gSearchTerms[i].obj.searchTerm first.
Could you start this routine with searchTerms.Clear() and then just append
all of the terms from gSearchTerms[i].obj in order directly to searchTerms?
It seems to me like that would be simpler. (Though I know from previous
experience that there a lots of gotchas in this code that are hard to
detect.

I'm going to change the review to feedback to make it clear that I am not an official peer.

Comment 22

[Magnus Melin [:mkmelin]]

(In reply to Kent James (:rkent) from comment #21)
> >+/**
> >+ * Save the search terms from the UI back to the actual search terms
> >+ * @param searchTerms  nsISupportsArray of terms
> >+ * @param termOwner    object which can contain and create the terms
> >+ *                     (will be unnecessary if we just make terms creatable
> >+ *                     via XPCOM)
> >+ */
> 
> Personally I tend to avoid using /* style comments in the main code,

Doxygen/javadoc style comments like this are indeed encouraged.

Comment 23

[:aceman]

(In reply to Kent James (:rkent) from comment #21)
> Could you start this routine with searchTerms.Clear() and then just append
> all of the terms from gSearchTerms[i].obj in order directly to searchTerms?
> It seems to me like that would be simpler. (Though I know from previous
> experience that there a lots of gotchas in this code that are hard to
> detect.

What happens if !gSearchTerms[i].initialized ? In your proposal I still have to save this term. Should I initialize it first?

Comment 24

[Kent James (:rkent)]

When I did the review, I convinced myself that the initialize function just gets the UI prepared to use. But the searchTerm itself was stored in the list of terms here:

gSearchTerms.splice(index, 0, {obj:searchTermObj, scope:scope, searchTerm:searchTerm, initialized:false});

When you are re-creating the search term array, then any search term that is not initialized also in theory had no UI changes, so it can just be copied directly.

But as a caution, I always found the search js confusing because of the so many similarly named objects. It is possible there is something I am missing here.

Comment 25

[:aceman]

Well, your proposal results in cleaner code and also seems to allow to remove the maintaining and need of the gSearchRemovedTerms array. However, rebuilding the whole term list from scratch in your proposal is logically slower.
How much slower? In my tests on a underclocked 800Mhz machine the difference on a 100 rule filter was 17 seconds vs. 1 second (on vanilla trunk or using my original patch). On a 3Ghz machine (Phenom II) it took 6 seconds to save the filter.
I would not make much of the slow machine, but if it is so slow on a normal 3ghz machine then I'd have a problem with that. We already have enough reports on slow handling of filters (when there are many of them, or have many rules). What do you think?

Comment 26

[:aceman]

Attachment: patch v2 (alternative)

So this is the version that simply rebuilds the list of term but is slow. Do you see any way to optimize it?

Comment 27

[Kent James (:rkent)]

Looking back to comment 25, it is clear that aceman is on top of issues of performance in this part of the code, and I am not. So though I had some questions on the approach taken in attachment 646905 [details] [diff] [review] (hence the feedback-), I don't really have any objections to that approach. So if the more complex approach there is needed for performance, that is fine with me.

So for now I'm just going to remove the feedback request for attachment 656970 [details] [diff] [review] since I think aceman would really prefer the other approach.

Comment 28

[:aceman]

Comment on attachment 646905 [details] [diff] [review]
patch 1

Yes, I'd prefer this version. It is more complex but that complexity was there before. I just don't know how to simplify it and keep the speed.

Comment 29

[[e-mail]]

Comment on attachment 646905 [details] [diff] [review]
patch 1

I don't suppose we can add insertTermAt and removeTerm functions to the various search term owners?

Comment 30

[:aceman]

Do you mean into the C backend and expose it in the idl? I asked that some comments in the past.

Comment 31

[[e-mail]]

(In reply to Kent James from comment #17)
> I don't see any reason to go to C++ here. The list of search terms is an
> nsISupportsArray, and you can get that list, change it, and save it back all
> in javascript AFAICT.
Except that the one function we really want, insertElementAt, is not available in JavaScript, so we would have to either a) add a helper method to nsIMsgFilter and nsIMsgSearchSession to insert a term or b) switch to nsIMutableArray. (Note that I'd rather hide the array completely because it's an accident waiting to happen, but that would be a big undertaking.)

Comment 32

[Kent James (:rkent)]

(In reply to [e-mail] from comment #31)
> (In reply to Kent James from comment #17)
> > I don't see any reason to go to C++ here. The list of search terms is an
> > nsISupportsArray, and you can get that list, change it, and save it back all
> > in javascript AFAICT.
> Except that the one function we really want, insertElementAt, is not
> available in JavaScript, so we would have to either a) add a helper method
> to nsIMsgFilter and nsIMsgSearchSession to insert a term or b) switch to
> nsIMutableArray. (Note that I'd rather hide the array completely because
> it's an accident waiting to happen, but that would be a big undertaking.)

I had not noticed this issue, so I can see that if there is a good reason than it is OK to go this way.

Comment 33

[:aceman]

Neil, is the JS reimplementation (workaround) or insertElementAt not enough?

Comment 34

[:aceman]

So if it is OK to create a insertElementAt equivalent in nsIMsgFilter.idl (e.g. insertTermAt), I'll try that.

I could also add removeTermAt so that the searchTerms array does not need to be accessed directly. Neil, is that what you propose in comment 31 (except changing the array to nsIMutableArray)?

Comment 35

[[e-mail]]

(In reply to aceman from comment #33)
> Neil, is the JS reimplementation (workaround) or insertElementAt not enough?
Did you mean "or" or "of"? I thought the concern was that the workaround was too slow, in particular inserting a search term involves manually shuffling all the other terms up a place.

(In reply to aceman from comment #34)
> I could also add removeTermAt so that the searchTerms array does not need to
> be accessed directly. Neil, is that what you propose in comment 31 (except
> changing the array to nsIMutableArray)?
No, when I said "hide the array completely" that would mean removing or making searchTerms unscriptable and adding a scriptable way to retrieve a copy of the list of search terms from a filter. As I said, it's a big undertaking.

Comment 36

[:aceman]

(In reply to [e-mail] from comment #35)
> (In reply to aceman from comment #33)
> > Neil, is the JS reimplementation (workaround) or insertElementAt not enough?
> Did you mean "or" or "of"? I thought the concern was that the workaround was
> too slow, in particular inserting a search term involves manually shuffling
> all the other terms up a place.
Yes, I mean "of". No, the JS workaround is not slow. The alternative patch v2 is slow, that rebuilds the whole searchTerms array, but eliminating the need for insertElementAt.

> (In reply to aceman from comment #34)
> > I could also add removeTermAt so that the searchTerms array does not need to
> > be accessed directly. Neil, is that what you propose in comment 31 (except
> > changing the array to nsIMutableArray)?
> No, when I said "hide the array completely" that would mean removing or
> making searchTerms unscriptable and adding a scriptable way to retrieve a
> copy of the list of search terms from a filter. As I said, it's a big
> undertaking.
Yes, and that is why I do not offer to make this undertaking. I just offer to create insertTermAt removeTermAt in the c++/idl so that this particular JS function saveSearchTerms does not need to access the array directly and the searchTerms argument can be removed. So I do not hide the array completely from the idl, just remove one usage of it.
In another bug I could go through all users of the .searchTerm property and see if any of those that modify the array directly can be converted to these 2 new functions.

Comment 37

[[e-mail]]

(In reply to aceman from comment #36)
> (In reply to [e-mail] from comment #35)
> > (In reply to aceman from comment #33)
> > > Neil, is the JS reimplementation (workaround) or insertElementAt not enough?
> > Did you mean "or" or "of"? I thought the concern was that the workaround was
> > too slow, in particular inserting a search term involves manually shuffling
> > all the other terms up a place.
> Yes, I mean "of". No, the JS workaround is not slow. The alternative patch
> v2 is slow, that rebuilds the whole searchTerms array, but eliminating the
> need for insertElementAt.
My apologies, I must have misunderstood the previous comments, but I am surprised, as inserting an element at the start still seems to require you to rewrite the entire array.
Perhaps a compromise solution would be acceptable? In this case, we wouldn't clear the existing array, we would just use SetElementAt to copy the search terms into it. (You still need to append enough terms to make the array long enough, but you can continue to do that as you find them.)
Of course to help choose a solution it would be good to know:
1. How long it takes to save a 100-condition filter with no new conditions
2. How long it takes to save with a new condition at the end
3. How long it takes to save with a new condition at the start

Comment 38

[:aceman]

It seems shuffling elements in the array is fast even when the whole array needs to be shifted by 1. But serializing all terms into the array from the listitems and even those that are not initialized is slow (for 100 terms). For some benchmarks, see comment 25. Those numbers are with the patch v2 and the same time is needed for all of your cases 1.-3.

For "patch 1" (the one with insertElementAt even coded in JS) case all of cases 1.-3. are instant. However saving a filter that has 100 new terms should be as slow as patch v2.

I am not sure what we are discussing here. Do you need the numbers to decide if it is worth to add a new method into the C++ backend?

Comment 39

[[e-mail]]

(In reply to aceman from comment #38)
> It seems shuffling elements in the array is fast even when the whole array
> needs to be shifted by 1. But serializing all terms into the array from the
> listitems and even those that are not initialized is slow (for 100 terms).
> For some benchmarks, see comment 25. Those numbers are with the patch v2 and
> the same time is needed for all of your cases 1.-3.
> 
> For "patch 1" (the one with insertElementAt even coded in JS) case all of
> cases 1.-3. are instant. However saving a filter that has 100 new terms
> should be as slow as patch v2.
I am surprised at the speed difference between the two approaches. I know rkent would have preferred not to use the original patch, which is why I suggested the compromise approach, but I would be happy to go with your original patch if it's our only fast non-C++ option.

Comment 40

[:aceman]

OK. But it introduces a new place that is messing with the array directly. And you wanted to eliminate that.

I would have preferred patch v2 too as it is semantically much easier to understand and it also does not manipulate the array. Only if it wasn't so slow. I can look at it again whether the speed is really that bad.

Comment 41

[:aceman]

What exactly is the compromise approach? Your comment 29 seems to be the same as my comment 34.

Comment 42

[:aceman]

So the new benchmarks for 100 terms (on AMD Phenom II 3Ghz, Linux 32bit) according to Neil's ideas:
Patch v2:
1. 5s
2. 5s
3. 5s

Patch v1:
1. instant
2. instant
3. instant

Comment 43

[[e-mail]]

(In reply to aceman from comment #41)
> What exactly is the compromise approach? Your comment 29 seems to be the same as my comment 34.
I was thinking that the slowdown was because you were emptying and rebuilding the array, and my suggestion was simply to overwrite the array entries in place as you go. However having looked again at patch v2 the other possibility is that you're calling save on all the terms even if they haven't changed, and this is in fact what's slowing it down. If you weren't on Linux then I would have suggested trying out the profiler to see why v2 is so slow.

Comment 44

[:aceman]

(In reply to [e-mail] from comment #43)
> and my suggestion was simply to overwrite the array
> entries in place as you go.
I can try this. We'll see if createTerm() is the slow part or saveTo(). Or maybe something can be optimized there.

Comment 45

[:aceman]

Attachment: patch v3 (alternative 2)

Theoretically, this should be the implementation of comment 43. But it does not work when I remove a term in the middle of the list. I get this error:
Error: ** Error saving element 100: [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: chrome://messenger/content/searchTermOverlay.js :: searchTermContainer.prototype.save :: line 114"  data: no]
Source file: chrome://messenger/content/searchTermOverlay.js
Line: 488

So it looks like the searchTerm array member (that got deleted) does not like to be rewritten to another term.

Comment 46

[[e-mail]]

Attachment: Alternative

This is what I meant when I said to set the elements.

Comment 47

[:aceman]

Comment on attachment 679656 [details] [diff] [review]
Alternative

OK, apart from the patch not compiling (surplus bracket), it seems to work. It is a bit hard to follow because in the case of a new term, you still create it, append at the end, but also replace the element at i. Then further terms actually shift the whole array by 1 and the last term overwrites the newly created term. It is a hidden form of my InsertElementAt implementation :) Mine has the fault, that it rewrites the array for each new element. Your's only runs over the array once but does re-set all the elements unconditionally. The speed is similar to my patch 1, so it is fine.

I would be OK with this, but it introduces a new point of touching the array directly. And you theoretically wanted to eliminate those.

Comment 48

[[e-mail]]

(In reply to aceman from comment #47)
> OK, apart from the patch not compiling (surplus bracket)
Ah yes, I see it now. I think it must have got left behind when I was removing the dump(.

> The speed is similar to my patch 1, so it is fine.
> 
> I would be OK with this, but it introduces a new point of touching the array
> directly. And you theoretically wanted to eliminate those.
But rkent wanted to avoid adding C++ so...

Comment 49

[[e-mail]]

Attachment: Possible patch

Comment 50

[Kent James (:rkent)]

"But rkent wanted to avoid adding C++" if there is no need and javascript will do the job. Otherwise I have no problems with adding C++ methods or fixes.

Comment 51

[Kent James (:rkent)]

Comment on attachment 680284 [details] [diff] [review]
Possible patch

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

This works great, but it took me hours to figure out why. I few comments would have helped immensely. r+=me with the requested comments.

::: mailnews/base/search/content/searchTermOverlay.js
@@ +485,4 @@
>              if (searchTerm)
>                  gSearchTerms[i].obj.save();
> +            else if (!gSearchTerms[i].initialized)
> +              searchTerm = gSearchTerms[i].searchTerm;

Add a comment like "this is an offscreen term that has not been changed by the UI" or use the original comment.

@@ +489,5 @@
>              else {
>                  // need to create a new searchTerm, and somehow save it to that
>                  searchTerm = termOwner.createTerm();
>                  gSearchTerms[i].obj.saveTo(searchTerm);
>                  termOwner.appendTerm(searchTerm);

Add a comment so that it is clear that this is called only for the side effects, and the actual term will be overwritten in the array.

Comment 52

[[e-mail]]

(In reply to Kent James from comment #50)
> "But rkent wanted to avoid adding C++" if there is no need and javascript will do the job. Otherwise I have no problems with adding C++ methods or fixes.
Well, it depends on what you mean by "need"... does JavaScript "need" to be able to access the underlying nsISupportsArray?

Comment 53

[[e-mail]]

Attachment: Addressed review comments

(Literally, in this case!)

Comment 54

[Kent James (:rkent)]

Comment on attachment 680783 [details] [diff] [review]
Addressed review comments

Yes that's fine.

Comment 55

[[e-mail]]

Pushed comm-central changeset 4959d2b96fbb.

Comment 56

[Mark Banner (:standard8)]

Comment on attachment 678110 [details] [diff] [review]
patch v3 (alternative 2)

I'm guessing this is an old request, hence I'm cancelling it. If it is really necessary, please re-open or consider filing a new bug. Thanks.

Comment 57

[:aceman]

Yes, all the attachments (except the landed one) are obsolete.