Closed Bug 561762 Opened 14 years ago Closed 12 years ago

Add additional error information to "This filter cannot be saved"

Categories

(MailNews Core :: Filters, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 18.0

People

(Reporter: rkent, Assigned: aceman)

References

Details

Attachments

(1 file, 6 obsolete files)

From m.s.thunderbird:

> When I modify an existing filter TB3 invariably comes up with an error 
> message saying "This filter cannot be saved because some search terms 
> are invalid in the current context".
> 
> There is nothing wrong with my search terms, but after clicking OK a few 
> times the messages usually goes away and the revised filter works as 
> expected.
> 
> This behaviour is new in TB3. Is it some kind of bug?
> 

The message and check were added to help with legitimate cases where the search terms can become invalid, particularly with the new custom search terms feature. But unexpectedly the message is much more common now as a "catch all" for lots of different filter problems. Those other problems may be bugs, but at the moment we really don't understand why this sometimes happens.

So the filter is detecting some sort of problem. It is not clear what would happen if that check were missing - perhaps the filter would get stored incorrectly, which would be worse. I guess what we need to do is to add some additional diagnostics around that message, so that there would at least be an indication in the error console of exactly what is going on.
I had seen this as well, per previous converation with rkent.  I didn't come up clear steps, simply failed some times and others not using the same set of steps
See Also: → 551707
Blocks: 551707
The problem is that the filter is NOT detecting a real problem. I can do something as simple as scroll the filter window a few lines and it will magically accept it. I've examined my 'msgFilterRules.dat' in great detail with a binary editor and there are no bogus characters in the file. Just to make absolutely sure, I used:

tr '\000-\011\013-\037\177-\377' | unix2dos

 to strip out any non-printing characters and then compared the result to the original. They matched.

This is with SeaMonkey 2.0.13 on XP_SP3
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.18)
Is this still valid? In the current state of the FilterEditor.js file I see there are reportError() calls before the message to the user is shown. Is that not enough? What more needs to be added?
This error is coming from the code at http://hg.mozilla.org/releases/comm-aurora/annotate/78caca3ca3fb/mailnews/base/search/content/FilterEditor.js#l344

When you save the filter, the filter is searching through the validity table to make sure that the searches are all valid for the filter context. So it is complaining about a specific term in the table, but the error message gives no report what that term is.

So the request is to include some sort of information about the failed term in the error message. It might be sufficient to simply report something to the error console about the problematic term.

Another alternative might be to save the filter anyway (or give a choice), then warn about the invalid term - once again perhaps with a message to the error console.
(In reply to Kent James (:rkent) from comment #4)
> So the request is to include some sort of information about the failed term
> in the error message. It might be sufficient to simply report something to
> the error console about the problematic term.
Yes, and as I said, there are already calls like: Components.utils.reportError("filter not saved because standard search term not available");
when a rule fails a check. So there are messages in the error console.
But none of them seems to indicate which of the rules has the problem. If that is what needs to be done here I can take it.
Assignee: nobody → acelists
(In reply to :aceman from comment #5)

> But none of them seems to indicate which of the rules has the problem. If
> that is what needs to be done here I can take it.

Yes that is what is needed.
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
Something like this?
Attachment #645473 - Flags: review?(kent)
Comment on attachment 645473 [details] [diff] [review]
patch

Could you assemble a string containing the search term name and operator (localized) and include that in the on-screen error message, instead of just a number?
Attached patch patch v2 (obsolete) — Splinter Review
Like this?
Attachment #645473 - Attachment is obsolete: true
Attachment #645473 - Flags: review?(kent)
Attachment #645517 - Flags: review?(kent)
Comment on attachment 645517 [details] [diff] [review]
patch v2

OK this looks good to me. Thanks!

BTW to see this error, do this:

1) Create a filter with Apply Filter When manually run
2) Add a search term Junk Status is Junk
3) Change to Apply filter when Checking Mail
4) OK to save the filter.

You will see the error message.
Attachment #645517 - Flags: review?(kent) → feedback+
Attachment #645517 - Flags: review?(neil)
Attachment #645517 - Flags: review?(mconley)
Comment on attachment 645517 [details] [diff] [review]
patch v2

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

Looks OK to me. Thanks aceman!
Attachment #645517 - Flags: review?(mconley) → review+
Attachment #645517 - Flags: review?(jh)
Comment on attachment 645517 [details] [diff] [review]
patch v2

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

Works well, thanks from me, too! Found some very minor nits. You may keep them, unless Neil is opposed.

::: mailnews/base/search/content/FilterEditor.js
@@ +296,5 @@
>    }
>  
>    // Check that all of the search attributes and operators are valid.
> +  let invalidRule = -1;
> +  for (let index = 0; (index < gSearchTerms.length) && (invalidRule == -1); index++)

Technically, the additional parentheses is completely unnecessary, and it's readable without, too. Don't know TB's coding guidelines by heart, though.

@@ +315,5 @@
>        if (!customTerm)
> +      {
> +        invalidRule = index;
> +        Components.utils.reportError("Filter not saved because custom search term ID " +
> +                                     obj.searchattribute.value + " in rule " + index + " not found.");

Here and below: Adding a full stop suggests this is now a sentence, but it isn't. If you really want to keep it short I'd replace the " because" by a colon and remove the full stop.
Attachment #645517 - Flags: review?(jh) → feedback+
Attached patch patch v3 (obsolete) — Splinter Review
Done.
Attachment #645517 - Attachment is obsolete: true
Attachment #645517 - Flags: review?(neil)
Attachment #660527 - Flags: ui-review?(bwinton)
Attachment #660527 - Flags: review?(iann_bugzilla)
Comment on attachment 660527 [details] [diff] [review]
patch v3

(As a side note, this patch didn't apply on the latest trunk, but was easy enough to fix.)

>+++ b/mail/locales/en-US/chrome/messenger/filter.properties
>@@ -18,17 +18,20 @@ invalidCustomHeader=One of your filters 
>-searchTermsInvalidMessage=This filter cannot be saved because some search terms are invalid in the current context.
>+# LOCALIZATION NOTE(searchTermsInvalidRule)
>+# %1$S=search attribute name from the bad rule
>+# %2$S=search operator from the bad rule
>+searchTermsInvalidRule=This filter cannot be saved because the search term "%1$S %2$S" is invalid in the current context.

Maybe say "from the invalid rule" instead of "from the bad rule"?

But other than that nit (and that's really a review-nit, not a ui-review-nit), ui-r=me!

Thanks,
Blake.
Attachment #660527 - Flags: ui-review?(bwinton) → ui-review+
Attached patch patch v4 (obsolete) — Splinter Review
Sure, thanks.
Attachment #660527 - Attachment is obsolete: true
Attachment #660527 - Flags: review?(iann_bugzilla)
Attachment #660934 - Flags: review?(iann_bugzilla)
Attached patch patch v5 (obsolete) — Splinter Review
Attachment #660934 - Attachment is obsolete: true
Attachment #660934 - Flags: review?(iann_bugzilla)
Attachment #661502 - Flags: review?(iann_bugzilla)
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #661502 - Attachment is obsolete: true
Attachment #661502 - Flags: review?(iann_bugzilla)
Attachment #662263 - Flags: review?(iann_bugzilla)
Blocks: 775665
Comment on attachment 662263 [details] [diff] [review]
patch v6

>+  let invalidRule = -1;
>+  for (let index = 0; index < gSearchTerms.length && invalidRule == -1; index++)
My preference is to use while rather than for here.

>+        Components.utils.reportError("Filter not saved because custom search term ID " +
>+                                     obj.searchattribute.value + " in rule " + index + " not found");
>+          Components.utils.reportError("Filter not saved because custom search term " +
>+                                       customTerm.name + " in rule " + index + " not available");
>+        Components.utils.reportError("Filter not saved because standard search term " +
>+                                     attribValue + " in rule " + index + " not available");
Could the rule name rather than index be used? or maybe both?
Attachment #662263 - Flags: review?(iann_bugzilla) → review+
Ian, what is the rule name? obj.searchattribute.label + obj.searchoperator.label as is shown to the user in the prompt below this?
(In reply to :aceman from comment #19)
> Ian, what is the rule name? obj.searchattribute.label +
> obj.searchoperator.label as is shown to the user in the prompt below this?

Hmmm, yes, just looking at the best way to give as much relevant information in the diagnostics. Thinking about it more, should it be index + 1 seeing users usually start counting at 1 whereas the first rule is zero internally?
Attached patch patch v7Splinter Review
OK, what about this?
Attachment #662263 - Attachment is obsolete: true
Attachment #664208 - Flags: review?(iann_bugzilla)
Comment on attachment 664208 [details] [diff] [review]
patch v7

Looks good to me, would any tests / changes to tests be required for TB?
Attachment #664208 - Flags: review?(iann_bugzilla) → review+
Fortunately mconley didn't request any.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/ba5cfbb941d6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
You need to log in before you can comment on or make changes to this bug.